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

[Aditya]

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
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.

> 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:

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

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?

Thanks
Aditya