[PATCH v7] checkpatch: warn for non-standard fixes tag style

[PATCH v7] checkpatch: warn for non-standard fixes tag style

[Niklas Söderlund]

Add a warning for fixes tags that does not follow community conventions.

Signed-off-by: Niklas Söderlund <niklas.soderlund@corigine.com>
Reviewed-by: Simon Horman <simon.horman@corigine.com>
Reviewed-by: Louis Peens <louis.peens@corigine.com>
Reviewed-by: Philippe Schenker <philippe.schenker@toradex.com>
Acked-by: Dwaipayan Ray <dwaipayanray1@gmail.com>
Reviewed-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
Acked-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
---
* Changes since v6
- Update first check to make sure that there is a likely SHA1 of some
  minimum length after the fixes line.
- s/fall in line with community standard/follow community conventions/.
- Improve grammar, thanks Lukas.

* Changes since v5
- Add support for --fix option for checkpatch.pl.

* Changes since v4
- Extend test to cover lines with whitespace before the fixes: tag, e.g.
  match check on /^\s*fixes:?/i.

* Changes since v3
- Add test that title in tag match title of commit referenced by sha1.

* Changes since v2
- Change the pattern to match on 'fixes:?' to catch more malformed
  tags.

* Changes since v1
- Update the documentation wording and add mention one cause of the
  message can be that email program splits the tag over multiple lines.
---
 Documentation/dev-tools/checkpatch.rst |  7 ++++
 scripts/checkpatch.pl                  | 44 ++++++++++++++++++++++++++
 2 files changed, 51 insertions(+)

diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
index b52452bc2963..c3389c6f3838 100644
--- a/Documentation/dev-tools/checkpatch.rst
+++ b/Documentation/dev-tools/checkpatch.rst
@@ -612,6 +612,13 @@ Commit message
 
     See: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes
 
+  **BAD_FIXES_TAG**
+    The Fixes: tag is malformed or does not follow the community conventions.
+    This can occur if the tag have been split into multiple lines (e.g., when
+    pasted in an email program with word wrapping enabled).
+
+    See: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes
+
 
 Comparison style
 ----------------
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 79e759aac543..ddc5c9d730c3 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3140,6 +3140,50 @@ sub process {
 			}
 		}
 
+# Check Fixes: styles is correct
+		if (!$in_header_lines &&
+		    $line =~ /^\s*fixes:?\s*(?:commit\s*)?[0-9a-f]{5,}\b/i) {
+			my $orig_commit = "";
+			my $id = "0123456789ab";
+			my $title = "commit title";
+			my $tag_case = 1;
+			my $tag_space = 1;
+			my $id_length = 1;
+			my $id_case = 1;
+			my $title_has_quotes = 0;
+
+			if ($line =~ /(\s*fixes:?)\s+([0-9a-f]{5,})\s+($balanced_parens)/i) {
+				my $tag = $1;
+				$orig_commit = $2;
+				$title = $3;
+
+				$tag_case = 0 if $tag eq "Fixes:";
+				$tag_space = 0 if ($line =~ /^fixes:? [0-9a-f]{5,} ($balanced_parens)/i);
+
+				$id_length = 0 if ($orig_commit =~ /^[0-9a-f]{12}$/i);
+				$id_case = 0 if ($orig_commit !~ /[A-F]/);
+
+				# Always strip leading/trailing parens then double quotes if existing
+				$title = substr($title, 1, -1);
+				if ($title =~ /^".*"$/) {
+					$title = substr($title, 1, -1);
+					$title_has_quotes = 1;
+				}
+			}
+
+			my ($cid, $ctitle) = git_commit_info($orig_commit, $id,
+							     $title);
+
+			if ($ctitle ne $title || $tag_case || $tag_space ||
+			    $id_length || $id_case || !$title_has_quotes) {
+				if (WARN("BAD_FIXES_TAG",
+				     "Please use correct Fixes: style 'Fixes: <12 chars of sha1> (\"<title line>\")' - ie: 'Fixes: $cid (\"$ctitle\")'\n" . $herecurr) &&
+				    $fix) {
+					$fixed[$fixlinenr] = "Fixes: $cid (\"$ctitle\")";
+				}
+			}
+		}
+
 # 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.37.3

