[PATCH 0/6] SubmittingPatches: update and flesh out trailer sections

[PATCH 0/6] SubmittingPatches: update and flesh out trailer sections

[kristofferhaugsbakk]

From: Kristoffer Haugsbakk <code@khaugsbakk.name>

Topic name: kh/submitting-patches-trailers

Topic summary: Flesh out and update the trailer sections.

All of these points have come up on the mailing list. At least for me.
And `Based-on-patch-by` is a nice-to-have documented kind of thing.

I have had these lying around since January since I got distracted. I just
wrote this cover letter today. But I think this still makes sense.

[1/6] SubmittingPatches: encourage trailer use for substantial help
[2/6] SubmittingPatches: discuss non-ident trailers
[3/6] SubmittingPatches: discourage common Linux trailers
[4/6] SubmittingPatches: document Based-on-patch-by trailer
[5/6] SubmittingPatches: be consistent with trailer markup
[6/6] SubmittingPatches: note that trailer order matters

 Documentation/SubmittingPatches | 46 +++++++++++++++++++++++++--------
 1 file changed, 35 insertions(+), 11 deletions(-)


base-commit: 1ff279f3404a482a83fb04c7457e41ab26884aea
-- 
2.54.0.22.g9e26862b904

[PATCH 1/6] SubmittingPatches: encourage trailer use for substantial help

[kristofferhaugsbakk]

From: Kristoffer Haugsbakk <code@khaugsbakk.name>

Trailers beyond the mandatory s-o-b are regularly used based on my
last two years of reading the mailing list. Moreover, reviewers might
encourage it.[1]

This is also in line with the project crediting both commit authors and
people mentioned in trailers each release; “Nobody is THE one making
contribution”.[2]

Adding trailers is already encouraged, but in the section `send-patches`.
Let’s replace “If you like” with outright encouragment in this section
so that all trailer discussion (except s-o-b; see `sign-off` section) is
contained in this section; a link to from `send-patches` makes this
information equally visible.

Now we need to make a heading for `commit-trailers` in order for the
HTML output to make sense.

At the same, it is important to temper this recommendation to a sign-
ificant enough contribution; in my experience beginners can be eager
to add a trailer for everyone who replies with an action point that is
followed up on.

Let’s also spell out that these trailers should follow the Git author/
committer format. One might naturally just write the name, but in that
case it will not be picked up by:

    git shortlog --group=trailer:<key>

and normalization via `.mailmap` will not work.

Also introduce the list of common trailers as such. Granted, this is
already implied by the later paragraph about “create your own trailer”,
so this just frontloads this information.

† 1: https://lore.kernel.org/git/CAP8UFD0POvYDgGtEx8GBhvKkd8XzzWQsy8XxAKL9M3+uz3ka+w@mail.gmail.com/#:~:text=for%20at%20least
† 2: https://lore.kernel.org/git/xmqqzh248sy0.fsf@gitster.c.googlers.com/

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
 Documentation/SubmittingPatches | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index 176567738d4..0b12badf86d 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -443,8 +443,16 @@ identifying, and not misleading.
 The goal of this policy is to allow us to have sufficient information to contact
 you if questions arise about your contribution.
 
+=== Commit trailers
 [[commit-trailers]]
-If you like, you can put extra trailers at the end:
+It is polite to credit people who have helped with your work to a
+substantial enough degree. This project uses commit trailers for that,
+where the credited person is written out like a Git author, i.e. with
+both their name and their email address. Note that the threshold to
+credit someone is a judgement call, and crediting someone for simple
+review work is certainly not necessary.
+
+These are the common trailers in use:
 
 . `Reported-by:` is used to credit someone who found the bug that
   the patch attempts to fix.
@@ -562,8 +570,8 @@ 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.
+`Tested-by:` (see <<commit-trailers,Commit trailers>>), and "cc:" them
+when sending such a final version for inclusion.
 
 ==== `format-patch` and `send-email`
 
-- 
2.54.0.22.g9e26862b904

[PATCH 2/6] SubmittingPatches: discuss non-ident trailers

[kristofferhaugsbakk]

From: Kristoffer Haugsbakk <code@khaugsbakk.name>

Briefly discuss trailers that do not credit people. This continues the
discussion from the previous commit about using trailers for *people*.

Using non-ident trailers can be relevant. The contributor should just be
encouraged to consider whether it is useful or not.

The larger trend here is to discourage using trailers as a dumping
ground for any kind of metadata in the spirit of “it doesn’t hurt”.

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
 Documentation/SubmittingPatches | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index 0b12badf86d..51c308a89a8 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -474,7 +474,10 @@ These are the common trailers in use:
 
 While you can also create your own trailer if the situation warrants it, we
 encourage you to instead use one of the common trailers in this project
-highlighted above.
+highlighted above. A trailer that credits someone might be more likely
+to be accepted since these are the most common ones. But another kind of
+trailer might be relevant, for example to link to an issue tracker
+belonging to a downstream project that is affected by a bug in Git.
 
 Only capitalize the very first letter of the trailer, i.e. favor
 "Signed-off-by" over "Signed-Off-By" and "Acked-by:" over "Acked-By".
