linux-kernel-mentees.lists.linuxfoundation.org archive mirror
 help / color / mirror / Atom feed
* [Linux-kernel-mentees] [PATCH] checkpatch: add fix for BAD_SIGN_OFF
@ 2020-11-08 13:41 Aditya Srivastava
  0 siblings, 0 replies; 13+ messages in thread
From: Aditya Srivastava @ 2020-11-08 13:41 UTC (permalink / raw)
  To: joe; +Cc: dwaipayanray1, linux-kernel-mentees, yashsri421

Currently, checkpatch warns us if the author of the commit signs-off
as co-developed-by.
E.g. for commit 6e88559470f5 ("Documentation: Add section about
CPU vulnerabilities for Spectre") we get:

WARNING: Co-developed-by: should not be used to attribute nominal
patch author 'Tim Chen <tim.c.chen@linux.intel.com>'
Co-developed-by: Tim Chen <tim.c.chen@linux.intel.com>

Provide a simple fix by removing the co-developed-by line from the
commit message

A quick evaluation on v4.13..v5.8 showed that this fix was getting
triggered 6 times.
A quick manual check found out that all fixes were correct in those
cases.

Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
---
 scripts/checkpatch.pl | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 58095d9d8f34..1030d4fc2abf 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2827,8 +2827,11 @@ sub process {
 # Check Co-developed-by: immediately followed by Signed-off-by: with same name and email
 			if ($sign_off =~ /^co-developed-by:$/i) {
 				if ($email eq $author) {
-					WARN("BAD_SIGN_OFF",
-					      "Co-developed-by: should not be used to attribute nominal patch author '$author'\n" . "$here\n" . $rawline);
+					if (WARN("BAD_SIGN_OFF",
+						 "Co-developed-by: should not be used to attribute nominal patch author '$author'\n" . "$here\n" . $rawline) &&
+					    $fix) {
+						fix_delete_line($fixlinenr, $rawline);
+					}
 				}
 				if (!defined $lines[$linenr]) {
 					WARN("BAD_SIGN_OFF",
-- 
2.17.1

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

* Re: [Linux-kernel-mentees] [PATCH] checkpatch: add fix for BAD_SIGN_OFF
  2020-11-08 19:57 ` Joe Perches
  2020-11-09  7:24   ` Aditya
@ 2020-12-04 10:06   ` Aditya
  1 sibling, 0 replies; 13+ messages in thread
From: Aditya @ 2020-12-04 10:06 UTC (permalink / raw)
  To: Joe Perches; +Cc: linux-kernel-mentees, linux-kernel, dwaipayanray1

On 9/11/20 1:27 am, Joe Perches wrote:
> On Sun, 2020-11-08 at 19:13 +0530, Aditya Srivastava wrote:
>> Currently, checkpatch warns us if the author of the commit signs-off
>> as co-developed-by.
> []
>> A quick manual check found out that all fixes were correct in those
>> cases.
> []
>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
>> @@ -2827,8 +2827,11 @@ sub process {
>>  # Check Co-developed-by: immediately followed by Signed-off-by: with same name and email
>>  			if ($sign_off =~ /^co-developed-by:$/i) {
>>  				if ($email eq $author) {
>> -					WARN("BAD_SIGN_OFF",
>> -					      "Co-developed-by: should not be used to attribute nominal patch author '$author'\n" . "$here\n" . $rawline);
>> +					if (WARN("BAD_SIGN_OFF",
>> +						 "Co-developed-by: should not be used to attribute nominal patch author '$author'\n" . "$here\n" . $rawline) &&
>> +					    $fix) {
>> +						fix_delete_line($fixlinenr, $rawline);
>> +					}
>>  				}
>>  				if (!defined $lines[$linenr]) {
>>  					WARN("BAD_SIGN_OFF",
> 
> Looks OK to me.
> 
> Another option might be to add a Signed-off-by: line derived from
> any "From:" line when:
> 
> 	if ($is_patch && $has_commit_log && $chk_signoff) {
> 		if ($signoff == 0) {
> 			ERROR("MISSING_SIGN_OFF",
> 			      "Missing Signed-off-by: line(s)\n");
> 
> etc...
> 

Hi Joe

You probably forgot to Ack this patch.

Thanks
Aditya
_______________________________________________
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] 13+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH] checkpatch: add fix for BAD_SIGN_OFF
  2020-11-08 19:57 ` Joe Perches
@ 2020-11-09  7:24   ` Aditya
  2020-12-04 10:06   ` Aditya
  1 sibling, 0 replies; 13+ messages in thread
From: Aditya @ 2020-11-09  7:24 UTC (permalink / raw)
  To: Joe Perches; +Cc: linux-kernel-mentees, linux-kernel, dwaipayanray1

On 9/11/20 1:27 am, Joe Perches wrote:
> On Sun, 2020-11-08 at 19:13 +0530, Aditya Srivastava wrote:
>> Currently, checkpatch warns us if the author of the commit signs-off
>> as co-developed-by.
> []
>> A quick manual check found out that all fixes were correct in those
>> cases.
> []
>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
>> @@ -2827,8 +2827,11 @@ sub process {
>>  # Check Co-developed-by: immediately followed by Signed-off-by: with same name and email
>>  			if ($sign_off =~ /^co-developed-by:$/i) {
>>  				if ($email eq $author) {
>> -					WARN("BAD_SIGN_OFF",
>> -					      "Co-developed-by: should not be used to attribute nominal patch author '$author'\n" . "$here\n" . $rawline);
>> +					if (WARN("BAD_SIGN_OFF",
>> +						 "Co-developed-by: should not be used to attribute nominal patch author '$author'\n" . "$here\n" . $rawline) &&
>> +					    $fix) {
>> +						fix_delete_line($fixlinenr, $rawline);
>> +					}
>>  				}
>>  				if (!defined $lines[$linenr]) {
>>  					WARN("BAD_SIGN_OFF",
> 
> Looks OK to me.
> 
> Another option might be to add a Signed-off-by: line derived from
> any "From:" line when:
> 
> 	if ($is_patch && $has_commit_log && $chk_signoff) {
> 		if ($signoff == 0) {
> 			ERROR("MISSING_SIGN_OFF",
> 			      "Missing Signed-off-by: line(s)\n");
> 
> etc...
> 

Yes, I just noticed. Thanks for the suggestion. Will do.

Thanks
Aditya
_______________________________________________
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] 13+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH] checkpatch: add fix for BAD_SIGN_OFF
  2020-11-08 13:43 Aditya Srivastava
@ 2020-11-08 19:57 ` Joe Perches
  2020-11-09  7:24   ` Aditya
  2020-12-04 10:06   ` Aditya
  0 siblings, 2 replies; 13+ messages in thread
From: Joe Perches @ 2020-11-08 19:57 UTC (permalink / raw)
  To: Aditya Srivastava; +Cc: linux-kernel-mentees, linux-kernel, dwaipayanray1

On Sun, 2020-11-08 at 19:13 +0530, Aditya Srivastava wrote:
> Currently, checkpatch warns us if the author of the commit signs-off
> as co-developed-by.
[]
> A quick manual check found out that all fixes were correct in those
> cases.
[]
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -2827,8 +2827,11 @@ sub process {
>  # Check Co-developed-by: immediately followed by Signed-off-by: with same name and email
>  			if ($sign_off =~ /^co-developed-by:$/i) {
>  				if ($email eq $author) {
> -					WARN("BAD_SIGN_OFF",
> -					      "Co-developed-by: should not be used to attribute nominal patch author '$author'\n" . "$here\n" . $rawline);
> +					if (WARN("BAD_SIGN_OFF",
> +						 "Co-developed-by: should not be used to attribute nominal patch author '$author'\n" . "$here\n" . $rawline) &&
> +					    $fix) {
> +						fix_delete_line($fixlinenr, $rawline);
> +					}
>  				}
>  				if (!defined $lines[$linenr]) {
>  					WARN("BAD_SIGN_OFF",

Looks OK to me.

Another option might be to add a Signed-off-by: line derived from
any "From:" line when:

	if ($is_patch && $has_commit_log && $chk_signoff) {
		if ($signoff == 0) {
			ERROR("MISSING_SIGN_OFF",
			      "Missing Signed-off-by: line(s)\n");

etc...

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

* [Linux-kernel-mentees] [PATCH] checkpatch: add fix for BAD_SIGN_OFF
@ 2020-11-08 13:43 Aditya Srivastava
  2020-11-08 19:57 ` Joe Perches
  0 siblings, 1 reply; 13+ messages in thread
From: Aditya Srivastava @ 2020-11-08 13:43 UTC (permalink / raw)
  To: joe; +Cc: dwaipayanray1, linux-kernel-mentees, linux-kernel, yashsri421

Currently, checkpatch warns us if the author of the commit signs-off
as co-developed-by.
E.g. for commit 6e88559470f5 ("Documentation: Add section about
CPU vulnerabilities for Spectre") we get:

WARNING: Co-developed-by: should not be used to attribute nominal
patch author 'Tim Chen <tim.c.chen@linux.intel.com>'
Co-developed-by: Tim Chen <tim.c.chen@linux.intel.com>

Provide a simple fix by removing the co-developed-by line from the
commit message

A quick evaluation on v4.13..v5.8 showed that this fix was getting
triggered 6 times.
A quick manual check found out that all fixes were correct in those
cases.

Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
---
 scripts/checkpatch.pl | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 58095d9d8f34..1030d4fc2abf 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2827,8 +2827,11 @@ sub process {
 # Check Co-developed-by: immediately followed by Signed-off-by: with same name and email
 			if ($sign_off =~ /^co-developed-by:$/i) {
 				if ($email eq $author) {
-					WARN("BAD_SIGN_OFF",
-					      "Co-developed-by: should not be used to attribute nominal patch author '$author'\n" . "$here\n" . $rawline);
+					if (WARN("BAD_SIGN_OFF",
+						 "Co-developed-by: should not be used to attribute nominal patch author '$author'\n" . "$here\n" . $rawline) &&
+					    $fix) {
+						fix_delete_line($fixlinenr, $rawline);
+					}
 				}
 				if (!defined $lines[$linenr]) {
 					WARN("BAD_SIGN_OFF",
-- 
2.17.1

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

* Re: [Linux-kernel-mentees] [PATCH] checkpatch: add fix for BAD_SIGN_OFF
  2020-11-08  9:19           ` Aditya
@ 2020-11-08 10:42             ` Lukas Bulwahn
  0 siblings, 0 replies; 13+ messages in thread
From: Lukas Bulwahn @ 2020-11-08 10:42 UTC (permalink / raw)
  To: Aditya; +Cc: linux-kernel-mentees


[-- Attachment #1.1: Type: text/plain, Size: 1614 bytes --]

On So., 8. Nov. 2020 at 10:19, Aditya <yashsri421@gmail.com> wrote:

> On 8/11/20 12:17 pm, Lukas Bulwahn wrote:
> >
> >
> > On Sun, 8 Nov 2020, Aditya wrote:
> >
> >> On 8/11/20 12:08 am, Lukas Bulwahn wrote:
> >>>
> >>> Can you share the six commits?
> >>>
> >>> Then we check if the fix is really the right fix of if something
> different
> >>> is wrong with these commits.
> >>>
> >>
> >>
> >> These are the commits where this warning for BAD_SIGN_OFF was getting
> >> triggered(over 4.13..5.8):
> >> 1) Commit 1d1f898df658 ("rcu: Do RCU GP kthread self-wakeup from
> >> softirq and interrupt")
> >> 2) Commit 6e88559470f5 ("Documentation: Add section about CPU
> >> vulnerabilities for Spectre")
> >> 3) Commit a35d16905efc ("rcu: Add basic support for kfree_rcu()
> batching")
> >> 4) Commit b7e4aadef28f ("locking/spinlocks: Document the semantics of
> >> spin_is_locked()")
> >> 5) Commit 621df431b0ac ("Documentation/memory-barriers.txt:
> >> Cross-reference "tools/memory-model/"")
> >> 6) Commit 1c27b644c0fd ("Automate memory-barriers.txt; provide
> >> Linux-kernel memory model")
> >>
> >
> > I checked all six cases and think the proposed fix is always okay.
> >
> > I guess there is still the warning that Co-developed-by: should
> > immediately be followed up the Signed-off-by:, right?
> >
> > A possible fix for that would be to check if the needed Signed-off-by:
> > follows somewhere and then just move that to the appropriate place in
> the
> > patch.
> >
>
> Should I add this in the current patch itself or create another patch
> for it?
>

Make that further fix feature a new patch.

Lukas

[-- Attachment #1.2: Type: text/html, Size: 2350 bytes --]

[-- Attachment #2: Type: text/plain, Size: 201 bytes --]

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

* Re: [Linux-kernel-mentees] [PATCH] checkpatch: add fix for BAD_SIGN_OFF
  2020-11-08  6:47         ` Lukas Bulwahn
@ 2020-11-08  9:19           ` Aditya
  2020-11-08 10:42             ` Lukas Bulwahn
  0 siblings, 1 reply; 13+ messages in thread
From: Aditya @ 2020-11-08  9:19 UTC (permalink / raw)
  To: Lukas Bulwahn; +Cc: linux-kernel-mentees

On 8/11/20 12:17 pm, Lukas Bulwahn wrote:
> 
> 
> On Sun, 8 Nov 2020, Aditya wrote:
> 
>> On 8/11/20 12:08 am, Lukas Bulwahn wrote:
>>>
>>> Can you share the six commits?
>>>
>>> Then we check if the fix is really the right fix of if something different
>>> is wrong with these commits.
>>>
>>
>>
>> These are the commits where this warning for BAD_SIGN_OFF was getting
>> triggered(over 4.13..5.8):
>> 1) Commit 1d1f898df658 ("rcu: Do RCU GP kthread self-wakeup from
>> softirq and interrupt")
>> 2) Commit 6e88559470f5 ("Documentation: Add section about CPU
>> vulnerabilities for Spectre")
>> 3) Commit a35d16905efc ("rcu: Add basic support for kfree_rcu() batching")
>> 4) Commit b7e4aadef28f ("locking/spinlocks: Document the semantics of
>> spin_is_locked()")
>> 5) Commit 621df431b0ac ("Documentation/memory-barriers.txt:
>> Cross-reference "tools/memory-model/"")
>> 6) Commit 1c27b644c0fd ("Automate memory-barriers.txt; provide
>> Linux-kernel memory model")
>>
> 
> I checked all six cases and think the proposed fix is always okay.
> 
> I guess there is still the warning that Co-developed-by: should 
> immediately be followed up the Signed-off-by:, right?
> 
> A possible fix for that would be to check if the needed Signed-off-by: 
> follows somewhere and then just move that to the appropriate place in the 
> patch.
> 

Should I add this in the current patch itself or create another patch
for it?

> When your patch includes this information on how many occurrences, you 
> found and that all of the fixes are correct in those cases, this patch is 
> ready for lkml and Joe Perches.
> 
> Joe will possibly ask to name the six commits, so he can check himself. 
> But no need to add them in the commit message or so.
> 
Okay.

Thanks
Aditya


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

* Re: [Linux-kernel-mentees] [PATCH] checkpatch: add fix for BAD_SIGN_OFF
  2020-11-07 18:55       ` Aditya
@ 2020-11-08  6:47         ` Lukas Bulwahn
  2020-11-08  9:19           ` Aditya
  0 siblings, 1 reply; 13+ messages in thread
From: Lukas Bulwahn @ 2020-11-08  6:47 UTC (permalink / raw)
  To: Aditya; +Cc: linux-kernel-mentees



On Sun, 8 Nov 2020, Aditya wrote:

> On 8/11/20 12:08 am, Lukas Bulwahn wrote:
> > 
> > Can you share the six commits?
> > 
> > Then we check if the fix is really the right fix of if something different
> > is wrong with these commits.
> > 
> 
> 
> These are the commits where this warning for BAD_SIGN_OFF was getting
> triggered(over 4.13..5.8):
> 1) Commit 1d1f898df658 ("rcu: Do RCU GP kthread self-wakeup from
> softirq and interrupt")
> 2) Commit 6e88559470f5 ("Documentation: Add section about CPU
> vulnerabilities for Spectre")
> 3) Commit a35d16905efc ("rcu: Add basic support for kfree_rcu() batching")
> 4) Commit b7e4aadef28f ("locking/spinlocks: Document the semantics of
> spin_is_locked()")
> 5) Commit 621df431b0ac ("Documentation/memory-barriers.txt:
> Cross-reference "tools/memory-model/"")
> 6) Commit 1c27b644c0fd ("Automate memory-barriers.txt; provide
> Linux-kernel memory model")
>

I checked all six cases and think the proposed fix is always okay.

I guess there is still the warning that Co-developed-by: should 
immediately be followed up the Signed-off-by:, right?

A possible fix for that would be to check if the needed Signed-off-by: 
follows somewhere and then just move that to the appropriate place in the 
patch.

When your patch includes this information on how many occurrences, you 
found and that all of the fixes are correct in those cases, this patch is 
ready for lkml and Joe Perches.

Joe will possibly ask to name the six commits, so he can check himself. 
But no need to add them in the commit message or so.

Lukas

> > This certainly needs to be mentioned in the commit message.
> >
> Okay.
> 
> 
> Thanks
> Aditya
> 
> >
> > 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] 13+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH] checkpatch: add fix for BAD_SIGN_OFF
  2020-11-07 18:38     ` Lukas Bulwahn
@ 2020-11-07 18:55       ` Aditya
  2020-11-08  6:47         ` Lukas Bulwahn
  0 siblings, 1 reply; 13+ messages in thread
From: Aditya @ 2020-11-07 18:55 UTC (permalink / raw)
  To: Lukas Bulwahn; +Cc: linux-kernel-mentees

On 8/11/20 12:08 am, Lukas Bulwahn wrote:
> 
> Can you share the six commits?
> 
> Then we check if the fix is really the right fix of if something different
> is wrong with these commits.
> 


These are the commits where this warning for BAD_SIGN_OFF was getting
triggered(over 4.13..5.8):
1) Commit 1d1f898df658 ("rcu: Do RCU GP kthread self-wakeup from
softirq and interrupt")
2) Commit 6e88559470f5 ("Documentation: Add section about CPU
vulnerabilities for Spectre")
3) Commit a35d16905efc ("rcu: Add basic support for kfree_rcu() batching")
4) Commit b7e4aadef28f ("locking/spinlocks: Document the semantics of
spin_is_locked()")
5) Commit 621df431b0ac ("Documentation/memory-barriers.txt:
Cross-reference "tools/memory-model/"")
6) Commit 1c27b644c0fd ("Automate memory-barriers.txt; provide
Linux-kernel memory model")

> This certainly needs to be mentioned in the commit message.
>
Okay.


Thanks
Aditya

>
> 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] 13+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH] checkpatch: add fix for BAD_SIGN_OFF
  2020-11-07  9:27   ` Aditya
@ 2020-11-07 18:38     ` Lukas Bulwahn
  2020-11-07 18:55       ` Aditya
  0 siblings, 1 reply; 13+ messages in thread
From: Lukas Bulwahn @ 2020-11-07 18:38 UTC (permalink / raw)
  To: Aditya; +Cc: linux-kernel-mentees


[-- Attachment #1.1: Type: text/plain, Size: 1908 bytes --]

On Sa., 7. Nov. 2020 at 10:27, Aditya <yashsri421@gmail.com> wrote:

> On 4/11/20 4:44 pm, Lukas Bulwahn wrote:
> > On Tue, Nov 3, 2020 at 4:19 PM Aditya Srivastava <yashsri421@gmail.com>
> wrote:
> >>
> >> Currently, checkpatch warns us if the author of the commit signs-off
> >> as co-developed-by.
> >> E.g. for commit 6e88559470f5 ("Documentation: Add section about
> >> CPU vulnerabilities for Spectre") we get:
> >>
> >> WARNING: Co-developed-by: should not be used to attribute nominal
> >> patch author 'Tim Chen <tim.c.chen@linux.intel.com>'
> >> Co-developed-by: Tim Chen <tim.c.chen@linux.intel.com>
> >>
> >> Provide a simple fix by removing the co-developed-by line from the
> >> commit message
> >>
> >
> > Patch looks good. Maybe you can add a statistics on how many cases of
> > the last 200,000 commits this rule would actually trigger?
> >
> > E.g., you can probably quickly filter all commits that have at least
> > one Co-developed-by tag among those last 200,000 commits (that is
> > already only a few thousands), then take that list and just check with
> > checkpatch.pl for this one rule quickly to see how often the rule
> > would even trigger at all.
> >
> > Once you have that and explain that in the commit message, I think the
> > patch is ready for lkml and Joe Perches.
> >
> > Lukas
> >
>
> Hi Sir
> I generated a report for around 200,000 commits (v4.13..v5.8), and
> found that this rule was being triggered only 6 times. Statistically
> this doesn't seem much. Is it still advisable to mention it in the
> commit message?
> Although, there's a possibility that user fixed this issue after
> receiving the warning manually before committing.
>

Can you share the six commits?

Then we check if the fix is really the right fix of if something different
is wrong with these commits.

This certainly needs to be mentioned in the commit message.


Lukas


> Thanks
> Aditya
>
>

[-- Attachment #1.2: Type: text/html, Size: 3060 bytes --]

[-- Attachment #2: Type: text/plain, Size: 201 bytes --]

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

* Re: [Linux-kernel-mentees] [PATCH] checkpatch: add fix for BAD_SIGN_OFF
  2020-11-04 11:14 ` Lukas Bulwahn
@ 2020-11-07  9:27   ` Aditya
  2020-11-07 18:38     ` Lukas Bulwahn
  0 siblings, 1 reply; 13+ messages in thread
From: Aditya @ 2020-11-07  9:27 UTC (permalink / raw)
  To: Lukas Bulwahn; +Cc: linux-kernel-mentees

On 4/11/20 4:44 pm, Lukas Bulwahn wrote:
> On Tue, Nov 3, 2020 at 4:19 PM Aditya Srivastava <yashsri421@gmail.com> wrote:
>>
>> Currently, checkpatch warns us if the author of the commit signs-off
>> as co-developed-by.
>> E.g. for commit 6e88559470f5 ("Documentation: Add section about
>> CPU vulnerabilities for Spectre") we get:
>>
>> WARNING: Co-developed-by: should not be used to attribute nominal
>> patch author 'Tim Chen <tim.c.chen@linux.intel.com>'
>> Co-developed-by: Tim Chen <tim.c.chen@linux.intel.com>
>>
>> Provide a simple fix by removing the co-developed-by line from the
>> commit message
>>
> 
> Patch looks good. Maybe you can add a statistics on how many cases of
> the last 200,000 commits this rule would actually trigger?
> 
> E.g., you can probably quickly filter all commits that have at least
> one Co-developed-by tag among those last 200,000 commits (that is
> already only a few thousands), then take that list and just check with
> checkpatch.pl for this one rule quickly to see how often the rule
> would even trigger at all.
> 
> Once you have that and explain that in the commit message, I think the
> patch is ready for lkml and Joe Perches.
> 
> Lukas
> 

Hi Sir
I generated a report for around 200,000 commits (v4.13..v5.8), and
found that this rule was being triggered only 6 times. Statistically
this doesn't seem much. Is it still advisable to mention it in the
commit message?
Although, there's a possibility that user fixed this issue after
receiving the warning manually before committing.

Thanks
Aditya

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

* Re: [Linux-kernel-mentees] [PATCH] checkpatch: add fix for BAD_SIGN_OFF
  2020-11-03 15:18 Aditya Srivastava
@ 2020-11-04 11:14 ` Lukas Bulwahn
  2020-11-07  9:27   ` Aditya
  0 siblings, 1 reply; 13+ messages in thread
From: Lukas Bulwahn @ 2020-11-04 11:14 UTC (permalink / raw)
  To: Aditya Srivastava; +Cc: linux-kernel-mentees

On Tue, Nov 3, 2020 at 4:19 PM Aditya Srivastava <yashsri421@gmail.com> wrote:
>
> Currently, checkpatch warns us if the author of the commit signs-off
> as co-developed-by.
> E.g. for commit 6e88559470f5 ("Documentation: Add section about
> CPU vulnerabilities for Spectre") we get:
>
> WARNING: Co-developed-by: should not be used to attribute nominal
> patch author 'Tim Chen <tim.c.chen@linux.intel.com>'
> Co-developed-by: Tim Chen <tim.c.chen@linux.intel.com>
>
> Provide a simple fix by removing the co-developed-by line from the
> commit message
>

Patch looks good. Maybe you can add a statistics on how many cases of
the last 200,000 commits this rule would actually trigger?

E.g., you can probably quickly filter all commits that have at least
one Co-developed-by tag among those last 200,000 commits (that is
already only a few thousands), then take that list and just check with
checkpatch.pl for this one rule quickly to see how often the rule
would even trigger at all.

Once you have that and explain that in the commit message, I think the
patch is ready for lkml and Joe Perches.

Lukas

> Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
> ---
>  scripts/checkpatch.pl | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 58095d9d8f34..1030d4fc2abf 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2827,8 +2827,11 @@ sub process {
>  # Check Co-developed-by: immediately followed by Signed-off-by: with same name and email
>                         if ($sign_off =~ /^co-developed-by:$/i) {
>                                 if ($email eq $author) {
> -                                       WARN("BAD_SIGN_OFF",
> -                                             "Co-developed-by: should not be used to attribute nominal patch author '$author'\n" . "$here\n" . $rawline);
> +                                       if (WARN("BAD_SIGN_OFF",
> +                                                "Co-developed-by: should not be used to attribute nominal patch author '$author'\n" . "$here\n" . $rawline) &&
> +                                           $fix) {
> +                                               fix_delete_line($fixlinenr, $rawline);
> +                                       }
>                                 }
>                                 if (!defined $lines[$linenr]) {
>                                         WARN("BAD_SIGN_OFF",
> --
> 2.17.1
>
_______________________________________________
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] 13+ messages in thread

* [Linux-kernel-mentees] [PATCH] checkpatch: add fix for BAD_SIGN_OFF
@ 2020-11-03 15:18 Aditya Srivastava
  2020-11-04 11:14 ` Lukas Bulwahn
  0 siblings, 1 reply; 13+ messages in thread
From: Aditya Srivastava @ 2020-11-03 15:18 UTC (permalink / raw)
  To: lukas.bulwahn; +Cc: linux-kernel-mentees, yashsri421

Currently, checkpatch warns us if the author of the commit signs-off
as co-developed-by.
E.g. for commit 6e88559470f5 ("Documentation: Add section about
CPU vulnerabilities for Spectre") we get:

WARNING: Co-developed-by: should not be used to attribute nominal
patch author 'Tim Chen <tim.c.chen@linux.intel.com>'
Co-developed-by: Tim Chen <tim.c.chen@linux.intel.com>

Provide a simple fix by removing the co-developed-by line from the
commit message

Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
---
 scripts/checkpatch.pl | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 58095d9d8f34..1030d4fc2abf 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2827,8 +2827,11 @@ sub process {
 # Check Co-developed-by: immediately followed by Signed-off-by: with same name and email
 			if ($sign_off =~ /^co-developed-by:$/i) {
 				if ($email eq $author) {
-					WARN("BAD_SIGN_OFF",
-					      "Co-developed-by: should not be used to attribute nominal patch author '$author'\n" . "$here\n" . $rawline);
+					if (WARN("BAD_SIGN_OFF",
+						 "Co-developed-by: should not be used to attribute nominal patch author '$author'\n" . "$here\n" . $rawline) &&
+					    $fix) {
+						fix_delete_line($fixlinenr, $rawline);
+					}
 				}
 				if (!defined $lines[$linenr]) {
 					WARN("BAD_SIGN_OFF",
-- 
2.17.1

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

end of thread, other threads:[~2020-12-04 10:06 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-08 13:41 [Linux-kernel-mentees] [PATCH] checkpatch: add fix for BAD_SIGN_OFF Aditya Srivastava
  -- strict thread matches above, loose matches on Subject: below --
2020-11-08 13:43 Aditya Srivastava
2020-11-08 19:57 ` Joe Perches
2020-11-09  7:24   ` Aditya
2020-12-04 10:06   ` Aditya
2020-11-03 15:18 Aditya Srivastava
2020-11-04 11:14 ` Lukas Bulwahn
2020-11-07  9:27   ` Aditya
2020-11-07 18:38     ` Lukas Bulwahn
2020-11-07 18:55       ` Aditya
2020-11-08  6:47         ` Lukas Bulwahn
2020-11-08  9:19           ` Aditya
2020-11-08 10:42             ` 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).