Review process improvements

[Emily Shaffer <emilyshaffer@google.com>]

Hi folks,

Recently in our team at Google I revived a conversation we've had many
times before upstream - in fact, at every contributor summit that I've
attended since I joined the project mid-2019. At each summit, we have
some conversation around review efficacy, process, tooling, and so on.
In late 2019[1], we discussed difficulty with performing reviews and
keeping track of them on the mailing list. In early 2020[2] we discussed
the unfortunate case of reviews with no responses whatsoever, and what
might be preventing Git contributors from reviewing more topics, more
regularly. In late 2020[3], lots of different topics in the
inclusion-focused contributor summit centered around reviews - tone,
tooling, workflows, and so on.

Some of those discussions resulted in changes - for example,
GitGitGadget was improved and added to git/git, and we enjoy easy,
non-noisy CI runs via GitHub Actions. But some things we thought were a
good idea never went into practice. In the next few months, the Google
Git team is planning to implement some of the following changes, and
we'd appreciate your thoughts ahead of time as well as your review later
on:

1. Draft a MAINTAINERS file
One recurring theme from the conversations I had with the Google Git
team was that selecting a topic to review in the first place can
actually be pretty time-consuming. Choosing from the series subject line
or the "What's cooking" mail isn't always straightforward - it can be
hard to tell whether the series is working on a goal you are interested
in or in a part of the codebase you care about. It's not feasible for
someone wanting to get involved to review every single series that comes
across the list - but it might be feasible for someone to review every
series in a particular subsystem or topic.

In the first half of January, I'd like to have a review out to the list
adding a kernel-style MAINTAINERS file with a few areas of interest. To
that file, I'd like to add "R:" ("CC me!") lines to subsystem mailing
lists and interested individuals. My hope is that that will make it easy
for someone to either add themselves as an "R:" or to subscribe to the
subsystem list, and then be able to filter their mail based on "stuff
I'm CC'd to" or "stuff sent to git-partial-clone@linux.dev" - and then
be able to review every patch in that list. I'd like to create lists on
topics where it makes sense, too.

