The ask of this thread was originally to change the semantics of
"signed-off-by" to only be "A committer who gave an explicit +1". That
was the ask from a community member and why I started this.
I want to tease this apart from the "reviewed-by" suggestion, as this
obviously needs a little more polishing. Specifically, this would change
what you had stated as what we "don't care about" today -- a committer
or community member can (today) be listed as the target of a Signed-off-by.
Are you OK with just the change of who can be listed in Signed-off-by,
Nick, while we continue to circle around on how to ensure contributor
reviews recognize merit? I would agree that we need to be sure that we
have the tooling in place to ensure that merit is assigned equally
(especially in a pull-request-centric world we now live in).
On 11/30/20 2:16 PM, Nick Dimiduk wrote:
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!
