All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] why restrict pull reqs to signed tags?
@ 2016-03-09 10:20 Laszlo Ersek
  2016-03-09 11:33 ` Paolo Bonzini
  2016-03-09 11:35 ` Peter Maydell
  0 siblings, 2 replies; 21+ messages in thread
From: Laszlo Ersek @ 2016-03-09 10:20 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Jordan Justen (Intel address),
	David Woodhouse, qemu devel list, Ard Biesheuvel

Hi,

the question in the subject is not loaded, it is not trying to suggest
the opposite. It's a genuine question.

If you have a few (tens of) minutes, please read through this sub-thread
(or the entire discussion):

http://thread.gmane.org/gmane.comp.bios.edk2.devel/8864/focus=8898

See also

http://thread.gmane.org/gmane.linux.kernel/1855303/focus=2172858

Thanks
Laszlo

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] why restrict pull reqs to signed tags?
  2016-03-09 10:20 [Qemu-devel] why restrict pull reqs to signed tags? Laszlo Ersek
@ 2016-03-09 11:33 ` Paolo Bonzini
  2016-03-09 11:35 ` Peter Maydell
  1 sibling, 0 replies; 21+ messages in thread
From: Paolo Bonzini @ 2016-03-09 11:33 UTC (permalink / raw)
  To: Laszlo Ersek, Peter Maydell
  Cc: Jordan Justen (Intel address),
	David Woodhouse, qemu devel list, Ard Biesheuvel



On 09/03/2016 11:20, Laszlo Ersek wrote:
> Hi,
> 
> the question in the subject is not loaded, it is not trying to suggest
> the opposite. It's a genuine question.

I think it boils down to this comment in the thread:

"Signed pulls are about personal trust. If you have signed emails or a
signed pull request from someone you trust sufficiently, you don't
*need* to look at the content. There is no difference between email and
pull requests, in this respect".

Paolo

> If you have a few (tens of) minutes, please read through this sub-thread
> (or the entire discussion):
> 
> http://thread.gmane.org/gmane.comp.bios.edk2.devel/8864/focus=8898
> 
> See also
> 
> http://thread.gmane.org/gmane.linux.kernel/1855303/focus=2172858

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] why restrict pull reqs to signed tags?
  2016-03-09 10:20 [Qemu-devel] why restrict pull reqs to signed tags? Laszlo Ersek
  2016-03-09 11:33 ` Paolo Bonzini
@ 2016-03-09 11:35 ` Peter Maydell
  2016-03-09 12:13   ` Laszlo Ersek
  1 sibling, 1 reply; 21+ messages in thread
From: Peter Maydell @ 2016-03-09 11:35 UTC (permalink / raw)
  To: Laszlo Ersek
  Cc: Jordan Justen (Intel address),
	David Woodhouse, qemu devel list, Ard Biesheuvel

On 9 March 2016 at 17:20, Laszlo Ersek <lersek@redhat.com> wrote:
> the question in the subject is not loaded, it is not trying to suggest
> the opposite. It's a genuine question.

So, with an initial disclaimer that we have to some extent cargo-culted
our process here from the kernel, my view is:

 * we only take pull requests from known submaintainers (ie I will
not take a pull request from an arbitrary person)
 * I don't do anything with pull requests beyond an automated build
test and eyeball of the git log for any obvious howlers
 * a pull request is therefore equivalent to being able to directly
commit to master, and so it's worth using the signed-tag machinery
to ensure that we only give those rights to the people (submaintainers)
we think we've given them to

Conversely, a random set of patches sent to the list is supposed
to be reviewed and tested by the submaintainer who applies them to
their tree -- that is the gateway at which review happens.

thanks
-- PMM

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] why restrict pull reqs to signed tags?
  2016-03-09 11:35 ` Peter Maydell
