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

[Lars Kurth <lars.kurth@xxxxxxxxxx>]

On 06/12/2019, 09:51, "Jan Beulich" <jbeulich@xxxxxxxx> wrote:

    On 06.12.2019 00:41, Lars Kurth wrote:
    > I propose to add the following section to code-review-guide.md
    > 
    > ----
    > ## <a name="problems"></a>Problematic Patch Reviews
    > 
    > A typical waterfall software development process is sequential with the 
following 
    > steps: define requirements, analyse, design, code, test and deploy. 
Problems 
    > uncovered by code review or testing at such a late stage can cause costly 
redesign 
    > and delays. The principle of **[Shift 
Left](https://devopedia.org/shift-left)** is to take a 
    > task that is traditionally performed at a late stage in the process and 
perform that task 
    > at earlier stages. The goal is to save time by avoiding refactoring.
    > 
    > Typically, problematic patch reviews uncover issues such as wrong or 
missed 
    > assumptions, a problematic architecture or design, or other bugs that 
require 
    > significant re-implementation of a patch series to fix the issue.
    > 
    > The principle of **Shift Left** also applies in code reviews. Let's 
assume a series has
    > a major flaw: ideally, this flaw would be picked up in the **first or 
second iteration** of 
    > the code review. As significant parts of the code may have to be 
re-written, it does not 
    > make sense for reviewers to highlight minor issues (such as style issues) 
until major 
    > flaws have been addressed. By providing feedback on minor issues 
reviewers cause 
    > the code author and themselves extra work by asking for changes to code, 
which 
    > ultimately may be changed later.
    > 
    > The question then becomes, how do code reviewers identify major issues 
early? 
    > ----
    > This is where I really need help. Are there any tips and recommendations 
that we could give?
    > I can clearly highlight that we have RFC series, but in practice that 
does not solve the problem as RFCs don’t get prioritized
    > How do reviewers normally approach a series: do you a) take a big picture 
view first, or b) do most of you work through a series sequentially
    
    Afaic - depends heavily on the patch / series. I wouldn't typically
    peek ahead in a series, but it has happened. But as you say
    (elsewhere) the cover letter should put in place the "big picture".
    A series should generally be reviewable going from patch to patch,
    having the cover letter in mind.

I am wondering what others do. 

I think explaining the basic work-flow from the viewpoint of a reviewer and 
code author maybe in a separate section, which is not tied to the problem case 
would make sense. More input from other maintainers would be valuable. My 
gut-feel is that most reviewers "read and review" series sequentially, which 
has implications for the author. E.g.
- docs/design docs should be at the beginning of a series
- key header files or changes to them should be at the beginning of a series
- Etc
   
    > I then propose to change the following section in 
communication-practice.md
    > ----
    > ### Prioritize significant flaws
    > If a patch or patch series has significant flaws, such as
    > * It is built on wrong assumptions
    > * There are issues with the architecture or the design
    
    In such a case a full review of course doesn't make much sense. But
    this is far from the typical situation. Way more often you have some
    _part_ of a patch or series which has a bigger issue, but other
    parts are in need of no or just minor changes.

I know that this is an unusual situation. But it has happened in clusters 
frequently in the past.

I am wondering whether we should introduce some informal convention to mark 
_part_ of a series as problematic. A simple example of how to do this in the 
cover letter would do
    
    > it does not make sense to do a detailed code review. In such cases, it is 
best to
    > focus on the major issues first and deal with style and minor issues in a 
subsequent
    > review. Not all series have significant flaws, but most series have 
different classes of 
    > changes that are required for acceptance: covering a range of major code 
    > modifications to minor code style fixes. To avoid misunderstandings 
between 
    > reviewers and contributors, it is important to establish and agree 
whether a series or 
    > part of a series has a significant flaw and agree a course of action. 
    > 
    > A pragmatic approach would be to
    > * Highlight problematic portions of a series in the cover letter 
    > * For the patch author and reviewer(s) to agree that for problematic to 
omit style and
    > minor issues in the review, until the significant flaw is addressed
    > 
    > This saves both the patch author and reviewer(s) time. Note that some 
background
    > is covered in detail in [Problematic Patch 
Reviews](resolving-disagreement.md#problems).
    
    I have no issues with the suggested text in general, but I also don't
    think it makes much of a difference wrt what I had mentioned before.
    I guess part of the problem here is that there are things which imo
    you can't really give recipes for how to approach, if the expectation
    is that it would fit at least the vast majority of cases. 

I think the document covers most of the common cases, plus some areas which are 
problematic
* From a people-interaction point-of-view - in other words there could be 
unnecessary conflict, which is bad for the community but also wastes time
* From an efficient usage of time point-of-view

For example: the whole thing about thanking, appreciation, ... is something 
targeted at newcomers and a desire to treat them with more thought and 
awareness. 
Granted it takes more time to do a review with a newcomer, but it should make 
subsequent reviews easier 

It happens regularly, but not that frequently
  
    For code
    reviews this means that I don't think there should be any wording
    suggesting they should be done in a certain form; there may be wording
    suggesting they _could_ be done in a certain form (e.g. to help
    people not knowing at all how to get started).

That was definitely my intention. Maybe I have not succeeded in making this 
clear enough

Regards
Lars

    

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