Re: Review process improvements

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Thu, Dec 16 2021, Emily Shaffer wrote:

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

Konstantin commented on some of this, and not to pile on, but I'd also
not like to see such a MAINTAINERS file in git.git for those sorts of
things.

I think this makes sense for linux.git as different subsystems have
different mailing lists that are used for patch review, coordination
etc. Linus then pulls from those and other subsystem maintainers. There
seem to be on the order of 100 of those split-outs now:

    $ git grep -h -o linux-.*@.*kernel.org MAINTAINERS|sort -u|wc -l
    77

Whereas the only cases that really applies to in git.git (and I think it
might be useful to have a MAINTAINERS for these) are:

    # but not all of po/, see caveats in po/README etc.
    po/ -> https://github.com/git-l10n/git-po/
    # Pulled from their respective upstreams
    {gitweb,git-gui}/
    # Anyhing else I'm missing? Maybe {sha1dc,sha1collisiondetection}/*?
    
But (and I know you just used me as an example, but it applies in
general) I'd really not like to be CC'd on any change to config.[ch]
just because I touched some code in those files that's clearly unrelated
to whatever change is now being proposed.

We should be leaning into helper scripts like
contrib/contacts/git-contacts (and I'm aware of some out-of-tree "better
git-contacts", but have not used them myself).

I.e. we almost always have a better and more accurate version of this
information in the commit history.

E.g. in the side-thread Randall asked to be CC'd on NonStop changes. I
think he should (and as does he), but I really don't see how it's
necessary to have a MAINTAINERS for such use-cases. If I'm editing what
little NonStop-specific code we have in tree, it should be impossible to
miss Randall as an interested party when findings someone to CC through
the usual means.

What are those "usual means"? I think they're different for most list
regulars, but my personal worklow is (and I think some version of this
applies to most regulars):

1. As I'm preparing a patch series I'll need to run some of "git log",
   "git blame", "git log -G<relevant-string>", "git log -L:<funcname>:<file>"
   etc. just to understand the code I'm modifying.

   As I'm doing that I take notes (usually these make it into draft commit
   messages, noting prominent commits whose behavior is being changed). The
   authors likewise make the draft CC list.

   This is basically an approximation of what a script like "git-contacts"
   tries to do, but I manually filter it. So e.g. I won't CC a person who
   just tree-wide renamed a function I'm changing, even if that's the last
   edit to the exact line I'm changing.

2. I apply (sometimes on the fly) some fuzzy filtering to prune the above list.
   Mostly this is to remove people from CC that I know to be inactive/gone, but
   I'll also lean towards removing people I know to be CC'd a lot already, unless
   the change really is highly relevant to them in particular.

This is manual for me now through a combination of not having bothered
to automate it, and from having peeked a bit at some of automated
tooling and concluded "it looks like I'd need to manually filter this
anyway". E.g. I think "git contacts" can produce some rather naïve
output.

But leaning into that approach would be a much better
direction. E.g. the "filter out inactive" could be mostly/entirely done
via some configured heuristic in such a tool, together with downloading
the LKML archive to see how long it's been since someone last posted
something.

I can also imagine a MAINTAINERS being very useful for cases that aren't
evident from scouring the commit logs, or for which it's nice to be
explicit about.

E.g. I've often chimed in on changes to do withn how/whether something
can/should be marked for translation. I added the i18n subsystem
initiall, so for those cases interested parties would probably find me
anyway, but I wouldn't mind being explicit about that. Ditto for people
we consider the authority on certain subsystems, such as (this may not
be any one person currently), refs, the index, SHA1<->SHA256 interop
etc.