[PATCH] checkpatch: Validate Fixes: tag using 'commit' checks

[PATCH] checkpatch: Validate Fixes: tag using 'commit' checks

[Sean Christopherson]

Rework checkpatch's commit checking to explicitly include the Fixes:
tag so that it catches errors like too short[1] or fat fingered[2] SHA1
references.

Add a new Fixes-only check to verify the fixed commit is a valid object
in the repository.

[1] https://lkml.kernel.org/r/20190830010615.GC27970@linux.intel.com
[2] https://lkml.kernel.org/r/20190825233120.18ac25e4@canb.auug.org.au

Cc: Stephen Rothwell <sfr@canb.auug.org.au>
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 scripts/checkpatch.pl | 39 +++++++++++++++++++++++----------------
 1 file changed, 23 insertions(+), 16 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 93a7edfe0f05..dead0eb41007 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2756,10 +2756,10 @@ sub process {
 		    $line !~ /^\s*(?:Link|Patchwork|http|https|BugLink):/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))) {
-			my $init_char = "c";
+		      $line !~ /[\<\[][0-9a-f]{12,40}[\>\]]/i))) {
+			my $init_tag = "";
 			my $orig_commit = "";
 			my $short = 1;
 			my $long = 0;
@@ -2771,29 +2771,31 @@ sub process {
 			my $orig_desc = "commit description";
 			my $description = "";
 
-			if ($line =~ /\b(c)ommit\s+([0-9a-f]{5,})\b/i) {
-				$init_char = $1;
+			if ($line =~ /\b(commit)\s+([0-9a-f]{5,})\b/i ||
+			    $line =~ /\b(fixes:)\s*([0-9a-f]{5,})\b/i) {
+				$init_tag = $1;
 				$orig_commit = lc($2);
 			} elsif ($line =~ /\b([0-9a-f]{12,40})\b/i) {
+				$init_tag = "commit";
 				$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$init_tag\s+[0-9a-f]{12,40}/i);
+			$long = 1 if ($line =~ /\b$init_tag\s+[0-9a-f]{41,}/i);
+			$space = 0 if ($line =~ /\b$init_tag [0-9a-f]/i);
+			$case = 0 if ($line =~ /\b$init_tag\s+[0-9a-f]{5,40}[^A-F]/);
+			if ($line =~ /\b$init_tag\s+[0-9a-f]{5,}\s+\("([^"]+)"\)/i) {
 				$orig_desc = $1;
 				$hasparens = 1;
-			} elsif ($line =~ /\bcommit\s+[0-9a-f]{5,}\s*$/i &&
+			} elsif ($line =~ /\b$init_tag\s+[0-9a-f]{5,}\s*$/i &&
 				 defined $rawlines[$linenr] &&
 				 $rawlines[$linenr] =~ /^\s*\("([^"]+)"\)/) {
 				$orig_desc = $1;
 				$hasparens = 1;
-			} elsif ($line =~ /\bcommit\s+[0-9a-f]{5,}\s+\("[^"]+$/i &&
+			} elsif ($line =~ /\b$init_tag\s+[0-9a-f]{5,}\s+\("[^"]+$/i &&
 				 defined $rawlines[$linenr] &&
 				 $rawlines[$linenr] =~ /^\s*[^"]+"\)/) {
-				$line =~ /\bcommit\s+[0-9a-f]{5,}\s+\("([^"]+)$/i;
+				$line =~ /\b$init_tag\s+[0-9a-f]{5,}\s+\("([^"]+)$/i;
 				$orig_desc = $1;
 				$rawlines[$linenr] =~ /^\s*([^"]+)"\)/;
 				$orig_desc .= " " . $1;
@@ -2803,10 +2805,15 @@ sub process {
 			($id, $description) = git_commit_info($orig_commit,
 							      $id, $orig_desc);
 
-			if (defined($id) &&
-			   ($short || $long || $space || $case || ($orig_desc ne $description) || !$hasparens)) {
+
+			if (!defined($id)) {
+				if ($init_tag =~ /fixes:/i) {
+					ERROR("GIT_COMMIT_ID",
+					      "Target SHA1 '$orig_commit' does not exist\n" . $herecurr);
+				}
+			} elsif ($short || $long || $space || $case || ($orig_desc ne $description) || !$hasparens) {
 				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 '$init_tag <12+ chars of sha1> (\"<title line>\")' - ie: '$init_tag $id (\"$description\")'\n" . $herecurr);
 			}
 		}
 
-- 
2.22.0

Re: [PATCH] checkpatch: Validate Fixes: tag using 'commit' checks

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 1236 bytes --]

Hi Sean,

On Fri, 30 Aug 2019 09:36:58 -0700 Sean Christopherson <sean.j.christopherson@intel.com> wrote:
>
> @@ -2803,10 +2805,15 @@ sub process {
>  			($id, $description) = git_commit_info($orig_commit,
>  							      $id, $orig_desc);
>  
> -			if (defined($id) &&
> -			   ($short || $long || $space || $case || ($orig_desc ne $description) || !$hasparens)) {
> +
> +			if (!defined($id)) {
> +				if ($init_tag =~ /fixes:/i) {
> +					ERROR("GIT_COMMIT_ID",
> +					      "Target SHA1 '$orig_commit' does not exist\n" . $herecurr);
> +				}

Unfortunately, git_commit_info() just returns the passed in $id (which
is explicitly set earlier) if git is not available or you are not in a
git repository (and that latter check is not entirely correct anyway).

Also, what you really need to test is if the specified commit is an
ancestor of the place in the maintainer's tree where this patch is to
be applied.  The commit may well exist in the developer's tree, but not
be in the maintainer's tree :-(

This will, however, catch the cases where the SHA1 has been mistyped,
but we should encourage people not to type them anyway, instead
generating them using "git log".

-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] checkpatch: Validate Fixes: tag using 'commit' checks

[Sean Christopherson]

On Sat, Aug 31, 2019 at 11:39:39AM +1000, Stephen Rothwell wrote:
> Hi Sean,
> 
> On Fri, 30 Aug 2019 09:36:58 -0700 Sean Christopherson <sean.j.christopherson@intel.com> wrote:
> >
> > @@ -2803,10 +2805,15 @@ sub process {
> >  			($id, $description) = git_commit_info($orig_commit,
> >  							      $id, $orig_desc);
> >  
> > -			if (defined($id) &&
> > -			   ($short || $long || $space || $case || ($orig_desc ne $description) || !$hasparens)) {
> > +
> > +			if (!defined($id)) {
> > +				if ($init_tag =~ /fixes:/i) {
> > +					ERROR("GIT_COMMIT_ID",
> > +					      "Target SHA1 '$orig_commit' does not exist\n" . $herecurr);
> > +				}
> 
> Unfortunately, git_commit_info() just returns the passed in $id (which
> is explicitly set earlier) if git is not available or you are not in a
> git repository (and that latter check is not entirely correct anyway).
> 
> Also, what you really need to test is if the specified commit is an
> ancestor of the place in the maintainer's tree where this patch is to
> be applied.  The commit may well exist in the developer's tree, but not
> be in the maintainer's tree :-(

True, but such an error would be caught if the maintainer or a reviewer
runs checkpatch after applying the commit, e.g. I'll run checkpatch as
part of reviewing a patch if I go through the effort of applying it,
which admittedly isn't all that often.
 
> This will, however, catch the cases where the SHA1 has been mistyped,
> but we should encourage people not to type them anyway, instead
> generating them using "git log".

What about adding an example formatting command to the error message, e.g.

  ERROR: Target SHA1 '265381004993' does not exist, use `git show -s
  --pretty='format:%h ("%s")'` or similar to verify and format the commit
  description


The same blurb could be also added to the error message for bad formatting

  ERROR: Please use git commit description style 'Fixes: <12+ chars of sha1>
  ("<title line>")', e.g. `git show -s --pretty='format:%h ("%s")'` -
  ie. 'Fixes: 265381004994 ("Merge tag '5.3-rc6-smb3-fixes' of
  git://git.samba.org/sfrench/cifs-2.6")