Review process improvements

Review process improvements

[Emily Shaffer]

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/

RE: Review process improvements

[rsbecker]

On December 16, 2021 5:46 PM, Emily Shaffer wrote:
> 1. Draft a MAINTAINERS file
> 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.

I am not sure about separate mailing lists. That gets really cumbersome as things get large. Perhaps some prefix in email based on the list.

I think starting with a YAML-style MAINTAINERS file might be better than the artificial tag structure proposed even as a starting point.

Somewhat like:

Submodules:
 Prefix: SUBM
 Files:
     submodule.[ch], git-submodule.sh, builtin/submodule.c,
     Documentation/git-submodule.txt
 Reviewers:
   git-submodules@example.com
   emilyshaffer@google.com

etc.

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

Some suggestions:

Platform:
 Linux:
  Prefix: LINUX
  Reviewers: etc.
 NonStop:
  Prefix: NS
  Reviewers:
   rsbecker@nexbridge.com (a.k.a. me!)
 etc.

Compat-Layer
 Prefix: COMPAT
Perl
Python
P4
SVN
Gitk
Signing
Future-Plans
Etc.

> 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"?

People learn what they see, so it is important to keep review style consistent both for the actual review and to keep the review culture consistent. In some places, I have seen review "Body of Knowledge" documents as the review culture evolves outlining what should be in a review beside the code style guidelines.

> 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.
><snip>
> 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!"

This is leading down a major git enhancement RFE, which may have general applicability. One great power of git is the idempotency of the commit across files. I realize that the current style for git itself is separate commits, but I think that partly is a limitation of the commit capability itself.

Suppose the cover letter is essentially a headline, with the subject being the headline headline of the commit. The commit comment could (the RFE) vary depending on the file being committed - an attribute of the node in the Merkel Tree perhaps. So the main commit node has the headline, the cover letter contents, while the file node has the main headline and the file-specific comment. In git log, you would see the headline, then the cover letter, then the file comment (if specifying a path). Just a thought. It might significantly reduce the size of history.

Just some ramblings,
-Randall

Re: Review process improvements

[Junio C Hamano]

Emily Shaffer <emilyshaffer@google.com> writes:

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

I was in some of the discussions that led to this message, and I
feel that some things need to be clarified.  The most important
thing is that it is not a declaration that Google's Git team is
somehow trying to take the project maintenance or the review process
over.  They are merely declaring what they will do to/on the list.

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

OK.  I expect that both the enumerations of "areas of interest" and
the parties who are interested in each area would be rather fluid,
though.  "config" might have somebody actively working on today, but
may not be in 3 months, and those who are interested in config today
may not be in 3 months.

Also, I'll mentally rephrase your "review" to "reviewable patch" in
my head from here on.

>   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

I am afraid not many people on this list knows about the convention.
They can probably guess "R:" is "CC me!" from the previous
paragraph, but they cannot guess (at least I can't) what it means to
have a mailing list there.  Does it mean a patch or a discussion
does not have to happen here as long as it is done on a "list"
elsewhere?

Also, the fluid-ness of the area might not work in favor of separate
mailing lists.  We'll see.

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

It is unclear what "maintainer" and "individual" reviews are;
hopefully we see a clear definition in the January patch.

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

The reason behind this is because these Googler patch authors in the
discussion feel that an initial "why is this even needed?" response
highly discouraging.  They are trying to, by proofreading the cover
letters and log messages among themselves before they are sent, make
sure that the motivation is clearly expressed in their patches.

I think it is a good idea and would encourage everybody to follow,
if possible.  Find a good sounding board in your buddies and ask
them to see if your cover letter and proposed log messages clearly
convey what you want to achieve and why it is a good thing, even to
those who do not necessarily share as much background context.  For
a solo developer (which I've been on this project for quite some
time), well, just "sleeping on" your patches often helps, as a
renewed you tomorrow will give you a different perspective.

But don't spend too much time on it to waste your momentum.  Having
sounding board in your buddies is much better than not having any,
but your buddies may already share too much background context with
you to blind them from noticing why it is unclear to outsiders.

> 4. Documentation changes to encourage commit message quality
> ...
> 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.

If we can do so by picking good and bad example by the same author,
that would make a good educational experience to the readers.  Knowing
that nobody is consistently great would make it less stressful than
always having to try to be perfect.

Re: Review process improvements

[Konstantin Ryabitsev]

On Thu, Dec 16, 2021 at 02:46:21PM -0800, Emily Shaffer wrote:
> 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:

I'd like to also mention that I'm hoping to add a few more features to b4 that
will hopefully improve the patch submission process for folks. This feature
set is far from being complete yet, but the gist will be as follows:

1. b4 submit --new: this will create a new tracking branch and define
   some metadata to go with it, such as a cover letter template. The cover
   letter can be edited using `b4 submit --edit-cover` at any time.

2. b4 submit --send: will generate a patch series from any commits created
   from the topical branch fork point and use the cover letter from the
   previous step. It will be able to send the patches using the traditional
   SMTP way, OR it will allow using a web-based submission service I'm setting
   up at kernel.org:

   - anyone will be able to submit valid patches through this service
   - one-off patch submissions will require an email confirmation roundtrip
   - any submitter can also register their ed25519/openssh patatt key with the
     submission service and just cryptographically sign their patches. As long
     as signatures validate, no roundtrip confirmation of patches will be
     required after the initial public key registration.
   - on the receiving end, the patches will be written to a dedicated
     lore.kernel.org feed *as-is*, but also sent to the recipients after doing
     the usual From/Reply-To substitution and moving the original From into
     the in-body git headers (i.e. same as GGG does).

3. b4 submit will properly include base-commit information to all submitted
   patches, as well as a unique change-id trailer (but in the basement of the
   cover letter, not in the commit message trailers as Gerrit does).

4. b4 submit --sync: will retrieve any received code review trailers
   (reviewed-by, acked-by, etc) and amend the corresponding commits in the
   topical branch, assuming we can match patch-id's (I've not tackled this
   yet, so I may be unduly optimistic here).

5. b4 submit --reroll: will prepare a v2 (v3, v4) of the series, reusing the
   same change-id trailer and adding a templated "Changes in v2" entry to the
   cover letter (that must be edited before --send works again).

6. b4 submit --send: will send the new version of the series.

