5 ms·
The article puts forward a false dilemma by implying your only choices are (a) squashing all of the commits into a single patch, (b) examining the end point and
by tytso 14y ago
The article puts forward a false dilemma by implying your only choices are (a) squashing all of the commits into a single patch, (b) examining the end point and then only signing the end point (with the risk that there might be some bad stuff hidden in some of the intermediate commits), and (c) examining and signing every single commit.
In fact what we do in the kernel community is most patches are in fact reviewed via e-mail. This increases the likelihood more people will look at the patch and notice any potential problem, and more importantly, the e-mail archive introduces an additional external record of the proposed change. After each patch in a patch series is reviewed, in isolation, with each change small enough that it can be easily reviewed in a mail reader, and with especial attention made to maintaining bisectability, the maintainer who will ultimately by signing the git pull request will apply it into his or her tree.
For larger changes and for larger subsystems where we have submaintainers, it is the maintainer's responsibility to make sure the submaintainer sends a signed pull request. At that point the submaintainer is assuming full responsibility for the set of patches which he or she is sending to the upper-level maintainer, just as the maintainer assumes responsibility for the patches he or she sends to Linus.
In the case where the submaintainer does not send a signed pull, or the maintainer does not have full trust in the submaintainer (either for social or technical reasons), it is the responsibility for the maintainer to check each and every commit for correctness. If he or she can't do that, he shouldn't accept the pull request; instead, he might request that the patch series be sent to the mailing list so everyone can review the patch series.
One of the things which follows from this is that for me as a kernel or e2fsprogs maintainer, github pull requests are not particularly useful. Most of the time I have no idea who the person is that initiated the pull request, because the primary community is on the mailing list. Worse, the pull request only gets sent to maintainer, which makes the maintainer the review bottleneck. If the patch series is sent to the mailing list, as individual commits, with no requirement to jump to a browser, it allows the review workload to be distributed to other developers on the mailing list.
As far as I'm concerned, a github pull request is the same thing as a patch sent via private, personal e-mail. I'm going to review it very carefully, and only apply it if I know that it is clean. Typically the review involves extracting the commit as a diff, and as has been discussed in other places, github doesn't encourage properly formatted commit descriptions, so I end up needing to extract the diff, and then reapply it with an appropriate commit description. So for me github pull requests generally represent negative, not positive value, because of the quality control and quality assurance requirements of the code bases in which I work.
- mikegerwitz 14y agoThe style of review you mentioned (which I made a brief mention of in the article) still falls within the three merge options. In the case above, maintainers seem to be doing option b or c, depending on trust. Instead of signing the merge commit, the pull request is signed. One of the downsides to this is that it does not extend the signature to the repository and requires the users to consult the mailing list. That said, I'm rather fond of the Linux and Git style of code review and would recommend it in addition to what was recommended in the article (with the former only being necessary if its benefits are desirable). Thank you for the detailed explanation; it did help to clarify details I assumed, but was uncertain of.
- tytso 14y agoNo, there's no need to consult the mailing list, except if you want to see the "legislative history" of the review process. The person requesting the pull signs the pull request by signing the git tag. The GPG signature on the git tag covers the complete history of up to that point in the tag, so there's no need to sign each and every git commit, as you propose. When the upstream maintainer (or Linus, at the top level) accepts the pull request, git will automatically create a merge request that contains a copy of the git tag's GPG signature in the merge request. (After this point, the git tag can be discarded). When Linus releases something, he signs a git tag for the overall git repository, and everyone can check that. So for example, when I am satisfied that everything in the ext4 tree that I want to push to Linus is OK, I create a signed tag named "ext4_for_linus", and send him a git pull request, which is created using the "git request-pull" command. This creates an e-mail message contains the URL of my git repository and the name of the signed tag. When Linus accepts my pull request, the resulting merge commit will contain a copy of my GPG signature from the git tag. This signature signs my name to the commit id of the git tag, and the chain of SHA-1 hashes inherent in git guarantees that the entire commits back to the beginning of time came from me. Of course, my changes will branch of something like v3.3-rc2, which is in Linus's tree, so what is being signed that is of interest is my new changes between v3.3-rc2 and the tip of the ext4 tree. Once Linus has accepted my pull, I'll reuse the ext4_for_linus name for the next GPG-signed tag for the next 2-3 month development cycle, since the important bit (the GPG signature) is preserved for long-term posterity in the merge commit. But what's important to note here is that the signature in the merge commit is signed by me, since it contains the contents of what I pushed to Linus, even though the merge commit was created by Linus when he accepted the pull request by merging in my changes into the repository. What Linus signs is the GPG tag for the actual released kernel (i.e., v3.3). See? So there's no need to refer to the mailing list to validate the pull request or to establish after-the-fact accountability. The mailing list is just used for review purposes. But of course the review is a critical part of the acceptance process.