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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

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

Jan

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