Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1353926 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 2/7] Docs: Bring SubmittingPatches more into the git era David Woodhouse <dwmw2@infradead.org> - 2016-03-09 10:50 +0100
    Re: [PATCH 2/7] Docs: Bring SubmittingPatches more into the git era Laszlo Ersek <lersek@redhat.com> - 2016-03-09 12:50 +0100
      Re: [PATCH 2/7] Docs: Bring SubmittingPatches more into the git era Jonathan Corbet <corbet@lwn.net> - 2016-03-09 15:20 +0100
    Re: [PATCH 2/7] Docs: Bring SubmittingPatches more into the git era Jonathan Corbet <corbet@lwn.net> - 2016-03-09 15:10 +0100
      Re: [PATCH 2/7] Docs: Bring SubmittingPatches more into the git era "David Woodhouse" <dwmw2@infradead.org> - 2016-03-09 16:20 +0100

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

[Multipart message — attachments visible in raw view] — view raw

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.
 
-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

[toc] | [next] | [standalone]

#1354055

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

[toc] | [prev] | [next] | [standalone]

#1354161

On Wed, 9 Mar 2016 12:44:26 +0100
Laszlo Ersek <lersek@redhat.com> wrote:

> 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"?

No, I really don't think so.  Signed tags just verify the origin of the
pull request.

Think of it as a form of defense in depth.  Anybody who merges code into
the kernel merges bugs on a regular basis, even if they carefully review
every line.  Review is a defense against threats like the deliberate
insertion of malevolent code, but it is not an absolute defense.  Signed
tags, one might hope, will at least keep code from deliberately forged
pull requests out of the stream of code needing review.

Or so I see it.

jon

[toc] | [prev] | [next] | [standalone]

#1354153

On Wed, 09 Mar 2016 09:45:10 +0000
David Woodhouse <dwmw2@infradead.org> 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.

I think I put something in there somewhere about a one-year statute of
limitation on review comments :)

I wrote that text that way because certain high-profile maintainers have
said exactly that sort of thing:

	You can send me patches, but for me to pull a git patch from you,
	I need to know that you know what you're doing, and I need to be
	able to trust things *without* then having to go and check every
	individual change by hand.

	-- Mr. T.  https://lwn.net/Articles/224135/

...and because, in truth, few maintainers do take pull requests.  There
*is* some value in having the code out on the lists in the clear, it
raises the chances of somebody *else* looking it over slightly.  There is
a reason why review is done on the lists, not directly from repositories.

Allowing the maintainer to attach tags certainly seems like another valid
reason to defer setting patches into git-implemented stone.  But I don't
see it as the only one.

We could, I suppose, run a poll to ask maintainers why they are reluctant
to take pull requests.  But the end result is kind of the same as far as
readers of SubmittingPatches are concerned - they need to send their
patches via email.

jon

[toc] | [prev] | [next] | [standalone]

#1354211

> I wrote that text that way because certain high-profile maintainers have
> said exactly that sort of thing:
>
> 	You can send me patches, but for me to pull a git patch from you,
> 	I need to know that you know what you're doing, and I need to be
> 	able to trust things *without* then having to go and check every
> 	individual change by hand.
>
> 	-- Mr. T.  https://lwn.net/Articles/224135/
>
> ...and because, in truth, few maintainers do take pull requests.  There
> *is* some value in having the code out on the lists in the clear, it
> raises the chances of somebody *else* looking it over slightly.  There is
> a reason why review is done on the lists, not directly from repositories.
>
> Allowing the maintainer to attach tags certainly seems like another valid
> reason to defer setting patches into git-implemented stone.  But I don't
> see it as the only one.
>
> We could, I suppose, run a poll to ask maintainers why they are reluctant
> to take pull requests.  But the end result is kind of the same as far as
> readers of SubmittingPatches are concerned - they need to send their
> patches via email.

You are quite right that it has the same effect in practice, for Linux.
The problem was that your words were being taken out of context in a
situation where email review *was* always going to be required anyway, but
I'm trying to get them to allow pull requests instead of always losing
history by *forcing* a rebase onto the current HEAD.

Which is a model we use often too -- post for review and feedback, but
submit a pull request with the *actual* set of commits that were tested,
on the base they were developed against. Instead of submitting *only*
patches and running the risk that what gets committed to today's tree has
*never* actually worked correctly, when we look back at the inaccurate
history.


-- 
dwmw2

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web