linux-kernel-mentees.lists.linuxfoundation.org archive mirror
 help / color / mirror / Atom feed
* [Linux-kernel-mentees] [PATCH v2] checkpatch: handle line break between commit and hash value
@ 2020-09-12  9:48 Ayush
  2020-09-12 11:17 ` Lukas Bulwahn
                   ` (5 more replies)
  0 siblings, 6 replies; 14+ messages in thread
From: Ayush @ 2020-09-12  9:48 UTC (permalink / raw)
  To: lukas.bulwahn; +Cc: linux-kernel-mentees

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.

This adds new conditions to parse and identify such commits and
to not report an error in such cases.

following type of commit reference is handled:

- commit
f4d51dffc6c01 ("Linux 5.9-rc4")

This issue was discovered through a thorough analysis of checkpatch.pl
errors and warnings of type GIT_COMMIT_ID on commits between v5.7 and v5.8.

Before applying this patch, checkpatch.pl reported 342 errors of type
GIT_COMMIT_ID. After applying patch, errors reduced to 284.

Signed-off-by: Ayush <ayush@disroot.org>
---
Change in v2:
    - Add conditions to check and not report error if "commit" is last
      word of line.
    - Remove handling of multiple quotes in commit message (Will be
      added in another patch).
    - If a line break is encountered between commit and hash value, then
      it will again check for next line (starting with hash) and report 
      error. Add a condition to skip checking next line.

 scripts/checkpatch.pl | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 149518d2a6a7..42de3939a445 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -68,7 +68,7 @@ my $allow_c99_comments = 1; # Can be overridden by --ignore C99_COMMENT_TOLERANC
 my $git_command ='export LANGUAGE=en_US.UTF-8; git';
 my $tabsize = 8;
 my ${CONFIG_} = "CONFIG_";
-
+my $c_count = 0;
 sub help {
 	my ($exitcode) = @_;
 
@@ -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 && ($rawlines[$linenr] =~ /^[0-9a-f]{5,}/i))||
+		     $line =~ /\bcommit\s+[0-9a-f]{5,}\b/i ||
+		     ((($line =~ /(?:\s|^)[0-9a-f]{12,40}(?:[\s"'\(\[]|$)/i) && ($c_count++ != 1)) &&
 		      $line !~ /[\<\[][0-9a-f]{12,40}[\>\]]/i &&
 		      $line !~ /\bfixes:\s*[0-9a-f]{12,40}/i))) {
 			my $init_char = "c";
@@ -2846,12 +2847,15 @@ sub process {
 			my $id = '0123456789ab';
 			my $orig_desc = "commit description";
 			my $description = "";
+			$c_count = 0;
 
 			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);
+			} 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 +2878,17 @@ 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;
+				}
+				$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);
+				$c_count = 1;
 			}
 
 			($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] 14+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v2] checkpatch: handle line break between commit and hash value
  2020-09-12  9:48 [Linux-kernel-mentees] [PATCH v2] checkpatch: handle line break between commit and hash value Ayush
@ 2020-09-12 11:17 ` Lukas Bulwahn
  2020-09-12 12:42 ` Lukas Bulwahn
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 14+ messages in thread
From: Lukas Bulwahn @ 2020-09-12 11:17 UTC (permalink / raw)
  To: Ayush; +Cc: linux-kernel-mentees



On Sat, 12 Sep 2020, Ayush wrote:

> 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.
> 
> This adds new conditions to parse and identify such commits and

s/commit/commit references/

> to not report an error in such cases.
>

---
> following type of commit reference is handled:
> 
> - commit
> f4d51dffc6c01 ("Linux 5.9-rc4")
>
---
This part above is superfluous; I do not see the added value here.
 
> This issue was discovered through a thorough analysis of checkpatch.pl
> errors and warnings of type GIT_COMMIT_ID on commits between v5.7 and v5.8.
> 
> Before applying this patch, checkpatch.pl reported 342 errors of type
> GIT_COMMIT_ID. After applying patch, errors reduced to 284.
>

Impressive numbers of improvement if that is true and not a bug.

I am currently running my evaluation to see if I see the same numbers; I 
will get back to you.