Re: [PATCH v7] checkpatch: warn for non-standard fixes tag style

[Joe Perches]

On Wed, 2022-09-14 at 12:02 +0200, Niklas Söderlund wrote:
> Add a warning for fixes tags that does not follow community conventions.
[]
> * Changes since v6
> - Update first check to make sure that there is a likely SHA1 of some
>   minimum length after the fixes line.

https://lore.kernel.org/lkml/2febb7893346b6234983453de7c037536e479bfc.camel@perches.com/

The goal here should be to identify a line that looks like a commit
reference.

So find lines that starts with 'fixes' and have a SHA1 commit id as
broadly as reasonable.

Did you run the grep pattern and look at the results?

One grep pattern to verify the non canonical fixes format that
are mistakenly used is:

$ git log --since=5-years-ago --no-merges --grep='^\s*fixes' -i --format=email -P | \
  grep -P -i '^\s*fixes' | \
  grep -P -v '^Fixes: [0-9a-f]{12,12}\s*\(".*")'

[]

There are many different styles.
Parenthesea are sometimes not used.

> +			if ($line =~ /(\s*fixes:?)\s+([0-9a-f]{5,})\s+($balanced_parens)/i) {

How about some pattern like

	/fixes\s*:?\s*(?:commit:?\s*)?[0-9a-f]{5,}/i

or maybe even more broadly:

	/fixes\b.*\b[0-9a-f]{5,}\b/i

Re: [PATCH v7] checkpatch: warn for non-standard fixes tag style

[Niklas Söderlund]

Hi Joe,

On 2022-09-14 09:09:25 -0700, Joe Perches wrote:
> On Wed, 2022-09-14 at 12:02 +0200, Niklas Söderlund wrote:
> > Add a warning for fixes tags that does not follow community conventions.
> []
> > * Changes since v6
> > - Update first check to make sure that there is a likely SHA1 of some
> >   minimum length after the fixes line.
> 
> https://lore.kernel.org/lkml/2febb7893346b6234983453de7c037536e479bfc.camel@perches.com/
> 
> The goal here should be to identify a line that looks like a commit
> reference.
> 
> So find lines that starts with 'fixes' and have a SHA1 commit id as
> broadly as reasonable.
> 
> Did you run the grep pattern and look at the results?
> 
> One grep pattern to verify the non canonical fixes format that
> are mistakenly used is:
> 
> $ git log --since=5-years-ago --no-merges --grep='^\s*fixes' -i --format=email -P | \
>   grep -P -i '^\s*fixes' | \
>   grep -P -v '^Fixes: [0-9a-f]{12,12}\s*\(".*")'
> 
> []
> 
> There are many different styles.
> Parenthesea are sometimes not used.

I understand this, and I did have a look at it.

> 
> > +			if ($line =~ /(\s*fixes:?)\s+([0-9a-f]{5,})\s+($balanced_parens)/i) {
> 
> How about some pattern like
> 
> 	/fixes\s*:?\s*(?:commit:?\s*)?[0-9a-f]{5,}/i
> 
> or maybe even more broadly:
> 
> 	/fixes\b.*\b[0-9a-f]{5,}\b/i

Maybe I misunderstand your comment, but this is what I do in this patch?

    if (!$in_header_lines &&
        $line =~ /^\s*fixes:?\s*(?:commit\s*)?[0-9a-f]{5,}\b/i) {

        ...

        if ($line =~ /(\s*fixes:?)\s+([0-9a-f]{5,})\s+($balanced_parens)/i) {
            ...
        }
    }

This will catch and warn about such tags but not attempt to break out 
it's components in order to suggest a potentially more correct fix. Is 
it this second filter you would like me to change?

-- 
Kind Regards,
Niklas Söderlund

Re: [PATCH v7] checkpatch: warn for non-standard fixes tag style

[Niklas Söderlund]

Hello Joe,

Just wanted to check on my misunderstanding below before posting a v8.

On 2022-09-15 11:15:06 +0200, Niklas Söderlund wrote:
> Hi Joe,
> 
> On 2022-09-14 09:09:25 -0700, Joe Perches wrote:
> > On Wed, 2022-09-14 at 12:02 +0200, Niklas Söderlund wrote:
> > > Add a warning for fixes tags that does not follow community conventions.
> > []
> > > * Changes since v6
> > > - Update first check to make sure that there is a likely SHA1 of some
> > >   minimum length after the fixes line.
> > 
> > https://lore.kernel.org/lkml/2febb7893346b6234983453de7c037536e479bfc.camel@perches.com/
> > 
> > The goal here should be to identify a line that looks like a commit
> > reference.
> > 
> > So find lines that starts with 'fixes' and have a SHA1 commit id as
> > broadly as reasonable.
> > 
> > Did you run the grep pattern and look at the results?
> > 
> > One grep pattern to verify the non canonical fixes format that
> > are mistakenly used is:
> > 
> > $ git log --since=5-years-ago --no-merges --grep='^\s*fixes' -i --format=email -P | \
> >   grep -P -i '^\s*fixes' | \
> >   grep -P -v '^Fixes: [0-9a-f]{12,12}\s*\(".*")'
> > 
> > []
> > 
> > There are many different styles.
> > Parenthesea are sometimes not used.
> 
> I understand this, and I did have a look at it.
> 
> > 
> > > +			if ($line =~ /(\s*fixes:?)\s+([0-9a-f]{5,})\s+($balanced_parens)/i) {
> > 
> > How about some pattern like
> > 
> > 	/fixes\s*:?\s*(?:commit:?\s*)?[0-9a-f]{5,}/i
> > 
> > or maybe even more broadly:
> > 
> > 	/fixes\b.*\b[0-9a-f]{5,}\b/i
> 
> Maybe I misunderstand your comment, but this is what I do in this patch?
> 
>     if (!$in_header_lines &&
>         $line =~ /^\s*fixes:?\s*(?:commit\s*)?[0-9a-f]{5,}\b/i) {
> 
>         ...
> 
>         if ($line =~ /(\s*fixes:?)\s+([0-9a-f]{5,})\s+($balanced_parens)/i) {
>             ...
>         }
>     }
> 
> This will catch and warn about such tags but not attempt to break out 
> it's components in order to suggest a potentially more correct fix. Is 
> it this second filter you would like me to change?
> 
> -- 
> Kind Regards,
> Niklas Söderlund

-- 
Kind Regards,
Niklas Söderlund

Re: [PATCH v7] checkpatch: warn for non-standard fixes tag style

[Joe Perches]

On Wed, 2022-09-21 at 16:25 +0200, Niklas Söderlund wrote:
> Hello Joe,

Hello Niklas

> Just wanted to check on my misunderstanding below before posting a v8.

I think v7 is ok, unless you found something else better.

> > > One grep pattern to verify the non canonical fixes format that
> > > are mistakenly used is:
> > > 
> > > $ git log --since=5-years-ago --no-merges --grep='^\s*fixes' -i --format=email -P | \
> > >   grep -P -i '^\s*fixes' | \
> > >   grep -P -v '^Fixes: [0-9a-f]{12,12}\s*\(".*")'
> > > There are many different styles.
> > > Parentheses are sometimes not used.
> > 
> > I understand this, and I did have a look at it.

Good enough for me.  Thanks.

Re: [PATCH v7] checkpatch: warn for non-standard fixes tag style

[Joe Perches]

On Wed, 2022-09-14 at 12:02 +0200, Niklas Söderlund wrote:
> Add a warning for fixes tags that does not follow community conventions.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund@corigine.com>
> Reviewed-by: Simon Horman <simon.horman@corigine.com>
> Reviewed-by: Louis Peens <louis.peens@corigine.com>
> Reviewed-by: Philippe Schenker <philippe.schenker@toradex.com>
> Acked-by: Dwaipayan Ray <dwaipayanray1@gmail.com>
> Reviewed-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> Acked-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>

Acked-by: Joe Perches <joe@perches.com>

> ---
> * Changes since v6
> - Update first check to make sure that there is a likely SHA1 of some
>   minimum length after the fixes line.
> - s/fall in line with community standard/follow community conventions/.
> - Improve grammar, thanks Lukas.
> 
> * Changes since v5
> - Add support for --fix option for checkpatch.pl.
> 
> * Changes since v4
> - Extend test to cover lines with whitespace before the fixes: tag, e.g.
>   match check on /^\s*fixes:?/i.
> 
> * Changes since v3
> - Add test that title in tag match title of commit referenced by sha1.
> 
> * Changes since v2
> - Change the pattern to match on 'fixes:?' to catch more malformed
>   tags.
> 
> * Changes since v1
> - Update the documentation wording and add mention one cause of the
>   message can be that email program splits the tag over multiple lines.
> ---
>  Documentation/dev-tools/checkpatch.rst |  7 ++++
>  scripts/checkpatch.pl                  | 44 ++++++++++++++++++++++++++
>  2 files changed, 51 insertions(+)
> 
> diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
> index b52452bc2963..c3389c6f3838 100644
> --- a/Documentation/dev-tools/checkpatch.rst
> +++ b/Documentation/dev-tools/checkpatch.rst
> @@ -612,6 +612,13 @@ Commit message
>  
>      See: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes
>  
> +  **BAD_FIXES_TAG**
> +    The Fixes: tag is malformed or does not follow the community conventions.
> +    This can occur if the tag have been split into multiple lines (e.g., when
> +    pasted in an email program with word wrapping enabled).
> +
> +    See: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes
> +
>  
>  Comparison style
>  ----------------
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 79e759aac543..ddc5c9d730c3 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -3140,6 +3140,50 @@ sub process {
>  			}
>  		}
>  
> +# Check Fixes: styles is correct
> +		if (!$in_header_lines &&
> +		    $line =~ /^\s*fixes:?\s*(?:commit\s*)?[0-9a-f]{5,}\b/i) {
> +			my $orig_commit = "";
> +			my $id = "0123456789ab";
> +			my $title = "commit title";
> +			my $tag_case = 1;
> +			my $tag_space = 1;
> +			my $id_length = 1;
> +			my $id_case = 1;
> +			my $title_has_quotes = 0;
> +
> +			if ($line =~ /(\s*fixes:?)\s+([0-9a-f]{5,})\s+($balanced_parens)/i) {
> +				my $tag = $1;
> +				$orig_commit = $2;
> +				$title = $3;
> +
> +				$tag_case = 0 if $tag eq "Fixes:";
> +				$tag_space = 0 if ($line =~ /^fixes:? [0-9a-f]{5,} ($balanced_parens)/i);
> +
> +				$id_length = 0 if ($orig_commit =~ /^[0-9a-f]{12}$/i);
> +				$id_case = 0 if ($orig_commit !~ /[A-F]/);
> +
> +				# Always strip leading/trailing parens then double quotes if existing
> +				$title = substr($title, 1, -1);
> +				if ($title =~ /^".*"$/) {
> +					$title = substr($title, 1, -1);
> +					$title_has_quotes = 1;
> +				}
> +			}
> +
> +			my ($cid, $ctitle) = git_commit_info($orig_commit, $id,
> +							     $title);
> +
> +			if ($ctitle ne $title || $tag_case || $tag_space ||
> +			    $id_length || $id_case || !$title_has_quotes) {
> +				if (WARN("BAD_FIXES_TAG",
> +				     "Please use correct Fixes: style 'Fixes: <12 chars of sha1> (\"<title line>\")' - ie: 'Fixes: $cid (\"$ctitle\")'\n" . $herecurr) &&
> +				    $fix) {
> +					$fixed[$fixlinenr] = "Fixes: $cid (\"$ctitle\")";
> +				}
> +			}
> +		}
> +
>  # 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) {