@ 2016-03-09 12:13   ` Laszlo Ersek
  2016-03-09 12:19     ` Paolo Bonzini
  2016-03-09 12:34     ` David Woodhouse
  0 siblings, 2 replies; 21+ messages in thread
From: Laszlo Ersek @ 2016-03-09 12:13 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Jordan Justen (Intel address),
	Paolo Bonzini, David Woodhouse, qemu devel list, Ard Biesheuvel

On 03/09/16 12:35, Peter Maydell wrote:
> On 9 March 2016 at 17:20, Laszlo Ersek <lersek@redhat.com> wrote:
>> the question in the subject is not loaded, it is not trying to suggest
>> the opposite. It's a genuine question.
> 
> So, with an initial disclaimer that we have to some extent cargo-culted
> our process here from the kernel, my view is:
> 
>  * we only take pull requests from known submaintainers (ie I will
> not take a pull request from an arbitrary person)
>  * I don't do anything with pull requests beyond an automated build
> test and eyeball of the git log for any obvious howlers
>  * a pull request is therefore equivalent to being able to directly
> commit to master, and so it's worth using the signed-tag machinery
> to ensure that we only give those rights to the people (submaintainers)
> we think we've given them to

I understand, thank you. Especially your "directly commit to master"
analogy is good. Pulling replaces your detailed personal review with the
trusted identity of the pull requestor -- you trust that the commits on
the requestor's branch are already sufficiently reviewed.

http://thread.gmane.org/gmane.linux.kernel/1855303/focus=2172988

> Conversely, a random set of patches sent to the list is supposed
> to be reviewed and tested by the submaintainer who applies them to
> their tree -- that is the gateway at which review happens.

This was my understanding, yes.

David is proposing that direct pull requests be allowed on edk2-devel,
immediately from contributors, so that the contributor may ask for
his/her exact history to be preserved. I'm looking for examples: high
profile projects that have adopted such a workflow *all the while*
enforcing patch-wise reviews. Thus far I've come up empty.

I think the idea we have thus far is:

- submitter posts the patches
- patches are reviewed on the list
- submitter picks up the R-b, A-b, T-b labels
- when converged, submitter sends a pull request with the labels
applied, with the history he or she likes
- maintainer fetches the branch, verifies that the commits indeed match
the patches on list; also verifies that the labels have been correctly
picked up from the list
- maintainer merges the branch locally and pushes the merge commit (and
its deps) to upstream master

I feel a bit uncertain that we're trailblazing this workflow. It could
work I guess.

Thank you
Laszlo

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] why restrict pull reqs to signed tags?
  2016-03-09 12:13   ` Laszlo Ersek
@ 2016-03-09 12:19     ` Paolo Bonzini
  2016-03-09 12:31       ` Laszlo Ersek
  2016-03-09 12:34     ` David Woodhouse
  1 sibling, 1 reply; 21+ messages in thread
From: Paolo Bonzini @ 2016-03-09 12:19 UTC (permalink / raw)
  To: Laszlo Ersek, Peter Maydell
  Cc: Jordan Justen (Intel address),
	David Woodhouse, qemu devel list, Ard Biesheuvel



On 09/03/2016 13:13, Laszlo Ersek wrote:
> David is proposing that direct pull requests be allowed on edk2-devel,
> immediately from contributors, so that the contributor may ask for
> his/her exact history to be preserved. I'm looking for examples: high
> profile projects that have adopted such a workflow *all the while*
> enforcing patch-wise reviews. Thus far I've come up empty.

Ironically, projects using github pull requests do this.  They do code
review through the website and merge with a button.  The resulting
history is non-linear.

Paolo

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] why restrict pull reqs to signed tags?
  2016-03-09 12:19     ` Paolo Bonzini
@ 2016-03-09 12:31       ` Laszlo Ersek
  2016-03-09 12:33         ` Paolo Bonzini
  2016-03-09 12:38         ` David Woodhouse
  0 siblings, 2 replies; 21+ messages in thread
From: Laszlo Ersek @ 2016-03-09 12:31 UTC (permalink / raw)
  To: Paolo Bonzini, Peter Maydell
  Cc: Jordan Justen (Intel address),
	David Woodhouse, qemu devel list, Ard Biesheuvel

On 03/09/16 13:19, Paolo Bonzini wrote:
> 
> 
> On 09/03/2016 13:13, Laszlo Ersek wrote:
>> David is proposing that direct pull requests be allowed on edk2-devel,
>> immediately from contributors, so that the contributor may ask for
>> his/her exact history to be preserved. I'm looking for examples: high
>> profile projects that have adopted such a workflow *all the while*
>> enforcing patch-wise reviews. Thus far I've come up empty.
> 
> Ironically, projects using github pull requests do this.  They do code
> review through the website and merge with a button.  The resulting
> history is non-linear.

The website based review is a big minus:
- email is more flexible for formulating a careful, detailed review
- the review discussion is independently archived, not held hostage in
  a proprietary system

The final result is also inferior I think:
- the various feedback tags are not captured in the commit message of
  each individual patch

We've been getting github pull requests for edk2. I'm always in a rush
to reject them, lest another maintainer click the button out of
oversight. I insist on keeping it all on-list.

Thanks
Laszlo

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] why restrict pull reqs to signed tags?
  2016-03-09 12:31       ` Laszlo Ersek
@ 2016-03-09 12:33         ` Paolo Bonzini
  2016-03-09 12:38         ` David Woodhouse
  1 sibling, 0 replies; 21+ messages in thread
From: Paolo Bonzini @ 2016-03-09 12:33 UTC (permalink / raw)
  To: Laszlo Ersek, Peter Maydell
  Cc: Jordan Justen (Intel address),
	David Woodhouse, qemu devel list, Ard Biesheuvel