Lukas
_______________________________________________
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] 14+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v2] checkpatch: handle line break between commit and hash value
  2020-09-12  9:48 [Linux-kernel-mentees] [PATCH v2] checkpatch: handle line break between commit and hash value Ayush
  2020-09-12 11:17 ` Lukas Bulwahn
@ 2020-09-12 12:42 ` Lukas Bulwahn
  2020-09-12 13:34 ` Ayush
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 14+ messages in thread
From: Lukas Bulwahn @ 2020-09-12 12:42 UTC (permalink / raw)
  To: Ayush; +Cc: linux-kernel-mentees



On Sat, 12 Sep 2020, Ayush wrote:

> 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.
> 
> This adds new conditions to parse and identify such commits and
> to not report an error in such cases.
> 
> following type of commit reference is handled:
> 
> - commit
> f4d51dffc6c01 ("Linux 5.9-rc4")
> 
> This issue was discovered through a thorough analysis of checkpatch.pl
> errors and warnings of type GIT_COMMIT_ID on commits between v5.7 and v5.8.
> 
> Before applying this patch, checkpatch.pl reported 342 errors of type
> GIT_COMMIT_ID. After applying patch, errors reduced to 284.
> 
> Signed-off-by: Ayush <ayush@disroot.org>

The patch does not apply on commit f4d51dffc6c0 ("Linux 5.9-rc4").

Applying: checkpatch: handle line break between commit and hash value
Checking patch scripts/checkpatch.pl...
error: while searching for:
my $git_command ='export LANGUAGE=en_US.UTF-8; git';
my $tabsize = 8;
my ${CONFIG_} = "CONFIG_";

sub help {
	my ($exitcode) = @_;


error: patch failed: scripts/checkpatch.pl:68
Hunk #2 succeeded at 2825 (offset -6 lines).
Hunk #3 succeeded at 2841 (offset -6 lines).
Hunk #4 succeeded at 2872 (offset -6 lines).
Applying patch scripts/checkpatch.pl with 1 reject...
Rejected hunk #1.
Hunk #2 applied cleanly.
Hunk #3 applied cleanly.
Hunk #4 applied cleanly.
Patch failed at 0001 checkpatch: handle line break between commit and hash value


What commit is your patch based on?

Try always to test that the commit applies on the current master and on 
the latest linux-next.

Lukas
_______________________________________________
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] 14+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v2] checkpatch: handle line break between commit and hash value
  2020-09-12  9:48 [Linux-kernel-mentees] [PATCH v2] checkpatch: handle line break between commit and hash value Ayush
  2020-09-12 11:17 ` Lukas Bulwahn
  2020-09-12 12:42 ` Lukas Bulwahn
@ 2020-09-12 13:34 ` Ayush
  2020-09-12 14:15 ` Lukas Bulwahn
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 14+ messages in thread
From: Ayush @ 2020-09-12 13:34 UTC (permalink / raw)
  To: Lukas Bulwahn; +Cc: linux-kernel-mentees

Hello sir,

> What commit is your patch based on?
> 
> Try always to test that the commit applies on the current master and on
> the latest linux-next.

This patch is tested on linux-next (tag: next-20200911).
It won't merge on master as it is some commits behind the next-20200911
and that is maybe causing conflict.

 
Thanks,
Ayush
_______________________________________________
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] 14+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v2] checkpatch: handle line break between commit and hash value
  2020-09-12  9:48 [Linux-kernel-mentees] [PATCH v2] checkpatch: handle line break between commit and hash value Ayush
                   ` (2 preceding siblings ...)
  2020-09-12 13:34 ` Ayush
@ 2020-09-12 14:15 ` Lukas Bulwahn
  2020-09-12 15:38 ` Ayush
  2020-09-12 19:37 ` Ayush
  5 siblings, 0 replies; 14+ messages in thread
From: Lukas Bulwahn @ 2020-09-12 14:15 UTC (permalink / raw)
  To: Ayush; +Cc: linux-kernel-mentees



On Sat, 12 Sep 2020, Ayush wrote:

> 
> Before applying this patch, checkpatch.pl reported 342 errors of type
> GIT_COMMIT_ID. After applying patch, errors reduced to 284.
>

I could not reproduce those numbers.

Can you share exactly what you are running? And can you share the list of 
differences in your report?

I have seen that the line number now has also changed in some cases.

We can check if the script really improves the detection or not.

>  my ${CONFIG_} = "CONFIG_";
> -
> +my $c_count = 0;

Why do you think this variable definition is supposed to be placed here?

Where are other variables with similar use defined in the checkpatch.pl 
script?

Lukas
_______________________________________________
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] 14+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v2] checkpatch: handle line break between commit and hash value
  2020-09-12  9:48 [Linux-kernel-mentees] [PATCH v2] checkpatch: handle line break between commit and hash value Ayush
                   ` (3 preceding siblings ...)
  2020-09-12 14:15 ` Lukas Bulwahn
@ 2020-09-12 15:38 ` Ayush
  2020-09-12 19:37 ` Ayush
  5 siblings, 0 replies; 14+ messages in thread
From: Ayush @ 2020-09-12 15:38 UTC (permalink / raw)
  To: Lukas Bulwahn; +Cc: linux-kernel-mentees

Sir,

> I could not reproduce those numbers.
> 
> Can you share exactly what you are running? And can you share the list of
> differences in your report?
> 
> I have seen that the line number now has also changed in some cases.
> 
> We can check if the script really improves the detection or not.

