[DISCUSS] Reviewers in commit message

[Ismael Juma <is...@juma.me.uk>]

Hi all,

As a general rule, we credit reviewers in the commit message. This is good.
However, it is not clear to me if there are guidelines on who should be
included as a reviewer (please correct me if I am wrong). I can think of a
few options:

   1. Anyone that commented on the patch (in the pull request or Review
   Board)
   2. The ones that have reviewed and approved the patch (+1, LGTM, Ship
   it, etc.)
   3. A more sophisticated system that differentiates between someone who
   reviews and approves a patch versus someone who simply comments on aspects
   of the patch [1]

On the surface, `1` seems appealing because it 's simple and credits people
who do partial reviews. The issue, however, is that people (including
myself) may not want to be tagged as a reviewer if they left a comment or
two, but didn't review the change properly. Option `2` is still simple and
it avoids this issue.

As such, I lean towards option `2`, although `3` would work for me too (the
additional complexity is the main downside).

Thoughts?

Best,
Ismael

[1] I don't think we should go this far, but the Linux Kernel is an extreme
example of this with `Signed-off-by`, `Acked-by`, `Cc`, `Reviewed-by`,
`Tested-by`, `Suggested-by`, `Reported-by`, `Fixes`, etc. More details in
their documentation:
https://www.kernel.org/doc/Documentation/SubmittingPatches

Re: [DISCUSS] Reviewers in commit message

[Ismael Juma <is...@juma.me.uk>]

On Thu, Jul 30, 2015 at 6:04 PM, Gwen Shapira <gs...@cloudera.com> wrote:

> I wasn't aware of this history, thanks for explaining!
>

No problem. :)