-- 
2.54.0.22.g9e26862b904

[PATCH 3/6] SubmittingPatches: discourage common Linux trailers

[kristofferhaugsbakk]

From: Kristoffer Haugsbakk <code@khaugsbakk.name>

The Linux Kernel regularly uses trailers (or “tags”) `Fixes` and
`Link`. Sometimes people submit patches to this project with them.
They have their use in that project but it is not clear what purpose
they would serve here.

For `Fixes`: Linux has many trees, and applying patches with
cherry-picks is common. A `Fixes` trailer in commit C2 pointing to
commit C1 helps the cherry-picker figure out that she probably needs
C2 if she wants to apply C1. See linux/d5d6281a (checkpatch: check for
missing Fixes tags, 2024-06-11):[1]

    Why are stable patches encouraged to have a fixes tag?  Some people
    mark their stable patches as "# 5.10" etc.  This is useful but a
    Fixes tag is still a good idea.  For example, the Fixes tag helps in
    review.  It helps people to not cherry-pick buggy patches without
    also cherry-picking the fix.

In contrast the Git project has few trees (to my knowledge), and there
is much less need to cherry-pick fixes as opposed to either using
backmerges or rebasing all of the downstream tree’s commits on top of
git.git `master` from time to time.

This project does regularly mention what commits a patch/commit fixes,
but that is done inline in the commit message proper (c.f. the trailer
block of the message).

For `Link`: These are used both to link back to the patch submission as
well as with footnotes. In contrast this project has `refs/notes/amlog`
for linking back to the patch submissions, and footnotes are only used
in the commit message proper.

† 1: Commit linux/d5d6281a has “linux” in front of it since this commit
     is from the Linux Kernel, not Git. Example of a Linux tree—as well
     as an example of `Link`—is [2].

Link: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/ [2]
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
 Documentation/SubmittingPatches | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index 51c308a89a8..5dc32128883 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -479,6 +479,10 @@ to be accepted since these are the most common ones. But another kind of
 trailer might be relevant, for example to link to an issue tracker
 belonging to a downstream project that is affected by a bug in Git.
 
+Other projects might regularly refer to other kinds of data, like
+`Fixes:` and `Link:` in the Linux Kernel project, but these ones in
+particular are not used in this project.
+
 Only capitalize the very first letter of the trailer, i.e. favor
 "Signed-off-by" over "Signed-Off-By" and "Acked-by:" over "Acked-By".
 
-- 
2.54.0.22.g9e26862b904

[PATCH 4/6] SubmittingPatches: document Based-on-patch-by trailer

[kristofferhaugsbakk]

From: Kristoffer Haugsbakk <code@khaugsbakk.name>

This trailer comes up often enough and the use case is not fully covered
by the other trailers here. For example, it is sometimes better to use
this trailer instead of `Co-authored-by:`.

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---

Notes (series):
    Note: “*can be* used”. Not “is”. Based on practical usage, I think
    it would be wrong to claim that someone sending a literal patch
    snippet via email means that this is the tag that you ought to
    use. I think e.g. `Helped-by` might be used instead.
    
    I also think that the “is” for `Helped-by` is too strongly worded
    for the same reasons.

 Documentation/SubmittingPatches | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index 5dc32128883..c1d12e38b28 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -465,6 +465,10 @@ These are the common trailers in use:
   and found it to have the desired effect.
 . `Co-authored-by:` is used to indicate that people exchanged drafts
    of a patch before submitting it.
+. `Based-on-patch-by:` can be used when someone else authored parts of
+  the patch that you are submitting. This might be relevant if someone
+  sent a patch to the mailing list without a commit message or a
+  `Signed-off-by:` and you have picked it up.
 . `Helped-by:` is used to credit someone who suggested ideas for
   changes without providing the precise changes in patch form.
 . `Mentored-by:` is used to credit someone with helping develop a
-- 
2.54.0.22.g9e26862b904

[PATCH 5/6] SubmittingPatches: be consistent with trailer markup

[kristofferhaugsbakk]

From: Kristoffer Haugsbakk <code@khaugsbakk.name>

