git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

Previous: Johannes SchindelinNext: Jonathan Tan
Message 13 of 18 in “[GIT 2.12.2 REGRESSION] git cherry-pick -x”
  1. Brian NorrisApr 21, 2017
  2. Stefan BellerApr 21, 2017
  3. Jeff KingApr 21, 2017
  4. sequencer: require trailing NL in footersJonathan Tan, Apr 25, 2017
  5. Stefan BellerApr 25, 2017
  6. Jonathan TanApr 25, 2017
  7. Stefan BellerApr 25, 2017
  8. Johannes SchindelinApr 25, 2017
  9. Jonathan NiederApr 25, 2017
  10. Brian NorrisApr 25, 2017
  11. Brian NorrisApr 26, 2017
  12. Johannes SchindelinApr 25, 2017
  13. Jonathan NiederApr 25, 2017
  14. sequencer: require trailing NL in footersJonathan Tan, Apr 25, 2017
  15. Jonathan NiederApr 26, 2017
  16. Junio C HamanoApr 26, 2017
  17. Johannes SchindelinApr 26, 2017
  18. sequencer: add newline before adding footersJonathan Tan, Apr 26, 2017

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.