Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <94d6a56174c56ef13c9794186fdbfcc12c376c00.camel@perches.com>
Subject: Re: [PATCH] checkpatch: fix false positive for COMMIT_LOG_LONG_LINE
 with URLs
From:   Joe Perches <joe@perches.com>
To:     Aditya Srivastava <yashsri421@gmail.com>
Cc:     lukas.bulwahn@gmail.com,
        linux-kernel-mentees@lists.linuxfoundation.org,
        linux-kernel@vger.kernel.org
Date:   Thu, 17 Dec 2020 09:03:06 -0800
In-Reply-To: <20201217134221.16178-1-yashsri421@gmail.com>
References: <20201217134221.16178-1-yashsri421@gmail.com>
Content-Type: text/plain; charset="ISO-8859-1"
User-Agent: Evolution 3.38.1-1 
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

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 {

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.

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