purpose and implementation of code reviews

[Bruno Haible]

Topics:
1) What are the purposes of having code reviews?
2) How should tooling that helps for code reviews work?
3) Should the "Information for Maintainers of GNU Software" mention
   code reviews as a good practice?


== What are the purposes of having code reviews?

The previously-known benefits are:
  * Find mistakes early.
  * Senior developers are teaching more junior developers.
  * A good atmosphere among developers: cooperation, not competition.

Since last week [1], we have to add another benefit:
  * It can detect (and thus avoid) evil behaviours from individual developers
    (even co-maintainers).


== How to do code reviews?

The main implication is that commits from *all* developers should be
reviewed from now on. Not only of the more junior ones. Not only of those
developers who frequently make mistakes.

Review before or after commit?

  - In critical packages with many developers, such as glibc, it is important
    that patches get reviewed before they get committed. For glibc, such
    tooling is already in place (although the lack of references between the
    bugzilla, the mailing list, and the patchwork site make it tedious to deal
    with).

  - In packages with few developers, on the other hand, where co-developers
    may not be available for review within 24 hours, requiring review before
    commit would significantly slow down the main developers' workflow. IMO
    for such packages it is appropriate to do the review after the commit.
    (In the 'xz' backdoor case, if there had been a review of *all* commits
    within 4 weeks after commit, it would have been sufficient.)


== How should tooling that helps for code reviews work?

Tooling for "review before commit" is already widely implemented.

Is anyone aware of tooling for "review after commit"?
I imagine that it could essentially manage a list of commits, and for each
commit
  - a list of reviewers that have reviewed it,
  - a list of reviewers that have been invited and not reviewed it.
Different reviewers might want to review different aspects (code style,
i18n, performance, security, etc.).
Also some way is needed to remind invited reviewers after a certain time.


== Should the "Information for Maintainers of GNU Software" [2] mention
   code reviews as a good practice?

Should it mention code reviews as a recommended good practice?

Should it go into details?

Opinions?


Bruno

[1] Lasse Collin wrote on IRC on 2024-03-30: "
The last things I didn't review were the final ifunc changes.
It's surprisingly little that has gone into the Git repo without any kind of 
review by me. But these changes were such.
...
The ifunc stuff had had some trouble (real bugs) and I was fifty-fifty if all 
ifunc should be ripped out just because it didn't feel worth it in this project.
"
[2] https://www.gnu.org/prep/maintain/maintain.html