On 09/03/2016 13:31, Laszlo Ersek wrote:
> > Ironically, projects using github pull requests do this.  They do code
> > review through the website and merge with a button.  The resulting
> > history is non-linear.
>
> The website based review is a big minus:
> - email is more flexible for formulating a careful, detailed review
> - the review discussion is independently archived, not held hostage in
>   a proprietary system
> 
> The final result is also inferior I think:
> - the various feedback tags are not captured in the commit message of
>   each individual patch
> 
> We've been getting github pull requests for edk2. I'm always in a rush
> to reject them, lest another maintainer click the button out of
> oversight. I insist on keeping it all on-list.

I agree, hence the "Ironically" part.  Still, the point remains that
github pull requests result in a much more non-linear history than Linux
or QEMU.

That said, it also doesn't do exactly what David says, because the tags
are recorded in the web interface only---not in the commit.

Paolo

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] why restrict pull reqs to signed tags?
  2016-03-09 12:13   ` Laszlo Ersek
  2016-03-09 12:19     ` Paolo Bonzini
@ 2016-03-09 12:34     ` David Woodhouse
  2016-03-09 12:42       ` Peter Maydell
  1 sibling, 1 reply; 21+ messages in thread
From: David Woodhouse @ 2016-03-09 12:34 UTC (permalink / raw)
  To: Laszlo Ersek, Peter Maydell
  Cc: Jordan Justen (Intel address),
	Paolo Bonzini, qemu devel list, Ard Biesheuvel

[-- Attachment #1: Type: text/plain, Size: 4282 bytes --]

On Wed, 2016-03-09 at 13:13 +0100, Laszlo Ersek wrote:
> I understand, thank you. Especially your "directly commit to master"
> analogy is good. Pulling replaces your detailed personal review with the
> trusted identity of the pull requestor -- you trust that the commits on
> the requestor's branch are already sufficiently reviewed.

Note that it doesn't *have* to. And again, there's nothing special
about email vs. pull requests here.

Pater is saying that he *chooses* not to bother reviewing what he pulls
in via pull requests, and *that's* why it's equivalent to direct commit
access.

I could just as well say that *I* choose to hold my nose and look the
other way while running 'git am', and thus *patches* would be
equivalent to direct commit access.

I won't tell Peter that his behaviour is wrong. I'll only say that
other projects don't have to do the same thing.

And repeat that from the trust point of view, there is *nothing*
fundamentally different about patches vs. pull requests.

> http://thread.gmane.org/gmane.linux.kernel/1855303/focus=2172988
> 
> > 
> > Conversely, a random set of patches sent to the list is supposed
> > to be reviewed and tested by the submaintainer who applies them to
> > their tree -- that is the gateway at which review happens.
> This was my understanding, yes.
> 
> David is proposing that direct pull requests be allowed on edk2-devel,
> immediately from contributors, so that the contributor may ask for
> his/her exact history to be preserved. I'm looking for examples: high
> profile projects that have adopted such a workflow *all the while*
> enforcing patch-wise reviews. Thus far I've come up empty.
>
> I think the idea we have thus far is:
> 
> - submitter posts the patches
> - patches are reviewed on the list
> - submitter picks up the R-b, A-b, T-b labels

 (as well as any other feedback, of course)

> - when converged, submitter sends a pull request with the labels
> applied, with the history he or she likes
> - maintainer fetches the branch, verifies that the commits indeed match
> the patches on list; also verifies that the labels have been correctly
> picked up from the list

 All of which the maintainer is already expected to do, right? Except
instead of 'fetches the branch' the maintainer is currently applying
the patches in his/her *own* mailbox, potentially to a current master
on which they don't actually work, and then doing the rest of what you
said.

> - maintainer merges the branch locally and pushes the merge commit (and
> its deps) to upstream master

Well, the above two steps would be 'pull, then look'. I don't think
explicit messing with topic branches and manual merges would be
necessary.

You do a 'git pull $SUBMITTED_TREE'. If you like what you get, you then
just do a 'git push'. If you don't, 'git reset --hard origin' and send
an email saying why it was rejected.


> I feel a bit uncertain that we're trailblazing this workflow. It could
> work I guess.

We wouldn't be trailblazing it at all. It's done all the time in Linux
and various other projects. It's just that the 'submitter' rôle is
split between individual contributors, and subsystem maintainers.

> - submitter posts the patches
> - patches are reviewed on the list
> - submitter picks up the R-b, A-b, T-b labels

In fact either the submitter will pick them up when sending a second
round of patches (if there are any *other* changes to make), or the
subsystem maintainer will pick them up (via patchwork, usually) when
applying the patches to the subsystem tree.

> - when converged, submitter sends a pull request with the labels
applied, with the history he or she likes

The subsystem maintainer sends the pull request to Linus.

> - maintainer fetches the branch, verifies that the commits indeed match
> the patches on list; also verifies that the labels have been correctly
> picked up from the list
> - maintainer merges the branch locally and pushes the merge commit
> (and its deps) to upstream master

Linus does a test pull, and either likes what he sees, or doesn't. Or
depending on who it comes from and how much he cared about the code
being modified, doesn't look too hard.


-- 
dwmw2


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5691 bytes --]

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] why restrict pull reqs to signed tags?
  2016-03-09 12:31       ` Laszlo Ersek
  2016-03-09 12:33         ` Paolo Bonzini
