Re: More builtin git-am issues..

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

quoted

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 [off-list ref]

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

    [some comment]
    Signed-off-by: Somebody Else [off-list ref]

    Signed-off-by: Jeff King [off-list ref]

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

    [some comment]
    Signed-off-by: Somebody Else [off-list ref]
    Signed-off-by: Jeff King [off-list ref]

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

    [some comment]
    Reviewed-by: Somebody Else [off-list ref]

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