Since the Git codebase isn't modularized into subsystems as plainly as
the kernel is, I don't think that the MAINTAINERS list needs to be
machine-parseable yet, although it would be a nice goal. Maybe over time
we will decide we'd rather organize the codebase differently and
implement a machine-parseable MAINTAINERS list to incorporate into a
sendemail-validate hook, or something. So I envision lines looking
something like this (just examples, sorry for directly calling people
out ;) ):

  Submodules
  submodule.[ch], git-submodule.sh, builtin/submodule.c,
    Documentation/git-submodule.txt, anything else adding a feature involving
    submodules
  R: git-submodules@example.com
  R: emilyshaffer@google.com

  Config improvements
  Documentation/config/*, config.[ch], builtin/config.c, changes which add new
    config options
  R: emilyshaffer@google.com
  R: avarab@gmail.com

It would be extremely useful to hear other suggestions from the mailing
list about subsystems which deserve a MAINTAINERS line and possibly a
subsystem mailing list.


2. Draft a ReviewingPatches doc
Another theme we discussed was the general ambiguity around the act of
performing code review. How detailed should the review be? Who should be
examining interoperability with the rest of the codebase? Is it OK to
reply with only "I don't know what you're trying to do, rewrite your
commit message please"?

Sometime in January I'd like to have a review out with an outline with
agreement on the content and organization for a ReviewingPatches doc.
I'm hoping that it can give some structure to review by including:
  - How to give different types of review (maintainer vs. individual vs.
    drive-by nits)
  - Review best practices (Sage Sharp's article on stages of review[4],
    CCing experts/lists from MAINTAINERS file above, short-circuiting by
    sending comments early instead of reviewing a patch in-depth when
    the idea seems so-so or unclear)
  - Checklists for new contributors to use or to help make in-depth
    review more thorough

Since this is intended to be checked into Git's codebase, I expect that
we'll want to discuss a lot and make sure we all buy into the contents
of that doc.

3. Google Git team will review cover letters and commit messages
   internally before sending series to the list
I often find that when I send a patch to the list, reviewers reply
without understanding the point of what I was trying to achieve in the
patch. It sounded like this happens to some other Google Gitters as
well. Luckily, that's fixable by the patch author.

To try and make sure we're sending patches that are easy to understand
and decide whether to review, we're adding a step to our process ASAP to
require commit messages, cover letters, and "---" notes to be reviewed
and approved by at least one other Googler before a topic goes to the
mailing list. Those reviews don't need to be formal, but do need to
happen for every reroll of a series. We also will intentionally *not*
review the diff of changes in this private setting - reviews for code
correctness, etc. should continue to happen upstream, in public.

I'm mentioning that takeaway in this email to say: if you find you don't
understand the purpose of a patch from a Googler, please let us know! It
would be really valuable for us to receive a review saying "I think you
might be saying X but it's still confusing because you wrote Y"
so that we can incorporate the source of your confusion as we continue
to review each other's "accompanying context".

4. Documentation changes to encourage commit message quality
Commit messages are important, but one comment I hear a lot from new
contributors to the Git project is: "why do these people care about the
commit message so much? The commit message doesn't compile!" That tells
me that instead of explaining the importance of quality commit messages
over and over at review time, we should be doing a better job of
explaining why we have high standards and helping them meet those
standards in documentation a contributor is likely to refer to.  On the
one hand, we want the list to be friendly and inclusive, but on the
other hand, it’s pretty demanding for a reviewer to review patches if
the commit message isn’t clear enough.

SubmittingPatches has a short blurb about commit message quality under
the "Make separate commits for logically separate changes" heading and a
longer one under "Describe your changes well". But I think it doesn't do
a great job explaining *why*, only explaining how. I'd like to send a
review sometime in January for a modification of SubmittingPatches to
include a paragraph about how we use commit messages - for example, to
identify the motivation behind a certain line of code during 'git
blame', or to decide whether a patch is worth reviewing. My hope is that
this documentation change would help convince authors that it's worth
their time to write a good commit message.

I'd also like to add a short hands-on section to MyFirstContribution,
looking up both bad and good example commits to underline the importance
of the commit message and the way that it's used after the patch is
merged. I don't think it needs to be long: "git show abcdef; can you
tell what the commit is doing? Now git show 123456; can you tell what
this one is doing?" where 'abcdef' is an example of a not-so-great
commit message and 123456 is an example of a fantastic commit message.
But I'd like to make sure that whatever commits we use as an example are
volunteered by the patch author, rather than chosen to be some example
of an antipattern. So if anybody wants to volunteer an example of a
particularly unclear commit that they wrote, that would be incredibly
useful.

For both these documents, the most useful help I could see from the
upstream mailing list would be to review and ensure we're conveying the
goals and the purpose of good commit message writing accurately.

---

For all 4 of the above, either I or another Google Git team member will
be responsible for taking action. If someone else wants to volunteer or
beats us to the punch with a patch on list, that's certainly fine with
me - but I'm not trying to ask for these changes from the community at
large. What I am really looking for is to be held accountable: if the
end of February 2022 rolls around and we still don't have any of the
things I described, feel free to raise a fuss. And I hope also to hear
from you all in the form of reviews and suggestions, or shouts of "no!
don't do that!" if something sounds incorrect or counterproductive to
you.

As it's reaching the end of the year and the holiday season, I'm
planning to spend the rest of the year with family and away from my work
email as much as possible. Some other Googler Gitters will be working on
and off and may chime in. But I am expecting to be able to engage more
with this mail in the first week of January.

Thanks very much for reading and discussing.
 - Emily

[1] https://lore.kernel.org/git/nycvar.QRO.7.76.6.1909261253400.15067@tvgsbejvaqbjf.bet/
(see discussion #10)

[2] https://lore.kernel.org/git/6DAC1E49-9CA0-4074-867E-F22CD26C9FEB@jramsay.com.au/

[3] https://docs.google.com/document/d/1g_QapVhHejHorpUzzjpT5DlPi6ovNlEK4423uYWypbU/edit#heading=h.9tcri25458a
(See Day 2 notes; it's possible these notes are closed to those who didn't
attend the summit)

[4] https://sage.thesharps.us/2014/09/01/the-gentle-art-of-patch-review/