I am re-evaluating my output and will give you a analysis of all commits
which are getting affected by my patch.
It will take time to run checkpatch.pl again on those commits, so I will
share as soon as it's finished.

>> my ${CONFIG_} = "CONFIG_";
>> -
>> +my $c_count = 0;
> 
> Why do you think this variable definition is supposed to be placed here?
> 
> Where are other variables with similar use defined in the checkpatch.pl
> script?

I think this variable should not be defined here, similar variables are added in
the subroutine "process".

In cases like:
- commit
1234567890ab ("foo bar")

It was checking line 1, then it was checking line 2 and then reports the error, which
should not be the case.

This variable is used to track such cases and do not check line 2 in such cases.

It need not be a global variable to work.
I will do the necessary changes in the next version.

Ayush
_______________________________________________
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] 14+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v2] checkpatch: handle line break between commit and hash value
  2020-09-12  9:48 [Linux-kernel-mentees] [PATCH v2] checkpatch: handle line break between commit and hash value Ayush
                   ` (4 preceding siblings ...)
  2020-09-12 15:38 ` Ayush
@ 2020-09-12 19:37 ` Ayush
  2020-09-13 11:01   ` Lukas Bulwahn
  2020-09-13 12:06   ` Ayush
  5 siblings, 2 replies; 14+ messages in thread
From: Ayush @ 2020-09-12 19:37 UTC (permalink / raw)
  To: Lukas Bulwahn; +Cc: linux-kernel-mentees

September 12, 2020 9:09 PM, "Ayush" <ayush@disroot.org> wrote:

> I am re-evaluating my output and will give you a analysis of all commits
> which are getting affected by my patch.
> It will take time to run checkpatch.pl again on those commits, so I will
> share as soon as it's finished.

I fixed my script used for analysis and re-evaluated the affect of my patch.

I have made a list of commits which are fixed with this patch, It can be found here:

https://gist.githubusercontent.com/eldraco19/e7bf729b04cf6e2eee6ec52785b318d4/raw/74e13760fe01deee3285a1ec047ea0ac036cae76/commit_report.txt


Ayush
_______________________________________________
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] 14+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v2] checkpatch: handle line break between commit and hash value
  2020-09-12 19:37 ` Ayush
@ 2020-09-13 11:01   ` Lukas Bulwahn
  2020-09-13 12:06   ` Ayush
  1 sibling, 0 replies; 14+ messages in thread
From: Lukas Bulwahn @ 2020-09-13 11:01 UTC (permalink / raw)
  To: Ayush; +Cc: linux-kernel-mentees



On Sat, 12 Sep 2020, Ayush wrote:

> September 12, 2020 9:09 PM, "Ayush" <ayush@disroot.org> wrote:
> 
> > I am re-evaluating my output and will give you a analysis of all commits
> > which are getting affected by my patch.
> > It will take time to run checkpatch.pl again on those commits, so I will
> > share as soon as it's finished.
> 
> I fixed my script used for analysis and re-evaluated the affect of my patch.
> 
> I have made a list of commits which are fixed with this patch, It can be found here:
> 
> https://gist.githubusercontent.com/eldraco19/e7bf729b04cf6e2eee6ec52785b318d4/raw/74e13760fe01deee3285a1ec047ea0ac036cae76/commit_report.txt
> 
>

Did checkpatch.pl output on other types change?

One of the testers on this list reported that two COMMIT_LOG_LONG_LINE 
reports disappeared. That should not have happened, right?

I had a look at the checkpatch.pl output for commit 0a2bd55c194a ("dm 
integrity: document allow_discard option").

I think the reported line is wrong here.

Also, the line number for other cases have changed.

Generally, the line number should point to the line where the error really 
happens.

Lukas
_______________________________________________
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] 14+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v2] checkpatch: handle line break between commit and hash value
  2020-09-12 19:37 ` Ayush
  2020-09-13 11:01   ` Lukas Bulwahn
