On 03/08/16 19:09, David Woodhouse wrote: > On Tue, 2016-03-08 at 19:00 +0100, Laszlo Ersek wrote:
>> Or do you recommend that contributors be *allowed* to email pull >> requests (alongside their patches), and if they do, their pull requests >> be merged correctly? > > Exactly this. They should be *allowed*, and for large submissions it > should be *recommended* but not mandatory. I found this: http://wiki.qemu.org/Contribute/SubmitAPullRequest The parts I find especially interesting are: (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. (2) "All pull requests must be signed. [...] Key signing requires meeting another community member *in person* so please make appropriate arrangements." Also, quoting Documentation/SubmittingPatches from Linux: "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. [...] 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." Further links: - https://lists.gnu.org/archive/html/qemu-devel/2014-09/msg00136.html - https://www.kernel.org/pub/software/scm/git/docs/howto/using-signed-tag-in-pull-request.html 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). Requesting the pull of a signed tag does not prevent the attack, but it does ensure one thing: non-repudiation. Which in turn explains why signed tags *only* work if people meet *in person* at key signing parties. ... 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. (a) Contributors are encouraged to name the fork-off point (commit hash) of their topic branch, from which they post their patches. (b) For a large series, the maintainer is expected to apply the patches on a local, temporary branch, forked off of exactly the named commit of the master branch. Review tags are also added on this branch. (c) Maintainer merges topic into master locally. (d) Maintainer pushes the merge commit to his or her personal repo. (e) Submitter fetches maintainer's personal repo, and confirms (in email, stating the hash of the merge commit) that it's fine. (f) Maintainer pushes the merge commit (and its dependencies of course) to upstream master. Now, steps (d) and (e) add a round-trip that can make the merge commit prepared in step (c) obsolete, by the time the confirmation arrives. So perhaps we can drop (d) and (e). The point is, your history would be precisely reconstructed, and you could get to say the final OK. * 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. This would still depend on the maintainers to approve of a non-linear git history. Thanks Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
