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

[Lars Kurth <lars.kurth.xen@xxxxxxxxx>]

On 9 Dec 2019, at 11:02, Lars Kurth <lars.kurth@xxxxxxxxxx> wrote:

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

Adding the log of a very productive IRC conversation for reference. Sorry for the formatting

Regards

Lars

‹lars_kurth›     It would also be good if some maintainers could have a look at https://lists.xenproject.org/archives/html/xen-devel/2019-12/threads.html#00348

‹gwd› lars_kurth: What exactly more feedback did you want?

‹lars_kurth›     gwd: primarily about maintainers workflow. How do you approach a review. Is there anything a code author can do. Right now, I have one data point from Jan. I basically want to validate my assumptions before I send out another revision

‹lars_kurth›     gwd: a code author can do to make your life easier => e.g. by putting big picture stuff at the beginning of a series

‹lars_kurth›     e.g by putting potentially controversial elements of it at the beginning, etc

‹lars_kurth›     Obviously, this would only help if most reviewers approach a review sequentially, which is a reasonable assumption, but I am not actually 100% sure this is true

‹royger›           lars_kurth: patch series must be reviewed sequentially, that's why it's numbered

‹gwd› lars_kurth: Actually it's often the case that you find uncontroversial side clean-up things in the course of doing a series; putting those at the beginning means they can be checked in independently of the whole series, which makes less work for everyone

‹gwd› They can be checked in after v1 or v2, and then 1) less work for authors to rebase, 2) less cognitive overhead for reviewers to see what's going on.

‹lars_kurth›     royger: That is not a necessarily obvious conclusion - at least for me. But if it is true, we should clearly state that

‹lars_kurth›     gwd: that's interesting. I guess there are differing objectives

‹royger›           I don't think any maintainer/reviewer does review non-sequentially, as it would lead to extreme confusion. It's quite common for patches on a patch series to rely on the previous ones

‹royger›           lars_kurth: I think it can be safely stated

‹lars_kurth›     1) get uncontroversial / easy to do stuff first

‹lars_kurth›     2) then put more complex things, but start with documents/headers/anything that is substantial to understand the rest of the changes - if they fit into logical units maybe repeat that pattern

‹gwd› I think every situation if different.

‹royger›           the only time you can get non-ordered review is if your patch series touches multiple subsystems and you have properly splitted this into different patches, in that case each subsystem maintainer is likelky to review his ares without looking at other patches

‹gwd›  royger: You're talking about checking in, not review I think?

‹gwd› Sometimes in a long series, {1,2}/10 will be clean-ups; {3-6}/10 will be general reorganisations which don't really seem to do anything; and then 7/10 will be the "meat", which helps you understand what {3-6}/10 were about.

‹lars_kurth›     royger, gwd: "It's quite common for patches on a patch series to rely on the previous ones" - is an important point.

‹gwd› Usually it's not possible to put 7/10 earlier without making it far more complicated.

‹lars_kurth›     I think the way how to best structure patch series is an important one, and there is practically NO information about this. So people learn stuff by trial and error. So I think it is worth exploring that

‹gwd› It's a bit like saying, "How do you teach someone something", or "How do you make an argument to convince someone of something". There are patterns, but it's so broad a topic you can't really give direct advice.

‹lars_kurth›     gwd: maybe showing some examples will help. I think the problem is that cover letters frequently don't help much

‹Diziet› I think what you just said above "Sometimes in a long series," would be a very useful thing to put in as a general rule.

‹Diziet› gwd: ^

‹Diziet› Sometimes some of these pieces will be empty.

‹Diziet› 1. cleanups 2. reorgs 3. headers/docs/etc. 4. meat 5. cleaning up any infelicities introduced temporarily 6. deleting old code

‹Diziet› If there are multiple subsystems involved, then these are best separated out where possible, so you end up with those 6 categories x N subsystems.

‹gwd›  Of course, sometimes there are several "meat" patches, which could be ordered in different ways; and then you may want to put reorgs in between the meat patches.

‹gwd›  The XSA-299 series is an example of that.

‹Diziet› In this context I am reminded of https://www.chiark.greenend.org.uk/pipermail/sgo-software-discuss/2019/000616.html My main achievement for the weekend, in a personal project.

‹gwd›  And lars_kurth could probably go back over the history and see the series develop; the "meat" patches were reorganized several times to try to find out which order created the most comprehensible series of individual patches.

‹Diziet› Subject:  New signature key arrangements

‹gwd›  lars_kurth: And say, if you take a look at the golang xenlight bindings series; v1 would have been useless without the entire thing being checked in. It's likely that v3 I'll be able to check in {1..16}/22, meaning v4 will have a lot fewer patches to rebase / recheck.

‹Diziet› I'm not suggesting it as an example in this context because I have done much less squashing than would be usual in a community like Xen where we have many more reviewers so the goal of making review easy and comprehensible is more important relative to the goal of later understanding what the original programmer was thinking when they wrote soemthing.

‹lars_kurth›     This is very useful. I think this is the missing piece

‹Diziet› But we should provide some examples. Do we have good example cover letters we could use ?

‹Diziet› I'm sure there are some libxl ones but ideally we would have an example touching multiple subsystems. And one which wasn't affected by release constraints.

‹lars_kurth›     That would be good. Maybe I can write something up and put a place-holder in place for good examples, if we can't think of one now

‹jbeulich›         gwd, lars_kurth: The opposite case (cleanups last) would often be preferable for series where the "meat" one(s) are to be backported, but the cleanups aren't.

‹lars_kurth›     jbeulich: interesting point. Do you have an example?

‹jbeulich›         An example of what? A series where we asked for re-ordering because of the above? If so, I don't think I could easily spot one.

‹jbeulich›         I can tell you though that the above is what I would typically do. Most prominently for security fixes *where the cleanup is being held back altogether).

‹lars_kurth›     jbeulich: I will try and put something together. I think I have enough to go on

 

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