| Message ID | 20210818154646.925351-1-efremov@linux.com |
|---|---|
| Headers | show |
| Series | selftests: floppy: basic tests | expand |
On Wed, 2021-08-18 at 18:46 +0300, Denis Efremov wrote: > Properly handle commits like: > commit f2791e7eadf4 ("Revert "floppy: refactor open() flags handling"") Try this one: https://lore.kernel.org/lkml/7f55d9d0369f1ea840fab83eeb77f9f3601fee41.camel@perches.com/ > > Cc: Joe Perches <joe@perches.com> > Signed-off-by: Denis Efremov <efremov@linux.com> > --- > scripts/checkpatch.pl | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > index 461d4221e4a4..cf31e8c994d3 100755 > --- a/scripts/checkpatch.pl > +++ b/scripts/checkpatch.pl > @@ -3200,20 +3200,20 @@ sub process { > $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 =~ /\bcommit\s+[0-9a-f]{5,}\s+\("(.+)"\)/i) { > $orig_desc = $1; > $hasparens = 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 && > + } elsif ($line =~ /\bcommit\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 =~ /\bcommit\s+[0-9a-f]{5,}\s+\("(.+)$/i; > $orig_desc = $1; > - $rawlines[$linenr] =~ /^\s*([^"]+)"\)/; > + $rawlines[$linenr] =~ /^\s*(.+)"\)/; > $orig_desc .= " " . $1; > $hasparens = 1; > }
On 8/18/21 7:00 PM, Joe Perches wrote: > On Wed, 2021-08-18 at 18:46 +0300, Denis Efremov wrote: >> Properly handle commits like: >> commit f2791e7eadf4 ("Revert "floppy: refactor open() flags handling"") > > Try this one: > > https://lore.kernel.org/lkml/7f55d9d0369f1ea840fab83eeb77f9f3601fee41.camel@perches.com/ > It works but why not to use .+? then? I'm not sure that non-greedy patterns will properly handle commits like: $ git log --oneline | fgrep '")' e.g. commit ece2619fe8ed ("extcon: arizona: Fix flags parameter to the gpiod_get("wlf,micd-pol") call") >> >> Cc: Joe Perches <joe@perches.com> >> Signed-off-by: Denis Efremov <efremov@linux.com> >> --- >> scripts/checkpatch.pl | 12 ++++++------ >> 1 file changed, 6 insertions(+), 6 deletions(-) >> >> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl >> index 461d4221e4a4..cf31e8c994d3 100755 >> --- a/scripts/checkpatch.pl >> +++ b/scripts/checkpatch.pl >> @@ -3200,20 +3200,20 @@ sub process { >> $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 =~ /\bcommit\s+[0-9a-f]{5,}\s+\("(.+)"\)/i) { >> $orig_desc = $1; >> $hasparens = 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 && >> + } elsif ($line =~ /\bcommit\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 =~ /\bcommit\s+[0-9a-f]{5,}\s+\("(.+)$/i; >> $orig_desc = $1; >> - $rawlines[$linenr] =~ /^\s*([^"]+)"\)/; >> + $rawlines[$linenr] =~ /^\s*(.+)"\)/; >> $orig_desc .= " " . $1; >> $hasparens = 1; >> } >
On Wed, 2021-08-18 at 19:21 +0300, Denis Efremov wrote: > > On 8/18/21 7:00 PM, Joe Perches wrote: > > On Wed, 2021-08-18 at 18:46 +0300, Denis Efremov wrote: > > > Properly handle commits like: > > > commit f2791e7eadf4 ("Revert "floppy: refactor open() flags handling"") > > > > Try this one: > > > > https://lore.kernel.org/lkml/7f55d9d0369f1ea840fab83eeb77f9f3601fee41.camel@perches.com/ > > > > It works but why not to use .+? then? > I'm not sure that non-greedy patterns will properly handle commits like: > $ git log --oneline | fgrep '")' > > e.g. > commit ece2619fe8ed ("extcon: arizona: Fix flags parameter to the gpiod_get("wlf,micd-pol") call") The only way to handle that is to use the $balanced_parens test but it wouldn't work on Andrew's perl version 5.8 Andrew? Do you still use perl 5.8? It's almost 20 years old now. Does anyone still use perl versions 5.8 or lower?
Hey Denis:
Try this one please and let me know what you think...
---
scripts/checkpatch.pl | 31 +++++++++++++------------------
1 file changed, 13 insertions(+), 18 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 161ce7fe5d1e5..4e2e79eff9b8c 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3196,26 +3196,21 @@ sub process {
$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) {
- $orig_desc = $1;
- $hasparens = 1;
- } elsif ($line =~ /\bcommit\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 &&
- defined $rawlines[$linenr] &&
- $rawlines[$linenr] =~ /^\s*[^"]+"\)/) {
- $line =~ /\bcommit\s+[0-9a-f]{5,}\s+\("([^"]+)$/i;
+ my $input = $line;
+ for (my $n = 0; $n < 2; $n++) {
+ $input .= " $rawlines[$linenr + $n]" if ($#lines >= $linenr + $n);
+ }
+
+ $short = 0 if ($input =~ /\bcommit\s+[0-9a-f]{12,40}/i);
+ $long = 1 if ($input =~ /\bcommit\s+[0-9a-f]{41,}/i);
+ $space = 0 if ($input =~ /\bcommit [0-9a-f]/i);
+ $case = 0 if ($input =~ /\b[Cc]ommit\s+[0-9a-f]{5,40}[^A-F]/);
+ if ($input =~ /\bcommit\s+[0-9a-f]{5,}\s+($balanced_parens)/i) {
$orig_desc = $1;
- $rawlines[$linenr] =~ /^\s*([^"]+)"\)/;
- $orig_desc .= " " . $1;
$hasparens = 1;
+ # Always strip leading/trailing parens then double quotes if existing
+ $orig_desc = substr($orig_desc, 1, -1);
+ $orig_desc = substr($orig_desc, 1, -1) if ($orig_desc =~ /^".*"$/);
}
($id, $description) = git_commit_info($orig_commit,
Hi, On 8/19/21 12:22 AM, Joe Perches wrote: > Hey Denis: > > Try this one please and let me know what you think... Looks good to me. Couple of nitpicks below > > --- > scripts/checkpatch.pl | 31 +++++++++++++------------------ > 1 file changed, 13 insertions(+), 18 deletions(-) > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > index 161ce7fe5d1e5..4e2e79eff9b8c 100755 > --- a/scripts/checkpatch.pl > +++ b/scripts/checkpatch.pl > @@ -3196,26 +3196,21 @@ sub process { > $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) { > - $orig_desc = $1; > - $hasparens = 1; > - } elsif ($line =~ /\bcommit\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 && > - defined $rawlines[$linenr] && > - $rawlines[$linenr] =~ /^\s*[^"]+"\)/) { > - $line =~ /\bcommit\s+[0-9a-f]{5,}\s+\("([^"]+)$/i; > + my $input = $line; > + for (my $n = 0; $n < 2; $n++) { > + $input .= " $rawlines[$linenr + $n]" if ($#lines >= $linenr + $n); > + } > + > + $short = 0 if ($input =~ /\bcommit\s+[0-9a-f]{12,40}/i); > + $long = 1 if ($input =~ /\bcommit\s+[0-9a-f]{41,}/i); > + $space = 0 if ($input =~ /\bcommit [0-9a-f]/i); > + $case = 0 if ($input =~ /\b[Cc]ommit\s+[0-9a-f]{5,40}[^A-F]/); > + if ($input =~ /\bcommit\s+[0-9a-f]{5,}\s+($balanced_parens)/i) { > $orig_desc = $1; > - $rawlines[$linenr] =~ /^\s*([^"]+)"\)/; > - $orig_desc .= " " . $1; > $hasparens = 1; > + # Always strip leading/trailing parens then double quotes if existing > + $orig_desc = substr($orig_desc, 1, -1); > + $orig_desc = substr($orig_desc, 1, -1) if ($orig_desc =~ /^".*"$/); Why do you want to add "if ($orig_desc =~ /^".*"$/);" here? and not just substr($orig_desc, 2, -2);? > } > > ($id, $description) = git_commit_info($orig_commit, > In your previous patch with '.*?' you added a branch to allow also newlines between commit and shas: ``` commit c3f157259438 (Revert "floppy: reintroduce O_NDELAY fix") ``` Maybe something like this will work (adding a last word from a prevline if line doesn't start from commit) + my $input = $line; if ($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); + $prevline =~ /(\w+)$/; + $line = $1 . " " . $prevline; } - my $input = $line; for (my $n = 0; $n < 2; $n++) { $input .= " $rawlines[$linenr + $n]" if ($#lines >= $linenr + $n); } Thanks, Denis
On Thu, 2021-08-19 at 22:52 +0300, Denis Efremov wrote: > Hi, > > On 8/19/21 12:22 AM, Joe Perches wrote: > > Hey Denis: > > > > Try this one please and let me know what you think... > > Looks good to me. Couple of nitpicks below yeah, thanks. How about this one: --- scripts/checkpatch.pl | 72 ++++++++++++++++++++++++++++++--------------------- 1 file changed, 43 insertions(+), 29 deletions(-) diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 161ce7fe5d1e5..4988515a0dfb3 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -1181,7 +1181,8 @@ sub git_commit_info { # git log --format='%H %s' -1 $line | # echo "commit $(cut -c 1-12,41-)" # done - } elsif ($lines[0] =~ /^fatal: ambiguous argument '$commit': unknown revision or path not in the working tree\./) { + } elsif ($lines[0] =~ /^fatal: ambiguous argument '$commit': unknown revision or path not in the working tree\./ || + $lines[0] =~ /^fatal: bad object $commit/) { $id = undef; } else { $id = substr($lines[0], 0, 12); @@ -2587,6 +2588,8 @@ sub process { my $reported_maintainer_file = 0; my $non_utf8_charset = 0; + my $last_git_commit_id_linenr = -1; + my $last_blank_line = 0; my $last_coalesced_string_linenr = -1; @@ -3173,7 +3176,8 @@ sub process { if ($in_commit_log && !$commit_log_possible_stack_dump && $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 =~ /\bcommit\s+[0-9a-f]{5,}\b/i || + ($line =~ /\bcommit\s*$/i && defined($rawlines[$linenr]) && $rawlines[$linenr] =~ /^\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))) { @@ -3183,49 +3187,59 @@ sub process { my $long = 0; my $case = 1; my $space = 1; - my $hasdesc = 0; my $hasparens = 0; my $id = '0123456789ab'; my $orig_desc = "commit description"; my $description = ""; + my $herectx = $herecurr; + my $has_parens = 0; + + my $input = $line; + if ($line =~ /(?:\bcommit\s+[0-9a-f]{5,}|\bcommit\s*$)/i) { + for (my $n = 0; $n < 2; $n++) { + if ($input =~ /\bcommit\s+[0-9a-f]{5,}\s*$balanced_parens/i) { + $has_parens = 1; + last; + } + last if ($#lines < $linenr + $n); + $input .= " " . trim($rawlines[$linenr + $n]); + $herectx .= "$rawlines[$linenr + $n]\n"; + } + $herectx = $herecurr if (!$has_parens); + } - if ($line =~ /\b(c)ommit\s+([0-9a-f]{5,})\b/i) { + if ($input =~ /\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) { + $short = 0 if ($input =~ /\bcommit\s+[0-9a-f]{12,40}/i); + $long = 1 if ($input =~ /\bcommit\s+[0-9a-f]{41,}/i); + $space = 0 if ($input =~ /\bcommit [0-9a-f]/i); + $case = 0 if ($input =~ /\b[Cc]ommit\s+[0-9a-f]{5,40}[^A-F]/); + + if ($input =~ /\bcommit\s+[0-9a-f]{5,}\s+($balanced_parens)/i) { + $orig_desc = $1; + # Always strip leading/trailing parens then double quotes if existing + $orig_desc = substr($orig_desc, 1, -1); + if ($orig_desc =~ /^".*"$/) { + $orig_desc = substr($orig_desc, 1, -1); + $hasparens = 1; + } + } + } elsif ($input =~ /\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) { - $orig_desc = $1; - $hasparens = 1; - } elsif ($line =~ /\bcommit\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 && - defined $rawlines[$linenr] && - $rawlines[$linenr] =~ /^\s*[^"]+"\)/) { - $line =~ /\bcommit\s+[0-9a-f]{5,}\s+\("([^"]+)$/i; - $orig_desc = $1; - $rawlines[$linenr] =~ /^\s*([^"]+)"\)/; - $orig_desc .= " " . $1; - $hasparens = 1; - } - ($id, $description) = git_commit_info($orig_commit, $id, $orig_desc); if (defined($id) && - ($short || $long || $space || $case || ($orig_desc ne $description) || !$hasparens)) { + ($short || $long || $space || $case || ($orig_desc ne $description) || !$hasparens) && + $last_git_commit_id_linenr != $linenr - 1) { 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 'commit <12+ chars of sha1> (\"<title line>\")' - ie: '${init_char}ommit $id (\"$description\")'\n" . $herectx); } + #don't report the next line if this line ends in commit and the sha1 hash is the next line + $last_git_commit_id_linenr = $linenr if ($line =~ /\bcommit\s*$/i); } # Check for added, moved or deleted files
On Thu, 2021-08-19 at 22:52 +0300, Denis Efremov wrote: > Hi, > > Why do you want to add "if ($orig_desc =~ /^".*"$/);" here? and not just substr($orig_desc, 2, -2);? Because commit descriptions sometimes to not have quotes like commit <deadbeef> (Multiple word description) btw: I tested the last proposal with this script: $ git log --grep="commit [0-9a-f]" -i --format=%h -1000 | \ while read commit ; do \ echo $commit; \ ./scripts/checkpatch.pl --git --no-summary --quiet --types=GIT_COMMIT_ID $commit ; \ done and there are still a fair number of ERRORs. And I'm not sure if this particular ERROR is that useful overall.
On 8/20/21 1:17 AM, Joe Perches wrote: > > And I'm not sure if this particular ERROR is that useful overall. I find it useful to check commit-id and that it matches a title. It's easy to make a typo in commit-id and get an invalid one. Denis
On Sat, 2021-08-21 at 09:47 +0300, Denis Efremov wrote: > > On 8/20/21 1:17 AM, Joe Perches wrote: > > > > And I'm not sure if this particular ERROR is that useful overall. > > I find it useful to check commit-id and that it matches a title. > It's easy to make a typo in commit-id and get an invalid one. That's true, but I meant requiring the sha1 hash to contain both the word "commit" and use ("title"). Looking at checkpatch's errors produced by this GIT_COMMIT_ID test makes the required form seem a bit too inflexible to me. For instance: a sha1 hash may be repeated in a commit message where the first instance has the correct form but the second use is just the hash and the warning is still produced.
On 8/21/21 10:12 AM, Joe Perches wrote: > On Sat, 2021-08-21 at 09:47 +0300, Denis Efremov wrote: >> >> On 8/20/21 1:17 AM, Joe Perches wrote: >>> >>> And I'm not sure if this particular ERROR is that useful overall. >> >> I find it useful to check commit-id and that it matches a title. >> It's easy to make a typo in commit-id and get an invalid one. > > That's true, but I meant requiring the sha1 hash to contain both > the word "commit" and use ("title"). > > Looking at checkpatch's errors produced by this GIT_COMMIT_ID > test makes the required form seem a bit too inflexible to me. > > For instance: a sha1 hash may be repeated in a commit message where > the first instance has the correct form but the second use is just > the hash and the warning is still produced. > I agree with you. There is also another example with list of commits: - commit <id-1> ("Title1") - commit <id-2> ("Title2") ... I see no reason in writing "commit" on each line.