Re: [PATCH] sequencer: require trailing NL in footers
From: Jonathan Nieder <hidden>
Date: 2017-04-25 23:34:58
Hi, Jonathan Tan wrote:
In commit 967dfd4 ("sequencer: use trailer's trailer layout",
2016-11-29), sequencer was taught to use the same mechanism as
interpret-trailers to determine the nature of the trailer of a commit
message (referred to as the "footer" in sequencer.c). However, the
requirement that a footer end in a newline character was inadvertently
removed. Restore that requirement.
While writing this commit, I noticed that if the "ignore_footer"
parameter in "has_conforming_footer" is greater than the distance
between the trailer start and sb->len, "has_conforming_footer" will
return an unexpected result. This does not occur in practice, because
"ignore_footer" is either zero or the return value of an invocation to
"ignore_non_trailer", which only skips empty lines and comment lines.
This commit contains a comment explaining this in the function's
documentation.
Reported-by: Brian Norris <computersforpeace@gmail.com>
Signed-off-by: Jonathan Tan <redacted>
---[...]
quoted hunk ↗ jump to hunk
--- a/sequencer.c +++ b/sequencer.c@@ -151,6 +151,12 @@ static const char *get_todo_path(const struct replay_opts *opts) * Returns 1 for conforming footer * Returns 2 when sob exists within conforming footer * Returns 3 when sob exists within conforming footer as last entry + * + * A footer that does not end in a newline is considered non-conforming. + * + * ignore_footer, if not zero, should be the return value of an invocation to + * ignore_non_trailer. See the documentation of that function for more + * information. */ static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob, int ignore_footer)@@ -159,6 +165,11 @@ static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob, int i; int found_sob = 0, found_sob_last = 0; + if (sb->len <= ignore_footer) + return 0; + if (sb->buf[sb->len - ignore_footer - 1] != '\n') + return 0; +
This is super subtle, but it does the right thing. The caller will notice it's not a conforming footer, add a newline to separate the new footer from it, and repair the footer as a side effect. Reviewed-by: Jonathan Nieder <redacted> Followup question: what should happen if there is a non-footer-shaped thing with no trailing newline at the end of the commit message? Should we add two newlines in that case when producing a new footer? Thanks, Jonathan