[PATCH] Make find_commit_subject() more robust

[PATCH] Make find_commit_subject() more robust

[Johannes Schindelin]

Just like the pretty printing machinery, we should simply ignore empty
lines at the beginning of the commit messages.

This discrepancy was noticed when an early version of the rebase--helper
produced commit objects with more than one empty line between the header
and the commit message.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Published-As: https://github.com/dscho/git/releases/tag/leading-empty-lines-v1

	Aaaaand another patch from the rebase--helper front. I guess I'll
	call it a day with this one.

 commit.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/commit.c b/commit.c
index 3f4f371..7b00989 100644
--- a/commit.c
+++ b/commit.c
@@ -415,6 +415,8 @@ int find_commit_subject(const char *commit_buffer, const char **subject)
 		p++;
 	if (*p) {
 		p += 2;
+		while (*p == '\n')
+			p++;
 		for (eol = p; *eol && *eol != '\n'; eol++)
 			; /* do nothing */
 	} else
-- 
2.9.0.119.gb7b8d21

base-commit: 05219a1276341e72d8082d76b7f5ed394b7437a4

Re: [PATCH] Make find_commit_subject() more robust

[Junio C Hamano]

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

> Just like the pretty printing machinery, we should simply ignore empty
> lines at the beginning of the commit messages.
>
> This discrepancy was noticed when an early version of the rebase--helper
> produced commit objects with more than one empty line between the header
> and the commit message.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> Published-As: https://github.com/dscho/git/releases/tag/leading-empty-lines-v1
>
> 	Aaaaand another patch from the rebase--helper front. I guess I'll
> 	call it a day with this one.

Makes sense.  This has a trivial textual conflict with cleanup
patches in flight, I think, but that is not a big problem.  It does
hint that we might want to find a library function that can replace
a hand-rolled while loop we are adding here, though ;-)

Perhaps cast this new behaviour in stone by adding a test?

Thanks.

>  commit.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/commit.c b/commit.c
> index 3f4f371..7b00989 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -415,6 +415,8 @@ int find_commit_subject(const char *commit_buffer, const char **subject)
>  		p++;
>  	if (*p) {
>  		p += 2;
> +		while (*p == '\n')
> +			p++;
>  		for (eol = p; *eol && *eol != '\n'; eol++)
>  			; /* do nothing */
>  	} else

Re: [PATCH] Make find_commit_subject() more robust

[Johannes Schindelin]

Hi Junio,

On Mon, 20 Jun 2016, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > Just like the pretty printing machinery, we should simply ignore empty
> > lines at the beginning of the commit messages.
> >
> > This discrepancy was noticed when an early version of the rebase--helper
> > produced commit objects with more than one empty line between the header
> > and the commit message.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> > Published-As: https://github.com/dscho/git/releases/tag/leading-empty-lines-v1
> >
> > 	Aaaaand another patch from the rebase--helper front. I guess I'll
> > 	call it a day with this one.
> 
> Makes sense.  This has a trivial textual conflict with cleanup
> patches in flight, I think, but that is not a big problem.

I will gladly resend rebased to `next`, if you wish.

> It does hint that we might want to find a library function that can
> replace a hand-rolled while loop we are adding here, though ;-)

Heh. I cannot help you with that ;-)

> Perhaps cast this new behaviour in stone by adding a test?

Will do.

Ciao,
Dscho

Re: [PATCH] Make find_commit_subject() more robust

[Junio C Hamano]

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

> Hi Junio,
>
> On Mon, 20 Jun 2016, Junio C Hamano wrote:
>
>> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>> 
>> > Just like the pretty printing machinery, we should simply ignore empty
>> > lines at the beginning of the commit messages.
>> >
>> > This discrepancy was noticed when an early version of the rebase--helper
>> > produced commit objects with more than one empty line between the header
>> > and the commit message.
>> >
>> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>> > ---
>> > Published-As: https://github.com/dscho/git/releases/tag/leading-empty-lines-v1
>> >
>> > 	Aaaaand another patch from the rebase--helper front. I guess I'll
>> > 	call it a day with this one.
>> 
>> Makes sense.  This has a trivial textual conflict with cleanup
>> patches in flight, I think, but that is not a big problem.
>
> I will gladly resend rebased to `next`, if you wish.

No, I'd prefer a patch that applies to 'master' for a new feature;
there is no need to deliberately get taken hostage by other topics.