@ 2020-09-13 12:06   ` Ayush
  2020-09-13 18:09     ` Lukas Bulwahn
  2020-09-13 20:53     ` Ayush
  1 sibling, 2 replies; 14+ messages in thread
From: Ayush @ 2020-09-13 12:06 UTC (permalink / raw)
  To: Lukas Bulwahn; +Cc: linux-kernel-mentees

> Did checkpatch.pl output on other types change?
> 
> One of the testers on this list reported that two COMMIT_LOG_LONG_LINE
> reports disappeared. That should not have happened, right?
> 
> I had a look at the checkpatch.pl output for commit 0a2bd55c194a ("dm
> integrity: document allow_discard option").
> 
> I think the reported line is wrong here.

After the patch, It reports the following line:

#6: 
Add decription of the allow_discard option added in commit

Which I think is right as the author wanted to refer commit, which started from the word "commit"
in the reported line only.

Please give your opinion on this. 


> Also, the line number for other cases have changed.


Actually I am noticing a weird thing. When I run checkpacth.pl on multiple commits using v5.7..v5.8,
It is not showing correct output, like for some commits, I can see the command when run on multiple commits
did not report an error but when I ran on that individual commit, it's reporting an error.


- Running scripts/checkpatch.pl --show-types -g v5.7..v5.8, gives (for this particular commit):

-----------------------------------------------------------------------------------------------
Commit 3dbf1ee6abbb ("pinctrl: cherryview: Add missing spinlock usage in chv_gpio_irq_handler")
-----------------------------------------------------------------------------------------------
total: 0 errors, 0 warnings, 15 lines checked

Commit 3dbf1ee6abbb ("pinctrl: cherryview: Add missing spinlock usage in chv_gpio_irq_handler") has no obvious style problems and is ready for submission.



- Running scripts/checkpatch.pl --show-types -g 3dbf1ee6abbb, gives:


ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit cdca06e4e859 ("pinctrl: baytrail: Add missing spinlock usage in byt_gpio_irq_handler")'
#11: 
cdca06e4e859 ("pinctrl: baytrail: Add missing spinlock usage in

total: 1 errors, 0 warnings, 15 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

Commit 3dbf1ee6abbb ("pinctrl: cherryview: Add missing spinlock usage in chv_gpio_irq_handler") has style problems, please review.



This is the reason the analysis on a large no. of commits was incorrect.

I need to investigate this more and check if it's an issue on my side or on the checkpatch.pl side.

Also, I noticed one more issue with my patch, it doesn't handle cases like:

- commit
1234567890ab ("foo
bar")


Due to this, some new GIT_COMMIT_ID errors were also reported.

So, I will try to address this in the next version.

Thanks,
Ayush
_______________________________________________
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] 14+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v2] checkpatch: handle line break between commit and hash value
  2020-09-13 12:06   ` Ayush
@ 2020-09-13 18:09     ` Lukas Bulwahn
  2020-09-13 20:53     ` Ayush
  1 sibling, 0 replies; 14+ messages in thread
From: Lukas Bulwahn @ 2020-09-13 18:09 UTC (permalink / raw)
  To: Ayush; +Cc: linux-kernel-mentees



On Sun, 13 Sep 2020, Ayush wrote:

> > Did checkpatch.pl output on other types change?
> > 
> > One of the testers on this list reported that two COMMIT_LOG_LONG_LINE
> > reports disappeared. That should not have happened, right?
> > 
> > I had a look at the checkpatch.pl output for commit 0a2bd55c194a ("dm
> > integrity: document allow_discard option").
> > 
> > I think the reported line is wrong here.
> 
> After the patch, It reports the following line:
> 
> #6: 
> Add decription of the allow_discard option added in commit
> 
> Which I think is right as the author wanted to refer commit, which started from the word "commit"
> in the reported line only.
> 
> Please give your opinion on this. 
>

Well, I think the line number should point to the line where the error is 
and which needs to be fixed.

Can you point me to the error in line 6 that checkpatch.pl complains 
about?

Or is the error in line 7? :)

By that logic above, it should then point out line 7.

I have seen compiler and tool output always point to the line where the 
syntax error is, not to the line where the command might have started etc.
 
> 
> > Also, the line number for other cases have changed.
> 
> 
> Actually I am noticing a weird thing. When I run checkpacth.pl on multiple commits using v5.7..v5.8,
> It is not showing correct output, like for some commits, I can see the command when run on multiple commits
> did not report an error but when I ran on that individual commit, it's reporting an error.
> 
> 
> - Running scripts/checkpatch.pl --show-types -g v5.7..v5.8, gives (for this particular commit):
> 
> -----------------------------------------------------------------------------------------------
> Commit 3dbf1ee6abbb ("pinctrl: cherryview: Add missing spinlock usage in chv_gpio_irq_handler")
> -----------------------------------------------------------------------------------------------
> total: 0 errors, 0 warnings, 15 lines checked
> 
> Commit 3dbf1ee6abbb ("pinctrl: cherryview: Add missing spinlock usage in chv_gpio_irq_handler") has no obvious style problems and is ready for submission.
> 
> 
> 
> - Running scripts/checkpatch.pl --show-types -g 3dbf1ee6abbb, gives:
> 
> 
> ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit cdca06e4e859 ("pinctrl: baytrail: Add missing spinlock usage in byt_gpio_irq_handler")'
> #11: 
> cdca06e4e859 ("pinctrl: baytrail: Add missing spinlock usage in
> 
> total: 1 errors, 0 warnings, 15 lines checked
> 
> NOTE: For some of the reported defects, checkpatch may be able to
>       mechanically convert to the typical style using --fix or --fix-inplace.
> 
> Commit 3dbf1ee6abbb ("pinctrl: cherryview: Add missing spinlock usage in chv_gpio_irq_handler") has style problems, please review.
> 
> 
> 
> This is the reason the analysis on a large no. of commits was incorrect.
> 
> I need to investigate this more and check if it's an issue on my side or on the checkpatch.pl side.
> 

Yes, we should investigate that. If checkpatch.pl does not work properly 
with git ranges, we should fix that first. Otherwise, all our evaluations 
are broken...

> Also, I noticed one more issue with my patch, it doesn't handle cases like:
> 
> - commit
> 1234567890ab ("foo
> bar")
> 
> 
> Due to this, some new GIT_COMMIT_ID errors were also reported.
> 
> So, I will try to address this in the next version.
> 

Looking forward to your next version.

Lukas
_______________________________________________
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] 14+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v2] checkpatch: handle line break between commit and hash value
  2020-09-13 12:06   ` Ayush
  2020-09-13 18:09     ` Lukas Bulwahn
