Nice discussion here. For my part, I am +1 for our community to define our meaning around this aspect of metadata.
However, I don't like using both "signed-off-by" and "reviewed-by" as a manual annotation on the part of the committer, because we as a community don't care about the distinction between a committer and a community member in this context. I think that enforcing correct usage of two metadata annotations, without automation, will be error-prone. If we had some plan to make use of this metadata, maybe it's worth it, but so far I see no concrete plan to use this information. So why increase the burden on committers? On Sun, Nov 22, 2020 at 11:41 PM Yu Li <car...@gmail.com> wrote: > TL;DR: +1 for document rules / guidance of review trailers in commit > message, and +1 for continuing using the signed-off-by message for > "reviewed by" and/or "co-authored-by" semantic (committers only), adding > explicit preamble in the "Git best practice" chapter in our hbase book [1]. > > I did some research around signed-off-by [2] [3], reviewed-by [3] and > co-authored-by [4], and would like to share my thoughts here: > > 1. We have been using signed-off-by as the "reviewed by" and/or > "co-authored by" semantic for a long time, starting from the review-board > era (long before github PR). > 2. I second that our usage of signed-off-by is a bit of a perversion of the > original [2], thus adding preamble as clarification is necessary. > 3. Git offers a signed-off-by switch (-s/--signoff) while no reviewed-by or > co-authored-by support yet, so we need to manually type the message if > choose to use Reviewed-by or Co-authored-by trailers, which means > additional efforts. > 4. Based on #3, I suggest that contributors / committers are free but not > required to add "Reviewed-by" and / or "Co-authored-by" trailers manually. > 5. Regarding recognizing the review efforts of (new) non-committer > contributors, I suggest we use the Github search [5] (and the commit > efforts as well [6]). > > Best Regards, > Yu > > [1] http://hbase.apache.org/book.html#git.best.practices > [2] > > https://stackoverflow.com/questions/1962094/what-is-the-sign-off-feature-in-git-for > [3] https://wiki.samba.org/index.php/CodeReview#commit_message_tags > [4] > > https://docs.github.com/en/free-pro-team@latest/github/committing-changes-to-your-project/creating-a-commit-with-multiple-authors > [5] https://github.com/apache/hbase/pulls?q=is%3Apr+involves%3Acarp84 > [6] https://github.com/apache/hbase/commits?author=carp84 > > On Mon, 23 Nov 2020 at 04:06, Sean Busbey <bus...@apache.org> wrote: > > > I expressly would like to see non-commiters given credit for reviews and > > have made a point of including them in prior commits for signed-off-by to > > do that. > > > > I'm fine with the idea of us using some other means to indicate this, but > > I'd like us to make sure there's not some already widely used bit of git > > metadata we could use before picking our own. > > > > It's kind of like when we moved away from amending author (I think that > was > > the phrase?) To co authored by when github started pushing that as a way > to > > show multiple authors on a commit. > > > > One thing to keep in mind also is that a big stumbling block to our > > consistent crediting of reviewers is a lack of tooling. Having to > > distinguish between binding and non binding reviews for putting together > > commit metadata will make that more complicated. > > > > On Fri, Nov 20, 2020, 18:15 Stack <st...@duboce.net> wrote: > > > > > Thanks for taking the time to do a write up Josh. > > > > > > Looks good to me. > > > > > > When Sean started in on the 'Signed-off-by:' I didn't get it > (especially > > > after reading the git definition). Sean then set me straight explaining > > our > > > use is a bit of a perversion of the original. I notice his definition > is > > > not in the refguide. Suggest a sentence preamble definition of > > > 'Signed-off-by:' and that we intentionally are different from the > > > definition cited by Bharath. > > > > > > I like the Bharath idea on 'Reviewed-by' too. We can talk up > > 'Reviewed-by' > > > credits as a way to earn standing in the community, of how they are > given > > > weight evaluating whether to make a candidate a committer/PMC'er or > not. > > > > > > S > > > > > > On Fri, Nov 20, 2020 at 3:13 PM Josh Elser <els...@apache.org> wrote: > > > > > > > On 11/20/20 1:07 PM, Bharath Vissapragada wrote: > > > > >> * 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? > > > > > > > > I would be happy with distinguishing Signed-off-by and Reviewed-by > as a > > > > way to better track metrics on contributors who review others' code. > > > > > > > > Great idea! > > > > > > > > > >
