On 03/09/16 00:37, David Woodhouse wrote:
> On Wed, 2016-03-09 at 00:05 +0100, Laszlo Ersek wrote:
>> (1) The submitter is himself/herself responsible for picking up review
>> tags, and then for posting a final (fully reviewed) PULL that can be
>> merged without *any* kind of rebase by the pulling maintainer.
>>
>> Corollary: since the first submission never has any reviews, the first
>> submission should never be a PULL.
>
> If submissions *require* review, the first submission isn't intended to
> be merged anyway. So it's meaningless to say *either* that the first
> submission is to be merged by applying patches, *or* that it's to be
> merged as a pull. This is fairly much a non-sequitur.
I disagree. A v1 can be (and very frequently is) applied from patches,
if it passes review at the first try. When the patches are picked up
from the list, and all patches get R-b's, then it's the maintainer that
adds the tags, so the contributor need not do anything else.
Whereas, for a purely merge-oriented workflow, even if the first
submission passed review fully, it would be the submitter's
responsibility to append the tags, and send a separate PULL with that
version of the series.
>> It seems to me that the extra trust for a pull request is needed because
>> it's possible to post patches to the list (for review) that *differ*
>> from the commits that *actually* lead up to the hash that is presented
>> in the pull request. An attacker can prepare two patch sets, an innocent
>> and a malicious one; post one series for review, but include the final
>> commit hash of the other series in the pull request.
>>
>> Whereas when the maintainer applies patches from the list (i.e., at that
>> point: from his own mailbox) directly, then the maintainer can be
>> (reasonably) sure that those are exactly the patches the community
>> reviewed (not counting mailbox break-ins etc).
>
> Nonsense. If I go through seven rounds of posting patches to the list,
> and I gather a series of acks from the first six but you apply the
> seventh, you have *no* good reason to believe that my final submission
> doesn't contain the trojan horse. You have to *look* before you apply
> it.
>
> And it's *just* the same if my final submission is actually in the form
> of a pull request. Sure, the signed tag means you know it comes from
> *me*, which can be achieved by signed email too (like this one). But
> you still have to trust *me* just the same.
>
> Or actually do a final review on the code you're actually *merging*.
> Whether it be in email patches or in a pull request, that's just the
> same.
>
> Really, from the trust point of view there is *not* a difference
> between pull requests and patches. Look at what you merge, or trust the
> submitter. It's as simple as that.
I was only trying to reconstruct the argument for this claim, from the
kernel docs:
Note, however, that pulling patches from a developer requires a
higher degree of trust than taking patches from a mailing list.
It is perfectly possible that my attempt to retrofit a security argument
to this claim failed. But, in that case, can you please explain why the
above claim is valid? Or are you suggesting that this statement of
SubmittingPatches is bogus?
>> ... So, I dunno what to do about this. I trust you, but I wouldn't want
>> to open up the possibility of *any* maintainer pulling from *any*
>> contributor.
>>
>> * How about an alternative.
> No, this is pointless complexity.
>
> Again, just *look* at what you merge. Whether it means a proper read
> through the seventh round of patches, or actually looking at the
> contents of a pull request, there's *no* difference.
I'm not saying you are wrong.
For several versions of the same patchset, I have a good workflow where
I can incrementally review only the new stuff (the changes), without
fully trusting the submitter. I compare the patch emails as well between
each other, and I can git-diff the end results of the various versions.
The same seems possible with branches that are to be merged, sure, it's
just that I don't yet have hands-on experience with that.
>> * There is another alternative. You should become an official
>> co-maintainer of CryptoPkg (using your Intel email address), and then
>> you could push your merges directly, after review. As I said, I trust
>> you, but I don't trust a setup where any maintainer can pull from any
>> contributor.
>
> That's a band-aid for the one issue. I'm trying to make things better
> for *everyone*.
Thank you, but thus far noone has complained as vehemently as you about
rebasing as a norm (for now), for the sake of a linear history. I've had
series with 50+ patches, and I don't recall any rebase-incurred problems.
I don't want to dismiss this endeavor, but with QEMU and top Linux
maintainers insisting on signed tag pulls, and you saying "nonsense" to
it (or to my attempt anyway to interpret that requirement), I feel much
less confident about a pull-based workflow.
Laszlo
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
