KS2011: Patch review [LWN.net]

KS2011: Patch review

Posted Oct 25, 2011 0:59 UTC (Tue) by neilbrown (subscriber, #359)
Parent article: KS2011: Patch review

> That led to some discussion of the relative meaning and merit of Reviewed-by and Signed-off-by. The latter is a much stronger statement in the end, and, it was agreed, carries more weight.

This makes no sense to me. As defined in SubmittingPatches, the Signed-off-by: line says nothing about the quality of the patch, only the origin.
It represents a "Developer's Certificate of Origin" and is meant to address questions of copyright, not questions of value.

So it seems to me that if the original statement is true, then people are interpreting SOB wrongly.

SubmittingPatches gives a "Reviewer's statement of oversight" which defines what Reviewed-by: is meant to mean, and it sounds like it should carry a whole lot more weight...

Do we need to revise SubmittingPatches ??

(Log in to post comments)

KS2011: Patch review

Posted Oct 26, 2011 3:32 UTC (Wed) by tytso (subscriber, #9993) [Link]

Let's consider when a Signed-off-by: should appear.

(1) when a patch is written by the original author and submitted for consideration. At that point, the author had better be convinced that it is Correct and Sane, since he or she is putting their name and reputation on the line with that patch. In some cases patches are submitted for comments, and in that case sometimes the signed-off-by line is not present (because the author knows some changes might be necessary), or to make the point that the patch really isn't suitable for inclusion, might even have a NOT-Signed-off-by: line to make that point clear.

(2) when the patch is copied from e-mail into a git repository. If the person with the git repository then sends a pull request to Linus, it had better mean that she has put her name and reputation on the line, and is confident that the patch is good. It may not mean that the person has reviewed it; it may be that he is depending on his reviewers or sub-maintainers to have done a good job. But ultimately, it is the maintainer's reputation which is on the line. If the maintainer isn't sure about a sub-maintainer's competence in review and good taste, then she should be doing spot checks or even review every single commit before accepting a pull from the sub-maintainer --- or not stop pulling form the sub-maintainer altogether.

This also implies that in practice, a commit should generally only have two signed-off-by lines. There is an exception, which is if there is a sub-maintainer who isn't using git, so a patch gets forwarded outside of git an additional time. An example of this Andrew Morton's mm tree, which is maintained as a quilt patch series outside of git. But in general, patches are reviewed on e-mail, and once they hit git, they're not going to accumulate any more signed-off-by lines. As those git commits get merged into more senior trees, the maintainer who accepts the pull request is effectively putting their imprimatur on them. So they had better have reviewed the patch, or be willing to put their reputation on the line based on their trust of their sub-maintainers.