* [Linux-kernel-mentees] [PATCH] checkpatch: handle line break between commit and hash value
@ 2020-09-10 6:38 Ayush
2020-09-10 7:02 ` Lukas Bulwahn
0 siblings, 1 reply; 2+ messages in thread
From: Ayush @ 2020-09-10 6:38 UTC (permalink / raw)
To: lukas.bulwahn; +Cc: linux-kernel-mentees
While checking for improperly formed commit desciptions,
those cases where there is a line break between commit and
hash value, are not handled properly and an error is given.
This adds new conditions to parse and identify such commits and
to not give error in such cases.
following type of cases are handled:
- Without quotes in commits message.
commit
f4d51dffc6c01 ("Linux 5.9-rc4")
- With quotes in commit message.
commit
77f4689de17c ("fix regression in "epoll: Keep a reference on files added to the check list"")
(commits taken as example only)
Signed-off-by: Ayush <ayush@disroot.org>
---
scripts/checkpatch.pl | 19 +++++++++++++++++--
1 file changed, 17 insertions(+), 2 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 149518d2a6a7..639eb55041eb 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2831,8 +2831,9 @@ 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 =~ /(?:\s|^)[0-9a-f]{12,40}(?:[\s"'\(\[]|$)/i &&
+ ($line =~ /\bcommit$/i ||
+ $line =~ /\bcommit\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";
@@ -2852,6 +2853,8 @@ sub process {
$orig_commit = lc($2);
} elsif ($line =~ /\b([0-9a-f]{12,40})\b/i) {
$orig_commit = lc($1);
+ } elsif ($rawlines[$linenr] =~ /^([0-9a-f]{12,40})\b/i) {
+ $orig_commit = lc($1);
}
$short = 0 if ($line =~ /\bcommit\s+[0-9a-f]{12,40}/i);
@@ -2874,6 +2877,18 @@ sub process {
$rawlines[$linenr] =~ /^\s*([^"]+)"\)/;
$orig_desc .= " " . $1;
$hasparens = 1;
+ } elsif ($line =~ /\bcommit$/i &&
+ defined $rawlines[$linenr]) {
+ if($rawlines[$linenr] =~ /\b[0-9a-f]{5,}\s+\("([^"]+)"\)/i) {
+ $orig_desc = $1;
+ } elsif($rawlines[$linenr] =~ /\b[0-9a-f]{5,}\s+\("([^"]+"[^"]+[^"]")"\)/i) {
+ $orig_desc = $1;
+ }
+ $hasparens = 1;
+ $space = 0;
+ $short = 0 if ($rawlines[$linenr] =~ /^[0-9a-f]{12,40}/i);
+ $long = 1 if ($rawlines[$linenr] =~ /^[0-9a-f]{41,}/i);
+ $case = 0 if ($rawlines[$linenr] =~ /^[0-9a-f]{5,40}[^A-F]/i);
}
($id, $description) = git_commit_info($orig_commit,
--
2.28.0
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH] checkpatch: handle line break between commit and hash value
2020-09-10 6:38 [Linux-kernel-mentees] [PATCH] checkpatch: handle line break between commit and hash value Ayush
@ 2020-09-10 7:02 ` Lukas Bulwahn
0 siblings, 0 replies; 2+ messages in thread
From: Lukas Bulwahn @ 2020-09-10 7:02 UTC (permalink / raw)
To: Ayush; +Cc: linux-kernel-mentees
On Thu, 10 Sep 2020, Ayush wrote:
> While checking for improperly formed commit desciptions,
> those cases where there is a line break between commit and
> hash value, are not handled properly and an error is given.
>
This sentence above is not comprehensible.
How about
checkpatch.pl checks for proper references to other commits with
the intended format, commit <12 characters of SHA-1 ID> ("<commit
message") and warns about typical mistakes. Currently, it does
not handle the case where there is a line break between commit and the
hash value.
It falsely warns that the hash is not prefixed by the word commit.
At the very end, you can also write:
This issue was discovered through a thorough investigation of
checkpatch.pl warnings of type COMMIT_ID on commit range ??.
Maybe you can then also report a before and after statistics to see the
impact of your change?
> This adds new conditions to parse and identify such commits and
> to not give error in such cases.
>
"to not give error" is not proper English, how about "to not report an
error"?
> following type of cases are handled:
>
> - Without quotes in commits message.
> commit
> f4d51dffc6c01 ("Linux 5.9-rc4")
>
> - With quotes in commit message.
> commit
> 77f4689de17c ("fix regression in "epoll: Keep a reference on files added to the check list"")
>
> (commits taken as example only)
>
THIS IS IMPORTANT:
You are doing two things at once, and hence the review is more difficult.
How about one commit for each new case you are handling?
One commit for handling the line break, then another commit for handling
quotes in commit messages?
Looking forward to the next version.
Lukas
> Signed-off-by: Ayush <ayush@disroot.org>
> ---
> scripts/checkpatch.pl | 19 +++++++++++++++++--
> 1 file changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 149518d2a6a7..639eb55041eb 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2831,8 +2831,9 @@ 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 =~ /(?:\s|^)[0-9a-f]{12,40}(?:[\s"'\(\[]|$)/i &&
> + ($line =~ /\bcommit$/i ||
> + $line =~ /\bcommit\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";
> @@ -2852,6 +2853,8 @@ sub process {
> $orig_commit = lc($2);
> } elsif ($line =~ /\b([0-9a-f]{12,40})\b/i) {
> $orig_commit = lc($1);
> + } elsif ($rawlines[$linenr] =~ /^([0-9a-f]{12,40})\b/i) {
> + $orig_commit = lc($1);
> }
>
> $short = 0 if ($line =~ /\bcommit\s+[0-9a-f]{12,40}/i);
> @@ -2874,6 +2877,18 @@ sub process {
> $rawlines[$linenr] =~ /^\s*([^"]+)"\)/;
> $orig_desc .= " " . $1;
> $hasparens = 1;
> + } elsif ($line =~ /\bcommit$/i &&
> + defined $rawlines[$linenr]) {
> + if($rawlines[$linenr] =~ /\b[0-9a-f]{5,}\s+\("([^"]+)"\)/i) {
> + $orig_desc = $1;
> + } elsif($rawlines[$linenr] =~ /\b[0-9a-f]{5,}\s+\("([^"]+"[^"]+[^"]")"\)/i) {
> + $orig_desc = $1;
> + }
> + $hasparens = 1;
> + $space = 0;
> + $short = 0 if ($rawlines[$linenr] =~ /^[0-9a-f]{12,40}/i);
> + $long = 1 if ($rawlines[$linenr] =~ /^[0-9a-f]{41,}/i);
> + $case = 0 if ($rawlines[$linenr] =~ /^[0-9a-f]{5,40}[^A-F]/i);
> }
>
> ($id, $description) = git_commit_info($orig_commit,
> --
> 2.28.0
>
>
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2020-09-10 7:02 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-10 6:38 [Linux-kernel-mentees] [PATCH] checkpatch: handle line break between commit and hash value Ayush
2020-09-10 7:02 ` Lukas Bulwahn
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).