linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] checkpatch: warn for non-standard fixes tag style
@ 2022-08-29 15:53 Niklas Söderlund
  2022-08-30  3:06 ` Joe Perches
  2022-08-30  3:37 ` Bagas Sanjaya
  0 siblings, 2 replies; 6+ messages in thread
From: Niklas Söderlund @ 2022-08-29 15:53 UTC (permalink / raw)
  To: Dwaipayan Ray, Lukas Bulwahn, Joe Perches, Jonathan Corbet,
	Andy Whitcroft, linux-doc, linux-kernel
  Cc: oss-drivers, Niklas Söderlund, Simon Horman, Louis Peens

Add a warning for fixes tags that does not fall in line with the
standards specified by the community.

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>
---
 Documentation/dev-tools/checkpatch.rst |  6 ++++
 scripts/checkpatch.pl                  | 41 ++++++++++++++++++++++++++
 2 files changed, 47 insertions(+)

diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
index b52452bc2963..8164f362a2fc 100644
--- a/Documentation/dev-tools/checkpatch.rst
+++ b/Documentation/dev-tools/checkpatch.rst
@@ -612,6 +612,12 @@ Commit message
 
     See: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes
 
+  **BAD_FIXES_TAG**
+    The fixes line does not fall in line with the standards specified by the
+    community.
+
+    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..9b8cdc582fb5 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3140,6 +3140,47 @@ sub process {
 			}
 		}
 
+# Check Fixes: styles is correct
+		if (!$in_header_lines && $line =~ /^fixes:/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 =~ /(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;
+				}
+			}
+
+			($id, $title) = git_commit_info($orig_commit, $id,
+							$title);
+
+			if ($tag_case || $tag_space || $id_length || $id_case ||
+			    !$title_has_quotes) {
+				WARN("BAD_FIXES_TAG",
+				     "Please use correct Fixes: style 'Fixes: <12 chars of sha1> (\"<title line>\")' - ie: 'Fixes: $id (\"$title\")'\n" . $herecurr);
+
+			}
+		}
+
 # 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.2


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] checkpatch: warn for non-standard fixes tag style
  2022-08-29 15:53 [PATCH] checkpatch: warn for non-standard fixes tag style Niklas Söderlund
@ 2022-08-30  3:06 ` Joe Perches
  2022-09-05 10:49   ` Niklas Söderlund
  2022-08-30  3:37 ` Bagas Sanjaya
  1 sibling, 1 reply; 6+ messages in thread
From: Joe Perches @ 2022-08-30  3:06 UTC (permalink / raw)
  To: Niklas Söderlund, Dwaipayan Ray, Lukas Bulwahn,
	Jonathan Corbet, Andy Whitcroft, linux-doc, linux-kernel
  Cc: oss-drivers, Simon Horman, Louis Peens

On Mon, 2022-08-29 at 17:53 +0200, Niklas Söderlund wrote:
> Add a warning for fixes tags that does not fall in line with the
> standards specified by the community.
> 
> 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>
> ---
>  Documentation/dev-tools/checkpatch.rst |  6 ++++
>  scripts/checkpatch.pl                  | 41 ++++++++++++++++++++++++++
>  2 files changed, 47 insertions(+)
> 
> diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
> index b52452bc2963..8164f362a2fc 100644
> --- a/Documentation/dev-tools/checkpatch.rst
> +++ b/Documentation/dev-tools/checkpatch.rst
> @@ -612,6 +612,12 @@ Commit message
>  
>      See: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes
>  
> +  **BAD_FIXES_TAG**
> +    The fixes line does not fall in line with the standards specified by the
> +    community.
> +
> +    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..9b8cdc582fb5 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -3140,6 +3140,47 @@ sub process {
>  			}
>  		}
>  
> +# Check Fixes: styles is correct
> +		if (!$in_header_lines && $line =~ /^fixes:/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 =~ /(fixes:)\s+([0-9a-f]{5,})\s+($balanced_parens)/i) {

Maybe use fixes:? so the colon is not required in poorly formed uses

> +				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);

fixes:? here too

Pity there's no simple way to consolidate this git commit test block
with the existing git id commit block.

> +
> +				$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;
> +				}
> +			}
> +
> +			($id, $title) = git_commit_info($orig_commit, $id,
> +							$title);
> +
> +			if ($tag_case || $tag_space || $id_length || $id_case ||
> +			    !$title_has_quotes) {
> +				WARN("BAD_FIXES_TAG",
> +				     "Please use correct Fixes: style 'Fixes: <12 chars of sha1> (\"<title line>\")' - ie: 'Fixes: $id (\"$title\")'\n" . $herecurr);
> +
> +			}
> +		}
> +
>  # 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) {


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] checkpatch: warn for non-standard fixes tag style
  2022-08-29 15:53 [PATCH] checkpatch: warn for non-standard fixes tag style Niklas Söderlund
  2022-08-30  3:06 ` Joe Perches
