# [PATCH] Make find_commit_subject() more robust 15 messages from 2016-06-18 to 2016-06-22. Participants: Johannes Schindelin, Junio C Hamano. Thread: https://gitlist.dev/t/42653 ## Johannes Schindelin, 2016-06-18 13:12 Subject: [PATCH] Make find_commit_subject() more robust Message-ID: <34ef85eb4e2aef0b342ef5d3bce9e468c8339486.1466255489.git.johannes.schindelin@gmx.de> URL: https://gitlist.dev/e/34ef85eb4e2aef0b342ef5d3bce9e468c8339486.1466255489.git.johannes.schindelin%40gmx.de ``` 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 --- 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 ``` ## Junio C Hamano, 2016-06-20 19:35 Subject: Re: [PATCH] Make find_commit_subject() more robust Message-ID: URL: https://gitlist.dev/e/xmqqeg7ru00i.fsf%40gitster.mtv.corp.google.com In-Reply-To: <34ef85eb4e2aef0b342ef5d3bce9e468c8339486.1466255489.git.johannes.schindelin@gmx.de> ``` Johannes Schindelin 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 > --- > 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 ``` ## Johannes Schindelin, 2016-06-21 12:03 Subject: Re: [PATCH] Make find_commit_subject() more robust Message-ID: URL: https://gitlist.dev/e/alpine.DEB.2.20.1606211402320.22630%40virtualbox In-Reply-To: ``` Hi Junio, On Mon, 20 Jun 2016, Junio C Hamano wrote: > Johannes Schindelin 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 > > --- > > 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 ``` ## Johannes Schindelin, 2016-06-21 12:43 Subject: [PATCH v2] Make find_commit_subject() more robust Message-ID: <901b24554eb4d0381d74ceb31f7bd08709d5eb25.1466512959.git.johannes.schindelin@gmx.de> URL: https://gitlist.dev/e/901b24554eb4d0381d74ceb31f7bd08709d5eb25.1466512959.git.johannes.schindelin%40gmx.de In-Reply-To: <34ef85eb4e2aef0b342ef5d3bce9e468c8339486.1466255489.git.johannes.schindelin@gmx.de> ``` 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 --- 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 123456789 +0000" \ + "committer C 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 123456789 +0000" \ + "committer C 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 ``` ## Junio C Hamano, 2016-06-21 20:34 Subject: Re: [PATCH v2] Make find_commit_subject() more robust Message-ID: URL: https://gitlist.dev/e/xmqq60t2p9fw.fsf%40gitster.mtv.corp.google.com In-Reply-To: <901b24554eb4d0381d74ceb31f7bd08709d5eb25.1466512959.git.johannes.schindelin@gmx.de> ``` Johannes Schindelin 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 > --- > 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 123456789 +0000" \ > + "committer C 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 123456789 +0000" \ > + "committer C 123456789 +0000" | > + git hash-object -w -t commit --stdin) && > + git blame --porcelain $commit -- single-file >output && > + grep "^summary oneline$" output > + ) > +' > + > test_done ``` ## Junio C Hamano, 2016-06-21 21:15 Subject: Re: [PATCH] Make find_commit_subject() more robust Message-ID: URL: https://gitlist.dev/e/xmqqziqent0e.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Johannes Schindelin writes: > Hi Junio, > > On Mon, 20 Jun 2016, Junio C Hamano wrote: > >> Johannes Schindelin 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 >> > --- >> > 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. ``` ## Johannes Schindelin, 2016-06-22 09:03 Subject: Re: [PATCH] Make find_commit_subject() more robust Message-ID: URL: https://gitlist.dev/e/alpine.DEB.2.20.1606221041320.10382%40virtualbox In-Reply-To: ``` 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 ``` ## Johannes Schindelin, 2016-06-22 09:34 Subject: [PATCH v3 0/2] Make find_commit_subject() consistent with --format=%s Message-ID: URL: https://gitlist.dev/e/cover.1466587806.git.johannes.schindelin%40gmx.de In-Reply-To: <901b24554eb4d0381d74ceb31f7bd08709d5eb25.1466512959.git.johannes.schindelin@gmx.de> ``` 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 123456789 +0000" \ "committer C 123456789 +0000" | -- 2.9.0.118.g0e1a633 base-commit: ab7797dbe95fff38d9265869ea367020046db118 ``` ## Johannes Schindelin, 2016-06-22 09:34 Subject: [PATCH v3 2/2] Make find_commit_subject() more robust Message-ID: URL: https://gitlist.dev/e/b6a4fa27a997455c7dc4164f2d0414c99a9bf342.1466587806.git.johannes.schindelin%40gmx.de In-Reply-To: ``` 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 --- 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 123456789 +0000" \ + "committer C 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 ``` ## Johannes Schindelin, 2016-06-22 09:34 Subject: [PATCH v3 1/2] Make the skip_empty_lines() function public Message-ID: <3f1f8a118900c4f572c5f70d1752eda0a48158d7.1466587806.git.johannes.schindelin@gmx.de> URL: https://gitlist.dev/e/3f1f8a118900c4f572c5f70d1752eda0a48158d7.1466587806.git.johannes.schindelin%40gmx.de In-Reply-To: ``` This function will be used also in the find_commit_subject() function. Signed-off-by: Johannes Schindelin --- 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 ``` ## Junio C Hamano, 2016-06-22 17:21 Subject: Re: [PATCH v3 0/2] Make find_commit_subject() consistent with --format=%s Message-ID: URL: https://gitlist.dev/e/xmqq1t3pnnpq.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Johannes Schindelin 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. ``` ## Johannes Schindelin, 2016-06-22 20:20 Subject: [PATCH v4 1/2] Make the skip_blank_lines() function public Message-ID: <293bdb45f4a7c8b635ec246763346710ff4e3111.1466626795.git.johannes.schindelin@gmx.de> URL: https://gitlist.dev/e/293bdb45f4a7c8b635ec246763346710ff4e3111.1466626795.git.johannes.schindelin%40gmx.de In-Reply-To: ``` 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 --- 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 ``` ## Johannes Schindelin, 2016-06-22 20:20 Subject: [PATCH v4 0/2] Make find_commit_subject() consistent with --format=%s Message-ID: URL: https://gitlist.dev/e/cover.1466626795.git.johannes.schindelin%40gmx.de In-Reply-To: ``` 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 ``` ## Johannes Schindelin, 2016-06-22 20:20 Subject: [PATCH v4 2/2] Make find_commit_subject() more robust Message-ID: <40e7a90edbb66b3523e19832e13ee10bc96b0371.1466626795.git.johannes.schindelin@gmx.de> URL: https://gitlist.dev/e/40e7a90edbb66b3523e19832e13ee10bc96b0371.1466626795.git.johannes.schindelin%40gmx.de In-Reply-To: ``` 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 --- 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 123456789 +0000" \ + "committer C 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 ``` ## Johannes Schindelin, 2016-06-22 20:20 Subject: Re: [PATCH v3 0/2] Make find_commit_subject() consistent with --format=%s Message-ID: URL: https://gitlist.dev/e/alpine.DEB.2.20.1606222220240.10382%40virtualbox In-Reply-To: ``` Hi Junio, On Wed, 22 Jun 2016, Junio C Hamano wrote: > Johannes Schindelin 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 ```