>> It does hint that we might want to find a library function that can
>> replace a hand-rolled while loop we are adding here, though ;-)
>
> Heh. I cannot help you with that ;-)

The reason it hints such a thing is because the line nearby that
does this:

  		for (eol = p; *eol && *eol != '\n'; eol++)
  			; /* do nothing */

gets rewritten to

		eol = strchrnul(p, '\n');

i.e. "give me the pointer to the first byte that is '\n', or EOS".

Your patch introduces a similar loop with similar (but different)
purpose:

		while (*p == '\n')
			p++;

which would have been helped if there were a helper with an
opposite function, i.e.

		p = strcchrnul(p, '\n');

i.e. "give me the pointer to the first byte that is not '\n', or EOS".

But there is no such thing.  Although p += strcspn(p, "\n") is a
possibility, that somehow feels a bit odd.  And that is why I did
not hint any existing function and said "might want to find".

HOWEVER.

Stepping back a bit, I think what we actually want is

		p = skip_blank_lines(p);

that skips any and all blank lines, including an empty line that
consists of all whitespace.

For example

	(
		# grab the header lines
		git cat-file commit HEAD | sed -e '/^$/q'
                # throw in random blank lines
		echo
                echo " "
                echo "  "
                echo "   "
                echo
                echo "Title line"
	) | git hash-object -w -t commit --stdin

would mint a commit object that has many blank lines in the front,
some have whitespace and are not empty.  If you give it to

	git show -s | cat -e
        git show -s --oneline | cat -e

I think you would see what I mean.

Re: [PATCH] Make find_commit_subject() more robust

[Johannes Schindelin]

Hi Junio,

On Tue, 21 Jun 2016, Junio C Hamano wrote:

> Your patch introduces a similar loop with similar (but different)
> purpose:
> 
> 		while (*p == '\n')
> 			p++;
> 
> which would have been helped if there were a helper with an
> opposite function, i.e.
> 
> 		p = strcchrnul(p, '\n');
> 
> i.e. "give me the pointer to the first byte that is not '\n', or EOS".
> 
> But there is no such thing.  Although p += strcspn(p, "\n") is a
> possibility, that somehow feels a bit odd.  And that is why I did
> not hint any existing function and said "might want to find".

Sure. And strcspn() is less efficient than the loop if you already know
that the second parameter contains only a single character.

> HOWEVER.
> 
> Stepping back a bit, I think what we actually want is
> 
> 		p = skip_blank_lines(p);
> 
> that skips any and all blank lines, including an empty line that
> consists of all whitespace.

My original aim was to make find_commit_subject() consistent with the
pretty-printing machinery. I failed to realize that skip_blank_lines()
does more than skipping empty lines, so let me re-roll the patch.

Ciao,
Dscho

[PATCH v2] Make find_commit_subject() more robust

[Johannes Schindelin]

Just like the pretty printing machinery, we should simply ignore empty
lines at the beginning of the commit messages.

This discrepancy was noticed when an early version of the rebase--helper
produced commit objects with more than one empty line between the header
and the commit message.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Published-As: https://github.com/dscho/git/releases/tag/leading-empty-lines-v2

	git blame seemed to be the most accessible user of
	find_commit_subject()...

 commit.c                 |  2 ++
 t/t8008-blame-formats.sh | 17 +++++++++++++++++
 2 files changed, 19 insertions(+)
Interdiff vs v1:

 diff --git a/t/t8008-blame-formats.sh b/t/t8008-blame-formats.sh
 index 29f84a6..03bd313 100755
 --- a/t/t8008-blame-formats.sh
 +++ b/t/t8008-blame-formats.sh
 @@ -87,4 +87,21 @@ test_expect_success 'blame --line-porcelain output' '
  	test_cmp expect actual
  '
  
 +test_expect_success '--porcelain detects first non-empty line as subject' '
 +	(
 +		GIT_INDEX_FILE=.git/tmp-index &&
 +		export GIT_INDEX_FILE &&
 +		echo "This is it" >single-file &&
 +		git add single-file &&
 +		tree=$(git write-tree) &&
 +		commit=$(printf "%s\n%s\n%s\n\n\noneline\n\nbody\n" \
 +			"tree $tree" \
 +			"author A <a@b.c> 123456789 +0000" \
 +			"committer C <c@d.e> 123456789 +0000" |
 +		git hash-object -w -t commit --stdin) &&
 +		git blame --porcelain $commit -- single-file >output &&
 +		grep "^summary oneline$" output
 +	)
 +'
 +
  test_done