@ 2016-03-09 12:38         ` David Woodhouse
  2016-03-09 12:40           ` Ard Biesheuvel
  1 sibling, 1 reply; 21+ messages in thread
From: David Woodhouse @ 2016-03-09 12:38 UTC (permalink / raw)
  To: Laszlo Ersek, Paolo Bonzini, Peter Maydell
  Cc: Jordan Justen (Intel address), qemu devel list, Ard Biesheuvel

[-- Attachment #1: Type: text/plain, Size: 464 bytes --]

On Wed, 2016-03-09 at 13:31 +0100, Laszlo Ersek wrote:
> 
> 
> The website based review is a big minus:
> - email is more flexible for formulating a careful, detailed review

Well, assuming people are capable of using email competently. Which
isn't necessarily the case in the EDK2 world. But anyway, I don't think
anyone is suggesting that we *not* require patches to be posted in
email for review. So that's something of a digression.,

-- 
dwmw2


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5691 bytes --]

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] why restrict pull reqs to signed tags?
  2016-03-09 12:38         ` David Woodhouse
@ 2016-03-09 12:40           ` Ard Biesheuvel
  2016-03-09 12:44             ` Peter Maydell
  0 siblings, 1 reply; 21+ messages in thread
From: Ard Biesheuvel @ 2016-03-09 12:40 UTC (permalink / raw)
  To: David Woodhouse
  Cc: Paolo Bonzini, Laszlo Ersek, qemu devel list,
	Jordan Justen (Intel address),
	Peter Maydell

On 9 March 2016 at 19:38, David Woodhouse <dwmw2@infradead.org> wrote:
> On Wed, 2016-03-09 at 13:31 +0100, Laszlo Ersek wrote:
>>
>>
>> The website based review is a big minus:
>> - email is more flexible for formulating a careful, detailed review
>
> Well, assuming people are capable of using email competently. Which
> isn't necessarily the case in the EDK2 world. But anyway, I don't think
> anyone is suggesting that we *not* require patches to be posted in
> email for review. So that's something of a digression.,
>

Laszlo is going to shoot me for saying this, but as a compromise
between the Windows crowd in the Intel firmware team and the more
Linux/open source oriented people on the opposite side, using github
does not sound like an entirely unreasonable compromise. At least it
means no emails anymore with garbled patches and 300 character lines
in the commit log

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] why restrict pull reqs to signed tags?
  2016-03-09 12:34     ` David Woodhouse
@ 2016-03-09 12:42       ` Peter Maydell
  2016-03-09 13:09         ` David Woodhouse
  0 siblings, 1 reply; 21+ messages in thread
From: Peter Maydell @ 2016-03-09 12:42 UTC (permalink / raw)
  To: David Woodhouse
  Cc: Jordan Justen (Intel address),
	Paolo Bonzini, Laszlo Ersek, qemu devel list, Ard Biesheuvel

On 9 March 2016 at 19:34, David Woodhouse <dwmw2@infradead.org> wrote:
> On Wed, 2016-03-09 at 13:13 +0100, Laszlo Ersek wrote:
>> I understand, thank you. Especially your "directly commit to master"
>> analogy is good. Pulling replaces your detailed personal review with the
>> trusted identity of the pull requestor -- you trust that the commits on
>> the requestor's branch are already sufficiently reviewed.
>
> Note that it doesn't *have* to. And again, there's nothing special
> about email vs. pull requests here.
>
> Pater is saying that he *chooses* not to bother reviewing what he pulls
> in via pull requests, and *that's* why it's equivalent to direct commit
> access.
>
> I could just as well say that *I* choose to hold my nose and look the
> other way while running 'git am', and thus *patches* would be
> equivalent to direct commit access.
>
> I won't tell Peter that his behaviour is wrong. I'll only say that
> other projects don't have to do the same thing.

Yes; I'm describing QEMU's workflow, not trying to present it as
the One True Way of doing things. I would quibble with the
"not to bother" phrasing, though -- if I reviewed everything going
into master this would not scale and I would very quickly burn out
and go spend my time studying Japanese instead. The pullreqs-from-
submaintainers setup is specifically intended to spread that
review and test workload out to a wider circle of people (who
also have local-subject-area expertise which I don't have).
That I don't review the patches flowing into master via the
pulls I merge is a feature of this workflow, not a bug.

thanks
-- PMM

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] why restrict pull reqs to signed tags?
  2016-03-09 12:40           ` Ard Biesheuvel
@ 2016-03-09 12:44             ` Peter Maydell
  2016-03-09 13:14               ` Laszlo Ersek
  0 siblings, 1 reply; 21+ messages in thread
From: Peter Maydell @ 2016-03-09 12:44 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Laszlo Ersek, David Woodhouse, qemu devel list,
	Jordan Justen (Intel address),
	Paolo Bonzini

On 9 March 2016 at 19:40, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> Laszlo is going to shoot me for saying this, but as a compromise
> between the Windows crowd in the Intel firmware team and the more
> Linux/open source oriented people on the opposite side, using github
> does not sound like an entirely unreasonable compromise. At least it
> means no emails anymore with garbled patches and 300 character lines
> in the commit log

By the way, why are you all discussing the EDK2 patch workflow
proposals on qemu-devel and not on an EDK mailing list?

thanks
-- PMM

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] why restrict pull reqs to signed tags?
  2016-03-09 12:42       ` Peter Maydell
