Re: More builtin git-am issues..

Linus Torvalds [off-list ref] writes:

but the failing cases have a comment by Andrew:

    [akpm@linux-foundation.org: coding-style fixes]
    Signed-off-by: Tang Chen [off-list ref]
    Cc: Xishi Qiu [off-list ref]
    Cc: Yasuaki Ishimatsu [off-list ref]
    Cc: Kamezawa Hiroyuki [off-list ref]
    Cc: Taku Izumi [off-list ref]
    Cc: Gu Zheng [off-list ref]
    Cc: Naoya Horiguchi [off-list ref]
    Cc: Vlastimil Babka [off-list ref]
    Cc: Mel Gorman [off-list ref]
    Cc: David Rientjes [off-list ref]
    Cc: [off-list ref]        [4.2.x]
    Signed-off-by: Andrew Morton [off-list ref]

    Signed-off-by: Linus Torvalds [off-list ref]

ie that "[akpm@linux-foundation.org: coding-style fixes]" makes git am
now decide that the previous block of text was not a sign-off block,
so it adds an empty line before adding my sign-off. But very obviously
it *was* a sign-off block.

Ahh, OK, scratch what I said earlier.  The user intended this to be
sign-off block, but the convention append_signoff() was taught from
very earlier days is that the sign-off block must consist of block
of text all of which look like rfc2822 "keyword: value" header lines,
and the comment thing makes it a non-conforming header.

Perhaps

    A block of text at the end of the existing text could be a
    signature block.  If all its lines that are rfc2822-like are at
    its end, then it is a sign-off block.  Otherwise it is not.

would allow the leading non-signature lines in the above example.

If the comment line (which I would say should have been separated by
a blank line from the signature block if only to make it easier to
read the whole thing) were in the middle, e.g.

    Signed-off-by: Tang Chen [off-list ref]
    Cc: Xishi Qiu [off-list ref]
    Cc: Yasuaki Ishimatsu [off-list ref]
    Cc: Vlastimil Babka [off-list ref]
    Cc: Mel Gorman [off-list ref]
    [akpm@linux-foundation.org: coding-style fixes]
    Cc: David Rientjes [off-list ref]
    Cc: [off-list ref]        [4.2.x]
    Signed-off-by: Andrew Morton [off-list ref]

that rule would still not think this is a signature block, but at
that point, do we really want to consider such a block of text a
signature block?

On Fri, Sep 4, 2015 at 6:06 PM, Junio C Hamano [off-list ref] wrote:

that rule would still not think this is a signature block, but at
that point, do we really want to consider such a block of text a
signature block?

So exactly why are you arguing for these rules that are known to break
in real life, that I gave actual examples for existing, and that I
also gave an actual example for not just giving a false negative, but
also a false positive?

I'm also pretty sure that what you are arguing for is a regression.

Now, as mentioned, it may well be true that we've had this odd
behavior before, and it's not a real regression - I may just have
picked up on this problem because I've been more careful. Maybe I
didn't notice these problems before.

But looking at the old git-am.sh script, it does simply seem to look
for that '^Signed-off-by:' pattern. It did

              ADD_SIGNOFF=$(
                  test "$LAST_SIGNED_OFF_BY" = "$SIGNOFF" || {
                  test '' = "$LAST_SIGNED_OFF_BY" && echo
                  echo "$SIGNOFF"
              })

which seems to literally just check the last sign-off line it found.
If it matches the new sign-off, it doesn't do anything (and doesn't
add the new one either), and if it doesn't exist at all (so it's
empty) it adds teh empty line.

Quite frankly, that not only worked for a long time, it's simply less
ambiguous than your made up rule.

It's very simple. "if you find a sign-off in the commit message, don't
add an new empty line before the new signoff".

Much better than "every line in the last set of lines must match some
weak format that isn't even true, and is too non-specific anyway".

                Linus