[PATCH] checkpatch: validate commit tag ordering

[PATCH] checkpatch: validate commit tag ordering

[Hendrik Hamerlinck]

Modified the checkpatch script to ensure that commit tags (e.g.,
Signed-off-by, Reviewed-by, Acked-by, Tested-by, etc.) appear in the
correct order according to kernel conventions [1].

checkpatch.pl will now emit a BAD_TAG_ORDER warning when tags are out of
the expected sequence. Multiple tags of the same type are allowed, but
they must also follow the order. 'Link:' tags in the changelog are still
allowed before the tag sequence begins, but once the sequence has started,
any 'Link:' tags must follow the ordered commit tags. 

Link: https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#ordering-of-commit-tags # [1]

Signed-off-by: Hendrik Hamerlinck <hendrik.hamerlinck@hammernet.be>
---
 Documentation/dev-tools/checkpatch.rst |  6 ++++
 scripts/checkpatch.pl                  | 40 ++++++++++++++++++++++++++
 2 files changed, 46 insertions(+)

diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
index 76bd0ddb0041..696b42bf4ff5 100644
--- a/Documentation/dev-tools/checkpatch.rst
+++ b/Documentation/dev-tools/checkpatch.rst
@@ -599,6 +599,12 @@ Commit message
 
     See: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes
 
+  **BAD_TAG_ORDER**
+    The tags in the commit message are not in the correct order according to
+    community conventions. Common tags like Signed-off-by, Reviewed-by,
+    Tested-by, Acked-by, Fixes, Cc, etc., should follow a standardized sequence.
+
+    See: https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#ordering-of-commit-tags
 
 Comparison style
 ----------------
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 664f7b7a622c..267ec02de9ec 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -661,6 +661,24 @@ foreach my $entry (@link_tags) {
 }
 $link_tags_search = "(?:${link_tags_search})";
 