@ 2016-03-09 13:09         ` David Woodhouse
  2016-03-09 13:27           ` Peter Maydell
  0 siblings, 1 reply; 21+ messages in thread
From: David Woodhouse @ 2016-03-09 13:09 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Jordan Justen (Intel address),
	Paolo Bonzini, Laszlo Ersek, qemu devel list, Ard Biesheuvel

[-- Attachment #1: Type: text/plain, Size: 998 bytes --]

On Wed, 2016-03-09 at 19:42 +0700, Peter Maydell wrote:
> 
> Yes; I'm describing QEMU's workflow, not trying to present it as
> the One True Way of doing things. I would quibble with the
> "not to bother" phrasing, though -- if I reviewed everything going
> into master this would not scale and I would very quickly burn out
> and go spend my time studying Japanese instead. The pullreqs-from-
> submaintainers setup is specifically intended to spread that
> review and test workload out to a wider circle of people (who
> also have local-subject-area expertise which I don't have).
> That I don't review the patches flowing into master via the
> pulls I merge is a feature of this workflow, not a bug.

Yeah, but the important criterion is *who* the thing comes from (and
again, a signed git tag is just as good as a set of signed emails).

It *isn't* about pull vs. email. That's just the transport mechanism.
There may be a correlation, but it isn't important.

-- 
dwmw2


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5691 bytes --]

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] why restrict pull reqs to signed tags?
  2016-03-09 12:44             ` Peter Maydell
@ 2016-03-09 13:14               ` Laszlo Ersek
  0 siblings, 0 replies; 21+ messages in thread
From: Laszlo Ersek @ 2016-03-09 13:14 UTC (permalink / raw)
  To: Peter Maydell, Ard Biesheuvel
  Cc: Paolo Bonzini, David Woodhouse, qemu devel list,
	Jordan Justen (Intel address)

On 03/09/16 13:44, Peter Maydell wrote:
> On 9 March 2016 at 19:40, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> Laszlo is going to shoot me for saying this, but as a compromise
>> between the Windows crowd in the Intel firmware team and the more
>> Linux/open source oriented people on the opposite side, using github
>> does not sound like an entirely unreasonable compromise. At least it
>> means no emails anymore with garbled patches and 300 character lines
>> in the commit log
> 
> By the way, why are you all discussing the EDK2 patch workflow
> proposals on qemu-devel and not on an EDK mailing list?

Sorry, that's where we started, and the rest of the discussion should
happen there; I agree. The reason for coming here was that we ended up
questioning / re-examining the practices we had taken as examples: QEMU,
Linux. I needed some info from the source, and I wanted to allow anyone
on qemu-devel to chime in. (This is why I didn't ask only you
personally, edk2-devel CC'd.)

Thanks
Laszlo

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] why restrict pull reqs to signed tags?
  2016-03-09 13:09         ` David Woodhouse
@ 2016-03-09 13:27           ` Peter Maydell
  2016-03-09 14:13             ` David Woodhouse
  0 siblings, 1 reply; 21+ messages in thread
From: Peter Maydell @ 2016-03-09 13:27 UTC (permalink / raw)
  To: David Woodhouse
  Cc: Jordan Justen (Intel address),
	Paolo Bonzini, Laszlo Ersek, qemu devel list, Ard Biesheuvel

On 9 March 2016 at 20:09, David Woodhouse <dwmw2@infradead.org> wrote:
> Yeah, but the important criterion is *who* the thing comes from (and
> again, a signed git tag is just as good as a set of signed emails).

Well, it's also important to me that it's easy to apply stuff
and that it comes in a single lump that's large enough that I
don't have a lot of overhead in processing it. Sure, you could
gpg sign individual patch mails and then check signatures when
doing git am, but I don't do that because it would be a complete
pain (and I'm not sure git has built-in tooling for doing it
the way it does with gpg signed tags and merges). So I definitely
would bounce an attempt by a submaintainer to send me stuff
as a pile of signed patchmails.

> It *isn't* about pull vs. email. That's just the transport mechanism.
> There may be a correlation, but it isn't important.

Right, but Laszlo didn't ask "why pull requests", he asked
"why signed pull requests", to which the answer is "because
of the trust implied by the way our workflow uses pullreqs".

thanks
-- PMM

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] why restrict pull reqs to signed tags?
  2016-03-09 13:27           ` Peter Maydell
