Re: More builtin git-am issues..

From: Jeff King <peff@peff.net>
Date: Sep 5, 2015, 08:03 UTC
Message-ID: <20150905080325.GA25039@sigill.intra.peff.net>
In-Reply-To: <55EA9A13.2050108@kdbg.org>

On Sat, Sep 05, 2015 at 09:30:27AM +0200, Johannes Sixt wrote:

Show 11 quoted lines

> >How about a bit looser rule like this?
> >
> >     A block of text at the end of the message, each and every
> >     line in which must match "^[^: 	]+:[      ]" (that is,
> >     a "keyword" that does not contain a whitespace nor a colon,
> >     followed by a colon and whitespace, and arbitrary value thru
> >     the end of line) is a signature block.
> 
> Why do we need a new rule? The old git-am had a logic that pleased everyone,
> and it must have been implemented somewhere. Shouldn't it be sufficient to
> just re-implement or re-use that logic?

That was my thought, too; if there is a regression, let's start by fixing that for the upcoming 2.6.0, and then we can worry about doing something fancier[1] later.

And I think the original behavior really is what Linus is asking for: we consider the final block, even with a "[]" comment, as a S-o-b block if it has a Signed-off-by in it. So if we have the final block:

    [some comment]
    Signed-off-by: Somebody Else <them@example.com>

Running "am -s" with the current master yields:

    [some comment]
    Signed-off-by: Somebody Else <them@example.com>

    Signed-off-by: Jeff King <peff@peff.net>

which is wrong. Running the same with v2.5.0 gets:

    [some comment]
    Signed-off-by: Somebody Else <them@example.com>
    Signed-off-by: Jeff King <peff@peff.net>

So far so good. Now let's change our input to:

    [some comment]
    Reviewed-by: Somebody Else <them@example.com>

and run "am -s". Current "master" continues to stick the extra line in there. No surprise. But now so does v2.5.0!

So I don't think the old behavior covered all cases well, either, and there's room for improvement. But likewise, I don't recall seeing a lot of complaints about it in practice. It seems like a sane thing to restore it for the upcoming release, and then build from there.

-Peff

[1] I think part of the reason people are interested in "fancy" here is
    that this topic extends beyond "git am". There's "commit -s", of
    course, but there's also the generic "interpret-trailers" command,
    which is supposed to be a generalization of the "--signoff" option.
    It would be nice if the rules remained consistent for when we add a
    trailer to an existing block, rather than special-casing signoffs.

    But again, I think that's something to shoot for in the long run.
    It's more important in the short term not to regress "am".