Messages in this thread | | | Subject | Re: [PATCH] checkpatch: fix false positive for COMMIT_LOG_LONG_LINE with URLs | From | Aditya <> | Date | Fri, 18 Dec 2020 01:05:31 +0530 |
| |
On 17/12/20 10:33 pm, Joe Perches wrote: > On Thu, 2020-12-17 at 19:12 +0530, Aditya Srivastava wrote: >> Currently checkpatch warns for long line in commit messages even for >> URL lines. >> >> An evaluation over v4.13..v5.8 showed that out of ~11000 warnings for >> this class, around 790 are due to the line containing link. >> >> E.g. running checkpatch on commit 3cde818cd02b ("ASoC: topology: >> Consolidate how dtexts and dvalues are freed") reports this warning: >> >> WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line) >> https://mailman.alsa-project.org/pipermail/alsa-devel/2019-January/144761.html >> >> Avoid giving users warning for character limit, instead suggest them to >> prefix the URLs with "Link:" >> >> Signed-off-by: Aditya Srivastava <yashsri421@gmail.com> >> --- >> scripts/checkpatch.pl | 10 ++++++++-- >> 1 file changed, 8 insertions(+), 2 deletions(-) >> >> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > [] >> @@ -3032,8 +3032,14 @@ sub process { >> $line =~ /^\s*(?:Fixes:|Link:|$signature_tags)/i || >> # A Fixes: or Link: line or signature tag 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 ($line =~ /(?:http|https|ftp):\/\//) { >> + WARN("COMMIT_LOG_LONG_LINE", >> + "Consider prefixing the URL with 'Link:'\n" . $herecurr); >> + } >> + else { >> + WARN("COMMIT_LOG_LONG_LINE", >> + "Possible unwrapped commit description (prefer a maximum 75 chars per line)\n" . $herecurr); >> + } > > NAK. > > Aditya, you've submitted several patches to checkpatch and > you should know better by now what coding style is necessary > for acceptance. > > } else { >
Sorry, I missed it. Will change it :)
> Make the URI/URL check follow the styles allowed by RFC 3986. > Look at the long_line check around line 3500 introduced by > commit 2e4bbbc550be336cbb3defc67430fc0700aa1426 > Author: Andreas Brauchli <a.brauchli@elementarea.net> > Date: Tue Feb 6 15:38:45 2018 -0800 > checkpatch: allow long lines containing URL > > Also likely the URI should not be allowed to exceed the line > maximum unless it's the first non-whitespace of the line and > not starting after some other word in the line. > > Lastly, this sets $commit_log_long_line even for lines that > are now nominally exempted from the long line check. >
Okay. I'll make these changes and send the modified patch.
> The number of nominal fixes you showed above is not correct. > > Retrospective testing of checkpatch using --git history > should be aware of changes to checkpatch. > > This should count only lines from 75 to 80 chars for the > commit range you tested and only for 75 to 100 for commits > after checkpatch changed its allowed long line maximum in > commit bdc48fa11e46f867ea4d75fa59ee87a7f48be144 > checkpatch/coding-style: deprecate 80-column warning >
Joe, I think this is probably true only for "WARNING:LONG_LINE". However, here I have analyzed the count for "WARNING:COMMIT_LOG_LONG_LINE".
I ran git log on these lines. Probably these lines were last changed at 2a076f40d8c9be95bee7bcf18436655e1140447f.
I think I can change the count with exact numbers.
What do you think?
Thanks Aditya
| |