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

[Jeff King <peff@peff.net>]

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).

> 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