[PATCH v3 0/2] Make find_commit_subject() consistent with --format=%s
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 +" \ "committer C 123456789 +" | -- 2.9.0.118.g0e1a633 base-commit: ab7797dbe95fff38d9265869ea367020046db118 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 0/2] Make find_commit_subject() consistent with --format=%s
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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 0/2] Make find_commit_subject() consistent with --format=%s
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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html