[Linux-kernel-mentees] [PATCH] checkpatch: add fix option for COMMIT_LOG_LONG_LINE

[Linux-kernel-mentees] [PATCH] checkpatch: add fix option for COMMIT_LOG_LONG_LINE

[Aditya Srivastava]

Currently, if a line exceeds 75 characters in a commit message,
checkpatch.pl gives a warning corresponding to the same.

E.g., running checkpatch on commit a1af2afbd244 ("drm/nouveau/volt:Fix
for some cards having 0 maximum voltage") reports this warning:

WARNING: Possible unwrapped commit description (prefer a maximum 75
chars per line)

Some, mostly Fermi, vbioses appear to have zero max voltage. That causes Nouveau to not parse voltage entries, thus users not being able to set higher clocks.

Fix this warning by truncating the line after 75 chars, and push the
remaining content to the next line

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

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 5329927fc1c1..a0107ed257d1 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2881,8 +2881,20 @@ sub process {
 		      $line =~ /^\s*(?:Fixes:|Link:)/i ||
 					# A Fixes: or Link: line
 		      $commit_log_possible_stack_dump)) {
-			WARN("COMMIT_LOG_LONG_LINE",
-			     "Possible unwrapped commit description (prefer a maximum 75 chars per line)\n" . $herecurr);
+			if (WARN("COMMIT_LOG_LONG_LINE",
+			     	 "Possible unwrapped commit description (prefer a maximum 75 chars per line)\n" . $herecurr) &&
+			    $fix) {
+				fix_delete_line($fixlinenr, $rawline);
+				my $curr_line = $rawline;
+				my $fixedline;
+				while (length($curr_line) > 0) {
+					# trim current line for any unnecessary spaces
+					$curr_line = trim($curr_line);
+					$fixedline = substr($curr_line, 0, 75);
+					fix_insert_line($fixlinenr+1, $fixedline);
+					$curr_line =~ s/$fixedline//;
+				}
+			}
 			$commit_log_long_line = 1;
 		}
 
-- 
2.17.1

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH] checkpatch: add fix option for COMMIT_LOG_LONG_LINE

[Aditya]

On 31/10/20 6:55 pm, Aditya Srivastava wrote:
> Currently, if a line exceeds 75 characters in a commit message,
> checkpatch.pl gives a warning corresponding to the same.
> 
> E.g., running checkpatch on commit a1af2afbd244 ("drm/nouveau/volt:Fix
> for some cards having 0 maximum voltage") reports this warning:
> 
> WARNING: Possible unwrapped commit description (prefer a maximum 75
> chars per line)
> 
> Some, mostly Fermi, vbioses appear to have zero max voltage. That causes Nouveau to not parse voltage entries, thus users not being able to set higher clocks.
> 
> Fix this warning by truncating the line after 75 chars, and push the
> remaining content to the next line
> 
> Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
> ---
>  scripts/checkpatch.pl | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 5329927fc1c1..a0107ed257d1 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2881,8 +2881,20 @@ sub process {
>  		      $line =~ /^\s*(?:Fixes:|Link:)/i ||
>  					# A Fixes: or Link: line
>  		      $commit_log_possible_stack_dump)) {
> -			WARN("COMMIT_LOG_LONG_LINE",
> -			     "Possible unwrapped commit description (prefer a maximum 75 chars per line)\n" . $herecurr);
> +			if (WARN("COMMIT_LOG_LONG_LINE",
> +			     	 "Possible unwrapped commit description (prefer a maximum 75 chars per line)\n" . $herecurr) &&
> +			    $fix) {
> +				fix_delete_line($fixlinenr, $rawline);
> +				my $curr_line = $rawline;
> +				my $fixedline;
> +				while (length($curr_line) > 0) {
> +					# trim current line for any unnecessary spaces
> +					$curr_line = trim($curr_line);
> +					$fixedline = substr($curr_line, 0, 75);
> +					fix_insert_line($fixlinenr+1, $fixedline);
> +					$curr_line =~ s/$fixedline//;
> +				}
> +			}
>  			$commit_log_long_line = 1;
>  		}
>  
> 