@ 2020-09-13 20:53     ` Ayush
  2020-09-14  5:12       ` Lukas Bulwahn
  2020-09-17 14:53       ` Ayush
  1 sibling, 2 replies; 14+ messages in thread
From: Ayush @ 2020-09-13 20:53 UTC (permalink / raw)
  To: Lukas Bulwahn; +Cc: linux-kernel-mentees

Sir,

> Yes, we should investigate that. If checkpatch.pl does not work properly
> with git ranges, we should fix that first. Otherwise, all our evaluations
> are broken...

I noticed one more issue (more likely issue with git):

If I run:

$ scripts/checkpatch.pl --show-types -g d31275a9dc0b..a02254f8a676

Output is: https://gist.githubusercontent.com/eldraco19/b979e752baa2c6fdc1776c8b65dfa21e/raw/39ad42113141929d93142d9a2962759b4b6b2e3b/checkpatch_issue.txt

which doesn't include some in-between commits like: 3dbf1ee6abbb, 69388e15f507 e.t.c.

Strangely it includes commit like 022467444515, which isn't even in the given range.

then with,

$ git log --no-merges --oneline 
.
.
a02254f8a676 dmaengine: ioat: Decreasing allocation chunk size 2M->512K
bd2bf302eef2 dmaengine: ioat: fixing chunk sizing macros dependency
2fea2906b5cb dmaengine: Fix misspelling of "Analog Devices"
b3cb14310eb4 dt-bindings: dma: renesas,usb-dmac: convert bindings to json-schema
cde9a96ee24f dt-bindings: dma: renesas,rcar-dmac: convert bindings to json-schema
fc6f5d0a4983 dmaengine: dw-edma: Decouple dw-edma-core.c from struct pci_dev
69388e15f507 pinctrl: cherryview: Add missing spinlock usage in chv_gpio_irq_handler
3dbf1ee6abbb pinctrl: cherryview: Add missing spinlock usage in chv_gpio_irq_handler
9f676e5313c1 gpio: mlxbf2: remove unused including <linux/version.h>
b392350ec3f2 ALSA: hda/hdmi: Add module option to disable audio component binding
4c2b54f73aba gpio: dwapb: Split out dwapb_get_irq() helper
c59042ed8965 gpio: dwapb: Drop of_match_ptr() & ACPI_PTR() calls
4f344e86c739 gpio: dwapb: Drop bogus BUG_ON()s
48ce80568346 gpio: dwapb: Switch to more usual pattern of RMW in dwapb_gpio_set_debounce()
d31275a9dc0b gpio: dwapb: Convert to use IRQ core provided macros
.
.
(trimmed output for specific range)

It shows all the commits in between.

But if I run:

$ git log --no-merges v5.7..v5.8 --oneline
.
.
a02254f8a676 dmaengine: ioat: Decreasing allocation chunk size 2M->512K
bd2bf302eef2 dmaengine: ioat: fixing chunk sizing macros dependency
2fea2906b5cb dmaengine: Fix misspelling of "Analog Devices"
b3cb14310eb4 dt-bindings: dma: renesas,usb-dmac: convert bindings to json-schema
cde9a96ee24f dt-bindings: dma: renesas,rcar-dmac: convert bindings to json-schema
fc6f5d0a4983 dmaengine: dw-edma: Decouple dw-edma-core.c from struct pci_dev
3dbf1ee6abbb pinctrl: cherryview: Add missing spinlock usage in chv_gpio_irq_handler
9f676e5313c1 gpio: mlxbf2: remove unused including <linux/version.h>
4c2b54f73aba gpio: dwapb: Split out dwapb_get_irq() helper
c59042ed8965 gpio: dwapb: Drop of_match_ptr() & ACPI_PTR() calls
4f344e86c739 gpio: dwapb: Drop bogus BUG_ON()s
48ce80568346 gpio: dwapb: Switch to more usual pattern of RMW in dwapb_gpio_set_debounce()
d31275a9dc0b gpio: dwapb: Convert to use IRQ core provided macros
.
.
(trimmed output for specific range)

It shows commit 3dbf1ee6abbb but not 69388e15f507.
I confirmed both commits are present in tree from:
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/log/?h=linux-5.8.y&ofs=18900


I think it is an issue with git.

Please give your opinion on this.

Thanks,
Ayush
_______________________________________________
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] 14+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v2] checkpatch: handle line break between commit and hash value
  2020-09-13 20:53     ` Ayush
