Messages in this thread Patch in this message |  | | Subject | Re: [PATCH 1/1] checkpatch: remove false warning for commit reference | | From | Heinrich Schuchardt <> | | Date | Mon, 24 Oct 2016 19:22:33 +0200 |
| |
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 <xypron.glpk@gmx.de> >> --- >> 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); > } > > >
|  |