Re: [PATCH] checkpatch: fix false positive for COMMIT_LOG_LONG_LINE with URLs

[Aditya]

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@xxxxxxxxx>
>> ---
>>  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@xxxxxxxxxxxxxxx>
> 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