On Thu, May 11, 2017 at 10:03:44PM -0700, Brian Malehorn wrote:
> 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