Hi Sir
Seeking your feedback on this one

Thanks
Aditya
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH] checkpatch: add fix option for COMMIT_LOG_LONG_LINE

[Lukas Bulwahn]

On Sat, 31 Oct 2020, Aditya Srivastava wrote:

> Currently, if a line exceeds 75 characters in a commit message,
> checkpatch.pl gives a warning corresponding to the same.
> 
> E.g., running checkpatch on commit a1af2afbd244 ("drm/nouveau/volt:Fix
> for some cards having 0 maximum voltage") reports this warning:
> 
> WARNING: Possible unwrapped commit description (prefer a maximum 75
> chars per line)
> 
> Some, mostly Fermi, vbioses appear to have zero max voltage. That causes Nouveau to not parse voltage entries, thus users not being able to set higher clocks.
> 
> Fix this warning by truncating the line after 75 chars, and push the
> remaining content to the next line
> 
> Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>

This is wrong on multiple levels:

1. We must first ensure that the COMMIT_LOG_LONG_LINE are actually 
largely true positives, i.e., it really makes sense to break those lines. 
In my evaluations, I found many cases where it is clear that lines should 
not be broken because it is tool output etc.

2. You cannot just break at character 75. You would need to identify word 
boundaries and break there.

3. The proper fix is to identify the whole paragraph and then word-wrap 
the whole paragraph with a limit of 75. Just adding some content in a new 
line will make the commit message even more unreadable than before. So, 
that is not a fix.


So, I suggest to come up with a good strategy to really identify 
COMMIT_LOG_LONG_LINE that sensibly can be word-wrapped, i.e.,
distinguish between free-floating commit description and tool output,
and then implement that word wrap properly.

Or to prioritize another issue and possiblity for fix rules.

You can see that there are potentially some good options for fixes around 
the tags and sign-offs Dwaipayan has investigated.

I am pretty sure this patch is this state will not be accepted.

Lukas

> ---
>  scripts/checkpatch.pl | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 5329927fc1c1..a0107ed257d1 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2881,8 +2881,20 @@ sub process {
>  		      $line =~ /^\s*(?:Fixes:|Link:)/i ||
>  					# A Fixes: or Link: line
>  		      $commit_log_possible_stack_dump)) {
> -			WARN("COMMIT_LOG_LONG_LINE",
> -			     "Possible unwrapped commit description (prefer a maximum 75 chars per line)\n" . $herecurr);
> +			if (WARN("COMMIT_LOG_LONG_LINE",
> +			     	 "Possible unwrapped commit description (prefer a maximum 75 chars per line)\n" . $herecurr) &&
> +			    $fix) {
> +				fix_delete_line($fixlinenr, $rawline);
> +				my $curr_line = $rawline;
> +				my $fixedline;
> +				while (length($curr_line) > 0) {
> +					# trim current line for any unnecessary spaces
> +					$curr_line = trim($curr_line);
> +					$fixedline = substr($curr_line, 0, 75);
> +					fix_insert_line($fixlinenr+1, $fixedline);
> +					$curr_line =~ s/$fixedline//;
> +				}
> +			}
>  			$commit_log_long_line = 1;
>  		}
>  
> -- 
> 2.17.1
> 
> 
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH] checkpatch: add fix option for COMMIT_LOG_LONG_LINE

[Lukas Bulwahn]

On Sun, 1 Nov 2020, Aditya wrote:

> On 31/10/20 6:55 pm, Aditya Srivastava wrote:
> > Currently, if a line exceeds 75 characters in a commit message,
> > checkpatch.pl gives a warning corresponding to the same.
> > 
> > E.g., running checkpatch on commit a1af2afbd244 ("drm/nouveau/volt:Fix
> > for some cards having 0 maximum voltage") reports this warning:
> > 
> > WARNING: Possible unwrapped commit description (prefer a maximum 75
> > chars per line)
> > 
> > Some, mostly Fermi, vbioses appear to have zero max voltage. That causes Nouveau to not parse voltage entries, thus users not being able to set higher clocks.
> > 
> > Fix this warning by truncating the line after 75 chars, and push the
> > remaining content to the next line
> > 
> > Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
> > ---
> >  scripts/checkpatch.pl | 16 ++++++++++++++--
> >  1 file changed, 14 insertions(+), 2 deletions(-)
> > 
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index 5329927fc1c1..a0107ed257d1 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -2881,8 +2881,20 @@ sub process {
> >  		      $line =~ /^\s*(?:Fixes:|Link:)/i ||
> >  					# A Fixes: or Link: line
> >  		      $commit_log_possible_stack_dump)) {
> > -			WARN("COMMIT_LOG_LONG_LINE",
> > -			     "Possible unwrapped commit description (prefer a maximum 75 chars per line)\n" . $herecurr);
> > +			if (WARN("COMMIT_LOG_LONG_LINE",
> > +			     	 "Possible unwrapped commit description (prefer a maximum 75 chars per line)\n" . $herecurr) &&
> > +			    $fix) {
> > +				fix_delete_line($fixlinenr, $rawline);
> > +				my $curr_line = $rawline;
> > +				my $fixedline;
> > +				while (length($curr_line) > 0) {
> > +					# trim current line for any unnecessary spaces
> > +					$curr_line = trim($curr_line);
> > +					$fixedline = substr($curr_line, 0, 75);
> > +					fix_insert_line($fixlinenr+1, $fixedline);
> > +					$curr_line =~ s/$fixedline//;
> > +				}
> > +			}
> >  			$commit_log_long_line = 1;
> >  		}
> >  
> > 
> 
> Hi Sir
> Seeking your feedback on this one
>

Please respect that I do have a full-time job and a weekend where I am 
supposed to work less than from Mon to Fri.

I will get back to you after two or three working days; if not, then you 
can ping me. Any ping early will just make me annoyed.

Lukas
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH] checkpatch: add fix option for COMMIT_LOG_LONG_LINE

[Aditya]

On 2/11/20 12:55 pm, Lukas Bulwahn wrote:
> 
> 
> On Sat, 31 Oct 2020, Aditya Srivastava wrote:
> 
>> Currently, if a line exceeds 75 characters in a commit message,
>> checkpatch.pl gives a warning corresponding to the same.
>>
>> E.g., running checkpatch on commit a1af2afbd244 ("drm/nouveau/volt:Fix
>> for some cards having 0 maximum voltage") reports this warning:
>>
>> WARNING: Possible unwrapped commit description (prefer a maximum 75
>> chars per line)
>>
>> Some, mostly Fermi, vbioses appear to have zero max voltage. That causes Nouveau to not parse voltage entries, thus users not being able to set higher clocks.
>>
>> Fix this warning by truncating the line after 75 chars, and push the
>> remaining content to the next line
>>
>> Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
> 
> This is wrong on multiple levels:
> 
> 1. We must first ensure that the COMMIT_LOG_LONG_LINE are actually 
> largely true positives, i.e., it really makes sense to break those lines. 
> In my evaluations, I found many cases where it is clear that lines should 
> not be broken because it is tool output etc.
> 
> 2. You cannot just break at character 75. You would need to identify word 
> boundaries and break there.
> 
> 3. The proper fix is to identify the whole paragraph and then word-wrap 
> the whole paragraph with a limit of 75. Just adding some content in a new 
> line will make the commit message even more unreadable than before. So, 
> that is not a fix.
> 
> 
> So, I suggest to come up with a good strategy to really identify 
> COMMIT_LOG_LONG_LINE that sensibly can be word-wrapped, i.e.,
> distinguish between free-floating commit description and tool output,
> and then implement that word wrap properly.
> 
> Or to prioritize another issue and possiblity for fix rules.
> 
> You can see that there are potentially some good options for fixes around 
> the tags and sign-offs Dwaipayan has investigated.
> 

I agree. I'll take it up again after sometime.

Thanks
Aditya
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH] checkpatch: add fix option for COMMIT_LOG_LONG_LINE