In most Apache projects I contributed to, the list of things that are
> stated in "reviewed by" are implied in a committer committing the
> patch. Reviewers are there to help the committer make the decision
> (thats why I sometimes mention "@ewencp please take a look and let me
> know what you think" in a JIRA), but the responsibility is on the
> committer, not the reviewers. Reviewing by community members is
> considered a valued contribution, and therefore "acknowledged" in the
> commit message (or sometimes in the JIRA).
>

Understood.

If the intent is to avoid surprises (which is a good idea), once we
> decide on how we do things we can clarify in the contributor guide
> (even if the decision is to keep current method, we should document
> it)
>

Excellent, this is indeed the main thing for me.

Best,
Ismael

Re: [DISCUSS] Reviewers in commit message

[Gwen Shapira <gs...@cloudera.com>]

I wasn't aware of this history, thanks for explaining!

In most Apache projects I contributed to, the list of things that are
stated in "reviewed by" are implied in a committer committing the
patch. Reviewers are there to help the committer make the decision
(thats why I sometimes mention "@ewencp please take a look and let me
know what you think" in a JIRA), but the responsibility is on the
committer, not the reviewers. Reviewing by community members is
considered a valued contribution, and therefore "acknowledged" in the
commit message (or sometimes in the JIRA).

If the intent is to avoid surprises (which is a good idea), once we
decide on how we do things we can clarify in the contributor guide
(even if the decision is to keep current method, we should document
it)



On Wed, Jul 29, 2015 at 5:33 PM, Ismael Juma <is...@juma.me.uk> wrote:
> On Wed, Jul 29, 2015 at 7:38 PM, Gwen Shapira <gs...@cloudera.com> wrote:
>
>> I guess we see the "reviewer" part with different interpretations.
>>
>
> Yes. As you know, Git was created for and initially used by the Linux
> Kernel. As such they were very influential in conventions, terminology and
> best practices. This is what their documentation states:
>
> "By offering my Reviewed-by: tag, I state that:
>
>   (a) I have carried out a technical review of this patch to
>      evaluate its appropriateness and readiness for inclusion into
>      the mainline kernel.
>
>  (b) Any problems, concerns, or questions relating to the patch
>      have been communicated back to the submitter.  I am satisfied
>      with the submitter's response to my comments.
>
>  (c) While there may be things that could be improved with this
>      submission, I believe that it is, at this time, (1) a
>      worthwhile modification to the kernel, and (2) free of known
>      issues which would argue against its inclusion.
>
>  (d) While I have reviewed the patch and believe it to be sound, I
>      do not (unless explicitly stated elsewhere) make any
>      warranties or guarantees that it will achieve its stated
>      purpose or function properly in any given situation."
>
> This is a common interpretation when the word reviewer or reviewed-by is
> used in a Git commit. If we mean something else, maybe it's better to use a
> different word.
>
> What are the benefits you see of formalizing who gets mentioned as reviewer?
>>
>
> * Consistency
> * Easier for new committers if there's a clear guideline
> * Avoid surprises for people who comment on PRs. Now that we are accepting
> pull requests via GitHub, it is more likely that people will comment on
> pull requests as they see it in their news feed (the repository has more
> than 300 watchers and this number is likely to rise); many/most of them
> won't be doing a proper review.
>
> In any case, if the committers don't think this is an issue, we can
> continue as it is and see if anyone else complains. Each community is
> different after all.
>
> Best,
> Ismael

Re: [DISCUSS] Reviewers in commit message

[Ismael Juma <is...@juma.me.uk>]

On Wed, Jul 29, 2015 at 7:38 PM, Gwen Shapira <gs...@cloudera.com> wrote:

> I guess we see the "reviewer" part with different interpretations.
>

Yes. As you know, Git was created for and initially used by the Linux
Kernel. As such they were very influential in conventions, terminology and
best practices. This is what their documentation states:

"By offering my Reviewed-by: tag, I state that:

  (a) I have carried out a technical review of this patch to
     evaluate its appropriateness and readiness for inclusion into
     the mainline kernel.

 (b) Any problems, concerns, or questions relating to the patch
     have been communicated back to the submitter.  I am satisfied
     with the submitter's response to my comments.

 (c) While there may be things that could be improved with this
     submission, I believe that it is, at this time, (1) a
     worthwhile modification to the kernel, and (2) free of known
     issues which would argue against its inclusion.

 (d) While I have reviewed the patch and believe it to be sound, I
     do not (unless explicitly stated elsewhere) make any
     warranties or guarantees that it will achieve its stated
     purpose or function properly in any given situation."

This is a common interpretation when the word reviewer or reviewed-by is
used in a Git commit. If we mean something else, maybe it's better to use a
different word.

What are the benefits you see of formalizing who gets mentioned as reviewer?
>

* Consistency
* Easier for new committers if there's a clear guideline
* Avoid surprises for people who comment on PRs. Now that we are accepting
pull requests via GitHub, it is more likely that people will comment on
pull requests as they see it in their news feed (the repository has more
than 300 watchers and this number is likely to rise); many/most of them
won't be doing a proper review.

In any case, if the committers don't think this is an issue, we can
continue as it is and see if anyone else complains. Each community is
different after all.

Best,
Ismael

Re: [DISCUSS] Reviewers in commit message

[Gwen Shapira <gs...@cloudera.com>]

I guess we see the "reviewer" part with different interpretations.

What are the benefits you see of formalizing who gets mentioned as reviewer?

On Wed, Jul 29, 2015 at 11:06 AM, Ismael Juma <is...@juma.me.uk> wrote:
> Hi Gwen,
>
> Thanks for the feedback. Comments below.
>
> On Wed, Jul 29, 2015 at 6:40 PM, Gwen Shapira <gs...@cloudera.com> wrote:
>
>> The jira comment is a way for the committer to say "thank you" to
>> people who were involved in the review process.
>
>
> If we just want to say thank you, then why not just say that then? Using
> the word "reviewers" in this context is unusual from my experience (and I
> am an obsessive reader of open-source commits :)).
>
> It doesn't have any
>> formal implications - the responsibility for committing good code is
>> on the committer (thats the whole point). It doesn't even have
>> informal implications - no one ever went after a reviewer if a code
>> turned out buggy.
>>
>
> Sure, it's not about going after people. We are nice around here. :) Still,
> correct attribution is important. Open-source code in GitHub is seen by
> many people and in various contexts.
>
> I suggest: Leave it up to the committer best judgement and not
>> introduce process where there's really no need for one.
>>
>
> Perhaps. Personally, I think we should consider what the contributors
> position too instead of just leaving it to the committer.
>
> Best,
> Ismael

Re: [DISCUSS] Reviewers in commit message

[Ismael Juma <is...@juma.me.uk>]

Hi Gwen,

Thanks for the feedback. Comments below.

On Wed, Jul 29, 2015 at 6:40 PM, Gwen Shapira <gs...@cloudera.com> wrote:

> The jira comment is a way for the committer to say "thank you" to
> people who were involved in the review process.


If we just want to say thank you, then why not just say that then? Using
the word "reviewers" in this context is unusual from my experience (and I
am an obsessive reader of open-source commits :)).

It doesn't have any
> formal implications - the responsibility for committing good code is
> on the committer (thats the whole point). It doesn't even have
> informal implications - no one ever went after a reviewer if a code
> turned out buggy.
>

Sure, it's not about going after people. We are nice around here. :) Still,
correct attribution is important. Open-source code in GitHub is seen by
many people and in various contexts.

I suggest: Leave it up to the committer best judgement and not
> introduce process where there's really no need for one.
>

Perhaps. Personally, I think we should consider what the contributors
position too instead of just leaving it to the committer.

Best,
Ismael

Re: [DISCUSS] Reviewers in commit message

[Ismael Juma <is...@juma.me.uk>]

Hi Parth,

On Wed, Jul 29, 2015 at 6:50 PM, Parth Brahmbhatt <
pbrahmbhatt@hortonworks.com> wrote:

> +1 on Gwen¹s suggestion.
>
> Consider this as my thank you for all the reviews everyone has done in
> past and are going to do in future. Don¹t make me say thanks on every
> single commit. Introducing another process when the project has > 50 PR
> open pretty much all the time is not really going to help.
>

