On 9 Dec 2019, at 11:02, Lars Kurth <lars.kurth@citrix.com> wrote:
On 06/12/2019, 09:51, "Jan Beulich" <jbeulich@suse.com> 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
- EtcI 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 doit 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
‹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