From: Philippe Schenker <[email protected]>
The page about submitting patches in
Documentation/process/submitting-patches.rst is very specific on how that
tag should be formatted: 'Fixes: <12+ chars of sha1> (\"<title line>\")'
Add a rule that warns if this format does not match. This commit is
introduced as in the past commits have been sent multiple times with
having the word commit also in the Fixes: tag which had to be corrected
by the maintainers. [1]
[1] https://lore.kernel.org/all/[email protected]/
Signed-off-by: Philippe Schenker <[email protected]>
---
scripts/checkpatch.pl | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 79e759aac543..0d7ce0c3801a 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3438,6 +3438,13 @@ sub process {
}
}
+# Check fixes tag format
+ if ($in_commit_log && ($line =~ /^\s*Fixes:/i) &&
+ !($line =~ /^\s*Fixes:\s[0-9a-f]{12,40}\s\(\".*\"\)/)) {
+ WARN("FIXES_TAG_FORMAT",
+ "Possible wrong format on Fixes: tag, please use format Fixes: <12+ chars of sha1> (\"<title line>\")\n" . $herecurr);
+ }
+
# ignore non-hunk lines and lines being removed
next if (!$hunk_line || $line =~ /^-/);
--
2.37.2
On Wed, 2022-09-07 at 14:35 +0200, Philippe Schenker wrote:
> From: Philippe Schenker <[email protected]>
>
> The page about submitting patches in
> Documentation/process/submitting-patches.rst is very specific on how that
> tag should be formatted: 'Fixes: <12+ chars of sha1> (\"<title line>\")'
>
> Add a rule that warns if this format does not match. This commit is
> introduced as in the past commits have been sent multiple times with
> having the word commit also in the Fixes: tag which had to be corrected
> by the maintainers. [1]
I preferred your first patch that added the commit description match
as that's a fairly common defect.
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -3438,6 +3438,13 @@ sub process {
> }
> }
>
> +# Check fixes tag format
> + if ($in_commit_log && ($line =~ /^\s*Fixes:/i) &&
> + !($line =~ /^\s*Fixes:\s[0-9a-f]{12,40}\s\(\".*\"\)/)) {
All fixes lines should start in the first column.
This allows spaces at the start of the line and the only white space
allowed after Fixes: and after the SHA1 should be a space not a tab.
I think the test better if it checks for a SHA1 after fixes.
And IMO
!(foo =~ /bar.../)
is better written as
foo !~ /bar.../
so
if ($in_commit_log &&
$line =~ /^\s*Fixes:?\s*[0-9a-f]{5,}\b/i &&
$line !~ /^Fixes: [0-9a-f]{12,40} \(\".*\"\)/)) {
Though it's arguable that the SHA1 should _only_ be length 12
and not longer.
Hi Joe,
On Wed, 07 Sep 2022 08:18:31 -0700 Joe Perches <[email protected]> wrote:
>
> I think the test better if it checks for a SHA1 after fixes.
>
> And IMO
>
> !(foo =~ /bar.../)
>
> is better written as
>
> foo !~ /bar.../
>
> so
>
> if ($in_commit_log &&
> $line =~ /^\s*Fixes:?\s*[0-9a-f]{5,}\b/i &&
> $line !~ /^Fixes: [0-9a-f]{12,40} \(\".*\"\)/)) {
>
> Though it's arguable that the SHA1 should _only_ be length 12
> and not longer.
It should be allowed to be longer - eventaully we will need to move on
from 12 as the repo gets bigger. Also, any line matching /^\s*Fixes:/i
should be checked, because people do add extra words before the SHA1
and sometimes just other text. You will get some hits that are not
meant to be Fixes tags, but very few.
--
Cheers,
Stephen Rothwell
Hi Philippe,
Thanks for adding me to CC.
I missed this mail earlier today when I sent out v3 so I could not add
you to CC, sorry about that. Please check [1] and I be sure to CC you if
there is a v4.
1. https://lore.kernel.org/linux-doc/[email protected]/
On 2022-09-08 10:01:38 +0000, Philippe Schenker wrote:
> On Wed, 2022-09-07 at 08:18 -0700, Joe Perches wrote:
> > On Wed, 2022-09-07 at 14:35 +0200, Philippe Schenker wrote:
> > > From: Philippe Schenker <[email protected]>
> > >
> > > The page about submitting patches in
> > > Documentation/process/submitting-patches.rst is very specific on how
> > > that
> > > tag should be formatted: 'Fixes: <12+ chars of sha1> (\"<title
> > > line>\")'
> > >
> > > Add a rule that warns if this format does not match. This commit is
> > > introduced as in the past commits have been sent multiple times with
> > > having the word commit also in the Fixes: tag which had to be
> > > corrected
> > > by the maintainers. [1]
> >
> > I preferred your first patch that added the commit description match
> > as that's a fairly common defect.
>
> Hi Joe, thanks for your review!
>
> This patch is my first one that I'm sending. I was not aware that Niklas
> sent the exact same thing few days earlier. Maybe you mix these two
> submissions. [1]
>
> How do we proceed? I guess it is up to you which approach you like
> better. Niklas has good parts in his submission which I could take in or
> I contribute in his v2 of the patch.
>
> Certainly, this shows that the check we're trying to add is helpful ????.
>
> Anyway, I'll answer your comments that not already got answered by
> Stephen.
>
> [1]
> https://lore.kernel.org/all/[email protected]/
>
> >
> > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > []
> > > @@ -3438,6 +3438,13 @@ sub process {
> > > }
> > > }
> > >
> > > +# Check fixes tag format
> > > + if ($in_commit_log && ($line =~ /^\s*Fixes:/i) &&
> > > + !($line =~ /^\s*Fixes:\s[0-9a-
> > > f]{12,40}\s\(\".*\"\)/)) {
> >
> > All fixes lines should start in the first column.
>
> Agree, I didn't want to make it too strict but this can be easily
> changed of course.
>
> >
> > This allows spaces at the start of the line and the only white space
> > allowed after Fixes: and after the SHA1 should be a space not a tab.
>
> Agree too, I'll change the regexp accordingly if you decide to go with
> my submission.
>
> >
> > I think the test better if it checks for a SHA1 after fixes.
> >
> > And IMO
> >
> > !(foo =~ /bar.../)
> >
> > is better written as
> >
> > foo !~ /bar.../
> >
> > so
> >
> > if ($in_commit_log &&
> > $line =~ /^\s*Fixes:?\s*[0-9a-f]{5,}\b/i &&
> > $line !~ /^Fixes: [0-9a-f]{12,40} \(\".*\"\)/)) {
> >
> > Though it's arguable that the SHA1 should _only_ be length 12
> > and not longer.
> >
>
--
Kind Regards,
Niklas Söderlund