Re: [PATCH] sequencer: require trailing NL in footers

[Jonathan Nieder <jrnieder@gmail.com>]

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 <jonathantanmy@google.com>
> ---
[...]
> --- 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