I'm hoping that this will improve the experience of patch submitters *and*
help ensure that CI can be incorporated into the process by streamlining the
submission procedure and providing a public-inbox feed that can be easily
monitored. A service like GGG can fairly easily convert well-formed patch
series (at least those carrying valid base-commit info) into ephemeral
worktrees, or even into simulated pull requests if the goal is to do this via
a generic CI service.

The important goal is to keep development fully decentralized so that even if
the web submission endpoint provided by kernel.org becomes unavailable, plain
old SMTP submission mechanisms can still be used as a fallback.

(Unfortunately, I need to focus my efforts on some kernel.org infrastructure
changes at this time, so I'm not sure when I'll be able to complete these
features.)

> 1. Draft a MAINTAINERS file

So, I *don't* recommend that you go this route. The biggest problem with the
MAINTAINERS file as used by the Linux development community is that making
changes to it is a very high-latency process. It will be less of a problem for
the much smaller git developer community, but it will still result in folks
operating from a stale copy of the file for weeks after it is updated
upstream.

One of the goals behind the "lei" tool by public-inbox is that it allows
search-based subscriptions (including things like "what files are being
modified, what functions are mentioned in the git context", etc). Anyone can
define a set of parameters they are interested in monitoring and receive only
threads matching those pre-filtered messages. We are currently rolling this
out as an end-user maintained setup [1], but the goal is to also offer this to
maintainers as a centrally managed service:

- maintainers will be able to define the search queries they are interested in
  monitoring
- we will set up a feed on our end matching those queries
- we will offer the feed as a separate public-inbox repository, a public IMAP
  folder, a read-only mailing list, and, I'm hoping, a read-only POP box (the
  latter mostly so it can be configured to feed into GMail).

The hope is that this will allow what you're looking for -- a way to
pre-filter the flow of patches to maintainers by making it topical, without
making the lives of patch submitters unnecessarily difficult by increasing the
chances that they will send their patches to the wrong list.

[1] https://people.kernel.org/monsieuricon/lore-lei-part-1-getting-started

I'm not sure where this all fits into your plans, but I wanted to show what is
happening on our end just so we're working in parallel, as opposed to in our
own separate worlds. :)

Best regards,
-K

Re: Review process improvements

[Christian Couder]

On Fri, Dec 17, 2021 at 11:00 PM Konstantin Ryabitsev
<konstantin@linuxfoundation.org> wrote:
>
> On Thu, Dec 16, 2021 at 02:46:21PM -0800, Emily Shaffer wrote:
> > 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,

Thanks for trying to make it easier and more efficient to contribute!

> > and
> > we'd appreciate your thoughts ahead of time as well as your review later
> > on:
>
> I'd like to also mention that I'm hoping to add a few more features to b4 that
> will hopefully improve the patch submission process for folks.

Thanks also for implementing and maintaining tools and sites that help
contributing!

I have added a new "Process related tools and sites" section to
https://git.github.io/Hacking-Git/ with links to GitGitGadget,
lore.kernel.org/git, public-inbox, lore+lei, b4 and git-series. I am
willing to add other tools and sites if someone knows others worth
mentioning.

[...]

> > 1. Draft a MAINTAINERS file
>
> So, I *don't* recommend that you go this route. The biggest problem with the
> MAINTAINERS file as used by the Linux development community is that making
> changes to it is a very high-latency process. It will be less of a problem for
> the much smaller git developer community, but it will still result in folks
> operating from a stale copy of the file for weeks after it is updated
> upstream.

My opinion is that a MAINTAINERS file makes more sense when there are
different mailing lists to send patches to, and trusted
lieutenants/maintainers. I am not sure that we are at that point yet.

About documentation, if some good documentation exists elsewhere, it
might be good enough to just point to it from an existing doc and/or
https://git.github.io/Hacking-Git/. Otherwise adding a new separate
doc might be better than growing an existing doc too much, which could
scare new comers.

Re: Review process improvements

[Ævar Arnfjörð Bjarmason]

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.

Re: Review process improvements

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 586 bytes --]

On Fri, Dec 17, 2021 at 01:39:42PM -0500, Konstantin Ryabitsev wrote:

> 2. b4 submit --send: will generate a patch series from any commits created
>    from the topical branch fork point and use the cover letter from the
>    previous step. It will be able to send the patches using the traditional
>    SMTP way, OR it will allow using a web-based submission service I'm setting
>    up at kernel.org:

One thing my equivalent of this does (I might've used patman but I don't
think I was aware of it at the time) is to tag each revision as it's
published, might be worth considering.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Review process improvements

[Eric Sunshine]

