Re: [PATCH 3/3] commit.c: skip scissors when computing trailers

From: Jeff King <peff@peff.net>
Date: May 12, 2017, 08:52 UTC
Message-ID: <20170512085253.p3oohlbu2utco2tk@sigill.intra.peff.net>
In-Reply-To: <20170512050347.30765-4-bmalehorn@gmail.com>

On Thu, May 11, 2017 at 10:03:47PM -0700, Brian Malehorn wrote:

> "scissors" ("----- >8 -----") can be automatically added to commit
> messages by setting commit.verbose = true. Prevent this from interfering
> with trailer calculations by automatically skipping over scissors,
> instead of (usually) treating them as a comment.

What's the sequence of commands where you end up with a scissors line in your "commit -v" output and it gets fed to interpret-trailers? Is it when you run interpret-trailers from a commit-msg hook? Or do we invoke it as part of "commit -v" itself?

I ask because I think we can probably come up with a more realistic test, which may impact what the solution looks like (see below). Grepping for ignore_non_trailer(), it looks like the issue may be the append_signoff() call in builtin/commit.c? I couldn't get "git commit -s -v" to fail, though (it handles the signoff with the verbose bits removed).

Show 28 quoted lines

> diff --git a/commit.c b/commit.c
> index 041cfa5a9..9a7b41d09 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -1701,10 +1701,10 @@ int is_scissors_line(const char *line)
>  /*
>   * Inspect the given string and determine the true "end" of the log message, in
>   * order to find where to put a new Signed-off-by: line.  Ignored are
> - * trailing comment lines and blank lines, and also the traditional
> - * "Conflicts:" block that is not commented out, so that we can use
> - * "git commit -s --amend" on an existing commit that forgot to remove
> - * it.
> + * trailing comment lines and blank lines.  To support "git commit -s
> + * --amend" on an existing commit, we also ignore "Conflicts:".  To
> + * support "git commit -v", we truncate at "---- >8 ----" and similar
> + * scissors lines.
>   *
>   * Returns the number of bytes from the tail to ignore, to be fed as
>   * the second parameter to append_signoff().
> @@ -1723,6 +1723,11 @@ int ignore_non_trailer(const char *buf, size_t len)
>  		else
>  			next_line++;
>  
> +		if (is_scissors_line(&buf[bol])) {
> +			if (!boc)
> +				boc = bol;
> +			break;
> +		}

This unconditionally ignores scissors lines. But that means if you have any inside your commit message, we may quietly corrupt the commit message. It would be better to remove the scissors lines only when we know that we've added them.

And that's part of my question above. If this is a call happening inside builtin/commit.c, then we definitely know when we've added scissors. But if it's interpret-trailers being run by a hook, passing the information down to this function is a little tricky.

Maybe I'm being overly picky. We already do a lot of gross heuristic stuff here like skipping comments and old-style Conflicts blocks (that we don't even generate anymore!).

-Peff