I think you misunderstood. This is done by the person who merges the pull
request via the merge tool. You, as the creator of the PR, don't have to do
anything. Search for "Reviewers" in the following for example:

https://github.com/apache/kafka/commit/e43c9aff92c57da6abb0c1d0af3431a550110a89

It is already part of the process (Gwen actually asked for this to be added
to the merge tool):

https://issues.apache.org/jira/browse/KAFKA-2344

There is no new process being suggested. It's just a clarification on
something that already exists.

Best,
Ismael

Re: [DISCUSS] Reviewers in commit message

[Parth Brahmbhatt <pb...@hortonworks.com>]

+1 on Gwen¹s suggestion.

Consider this as my thank you for all the reviews everyone has done in
past and are going to do in future. Don¹t make me say thanks on every
single commit. Introducing another process when the project has > 50 PR
open pretty much all the time is not really going to help.

Thanks
Parth

On 7/29/15, 10:40 AM, "Gwen Shapira" <gs...@cloudera.com> wrote:

>My two cents:
>
>The jira comment is a way for the committer to say "thank you" to
>people who were involved in the review process. It doesn't have any
>formal implications - the responsibility for committing good code is
>on the committer (thats the whole point). It doesn't even have
>informal implications - no one ever went after a reviewer if a code
>turned out buggy.
>
>I suggest: Leave it up to the committer best judgement and not
>introduce process where there's really no need for one.
>
>Gwen
>
>On Wed, Jul 29, 2015 at 6:18 AM, Ismael Juma <is...@juma.me.uk> wrote:
>> Hi all,
>>
>> As a general rule, we credit reviewers in the commit message. This is
>>good.
>> However, it is not clear to me if there are guidelines on who should be
>> included as a reviewer (please correct me if I am wrong). I can think
>>of a
>> few options:
>>
>>    1. Anyone that commented on the patch (in the pull request or Review
>>    Board)
>>    2. The ones that have reviewed and approved the patch (+1, LGTM, Ship
>>    it, etc.)
>>    3. A more sophisticated system that differentiates between someone
>>who
>>    reviews and approves a patch versus someone who simply comments on
>>aspects
>>    of the patch [1]
>>
>> On the surface, `1` seems appealing because it 's simple and credits
>>people
>> who do partial reviews. The issue, however, is that people (including
>> myself) may not want to be tagged as a reviewer if they left a comment
>>or
>> two, but didn't review the change properly. Option `2` is still simple
>>and
>> it avoids this issue.
>>
>> As such, I lean towards option `2`, although `3` would work for me too
>>(the
>> additional complexity is the main downside).
>>
>> Thoughts?
>>
>> Best,
>> Ismael
>>
>> [1] I don't think we should go this far, but the Linux Kernel is an
>>extreme
>> example of this with `Signed-off-by`, `Acked-by`, `Cc`, `Reviewed-by`,
>> `Tested-by`, `Suggested-by`, `Reported-by`, `Fixes`, etc. More details
>>in
>> their documentation:
>> https://www.kernel.org/doc/Documentation/SubmittingPatches
>

Re: [DISCUSS] Reviewers in commit message

[Gwen Shapira <gs...@cloudera.com>]

My two cents:

The jira comment is a way for the committer to say "thank you" to
people who were involved in the review process. It doesn't have any
formal implications - the responsibility for committing good code is
on the committer (thats the whole point). It doesn't even have
informal implications - no one ever went after a reviewer if a code
turned out buggy.

I suggest: Leave it up to the committer best judgement and not
introduce process where there's really no need for one.

Gwen

On Wed, Jul 29, 2015 at 6:18 AM, Ismael Juma <is...@juma.me.uk> wrote:
> Hi all,
>
> As a general rule, we credit reviewers in the commit message. This is good.
> However, it is not clear to me if there are guidelines on who should be
> included as a reviewer (please correct me if I am wrong). I can think of a
> few options:
>
>    1. Anyone that commented on the patch (in the pull request or Review
>    Board)
>    2. The ones that have reviewed and approved the patch (+1, LGTM, Ship
>    it, etc.)
>    3. A more sophisticated system that differentiates between someone who
>    reviews and approves a patch versus someone who simply comments on aspects
>    of the patch [1]
>
> On the surface, `1` seems appealing because it 's simple and credits people
> who do partial reviews. The issue, however, is that people (including
> myself) may not want to be tagged as a reviewer if they left a comment or
> two, but didn't review the change properly. Option `2` is still simple and
> it avoids this issue.
>
> As such, I lean towards option `2`, although `3` would work for me too (the
> additional complexity is the main downside).
>
> Thoughts?
>
> Best,
> Ismael
>
> [1] I don't think we should go this far, but the Linux Kernel is an extreme
> example of this with `Signed-off-by`, `Acked-by`, `Cc`, `Reviewed-by`,
> `Tested-by`, `Suggested-by`, `Reported-by`, `Fixes`, etc. More details in
> their documentation:
> https://www.kernel.org/doc/Documentation/SubmittingPatches