The rest of this section and (most importantly) the list has decided to
use `<key>:`. So let’s use backticks (`) and a colon (:) throughout the
document.

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
 Documentation/SubmittingPatches | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index c1d12e38b28..3d2e9ecfbb8 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -374,7 +374,7 @@ or, on an older version of Git without support for --pretty=reference:
 ....
 
 [[sign-off]]
-=== Certify your work by adding your `Signed-off-by` trailer
+=== Certify your work by adding your `Signed-off-by:` trailer
 
 To improve tracking of who did what, we ask you to certify that you
 wrote the patch or have the right to pass it on under the same license
@@ -411,7 +411,7 @@ d. I understand and agree that this project and the contribution
    this project or the open source license(s) involved.
 ____
 
-you add a "Signed-off-by" trailer to your commit, that looks like
+you add a `Signed-off-by:` trailer to your commit, that looks like
 this:
 
 ....
@@ -421,7 +421,7 @@ this:
 This line can be added by Git if you run the git-commit command with
 the -s option.
 
-Notice that you can place your own `Signed-off-by` trailer when
+Notice that you can place your own `Signed-off-by:` trailer when
 forwarding somebody else's patch with the above rules for
 D-C-O.  Indeed you are encouraged to do so.  Do not forget to
 place an in-body "From: " line at the beginning to properly attribute
@@ -433,7 +433,7 @@ your patch differs from project to project, so it may be different
 from that of the project you are accustomed to.
 
 [[real-name]]
-Please use a known identity in the `Signed-off-by` trailer, since we cannot
+Please use a known identity in the `Signed-off-by:` trailer, since we cannot
 accept anonymous contributions. It is common, but not required, to use some form
 of your real name. We realize that some contributors are not comfortable doing
 so or prefer to contribute under a pseudonym or preferred name and we can accept
@@ -488,7 +488,7 @@ Other projects might regularly refer to other kinds of data, like
 particular are not used in this project.
 
 Only capitalize the very first letter of the trailer, i.e. favor
-"Signed-off-by" over "Signed-Off-By" and "Acked-by:" over "Acked-By".
+`Signed-off-by:` over `Signed-Off-By:` and `Acked-by:` over `Acked-By:`.
 
 [[ai]]
 === Use of Artificial Intelligence (AI)
@@ -610,7 +610,7 @@ Here is a link:MyFirstContribution.html#v2-git-send-email[step-by-step guide] on
 how to submit updated versions of a patch series.
 
 If your log message (including your name on the
-`Signed-off-by` trailer) is not writable in ASCII, make sure that
+`Signed-off-by:` trailer) is not writable in ASCII, make sure that
 you send off a message in the correct encoding.
 
 WARNING: Be wary of your MUAs word-wrap
@@ -630,7 +630,7 @@ previously sent.
 The `git format-patch` command follows the best current practice to
 format the body of an e-mail message.  At the beginning of the
 patch should come your commit message, ending with the
-`Signed-off-by` trailers, and a line that consists of three dashes,
+`Signed-off-by:` trailers, and a line that consists of three dashes,
 followed by the diffstat information and the patch itself.  If
 you are forwarding a patch from somebody else, optionally, at
 the beginning of the e-mail message just before the commit
-- 
2.54.0.22.g9e26862b904

[PATCH 6/6] SubmittingPatches: note that trailer order matters

[kristofferhaugsbakk]

From: Kristoffer Haugsbakk <code@khaugsbakk.name>

It matters where you put the s-o-b; it should be last. You are signing
off on the patch as well as the whole message up to that point.

This also makes it clear who added what:

    Acked-by: The Reviewer <r@example.org>
    Signed-off-by: The Contributor <c@example.org>
    Acked-by: The (Late) Reviewer <late@example.org>
    Signed-off-by: The Maintainer <m@example.org>

The the first ack was added by the contributor and the second one was
added by the maintainer.

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
 Documentation/SubmittingPatches | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index 3d2e9ecfbb8..2045708bdf5 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -490,6 +490,11 @@ particular are not used in this project.
 Only capitalize the very first letter of the trailer, i.e. favor
 `Signed-off-by:` over `Signed-Off-By:` and `Acked-by:` over `Acked-By:`.
 
+Note that these trailers should come before your `Signed-off-by:`
+trailer. You are signing off to the patch as well as the message. This
+also makes it clear who added trailers when multiple people have signed
+off on a patch.
+
 [[ai]]
 === Use of Artificial Intelligence (AI)
 
-- 
2.54.0.22.g9e26862b904

Re: [PATCH 6/6] SubmittingPatches: note that trailer order matters

[Junio C Hamano]

kristofferhaugsbakk@fastmail.com writes:

> From: Kristoffer Haugsbakk <code@khaugsbakk.name>
>
> It matters where you put the s-o-b; it should be last. You are signing
> off on the patch as well as the whole message up to that point.
>
> This also makes it clear who added what:
>
>     Acked-by: The Reviewer <r@example.org>
>     Signed-off-by: The Contributor <c@example.org>
>     Acked-by: The (Late) Reviewer <late@example.org>
>     Signed-off-by: The Maintainer <m@example.org>
>
> The the first ack was added by the contributor and the second one was
> added by the maintainer.
>
> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
> ---
>  Documentation/SubmittingPatches | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
> index 3d2e9ecfbb8..2045708bdf5 100644
> --- a/Documentation/SubmittingPatches
> +++ b/Documentation/SubmittingPatches
> @@ -490,6 +490,11 @@ particular are not used in this project.
>  Only capitalize the very first letter of the trailer, i.e. favor
>  `Signed-off-by:` over `Signed-Off-By:` and `Acked-by:` over `Acked-By:`.
>  
> +Note that these trailers should come before your `Signed-off-by:`
> +trailer. You are signing off to the patch as well as the message. This
> +also makes it clear who added trailers when multiple people have signed
> +off on a patch.

Perhaps first mention the underlying rule that they are added in the
order that helps us to understand the chronological order of events.
That would avoid giving a wrong impression that the nature of each
trailer keys determine the order of these lines.

Thanks.