linux-kernel - Re: [PATCH] checkpatch: fix false positive for COMMIT_LOG_LONG

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 18 Dec 2020 01:05:31 +0530
From:   Aditya <yashsri421@...il.com>
To:     Joe Perches <joe@...ches.com>
Cc:     lukas.bulwahn@...il.com,
        linux-kernel-mentees@...ts.linuxfoundation.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] checkpatch: fix false positive for COMMIT_LOG_LONG_LINE
 with URLs

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@...il.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@...mentarea.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