[PATCH 0/5] Use skip_blank_lines() for more commit messages

[PATCH 0/5] Use skip_blank_lines() for more commit messages

[Johannes Schindelin]

These patches are on top of js/find-commit-subject-ignore-leading-blanks
(whether you want to put them into the same topic branch before merging
or put them into their own add-on topic branch does not really matter to
me).

It occurred to me that there might be more code locations where finding
the commit object's body is hand-rolled, and could benefit from using
the skip_blank_lines() function to handle not-quite-standard but
still-allowed objects from being processed correctly.

I found a couple of instances, and made them consistent with the
pretty-printing machinery.

I made fine-grained commits to allow for dropping contentious patches
more easily.

There are four more instances which I did *not* change:

- fast-export: I designed this command to be as faithful in exporting
  the commits as possible. As such, even non-standard commit objects
  should be kept as-are.

- fmt-merge-msg: when showing signed commit messages, it is better to
  show the commit messages verbatim rather than trimming them.

- when signing a commit, it is better to use the original commit message
  than a munged one.

- notes' messages are left alone. I do not know whether there might be
  an intentional use of leading blank lines, but I wanted to err on the
  safe side.

I also considered treating tag messages the same way, but tags are much
more likely to be signed, and I did not want to risk munging the tag
messages just before signing them.


Johannes Schindelin (5):
  commit -C: skip blank lines at the beginning of the message
  sequencer: use skip_blank_lines() to find the commit subject
  reset --hard: skip blank lines when reporting the commit subject
  commit -S: avoid invalid pointer with empty message
  Skip blank lines when matching <commit>^{/foo}

 builtin/commit.c | 2 +-
 builtin/reset.c  | 2 +-
 commit.c         | 6 +++++-
 sequencer.c      | 6 ++----
 sha1_name.c      | 3 ++-
 5 files changed, 11 insertions(+), 8 deletions(-)

Published-As: https://github.com/dscho/git/releases/tag/more-blank-line-skipping-v1
-- 
2.9.0.270.g810e421

base-commit: 38a2ea1900489a76d9840002bf60bd7ff29f07cd

[PATCH 1/5] commit -C: skip blank lines at the beginning of the message

[Johannes Schindelin]

Consistent with the pretty-printing machinery, we skip leading blank
lines (if any) of existing commit messages.

While Git itself only produces commit objects with a single empty line
between commit header and commit message, it is legal to have more than
one blank line (i.e. lines containing only white space, or no
characters) at the beginning of the commit message, and the
pretty-printing code already handles that.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/commit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 3f18942..1f6dbcd 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -715,7 +715,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		char *buffer;
 		buffer = strstr(use_message_buffer, "\n\n");
 		if (buffer)