@ 2020-09-14  5:12       ` Lukas Bulwahn
  2020-09-17 14:53       ` Ayush
  1 sibling, 0 replies; 14+ messages in thread
From: Lukas Bulwahn @ 2020-09-14  5:12 UTC (permalink / raw)
  To: Ayush; +Cc: linux-kernel-mentees



On Sun, 13 Sep 2020, Ayush wrote:

> Sir,
> 
> > Yes, we should investigate that. If checkpatch.pl does not work properly
> > with git ranges, we should fix that first. Otherwise, all our evaluations
> > are broken...
> 
> I noticed one more issue (more likely issue with git):
> 
> If I run:
> 
> $ scripts/checkpatch.pl --show-types -g d31275a9dc0b..a02254f8a676
> 
> Output is: https://gist.githubusercontent.com/eldraco19/b979e752baa2c6fdc1776c8b65dfa21e/raw/39ad42113141929d93142d9a2962759b4b6b2e3b/checkpatch_issue.txt
> 
> which doesn't include some in-between commits like: 3dbf1ee6abbb, 69388e15f507 e.t.c.
> 
> Strangely it includes commit like 022467444515, which isn't even in the given range.
> 
> then with,
> 
> $ git log --no-merges --oneline 
> .
> .
> a02254f8a676 dmaengine: ioat: Decreasing allocation chunk size 2M->512K
> bd2bf302eef2 dmaengine: ioat: fixing chunk sizing macros dependency
> 2fea2906b5cb dmaengine: Fix misspelling of "Analog Devices"
> b3cb14310eb4 dt-bindings: dma: renesas,usb-dmac: convert bindings to json-schema
> cde9a96ee24f dt-bindings: dma: renesas,rcar-dmac: convert bindings to json-schema
> fc6f5d0a4983 dmaengine: dw-edma: Decouple dw-edma-core.c from struct pci_dev
> 69388e15f507 pinctrl: cherryview: Add missing spinlock usage in chv_gpio_irq_handler
> 3dbf1ee6abbb pinctrl: cherryview: Add missing spinlock usage in chv_gpio_irq_handler
> 9f676e5313c1 gpio: mlxbf2: remove unused including <linux/version.h>
> b392350ec3f2 ALSA: hda/hdmi: Add module option to disable audio component binding
> 4c2b54f73aba gpio: dwapb: Split out dwapb_get_irq() helper
> c59042ed8965 gpio: dwapb: Drop of_match_ptr() & ACPI_PTR() calls
> 4f344e86c739 gpio: dwapb: Drop bogus BUG_ON()s
> 48ce80568346 gpio: dwapb: Switch to more usual pattern of RMW in dwapb_gpio_set_debounce()
> d31275a9dc0b gpio: dwapb: Convert to use IRQ core provided macros
> .
> .
> (trimmed output for specific range)
> 
> It shows all the commits in between.
> 
> But if I run:
> 
> $ git log --no-merges v5.7..v5.8 --oneline
> .
> .
> a02254f8a676 dmaengine: ioat: Decreasing allocation chunk size 2M->512K
> bd2bf302eef2 dmaengine: ioat: fixing chunk sizing macros dependency
> 2fea2906b5cb dmaengine: Fix misspelling of "Analog Devices"
> b3cb14310eb4 dt-bindings: dma: renesas,usb-dmac: convert bindings to json-schema
> cde9a96ee24f dt-bindings: dma: renesas,rcar-dmac: convert bindings to json-schema
> fc6f5d0a4983 dmaengine: dw-edma: Decouple dw-edma-core.c from struct pci_dev
> 3dbf1ee6abbb pinctrl: cherryview: Add missing spinlock usage in chv_gpio_irq_handler
> 9f676e5313c1 gpio: mlxbf2: remove unused including <linux/version.h>
> 4c2b54f73aba gpio: dwapb: Split out dwapb_get_irq() helper
> c59042ed8965 gpio: dwapb: Drop of_match_ptr() & ACPI_PTR() calls
> 4f344e86c739 gpio: dwapb: Drop bogus BUG_ON()s
> 48ce80568346 gpio: dwapb: Switch to more usual pattern of RMW in dwapb_gpio_set_debounce()
> d31275a9dc0b gpio: dwapb: Convert to use IRQ core provided macros
> .
> .
> (trimmed output for specific range)
> 
> It shows commit 3dbf1ee6abbb but not 69388e15f507.
> I confirmed both commits are present in tree from:
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/log/?h=linux-5.8.y&ofs=18900
> 
> 
> I think it is an issue with git.
> 
> Please give your opinion on this.
>

