On Wed, Apr 26, 2017 at 08:14:31AM -0400, John Ferlan wrote:
Still for the most part, I would "hope" that an S-o-B could come with the implicit expectation that they've run "make check syntax-check". Similarly a reviewer would I think for a majority of what they review, apply the patches and at least build them. Many times when I have patches that I'm not sure will trip up Coverity - I'll run them through a test coverity build too, but I don't attribute that ever as that seems to draw unwanted attention sometimes...
In general S-o-B is /only/ about stating your compliance with the legal side of contribution rules. ie confirming you have the right to submit the patch. Whether they've complied with coding guidelines is tangential.
As for Reviewed-by - I do see that being something that's a bit more useful. Although how does one attribute R-b for trivial or build breaker patches?
S-o-B is considered to be a superset of Reviewed-by. The person who merges the patch to git master would add their own S-o-B line to indicate that they have the right to merge to it, and implicitly that they've reviewed it. IOW, you wouldn't have S-o-B and R-b for the same person on the same patch - just S-o-B is sufficient. This makes it trivial for the person merging the patch to git, since they simply can do git am <the patch> and then "git commit --amend -s" to add their S-o-B
Since they aren't "automagically added", that means adding them requires the Submittor to actually make the effort. So if that's then the case that every submit has to have them, how does that get enforced?
It would be possible to write a git hook that refused a patch unless the patch has a S-o-B from the person doing the push. Likewise the git hook can validate that the patch author has also proivded an S-o-B.
What happens if one forgets or one consistently doesn't provide the tags? Is their push privilege taken away? IOW, what's the penalty for not following the accepted community rule?
Again, I'm not against this, but sometimes getting *any* commit message for a patch is a struggle for some! Adding tags could be torturous ;-)
Dealing with S-o-B is trivial, since it just means adding -s flag to git. The other tags are more manual, but not that much work. The reviewed-by tags serve two purposes - one they give an indication to the subsystem maintainer that the code has a certain level of review and thus might be ready for merging, and two they give a historic record of who did what. In our model with much broader set of committers, I don't think the reviewed-by gives much benefit, so it just becomes a record of who did what - which is already something present in the mailing list. Personally I'd just keep things simple and only require a S-o-B from the patch author, and the person committing. If people want to add other tags fine, but I certaily wouldn't enforce anything other than S-o-B Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|