-			strbuf_addstr(&sb, buffer + 2);
+			strbuf_addstr(&sb, skip_blank_lines(buffer + 2));
 		hook_arg1 = "commit";
 		hook_arg2 = use_message;
 	} else if (fixup_message) {
-- 
2.9.0.270.g810e421

[PATCH 2/5] sequencer: use skip_blank_lines() to find the commit subject

[Johannes Schindelin]

Just like we already taught the find_commit_subject() function (to make
it consistent with the code in pretty.c), we now simply skip leading
blank lines of the commit message.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 7d7add6..cdfac82 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -544,10 +544,8 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 		 * information followed by "\n\n".
 		 */
 		p = strstr(msg.message, "\n\n");
-		if (p) {
-			p += 2;
-			strbuf_addstr(&msgbuf, p);
-		}
+		if (p)
+			strbuf_addstr(&msgbuf, skip_blank_lines(p + 2));
 
 		if (opts->record_origin) {
 			if (!has_conforming_footer(&msgbuf, NULL, 0))
-- 
2.9.0.270.g810e421

[PATCH 3/5] reset --hard: skip blank lines when reporting the commit subject

[Johannes Schindelin]

When there are blank lines at the beginning of a commit message, the
pretty printing machinery already skips them when showing a commit
subject (or the complete commit message). We shall henceforth do the
same when reporting the commit subject after the user called

	git reset --hard <commit>

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/reset.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/reset.c b/builtin/reset.c
index acd6278..5c6206b 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -103,7 +103,7 @@ static void print_new_head_line(struct commit *commit)
 	if (body) {
 		const char *eol;
 		size_t len;
-		body += 2;
+		body = skip_blank_lines(body + 2);
 		eol = strchr(body, '\n');
 		len = eol ? eol - body : strlen(body);
 		printf(" %.*s\n", (int) len, body);
-- 
2.9.0.270.g810e421

[PATCH 4/5] commit -S: avoid invalid pointer with empty message

[Johannes Schindelin]

While it is not recommended, `git fsck` says that:

	Not having a body is not a crime [...]

... which means that we cannot assume that the commit buffer contains an
empty line to separate header from body (essentially saying that there
is only a header).

So let's tread carefully here.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 commit.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/commit.c b/commit.c
index 24d4715..0bb51a2 100644
--- a/commit.c
+++ b/commit.c
@@ -1090,11 +1090,15 @@ static const int gpg_sig_header_len = sizeof(gpg_sig_header) - 1;
 
 static int do_sign_commit(struct strbuf *buf, const char *keyid)
 {
+	const char *eoh = strstr(buf->buf, "\n\n");
 	struct strbuf sig = STRBUF_INIT;
 	int inspos, copypos;
 
 	/* find the end of the header */
-	inspos = strstr(buf->buf, "\n\n") - buf->buf + 1;
+	if (!eoh)
+		inspos = buf->len;
+	else
+		inspos = eoh - buf->buf + 1;
 
 	if (!keyid || !*keyid)
 		keyid = get_signing_key();
-- 
2.9.0.270.g810e421

[PATCH 5/5] Skip blank lines when matching <commit>^{/foo}

[Johannes Schindelin]

When trying to match a pattern in the commit subject, simply skip leading
blank lines in the commit message. This is consistent with the
pretty-printing machinery: it silently ignores leading blank lines in the
commit object's body.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sha1_name.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/sha1_name.c b/sha1_name.c
index ca7ddd6..da354a5 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -912,7 +912,8 @@ static int get_sha1_oneline(const char *prefix, unsigned char *sha1,
 			continue;
 		buf = get_commit_buffer(commit, NULL);
 		p = strstr(buf, "\n\n");
-		matches = negative ^ (p && !regexec(&regex, p + 2, 0, NULL, 0));
+		matches = negative ^ (p && !regexec(&regex,
+			skip_blank_lines(p + 2), 0, NULL, 0));
 		unuse_commit_buffer(commit, buf);
 
 		if (matches) {
-- 
2.9.0.270.g810e421

Re: [PATCH 0/5] Use skip_blank_lines() for more commit messages

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> It occurred to me that there might be more code locations where finding
> the commit object's body is hand-rolled, and could benefit from using
> the skip_blank_lines() function to handle not-quite-standard but
> still-allowed objects from being processed correctly.

Thanks for following up on this one.  We could have left them as
low-hanging-fruit for new people, but doing these ourselves while
we still remember is a good idea.

Re: [PATCH 1/5] commit -C: skip blank lines at the beginning of the message

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> Consistent with the pretty-printing machinery, we skip leading blank
> lines (if any) of existing commit messages.
>
> While Git itself only produces commit objects with a single empty line
> between commit header and commit message, it is legal to have more than
> one blank line (i.e. lines containing only white space, or no
> characters) at the beginning of the commit message, and the
> pretty-printing code already handles that.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  builtin/commit.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 3f18942..1f6dbcd 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -715,7 +715,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>  		char *buffer;
>  		buffer = strstr(use_message_buffer, "\n\n");
>  		if (buffer)
> -			strbuf_addstr(&sb, buffer + 2);
> +			strbuf_addstr(&sb, skip_blank_lines(buffer + 2));
>  		hook_arg1 = "commit";
>  		hook_arg2 = use_message;
>  	} else if (fixup_message) {

use_message_buffer is the contents of the commit object read
elsewhere, and strstr() skips the header part, so this follows
exactly the same pattern as the one you fixed earlier.  Good.

Re: [PATCH 2/5] sequencer: use skip_blank_lines() to find the commit subject

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> Just like we already taught the find_commit_subject() function (to make
> it consistent with the code in pretty.c), we now simply skip leading
> blank lines of the commit message.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  sequencer.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 7d7add6..cdfac82 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -544,10 +544,8 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
>  		 * information followed by "\n\n".
>  		 */
>  		p = strstr(msg.message, "\n\n");
> -		if (p) {
> -			p += 2;
> -			strbuf_addstr(&msgbuf, p);
> -		}
> +		if (p)
> +			strbuf_addstr(&msgbuf, skip_blank_lines(p + 2));
>  
>  		if (opts->record_origin) {
>  			if (!has_conforming_footer(&msgbuf, NULL, 0))

Here, msg.message was read in get_message() by calling
logmsg_reencode() that reads from get_commit_buffer().  The code
structure is exactly the same as the previous one.

Good.

Re: [PATCH 5/5] Skip blank lines when matching <commit>^{/foo}

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> When trying to match a pattern in the commit subject, simply skip leading
> blank lines in the commit message. This is consistent with the
> pretty-printing machinery: it silently ignores leading blank lines in the
> commit object's body.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---

Hmm.

I just tried

    $ git commit -s -m '  pull: fast-forward "pull --rebase=true"'

expecting that ":/^  pull:" would find it ;-)

The point of this patch is to make it not work, of course, which 
may or may not be a good thing.  I'd rather keep this separate from
the other fixes.

Also 4/5 is another different class of true bugfix, I would suspect,
so let's take it on a separate topic that can be merged down to
older maintainance tracks.

Thanks.

>  sha1_name.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/sha1_name.c b/sha1_name.c
> index ca7ddd6..da354a5 100644
> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -912,7 +912,8 @@ static int get_sha1_oneline(const char *prefix, unsigned char *sha1,
>  			continue;
>  		buf = get_commit_buffer(commit, NULL);
>  		p = strstr(buf, "\n\n");
> -		matches = negative ^ (p && !regexec(&regex, p + 2, 0, NULL, 0));
> +		matches = negative ^ (p && !regexec(&regex,
> +			skip_blank_lines(p + 2), 0, NULL, 0));
>  		unuse_commit_buffer(commit, buf);
>  
>  		if (matches) {

Re: [PATCH 5/5] Skip blank lines when matching <commit>^{/foo}

[Johannes Schindelin]

Hi Junio,

On Wed, 29 Jun 2016, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > When trying to match a pattern in the commit subject, simply skip leading
> > blank lines in the commit message. This is consistent with the
> > pretty-printing machinery: it silently ignores leading blank lines in the
> > commit object's body.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> 
> Hmm.
> 
> I just tried
> 
>     $ git commit -s -m '  pull: fast-forward "pull --rebase=true"'
> 
> expecting that ":/^  pull:" would find it ;-)

This patch would try to address a different concern, though:

	git write-object -w --stdin <<-EOF
	tree $(git rev-parse HEAD:)
	parent $(git rev-parse HEAD)
	author B O Gus <bo@gus.com> 1234567890 +0000
	committer F A Ke <fa@ke.org> 1234567890 +0000


	There are two blank lines before the subject
	EOF

It is not about leading whitespace in the commit subject, but about blank
lines before it.

Granted, our current tools do not generate such commit objects, but we
accept them, and already handle them in some places.

> Also 4/5 is another different class of true bugfix, I would suspect,
> so let's take it on a separate topic that can be merged down to
> older maintainance tracks.

Sure!

Thanks,
Dscho