You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hbase.apache.org by Josh Elser <el...@apache.org> on 2020/11/20 17:19:34 UTC
[DISCUSS] Clarifying guidance around Signed-off-by in commit messages
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)
* 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
Re: [DISCUSS] Clarifying guidance around Signed-off-by in commit
messages
Posted by Josh Elser <el...@apache.org>.
Yeah, that's the intent of what Bharath had suggested and I liked. In
parallel, see other part of thread from Yu and Nick.
On 11/21/20 11:31 AM, Reid Chan wrote:
> Does that mean:
> Signed-off-by for binding +1 (from committer),
> Reviewed-by for non-binding +1 (from volunteer)?
>
> Sounds good to me.
>
>
>
>
>
> --------------------------
>
> Best regards,
> R.C
>
>
>
> ________________________________________
> From: Jan Hentschel <ja...@ultratendency.com>
> Sent: 21 November 2020 19:37
> To: dev@hbase.apache.org
> Subject: Re: [DISCUSS] Clarifying guidance around Signed-off-by in commit messages
>
> Also +1 for both suggestions as long as it is clear when to use which. Starting point (after the discussion) probably would be to include it in our ref guide.
>
> From: Wellington Chevreuil <we...@gmail.com>
> Reply-To: "dev@hbase.apache.org" <de...@hbase.apache.org>
> Date: Saturday, November 21, 2020 at 11:37 AM
> To: dev <de...@hbase.apache.org>
> Subject: Re: [DISCUSS] Clarifying guidance around Signed-off-by in commit messages
>
> +1 for both suggestions ('Signed-off-by' and 'Reviewed-by');
>
> Em sáb., 21 de nov. de 2020 às 00:15, Stack <st...@duboce.net>> escreveu:
>
> 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 <el...@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<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!
>>
>
>
Re: [DISCUSS] Clarifying guidance around Signed-off-by in commit
messages
Posted by Reid Chan <re...@outlook.com>.
Does that mean:
Signed-off-by for binding +1 (from committer),
Reviewed-by for non-binding +1 (from volunteer)?
Sounds good to me.
--------------------------
Best regards,
R.C
________________________________________
From: Jan Hentschel <ja...@ultratendency.com>
Sent: 21 November 2020 19:37
To: dev@hbase.apache.org
Subject: Re: [DISCUSS] Clarifying guidance around Signed-off-by in commit messages
Also +1 for both suggestions as long as it is clear when to use which. Starting point (after the discussion) probably would be to include it in our ref guide.
From: Wellington Chevreuil <we...@gmail.com>
Reply-To: "dev@hbase.apache.org" <de...@hbase.apache.org>
Date: Saturday, November 21, 2020 at 11:37 AM
To: dev <de...@hbase.apache.org>
Subject: Re: [DISCUSS] Clarifying guidance around Signed-off-by in commit messages
+1 for both suggestions ('Signed-off-by' and 'Reviewed-by');
Em sáb., 21 de nov. de 2020 às 00:15, Stack <st...@duboce.net>> escreveu:
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 <el...@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<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!
>
Re: [DISCUSS] Clarifying guidance around Signed-off-by in commit
messages
Posted by Jan Hentschel <ja...@ultratendency.com>.
Also +1 for both suggestions as long as it is clear when to use which. Starting point (after the discussion) probably would be to include it in our ref guide.
From: Wellington Chevreuil <we...@gmail.com>
Reply-To: "dev@hbase.apache.org" <de...@hbase.apache.org>
Date: Saturday, November 21, 2020 at 11:37 AM
To: dev <de...@hbase.apache.org>
Subject: Re: [DISCUSS] Clarifying guidance around Signed-off-by in commit messages
+1 for both suggestions ('Signed-off-by' and 'Reviewed-by');
Em sáb., 21 de nov. de 2020 às 00:15, Stack <st...@duboce.net>> escreveu:
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 <el...@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<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!
>
Re: [DISCUSS] Clarifying guidance around Signed-off-by in commit messages
Posted by Wellington Chevreuil <we...@gmail.com>.
+1 for both suggestions ('Signed-off-by' and 'Reviewed-by');
Em sáb., 21 de nov. de 2020 às 00:15, Stack <st...@duboce.net> escreveu:
> 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 <el...@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!
> >
>
Re: [DISCUSS] Clarifying guidance around Signed-off-by in commit
messages
Posted by Josh Elser <el...@apache.org>.
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 <ca...@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 <bu...@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 <el...@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!
>>>>>
>>>>
>>>
>>
>
Re: [DISCUSS] Clarifying guidance around Signed-off-by in commit messages
Posted by Nick Dimiduk <nd...@apache.org>.
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 <ca...@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 <bu...@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 <el...@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!
> > > >
> > >
> >
>
Re: [DISCUSS] Clarifying guidance around Signed-off-by in commit messages
Posted by Yu Li <ca...@gmail.com>.
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 <bu...@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 <el...@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!
> > >
> >
>
Re: [DISCUSS] Clarifying guidance around Signed-off-by in commit messages
Posted by Sean Busbey <bu...@apache.org>.
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 <el...@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!
> >
>
Re: [DISCUSS] Clarifying guidance around Signed-off-by in commit messages
Posted by Stack <st...@duboce.net>.
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 <el...@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!
>
Re: [DISCUSS] Clarifying guidance around Signed-off-by in commit
messages
Posted by Josh Elser <el...@apache.org>.
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!
Re: [DISCUSS] Clarifying guidance around Signed-off-by in commit messages
Posted by Bharath Vissapragada <bh...@apache.org>.
Makes sense to me except a nit (inline).
On Fri, Nov 20, 2020 at 9:20 AM Josh Elser <el...@apache.org> 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
>