Re: More builtin git-am issues..

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Sep 5, 2015, 00:36 UTC
Message-ID: <CA+55aFzR2kZuS=b5D0sSV7u7Wm3a2_FENOD1_kYmr4gqdcVS2w@mail.gmail.com>
In-Reply-To: <20150905000745.GC11443@sigill.intra.peff.net>

On Fri, Sep 4, 2015 at 5:07 PM, Jeff King <peff@peff.net> wrote:

Show 13 quoted lines

>
> 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:

Show 7 quoted lines

> 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: <stable@vger.kernel.org> # 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