@ 2022-08-30  3:37 ` Bagas Sanjaya
  1 sibling, 0 replies; 6+ messages in thread
From: Bagas Sanjaya @ 2022-08-30  3:37 UTC (permalink / raw)
  To: Niklas Söderlund, Dwaipayan Ray, Lukas Bulwahn, Joe Perches,
	Jonathan Corbet, Andy Whitcroft, linux-doc, linux-kernel
  Cc: oss-drivers, Simon Horman, Louis Peens

On 8/29/22 22:53, Niklas Söderlund wrote:
> +  **BAD_FIXES_TAG**
> +    The fixes line does not fall in line with the standards specified by the
> +    community.
> +
> +    See: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes
> +
>  

Did you mean "The Fixes: tag is malformed"?

Also, mention that this message can occur when the tag is split
into multiple lines (e.g., when pasted in email program with word
wrapping enabled).

Thanks.

-- 
An old man doll... just what I always wanted! - Clara

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] checkpatch: warn for non-standard fixes tag style
  2022-08-30  3:06 ` Joe Perches
@ 2022-09-05 10:49   ` Niklas Söderlund
  2022-09-05 18:00     ` Joe Perches
  0 siblings, 1 reply; 6+ messages in thread
From: Niklas Söderlund @ 2022-09-05 10:49 UTC (permalink / raw)
  To: Joe Perches
  Cc: Dwaipayan Ray, Lukas Bulwahn, Jonathan Corbet, Andy Whitcroft,
	linux-doc, linux-kernel, oss-drivers, Simon Horman, Louis Peens

Hi Joe,

Thanks for your feedback.

On 2022-08-29 23:06:43 -0400, Joe Perches wrote:
> > +			if ($line =~ 
> > /(fixes:)\s+([0-9a-f]{5,})\s+($balanced_parens)/i) {
> 
> Maybe use fixes:? so the colon is not required in poorly formed uses

I tried that but I think it brings more problems then it is worth. With 
that change the check would run for each line of the commit message that 
begins with the string 'fixes', not just in the tags section of the 
message. So it would warn for the commit message,

    The work on foo and bar introduced a bug that can be
    fixed by doing baz.

I think it's for this reason other checks for tags include the ':'.

> 
> > +				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);
> 
> fixes:? here too
> 
> Pity there's no simple way to consolidate this git commit test block
> with the existing git id commit block.

I agree.

