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@xxxxxxxxx>
> Signed-off-by: Jonathan Tan <jonathantanmy@xxxxxxxxxx>
> ---
[...]
> --- 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@xxxxxxxxx>
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