Re: [PATCH v3 5/8] SubmittingPatches: discuss reviewers first

From: Linus Arver <linusa@google.com>
Date: Apr 10, 2024, 01:13 UTC
Message-ID: <owlycyqyrpdq.fsf@fine.c.googlers.com>
In-Reply-To: <CAPig+cSw5wsYpm4Szk6HzgT3u+wMVz77NfqR1rLJrmCzejxvmg@mail.gmail.com>

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 22 quoted lines

> On Tue, Apr 9, 2024 at 5:57 PM Linus Arver via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>> No matter how well someone configures their email tooling, understanding
>> who to send the patches to is something that must always be considered.
>> So discuss it first instead of at the end.
>>
>> In the following commit we will clean up the (now redundant) discussion
>> about sending security patches to the Git Security mailing list.
>>
>> Signed-off-by: Linus Arver <linusa@google.com>
>> ---
>> diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
>> @@ -397,6 +397,36 @@ letter.
>> +After the list reached a consensus that it is a good idea to apply the
>> +patch, re-send it with "To:" set to the maintainer{current-maintainer}
>> +and "cc:" the list{git-ml} for inclusion.  This is especially relevant
>> +when the maintainer did not heavily participate in the discussion and
>> +instead left the review to trusted others.
>
> This isn't a new problem since you're merely relocating this text
> (thus, very likely may be outside the scope of this series), but is
> this recommendation still accurate?

I don't have much history on this list to know one way or the other, but it would certainly help to double-check all of the advice contained in here for accuracy.

I also think that we need to add some more structure to the SubmittingPatches doc. It is currently pretty long and could use some help in being broken up a bit more.

One thing I noticed while drafting this series was that we don't really separate minutiae from what is _really_ important. For example even the advice around adding "Acked-by:" and other trailers --- is it really critical? Other than the "Signed-off-by: " of the patch author (required for legal reasons), it's not the end of the world if someone forgot to add a "Reviewed-by: ". We should do a better job of separating absolutely critical things that must be done correctly to ensure smooth function of the review process, from the rest that are not so important.