diff --git a/commit.c b/commit.c
index 3f4f371..7b00989 100644
--- a/commit.c
+++ b/commit.c
@@ -415,6 +415,8 @@ int find_commit_subject(const char *commit_buffer, const char **subject)
 		p++;
 	if (*p) {
 		p += 2;
+		while (*p == '\n')
+			p++;
 		for (eol = p; *eol && *eol != '\n'; eol++)
 			; /* do nothing */
 	} else
diff --git a/t/t8008-blame-formats.sh b/t/t8008-blame-formats.sh
index 29f84a6..03bd313 100755
--- a/t/t8008-blame-formats.sh
+++ b/t/t8008-blame-formats.sh
@@ -87,4 +87,21 @@ test_expect_success 'blame --line-porcelain output' '
 	test_cmp expect actual
 '
 
+test_expect_success '--porcelain detects first non-empty line as subject' '
+	(
+		GIT_INDEX_FILE=.git/tmp-index &&
+		export GIT_INDEX_FILE &&
+		echo "This is it" >single-file &&
+		git add single-file &&
+		tree=$(git write-tree) &&
+		commit=$(printf "%s\n%s\n%s\n\n\noneline\n\nbody\n" \
+			"tree $tree" \
+			"author A <a@b.c> 123456789 +0000" \
+			"committer C <c@d.e> 123456789 +0000" |
+		git hash-object -w -t commit --stdin) &&
+		git blame --porcelain $commit -- single-file >output &&
+		grep "^summary oneline$" output
+	)
+'
+
 test_done
-- 
2.9.0.118.g0e1a633

base-commit: ab7797dbe95fff38d9265869ea367020046db118

Re: [PATCH v2] Make find_commit_subject() more robust

[Junio C Hamano]

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

> Just like the pretty printing machinery, we should simply ignore empty
> lines at the beginning of the commit messages.

Thanks.

>
> This discrepancy was noticed when an early version of the rebase--helper
> produced commit objects with more than one empty line between the header
> and the commit message.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> Published-As: https://github.com/dscho/git/releases/tag/leading-empty-lines-v2
>
> 	git blame seemed to be the most accessible user of
> 	find_commit_subject()...
>
>  commit.c                 |  2 ++
>  t/t8008-blame-formats.sh | 17 +++++++++++++++++
>  2 files changed, 19 insertions(+)
> Interdiff vs v1:
>
>  diff --git a/t/t8008-blame-formats.sh b/t/t8008-blame-formats.sh
>  index 29f84a6..03bd313 100755
>  --- a/t/t8008-blame-formats.sh
>  +++ b/t/t8008-blame-formats.sh
>  @@ -87,4 +87,21 @@ test_expect_success 'blame --line-porcelain output' '
>   	test_cmp expect actual
>   '
>   
>  +test_expect_success '--porcelain detects first non-empty line as subject' '
>  +	(
>  +		GIT_INDEX_FILE=.git/tmp-index &&
>  +		export GIT_INDEX_FILE &&
>  +		echo "This is it" >single-file &&
>  +		git add single-file &&
>  +		tree=$(git write-tree) &&
>  +		commit=$(printf "%s\n%s\n%s\n\n\noneline\n\nbody\n" \
>  +			"tree $tree" \
>  +			"author A <a@b.c> 123456789 +0000" \
>  +			"committer C <c@d.e> 123456789 +0000" |
>  +		git hash-object -w -t commit --stdin) &&
>  +		git blame --porcelain $commit -- single-file >output &&
>  +		grep "^summary oneline$" output
>  +	)
>  +'
>  +
>   test_done
>
>
> diff --git a/commit.c b/commit.c
> index 3f4f371..7b00989 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -415,6 +415,8 @@ int find_commit_subject(const char *commit_buffer, const char **subject)
>  		p++;
>  	if (*p) {
>  		p += 2;
> +		while (*p == '\n')
> +			p++;
>  		for (eol = p; *eol && *eol != '\n'; eol++)
>  			; /* do nothing */
>  	} else
> diff --git a/t/t8008-blame-formats.sh b/t/t8008-blame-formats.sh
> index 29f84a6..03bd313 100755
> --- a/t/t8008-blame-formats.sh
> +++ b/t/t8008-blame-formats.sh
> @@ -87,4 +87,21 @@ test_expect_success 'blame --line-porcelain output' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success '--porcelain detects first non-empty line as subject' '
> +	(
> +		GIT_INDEX_FILE=.git/tmp-index &&
> +		export GIT_INDEX_FILE &&
> +		echo "This is it" >single-file &&
> +		git add single-file &&
> +		tree=$(git write-tree) &&
> +		commit=$(printf "%s\n%s\n%s\n\n\noneline\n\nbody\n" \
> +			"tree $tree" \
> +			"author A <a@b.c> 123456789 +0000" \
> +			"committer C <c@d.e> 123456789 +0000" |
> +		git hash-object -w -t commit --stdin) &&
> +		git blame --porcelain $commit -- single-file >output &&
> +		grep "^summary oneline$" output
> +	)
> +'
> +
>  test_done

