Re: [PATCH] sequencer: require trailing NL in footers
- From
Jonathan Nieder <jrnieder@gmail.com>
- Date
- Apr 25, 2017, 23:34 UTC
- Message-ID
- <20170425233407.GO28740@aiede.svl.corp.google.com>
- In-Reply-To
- <20170425190651.8910-1-jonathantanmy@google.com>
Hi,
Jonathan Tan wrote:
Show 19 quoted lines
> 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 <jonathantanmy@google.com>
> ---[...]
Show 24 quoted lines
> --- 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 <jrnieder@gmail.com>
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