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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Apr 10, 2024, 00:27 UTC
Message-ID: <CAPig+cSw5wsYpm4Szk6HzgT3u+wMVz77NfqR1rLJrmCzejxvmg@mail.gmail.com>
In-Reply-To: <6f71b1731f2aed9c2f4dc101bf4349344b575d73.1712699815.git.gitgitgadget@gmail.com>

On Tue, Apr 9, 2024 at 5:57 PM Linus Arver via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 16 quoted lines

> 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? Although I'm unable to locate it in the mailing list, I have some vague recollection of Junio relatively recently wondering why a patch series had been resent with no changes and why he had been made the direct To: recipient. It turned out that the author was following the above instructions.

Generally speaking, Junio is quite good at picking up a patch series without the author having to follow these instructions to resend a patch series with no changes other than the To: header, so such instructions place unnecessary burden upon both submitters as well as reviewers (who have to spend extra cycles wondering why a series was rerolled and whether any changes were made).

It would probably be more helpful (and less wasteful of reviewer time) to instruct the patch submitter to monitor "What's Cooking" and Junio's "seen" branch, and to ping the list (after a week or two) if the patch series hasn't been picked up or seen any response.

> +Do not forget to add trailers such as `Acked-by:`, `Reviewed-by:` and
> +`Tested-by:` lines as necessary to credit people who helped your
> +patch, and "cc:" them when sending such a final version for inclusion.

Again, not a new problem introduced by this patch, but it seems like all of these are actively wrong. In every case, these trailers are _given_ by reviewers _after_ a series has been submitted (thus, too late for the author to add them), and Junio typically is the one who latches the Reviewed-by:, Acked-by:, etc. by adding the trailer to the patches already in his tree.

Instead of the above, much more useful trailers that a patch author can add are Helped-by: and Reported-by:.