Re: [MirageOS-devel] [Xen-devel] [PATCH v2 4/6] Add Code Review Guide

[Lars Kurth <lars.kurth@xxxxxxxxxx>]

On 28/11/2019, 04:09, "Jan Beulich" <jbeulich@xxxxxxxx> wrote:

    On 28.11.2019 01:54, Stefano Stabellini wrote:
    > On Thu, 26 Sep 2019, Lars Kurth wrote:
    >> From: Lars Kurth <lars.kurth@xxxxxxxxxx>
    >>
    >> This document highlights what reviewers such as maintainers and 
committers look
    >> for when reviewing code. It sets expectations for code authors and 
provides
    >> a framework for code reviewers.
    > 
    > I think the document is missing a couple of things:
    > 
    > - a simple one line statement that possibly the most important thing in
    >   a code review is to indentify any bugs in the code
    > 
    > - an explanation that requests for major changes to the series should be
    >   made early on (i.e. let's not change the architecture of a feature at
    >   v9 if possible) I also made this comment in reply to patch #5. I'll
    >   let you decide where is the best place for it.
    
    This needs balancing. People crucial to the evaluation of a new
    feature and its implementation simply may not have the time to
    reply prior to v9. We've had situations where people posted new
    revisions every other day, sometimes even more than one per day.

I can certainly add something on the timing , along the lines of
* For complex series, consider the time it takes to do reviews (maybe with a 
guide of LOC per hour) and give reviewers enough time to
* For series with design issues or large questions, try and highlight the key 
open issues in cover letters clearly and solicit feedback from key maintainers 
who can comment on the open issue. The idea is to save both the contributor and 
the reviewers time by focussing on what needs to be resolved 
* Don’t repost a series, unless all review comments are addressed or the 
reviewers asked you to do so. The problem with this is that this is somewhat in 
conflict with the "let's focus on the core issues and not get distracted by 
details early on in a review cycle". In other words, this can only work, if 
reviewers focus on major issues in early reviews only and do not focus on 
style, coding standards, etc. As soon as a reviewer comes back with detailed 
feedback, the contributor will feel obliged to fix these. This creates a 
motivation to want to please the reviewer send out new versions of series 
fixing cosmetic issues without addressing the substantial issues, leading to 
what Jan describes. I am looking for opinions here.  
    
    As indicated in several other contexts before - imo people not
    helping to shoulder the review load should also not have the
    expectation that their (large) contributions will be looked at
    in due course. 
    
I can add something to this effect.  

Lars
    

_______________________________________________
MirageOS-devel mailing list
MirageOS-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/mirageos-devel