-- 
Kind Regards,
Niklas Söderlund

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] checkpatch: warn for non-standard fixes tag style
  2022-09-05 10:49   ` Niklas Söderlund
@ 2022-09-05 18:00     ` Joe Perches
  2022-09-06  9:19       ` Niklas Söderlund
  0 siblings, 1 reply; 6+ messages in thread
From: Joe Perches @ 2022-09-05 18:00 UTC (permalink / raw)
  To: Niklas Söderlund
  Cc: Dwaipayan Ray, Lukas Bulwahn, Jonathan Corbet, Andy Whitcroft,
	linux-doc, linux-kernel, oss-drivers, Simon Horman, Louis Peens

On Mon, 2022-09-05 at 12:49 +0200, Niklas Söderlund wrote:
> Hi Joe,
> 
> Thanks for your feedback.
> 
> On 2022-08-29 23:06:43 -0400, Joe Perches wrote:
> > > +			if ($line =~ 
> > > /(fixes:)\s+([0-9a-f]{5,})\s+($balanced_parens)/i) {
> > 
> > Maybe use fixes:? so the colon is not required in poorly formed uses
> 
> I tried that but I think it brings more problems then it is worth. With 
> that change the check would run for each line of the commit message that 
> begins with the string 'fixes', not just in the tags section of the 
> message. So it would warn for the commit message,

I think it's not a problem.
Look at the results of:

$ git log -100000 --no-merges --format=email --grep="^fixes" -i | \
  grep -i -P -oh '^fixes:?\s*[0-9a-f]{5,}\s*..' | \
  sed -r -e 's/^(fixes:?\s*)[0-9a-f]+/\1/i' | \
  sort | uniq -c | sort -rn | head -20
  73974 Fixes:  ("
   1345 Fixes:  ('
    399 Fixes: 
    246 Fixes:  (A
    215 Fixes: ("
    172 Fixes:  (c
    121 Fixes:  (s
    114 Fixes:  (d
    110 Fixes:  (P
     98 Fixes:  (i
     90 Fixes:  (m
     86 Fixes:  (n
     78 Fixes: : 
     57 Fixes  ("
     51 Fixes:  "n
     47 Fixes:  (a
     46 Fixes:  (t
     43 fixes:  ("
     42 Fixes:  (p
     41 Fixes: ('



^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] checkpatch: warn for non-standard fixes tag style
  2022-09-05 18:00     ` Joe Perches
@ 2022-09-06  9:19       ` Niklas Söderlund
  0 siblings, 0 replies; 6+ messages in thread
From: Niklas Söderlund @ 2022-09-06  9:19 UTC (permalink / raw)
  To: Joe Perches
  Cc: Dwaipayan Ray, Lukas Bulwahn, Jonathan Corbet, Andy Whitcroft,
	linux-doc, linux-kernel, oss-drivers, Simon Horman, Louis Peens

Hello Joe,

On 2022-09-05 11:00:35 -0700, Joe Perches wrote:
> On Mon, 2022-09-05 at 12:49 +0200, Niklas Söderlund wrote:
> > Hi Joe,
> > 
> > Thanks for your feedback.
> > 
> > On 2022-08-29 23:06:43 -0400, Joe Perches wrote:
> > > > +			if ($line =~ 
> > > > /(fixes:)\s+([0-9a-f]{5,})\s+($balanced_parens)/i) {
> > > 
> > > Maybe use fixes:? so the colon is not required in poorly formed uses
> > 
> > I tried that but I think it brings more problems then it is worth. With 
> > that change the check would run for each line of the commit message that 
> > begins with the string 'fixes', not just in the tags section of the 
> > message. So it would warn for the commit message,
> 
> I think it's not a problem.
> Look at the results of:
> 
> $ git log -100000 --no-merges --format=email --grep="^fixes" -i | \
>   grep -i -P -oh '^fixes:?\s*[0-9a-f]{5,}\s*..' | \
>   sed -r -e 's/^(fixes:?\s*)[0-9a-f]+/\1/i' | \
>   sort | uniq -c | sort -rn | head -20
>   73974 Fixes:  ("
>    1345 Fixes:  ('
>     399 Fixes: 
>     246 Fixes:  (A
>     215 Fixes: ("
>     172 Fixes:  (c
>     121 Fixes:  (s
>     114 Fixes:  (d
>     110 Fixes:  (P
>      98 Fixes:  (i
>      90 Fixes:  (m
>      86 Fixes:  (n
>      78 Fixes: : 
>      57 Fixes  ("
>      51 Fixes:  "n
>      47 Fixes:  (a
>      46 Fixes:  (t
>      43 fixes:  ("
>      42 Fixes:  (p
>      41 Fixes: ('

Thanks for checking this. With this background I agree with you, there 
is no problem here. I will spin a v3 with your suggestion.
> 

-- 
Kind Regards,
Niklas Söderlund

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-09-06  9:19 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-29 15:53 [PATCH] checkpatch: warn for non-standard fixes tag style Niklas Söderlund
2022-08-30  3:06 ` Joe Perches
2022-09-05 10:49   ` Niklas Söderlund
2022-09-05 18:00     ` Joe Perches
2022-09-06  9:19       ` Niklas Söderlund
2022-08-30  3:37 ` Bagas Sanjaya

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).