Re: Review process improvements

[Emily Shaffer <emilyshaffer@google.com>]

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