Re: [PATCH 2/7] Docs: Bring SubmittingPatches more into the git era

[Laszlo Ersek]

On 03/09/16 10:45, David Woodhouse wrote:
> On Tue, 2014-12-23 at 09:32 -0700, Jonathan Corbet wrote:
>>  
>> -16) Sending "git pull" requests  (from Linus emails)
>> +16) Sending "git pull" requests
>> +-------------------------------
>> +
>> +If you have a series of patches, it may be most convenient to have the
>> +maintainer pull them directly into the subsystem repository with a
>> +"git pull" operation.  Note, however, that pulling patches from a developer
>> +requires a higher degree of trust than taking patches from a mailing list.
> 
> This isn't really true, is it?
> 
> If I accept a stream of patches in email, or if I accept them in a pull
> request, I can â and should â still actually *look* at what's being
> applied before I push it back out again.
> 
> In email I should never take someone's word that v7 of a given patch
> set, with accrued Reviewed-by: tags from the previous 6 rounds of the
> submission, hasn't introduced a trojan horse or done something else
> stupid. There's absolutely *nothing* that's more fundamentally
> trustworthy about email vs. 'git pull', is there? You can't even trust
> that the version in your mailbox is the same as the one that was sent
> to the list :)
> 
> So why would it ever be safer to blindly save a patch series and apply
> it with 'git am', than it is to pull the same?
> 
> Either you *look* what what you merge, or you don't.
> 
> So I don't really understand the 'higher degree of trust' comment.
> Perhaps that was true in the days before git-am. But now that you can
> save a whole set of emails and just apply them all with one command
> that's as easy as a pull, there isn't really any difference, is there?
> Neither tool actually *forces* you to look at what you're merging.
> 
> The main reason for preferring email over pull requests, as I
> understand it, is probably just to ensure that Reviewed-by: and other
> tags can be applied at the time it's committed.
> 
> So perhaps something like this...?
> 
> iff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
> index d603fa0..c8f7f9c 100644
> --- a/Documentation/SubmittingPatches
> +++ b/Documentation/SubmittingPatches
> @@ -737,10 +737,11 @@ the cover email text) to link to an earlier version of the patch series.
>  
>  If you have a series of patches, it may be most convenient to have the
>  maintainer pull them directly into the subsystem repository with a
> -"git pull" operation.  Note, however, that pulling patches from a developer
> -requires a higher degree of trust than taking patches from a mailing list.
> -As a result, many subsystem maintainers are reluctant to take pull
> -requests, especially from new, unknown developers.  If in doubt you can use
> +"git pull" operation.  Note, however, that commits should be considered
> +immutable as soon as they are visible in public, and this means that
> +additional tags such as Reviewed-by: and Tested-by: cannot be included.
> +For this reason, some subsystem maintainers are reluctant to take pull
> +requests; especially from new, unknown developers.  If in doubt you can use
>  the pull request as the cover letter for a normal posting of the patch
>  series, giving the maintainer the option of using either.

I wish David hadn't removed the rest of the patch from his response, because now I could comment on another part of the patch (I don't have the original patch email).

I'll reproduce the hunk manually:

> +Some maintainers (including Linus) want to see pull requests from signed
> +commits; that increases their confidence that the request actually came
> +from you.  Linus, in particular, will not pull from public hosting sites
> +like GitHub in the absence of a signed tag.

I think this hunk is related to the one that David quoted; I think this should possibly be extended simultaneously with the other's update.

Namely, do signed tags serve the purpose that a higher level maintainer can pull from a trusted, lower level maintainer without looking?

At these higher levels of the patch flow, does "trusted identity" replace "review"?

I'm trying to understand if requiring signed tags in pull requests makes sense on the lowest level of patch flow. I can imagine an argument like:

- On the lowest level, patch emails or a pull req arrives from a relatively unknown contributor. The subsystem maintainer carefully reviews the patches (regardless of the requested form of merging, i.e., git-am vs. git-pull), and adds his or her Reviewed-by lines (rebasing the series to the same base commit if the form of submission was a pull req) in his or her tree. According to David (AIUI), at this stage git-am and git-pull don't differ in trust, so I think it follows that signed tags should not be a requirement when the submitter sent a pull req instead of patches.

- On higher levels, maintainers with "higher granularities" are not supposed to review individual patches; that's the job of the subsystem maintainers. They only want to make sure the pull req comes from a trusted individual, hence the requirement for signed tags. The higher level maintainer will only sign off on the merge commit.

If this is indeed the argument for signed tags, then I believe the hunk I quoted above should be made more precise as well:

> diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
> index d603fa078235..60242e9349e1 100644
> --- a/Documentation/SubmittingPatches
> +++ b/Documentation/SubmittingPatches
> @@ -760,10 +760,14 @@ themselves, and a diffstat showing the overall effect of the patch series.
>  The easiest way to get all this information together is, of course, to let
>  git do it for you with the "git request-pull" command.
>  
> -Some maintainers (including Linus) want to see pull requests from signed
> -commits; that increases their confidence that the request actually came
> -from you.  Linus, in particular, will not pull from public hosting sites
> -like GitHub in the absence of a signed tag.
> +Higher level maintainers (including Linus) who don't personally review the
> +patches that they integrate want to see pull requests from signed commits;
> +that increases their confidence that the request actually came from you.
> +Your trusted identity replaces their personal reviews; they trust you that
> +the patches you ask them to integrate have already been reviewed by people
> +you trust (including yourself, for patches you didn't author). Linus, in
> +particular, will not pull from public hosting sites like GitHub in the
> +absence of a signed tag.
>  
>  The first step toward creating such tags is to make a GNUPG key and get it
>  signed by one or more core kernel developers.  This step can be hard for

Thanks
Laszlo