checkpatch: add support to check 'Fixes:' tag format

PATCH Superseded Wang YanQing 3 participants 6 messages May 1, 2020

Subsystems (3)

Documentation Kernel janitors Checkpatch

lore

Share & tools

lore

Wang YanQing

author

2020-05-01 15:40 UTC #0 lore raw

PATCH v3 1/1 checkpatch: add support to check 'Fixes:' tag format

According to submitting-patches.rst, 'Fixes:' tag has a little stricter condition about the one line summary than normal git commit description:
“...
Do not split the tag across multiple lines, tags are exempt from the "wrap at 75 columns" rule in order to simplify parsing scripts ...”

And there is no sanity check for 'Fixes:' tag format in checkpatch the same as GIT_COMMIT_ID for git commit description, so let's expand the GIT_COMMIT_ID to add 'Fixes:' tag format check support.

The check supports below formats:

Fixes 54a4f0239f2e ("KVM: MMU: make kvm_mmu_zap_page() return the number of pages it actually freed")

Fixes 85f7cd3a2aad ("Revert "media: Kconfig: better support hybrid TV devices"")

Fixes 878520ac45f9 ("ext4: save the error code which triggered...")

Fixes 878520ac45f9 ("ext4: save the error code which triggered")

Fixes 277f27e2f277 ("SUNRPC/cache: Allow garbage collection ... ")

The check doesn't support below formats:

Fixes f2c2e717642c ("usb: gadget: add raw-gadget interface"

Fixes 6c73698904aa pinctrl: qcom: Introduce readl/writel accessors

Fixes 3fd6e7d9a146 (ASoC: tas571x: New driver for TI TAS571x power amplifiers)

Fixes 55697cbb44e4 ("arm64: dts: renesas: r8a779{65,80,90}: Add IPMMU devices nodes)

Fixes ba35f8588f47 (“ipvlan: Defer multicast / broadcast processing to a work-queue”)

Fixes cd758a9b57ee "KVM: PPC: Book3S HV: Use __gfn_to_pfn_memslot in HPT page fault handler"

Fixes 9b1640686470 ("scsi: lpfc: Fix use-after-free mailbox cmd completion")

Fixes 03f6fc6de919 ('ASoC: rt5682: Add the soundwire support')

Note: this patch also fixes double quotation mark issue for normal git
commit description, and now it supports double quotation mark in title line, for example:
Commit e33e2241e272 ("Revert "cfg80211: Use 5MHz bandwidth by default when checking usable channels"")

Based on original patch by Joe Perches <[email protected]>

Link https://lore.kernel.org/lkml/[email protected]/ open here

Signed-off-by Wang YanQing <[email protected]>

---
v3:
1: Fix a bug in short title line support.

v2:
1: Add support for double quotation mark in title line, suggested by Markus Elfring. 2: Add support for short title line with/without ellipsis.
3: Add supported format examples and unsupported format examples in changelog. 4: Fix a little wording issue in changelog , suggested by Markus Elfring.

 scripts/checkpatch.pl | 68 +++++++++++++++++++++++++++++++------------
 1 file changed, 49 insertions(+), 19 deletions(-)

scripts/checkpatch.pl +49 ~~-19~~

index 23a001a66006..a649b9f711b6 100755
@@ -2818,57 +2818,87 @@ sub process {
 		    $line !~ /^\s*(?:Link|Patchwork|http|https|BugLink|base-commit):/i &&
 		    $line !~ /^This reverts commit [0-9a-f]{7,40}/ &&
 		    ($line =~ /\bcommit\s+[0-9a-f]{5,}\b/i ||
+		     $line =~ /\bfixes:\s+[0-9a-f]{5,}\b/i ||
 		     ($line =~ /(?:\s|^)[0-9a-f]{12,40}(?:[\s"'\(\[]|$)/i &&
-		      $line !~ /[\<\[][0-9a-f]{12,40}[\>\]]/i &&
-		      $line !~ /\bfixes:\s*[0-9a-f]{12,40}/i))) {
+		      $line !~ /[\<\[][0-9a-f]{12,40}[\>\]]/i))) {
 			my $init_char = "c";
 			my $orig_commit = "";
+			my $prefix = "commit";
+			my $prefix_case = "[Cc]ommit";
 			my $short = 1;
 			my $long = 0;
 			my $case = 1;
 			my $space = 1;
 			my $hasdesc = 0;
-			my $hasparens = 0;
+			my $has_parens_and_dqm = 0; # Double quotation mark
 			my $id = '0123456789ab';
 			my $orig_desc = "commit description";
 			my $description = "";
+			my $acrosslines = 0;
+			my $title = "title line";
+			my $desc_mismatch = 0;
 
-			if ($line =~ /\b(c)ommit\s+([0-9a-f]{5,})\b/i) {
+			if ($line =~ /\b(f)ixes:\s+([0-9a-f]{5,})\b/i) {
+				$init_char = $1;
+				$orig_commit = lc($2);
+				$prefix = "Fixes:";
+				$prefix_case = "Fixes:";
+				$init_char = "F";
+				$title = "a single line title (without line breaks but ellipsis is fine!)";
+			} elsif ($line =~ /\b(c)ommit\s+([0-9a-f]{5,})\b/i) {
 				$init_char = $1;
 				$orig_commit = lc($2);
 			} elsif ($line =~ /\b([0-9a-f]{12,40})\b/i) {
 				$orig_commit = lc($1);
 			}
 
-			$short = 0 if ($line =~ /\bcommit\s+[0-9a-f]{12,40}/i);
-			$long = 1 if ($line =~ /\bcommit\s+[0-9a-f]{41,}/i);
-			$space = 0 if ($line =~ /\bcommit [0-9a-f]/i);
-			$case = 0 if ($line =~ /\b[Cc]ommit\s+[0-9a-f]{5,40}[^A-F]/);
-			if ($line =~ /\bcommit\s+[0-9a-f]{5,}\s+\("([^"]+)"\)/i) {
+			$short = 0 if ($line =~ /\b$prefix\s+[0-9a-f]{12,40}/i);
+			$long = 1 if ($line =~ /\b$prefix\s+[0-9a-f]{41,}/i);
+			$space = 0 if ($line =~ /\b$prefix [0-9a-f]/i);
+			$case = 0 if ($line =~ /\b$prefix_case\s+[0-9a-f]{5,40}[^A-F]/);
+			if ($line =~ /\b$prefix\s+[0-9a-f]{5,}\s+\("(.+)"\)/i) {
 				$orig_desc = $1;
-				$hasparens = 1;
+				$has_parens_and_dqm = 1;
+				# Drop the ellipsis
+				if ($prefix eq "Fixes:" && $orig_desc =~ /(\s*\.{3}\s*$)/) {
+				    $orig_desc = substr($orig_desc, 0, length($orig_desc) - length($1));
+				}
 			} elsif ($line =~ /\bcommit\s+[0-9a-f]{5,}\s*$/i &&
 				 defined $rawlines[$linenr] &&
-				 $rawlines[$linenr] =~ /^\s*\("([^"]+)"\)/) {
+				 $rawlines[$linenr] =~ /^\s*\("(.+)"\)/) {
 				$orig_desc = $1;
-				$hasparens = 1;
-			} elsif ($line =~ /\bcommit\s+[0-9a-f]{5,}\s+\("[^"]+$/i &&
+				$has_parens_and_dqm = 1;
+			} elsif ($line =~ /\b$prefix\s+[0-9a-f]{5,}\s+\(".+$/i &&
 				 defined $rawlines[$linenr] &&
-				 $rawlines[$linenr] =~ /^\s*[^"]+"\)/) {
-				$line =~ /\bcommit\s+[0-9a-f]{5,}\s+\("([^"]+)$/i;
+				 $rawlines[$linenr] =~ /^\s*.+"\)/) {
+				$line =~ /\b$prefix\s+[0-9a-f]{5,}\s+\("(.+)$/i;
 				$orig_desc = $1;
-				$rawlines[$linenr] =~ /^\s*([^"]+)"\)/;
+				$rawlines[$linenr] =~ /^\s*(.+)"\)/;
 				$orig_desc .= " " . $1;
-				$hasparens = 1;
+				$has_parens_and_dqm = 1;
+				$acrosslines = 1 if ($prefix eq "Fixes:");
 			}
 
 			($id, $description) = git_commit_info($orig_commit,
 							      $id, $orig_desc);
 
+			if (defined($id) && ($orig_desc ne $description)) {
+			    # Allow short description without too short!
+			    if ($prefix eq "Fixes:" && $has_parens_and_dqm  && length($orig_desc) >= length($description)/2) {
+				my $desc = substr($description, 0, length($orig_desc));
+
+				if ($orig_desc ne $desc) {
+				    $desc_mismatch = 1;
+				}
+			    } else {
+				$desc_mismatch = 1;
+			    }
+			}
+
 			if (defined($id) &&
-			   ($short || $long || $space || $case || ($orig_desc ne $description) || !$hasparens)) {
+			   ($short || $long || $space || $case || $desc_mismatch || !$has_parens_and_dqm || $acrosslines)) {
 				ERROR("GIT_COMMIT_ID",
-				      "Please use git commit description style 'commit <12+ chars of sha1> (\"<title line>\")' - ie: '${init_char}ommit $id (\"$description\")'\n" . $herecurr);
+				      "Please use git commit description style '$prefix <12+ chars of sha1> (\"<$title>\")' - ie: '${init_char}" . substr($prefix, 1) . " $id (\"$description\")'\n" . $herecurr);
 			}
 		}

Signature · 1 line

2.17.1

Joe Perches

maintainer

2020-05-01 15:57 UTC #1 lore raw

Quoted 12 lines · Wang YanQing

According to submitting-patches.rst, 'Fixes:' tag has a little stricter condition about the one line summary than normal git commit description:
“...
Do not split the tag across multiple lines, tags are exempt from the "wrap at 75 columns" rule in order to simplify parsing scripts ...”
And there is no sanity check for 'Fixes:' tag format in checkpatch the same as GIT_COMMIT_ID for git commit description, so let's expand the GIT_COMMIT_ID to add 'Fixes:' tag format check support.

Quoted 6 lines

The check supports below formats:
Fixes 54a4f0239f2e ("KVM: MMU: make kvm_mmu_zap_page() return the number of pages it actually freed")
Fixes 85f7cd3a2aad ("Revert "media: Kconfig: better support hybrid TV devices"")
Fixes 878520ac45f9 ("ext4: save the error code which triggered...")
Fixes 878520ac45f9 ("ext4: save the error code which triggered")
Fixes 277f27e2f277 ("SUNRPC/cache: Allow garbage collection ... ")

Hi again YanQing.

I think all the non-standard and incomplete forms
should have a warning emitted.

Quoted 11 lines

The check doesn't support below formats:
Fixes f2c2e717642c ("usb: gadget: add raw-gadget interface"
Fixes 6c73698904aa pinctrl: qcom: Introduce readl/writel accessors
Fixes 3fd6e7d9a146 (ASoC: tas571x: New driver for TI TAS571x power amplifiers)
Fixes 55697cbb44e4 ("arm64: dts: renesas: r8a779{65,80,90}: Add IPMMU devices nodes)
Fixes ba35f8588f47 (“ipvlan: Defer multicast / broadcast processing to a work-queue”)
Fixes cd758a9b57ee "KVM: PPC: Book3S HV: Use __gfn_to_pfn_memslot in HPT page fault handler"
Fixes 9b1640686470 ("scsi: lpfc: Fix use-after-free mailbox cmd completion")
Fixes 03f6fc6de919 ('ASoC: rt5682: Add the soundwire support')

Quoted 5 lines

Note: this patch also fixes double quotation mark issue for normal git
commit description, and now it supports double quotation mark in title line, for example:
Commit e33e2241e272 ("Revert "cfg80211: Use 5MHz bandwidth by default when checking usable channels"")

Nice.

Wang YanQing

author

2020-05-01 16:34 UTC #2 lore raw

Quoted 20 lines · Joe Perches

Quoted 12 lines · Wang YanQing
According to submitting-patches.rst, 'Fixes:' tag has a little stricter condition about the one line summary than normal git commit description:
“...
Do not split the tag across multiple lines, tags are exempt from the "wrap at 75 columns" rule in order to simplify parsing scripts ...”
And there is no sanity check for 'Fixes:' tag format in checkpatch the same as GIT_COMMIT_ID for git commit description, so let's expand the GIT_COMMIT_ID to add 'Fixes:' tag format check support.
Quoted 6 lines
The check supports below formats:
Fixes 54a4f0239f2e ("KVM: MMU: make kvm_mmu_zap_page() return the number of pages it actually freed")
Fixes 85f7cd3a2aad ("Revert "media: Kconfig: better support hybrid TV devices"")
Fixes 878520ac45f9 ("ext4: save the error code which triggered...")
Fixes 878520ac45f9 ("ext4: save the error code which triggered")
Fixes 277f27e2f277 ("SUNRPC/cache: Allow garbage collection ... ")

Quoted 4 lines

Hi again YanQing.
I think all the non-standard and incomplete forms
should have a warning emitted.

Hi Joe Perches
I am not sure whether I get your words, you mean we need to emit warning for incomplete title line format? For example:

Fixes 277f27e2f277 ("SUNRPC/cache: Allow garbage collection ... ")

Thanks.

Quoted 20 lines

Quoted 12 lines
The check doesn't support below formats:
Fixes f2c2e717642c ("usb: gadget: add raw-gadget interface"
Fixes 6c73698904aa pinctrl: qcom: Introduce readl/writel accessors
Fixes 3fd6e7d9a146 (ASoC: tas571x: New driver for TI TAS571x power amplifiers)
Fixes 55697cbb44e4 ("arm64: dts: renesas: r8a779{65,80,90}: Add IPMMU devices nodes)
Fixes ba35f8588f47 (“ipvlan: Defer multicast / broadcast processing to a work-queue”)
Fixes cd758a9b57ee "KVM: PPC: Book3S HV: Use __gfn_to_pfn_memslot in HPT page fault handler"
Fixes 9b1640686470 ("scsi: lpfc: Fix use-after-free mailbox cmd completion")
Fixes 03f6fc6de919 ('ASoC: rt5682: Add the soundwire support')
Quoted 5 lines
Note: this patch also fixes double quotation mark issue for normal git
commit description, and now it supports double quotation mark in title line, for example:
Commit e33e2241e272 ("Revert "cfg80211: Use 5MHz bandwidth by default when checking usable channels"")
Nice.

Joe Perches

maintainer

2020-05-01 16:44 UTC #3 lore raw

Quoted 21 lines · Wang YanQing

Quoted 20 lines · Joe Perches
Quoted 18 lines · Wang YanQing
According to submitting-patches.rst, 'Fixes:' tag has a little stricter condition about the one line summary than normal git commit description:
“...
Do not split the tag across multiple lines, tags are exempt from the "wrap at 75 columns" rule in order to simplify parsing scripts ...”
And there is no sanity check for 'Fixes:' tag format in checkpatch the same as GIT_COMMIT_ID for git commit description, so let's expand the GIT_COMMIT_ID to add 'Fixes:' tag format check support.
The check supports below formats:
Fixes 54a4f0239f2e ("KVM: MMU: make kvm_mmu_zap_page() return the number of pages it actually freed")
Fixes 85f7cd3a2aad ("Revert "media: Kconfig: better support hybrid TV devices"")
Fixes 878520ac45f9 ("ext4: save the error code which triggered...")
Fixes 878520ac45f9 ("ext4: save the error code which triggered")
Fixes 277f27e2f277 ("SUNRPC/cache: Allow garbage collection ... ")

Quoted 6 lines

Quoted 4 lines
Hi again YanQing.
I think all the non-standard and incomplete forms
should have a warning emitted.
Hi Joe Perches

Hi, again. It's just Joe to my friends...

Quoted 3 lines

I am not sure whether I get your words, you mean we need to emit warning for incomplete title line format? For example:
Fixes 277f27e2f277 ("SUNRPC/cache: Allow garbage collection ... ")

I think so yes.

It _might_ be useful to show "why" the message is being emitted. (sha1 too short, no quotes around description, etc...)

Markus Elfring

2020-05-01 16:52 UTC #4 lore raw

In reply to Wang YanQing

Quoted 3 lines

I am not sure whether I get your words, you mean we need to emit warning for incomplete title line format? For example:
Fixes 277f27e2f277 ("SUNRPC/cache: Allow garbage collection ... ")

I suggest to increase the precision for the usage the ellipsis at the end.

* Triple ASCII dots

* U+2026

* The commit title should probably not be abbreviated too much.

Regards,
Markus

Markus Elfring

2020-05-01 16:08 UTC #5 lore raw

In reply to Wang YanQing

Quoted 1 line

The check supports below formats:

I suggest to omit the concrete examples.
I would prefer the explicit wording for the support of (Unicode) ellipses also in the shown commit titles.

Will the document “submitting-patches.rst” need a corresponding adjustment?

Quoted 1 line

The check doesn't support below formats:

How do you think about to extend error diagnostics for patch checking?

Quoted 1 line

Fixes f2c2e717642c ("usb: gadget: add raw-gadget interface"

Missing closing parenthesis

Quoted 1 line

Fixes 6c73698904aa pinctrl: qcom: Introduce readl/writel accessors

Missing enclosing delimiters

Quoted 2 lines

Fixes 3fd6e7d9a146 (ASoC: tas571x: New driver for TI TAS571x power amplifiers)
Fixes 55697cbb44e4 ("arm64: dts: renesas: r8a779{65,80,90}: Add IPMMU devices nodes)

Missing quotation characters

Quoted 1 line

Fixes ba35f8588f47 (“ipvlan: Defer multicast / broadcast processing to a work-queue”)

…

Quoted 1 line

Fixes 03f6fc6de919 ('ASoC: rt5682: Add the soundwire support')

Would we like to tolerate such quotation character alternatives?

Quoted 1 line

Fixes 9b1640686470 ("scsi: lpfc: Fix use-after-free mailbox cmd completion")

How do you think about to tolerate extra white-space characters around the commit identifier?

Quoted 5 lines

Note: this patch also fixes double quotation mark issue for normal git
commit description, and now it supports double quotation mark in title line, for example:
Commit e33e2241e272 ("Revert "cfg80211: Use 5MHz bandwidth by default when checking usable channels"")

I find that such an example should trigger further software development considerations for a safe data format description.

Quoted 2 lines

v2:
1: Add support for double quotation mark in title line, suggested by Markus Elfring.

I guess that the clarification is still evolving also around this aspect.

Regards,
Markus