[PATCH v3 0/2] Make find_commit_subject() consistent with --format=%s

[Johannes Schindelin]

In an intermediate iteration of my rebase--helper patches, I
accidentally generated commits with more than one empty line
between the header and the commit message. When using
find_commit_subject() to show the oneline, it turned up empty, even
if the output of `git show --format=%s` looked fine.

Turned out that the pretty-printing machinery helpfully skipped any
blank lines before the commit message.

In the first iteration of this patch, I failed to notice that
the skip_empty_lines() function used by the pretty printing (which is
declared static, and therefore I originally did not use it in order to
keep the patch as minimal as possible) skips also blank lines.

To make things truly consistent, I now just make the skip_empty_lines()
function public, and then use it.


Johannes Schindelin (2):
  Make the skip_empty_lines() function public
  Make find_commit_subject() more robust

 commit.c                 |  2 +-
 commit.h                 |  1 +
 pretty.c                 |  2 +-
 t/t8008-blame-formats.sh | 17 +++++++++++++++++
 4 files changed, 20 insertions(+), 2 deletions(-)

Published-As: https://github.com/dscho/git/releases/tag/leading-empty-lines-v3
Interdiff vs v2:

 diff --git a/commit.c b/commit.c
 index 7b00989..0bf868f 100644
 --- a/commit.c
 +++ b/commit.c
 @@ -414,9 +414,7 @@ int find_commit_subject(const char *commit_buffer, const char **subject)
  	while (*p && (*p != '\n' || p[1] != '\n'))
  		p++;
  	if (*p) {
 -		p += 2;
 -		while (*p == '\n')
 -			p++;
 +		p = skip_empty_lines(p + 2);
  		for (eol = p; *eol && *eol != '\n'; eol++)
  			; /* do nothing */
  	} else
 diff --git a/commit.h b/commit.h
 index b06db4d..fbdd18d 100644
 --- a/commit.h
 +++ b/commit.h
 @@ -177,6 +177,7 @@ extern const char *format_subject(struct strbuf *sb, const char *msg,
  				  const char *line_separator);
  extern void userformat_find_requirements(const char *fmt, struct userformat_want *w);
  extern int commit_format_is_empty(enum cmit_fmt);
 +extern const char *skip_empty_lines(const char *msg);
  extern void format_commit_message(const struct commit *commit,
  				  const char *format, struct strbuf *sb,
  				  const struct pretty_print_context *context);
 diff --git a/pretty.c b/pretty.c
 index c3ec430..1b807b4 100644
 --- a/pretty.c
 +++ b/pretty.c
 @@ -516,7 +516,7 @@ static int is_empty_line(const char *line, int *len_p)
  	return !len;
  }
  
 -static const char *skip_empty_lines(const char *msg)
 +const char *skip_empty_lines(const char *msg)
  {
  	for (;;) {
  		int linelen = get_one_line(msg);
 diff --git a/t/t8008-blame-formats.sh b/t/t8008-blame-formats.sh
 index 03bd313..b98f9a4 100755
 --- a/t/t8008-blame-formats.sh
 +++ b/t/t8008-blame-formats.sh
 @@ -94,7 +94,7 @@ test_expect_success '--porcelain detects first non-empty line as subject' '
  		echo "This is it" >single-file &&
  		git add single-file &&
  		tree=$(git write-tree) &&
 -		commit=$(printf "%s\n%s\n%s\n\n\noneline\n\nbody\n" \
 +		commit=$(printf "%s\n%s\n%s\n\n\n  \noneline\n\nbody\n" \
  			"tree $tree" \
  			"author A <a@b.c> 123456789 +0000" \
  			"committer C <c@d.e> 123456789 +0000" |

-- 
2.9.0.118.g0e1a633

base-commit: ab7797dbe95fff38d9265869ea367020046db118

[PATCH v3 1/2] Make the skip_empty_lines() function public

[Johannes Schindelin]

This function will be used also in the find_commit_subject() function.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 commit.h | 1 +
 pretty.c | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/commit.h b/commit.h
index b06db4d..fbdd18d 100644
--- a/commit.h
+++ b/commit.h
@@ -177,6 +177,7 @@ extern const char *format_subject(struct strbuf *sb, const char *msg,
 				  const char *line_separator);
 extern void userformat_find_requirements(const char *fmt, struct userformat_want *w);
 extern int commit_format_is_empty(enum cmit_fmt);
+extern const char *skip_empty_lines(const char *msg);
 extern void format_commit_message(const struct commit *commit,
 				  const char *format, struct strbuf *sb,
 				  const struct pretty_print_context *context);
diff --git a/pretty.c b/pretty.c
index c3ec430..1b807b4 100644
--- a/pretty.c
+++ b/pretty.c
@@ -516,7 +516,7 @@ static int is_empty_line(const char *line, int *len_p)
 	return !len;
 }
 
-static const char *skip_empty_lines(const char *msg)
+const char *skip_empty_lines(const char *msg)
 {
 	for (;;) {
 		int linelen = get_one_line(msg);
-- 
2.9.0.118.g0e1a633

[PATCH v3 2/2] Make find_commit_subject() more robust

[Johannes Schindelin]

Just like the pretty printing machinery, we should simply ignore blank
lines at the beginning of the commit messages.

This discrepancy was noticed when an early version of the rebase--helper
produced commit objects with more than one empty line between the header
and the commit message.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 commit.c                 |  2 +-
 t/t8008-blame-formats.sh | 17 +++++++++++++++++
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/commit.c b/commit.c
index 3f4f371..0bf868f 100644
--- a/commit.c
+++ b/commit.c
@@ -414,7 +414,7 @@ int find_commit_subject(const char *commit_buffer, const char **subject)
 	while (*p && (*p != '\n' || p[1] != '\n'))
 		p++;
 	if (*p) {
-		p += 2;
+		p = skip_empty_lines(p + 2);
 		for (eol = p; *eol && *eol != '\n'; eol++)
 			; /* do nothing */
 	} else
diff --git a/t/t8008-blame-formats.sh b/t/t8008-blame-formats.sh
index 29f84a6..b98f9a4 100755
--- a/t/t8008-blame-formats.sh
+++ b/t/t8008-blame-formats.sh
@@ -87,4 +87,21 @@ test_expect_success 'blame --line-porcelain output' '
 	test_cmp expect actual
 '
 
+test_expect_success '--porcelain detects first non-empty line as subject' '
+	(
+		GIT_INDEX_FILE=.git/tmp-index &&
+		export GIT_INDEX_FILE &&
+		echo "This is it" >single-file &&
+		git add single-file &&
+		tree=$(git write-tree) &&
+		commit=$(printf "%s\n%s\n%s\n\n\n  \noneline\n\nbody\n" \
+			"tree $tree" \
+			"author A <a@b.c> 123456789 +0000" \
+			"committer C <c@d.e> 123456789 +0000" |
+		git hash-object -w -t commit --stdin) &&
+		git blame --porcelain $commit -- single-file >output &&
+		grep "^summary oneline$" output
+	)
+'
+
 test_done
-- 
2.9.0.118.g0e1a633

Re: [PATCH v3 0/2] Make find_commit_subject() consistent with --format=%s

[Junio C Hamano]

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

> In an intermediate iteration of my rebase--helper patches, I
> accidentally generated commits with more than one empty line
> between the header and the commit message. When using
> find_commit_subject() to show the oneline, it turned up empty, even
> if the output of `git show --format=%s` looked fine.

Much easier to read with s/this developer/I/g ;-)

> Turned out that the pretty-printing machinery helpfully skipped any
> blank lines before the commit message.
>
> In the first iteration of this patch, I failed to notice that
> the skip_empty_lines() function used by the pretty printing (which is
> declared static, and therefore I originally did not use it in order to
> keep the patch as minimal as possible) skips also blank lines.

By the way, I think skip_empty_lines() is misnamed, and I think your
use of "blank lines" in the previous paragraph indicates that you
agree ;-) It probably was OK back when it was a file-local static
helper in pretty.c, but it becomes a part of the global API with
1/2, renaming it to skip_blank_lines() may be a good thing to do
there at the same time.

I could do the tweaking while queuing if you too think it should
happen; that way we'd save one roundtrip ;-).

> To make things truly consistent, I now just make the skip_empty_lines()
> function public, and then use it.

Thanks.  Interdiff looks sensible.

Re: [PATCH v3 0/2] Make find_commit_subject() consistent with --format=%s

[Johannes Schindelin]

Hi Junio,

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

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > In an intermediate iteration of my rebase--helper patches, I
> > accidentally generated commits with more than one empty line
> > between the header and the commit message. When using
> > find_commit_subject() to show the oneline, it turned up empty, even
> > if the output of `git show --format=%s` looked fine.
> 
> Much easier to read with s/this developer/I/g ;-)

:-P

> > Turned out that the pretty-printing machinery helpfully skipped any
> > blank lines before the commit message.
> >
> > In the first iteration of this patch, I failed to notice that
> > the skip_empty_lines() function used by the pretty printing (which is
> > declared static, and therefore I originally did not use it in order to
> > keep the patch as minimal as possible) skips also blank lines.
> 
> By the way, I think skip_empty_lines() is misnamed, and I think your
> use of "blank lines" in the previous paragraph indicates that you
> agree ;-) It probably was OK back when it was a file-local static
> helper in pretty.c, but it becomes a part of the global API with
> 1/2, renaming it to skip_blank_lines() may be a good thing to do
> there at the same time.
> 
> I could do the tweaking while queuing if you too think it should
> happen; that way we'd save one roundtrip ;-).

I just sent another iteration.

Ciao,
Dscho

[PATCH v4 0/2] Make find_commit_subject() consistent with --format=%s

[Johannes Schindelin]

In an intermediate iteration of my rebase--helper patches, I
accidentally generated commits with more than one empty line
between the header and the commit message. When using
find_commit_subject() to show the oneline, it turned up empty, even
if the output of `git show --format=%s` looked fine.

Turned out that the pretty-printing machinery helpfully skipped any
blank lines before the commit message.

I simply make pretty.c's skip_empty_lines() public (now appropriately
named skip_blank_lines()) to make things consistent.


