Re: More builtin git-am issues..

On Fri, Sep 4, 2015 at 5:07 PM, Jeff King [off-list ref] wrote:

Do we want to make Signed-off-by a special token here? What about "Cc:",
and other project-specific trailers? You wouldn't want to end up with:

  [some comment]
  Cc: somebody

  Signed-off-by: somebody else

It's not a problem for git-am, which should be taking in patches that
are already signed-off (and after all, if your project does not use
signoffs, why are you using "am -s"?). But won't "git commit -s" run
into the same problem?

So I'm a *bit* worried about taking anything else than Signed-off-by:
exactly because of the problem you mention:

We could look for an arbitrary rfc2822-ish string, but I'd be worried
slightly about false positives, like:

  This is a paragraph with arbitrary text. But one
  of the lines will use a colon, and that a causes a
  problem: because of wrapping, this line kind of looks
  like a trailer.

which clearly needs an empty line before the sign-off.

And even if we limit it to just the last line, like you suggest:

Maybe only the final line needs to look rfc2822-ish?

I'd still worry, for the same exact reason. The "rfc2822-ish" check is
_so_ weak that it can be trivially triggered by normal text.

So maybe it doesn't require a strict "Signed-off-by:" match, but I
think it needs something stricter than the found_rfc2822 format (like
maybe "looks like a name/email").  We just don't want to require that
*all* the lines are that way, because at least in the kernel we often
do end up adding small comments in that section.

The git project sign-off rules seem to be stricter, and it looks like
it's almost universally of the form "Signed-off-by:" with a few
"Helped-by:" and "Reviewed-by:". In the kernel, we really do tend to
be messier in that area, with the sign-off chunk not just having the
sign-offs and cc's etc, but also tends to have those occasional small
notes left by the people forwarding the emails.

And we also often don't have _strictly_ just an email. We tend to have
things like

    Cc: [off-list ref] # v3.13+

etc, and sometimes we don't have have any email at all, but things like

    Reported-by: coverity
    Tested-by: MysterX on #openelec
    Generated-by: Coccinelle SmPL

so it's hard to give a really strict rule. It's fairly free-format.

That said, I would argue that when you apply a patch with sign-off,
pretty much by definition that patch _should_ have a sign-off from the
originator. So I suspect that the "there is an existing sign-off in
that last chunk" is a fairly good rule. Even if there are lots of
other lines too.

If you're using "git commit -s" to just commit your own work, you
presumably would normally want the extra sign-off. Or at least you can
do a "git commit --amend" fairly easily to fix things up. Doing the
amending later for "git am" is rather impractical, when it might have
been a series of 100+ emails..

                  Linus