+# Ordered commit tags
+our @commit_tags = (
+	"Fixes:",
+	"Reported-by:",
+	"Closes:",
+	"Originally-by:",
+	"Suggested-by:",
+	"Co-developed-by:",
+	"Signed-off-by:",
+	"Tested-by:",
+	"Reviewed-by",
+	"Acked-by:",
+	"Cc:",
+	"Link:"
+);
+our $commit_tag_pattern = join '|', map { quotemeta($_) } @commit_tags;
+our $commit_tags_regex = qr{(?xi: ^\s*($commit_tag_pattern))};
+
 our $tracing_logging_tags = qr{(?xi:
 	[=-]*> |
 	<[=-]* |
@@ -2712,6 +2730,8 @@ sub process {
 
 	my $checklicenseline = 1;
 
+	my $last_matched_tag;
+
 	sanitise_line_reset();
 	my $line;
 	foreach my $rawline (@rawlines) {
@@ -3258,6 +3278,26 @@ sub process {
 			}
 		}
 
+# Check commit tags sorting
+		if (!$in_header_lines && $line =~ $commit_tags_regex) {
+			my $tag = $1;
+			my ($tag_index) = grep { lc($commit_tags[$_]) eq lc($tag) } 0..$#commit_tags;
+
+			if ($last_matched_tag &&
+			    $last_matched_tag->{tag_index} > $tag_index) {
+				WARN("BAD_TAG_ORDER",
+				     "Tag '$tag' is out of order. Should come before '$last_matched_tag->{tag}'\n" . $herecurr);
+			}
+
+			# Allow link tags to occur before the commit tags
+			if (lc($tag) ne "link:" || defined $last_matched_tag) {
+				$last_matched_tag = {
+					tag       => $tag,
+					tag_index => $tag_index,
+				};
+			}
+		}
+
 # Check email subject for common tools that don't need to be mentioned
 		if ($in_header_lines &&
 		    $line =~ /^Subject:.*\b(?:checkpatch|sparse|smatch)\b[^:]/i) {
-- 
2.43.0

Re: [PATCH] checkpatch: validate commit tag ordering

[Akira Yokosawa]

Hello,

On Thu, 24 Jul 2025 09:20:32 +0200, Hendrik Hamerlinck wrote:
> Modified the checkpatch script to ensure that commit tags (e.g.,
> Signed-off-by, Reviewed-by, Acked-by, Tested-by, etc.) appear in the
> correct order according to kernel conventions [1].
> 
> checkpatch.pl will now emit a BAD_TAG_ORDER warning when tags are out of
> the expected sequence. Multiple tags of the same type are allowed, but
> they must also follow the order. 'Link:' tags in the changelog are still
> allowed before the tag sequence begins, but once the sequence has started,
> any 'Link:' tags must follow the ordered commit tags. 
> 
> Link: https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#ordering-of-commit-tags # [1]
>

So, you are citing ordering rules specific to the "tip" tree.

I don't think there is any wider consensus with regard to such strict
rules.

I have to say that your change will make a lot of unneeded noise for
most contributors.

If such warnings are only shown when the user opted in to the tip rule,
say, by using a "--tip" flag or something, they might be helpful.

BR, Akira

> Signed-off-by: Hendrik Hamerlinck <hendrik.hamerlinck@hammernet.be>
> ---
>  Documentation/dev-tools/checkpatch.rst |  6 ++++
>  scripts/checkpatch.pl                  | 40 ++++++++++++++++++++++++++
>  2 files changed, 46 insertions(+)
[...]

Re: [PATCH] checkpatch: validate commit tag ordering

[Jeff Johnson]

On 7/24/2025 12:20 AM, Hendrik Hamerlinck wrote:
> Modified the checkpatch script to ensure that commit tags (e.g.,
> Signed-off-by, Reviewed-by, Acked-by, Tested-by, etc.) appear in the
> correct order according to kernel conventions [1].
> 
> checkpatch.pl will now emit a BAD_TAG_ORDER warning when tags are out of
> the expected sequence. Multiple tags of the same type are allowed, but
> they must also follow the order. 'Link:' tags in the changelog are still
> allowed before the tag sequence begins, but once the sequence has started,
> any 'Link:' tags must follow the ordered commit tags. 
> 
> Link: https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#ordering-of-commit-tags # [1]
> 
> Signed-off-by: Hendrik Hamerlinck <hendrik.hamerlinck@hammernet.be>
> ---
>  Documentation/dev-tools/checkpatch.rst |  6 ++++
>  scripts/checkpatch.pl                  | 40 ++++++++++++++++++++++++++
>  2 files changed, 46 insertions(+)
> 
> diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
> index 76bd0ddb0041..696b42bf4ff5 100644
> --- a/Documentation/dev-tools/checkpatch.rst
> +++ b/Documentation/dev-tools/checkpatch.rst
> @@ -599,6 +599,12 @@ Commit message
>  
>      See: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes
>  
> +  **BAD_TAG_ORDER**
> +    The tags in the commit message are not in the correct order according to
> +    community conventions. Common tags like Signed-off-by, Reviewed-by,
> +    Tested-by, Acked-by, Fixes, Cc, etc., should follow a standardized sequence.
> +
> +    See: https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#ordering-of-commit-tags
>  
>  Comparison style
>  ----------------
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 664f7b7a622c..267ec02de9ec 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -661,6 +661,24 @@ foreach my $entry (@link_tags) {
>  }
>  $link_tags_search = "(?:${link_tags_search})";
>  
> +# Ordered commit tags
> +our @commit_tags = (
> +	"Fixes:",
> +	"Reported-by:",
> +	"Closes:",
> +	"Originally-by:",
> +	"Suggested-by:",
> +	"Co-developed-by:",
> +	"Signed-off-by:",
> +	"Tested-by:",
> +	"Reviewed-by",
> +	"Acked-by:",
> +	"Cc:",
> +	"Link:"
> +);
> +our $commit_tag_pattern = join '|', map { quotemeta($_) } @commit_tags;
> +our $commit_tags_regex = qr{(?xi: ^\s*($commit_tag_pattern))};
> +
>  our $tracing_logging_tags = qr{(?xi:
>  	[=-]*> |
>  	<[=-]* |
> @@ -2712,6 +2730,8 @@ sub process {
>  
>  	my $checklicenseline = 1;
>  
> +	my $last_matched_tag;
> +
>  	sanitise_line_reset();
>  	my $line;
>  	foreach my $rawline (@rawlines) {
> @@ -3258,6 +3278,26 @@ sub process {
>  			}
>  		}
>  
> +# Check commit tags sorting
> +		if (!$in_header_lines && $line =~ $commit_tags_regex) {
> +			my $tag = $1;
> +			my ($tag_index) = grep { lc($commit_tags[$_]) eq lc($tag) } 0..$#commit_tags;
> +
> +			if ($last_matched_tag &&
> +			    $last_matched_tag->{tag_index} > $tag_index) {
> +				WARN("BAD_TAG_ORDER",
> +				     "Tag '$tag' is out of order. Should come before '$last_matched_tag->{tag}'\n" . $herecurr);
> +			}
> +
> +			# Allow link tags to occur before the commit tags
> +			if (lc($tag) ne "link:" || defined $last_matched_tag) {
> +				$last_matched_tag = {
> +					tag       => $tag,
> +					tag_index => $tag_index,
> +				};
> +			}
> +		}
> +
>  # Check email subject for common tools that don't need to be mentioned
>  		if ($in_header_lines &&
>  		    $line =~ /^Subject:.*\b(?:checkpatch|sparse|smatch)\b[^:]/i) {

Seems this logic would fail when there are multiple reporters, such as in
commit 9a44b5e36cd699fdd2150a63fab225ac510c1971

Fixes: 49e47223ecc4 ("wifi: cfg80211: allocate memory for link_station info structure")
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/all/81f30515-a83d-4b05-a9d1-e349969df9e9@sabinyo.mountain/
Reported-by: syzbot+4ba6272678aa468132c8@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/all/68655325.a70a0220.5d25f.0316.GAE@google.com

Re: [PATCH] checkpatch: validate commit tag ordering

[Konstantin Ryabitsev]

On Thu, Jul 24, 2025 at 09:20:32AM +0200, Hendrik Hamerlinck wrote:
> Modified the checkpatch script to ensure that commit tags (e.g.,
> Signed-off-by, Reviewed-by, Acked-by, Tested-by, etc.) appear in the
> correct order according to kernel conventions [1].

As already indicated, this is the convention used by the TIP tree and is not
universal. Moreover, there is a lot more nuance to how trailers are used, with
many other subsystems strongly preferring chain-of-custody boundaries. For
example, the following trailers indicate a history of how the patch was
reviewed:

| Suggested-by: Sug Gester <sug@example.com>
| Signed-off-by: Alex Dev <alex@example.com>          -- boundary 1
| Acked-by: Acker Mack <acker@example.com>
| Tested-by: Test Rogen <test@example.com>
| Signed-off-by: Sub Maintainer <sub@example.com>     -- boundary 2
| Reviewed-by: Rev Yewer <rev@example.com>
| Tested-by: Integration Bot <int@example.com>
| Link: https://patch.msgid.link/foomsgid@exmple.com
| Signed-off-by: Main Tainer <main@example.com>       -- boundary 3

There are three chain of custody boundaries in the example above and in the
chain-of-custody scenario the trailers should NOT be moved around between
these boundaries, because each of the boundaries indicates what each
signed-off-by person is claiming as their responsibility.

Everything above boundary 1 is claimed by Alex Dev; all trailers above
boundary 2 were collected and applied by Sub Maintainer; all trailers
above boundary 3 were collected and applied by Main Tainer. Alex Dev has no
responsibility for the tag provided by the Integration Bot, so moving their
signed-off-by to the bottom of this series of trailer would imply that they
are.

I would leave the trailer order entirely alone and out of tools like
checkpatch, so this is a gentle but firm NACK from me.

-K

Re: [PATCH] checkpatch: validate commit tag ordering

[Krzysztof Kozlowski]

On 24/07/2025 09:20, Hendrik Hamerlinck wrote:
> Modified the checkpatch script to ensure that commit tags (e.g.,
> Signed-off-by, Reviewed-by, Acked-by, Tested-by, etc.) appear in the
> correct order according to kernel conventions [1].


These are not the conventions I use for my subsystems and ask others to
follow, so imposing TIP rules to all maintainers needs broad consensus,
not (yet) checkpatch.

What's more, I think above TIP rules are contradicting with existing,
widely used and approved toolset - b4. So no, if you want universal
tool, please use b4 or whatever b4 defines.

Best regards,
Krzysztof

Re: [PATCH] checkpatch: validate commit tag ordering

[Hendrik Hamerlinck]

On 7/24/25 09:20, Hendrik Hamerlinck wrote:
> Modified the checkpatch script to ensure that commit tags (e.g.,
> Signed-off-by, Reviewed-by, Acked-by, Tested-by, etc.) appear in the
> correct order according to kernel conventions [1].
Hello all,

Thank you for the feedback. I wasn’t aware that the tag ordering
conventions used in the TIP tree are not universally followed across all
kernel subsystems.

My motivation for this change came from a recent mistake I made in a patch
submission, where I incorrectly placed a Fixes: tag after the
Signed-off-by: line. I realized that checkpatch.pl didn’t flag this, and I
thought a warning might be helpful, especially for newer contributors like
myself.

I now realize that my approach is too strict by trying to enforce an order
for all tags. However, I still believe that a targeted warning could be
useful. Another mentee I work with recently made the same mistake, so it
may be a common pitfall.

Is there a general consensus on placing the first Fixes: tag at the start
of the tag sequence? If so, a warning might be helpful for newer
contributors?

I was still using checkpatch as that was how I initially learned it. I'll
definitely look into using b4 as well.

Kind regards,
Hendrik

Re: [PATCH] checkpatch: validate commit tag ordering

[Geert Uytterhoeven]

Hi Krzysztof,

On Fri, 25 Jul 2025 at 10:42, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On 24/07/2025 09:20, Hendrik Hamerlinck wrote:
> > Modified the checkpatch script to ensure that commit tags (e.g.,
> > Signed-off-by, Reviewed-by, Acked-by, Tested-by, etc.) appear in the
> > correct order according to kernel conventions [1].
>
> These are not the conventions I use for my subsystems and ask others to
> follow, so imposing TIP rules to all maintainers needs broad consensus,
> not (yet) checkpatch.
>
> What's more, I think above TIP rules are contradicting with existing,
> widely used and approved toolset - b4. So no, if you want universal
> tool, please use b4 or whatever b4 defines.

B4 does not follow the proper order:
  1. Multiple Reviewed-by tags may be added in a different order
      than given,
  2. When applying my own patches, b4 adds the given tags before
     instead of after my own SoB.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] checkpatch: validate commit tag ordering

[Krzysztof Kozlowski]

On 31/07/2025 13:55, Geert Uytterhoeven wrote:
> Hi Krzysztof,
> 
> On Fri, 25 Jul 2025 at 10:42, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>> On 24/07/2025 09:20, Hendrik Hamerlinck wrote:
>>> Modified the checkpatch script to ensure that commit tags (e.g.,
>>> Signed-off-by, Reviewed-by, Acked-by, Tested-by, etc.) appear in the
>>> correct order according to kernel conventions [1].
>>
>> These are not the conventions I use for my subsystems and ask others to
>> follow, so imposing TIP rules to all maintainers needs broad consensus,
>> not (yet) checkpatch.
>>
>> What's more, I think above TIP rules are contradicting with existing,
>> widely used and approved toolset - b4. So no, if you want universal
>> tool, please use b4 or whatever b4 defines.
> 
> B4 does not follow the proper order:

There is no "proper order" in terms of absolute facts.

>   1. Multiple Reviewed-by tags may be added in a different order
>       than given,

Maybe this could be fixed.

>   2. When applying my own patches, b4 adds the given tags before
>      instead of after my own SoB.

This is working exactly as expected (intentional), explained few times
by Konstantin. Some people of course have different opinion and prefer
different order (I know few subsystems), but majority I think accepted
Konstantin's explanation.

Best regards,
Krzysztof

Re: [PATCH] checkpatch: validate commit tag ordering

[Jani Nikula]

On Thu, 31 Jul 2025, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On 31/07/2025 13:55, Geert Uytterhoeven wrote:
>> B4 does not follow the proper order:
>
> There is no "proper order" in terms of absolute facts.

Let's just decide whatever order b4 uses *is* the proper order, and save
ourselves endless hours of debating! :p

BR,
Jani.


-- 
Jani Nikula, Intel

Re: [PATCH] checkpatch: validate commit tag ordering

[Mauro Carvalho Chehab]

Em Fri, 01 Aug 2025 10:55:55 +0300
Jani Nikula <jani.nikula@intel.com> escreveu:

> On Thu, 31 Jul 2025, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > On 31/07/2025 13:55, Geert Uytterhoeven wrote:  
> >> B4 does not follow the proper order:  
> >
> > There is no "proper order" in terms of absolute facts.  
> 
> Let's just decide whatever order b4 uses *is* the proper order, and save
> ourselves endless hours of debating! :p

I don't think it makes sense to have a "proper order" verified on
checkpatch, as some tags may appear on different places.

For instance, the custody chain was designed to have SoBs appearing
in different places:

- author(s) SoB together co-developed-by are usually the first ones;
- then patches may have been reviewed, tested, acked or passed on some
  other trees, gaining tags like tested-by, R-B, A-B, SoB, Cc;
- the subsystem maintainer will add his SoB in the end.

non-custody chain tags, like fixes, closes, reported-by...
usually comes first, but I don't think we need to enforce an specific
order.

Link, for instance, could be used on different places, with different
purposes.

At least for me, the only part that shall really follow a proper
order is the custody chain: It has to follow how the patch was handled,
from the authors at the top up to the maintainers at the bottom.

Thanks,
Mauro

Re: [PATCH] checkpatch: validate commit tag ordering

[Konstantin Ryabitsev]

On Sat, Aug 02, 2025 at 12:12:00PM +0200, Mauro Carvalho Chehab wrote:
> > Let's just decide whatever order b4 uses *is* the proper order, and save
> > ourselves endless hours of debating! :p
> 
> I don't think it makes sense to have a "proper order" verified on
> checkpatch, as some tags may appear on different places.
> 
> For instance, the custody chain was designed to have SoBs appearing
> in different places:
> 
> - author(s) SoB together co-developed-by are usually the first ones;
> - then patches may have been reviewed, tested, acked or passed on some
>   other trees, gaining tags like tested-by, R-B, A-B, SoB, Cc;
> - the subsystem maintainer will add his SoB in the end.
> 
> non-custody chain tags, like fixes, closes, reported-by...
> usually comes first, but I don't think we need to enforce an specific
> order.

I wholeheartedly agree -- it really doesn't matter the order the trailers are
in, as long as it's clear who is the person who pulled the trailer in, which
is why I stick to the chain of custody. I'm pretty sure nobody has ever looked
at a commit and went "I can't believe they put the Link trailer above the
Suggested-by trailer," so enforcing it in checkpatch seems like wasted effort
to me.

-K