Johannes Schindelin (2):
  Make the skip_blank_lines() function public
  Make find_commit_subject() more robust

 commit.c                 |  2 +-
 commit.h                 |  1 +
 pretty.c                 | 16 ++++++++--------
 t/t8008-blame-formats.sh | 17 +++++++++++++++++
 4 files changed, 27 insertions(+), 9 deletions(-)

Published-As: https://github.com/dscho/git/releases/tag/leading-empty-lines-v4
Interdiff vs v3:

 diff --git a/commit.c b/commit.c
 index 0bf868f..24d4715 100644
 --- a/commit.c
 +++ b/commit.c
 @@ -414,7 +414,7 @@ int find_commit_subject(const char *commit_buffer, const char **subject)
  	while (*p && (*p != '\n' || p[1] != '\n'))
  		p++;
  	if (*p) {
 -		p = skip_empty_lines(p + 2);
 +		p = skip_blank_lines(p + 2);
  		for (eol = p; *eol && *eol != '\n'; eol++)
  			; /* do nothing */
  	} else
 diff --git a/commit.h b/commit.h
 index fbdd18d..5b78f83 100644
 --- a/commit.h
 +++ b/commit.h
 @@ -177,7 +177,7 @@ extern const char *format_subject(struct strbuf *sb, const char *msg,
  				  const char *line_separator);
  extern void userformat_find_requirements(const char *fmt, struct userformat_want *w);
  extern int commit_format_is_empty(enum cmit_fmt);
 -extern const char *skip_empty_lines(const char *msg);
 +extern const char *skip_blank_lines(const char *msg);
  extern void format_commit_message(const struct commit *commit,
  				  const char *format, struct strbuf *sb,
  				  const struct pretty_print_context *context);
 diff --git a/pretty.c b/pretty.c
 index 1b807b4..3b6bff7 100644
 --- a/pretty.c
 +++ b/pretty.c
 @@ -507,7 +507,7 @@ void pp_user_info(struct pretty_print_context *pp,
  	}
  }
  
 -static int is_empty_line(const char *line, int *len_p)
 +static int is_blank_line(const char *line, int *len_p)
  {
  	int len = *len_p;
  	while (len && isspace(line[len - 1]))
 @@ -516,14 +516,14 @@ static int is_empty_line(const char *line, int *len_p)
  	return !len;
  }
  
 -const char *skip_empty_lines(const char *msg)
 +const char *skip_blank_lines(const char *msg)
  {
  	for (;;) {
  		int linelen = get_one_line(msg);
  		int ll = linelen;
  		if (!linelen)
  			break;
 -		if (!is_empty_line(msg, &ll))
 +		if (!is_blank_line(msg, &ll))
  			break;
  		msg += linelen;
  	}
 @@ -875,7 +875,7 @@ const char *format_subject(struct strbuf *sb, const char *msg,
  		int linelen = get_one_line(line);
  
  		msg += linelen;
 -		if (!linelen || is_empty_line(line, &linelen))
 +		if (!linelen || is_blank_line(line, &linelen))
  			break;
  
  		if (!sb)
 @@ -894,11 +894,11 @@ static void parse_commit_message(struct format_commit_context *c)
  	const char *msg = c->message + c->message_off;
  	const char *start = c->message;
  
 -	msg = skip_empty_lines(msg);
 +	msg = skip_blank_lines(msg);
  	c->subject_off = msg - start;
  
  	msg = format_subject(NULL, msg, NULL);
 -	msg = skip_empty_lines(msg);
 +	msg = skip_blank_lines(msg);
  	c->body_off = msg - start;
  
  	c->commit_message_parsed = 1;
 @@ -1711,7 +1711,7 @@ void pp_remainder(struct pretty_print_context *pp,
  		if (!linelen)
  			break;
  
 -		if (is_empty_line(line, &linelen)) {
 +		if (is_blank_line(line, &linelen)) {
  			if (first)
  				continue;
  			if (pp->fmt == CMIT_FMT_SHORT)
 @@ -1782,7 +1782,7 @@ void pretty_print_commit(struct pretty_print_context *pp,
  	}
  
  	/* Skip excess blank lines at the beginning of body, if any... */
 -	msg = skip_empty_lines(msg);
 +	msg = skip_blank_lines(msg);
  
  	/* These formats treat the title line specially. */
  	if (pp->fmt == CMIT_FMT_ONELINE || pp->fmt == CMIT_FMT_EMAIL)
 diff --git a/t/t8008-blame-formats.sh b/t/t8008-blame-formats.sh
 index b98f9a4..92c8e79 100755
 --- a/t/t8008-blame-formats.sh
 +++ b/t/t8008-blame-formats.sh
 @@ -87,7 +87,7 @@ test_expect_success 'blame --line-porcelain output' '
  	test_cmp expect actual
  '
  
 -test_expect_success '--porcelain detects first non-empty line as subject' '
 +test_expect_success '--porcelain detects first non-blank line as subject' '
  	(
  		GIT_INDEX_FILE=.git/tmp-index &&
  		export GIT_INDEX_FILE &&

-- 
2.9.0.118.g0e1a633

base-commit: ab7797dbe95fff38d9265869ea367020046db118

[PATCH v4 1/2] Make the skip_blank_lines() function public

[Johannes Schindelin]

This function will be used also in the find_commit_subject() function.

While at it, rename the function to reflect that it skips not only
empty lines, but any lines consisting of only whitespace, too.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 commit.h |  1 +
 pretty.c | 16 ++++++++--------
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/commit.h b/commit.h
index b06db4d..5b78f83 100644
--- a/commit.h
+++ b/commit.h
@@ -177,6 +177,7 @@ extern const char *format_subject(struct strbuf *sb, const char *msg,
 				  const char *line_separator);
 extern void userformat_find_requirements(const char *fmt, struct userformat_want *w);
 extern int commit_format_is_empty(enum cmit_fmt);
+extern const char *skip_blank_lines(const char *msg);
 extern void format_commit_message(const struct commit *commit,
 				  const char *format, struct strbuf *sb,
 				  const struct pretty_print_context *context);
diff --git a/pretty.c b/pretty.c
index c3ec430..3b6bff7 100644
--- a/pretty.c
+++ b/pretty.c
@@ -507,7 +507,7 @@ void pp_user_info(struct pretty_print_context *pp,
 	}
 }
 
-static int is_empty_line(const char *line, int *len_p)
+static int is_blank_line(const char *line, int *len_p)
 {
 	int len = *len_p;
 	while (len && isspace(line[len - 1]))
@@ -516,14 +516,14 @@ static int is_empty_line(const char *line, int *len_p)
 	return !len;
 }
 
-static const char *skip_empty_lines(const char *msg)
+const char *skip_blank_lines(const char *msg)
 {
 	for (;;) {
 		int linelen = get_one_line(msg);
 		int ll = linelen;
 		if (!linelen)
 			break;
-		if (!is_empty_line(msg, &ll))
+		if (!is_blank_line(msg, &ll))
 			break;
 		msg += linelen;
 	}
@@ -875,7 +875,7 @@ const char *format_subject(struct strbuf *sb, const char *msg,
 		int linelen = get_one_line(line);
 
 		msg += linelen;
-		if (!linelen || is_empty_line(line, &linelen))
+		if (!linelen || is_blank_line(line, &linelen))
 			break;
 
 		if (!sb)
@@ -894,11 +894,11 @@ static void parse_commit_message(struct format_commit_context *c)
 	const char *msg = c->message + c->message_off;
 	const char *start = c->message;
 
-	msg = skip_empty_lines(msg);
+	msg = skip_blank_lines(msg);
 	c->subject_off = msg - start;
 
 	msg = format_subject(NULL, msg, NULL);
-	msg = skip_empty_lines(msg);
+	msg = skip_blank_lines(msg);
 	c->body_off = msg - start;
 
 	c->commit_message_parsed = 1;
@@ -1711,7 +1711,7 @@ void pp_remainder(struct pretty_print_context *pp,
 		if (!linelen)
 			break;
 
-		if (is_empty_line(line, &linelen)) {
+		if (is_blank_line(line, &linelen)) {
 			if (first)
 				continue;
 			if (pp->fmt == CMIT_FMT_SHORT)
@@ -1782,7 +1782,7 @@ void pretty_print_commit(struct pretty_print_context *pp,
 	}
 
 	/* Skip excess blank lines at the beginning of body, if any... */
-	msg = skip_empty_lines(msg);
+	msg = skip_blank_lines(msg);
 
 	/* These formats treat the title line specially. */
 	if (pp->fmt == CMIT_FMT_ONELINE || pp->fmt == CMIT_FMT_EMAIL)
-- 
2.9.0.118.g0e1a633

[PATCH v4 2/2] Make find_commit_subject() more robust

[Johannes Schindelin]

Just like the pretty printing machinery, we should simply ignore blank
lines at the beginning of the commit messages.

This discrepancy was noticed when an early version of the rebase--helper
produced commit objects with more than one empty line between the header
and the commit message.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 commit.c                 |  2 +-
 t/t8008-blame-formats.sh | 17 +++++++++++++++++
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/commit.c b/commit.c
index 3f4f371..24d4715 100644
--- a/commit.c
+++ b/commit.c
@@ -414,7 +414,7 @@ int find_commit_subject(const char *commit_buffer, const char **subject)
 	while (*p && (*p != '\n' || p[1] != '\n'))
 		p++;
 	if (*p) {
-		p += 2;
+		p = skip_blank_lines(p + 2);
 		for (eol = p; *eol && *eol != '\n'; eol++)
 			; /* do nothing */
 	} else
diff --git a/t/t8008-blame-formats.sh b/t/t8008-blame-formats.sh
index 29f84a6..92c8e79 100755
--- a/t/t8008-blame-formats.sh
+++ b/t/t8008-blame-formats.sh
@@ -87,4 +87,21 @@ test_expect_success 'blame --line-porcelain output' '
 	test_cmp expect actual
 '
 
+test_expect_success '--porcelain detects first non-blank line as subject' '
+	(
+		GIT_INDEX_FILE=.git/tmp-index &&
+		export GIT_INDEX_FILE &&
+		echo "This is it" >single-file &&
+		git add single-file &&
+		tree=$(git write-tree) &&
+		commit=$(printf "%s\n%s\n%s\n\n\n  \noneline\n\nbody\n" \
+			"tree $tree" \
+			"author A <a@b.c> 123456789 +0000" \
+			"committer C <c@d.e> 123456789 +0000" |
+		git hash-object -w -t commit --stdin) &&
+		git blame --porcelain $commit -- single-file >output &&
+		grep "^summary oneline$" output
+	)
+'
+
 test_done
-- 
2.9.0.118.g0e1a633