[Linux-kernel-mentees] [PATCH] checkpatch: fix false positive for COMMIT_LOG_LONG_LINE with URLs

[Linux-kernel-mentees] [PATCH] checkpatch: fix false positive for COMMIT_LOG_LONG_LINE with URLs

[Aditya Srivastava]

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
index abd5a3d2e913..23da1f50fe6a 100755
--- 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);
+			}
 			$commit_log_long_line = 1;
 		}
 
-- 
2.17.1

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH] checkpatch: fix false positive for COMMIT_LOG_LONG_LINE with URLs

[Lukas Bulwahn]

On Wed, Dec 16, 2020 at 1:32 PM Aditya Srivastava <yashsri421@gmail.com> 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:"
>

Looks good to me. This might be also a good one for a fix option.

Lukas

> 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
> index abd5a3d2e913..23da1f50fe6a 100755
> --- 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);
> +                       }
>                         $commit_log_long_line = 1;
>                 }
>
> --
> 2.17.1
>
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH] checkpatch: fix false positive for COMMIT_LOG_LONG_LINE with URLs

[Aditya]

On 16/12/20 6:05 pm, Lukas Bulwahn wrote:
> On Wed, Dec 16, 2020 at 1:32 PM Aditya Srivastava <yashsri421@gmail.com> 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:"
>>
> 
> Looks good to me. This might be also a good one for a fix option.
> 

I agree. Should I add the fix in this patch itself?

Thanks
Aditya

> Lukas
> 
>> 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
>> index abd5a3d2e913..23da1f50fe6a 100755
>> --- 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);
>> +                       }
>>                         $commit_log_long_line = 1;
>>                 }
>>
>> --
>> 2.17.1
>>

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH] checkpatch: fix false positive for COMMIT_LOG_LONG_LINE with URLs

[Lukas Bulwahn]

On Wed, Dec 16, 2020 at 1:54 PM Aditya <yashsri421@gmail.com> wrote:
>
> On 16/12/20 6:05 pm, Lukas Bulwahn wrote:
> > On Wed, Dec 16, 2020 at 1:32 PM Aditya Srivastava <yashsri421@gmail.com> 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:"
> >>
> >
> > Looks good to me. This might be also a good one for a fix option.
> >
>
> I agree. Should I add the fix in this patch itself?
>

No, make two patches and send them as a patch series to the mailing list.

Lukas

> Thanks
> Aditya
>
> > Lukas
> >
> >> 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
> >> index abd5a3d2e913..23da1f50fe6a 100755
> >> --- 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);
> >> +                       }
> >>                         $commit_log_long_line = 1;
> >>                 }
> >>
> >> --
> >> 2.17.1
> >>
>
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH] checkpatch: fix false positive for COMMIT_LOG_LONG_LINE with URLs

[Aditya]

On 16/12/20 6:27 pm, Lukas Bulwahn wrote:
> On Wed, Dec 16, 2020 at 1:54 PM Aditya <yashsri421@gmail.com> wrote:
>>
>> On 16/12/20 6:05 pm, Lukas Bulwahn wrote:
>>> On Wed, Dec 16, 2020 at 1:32 PM Aditya Srivastava <yashsri421@gmail.com> 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:"
>>>>
>>>
>>> Looks good to me. This might be also a good one for a fix option.
>>>
>>
>> I agree. Should I add the fix in this patch itself?
>>
> 
> No, make two patches and send them as a patch series to the mailing list.
> 
Hi Lukas
So, I generated a list of lines, which are URLs and reported by
COMMIT_LOG_LONG_LINE here:
https://github.com/AdityaSrivast/kernel-tasks/blob/master/Task6/links/long_line_links.txt

These occur in different forms:

1) A large part of these occur as \s*(http|https).

2) These also occur as [1]: https, [2]: https, etc

3) BugLink: https:// (e.g.: BugLink:
http://lkml.kernel.org/r/20180426045223.GA15307@dragonet.kaist.ac.kr)

