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

[Lukas Bulwahn]

On Sun, Nov 8, 2020 at 3:31 PM Aditya <yashsri421 at gmail.com> wrote:
>
> 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:'?
>

This should first be another warning, that is the first patch.
Then, we can create some fix option for that warning. That is a second patch.

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

No, we might consider a different output for tool output.

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

We should warn at a longer line limit.

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

I wrote tags. I think tag lines should not be warned about long length
(or at least for a different length).

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

That is a reasonable checkpatch.pl rule and can deserve a fix option as well.

Lukas