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

[Lukas Bulwahn]

On Sun, 8 Nov 2020, Aditya wrote:

> On 8/11/20 12:13 am, Lukas Bulwahn wrote:
> > On Sa., 7. Nov. 2020 at 15:49, Aditya <yashsri421 at gmail.com> wrote:
> > 
> >> On 7/11/20 5:47 pm, Aditya Srivastava wrote:
> >>> Currently, when 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") we get 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.
> >>>
> >>> Provide a fix by consuming first 70 chars of the corresponding line,
> >>> then stop at the next whitespace, and substitute it with the consumed
> >>> chars and a newline char.
> >>>
> >>> Signed-off-by: Aditya Srivastava <yashsri421 at gmail.com>
> >>> ---
> >>>  scripts/checkpatch.pl | 7 +++++--
> >>>  1 file changed, 5 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> >>> index 5a1096a4e220..ee665208ef02 100755
> >>> --- a/scripts/checkpatch.pl
> >>> +++ b/scripts/checkpatch.pl
> >>> @@ -2881,8 +2881,11 @@ 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) {
> >>> +                             $fixed[$fixlinenr] =~
> >> s/(.{70}[^\s]*)\s+/$1\n/g;
> >>> +                     }
> >>>                       $commit_log_long_line = 1;
> >>>               }
> >>>
> >>>
> >>
> > 
> > Why do you not implement a proper word wrap function?
> > 
> 
> Actually, I had searched on the web regarding it before implementation.
> Here I have implemented text wrap only and regex is the best way I
> could find of implementing it. Also the same was advised at
> stackoverflow. I took help from the same.
> Link:
> https://stackoverflow.com/questions/956379/how-can-i-word-wrap-a-string-in-perl

And stackoverflow is not always a good reference.

> Apart from it, perl also provides a wrap function using module
> Text::Wrap, but I wasn't sure if I could use this module in my
> changes. So, I proceeded with regex.
>

You can look into that, understand it a bit and then just implement a word 
wrap from scratch as we would need it here in checkpatch.pl, it should not 
be that complicated. 
 
> > A single commit tells us very little if this rule in checkpatch.pl really
> > is a good rule or not. A proper evaluation of typical cases of long commit
> > lines would help much more to improve this rule.
> > 
> 
> I generated a list of commit_long_long_line warnings with the line for
> which they are occurring at:(v4.13..v5.8)
> https://github.com/AdityaSrivast/kernel-tasks/blob/master/Task6/summary.txt
> 
> As I could see, most of these messages are plain english. But there
> are few instances of links, hex, file locations, such as:
>

I see many false positive warnings; so most are cases where I would not 
break the line.

> 1)https://lore.kernel.org/lkml/1559767582-11081-1-git-send-email-info@metux.net/
> 

How about detecting all likely URLs and suggesting a proper Link: tag in 
front of them?

> 2)  /home6/tmricht/linux-next/tools/build/Makefile.build:96: recipe
> for target 'libbpf.o' failed
> 
> 3)	8b 35 7b fb e2 00	mov    0xe2fb7b(%rip),%esi # ffffffff82db9e64
> <nr_cpu_ids>
> 
> I think we can use a better regex like:
> s/(.{1,75}|\S{76,})(?:\s[^\S\r\n]*|\Z)/$1\n/g
> 
> I tested this regex with the above cases, in which
> 1) remains unaffected
> 2) gets wrapped at "..recipe for" (which is 69 chars in length)
> 3) gets wrapped at "# ffffffff82db9e64", which is 64 in length
> 
> I guess I can generate another file with how these lines will be
> changed, when used with fix option.
> What do you think?
>

I think 1, 2 and 3 should not be wrapped.

1) should be fixed by adding a Link: prefix.
2) should be warned to just drop the absolute path or so.
2) and 3) need to detected to be tool output and hence, we accept a longer 
line limit here.

I also see a few tags and long commit log warnings.

Does checkpatch.pl also warn on long tags? That should be excluded from 
that check.

Also, I noticed quite a few 'References: <git-sha> ("<git commit 
message>")'. I think there is no standard for this yet, but maybe that is 
worth a discussion to agree on something and then add documentation and a 
check for that.


Lukas