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

[Aditya]

On 8/11/20 12:52 pm, Lukas Bulwahn wrote:
> 
> 
> On Sun, 8 Nov 2020, Aditya wrote:
>>>
>>> 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. 
>  
Okay.

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

Should we add another warning description for this one? Or while
fixing, just prefix it with 'Link:'?

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

Okay.

> 2) should be warned to just drop the absolute path or so.

We are already warning for the absolute path for this case. All these
warnings are emitted for:
Commit 1ce6a9fc1549 ("bpf: fix build error in libbpf with
EXTRA_CFLAGS="-Wp, -D_FORTIFY_SOURCE=2 -O2"")

I guess for (2), we can check if a line is reported by
USE_RELATIVE_PATH, we can omit it for long line warning

> 2) and 3) need to detected to be tool output and hence, we accept a longer 
> line limit here.
> 
So, we should wrap these lines after more characters? Or can we treat
them as false positives for this warning and just avoid checking such
lines for this class of warning?

> 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.
> 
Yes, checkpatch warns currently if we use long commit logs. Eg for
commit '47987970175 ("ASoC: Intel: create haswell folder and move
haswell platform files in")'

we get:
WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description
(prefer a maximum 75 chars per line)
#13:
commit ba57f68235cf ("ASoC: Intel: create haswell folder and move
haswell platform files in")

But probably it will be best if we can wrap these line. I feel so, as
users tend to use these along the common english lines, and not always
in a different line, such as:

- MC-aware mode was introduced to mlxsw in commit 7b8195306694
("mlxsw: spectrum:

for the commit f9d5b1d50840.

Also I feel that wrapping it might not affect readability as well. For
eg, I have wrapped the commit messages for my patches as well.

E.g., at,
https://lore.kernel.org/lkml/20201030114447.24199-1-yashsri421@gmail.com/
I wrapped the 'E.g.' line which describes that I ran checkpatch.pl,
and contains the commit message.

What do you think?

> 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.
> 
Thanks for pointing it out. I observed all the lines prefixed by
'References' and found out that they are either followed by link or
commit log, except once ie
"I/O hangs after resuming from suspend-to-ram"
(https://marc.info/?l=linux-block&m=150340235201348).
Here user is probably quoting his line and referencing it to the link
where he might have seen it.

I guess we can either avoid the line prefixed with 'References:'
completely or selectively avoid links and treat commit logs similar to
whatever we decide for above.

Thanks
Aditya