On Mon, Dec 20, 2021 at 7:27 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> 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}/*?
>
> 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).

For completeness, the granddaddy of out-of-tree "better" tools is
Felipe's git-related[1], which is maintained and more functional.

[1]: https://github.com/felipec/git-related

Re: Review process improvements

[João Victor Bonfim]

I added on to this on another e-mail thread, however, it got no response, specially because I didn't have an e-mail to reply to, so I am copy and pasting the messages here.

‐‐‐‐‐‐‐ E-mail one ‐‐‐‐‐‐‐
 First and foremost, there is a saying in Brazil that goes by as "an empty bag never holds itself standing", so always remember to care for yourself, eat a snack, drink fluids of your choice (from tea, water and coffee to soup and porridge), rest a little, stop doom scrolling social media and so on.

 Second, want to address the giant essay by Emily Shaffer (aka: Review process improvements) and give my opinions on some of the points made. Third, sorry for the botched response, but I wasn't on the mailing list before the mail was posted, so couldn't directly respond the message.

 Addressing point #1, titled "Draft a MAINTAINERS file": seems quite reasonable, I would also like to address some matters about this, first is that there is no point of contact for "trusted sources" within the git project and it is quite negative for historical purposes, because maintainers and more prolific parties will inevitably retire or move on from Git themselves and their prestige won't be recorded beyond their commit history within the project and it might lead to their contributions being forgotten. Second is that it is hard to know who is responsible to what from the get go without being in the know already. Third, please make all entries on any and such file that might auto send messages non-committal, why? Nobody wants to receive a message from a third party that the git mailing list deems "trustwothy" only for it to be some scam of sorts that only happened because a modification to the file managed to fly under the radar, so make it a one way pass and all tags are only to people, not from. Fourth, I, personally, only want to partake on discussions, but barely want to see patches and commits, maybe allow for some sot of group inheritance and group message allow or deny lists? So people that don't want patches don't receive patches, but they can filter to receive only "memory allocation" topics, but they won't receive patches that are for memory allocation, because the "patches" and "discussion" topics take higher system priority than the "memory allocation" topic, be it user by user or system wide. Maybe also auto-filter messages, even in a dumb way or in a sender dependant way.

 Addressing point #2, titled "Draft a ReviewingPatches doc", and point #3, titled "Google Git team will review cover letters and commit messages internally before sending series to the list": not much to say beyond, people, share your reviewing know how, including you Google folks, if you interpret the reviewing process as an algorithm, it would make sense that all mechanisms of human interaction and review to be shared as part of the code base. So please, Google people, share what and how you do your reviews. It is also a matter of security, if your review process isn't transparent, we can't know whether we can trust everything you provide, because you might be leaving or dismissing problems and it might fly under the radar or malicious action could be taken and, since the group as a whole is already trusted, some malicious code could be included, even if it is not explicitly so.

 Addressing point #4, titled "Documentation changes to encourage commit message quality": This isn't stressed enough in many Git tutorials and the like, and it should, the commit messages are for the journaling of changes, so you or third parties can understand the thought process behind changes and not feel like headless chickens running around a barn whenever you attempt to understand what has been done, maybe addressing it on the source code of git and its documentation might help address this topic.

 Extra notes: As a person with ADHD, REAMEs can be daunting at times, specially when they are boring word walls, and they can be incomplete or repetitive if there are other documents addressing information contained within them, maybe reference files such as "Contributing.md" instead of copying them verbatim? An example would be "To read more on how to contribute to projects, read 'Contributing.md'." instead of what is information contained within "Contributing.md", it would help a lot with human to human interactions.

 Thanks for the attention, take care!

 João Victor Bonfim (any pronouns).
‐‐‐‐‐‐‐ E-mail two ‐‐‐‐‐‐‐
To address Junio Hamano's comment:
>  But don't spend too much time on it to waste your momentum.  Having
> sounding board in your buddies is much better than not having any,
> but your buddies may already share too much background context with
> you to blind them from noticing why it is unclear to outsiders.

 Maybe ask someone like your mom, dad, aunt, cousin, long past college acquaintance to comment on the commit/log/patch comment itself and maybe also the code published? It might me more helpful the less they understand about code or are knowledgeable about your personal quirks.

João Victor Bonfim (any pronouns).

‐‐‐‐‐‐‐------------‐‐‐‐‐‐‐

Yes, I know I wrote an essay-esque message myself.
It was a lot.
Hope it helps to move the conversation along.

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐

Em segunda-feira, 20 de dezembro de 2021 às 14:14, Eric Sunshine <sunshine@sunshineco.com> escreveu:

> On Mon, Dec 20, 2021 at 7:27 AM Ævar Arnfjörð Bjarmason
>
> avarab@gmail.com wrote:
>
> > 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}/*?
> >
> >
> > 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).
>
> For completeness, the granddaddy of out-of-tree "better" tools is
>
> Felipe's git-related[1], which is maintained and more functional.
>
> [1]: https://github.com/felipec/git-related

Re: Review process improvements

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1176 bytes --]

On 2021-12-20 at 11:22:32, Ævar Arnfjörð Bjarmason wrote:
> 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.

I have limited time for the list, but I also tend to pop in on certain
topics: the SHA-256 transition, commit and tag signing, authentication,
translations, and so on.  However, outside of the signing code, a lot of
that isn't centralized in one place.

I also wouldn't be able to commit to signing up for any subsystem in a
MAINTAINERS file, but I'm happy to be CC'd if folks think I'm
knowledgeable about something or if it's an area they think my opinion
would be valuable.

I can't speak for others, though, so I don't know if a MAINTAINERS-style
approach would be valuable for them.
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: Review process improvements

[Ævar Arnfjörð Bjarmason]

On Fri, Dec 17 2021, Konstantin Ryabitsev wrote:

> [...]
>    - on the receiving end, the patches will be written to a dedicated
>      lore.kernel.org feed *as-is*, but also sent to the recipients after doing
>      the usual From/Reply-To substitution and moving the original From into
>      the in-body git headers (i.e. same as GGG does).

That GGG does this is one reason I haven't considered using it. It
breaks all sorts of E-Mail workflow assumptions from polluting the
address book for every person who uses it, to any "from:<addr>" search
needing special consideration etc.

Of course you'd need authentication etc, but is there a reason for why
such tooling can't work more like an SMTP relay and less like GGG which
(for some reason) insists on taking over the "From" header?

I think in its case it's because it wanted to mirror all the discussion
to GitHub. But presumably a ML-native tool won't have that problem (and
presumably GGG could do the same mirroring by following the ML after
submission).

Re: Review process improvements

[Fabian Stelzer]

On 22.12.2021 04:26, Ævar Arnfjörð Bjarmason wrote:
>
>On Fri, Dec 17 2021, Konstantin Ryabitsev wrote:
>
>> [...]
>>    - on the receiving end, the patches will be written to a dedicated
>>      lore.kernel.org feed *as-is*, but also sent to the recipients after doing
>>      the usual From/Reply-To substitution and moving the original From into
>>      the in-body git headers (i.e. same as GGG does).
>
>That GGG does this is one reason I haven't considered using it. It
>breaks all sorts of E-Mail workflow assumptions from polluting the
>address book for every person who uses it, to any "from:<addr>" search
>needing special consideration etc.

I agree that this is a very annoying side-effect of GGG.

>
>Of course you'd need authentication etc, but is there a reason for why
>such tooling can't work more like an SMTP relay and less like GGG which
>(for some reason) insists on taking over the "From" header?
>

I think SPF/DMARC spam filtering will prevent tools like GGG to do this any 
other way. For GGG to be able to send with your From: you'd either need to 
give it access to your smtp credentials or configure your whole domain to 
allow GGG mail servers (basically github) to send email for it. I think both 
options are not really something you'd want.

>I think in its case it's because it wanted to mirror all the discussion
>to GitHub. But presumably a ML-native tool won't have that problem (and
>presumably GGG could do the same mirroring by following the ML after
>submission).
>

Re: Review process improvements

[Konstantin Ryabitsev]

On Wed, Dec 22, 2021 at 04:26:39AM +0100, Ævar Arnfjörð Bjarmason wrote:
> That GGG does this is one reason I haven't considered using it. It
> breaks all sorts of E-Mail workflow assumptions from polluting the
> address book for every person who uses it, to any "from:<addr>" search
> needing special consideration etc.
> 
> Of course you'd need authentication etc, but is there a reason for why
> such tooling can't work more like an SMTP relay and less like GGG which
> (for some reason) insists on taking over the "From" header?

This would require pretending that we're authorized to send mail from the
domain name of the commit author, so this unfortunately won't work (and hence
the reason why GGG does it this way). E.g. say you have:

From: foo@redhat.com
Subject: [PATCH] Fix foo

For DMARC policies to properly work, this message would need to have a DKIM
signature from redhat.com, so we'd need to have access to Red Hat's private
keys. If we don't use DKIM signatures, then the recipient SMTP gateways may
mark the message as spam (and they would be right, since we are pretending to
be foo@redhat.com, i.e. acting just like phishers).

The only way for this to work is to do the From / X-Original-From /
Follow-up-to / Reply-To header dance. Git does support this very well, and
most email clients do the right thing when encountering this situation, but
it's not going to have the exact same visual flow as patches sent directly via
SMTP.

However, we will also write these messages to a public-inbox repository before
making the From: substitutions, so if someone retrieves these patches with b4
or lei, as opposed to receiving them via their SMTP mailbox, they should be
able to get proper From: in the headers.

-K

Re: Review process improvements

[Junio C Hamano]

Konstantin Ryabitsev <konstantin@linuxfoundation.org> writes:

> This would require pretending that we're authorized to send mail from the
> domain name of the commit author, so this unfortunately won't work (and hence
> the reason why GGG does it this way). E.g. say you have:
>
> From: foo@redhat.com
> Subject: [PATCH] Fix foo

Would it help to use "Sender:"?  When GGG or any other automation
are trying to send e-mail on behalf of the person shown on "From:",
I thought that it is the mechanism for them to use to identify
themselves.

Re: Review process improvements

[Konstantin Ryabitsev]

On Wed, Dec 22, 2021 at 11:42:02AM -0800, Junio C Hamano wrote:
> > This would require pretending that we're authorized to send mail from the
> > domain name of the commit author, so this unfortunately won't work (and hence
> > the reason why GGG does it this way). E.g. say you have:
> >
> > From: foo@redhat.com
> > Subject: [PATCH] Fix foo
> 
> Would it help to use "Sender:"?  When GGG or any other automation
> are trying to send e-mail on behalf of the person shown on "From:",
> I thought that it is the mechanism for them to use to identify
> themselves.

Indeed, that's how the DKIM standard wanted to deal with this problem, however
when the DMARC RFC was being drafted, this approach was deemed insufficient.
They have a good explanation for it -- there is no standard among UI clients
to handle the Sender/From discrepancy. Most MUAs will happily ignore the
Sender: field and will only show what is in From:, so this approach was
considered ineffective against phishing attacks. An attacker could easily
register a domain, set DKIM records, and then use any From: they wanted as
long as they used a valid Sender: header, knowing that it would be ignored by
most mail clients.

So, DMARC deliberately ignores the Sender: header and *only* pays attention to
the From: field for its purpose.

-K

Re: Review process improvements

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1487 bytes --]

> I'd like to also mention that I'm hoping to add a few more features to b4 that
> will hopefully improve the patch submission process for folks.

This looks excellent! I wanted to mention a few features that have been
popular in the git-publish patch series management tool
(https://github.com/stefanha/git-publish/) in case you want to include
something similar in b4:

- "Profiles" (.gitpublishprofile or stored in .git/config) with To and
  CC addresses, --subject-prefix, cover letter templates, etc. The
  default profile is automatically loaded when you run "git publish". If
  the git repo you're working on includes a default profile then you
  don't need to specify any command-line options to send a correctly
  formatted patch series to the maintainers! A great help for one-off
  contributors but also a time-saver for regular contributors.

- An interactive mode that lets you inspect the final patch series and
  edit the CC list. Avoids mistakes and embarassment :).

- Saving the CC list between revisions (v2, v3, etc).

- Sending pull requests (for sub-maintainers) because there are times
  when pull requests also need a v2 or even a v3 ;). The workflow is
  basically the same as sending patch series.

- githooks(5) before sending patches series and pull requests. Useful
  for coding style and patch series linters.

The git-publish man page is here if you want to read more:
https://github.com/stefanha/git-publish/blob/master/git-publish.pod#quickstart

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Review process improvements

[Emily Shaffer]

On Thu, Dec 16, 2021 at 02:46:21PM -0800, Emily Shaffer wrote:

Reshuffling context....

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

Having had a very restful couple of weeks off, I've now read through all
the replies to this mail - thank you! - and will summarize as much as I
can.

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

Summary: Everybody pretty much hated this idea. ;) I count at least five
replies saying "please let's not, it's too cumbersome".

That makes sense to me. As Ævar mentioned - just because he and I are
working on config a lot right now, doesn't mean we will still care in
2023. And remembering to remove yourself from the MAINTAINERS file is a
pain.

Ævar suggested leaning in harder to improving the tooling around "who do
I CC?"
(https://lore.kernel.org/git/211220.86fsqnwkzs.gmgdl%40evledraar.gmail.com)
and I think that's a reasonable approach, if we pair it with updating
SubmittingPatches and MyFirstContribution to include recommending
running the "who do I CC?" script.

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

Randall suggested using a prefix/tagging approach to make it easy for
reviewers to filter by topic. I think that's a pretty good idea! But I
also think for it to be useful as a filtering tool we'd want to
formalize "this topic goes to this tag" - which brings us back along to
the cumbersome-ness of the MAINTAINERS file.

I see a couple options:

1) Eat the maintenance overhead of having a checked-in file mapping
filespec/topic to subject line tag, like the sample Randall gave
(https://lore.kernel.org/git/00b901d7f2d3%24c0d2ca20%2442785e60%24%40nexbridge.com)
but minus the "Reviewers:" section:

Submodules:
  Prefix: SUBM
  Files:
    submodule.[ch], git-submodule.sh, builtin/submodule.c,
    Documentation/git-submodule.txt
Platform:
  Linux:
    Prefix: LINUX
  NonStop:
    Prefix: NS

Having these mails all still delivered to git@vger.kernel.org means that
it's not such a big deal if the prefix becomes obsolete, and not
including an individual CC list means that nobody needs to remove
themselves from CC list after they stop caring about some part of the
codebase.

An alternative to subject lines like "Subject: SUBM: share config
between blah blah" might be possible. Some providers (at least Gmail,
and based on João's reply-to address, protonmail as well) allow mails
addressed to e.g. "my-name+from-git-list@example.com" to be delivered to
"my-name@example.com" regardless of what follows the +; if vger allows
this kind of thing, we could save some visual space by adopting the
convention that e.g. submodule topics should go to
"git-subm@vger.kernel.org" and allow folks to write their mail filters
accordingly. (I took a poke through IETF RFC5322 to see if this +
comment/tag/whatever-it-is is standardized and didn't see anything
promising, so maybe this alternative is a bad idea. Or maybe I didn't
Google well enough.)

2) Put that kind of mapping into an easier-to-modify place, like a wiki. The
downside is that, as far as I'm aware, we don't have or maintain an
existing git.git wiki, so it'd probably end up going unused as people
don't want to start looking at yet another discussion area.



Either way, "Check this file/page and determine what additional tag to
add to your outgoing patches" would need to be called out in
documentation like SubmittingPatches//MyFirstContribution - and modeled
by veteran prolific contributors in their own patches.

If we're generally happy with the idea of tagging rather than additional
lists, then I think there's nothing in the way of beginning to draft
changes to either of those docs in order to agree on the standard we
want to adopt. But I'd like to make sure that we are in agreement first.
Maybe the best way to find that agreement is via the doc patches
themselves?

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

This idea seemed generally well-received. I noticed that João was
curious to hear more about Google's review process; I'll state that
there is less magic than you might think, and whatever we do know will
certainly make it into a draft.

I think there is nothing standing in the way of sending out an outline
and/or first draft of such a document for us to all discuss and quibble
over. :)

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

It seems that folks liked this idea, and furthermore, that it would be
worth suggesting such a process - "ask your friend to read your commit
message!" - in one or both of SubmittingPatches and MyFirstContribution.

> 
> 4. Documentation changes to encourage commit message quality

I think I didn't hear much response to this other than "sure, sounds
fine" - so I expect that with the changes themselves will come more
detailed discussion. I also don't think there is anything stopping these
changes from happening.

---

In general, I think everybody seems enough on board for us to begin
sending doc patches. So the next step for me is to work out which of we
Googlers will handle which, and for us to start sending stuff for
review. Thank you to everybody who contributed on this thread and thanks
in advance for reviews on the patches to come.

 - Emily

Re: Review process improvements

[Emily Shaffer]

On Mon, Dec 20, 2021 at 09:27:41PM +0000, João Victor Bonfim wrote:
> 
> I added on to this on another e-mail thread, however, it got no
> response, specially because I didn't have an e-mail to reply to, so I
> am copy and pasting the messages here.

Since you mentioned not receiving a reply earlier, I thought I would
reply directly to your comments.

I have only recently seen you begin to post here, so welcome! and I
think I saw someone else mention in another thread, but I'll say again
now: in general, please wrap your lines at ~80ch when replying to the
mailing list; having very very long lines like the ones you sent is
annoying for some mail clients, so the Git list convention is to wrap
them. I rewrapped your mail below so that it would fit more easily into
my editor.

> 
> ‐‐‐‐‐‐‐ E-mail one ‐‐‐‐‐‐‐
>  Addressing point #1, titled "Draft a MAINTAINERS file": seems quite
>  reasonable, I would also like to address some matters about this,
>  first is that there is no point of contact for "trusted sources"
>  within the git project and it is quite negative for historical
>  purposes, because maintainers and more prolific parties will
>  inevitably retire or move on from Git themselves and their prestige
>  won't be recorded beyond their commit history within the project and
>  it might lead to their contributions being forgotten. Second is that
>  it is hard to know who is responsible to what from the get go without
>  being in the know already. Third, please make all entries on any and
>  such file that might auto send messages non-committal, why? Nobody
>  wants to receive a message from a third party that the git mailing
>  list deems "trustwothy" only for it to be some scam of sorts that
>  only happened because a modification to the file managed to fly under
>  the radar, so make it a one way pass and all tags are only to people,
>  not from.

I'm not really sure what you mean by "non-committal" here. I will say
that messages coming from the Git mailing list does not imply that
messages are safe in any way; we get plenty of spam and phishing mails
making it past vger.kernel.org's filters. The proposal of a MAINTAINERS
file was definitely not a proposal to modify the review process itself;
as always, the expectation is that code should be reviewed by a number
of contributors to ensure it's doing what it says it is trying to do. I
don't see that that will ever change.

>  Fourth, I, personally, only want to partake on discussions,
>  but barely want to see patches and commits, maybe allow for some sot
>  of group inheritance and group message allow or deny lists? So people
>  that don't want patches don't receive patches, but they can filter to
>  receive only "memory allocation" topics, but they won't receive
>  patches that are for memory allocation, because the "patches" and
>  "discussion" topics take higher system priority than the "memory
>  allocation" topic, be it user by user or system wide. Maybe also
>  auto-filter messages, even in a dumb way or in a sender dependant
>  way.

For what it's worth, in my Gmail web client I filter out any mails
beginning with `[PATCH` - because in web client I am like you and
usually only want to participate in broader discussions. So I think
there is already a solution for filtering the way that you want to.

> 
>  Addressing point #2, titled "Draft a ReviewingPatches doc", and point
>  #3, titled "Google Git team will review cover letters and commit
>  messages internally before sending series to the list": not much to
>  say beyond, people, share your reviewing know how, including you
>  Google folks, if you interpret the reviewing process as an algorithm,
>  it would make sense that all mechanisms of human interaction and
>  review to be shared as part of the code base. So please, Google
>  people, share what and how you do your reviews. It is also a matter
>  of security, if your review process isn't transparent, we can't know
>  whether we can trust everything you provide, because you might be
>  leaving or dismissing problems and it might fly under the radar or
>  malicious action could be taken and, since the group as a whole is
>  already trusted, some malicious code could be included, even if it is
>  not explicitly so.

As I mentioned in my mail, we are only conducting review of commit
messages and cover letters, not of code - specifically *because* it is
so important to perform in-depth code review out in the open, for the
reasons you say. Code review should always happen on this list, and I'm
not suggesting to change that. (That's true of patches submitted via
GitGitGadget too, by the way - we don't perform review in the comments
on those GGG pull requests, for these same reasons.)

As for the Googley review know-how.... like I mentioned in my reply to
the main thread a moment ago, there's not that much "secret sauce". :)
However, if you're curious, you might keep an eye on the #git-devel
channel - we have recently started doing public "review club" live
meetings and inviting the Git community to join us in reviewing patches
on the Git list. The next one is this week, so if you're interested and
the timezone works out, you'd be welcome to join us.

> 
>  Extra notes: As a person with ADHD, REAMEs can be daunting at times,
>  specially when they are boring word walls, and they can be incomplete
>  or repetitive if there are other documents addressing information
>  contained within them, maybe reference files such as
>  "Contributing.md" instead of copying them verbatim? An example would
>  be "To read more on how to contribute to projects, read
>  'Contributing.md'." instead of what is information contained within
>  "Contributing.md", it would help a lot with human to human
>  interactions.

Yes, I agree this is the best way to do documentation. Human ability to
parse or no - having the same information in two places means that
inevitably, one place will become stale. ;)

>  Thanks for the attention, take care!

Again, welcome to the project.

 - Emily

Re: Review process improvements

[João Victor Bonfim]

> That makes sense to me. As Ævar mentioned - just because he and I are
> working on config a lot right now, doesn't mean we will still care in
> 2023. And remembering to remove yourself from the MAINTAINERS file is a
> pain.

 Maybe make a "contributors" file instead and merely list there who have
contributed with Git, what have they contributed with (or group
contributors by their types of contribution) and means of contact, like
institutions they are a part of, e-mail addresses, telephone numbers,
while highlighting that all e-mails related to Git are expected to be
shared with the mailing list unless stated otherwise. Also make the
input of personal information beyond name voluntary.

> Some providers (at least Gmail, and based on João's reply-to address,
> protonmail as well) allow mails addressed to
> e.g. "my-name+from-git-list@example.com" to be delivered to
> "my-name@example.com" regardless of what follows the +;

 Although ProtonMail allows the use of + aliases, no filtering
is done automatically.

> I have only recently seen you begin to post here, so welcome! and I
> think I saw someone else mention in another thread, but I'll say again
> now: in general, please wrap your lines at ~80ch when replying to the
> mailing list;

 Is "80 ch" 80 American units? (half-joke)
 I also have no idea on how to configure that.

> I'm not really sure what you mean by "non-committal" here. I will say
> that messages coming from the Git mailing list does not imply that
> messages are safe in any way; we get plenty of spam and phishing mails
> making it past vger.kernel.org's filters. The proposal of a MAINTAINERS
> file was definitely not a proposal to modify the review process itself;
> as always, the expectation is that code should be reviewed by a number
> of contributors to ensure it's doing what it says it is trying to do. I
> don't see that that will ever change.

 Brain fog, I don't exactly remember either. From what I got I was
thinking about the servers sending messages in the name of the parties
listed, not only to them. Also I might be thinking of people sending
malicious code through commits and git operations and it causing
problems and also about e-mail addresses being compromised.

> For what it's worth, in my Gmail web client I filter out any mails
> beginning with `[PATCH` - because in web client I am like you and
> usually only want to participate in broader discussions. So I think
> there is already a solution for filtering the way that you want to.

 Can't we automate it at the source, though...
 ;A;

> As I mentioned in my mail, we are only conducting review of commit
> messages and cover letters, not of code - specifically because it is
> so important to perform in-depth code review out in the open, for the
> reasons you say. Code review should always happen on this list, and I'm
> not suggesting to change that. (That's true of patches submitted via
> GitGitGadget too, by the way - we don't perform review in the comments
> on those GGG pull requests, for these same reasons.)
>
> As for the Googley review know-how.... like I mentioned in my reply to
> the main thread a moment ago, there's not that much "secret sauce". :)

 I still believe that methods of reviewing commit messages might be
helpful if shared collectively.

> However, if you're curious, you might keep an eye on the #git-devel
> channel - we have recently started doing public "review club" live
> meetings and inviting the Git community to join us in reviewing patches
> on the Git list. The next one is this week, so if you're interested and
> the timezone works out, you'd be welcome to join us.

 I both don't know what is Devel and how to use it and don't believe
anxiety will allow me, specially since I am very socially anxious, I
am using a TV screen as a "temporary" monitor and it triggers my
sensory sensitivities, because of the amount of information and light
(having a text heavy profession might have been a bad idea), and...

*panic*

... anxiety??? (better not delve too deep into that can of worms)

> Yes, I agree this is the best way to do documentation. Human ability to
> parse or no - having the same information in two places means that
> inevitably, one place will become stale. ;)

 Maybe use <iframe>s to embed files onto each other? Also make explicit
or (inclusively) allow the frames to be hidden or collapsed with a button?

> Again, welcome to the project.

<3
Thank you!

 - João Victor Bonfim, please use any pronouns.

Re: Review process improvements

[Matthias Aßhauer]

On Tue, 4 Jan 2022, Emily Shaffer wrote:

> On Thu, Dec 16, 2021 at 02:46:21PM -0800, Emily Shaffer wrote:
>
> Reshuffling context....
>
>> 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.
>
> Having had a very restful couple of weeks off, I've now read through all
> the replies to this mail - thank you! - and will summarize as much as I
> can.
>
>> 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.
>
> Summary: Everybody pretty much hated this idea. ;) I count at least five
> replies saying "please let's not, it's too cumbersome".
>
> That makes sense to me. As Ævar mentioned - just because he and I are
> working on config a lot right now, doesn't mean we will still care in
> 2023. And remembering to remove yourself from the MAINTAINERS file is a
> pain.
>
> Ævar suggested leaning in harder to improving the tooling around "who do
> I CC?"
> (https://lore.kernel.org/git/211220.86fsqnwkzs.gmgdl%40evledraar.gmail.com)
> and I think that's a reasonable approach, if we pair it with updating
> SubmittingPatches and MyFirstContribution to include recommending
> running the "who do I CC?" script.
>
>> 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.
>
> Randall suggested using a prefix/tagging approach to make it easy for
> reviewers to filter by topic. I think that's a pretty good idea! But I
> also think for it to be useful as a filtering tool we'd want to
> formalize "this topic goes to this tag" - which brings us back along to
> the cumbersome-ness of the MAINTAINERS file.
>
> I see a couple options:
>
> 1) Eat the maintenance overhead of having a checked-in file mapping
> filespec/topic to subject line tag, like the sample Randall gave
> (https://lore.kernel.org/git/00b901d7f2d3%24c0d2ca20%2442785e60%24%40nexbridge.com)
> but minus the "Reviewers:" section:
>
> Submodules:
>  Prefix: SUBM
>  Files:
>    submodule.[ch], git-submodule.sh, builtin/submodule.c,
>    Documentation/git-submodule.txt
> Platform:
>  Linux:
>    Prefix: LINUX
>  NonStop:
>    Prefix: NS
>
> Having these mails all still delivered to git@vger.kernel.org means that
> it's not such a big deal if the prefix becomes obsolete, and not
> including an individual CC list means that nobody needs to remove
> themselves from CC list after they stop caring about some part of the
> codebase.
>
> An alternative to subject lines like "Subject: SUBM: share config
> between blah blah" might be possible. Some providers (at least Gmail,
> and based on João's reply-to address, protonmail as well) allow mails
> addressed to e.g. "my-name+from-git-list@example.com" to be delivered to
> "my-name@example.com" regardless of what follows the +; if vger allows
> this kind of thing, we could save some visual space by adopting the
> convention that e.g. submodule topics should go to
> "git-subm@vger.kernel.org" and allow folks to write their mail filters
> accordingly. (I took a poke through IETF RFC5322 to see if this +
> comment/tag/whatever-it-is is standardized and didn't see anything
> promising, so maybe this alternative is a bad idea. Or maybe I didn't
> Google well enough.)
>

The IETF calls it subaddressing. It's not strictly speaking standardised.
There is one RFC about how to handle subaddressing, RFC5233 [1] and one 
old draft to standardise it [2], but no real standard for it.

There's a lot more mail providers and MTAs that support it in some way,
but some use different separators and RFC5233 even mentions puting the 
detail part before the user part e.g. "from-git-list+my-name@example.com".

Other providers include Yahoo, iCloud, Outlook.com and Pobox [3] and MTAs 
include sendmail [4], postfix [3] and MS Exchange [5] (though apparently 
currently only Exchange Online and 365 instances), from a cursory search.

[1] https://datatracker.ietf.org/doc/html/rfc5233
[2] https://tools.ietf.org/id/draft-newman-email-subaddr-01.html
[3] https://en.wikipedia.org/wiki/Email_address#subaddressing
[4] https://people.cs.rutgers.edu/~watrous/plus-signs-in-email-addresses.html
[5] https://docs.microsoft.com/en-us/exchange/recipients-in-exchange-online/plus-addressing-in-exchange-online

> 2) Put that kind of mapping into an easier-to-modify place, like a wiki. The
> downside is that, as far as I'm aware, we don't have or maintain an
> existing git.git wiki, so it'd probably end up going unused as people
> don't want to start looking at yet another discussion area.

From my understanding we have a wiki on kernel.org [6] that's pretty 
official, but isn't very active.

Best regards

Matthias

[6] https://git.wiki.kernel.org/index.php/Main_Page

>
> Either way, "Check this file/page and determine what additional tag to
> add to your outgoing patches" would need to be called out in
> documentation like SubmittingPatches//MyFirstContribution - and modeled
> by veteran prolific contributors in their own patches.
>
> If we're generally happy with the idea of tagging rather than additional
> lists, then I think there's nothing in the way of beginning to draft
> changes to either of those docs in order to agree on the standard we
> want to adopt. But I'd like to make sure that we are in agreement first.
> Maybe the best way to find that agreement is via the doc patches
> themselves?
>
>> 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.
>
> This idea seemed generally well-received. I noticed that João was
> curious to hear more about Google's review process; I'll state that
> there is less magic than you might think, and whatever we do know will
> certainly make it into a draft.
>
> I think there is nothing standing in the way of sending out an outline
> and/or first draft of such a document for us to all discuss and quibble
> over. :)
>
>>
>> 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".
>
> It seems that folks liked this idea, and furthermore, that it would be
> worth suggesting such a process - "ask your friend to read your commit
> message!" - in one or both of SubmittingPatches and MyFirstContribution.
>
>>
>> 4. Documentation changes to encourage commit message quality
>
> I think I didn't hear much response to this other than "sure, sounds
> fine" - so I expect that with the changes themselves will come more
> detailed discussion. I also don't think there is anything stopping these
> changes from happening.
>
> ---
>
> In general, I think everybody seems enough on board for us to begin
> sending doc patches. So the next step for me is to work out which of we
> Googlers will handle which, and for us to start sending stuff for
> review. Thank you to everybody who contributed on this thread and thanks
> in advance for reviews on the patches to come.
>
> - Emily
>

Why GitGitGadget does not use Sender:, was Re: Review process improvements

[Johannes Schindelin]

Hi Junio,

On Wed, 22 Dec 2021, Junio C Hamano wrote:

> Konstantin Ryabitsev <konstantin@linuxfoundation.org> writes:
>
> > This would require pretending that we're authorized to send mail from the
> > domain name of the commit author, so this unfortunately won't work (and hence
> > the reason why GGG does it this way). E.g. say you have:
> >
> > From: foo@redhat.com
> > Subject: [PATCH] Fix foo
>
> Would it help to use "Sender:"?

You had suggested that before, and I explained why it does not work in
https://lore.kernel.org/git/nycvar.QRO.7.76.6.2008241445200.56@tvgsbejvaqbjf.bet/.
The gist is: GMail interferes with your cunning plan.

> When GGG or any other automation are trying to send e-mail on behalf of
> the person shown on "From:", I thought that it is the mechanism for them
> to use to identify themselves.

Ciao,
Dscho

Re: Why GitGitGadget does not use Sender:, was Re: Review process improvements

[Junio C Hamano]

Konstantin gave a more generally applicable answer to think about
this issue in <20211222213247.5dnj3zlj53lh6l32@meerkat.local> in a
separate thread last year.  I would not be surprised if GMail pays
attention to the reasoning he gave in that message and that is the
reason why it does not use the "Sender" as I hoped.  I expect other
e-mail providers should do the same.

Thanks.  

Re: Review process improvements

[João Victor Bonfim]

To address Junio Hamano's comment:
>  But don't spend too much time on it to waste your momentum.  Having
> sounding board in your buddies is much better than not having any,
> but your buddies may already share too much background context with
> you to blind them from noticing why it is unclear to outsiders.

 Maybe ask someone like your mom, dad, aunt, cousin, long past college acquaintance to comment on the commit/log/patch comment itself and maybe also the code published? It might me more helpful the less they understand about code or are knowledgeable about your personal quirks.

João Victor Bonfim (any pronouns).

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐

Em segunda-feira, 20 de dezembro de 2021 às 00:35, João Victor Bonfim <JoaoVictorBonfim@protonmail.com> escreveu:

> First and foremost, there is a saying in Brazil that goes by as "an empty bag never holds itself standing", so always remember to care for yourself, eat a snack, drink fluids of your choice (from tea, water and coffee to soup and porridge), rest a little, stop doom scrolling social media and so on.
>
> Second, want to address the giant essay by Emily Shaffer (aka: Review process improvements) and give my opinions on some of the points made. Third, sorry for the botched response, but I wasn't on the mailing list before the mail was posted, so couldn't directly respond the message.
>
> Addressing point #1, titled "Draft a MAINTAINERS file": seems quite reasonable, I would also like to address some matters about this, first is that there is no point of contact for "trusted sources" within the git project and it is quite negative for historical purposes, because maintainers and more prolific parties will inevitably retire or move on from Git themselves and their prestige won't be recorded beyond their commit history within the project and it might lead to their contributions being forgotten. Second is that it is hard to know who is responsible to what from the get go without being in the know already. Third, please make all entries on any and such file that might auto send messages non-committal, why? Nobody wants to receive a message from a third party that the git mailing list deems "trustwothy" only for it to be some scam of sorts that only happened because a modification to the file managed to fly under the radar, so make it a one way pass and all tags are only to people, not from. Fourth, I, personally, only want to partake on discussions, but barely want to see patches and commits, maybe allow for some sot of group inheritance and group message allow or deny lists? So people that don't want patches don't receive patches, but they can filter to receive only "memory allocation" topics, but they won't receive patches that are for memory allocation, because the "patches" and "discussion" topics take higher system priority than the "memory allocation" topic, be it user by user or
>
> system wide. Maybe also auto-filter messages, even in a dumb way or in a sender dependant way.
>
> Addressing point #2, titled "Draft a ReviewingPatches doc", and point #3, titled "Google Git team will review cover letters and commit messages internally before sending series to the list": not much to say beyond, people, share your reviewing know how, including you Google folks, if you interpret the reviewing process as an algorithm, it would make sense that all mechanisms of human interaction and review to be shared as part of the code base. So please, Google people, share what and how you do your reviews. It is also a matter of security, if your review process isn't transparent, we can't know whether we can trust everything you provide, because you might be leaving or dismissing problems and it might fly under the radar or malicious action could be taken and, since the group as a whole is already trusted, some malicious code could be included, even if it is not explicitly so.
>
> Addressing point #4, titled "Documentation changes to encourage commit message quality": This isn't stressed enough in many Git tutorials and the like, and it should, the commit messages are for the journaling of changes, so you or third parties can understand the thought process behind changes and not feel like headless chickens running around a barn whenever you attempt to understand what has been done, maybe addressing it on the source code of git and its documentation might help address this topic.
>
> Extra notes: As a person with ADHD, REAMEs can be daunting at times, specially when they are boring word walls, and they can be incomplete or repetitive if there are other documents addressing information contained within them, maybe reference files such as "Contributing.md" instead of copying them verbatim? An example would be "To read more on how to contribute to projects, read 'Contributing.md'." instead of what is information contained within "Contributing.md", it would help a lot with human to human interactions.
>
> Thanks for the attention, take care!
>
> João Victor Bonfim (any pronouns).

Re: Review process improvements

[João Victor Bonfim]

 First and foremost, there is a saying in Brazil that goes by as "an empty bag never holds itself standing", so always remember to care for yourself, eat a snack, drink fluids of your choice (from tea, water and coffee to soup and porridge), rest a little, stop doom scrolling social media and so on.
 Second, want to address the giant essay by Emily Shaffer (aka: Review process improvements) and give my opinions on some of the points made. Third, sorry for the botched response, but I wasn't on the mailing list before the mail was posted, so couldn't directly respond the message.

 Addressing point #1, titled "Draft a MAINTAINERS file": seems quite reasonable, I would also like to address some matters about this, first is that there is no point of contact for "trusted sources" within the git project and it is quite negative for historical purposes, because maintainers and more prolific parties will inevitably retire or move on from Git themselves and their prestige won't be recorded beyond their commit history within the project and it might lead to their contributions being forgotten. Second is that it is hard to know who is responsible to what from the get go without being in the know already. Third, please make all entries on any and such file that might auto send messages non-committal, why? Nobody wants to receive a message from a third party that the git mailing list deems "trustwothy" only for it to be some scam of sorts that only happened because a modification to the file managed to fly under the radar, so make it a one way pass and all tags are only to people, not from. Fourth, I, personally, only want to partake on discussions, but barely want to see patches and commits, maybe allow for some sot of group inheritance and group message allow or deny lists? So people that don't want patches don't receive patches, but they can filter to receive only "memory allocation" topics, but they won't receive patches that are for memory allocation, because the "patches" and "discussion" topics take higher system priority than the "memory allocation" topic, be it user by user or
system wide. Maybe also auto-filter messages, even in a dumb way or in a sender dependant way.

 Addressing point #2, titled "Draft a ReviewingPatches doc", and point #3, titled "Google Git team will review cover letters and commit messages internally before sending series to the list": not much to say beyond, people, share your reviewing know how, including you Google folks, if you interpret the reviewing process as an algorithm, it would make sense that all mechanisms of human interaction and review to be shared as part of the code base. So please, Google people, share what and how you do your reviews. It is also a matter of security, if your review process isn't transparent, we can't know whether we can trust everything you provide, because you might be leaving or dismissing problems and it might fly under the radar or malicious action could be taken and, since the group as a whole is already trusted, some malicious code could be included, even if it is not explicitly so.

 Addressing point #4, titled "Documentation changes to encourage commit message quality": This isn't stressed enough in many Git tutorials and the like, and it should, the commit messages are for the journaling of changes, so you or third parties can understand the thought process behind changes and not feel like headless chickens running around a barn whenever you attempt to understand what has been done, maybe addressing it on the source code of git and its documentation might help address this topic.

 Extra notes: As a person with ADHD, REAMEs can be daunting at times, specially when they are boring word walls, and they can be incomplete or repetitive if there are other documents addressing information contained within them, maybe reference files such as "Contributing.md" instead of copying them verbatim? An example would be "To read more on how to contribute to projects, read 'Contributing.md'." instead of what is information contained within "Contributing.md", it would help a lot with human to human interactions.

Thanks for the attention, take care!
João Victor Bonfim (any pronouns).