Review process improvements: drafting a ReviewingPatches doc

Review process improvements: drafting a ReviewingPatches doc

[calvinwan]

Hi all,

In continuation of Emily’s "Review process improvements"[1], I am
drafting a ReviewingPatches doc to help standardize the review process
here on the git mailing list and to provide a rubric/checklist for new
reviewers. In order to do so, I am looking to compile examples and
input from everyone by asking the list a set of questions biweekly.
Please contextualize your answer in terms of whether this was a
maintainer, individual, or drive-by review.

Maintainer: in the context of the overall project, is this series a
good idea? Does it interact well with other features? Is it
maintainable in the long term?

Individual: in the context of the individual's own experience, is this
series a good idea? Is the implementation readable, correct, and
efficient?

Drive-by / nitpicks: does not answer whether the series is a good
idea. Makes suggestions for missing pieces, bugs, and/or code style.

Here are the first set of questions I had in mind:

What is the best review(s) that you have ever received? What was
especially constructive about this review and do you feel like it
could be improved upon or is missing something?  Conversely, what is
the best review(s) that you have ever given? Feel free to leave
examples for each of the review types.

[1] https://lore.kernel.org/git/YbvBvch8JcHED+A9@google.com/

Re: Review process improvements: drafting a ReviewingPatches doc

[Junio C Hamano]

calvinwan@google.com writes:

> In continuation of Emily’s "Review process improvements"[1], I am
> drafting a ReviewingPatches doc to help standardize the review process
> here on the git mailing list and to provide a rubric/checklist for new
> reviewers. In order to do so, I am looking to compile examples and
> input from everyone by asking the list a set of questions biweekly.
> Please contextualize your answer in terms of whether this was a
> maintainer, individual, or drive-by review.

Often people other than the patch contributors themselves find
others' reviews a useful learning opportunity.  I take it that your
"contextualize" request is asking for comments like "As the
maintainer, I found this review by an area expert very helpful
because ...", as well as "I sent this patch and a drive-by typofix
at this URL was not very helpful"?

When soliciting input from the list, it would probably make it
easier for others if you led by example by sending your answers,
to show the level of detail you'd find useful.

Re: Review process improvements: drafting a ReviewingPatches doc

[Calvin Wan]

Thank you for the suggestion Junio!

I will start us off by showcasing an individual review by Jonathan Tan
I received recently[1]. Overall, I appreciate how thorough Jonathan
was with his review. It covers everything from overall architecture of
the code to small stylistic nuances in the commit message. The review
can be generally separated into two sections: one that goes line by
line through the entire changeset and suggests possible improvements,
and then another section that summarizes the overall direction the
patch author should take to move the patch into a mergeable state.

For an individual review, I am glad the following was included in the review:
1. Stylistic improvement for the commit message
2. Suggestions for clarifying the patch context
3. Rule for referencing merged code
4. Variable name correctness
5. Showcases where certain functions/variables/classes should belong
for better structure
6. Suggestions for code that can be refactored
7. Makes considerations about edge cases
8. Determines whether the test cases are sufficient and if not,
suggests new test cases
9. Stylistic improvements of the test cases
10. Suggestions for overall architecture of the patch

Some items I think that were missing:
1. The review does not answer whether the overall idea of this patch
is worth merging. (There is some loss of context here since internally
here at Google this feature has been deemed worthy and the legwork for
the server side of this patch has already been merged up).
2. Jonathan jumps straight into the review. A brief summary at the
beginning to bring context and to tell the patch author what he/she
did well would be nice.

I do many things wrong in my patch, but at no point is Jonathan
demeaning towards me. He assumes the best intentions, and makes his
best effort to help me improve my patch. To be able to clearly see the
path forward and to see my patch welcomed on the list motivates me to
continue working on this patch.

[1] https://lore.kernel.org/git/CAFySSZCZfekrdBH1NMArgPLdF4o1KQVY251BLyFgRxp=pDgEOA@mail.gmail.com/T/#m5a033fd28089d41befc937e8adbb133874eee528





On Fri, Feb 11, 2022 at 9:28 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> calvinwan@google.com writes:
>
> > In continuation of Emily’s "Review process improvements"[1], I am
> > drafting a ReviewingPatches doc to help standardize the review process
> > here on the git mailing list and to provide a rubric/checklist for new
> > reviewers. In order to do so, I am looking to compile examples and
> > input from everyone by asking the list a set of questions biweekly.
> > Please contextualize your answer in terms of whether this was a
> > maintainer, individual, or drive-by review.
>
> Often people other than the patch contributors themselves find
> others' reviews a useful learning opportunity.  I take it that your
> "contextualize" request is asking for comments like "As the
> maintainer, I found this review by an area expert very helpful
> because ...", as well as "I sent this patch and a drive-by typofix
> at this URL was not very helpful"?
>
> When soliciting input from the list, it would probably make it
> easier for others if you led by example by sending your answers,
> to show the level of detail you'd find useful.