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

[Lars Kurth <lars.kurth@xxxxxxxxxx>]

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

    On 28.11.2019 14:06, Lars Kurth wrote:
    > 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.
    
    But this doesn't make much sense either, because then full re-reviews
    need to happen anyway on later versions, to also deal with the minor
    issues. For RFC kind of series omitting style and alike feedback
    certainly makes sense, but as soon as a patch is non-RFC, it should
    be considered good to go in by the submitter.
    
OK, I think we have a disconnect between ideal and reality. 

I see two issues today
* Key maintainers don't always review RFC series [they end up at the bottom of 
the priority list, even though spending time on RFCs will save time elsewhere 
later]. So the effect is that then the contributor assumes there are no major 
issues and ends it as a proper series
* In practice what has happened often in the past is that design, architecture, 
assumption flaws are found in early versions of a series.
   - This usually happens because of an oversight or because there was no 
design discussion prior to the series being posted and agreed
   - Common sense would dictate that the biggest benefit for both the reviewer, 
the contributor and the community as a whole would be to try and focus on such 
flaws and leave everything aside
   - Of course there may be value in doing a detailed reviews of such a series 
as there may be bits that are unaffected by such a flaw
   - But there will likely be parts which are not: doing a detailed review of 
such portions wastes everyone's time

So coming back to your point. Ideally, it would be nice if we had the 
capability to call out parts of a series as "problematic" and treating such 
parts differently

Lars

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