I cannot follow your description and explanation.

What is the problem you encountered? What did you expect? What did you 
observe? Which lines of code are involved? What git command is called?

Why do you expect a certain behaviour?

I would bet that you misunderstand git and git is not broken; so, convince 
us on the mailing list that I lose this bet.

You can do better explaining, then we might help you.

Lukas
_______________________________________________
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] 14+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v2] checkpatch: handle line break between commit and hash value
  2020-09-13 20:53     ` Ayush
  2020-09-14  5:12       ` Lukas Bulwahn
@ 2020-09-17 14:53       ` Ayush
  2020-09-17 15:17         ` Lukas Bulwahn
  1 sibling, 1 reply; 14+ messages in thread
From: Ayush @ 2020-09-17 14:53 UTC (permalink / raw)
  To: Lukas Bulwahn; +Cc: linux-kernel-mentees

Sir,

I am sorry for the late reply, my health condition was not good in last few days.

> What is the problem you encountered? What did you expect? What did you
> observe? Which lines of code are involved? What git command is called?
> 
> Why do you expect a certain behaviour?were

When the git log is (trimmed portion of git log --no-merges --oneline):

a02254f8a676 dmaengine: ioat: Decreasing allocation chunk size 2M->512K
bd2bf302eef2 dmaengine: ioat: fixing chunk sizing macros dependency
2fea2906b5cb dmaengine: Fix misspelling of "Analog Devices"
b3cb14310eb4 dt-bindings: dma: renesas,usb-dmac: convert bindings to json-schema
cde9a96ee24f dt-bindings: dma: renesas,rcar-dmac: convert bindings to json-schema
fc6f5d0a4983 dmaengine: dw-edma: Decouple dw-edma-core.c from struct pci_dev
69388e15f507 pinctrl: cherryview: Add missing spinlock usage in chv_gpio_irq_handler
3dbf1ee6abbb pinctrl: cherryview: Add missing spinlock usage in chv_gpio_irq_handler
9f676e5313c1 gpio: mlxbf2: remove unused including <linux/version.h>
b392350ec3f2 ALSA: hda/hdmi: Add module option to disable audio component binding
4c2b54f73aba gpio: dwapb: Split out dwapb_get_irq() helper
c59042ed8965 gpio: dwapb: Drop of_match_ptr() & ACPI_PTR() calls
4f344e86c739 gpio: dwapb: Drop bogus BUG_ON()s
48ce80568346 gpio: dwapb: Switch to more usual pattern of RMW in dwapb_gpio_set_debounce()
d31275a9dc0b gpio: dwapb: Convert to use IRQ core provided macros

then to check commits in this range, I will run:

$ scripts/checkpatch.pl --show-types -g d31275a9dc0b..a02254f8a676

The output is
https://gist.githubusercontent.com/eldraco19/b979e752baa2c6fdc1776c8b65dfa21e/raw/39ad42113141929d93
42d9a2962759b4b6b2e3b/checkpatch_issue.txt

here we can see:
1. Not all in-between commits are checked by checkpatch.pl
2. Some new commits (which are not in the given range) are also checked by checkpatch.pl

checkpatch internally used the following command for collecting the commit log:

$ git log --no-color --no-merges --pretty=format:'%H %s' $git_range

The result of this command is not what we expect checkpatch to test.

Please correct me if I am wrong.

Also I checked other evaluation too.
(https://lore.kernel.org/linux-kernel-mentees/e6fa87d7-3b80-390f-4db2-40e977a4b635@gmail.com/T/#m17c
7f43426bd44b16c4b7f7e59559ed2c8b69a0)

I am looking into that bug and will try to fix it in v3.

Regards
Ayush
_______________________________________________
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] 14+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v2] checkpatch: handle line break between commit and hash value
  2020-09-17 14:53       ` Ayush
@ 2020-09-17 15:17         ` Lukas Bulwahn
  0 siblings, 0 replies; 14+ messages in thread
From: Lukas Bulwahn @ 2020-09-17 15:17 UTC (permalink / raw)
  To: Ayush; +Cc: linux-kernel-mentees



On Thu, 17 Sep 2020, Ayush wrote:

> Sir,
> 
> I am sorry for the late reply, my health condition was not good in last few days.
> 
> > What is the problem you encountered? What did you expect? What did you
> > observe? Which lines of code are involved? What git command is called?
> > 
> > Why do you expect a certain behaviour?were
>

Very short: you do not understand git. Okay, let us go into detail:
 
