Message-ID: <a1c33600-14e6-be37-c026-8d8b8e4bad92@oracle.com>
Date:   Thu, 17 Oct 2019 14:23:58 +0200
From:   Vegard Nossum <vegard.nossum@...cle.com>
To:     Pratyush Yadav <me@...avpratyush.com>
Cc:     workflows@...r.kernel.org, Git Mailing List <git@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Konstantin Ryabitsev <konstantin@...uxfoundation.org>,
        Eric Wong <e@...24.org>
Subject: Re: email as a bona fide git transport


On 10/16/19 5:00 PM, Pratyush Yadav wrote:
> On 16/10/19 12:22PM, Vegard Nossum wrote:
> Just to play the devil's advocate, even though I'm in favor of something
> like this, I'll add in another disadvantage:
> 
> - The maintainer can't make small edits before pushing the changes out.
> 
> I do that every now and then for git-gui, and Junio does that sometimes
> for Git. I don't know if the folks over at Linux do something like this,
> but using '--exact' would mean that contributors would have to send a
> re-roll for even minor changes. Its mostly an inconvenience instead of a
> problem, but I thought I'd point it out.

I don't think this is a problem.

The point of 'git am --exact' is not for maintainers per se (although
they should use it if they don't have any manual changes to make), but
for the bot that keeps track of patchsets submitted via email.

The important part is that there is a git reference to the patchset that
was submitted in the patchset that was merged. You could see it as the
maintainer rolling a new version of the patchset locally and merging
that instead of merging what was submitted directly.

Of course, this relies strongly on actually having (correct) sha1
references to previous versions inside the changelog. In my original
idea, this reference would only appear inside the merge commit that
binds the patchset together to minimise churn, although maybe it is
feasible to also append it to each patch -- in that case, the "patchset"
command from my first email is not sufficient to create a new version of
a patchset.

> One more question, not strictly related to your proposal: right now,
> when I apply patches from contributors, I pass '-s' to 'am', so the
> applied commit would have my sign-off. The way I see it, that sign-off
> is supposed to signify that I have the right to push out the commit to
> the "main" repo, just like the author's sign-off means that they have
> the right to send me that commit.
> 
> Looking at git.git, I notice that Junio does the same. The new '--exact'
> would be incompatible with '-s', correct (since the commit message has
> changed, the SHA1 would also change)? So firstly, make sure you account
> for something like that if you haven't already (I haven't found the time
> to read your patches yet). Secondly, is it all right for the maintainer
> to just not sign-off on the commits they push out?

In the Linux kernel at least, only the front-line maintainers add their
signoffs; higher-level maintainers take pull requests and don't add
their own sign-offs. Only Linus (and Greg, perhaps) has commit rights to
the main repository, but he only signs off on the patches that he either
writes himself or applies directly from email.

In any case, I don't think this is a concern because of what I wrote
above -- somebody who wants to add their signoff can do it by
essentially rolling a new version of the patchset that has the signoff,
but refers to the sha1 that was submitted to them by the patchset author
so that you can still find the original commits (without the signoffs)
and reviews/discussions.

I don't want to create extra work for maintainers, so I think any
solution should involve having the existing git tools/workflows do the
right thing automatically.

How about this? If 'git am' or 'git am -s' (without --exact) finds an
email patch where the exact commit metadata is present, it automatically
appends a line to the changelog saying where it was taken from:

     Submitted-as: 111122223333444455556666777788889999aaaa

or

     Applied-from: 111122223333444455556666777788889999aaaa

Although, again, this would modify the changelog of the patch itself
rather than just the changelog of the merge commit... Maybe this is
enough and we can have the "patchset" command also add references to
previous versions of each patch rather than the patchset as a whole.


Vegard

Powered by blists - more mailing lists