checkpatch: add check for fixes: tag

[PATCH] checkpatch: add check for fixes: tag

Posted by Philippe Schenker 3 years, 7 months ago

From: Philippe Schenker <philippe.schenker@toradex.com>

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/20220906073746.1f2713f7@canb.auug.org.au/
Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com>

---

 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

Re: [PATCH] checkpatch: add check for fixes: tag

Posted by Joe Perches 3 years, 7 months ago

On Wed, 2022-09-07 at 14:35 +0200, Philippe Schenker wrote:
> From: Philippe Schenker <philippe.schenker@toradex.com>
> 
> 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.

Re: [PATCH] checkpatch: add check for fixes: tag

Posted by Stephen Rothwell 3 years, 7 months ago

Hi Joe,

On Wed, 07 Sep 2022 08:18:31 -0700 Joe Perches <joe@perches.com> 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

Re: [PATCH] checkpatch: add check for fixes: tag

Posted by Philippe Schenker 3 years, 7 months ago

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 <philippe.schenker@toradex.com>
> > 
> > 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/20220905105247.920676-1-niklas.soderlund@corigine.com/

> 
> > 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.
>

Re: [PATCH] checkpatch: add check for fixes: tag

Posted by niklas.soderlund@corigine.com 3 years, 7 months ago

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/20220908114325.4153436-1-niklas.soderlund@corigine.com/

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 <philippe.schenker@toradex.com>
> > > 
> > > 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/20220905105247.920676-1-niklas.soderlund@corigine.com/
> 
> > 
> > > 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