@ 2016-03-09 14:13             ` David Woodhouse
  2016-03-09 14:41               ` Laszlo Ersek
  0 siblings, 1 reply; 21+ messages in thread
From: David Woodhouse @ 2016-03-09 14:13 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Ard Biesheuvel, David Woodhouse, qemu devel list,
	Jordan Justen (Intel address),
	Paolo Bonzini, Laszlo Ersek


> On 9 March 2016 at 20:09, David Woodhouse <dwmw2@infradead.org> wrote:
>> Yeah, but the important criterion is *who* the thing comes from (and
>> again, a signed git tag is just as good as a set of signed emails).
>
> Well, it's also important to me that it's easy to apply stuff
> and that it comes in a single lump that's large enough that I
> don't have a lot of overhead in processing it. Sure, you could
> gpg sign individual patch mails and then check signatures when
> doing git am, but I don't do that because it would be a complete
> pain (and I'm not sure git has built-in tooling for doing it
> the way it does with gpg signed tags and merges). So I definitely
> would bounce an attempt by a submaintainer to send me stuff
> as a pile of signed patchmails.

Sure,  pull requests are better than email in fairly much every way :)

>> It *isn't* about pull vs. email. That's just the transport mechanism.
>> There may be a correlation, but it isn't important.
>
> Right, but Laszlo didn't ask "why pull requests", he asked
> "why signed pull requests", to which the answer is "because
> of the trust implied by the way our workflow uses pullreqs".

I believe he saw the discussion of *signed* pull requests and was
concerned that pull requests  were somehow dangerous in a way that
required extra validation before you could touch them. When in fact that's
not the case at all. The use of signatures permits personal trust to
eliminate the additional checking at the time you merge -- but that's a
completely orthogonal thing which *can* also apply with emails  (although
less easily as you observe).

The background is that they currently use a workshop which enforces
rebasing onto the latest master, thus leading to lost history and the
potential for commits which apparently *never* worked, in cases when we
*should* have a working commit and a subsequently broken merge. And I'm
trying to get them to fix that and use git properly, :)

-- 
dwmw2

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] why restrict pull reqs to signed tags?
  2016-03-09 14:13             ` David Woodhouse
@ 2016-03-09 14:41               ` Laszlo Ersek
  2016-03-10  8:21                 ` David Woodhouse
  0 siblings, 1 reply; 21+ messages in thread
From: Laszlo Ersek @ 2016-03-09 14:41 UTC (permalink / raw)
  To: David Woodhouse, Peter Maydell
  Cc: Jordan Justen (Intel address),
	Paolo Bonzini, qemu devel list, Ard Biesheuvel

On 03/09/16 15:13, David Woodhouse wrote:

> The background is that they currently use a workshop which enforces
> rebasing onto the latest master, thus leading to lost history

This is exactly what the QEMU development model enforces, as I've
repeated many times. Maintainers who review and pick up your QEMU
patches will freely rebase them, reorder them against other
contributors' series they queue until their next pull, and resolve
rebase conflicts without asking you if they can.

What I may have forgotten to say (but have been reminded of) is that
maintainers are personally responsible for *testing* such rebases before
they send a PULL with them.

Nonetheless, I don't think such testing would make much difference for
you, because (a) the history would be changed anyway, which you can't
accept, and (b) IIRC I volunteered to test your (great) OpenSSL work in
practice even before you did, even without being a CryptoPkg
co-maintainer, but that still doesn't seem to give you enough confidence
in our workflow.

My point is that the workflow we currently use in edk2 is not
*inherently* broken. Many other projects, like QEMU, use it.

> and the
> potential for commits which apparently *never* worked, in cases when we
> *should* have a working commit and a subsequently broken merge. And I'm
> trying to get them to fix that and use git properly, :)

According to your description, QEMU uses git improperly, so does
libvirt, and the KVM (kernel) queue too.

Anyway, I think we're running in circles. I won't try to block this
endeavor in edk2. If (a) merges become generally accepted by the other
package maintainers in edk2, and (b) a contributor asks me personally
that his branch be pulled rather than rebased, I'll do my best to
conform, while ensuring correctness & security. Until (a&&b), I as an
edk2 co-maintainer will definitely follow the current rules.

I apologize again for the noise on qemu-devel; I'm out.

Laszlo

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] why restrict pull reqs to signed tags?
  2016-03-09 14:41               ` Laszlo Ersek
@ 2016-03-10  8:21                 ` David Woodhouse
  2016-03-10  8:52                   ` Markus Armbruster
  0 siblings, 1 reply; 21+ messages in thread
