[Linux-kernel-mentees] [PATCH v2] checkpatch: add fix option for COMMIT_LOG_LONG_LINE
yashsri421 at gmail.com
Sun Nov 8 14:31:36 UTC 2020
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.
> 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)
>> 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.
> 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
>> I think we can use a better regex like:
>> 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.
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")'
WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description
(prefer a maximum 75 chars per line)
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
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.
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"
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.
More information about the Linux-kernel-mentees