> When the git log is (trimmed portion of git log --no-merges --oneline):
> 
> a02254f8a676 dmaengine: ioat: Decreasing allocation chunk size 2M->512K
> bd2bf302eef2 dmaengine: ioat: fixing chunk sizing macros dependency
> 2fea2906b5cb dmaengine: Fix misspelling of "Analog Devices"
> b3cb14310eb4 dt-bindings: dma: renesas,usb-dmac: convert bindings to json-schema
> cde9a96ee24f dt-bindings: dma: renesas,rcar-dmac: convert bindings to json-schema
> fc6f5d0a4983 dmaengine: dw-edma: Decouple dw-edma-core.c from struct pci_dev
> 69388e15f507 pinctrl: cherryview: Add missing spinlock usage in chv_gpio_irq_handler
> 3dbf1ee6abbb pinctrl: cherryview: Add missing spinlock usage in chv_gpio_irq_handler
> 9f676e5313c1 gpio: mlxbf2: remove unused including <linux/version.h>
> b392350ec3f2 ALSA: hda/hdmi: Add module option to disable audio component binding
> 4c2b54f73aba gpio: dwapb: Split out dwapb_get_irq() helper
> c59042ed8965 gpio: dwapb: Drop of_match_ptr() & ACPI_PTR() calls
> 4f344e86c739 gpio: dwapb: Drop bogus BUG_ON()s
> 48ce80568346 gpio: dwapb: Switch to more usual pattern of RMW in dwapb_gpio_set_debounce()
> d31275a9dc0b gpio: dwapb: Convert to use IRQ core provided macros
>

How did you generate this list? (Trimming a list is not a valid 
operation...)

If you run:

$ git log --no-merges --oneline d31275a9dc0b..a02254f8a676
a02254f8a676 dmaengine: ioat: Decreasing allocation chunk size 2M->512K
bd2bf302eef2 dmaengine: ioat: fixing chunk sizing macros dependency
2fea2906b5cb dmaengine: Fix misspelling of "Analog Devices"
b3cb14310eb4 dt-bindings: dma: renesas,usb-dmac: convert bindings to json-schema
cde9a96ee24f dt-bindings: dma: renesas,rcar-dmac: convert bindings to json-schema
fc6f5d0a4983 dmaengine: dw-edma: Decouple dw-edma-core.c from struct pci_dev
c2ce6bbcfc9f dmaengine: idxd: export hw version through sysfs
a4e688535a08 dmaengine: ti: k3-udma: Disable memcopy via MCU NAVSS on am654
022467444515 dmaengine: ti: k3-udma: Drop COMPILE_TEST for the drivers for now

that is the full list you get.

If you just copy a subset of git commits from a git log that is not its 
range. A git history is not linear, but only your console is. So something 
happens to make the history look linear, but do not judge a git history by 
its look, its beauty is in its inside.


> then to check commits in this range, I will run:
> 
> $ scripts/checkpatch.pl --show-types -g d31275a9dc0b..a02254f8a676
> 
> The output is
> https://gist.githubusercontent.com/eldraco19/b979e752baa2c6fdc1776c8b65dfa21e/raw/39ad42113141929d93
> 42d9a2962759b4b6b2e3b/checkpatch_issue.txt
> 
> here we can see:
> 1. Not all in-between commits are checked by checkpatch.pl
> 2. Some new commits (which are not in the given range) are also checked by checkpatch.pl
> 
> checkpatch internally used the following command for collecting the commit log:
> 
> $ git log --no-color --no-merges --pretty=format:'%H %s' $git_range
> 
> The result of this command is not what we expect checkpatch to test.
> 
> Please correct me if I am wrong.
>

Yes, you are wrong. Understand git and you will understand the mistake.

> Also I checked other evaluation too.
> (https://lore.kernel.org/linux-kernel-mentees/e6fa87d7-3b80-390f-4db2-40e977a4b635@gmail.com/T/#m17c
> 7f43426bd44b16c4b7f7e59559ed2c8b69a0)
> 
> I am looking into that bug and will try to fix it in v3.
>

Please do. Good luck :)

Lukas
_______________________________________________
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] 14+ messages in thread

end of thread, other threads:[~2020-09-17 15:17 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-12  9:48 [Linux-kernel-mentees] [PATCH v2] checkpatch: handle line break between commit and hash value Ayush
2020-09-12 11:17 ` Lukas Bulwahn
2020-09-12 12:42 ` Lukas Bulwahn
2020-09-12 13:34 ` Ayush
2020-09-12 14:15 ` Lukas Bulwahn
2020-09-12 15:38 ` Ayush
2020-09-12 19:37 ` Ayush
2020-09-13 11:01   ` Lukas Bulwahn
2020-09-13 12:06   ` Ayush
2020-09-13 18:09     ` Lukas Bulwahn
2020-09-13 20:53     ` Ayush
2020-09-14  5:12       ` Lukas Bulwahn
2020-09-17 14:53       ` Ayush
2020-09-17 15:17         ` 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).