4) References: https:// (e.g.: References:
http://mid.mail-archive.com/alpine.DEB.2.20.1905262211270.24390@whs-18.cs.helsinki.fi)

5) Link to v1: (E.g.: Link to v1:
https://patchwork.freedesktop.org/patch/msgid/20190228210939.83386-6-sean@poorly.run)

6) Datasheet (e.g.
Datasheet:http://www.winbond.com/resource-files/w25n01gv%20revl%20050918%20unsecured.pdf)

7) URL enclosed within brackets (E.g.:
(https://syzkaller.appspot.com/bug?id=ad7e0351fbc90535558514a71cd3edc11681997a).)

8) Link within sentences (E.g.: suite can be found at
https://github.com/Cyan4973/xxHash/blob/cf46e0c/xxhsum.c#L664)

etc.

Among these I guess we can fix occurrences like 1st by prefixing with
"Link:". But I am not sure about the rest.

What do you think?

Thanks
Aditya
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH] checkpatch: fix false positive for COMMIT_LOG_LONG_LINE with URLs

[Aditya]

On 16/12/20 9:59 pm, Aditya wrote:
> On 16/12/20 6:27 pm, Lukas Bulwahn wrote:
>> On Wed, Dec 16, 2020 at 1:54 PM Aditya <yashsri421@gmail.com> wrote:
>>>
>>> On 16/12/20 6:05 pm, Lukas Bulwahn wrote:
>>>> On Wed, Dec 16, 2020 at 1:32 PM Aditya Srivastava <yashsri421@gmail.com> 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:"
>>>>>
>>>>
>>>> Looks good to me. This might be also a good one for a fix option.
>>>>
>>>
>>> I agree. Should I add the fix in this patch itself?
>>>
>>
>> No, make two patches and send them as a patch series to the mailing list.
>>
> Hi Lukas
> So, I generated a list of lines, which are URLs and reported by
> COMMIT_LOG_LONG_LINE here:
> https://github.com/AdityaSrivast/kernel-tasks/blob/master/Task6/links/long_line_links.txt
> 
> These occur in different forms:
> 
> 1) A large part of these occur as \s*(http|https).
> 
> 2) These also occur as [1]: https, [2]: https, etc
> 
> 3) BugLink: https:// (e.g.: BugLink:
> http://lkml.kernel.org/r/20180426045223.GA15307@dragonet.kaist.ac.kr)
> 
> 4) References: https:// (e.g.: References:
> http://mid.mail-archive.com/alpine.DEB.2.20.1905262211270.24390@whs-18.cs.helsinki.fi)
> 
> 5) Link to v1: (E.g.: Link to v1:
> https://patchwork.freedesktop.org/patch/msgid/20190228210939.83386-6-sean@poorly.run)
> 
> 6) Datasheet (e.g.
> Datasheet:http://www.winbond.com/resource-files/w25n01gv%20revl%20050918%20unsecured.pdf)
> 
> 7) URL enclosed within brackets (E.g.:
> (https://syzkaller.appspot.com/bug?id=ad7e0351fbc90535558514a71cd3edc11681997a).)
> 
> 8) Link within sentences (E.g.: suite can be found at
> https://github.com/Cyan4973/xxHash/blob/cf46e0c/xxhsum.c#L664)
> 
> etc.
> 
> Among these I guess we can fix occurrences like 1st by prefixing with
> "Link:". But I am not sure about the rest.
> 
> What do you think?
> 

I think we can only fix the lines with ^\s*(http|https|ftp) and should
just give warning for the others. The user can then decide how they
want to fix.

Or should we give warning also for ^\s*(http|https|ftp) only?

I guess we can send this patch to Joe and see what he thinks. This
might give us a better idea for the fix.

What do you think?

Thanks
Aditya
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH] checkpatch: fix false positive for COMMIT_LOG_LONG_LINE with URLs

[Lukas Bulwahn]

[-- Attachment #1.1: Type: text/plain, Size: 3459 bytes --]

On Thu, Dec 17, 2020 at 12:12 PM Aditya <yashsri421@gmail.com> wrote:

> On 16/12/20 9:59 pm, Aditya wrote:
> > On 16/12/20 6:27 pm, Lukas Bulwahn wrote:
> >> On Wed, Dec 16, 2020 at 1:54 PM Aditya <yashsri421@gmail.com> wrote:
> >>>
> >>> On 16/12/20 6:05 pm, Lukas Bulwahn wrote:
> >>>> On Wed, Dec 16, 2020 at 1:32 PM Aditya Srivastava <
> yashsri421@gmail.com> 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:"
> >>>>>
> >>>>
> >>>> Looks good to me. This might be also a good one for a fix option.
> >>>>
> >>>
> >>> I agree. Should I add the fix in this patch itself?
> >>>
> >>
> >> No, make two patches and send them as a patch series to the mailing
> list.
> >>
> > Hi Lukas
> > So, I generated a list of lines, which are URLs and reported by
> > COMMIT_LOG_LONG_LINE here:
> >
> https://github.com/AdityaSrivast/kernel-tasks/blob/master/Task6/links/long_line_links.txt
> >
> > These occur in different forms:
> >
> > 1) A large part of these occur as \s*(http|https).
> >
> > 2) These also occur as [1]: https, [2]: https, etc
> >
> > 3) BugLink: https:// (e.g.: BugLink:
> > http://lkml.kernel.org/r/20180426045223.GA15307@dragonet.kaist.ac.kr)
> >
> > 4) References: https:// (e.g.: References:
> >
> http://mid.mail-archive.com/alpine.DEB.2.20.1905262211270.24390@whs-18.cs.helsinki.fi
> )
> >
> > 5) Link to v1: (E.g.: Link to v1:
> >
> https://patchwork.freedesktop.org/patch/msgid/20190228210939.83386-6-sean@poorly.run
> )
> >
> > 6) Datasheet (e.g.
> > Datasheet:
> http://www.winbond.com/resource-files/w25n01gv%20revl%20050918%20unsecured.pdf
> )
> >
> > 7) URL enclosed within brackets (E.g.:
> > (
> https://syzkaller.appspot.com/bug?id=ad7e0351fbc90535558514a71cd3edc11681997a)
> .)
> >
> > 8) Link within sentences (E.g.: suite can be found at
> > https://github.com/Cyan4973/xxHash/blob/cf46e0c/xxhsum.c#L664)
> >
> > etc.
> >
> > Among these I guess we can fix occurrences like 1st by prefixing with
> > "Link:". But I am not sure about the rest.
> >
> > What do you think?
> >
>
> I think we can only fix the lines with ^\s*(http|https|ftp) and should
> just give warning for the others. The user can then decide how they
> want to fix.
>
> Or should we give warning also for ^\s*(http|https|ftp) only?
>
> I guess we can send this patch to Joe and see what he thinks. This
> might give us a better idea for the fix.
>
> What do you think?
>
>
Agree. If we summarize the use of hyperlinks in commit messages properly,
we might also ask on the ksummit-discuss list for some feedback on
"standardizing the use of hyperlinks in commit messages beyond Link-tags".

All those examples show that there are various different preferences but
really no common agreed way that everyone uses.

Let us start the discussion with Joe and then see.

Lukas

[-- Attachment #1.2: Type: text/html, Size: 5722 bytes --]

[-- Attachment #2: Type: text/plain, Size: 201 bytes --]

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

[Linux-kernel-mentees] [PATCH] checkpatch: fix false positive for COMMIT_LOG_LONG_LINE with URLs

[Aditya Srivastava]

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
index abd5a3d2e913..23da1f50fe6a 100755
--- 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);
+			}
 			$commit_log_long_line = 1;
 		}
 
-- 
2.17.1

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH] checkpatch: fix false positive for COMMIT_LOG_LONG_LINE with URLs

[Joe Perches]

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



_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [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@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
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees