linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Wang YanQing <udknight@gmail.com>
To: joe@perches.com
Cc: Andy Whitcroft <apw@canonical.com>,
	linux-kernel@vger.kernel.org,
	Alexei Starovoitov <alexei.starovoitov@gmail.com>,
	Matteo Croce <mcroce@redhat.com>,
	Markus.Elfring@web.de, kernel-janitors@vger.kernel.org
Subject: [PATCH v4] checkpatch: add support to check 'Fixes:' tag format
Date: Sun, 3 May 2020 02:54:21 +0800	[thread overview]
Message-ID: <20200502185421.GB9082@udknight> (raw)

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 and it will emit diagnostics info for them:
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')

Because after GIT_COMMIT_ID supports 'Fixes:' tag format check, it could do
the same check as the UNKNOWN_COMMIT_ID, so we don't need UNKNOWN_COMMIT_ID
anymore and I decide to delete it.

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"")

Note: this patch also adds diagnostics info support for normal git commit
      description format check.

Based on original patch by Joe Perches <joe@perches.com>

Link: https://lore.kernel.org/lkml/40bfc40958fca6e2cc9b86101153aa0715fac4f7.camel@perches.com/
Signed-off-by: Wang YanQing <udknight@gmail.com>
---
 v4:
 1: Add diagnostics info support, suggested by Joe Perches and Markus Elfring.
 2: Delete UNKNOWN_COMMIT_ID and do the check in GIT_COMMIT_ID.

 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 | 115 +++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 85 insertions(+), 30 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 143bb43..b5768e0 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2818,48 +2818,93 @@ 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 $hasprefix = 1;
 			my $id = '0123456789ab';
 			my $orig_desc = "commit description";
 			my $description = "";
+			my $acrosslines = 0;
+			my $title = "title line";
+			my $desc_mismatch = 0;
+			my $diagnostics = "Diagnostics info:\n";
 
-			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) {
+			if ($line !~ /\b$prefix\s+/i) {
+				$hasprefix = 0;
+				$diagnostics .= "Missing prefix \"$prefix_case\".\n";
+			} elsif ($line =~ /\b$prefix\s+[0-9a-f]{41,}/i) {
+				$long = 1;
+			} elsif ($line =~ /\b$prefix\s+[0-9a-f]{12,40}/i) {
+				$short = 0;
+			} else {
+				$diagnostics .= "Commit id $orig_commit is too short.\n";
+			}
+
+			if ($line =~ /\b$prefix [0-9a-f]/i) {
+				$space = 0;
+			} elsif ($hasprefix) {
+				$diagnostics .= "Extra whitespace between prefix \"${init_char}" . substr($prefix, 1) . "\" and $orig_commit.\n";
+			}
+
+			if ($line =~ /\b$prefix_case\s+[0-9a-f]{5,40}[^A-F]/) {
+				$case = 0;
+			} elsif ($hasprefix) {
+				$line =~ /(\b$prefix)\s+[0-9a-f]{5,40}[^A-F]/i;
+				$diagnostics .= "The prefix \"$1\" has case error.\n";
+			}
+
+			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;
+
+				if ($prefix eq "Fixes:") {
+					$acrosslines = 1;
+					$diagnostics .= "The title acrosses lines.\n";
+				}
+			} elsif ($hasprefix) {
+				$diagnostics .= "Missing a pair of parentheses '()' or a pair of double quotation marks (\"\").\n";
 			}
 
 			($id, $description) = git_commit_info($orig_commit,
@@ -2875,10 +2920,31 @@ sub process {
 			    }
 			}
 
+			if (defined($id) && $has_parens_and_dqm && ($orig_desc ne $description)) {
+			    # Allow short description without too short!
+			    if ($prefix eq "Fixes:") {
+				if (length($orig_desc) >= length($description)/2) {
+					my $desc = substr($description, 0, length($orig_desc));
+
+					if ($orig_desc ne $desc) {
+						$desc_mismatch = 1;
+						$diagnostics .= "The title doesn't match the original commit.\n";
+					}
+				} else {
+					$desc_mismatch = 1;
+					$diagnostics .= "The title is too abbreviated, at least half of orignial commit title is necessary.\n";
+				}
+			    } else {
+					$desc_mismatch = 1;
+					$diagnostics .= "The title doesn't match the original commit.\n";
+			    }
+			}
+
 			if (defined($id) &&
-			   ($short || $space || $case || ($orig_desc ne $description) || !$hasparens)) {
+			   ($short || $space || $case || $desc_mismatch || !$hasprefix || !$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" .
+				      $diagnostics . $herecurr);
 			}
 		}
 
@@ -2978,17 +3044,6 @@ sub process {
 			}
 		}
 
-# check for invalid commit id
-		if ($in_commit_log && $line =~ /(^fixes:)\s+([0-9a-f]{6,40})\b/i) {
-			my $id;
-			my $description;
-			($id, $description) = git_commit_info($2, undef, undef);
-			if (!defined($id)) {
-				WARN("UNKNOWN_COMMIT_ID",
-				     "Unknown commit id '$2', maybe rebased or not pulled?\n" . $herecurr);
-			}
-		}
-
 # ignore non-hunk lines and lines being removed
 		next if (!$hunk_line || $line =~ /^-/);
 
-- 
1.8.5.6.2.g3d8a54e.dirty

             reply	other threads:[~2020-05-02 18:54 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-02 18:54 Wang YanQing [this message]
2020-05-02 19:40 ` [PATCH v4] checkpatch: add support to check 'Fixes:' tag format Markus Elfring
2020-05-02 20:00   ` Joe Perches
2020-05-02 20:07     ` [v4] " Markus Elfring
2020-05-03 11:45   ` [PATCH v4] " Wang YanQing
2020-05-03 12:32     ` [v4] " Markus Elfring

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200502185421.GB9082@udknight \
    --to=udknight@gmail.com \
    --cc=Markus.Elfring@web.de \
    --cc=alexei.starovoitov@gmail.com \
    --cc=apw@canonical.com \
    --cc=joe@perches.com \
    --cc=kernel-janitors@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mcroce@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).