[Aditya]

> On 2/11/20 12:55 pm, Lukas Bulwahn wrote:
>>
>> Or to prioritize another issue and possiblity for fix rules.
>>
>> You can see that there are potentially some good options for fixes around 
>> the tags and sign-offs Dwaipayan has investigated.
>>

Sir, I am working on BAD_SIGN_OFF, which occurs in multiple variants,
for eg.
1) WARNING:BAD_SIGN_OFF: Co-developed-by: should not be used to
attribute nominal patch author '"Zhang, Jun" <jun.zhang@intel.com>'

2) WARNING:BAD_SIGN_OFF: email address '"Matthew Wilcox (Oracle)"
<willy@infradead.org>' might be better as 'Matthew Wilcox (Oracle)
<willy@infradead.org>'

3) WARNING:BAD_SIGN_OFF: Duplicate signature
etc.

Should I answer all these variants in one patch?

Thanks
Aditya


_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH] checkpatch: add fix option for COMMIT_LOG_LONG_LINE

[Lukas Bulwahn]

On Tue, Nov 3, 2020 at 9:38 AM Aditya <yashsri421@gmail.com> wrote:
>
> > On 2/11/20 12:55 pm, Lukas Bulwahn wrote:
> >>
> >> Or to prioritize another issue and possiblity for fix rules.
> >>
> >> You can see that there are potentially some good options for fixes around
> >> the tags and sign-offs Dwaipayan has investigated.
> >>
>
> Sir, I am working on BAD_SIGN_OFF, which occurs in multiple variants,
> for eg.
> 1) WARNING:BAD_SIGN_OFF: Co-developed-by: should not be used to
> attribute nominal patch author '"Zhang, Jun" <jun.zhang@intel.com>'
>

I think the fix here might be to drop the Co-developed-by: line, right?

Can you confirm this fix on your evaluation?

> 2) WARNING:BAD_SIGN_OFF: email address '"Matthew Wilcox (Oracle)"
> <willy@infradead.org>' might be better as 'Matthew Wilcox (Oracle)
> <willy@infradead.org>'
>

I think Dwaipayan and Joe are discussing the proper handling of
quotes. I suggest to understand the resolution that Dwaipayan and Joe
are working out before thinking about a fix for this option. They
might decide that only a few specific cases should be warned about or
not at all in case of quote mismatches.

> 3) WARNING:BAD_SIGN_OFF: Duplicate signature
> etc.
>

I do not see the fix option for this case 3; the duplicate signatures
are mostly due to the integration Signed-off-by trail.

> Should I answer all these variants in one patch?
>
> Thanks
> Aditya
>
>
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH] checkpatch: add fix option for COMMIT_LOG_LONG_LINE

[Lukas Bulwahn]

On Tue, Nov 3, 2020 at 9:38 AM Aditya <yashsri421@gmail.com> wrote:
>
> > On 2/11/20 12:55 pm, Lukas Bulwahn wrote:
> >>
> >> Or to prioritize another issue and possiblity for fix rules.
> >>
> >> You can see that there are potentially some good options for fixes around
> >> the tags and sign-offs Dwaipayan has investigated.
> >>
>
> Sir, I am working on BAD_SIGN_OFF, which occurs in multiple variants,
> for eg.
> 1) WARNING:BAD_SIGN_OFF: Co-developed-by: should not be used to
> attribute nominal patch author '"Zhang, Jun" <jun.zhang@intel.com>'
>
> 2) WARNING:BAD_SIGN_OFF: email address '"Matthew Wilcox (Oracle)"
> <willy@infradead.org>' might be better as 'Matthew Wilcox (Oracle)
> <willy@infradead.org>'
>
> 3) WARNING:BAD_SIGN_OFF: Duplicate signature
> etc.
>
> Should I answer all these variants in one patch?
>

Also, please open an email with a new subject when the subject of the
discussion changes.

Lukas
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees