Re: [PATCH 0/3] interpret-trailers + commit -v bugfix

From: Jeff King <peff@peff.net>
Date: May 12, 2017, 09:00 UTC
Message-ID: <20170512090032.coddhlsrs6s3zm2f@sigill.intra.peff.net>
In-Reply-To: <20170512050347.30765-1-bmalehorn@gmail.com>

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

Show 9 quoted lines

> This patch series addresses a bug in interpret-trailers. If the commit
> that is being editted is "verbose", it will contain a scissors string
> ("-- >8 --") and a diff. interpret-trailers doesn't interpret the
> scissors and therefore places trailer information after the diff. A
> simple reproduction is:
> 
> 	git config commit.verbose true
> 	GIT_EDITOR='git interpret-trailers --in-place --trailer Acked-by:me' \
> 		git commit --amend

Ah, I should have read your cover letter more carefully before responding to the third patch (or alternatively, you should have put your motivating example into the third patch's commit message :) ).

Your example definitely makes sense. But it does make me wonder if this should be an option to interpret-trailers, so that it doesn't accidentally trigger. Unfortunately you'd have to manually specify it (even though you'd presumably have commit.verbose in your config and have long forgotten about it). And at that point, you might as well just say "--no-verbose" on the commit command-line.

> P.S. This is my first patch series to the git mailing list, to feel free
> to point out any mistakes I made when submitting.

My responses so far have all been critical, so let me be positive for a moment. :)

Welcome to the community. Everything in your patches generally looks well-formatted, etc. And I do think you're on the right track with your solution.

As I said, I'm a little iffy on doing this unconditionally, but it may be the least-bad solution. I'd just worry about collateral damage to somebody who doesn't use commit.verbose, but has something scissors-like in their commit message.

If you were to switch out is_scissors_line() for checking the exact cut_line[] from wt-status.c, I think that would be a big improvement. We'd still have the possibility of a false positive, but it would be much less likely in practice.

-Peff