Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965004AbcJXR3B (ORCPT ); Mon, 24 Oct 2016 13:29:01 -0400 Received: from mout.gmx.net ([212.227.17.20]:60965 "EHLO mout.gmx.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S935553AbcJXR27 (ORCPT ); Mon, 24 Oct 2016 13:28:59 -0400 Subject: Re: [PATCH 1/1] checkpatch: remove false warning for commit reference To: Joe Perches , Andy Whitcroft , Andrew Morton References: <1465608362-4677-1-git-send-email-xypron.glpk@gmx.de> <1477208091-4887-1-git-send-email-xypron.glpk@gmx.de> <1477255021.3561.9.camel@perches.com> Cc: linux-kernel@vger.kernel.org From: Heinrich Schuchardt X-Enigmail-Draft-Status: N1110 Message-ID: Date: Mon, 24 Oct 2016 19:22:33 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Icedove/45.4.0 MIME-Version: 1.0 In-Reply-To: <1477255021.3561.9.camel@perches.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit X-Provags-ID: V03:K0:/GKFSqKZU45vSro7lPDfSJtpvW106veFSzGh+pgGWGm8899bTEA fXfiwXSmX9hQ6cCnsixml33kFgtQfMm+85OBHfA3PBiaos/a84l4oMr04hQKYYTtiRM95lt 5hOwTWN4c0MlqnVPW8+6ZPk1zRxRRUrrSxDfPhpMCYR8PR0gMlNcLDxHcqgdFkzkV8Kttw1 nSfcUcwnHRwSfOtT4Bvxw== X-UI-Out-Filterresults: notjunk:1;V01:K0:GdeekLhMFqU=:HKN0Uq3lNjuI32MPpuw9Ly xCSIxcg+OCGPWk+QZzyh4931jOBbuiH+5jo24PvZfJq0/GdMRmn8j1snTPkQ/jnFANYKoo9Bx I+7ayFdAj/hp2JncHZl4xbVz7hpqoUcdXydvW6q2VI48vs2Z2MrxndoIQcf8K4UjXYRj7lhO/ UmLq0fXK5eSI/W01nHqatGHxwWlLdGobGoNMDzl5awL9J7r2IZA9TeQsZqbqbQ8g+yxk8yX57 I90BM6SAWf06vfw4iQfrmhWfnAA1XMadGGdb+SJ8wFSlVT0FFOrZV/G1e458dsSXXAtOzLtZX 5gLaKj9UBIZZlE9WQkgm9+KE0BSq93oshBRS7kAg1A9f+QHenw4mqOTXzzg1iod82ogTQER0q 43W5I820npNCM/DV7d/GTJlCo89h+YVYm07ofLZMqVl1vKaQ3i9bRVmW504Qy+qy8xs6lD8Yl d0aLP69a79xcpkH0/mmRxsyGF4hYs8nZq/n0gvU6mmCVbeNzPslE1q9Hc/IvNmSdwHJ0kMOUK j2lDDtYKfByPuU7CgeAiMd8fhX6fePqtMFTYe1cVyPqcrJkHEe0bi1AUoB0FfGawckQHR+FkJ o0iiD2Or5S6fo7lp/3tFTD4BzwP+vIWEzyFRVWDCkan1gGob0UAkwyQ7Z+BPH4u1nfvBr9pS/ CLQfjczfa+5CpHXYLV4NSgsJBPoLp9nMrfni9TPMKsZO8+rtNk4thtfBSYIpJl7q9edwkQWgX UMmLffdJAG/uU002ei77I+U8xqcaRIae8bBbMNSC5WxHZZ4gPAqWOZ1Ju1sRgm9uL4J73w4zB SeWoy2I Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4294 Lines: 127 On 10/23/2016 10:37 PM, Joe Perches wrote: > On Sun, 2016-10-23 at 09:34 +0200, Heinrich Schuchardt wrote: >> Checkpatch warns of an incorrect commit reference style >> for any hexadecimal number of 12 digits and more. >> >> Numbers of 12 digits are not necessarily commit ids. >> >> For an example provoking the problem see >> https://patchwork.kernel.org/patch/9170897/ >> >> Checkpatch should only warn if the number refers to an >> existing commit. > > That seems sensible. > >> Signed-off-by: Heinrich Schuchardt >> --- >> scripts/checkpatch.pl | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > [] >> @@ -848,6 +848,7 @@ sub git_commit_info { >> # echo "commit $(cut -c 1-12,41-)" >> # done >> } elsif ($lines[0] =~ /^fatal: ambiguous argument '$commit': unknown revision or path not in the working tree\./) { >> + $id = undef; > > OK > >> } else { >> $id = substr($lines[0], 0, 12); >> $desc = substr($lines[0], 41); >> @@ -2577,7 +2578,9 @@ sub process { >> ($id, $description) = git_commit_info($orig_commit, >> $id, $orig_desc); >> >> - if ($short || $long || $space || $case || ($orig_desc ne $description) || !$hasparens) { >> + if ($id && ($short || $long || $space || $case >> + || ($orig_desc ne $description) >> + || !$hasparens)) { > > I'd prefer > if (defined($id) && For $id = 0 or $id = "" this would make a difference. Surely I can update the patch like this to make the test more readable. > ($short || $long || $space || $case || ($orig_desc ne $description) || !$hasparens)) { > > and not wrap the tests. Putting defined($id) on a line of its own is fine. Not wrapping the tests will produce a line of over 80 characters and give a warning in scripts/checkpatch.pl. The script should respect the standards it imposes on others. > > Maybe ignore all the cases there git is not installed too. > Something like: > --- > scripts/checkpatch.pl | 16 ++++++++++------ > 1 file changed, 10 insertions(+), 6 deletions(-) > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > index a8368d1c4348..a46b6ebe067b 100755 > --- a/scripts/checkpatch.pl > +++ b/scripts/checkpatch.pl > @@ -829,13 +829,16 @@ sub seed_camelcase_includes { > sub git_commit_info { > my ($commit, $id, $desc) = @_; > > - return ($id, $desc) if ((which("git") eq "") || !(-e ".git")); > + my $git_id = undef; > + my $git_desc = undef; > + > + return ($git_id, $git_desc) if ((which("git") eq "") || !(-e ".git")); Why not simply return (undef, undef) if ((which("git") eq "") || !(-e ".git")); Should we provide a warning: "git not found"? > > my $output = `git log --no-color --format='%H %s' -1 $commit 2>&1`; > $output =~ s/^\s*//gm; > my @lines = split("\n", $output); > > - return ($id, $desc) if ($#lines < 0); > + return ($git_id, $git_desc) if ($#lines < 0); > > if ($lines[0] =~ /^error: short SHA1 $commit is ambiguous\./) { > # Maybe one day convert this block of bash into something that returns > @@ -849,11 +852,11 @@ sub git_commit_info { > # done > } elsif ($lines[0] =~ /^fatal: ambiguous argument '$commit': unknown revision or path not in the working tree\./) { > } else { > - $id = substr($lines[0], 0, 12); > - $desc = substr($lines[0], 41); > + $git_id = substr($lines[0], 0, 12); > + $git_desc = substr($lines[0], 41); > } > > - return ($id, $desc); > + return ($git_id, $git_desc); This will change the program logic for if ($lines[0] =~ /^error: short SHA1 $commit is ambiguous\./) > } > > $chk_signoff = 0 if ($file); > @@ -2577,7 +2580,8 @@ sub process { > ($id, $description) = git_commit_info($orig_commit, > $id, $orig_desc); > > - if ($short || $long || $space || $case || ($orig_desc ne $description) || !$hasparens) { > + if (defined($id) && > + ($short || $long || $space || $case || ($orig_desc ne $description) || !$hasparens)) { This line is over 80 characters. Which is not accepatble according to checkpatch.pl. > ERROR("GIT_COMMIT_ID", > "Please use git commit description style 'commit <12+ chars of sha1> (\"\")' - ie: '${init_char}ommit $id (\"$description\")'\n" . $herecurr); > } > > >