From: David Woodhouse @ 2016-03-10  8:21 UTC (permalink / raw)
  To: Laszlo Ersek, Peter Maydell
  Cc: Jordan Justen (Intel address),
	Paolo Bonzini, qemu devel list, Ard Biesheuvel

[-- Attachment #1: Type: text/plain, Size: 2129 bytes --]

On Wed, 2016-03-09 at 15:41 +0100, Laszlo Ersek wrote:
> According to your description, QEMU uses git improperly, so does
> libvirt, and the KVM (kernel) queue too.

Let's see...

Imagine I'm working on a new feature, and I submit a carefully tested
sequence of commits in a pull request.

Someone *rebases* my work onto a different point in history, which
introduces a bug.

The record now shows that I submitted something *broken*. Like
https://github.com/openssl/openssl/commit/963bb621 for example, which
is utterly hosed and broke the build for everyone except me. 

On that occasion, I was able to look back at what I *actually*
submitted and point out that it wasn't my fault. But sometimes the
breakage is more subtle. You end up looking back a few months later and
trying to work out why an esoteric corner case is failing. You might
ask me, and I'll say that I *distinctly* remember I thought about it,
and that I could have sworn I *had* tested it.... 

But again the record shows that what got merged has *never* worked.
That's no longer just a problem of embarrassment for the submitter, by
misrepresenting their work. That's now causing problems for the
*project*. Because if history had been represented *correctly*, with a
working commit and then a subsequent merge introducing the breakage,
then that is a *whole* lot easier to figure out.

Preserving accurate history is a large part of the reason we *have*
version control systems. And yes, if any of those projects you list are
deliberately throwing away history as a matter of course on non-trivial 
submissions, then I would say that they are using the tools improperly.

Of course, for simple patches it often makes no difference (well, apart
from the OpenSSL example I gave above). And for larger submissions it
doesn't *often* make a difference. But that's not the point. Sure, let
people apply their *discretion* about rebasing stuff, if you really
must — but a workflow which *enforces* a rebase, and *never* lets you
pull a complex submission as it *actually* happened, is quite wrong.


-- 
dwmw2


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5691 bytes --]

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] why restrict pull reqs to signed tags?
  2016-03-10  8:21                 ` David Woodhouse
@ 2016-03-10  8:52                   ` Markus Armbruster
  2016-03-10 10:34                     ` David Woodhouse
  0 siblings, 1 reply; 21+ messages in thread
From: Markus Armbruster @ 2016-03-10  8:52 UTC (permalink / raw)
  To: David Woodhouse
  Cc: Peter Maydell, Ard Biesheuvel, Jordan Justen (Intel address),
	qemu devel list, Paolo Bonzini, Laszlo Ersek

David Woodhouse <dwmw2@infradead.org> writes:

> On Wed, 2016-03-09 at 15:41 +0100, Laszlo Ersek wrote:
>> According to your description, QEMU uses git improperly, so does
>> libvirt, and the KVM (kernel) queue too.
>
> Let's see...
>
> Imagine I'm working on a new feature, and I submit a carefully tested
> sequence of commits in a pull request.
>
> Someone *rebases* my work onto a different point in history, which
> introduces a bug.
>
> The record now shows that I submitted something *broken*. Like
> https://github.com/openssl/openssl/commit/963bb621 for example, which
> is utterly hosed and broke the build for everyone except me. 
>
> On that occasion, I was able to look back at what I *actually*
> submitted and point out that it wasn't my fault. But sometimes the
> breakage is more subtle. You end up looking back a few months later and
> trying to work out why an esoteric corner case is failing. You might
> ask me, and I'll say that I *distinctly* remember I thought about it,
> and that I could have sworn I *had* tested it.... 
>
> But again the record shows that what got merged has *never* worked.
> That's no longer just a problem of embarrassment for the submitter, by
> misrepresenting their work. That's now causing problems for the
> *project*. Because if history had been represented *correctly*, with a
> working commit and then a subsequent merge introducing the breakage,
> then that is a *whole* lot easier to figure out.
>
> Preserving accurate history is a large part of the reason we *have*
> version control systems. And yes, if any of those projects you list are
> deliberately throwing away history as a matter of course on non-trivial 
> submissions, then I would say that they are using the tools improperly.
>
> Of course, for simple patches it often makes no difference (well, apart
> from the OpenSSL example I gave above). And for larger submissions it
> doesn't *often* make a difference. But that's not the point. Sure, let
> people apply their *discretion* about rebasing stuff, if you really
> must — but a workflow which *enforces* a rebase, and *never* lets you
> pull a complex submission as it *actually* happened, is quite wrong.

Strawman alert: we don't *enforce* rebase.  We leave it to the
maintainer's discretion.

Nothing stops a maintainer (or a chain of them) from accepting pull
requests.  But for better or worse, most maintainers rebase most of the
time.  When they do, they add their S-o-B to every patch, which provides
a measure of accountability.

I acknowledge your points are worth considering, even though you
exaggerated for effect :)

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] why restrict pull reqs to signed tags?
  2016-03-10  8:52                   ` Markus Armbruster
