Re: email as a bona fide git transport

From: Vegard Nossum <vegard.nossum@oracle.com>
Date: Oct 17, 2019, 12:23 UTC
Message-ID: <a1c33600-14e6-be37-c026-8d8b8e4bad92@oracle.com>
In-Reply-To: <20191016150020.cr6jgfpd2c6fyg7t@yadavpratyush.com>

On 10/16/19 5:00 PM, Pratyush Yadav wrote:

Show 11 quoted lines

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

Show 13 quoted lines

> 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