| Message ID | 6f71b1731f2aed9c2f4dc101bf4349344b575d73.1712699815.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | docs: recommend using contrib/contacts/git-contacts | expand |
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? 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:.
Eric Sunshine <sunshine@sunshineco.com> writes: >> +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), ... Well, this is another instance that I may be trying to be too helpful and over extending myself, which does not make the process scale well (the other one being the "one final resend after the list reached a consensus"). If the authors collect Acks and Reviewed-by's and resend after the list reached the concensus, it may take one extra iteration, but I no longer have to keep track of these trailers myself, which could be a big win. So, I dunno.
Eric Sunshine <sunshine@sunshineco.com> writes: > 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.
diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches index 8594a3dda36..392bbccc452 100644 --- a/Documentation/SubmittingPatches +++ b/Documentation/SubmittingPatches @@ -397,6 +397,36 @@ letter. [[send-patches]] === Sending your patches. +==== Choosing your reviewers + +:security-ml-ref: footnoteref:[security-ml] + +As mentioned at the beginning of the section, patches that may be +security relevant should not be submitted to the public mailing list +mentioned below, but should instead be sent privately to the Git +Security mailing list{security-ml-ref}. + +Send your patch with "To:" set to the mailing list, with "cc:" listing +people who are involved in the area you are touching (the `git contacts` +command in `contrib/contacts/` can help to +identify them), to solicit comments and reviews. Also, when you made +trial merges of your topic to `next` and `seen`, you may have noticed +work by others conflicting with your changes. There is a good possibility +that these people may know the area you are touching well. + +:current-maintainer: footnote:[The current maintainer: gitster@pobox.com] +:git-ml: footnote:[The mailing list: git@vger.kernel.org] + +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. + +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. + :security-ml: footnoteref:[security-ml,The Git Security mailing list: git-security@googlegroups.com] Before sending any patches, please note that patches that may be @@ -490,34 +520,6 @@ patch, format it as "multipart/signed", not a text/plain message that starts with `-----BEGIN PGP SIGNED MESSAGE-----`. That is not a text/plain, it's something else. -:security-ml-ref: footnoteref:[security-ml] - -As mentioned at the beginning of the section, patches that may be -security relevant should not be submitted to the public mailing list -mentioned below, but should instead be sent privately to the Git -Security mailing list{security-ml-ref}. - -Send your patch with "To:" set to the mailing list, with "cc:" listing -people who are involved in the area you are touching (the `git contacts` -command in `contrib/contacts/` can help to -identify them), to solicit comments and reviews. Also, when you made -trial merges of your topic to `next` and `seen`, you may have noticed -work by others conflicting with your changes. There is a good possibility -that these people may know the area you are touching well. - -:current-maintainer: footnote:[The current maintainer: gitster@pobox.com] -:git-ml: footnote:[The mailing list: git@vger.kernel.org] - -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. - -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. - == Subsystems with dedicated maintainers Some parts of the system have dedicated maintainers with their own