Makes sense to me except a nit (inline). On Fri, Nov 20, 2020 at 9:20 AM Josh Elser <[email protected]> wrote:
> Hi! > > As most of you know, we've been using the "Signed-off-by: <name> > <email>" line in out commit messages more and more lately to indicate > who reviewed some change. > > We've recently had an event in which one of these Signed-off-by lines > showed up with someone's name who didn't consider themselves to have > signed-off on the change. This is akin to saying someone gave a +1 for > some change when they did not. As an RTC community, that's worrisome. > > I went reading the HBase book and was surprised to not find guidance on > how we expect this to work, so I'd like to have some discussion about > how we should treat these lines. I'll start this off by making > suggestions about what seems reasonable to me. > > When a committer is applying some change in a commit: > > * All individuals mentioned in a sign-off *must* be capable of giving a > binding vote (i.e. they are an HBase committer) > It appears that the original intent <http://web.archive.org/web/20160507011446/http://gerrit.googlecode.com/svn/documentation/2.0/user-signedoffby.html>of this sign-off feature in git mandates that the signing-off party to be a maintainer. So agree with you in theory. However, most times non-committers also give great feedback and help with the code review process (code reviews, testing, perf etc). I think acknowledging their contribution in some form would be nice and that encourages potential-future-committers to actively review PRs IMO. So how about we annotate their names with Reviewed-by tags? A related discussion <https://lists.x.org/archives/xorg-devel/2009-October/003036.html> on a different open source project has more tag definitions if we are interested in taking that route. (I know you are only talking about the "signed-off by" tag but I thought this discussion would be relevant when documenting this in the dev guidelines, hence bringing it up). What do you think? > * Any individual in a sign-off *must* have given approval via an > explicit "+1" or the "Approved" via the Github Pull Request review > function. > * Approval *must* be publicly visible and memorialized on the code > review (e.g. no private emails or chat message to give approval) > * The committer _should_ (not *must*) create a sign-off line for each > binding reviewer who gave approval > > I think these are generally how we have been operating, but it would be > good to make sure they are documented as such. > > Thoughts/concerns? > > - Josh >
