Review process improvements: drafting a ReviewingPatches doc

calvinwan@google.com· Feb 10, 2022, 23:33 UTC · lore

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/

Junio C Hamano· Feb 11, 2022, 17:28 UTC · re: calvinwan@google.com · lore

Re: Review process improvements: drafting a ReviewingPatches doc

calvinwan@google.com writes:

Show 7 quoted lines

> 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.

Calvin Wan· Feb 11, 2022, 19:44 UTC · re: Junio C Hamano · lore

Re: Review process improvements: drafting a ReviewingPatches doc

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:

Show 21 quoted lines

>
> 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.