@ 2016-03-10 10:34                     ` David Woodhouse
  2016-03-10 12:38                       ` Laszlo Ersek
  0 siblings, 1 reply; 21+ messages in thread
From: David Woodhouse @ 2016-03-10 10:34 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: Peter Maydell, Ard Biesheuvel, Jordan Justen (Intel address),
	qemu devel list, Paolo Bonzini, Laszlo Ersek

[-- Attachment #1: Type: text/plain, Size: 524 bytes --]

On Thu, 2016-03-10 at 09:52 +0100, Markus Armbruster wrote:
> Strawman alert: we don't *enforce* rebase.  We leave it to the
> maintainer's discretion. Nothing stops a maintainer (or a chain of
> them) from accepting pull requests. 

Which is all I was asking EDK2 to do. They *do* enforce rebase, which
is wrong.

Laszlo appeared to be saying "but qemu works like this; are they wrong
too?".

To which the answer is apparently "no, they don't work like this."

Thanks for clearing that up.

-- 
dwmw2


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5691 bytes --]

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] why restrict pull reqs to signed tags?
  2016-03-10 10:34                     ` David Woodhouse
@ 2016-03-10 12:38                       ` Laszlo Ersek
  0 siblings, 0 replies; 21+ messages in thread
From: Laszlo Ersek @ 2016-03-10 12:38 UTC (permalink / raw)
  To: David Woodhouse, Markus Armbruster
  Cc: Peter Maydell, Paolo Bonzini, qemu devel list,
	Jordan Justen (Intel address),
	Ard Biesheuvel

On 03/10/16 11:34, David Woodhouse wrote:
> On Thu, 2016-03-10 at 09:52 +0100, Markus Armbruster wrote:
>> Strawman alert: we don't *enforce* rebase.  We leave it to the
>> maintainer's discretion. Nothing stops a maintainer (or a chain of
>> them) from accepting pull requests. 
> 
> Which is all I was asking EDK2 to do. They *do* enforce rebase, which
> is wrong.
> 
> Laszlo appeared to be saying "but qemu works like this; are they wrong
> too?".
> 
> To which the answer is apparently "no, they don't work like this."
> 
> Thanks for clearing that up.

Markus, thank you for clearing that up. I failed to distinguish
"enforcement" from "practice that is applied in 99.999% of the time".

David, it doesn't change anything relative to one of my earliest emails:

http://thread.gmane.org/gmane.comp.bios.edk2.devel/8864/focus=8889

I personally agreed to your proposal very early (and have repeated that
agreement a few times since), dependent on agreement from the other edk2
maintainers too. The linear history requirement is not mine in edk2. I
don't enforce it, I comply with it. In my "unkempt" guide, I relay that
requirement, don't dictate it. My explanation of it may not have been
entirely correct, yes. However, Jordan also told you that it is
temporary, while the edk2 people's git expertise matures.

If you want to gather feedback on immediately introducing a workflow to
edk2 that allows merges, please write a focused group email to the
maintainers listed in "Maintainers.txt". Some of them might feel better
about discussing this question, and/or feel more closely addressed, if
it doesn't happen on the list.

Going forward, please refrain from over-using your "cluebat" (e.g., the
tons of bold in your email). Discussing workflow is hard enough in its
own right, you don't need to make it harder by alienating people. Linus
gets away with management by perkele because he's a chief maintainer. In
this case, it's you who wants to achieve something, even if you position
it as "fixing the workflow for everyone". You are right about merging
(and I never denied that), but I do find myself struggling harder and
harder to open your next email.

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2016-03-10 12:39 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-09 10:20 [Qemu-devel] why restrict pull reqs to signed tags? Laszlo Ersek
2016-03-09 11:33 ` Paolo Bonzini
2016-03-09 11:35 ` Peter Maydell
2016-03-09 12:13   ` Laszlo Ersek
2016-03-09 12:19     ` Paolo Bonzini
2016-03-09 12:31       ` Laszlo Ersek
2016-03-09 12:33         ` Paolo Bonzini
2016-03-09 12:38         ` David Woodhouse
2016-03-09 12:40           ` Ard Biesheuvel
2016-03-09 12:44             ` Peter Maydell
2016-03-09 13:14               ` Laszlo Ersek
2016-03-09 12:34     ` David Woodhouse
2016-03-09 12:42       ` Peter Maydell
2016-03-09 13:09         ` David Woodhouse
2016-03-09 13:27           ` Peter Maydell
2016-03-09 14:13             ` David Woodhouse
2016-03-09 14:41               ` Laszlo Ersek
2016-03-10  8:21                 ` David Woodhouse
2016-03-10  8:52                   ` Markus Armbruster
2016-03-10 10:34                     ` David Woodhouse
2016-03-10 12:38                       ` Laszlo Ersek

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.