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@xxxxxxxxxxxxxx/T/#m5a033fd28089d41befc937e8adbb133874eee528 On Fri, Feb 11, 2022 at 9:28 AM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > calvinwan@xxxxxxxxxx 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.
