[v2,2/2] checkpatch: allow Closes tags with links

  As a follow-up of the previous patch modifying the documentation to
allow using the "Closes:" tag, checkpatch.pl is updated accordingly.

checkpatch.pl now mentions the "Closes:" tag between brackets to express
the fact it should be used only if it makes sense.

While at it, checkpatch.pl will not complain if the "Closes" tag is used
with a "long" line, similar to what is done with the "Link" tag.

Fixes: 76f381bb77a0 ("checkpatch: warn when unknown tags are used for links")
Fixes: d7f1d71e5ef6 ("checkpatch: warn when Reported-by: is not followed by Link:")
Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/373
Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
---
 scripts/checkpatch.pl | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)
  

On Fri, 2023-03-24 at 19:52 +0100, Matthieu Baerts wrote:
> As a follow-up of the previous patch modifying the documentation to
> allow using the "Closes:" tag, checkpatch.pl is updated accordingly.
> 
> checkpatch.pl now mentions the "Closes:" tag between brackets to express
> the fact it should be used only if it makes sense.
> 
> While at it, checkpatch.pl will not complain if the "Closes" tag is used
> with a "long" line, similar to what is done with the "Link" tag.
> 
> Fixes: 76f381bb77a0 ("checkpatch: warn when unknown tags are used for links")
> Fixes: d7f1d71e5ef6 ("checkpatch: warn when Reported-by: is not followed by Link:")
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/373
> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> ---
>  scripts/checkpatch.pl | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index bd44d12965c9..d6376e0b68cc 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -3158,14 +3158,14 @@ sub process {
>  				}
>  			}
>  
> -# check if Reported-by: is followed by a Link:
> +# check if Reported-by: is followed by a Link: (or Closes:) tag
>  			if ($sign_off =~ /^reported(?:|-and-tested)-by:$/i) {
>  				if (!defined $lines[$linenr]) {
>  					WARN("BAD_REPORTED_BY_LINK",
> -					     "Reported-by: should be immediately followed by Link: to the report\n" . $herecurr . $rawlines[$linenr] . "\n");
> -				} elsif ($rawlines[$linenr] !~ m{^link:\s*https?://}i) {
> +					     "Reported-by: should be immediately followed by Link: (or Closes:) to the report\n" . $herecurr . $rawlines[$linenr] . "\n");
> +				} elsif ($rawlines[$linenr] !~ m{^(link|closes):\s*https?://}i) {

Please do not use an unnecessary capture group.

		(?:link|closes)

And because it's somewhat likely that _more_ of these keywords
could be added, perhaps use some array like deprecated_apis


>  					WARN("BAD_REPORTED_BY_LINK",
> -					     "Reported-by: should be immediately followed by Link: with a URL to the report\n" . $herecurr . $rawlines[$linenr] . "\n");
> +					     "Reported-by: should be immediately followed by Link: (or Closes:) with a URL to the report\n" . $herecurr . $rawlines[$linenr] . "\n");
>  				}
>  			}
>  		}
> @@ -3250,8 +3250,8 @@ sub process {
>  					# file delta changes
>  		      $line =~ /^\s*(?:[\w\.\-\+]*\/)++[\w\.\-\+]+:/ ||
>  					# filename then :
> -		      $line =~ /^\s*(?:Fixes:|Link:|$signature_tags)/i ||
> -					# A Fixes: or Link: line or signature tag line
> +		      $line =~ /^\s*(?:Fixes:|Link:|Closes:|$signature_tags)/i ||
> +					# A Fixes:, Link:, Closes: 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);
> @@ -3266,13 +3266,13 @@ sub process {
>  
>  # Check for odd tags before a URI/URL
>  		if ($in_commit_log &&
> -		    $line =~ /^\s*(\w+):\s*http/ && $1 ne 'Link') {
> +		    $line =~ /^\s*(\w+):\s*http/ && $1 ne 'Link' && $1 ne 'Closes') {
>  			if ($1 =~ /^v(?:ersion)?\d+/i) {
>  				WARN("COMMIT_LOG_VERSIONING",
>  				     "Patch version information should be after the --- line\n" . $herecurr);
>  			} else {
>  				WARN("COMMIT_LOG_USE_LINK",
> -				     "Unknown link reference '$1:', use 'Link:' instead\n" . $herecurr);
> +				     "Unknown link reference '$1:', use 'Link:' (or 'Closes:') instead\n" . $herecurr);
>  			}
>  		}
>  
>
  

On 24.03.23 19:52, Matthieu Baerts wrote:
> As a follow-up of the previous patch modifying the documentation to
> allow using the "Closes:" tag, checkpatch.pl is updated accordingly.
> 
> checkpatch.pl now mentions the "Closes:" tag between brackets to express
> the fact it should be used only if it makes sense.
> 
> While at it, checkpatch.pl will not complain if the "Closes" tag is used
> with a "long" line, similar to what is done with the "Link" tag.
> 
> [...]
>  
> -# check if Reported-by: is followed by a Link:
> +# check if Reported-by: is followed by a Link: (or Closes:) tag

Small detail: why the parenthesis here? Why no simply "check if
Reported-by: is followed by a either Link: or Closes: tag". Same below...

>  			if ($sign_off =~ /^reported(?:|-and-tested)-by:$/i) {
>  				if (!defined $lines[$linenr]) {
>  					WARN("BAD_REPORTED_BY_LINK",
> -					     "Reported-by: should be immediately followed by Link: to the report\n" . $herecurr . $rawlines[$linenr] . "\n");
> -				} elsif ($rawlines[$linenr] !~ m{^link:\s*https?://}i) {
> +					     "Reported-by: should be immediately followed by Link: (or Closes:) to the report\n" . $herecurr . $rawlines[$linenr] . "\n");

...here, where users actually get to see this and might wonder why it's
written like that, without getting any answer.

Ciao, Thorsten
  

Hi Joe,

Thank you for the review!

On 24/03/2023 20:13, Joe Perches wrote:
> On Fri, 2023-03-24 at 19:52 +0100, Matthieu Baerts wrote:
>> As a follow-up of the previous patch modifying the documentation to
>> allow using the "Closes:" tag, checkpatch.pl is updated accordingly.
>>
>> checkpatch.pl now mentions the "Closes:" tag between brackets to express
>> the fact it should be used only if it makes sense.
>>
>> While at it, checkpatch.pl will not complain if the "Closes" tag is used
>> with a "long" line, similar to what is done with the "Link" tag.
>>
>> Fixes: 76f381bb77a0 ("checkpatch: warn when unknown tags are used for links")
>> Fixes: d7f1d71e5ef6 ("checkpatch: warn when Reported-by: is not followed by Link:")
>> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/373
>> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
>> ---
>>  scripts/checkpatch.pl | 16 ++++++++--------
>>  1 file changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>> index bd44d12965c9..d6376e0b68cc 100755
>> --- a/scripts/checkpatch.pl
>> +++ b/scripts/checkpatch.pl
>> @@ -3158,14 +3158,14 @@ sub process {
>>  				}
>>  			}
>>  
>> -# check if Reported-by: is followed by a Link:
>> +# check if Reported-by: is followed by a Link: (or Closes:) tag
>>  			if ($sign_off =~ /^reported(?:|-and-tested)-by:$/i) {
>>  				if (!defined $lines[$linenr]) {
>>  					WARN("BAD_REPORTED_BY_LINK",
>> -					     "Reported-by: should be immediately followed by Link: to the report\n" . $herecurr . $rawlines[$linenr] . "\n");
>> -				} elsif ($rawlines[$linenr] !~ m{^link:\s*https?://}i) {
>> +					     "Reported-by: should be immediately followed by Link: (or Closes:) to the report\n" . $herecurr . $rawlines[$linenr] . "\n");
>> +				} elsif ($rawlines[$linenr] !~ m{^(link|closes):\s*https?://}i) {
> 
> Please do not use an unnecessary capture group.
> 
> 		(?:link|closes)

Good point, thank you, that will be in the v3.

> And because it's somewhat likely that _more_ of these keywords
> could be added, perhaps use some array like deprecated_apis

I can but from the discussions we had on the v1, it looks unlikely to me
that more of these keywords will be allowed (if this one already ends up
being accepted :) ). Strangely, we might not even want to make it easy
to add new tags.

But I'm fine to change that in the v3 if you prefer to have an array here.

Cheers,
Matt
  

Hi Thorsten,

On 25/03/2023 07:25, Thorsten Leemhuis wrote:
> On 24.03.23 19:52, Matthieu Baerts wrote:
>> As a follow-up of the previous patch modifying the documentation to
>> allow using the "Closes:" tag, checkpatch.pl is updated accordingly.
>>
>> checkpatch.pl now mentions the "Closes:" tag between brackets to express
>> the fact it should be used only if it makes sense.
>>
>> While at it, checkpatch.pl will not complain if the "Closes" tag is used
>> with a "long" line, similar to what is done with the "Link" tag.
>>
>> [...]
>>  
>> -# check if Reported-by: is followed by a Link:
>> +# check if Reported-by: is followed by a Link: (or Closes:) tag
> 
> Small detail: why the parenthesis here? Why no simply "check if
> Reported-by: is followed by a either Link: or Closes: tag". Same below...
> 
>>  			if ($sign_off =~ /^reported(?:|-and-tested)-by:$/i) {
>>  				if (!defined $lines[$linenr]) {
>>  					WARN("BAD_REPORTED_BY_LINK",
>> -					     "Reported-by: should be immediately followed by Link: to the report\n" . $herecurr . $rawlines[$linenr] . "\n");
>> -				} elsif ($rawlines[$linenr] !~ m{^link:\s*https?://}i) {
>> +					     "Reported-by: should be immediately followed by Link: (or Closes:) to the report\n" . $herecurr . $rawlines[$linenr] . "\n");
> 
> ...here, where users actually get to see this and might wonder why it's
> written like that, without getting any answer.

I tried to explain that in the cover-letter but maybe I should add an
additional comment in the code: checkpatch.pl now mentions the "Closes:"
tag between parenthesis to express the fact it should be used only if it
makes sense. I didn't find any other short ways to express that but I'm
open to suggestions.

Now as discussed on patch 1/2, if the "Closes:" tag can be used with any
public link, we should definitively remove the parenthesis here and
probably below (see "Check for odd tags before a URI/URL") as well.

Cheers,
Matt
  

On 27.03.23 15:06, Matthieu Baerts wrote:
> Hi Thorsten,
> 
> On 25/03/2023 07:25, Thorsten Leemhuis wrote:
>> On 24.03.23 19:52, Matthieu Baerts wrote:
>>> As a follow-up of the previous patch modifying the documentation to
>>> allow using the "Closes:" tag, checkpatch.pl is updated accordingly.
>>>
>>> checkpatch.pl now mentions the "Closes:" tag between brackets to express
>>> the fact it should be used only if it makes sense.
>>>
>>> While at it, checkpatch.pl will not complain if the "Closes" tag is used
>>> with a "long" line, similar to what is done with the "Link" tag.
>>>
>>> [...]
>>>  
>>> -# check if Reported-by: is followed by a Link:
>>> +# check if Reported-by: is followed by a Link: (or Closes:) tag
>>
>> Small detail: why the parenthesis here? Why no simply "check if
>> Reported-by: is followed by a either Link: or Closes: tag". Same below...
>>
>>>  			if ($sign_off =~ /^reported(?:|-and-tested)-by:$/i) {
>>>  				if (!defined $lines[$linenr]) {
>>>  					WARN("BAD_REPORTED_BY_LINK",
>>> -					     "Reported-by: should be immediately followed by Link: to the report\n" . $herecurr . $rawlines[$linenr] . "\n");
>>> -				} elsif ($rawlines[$linenr] !~ m{^link:\s*https?://}i) {
>>> +					     "Reported-by: should be immediately followed by Link: (or Closes:) to the report\n" . $herecurr . $rawlines[$linenr] . "\n");
>>
>> ...here, where users actually get to see this and might wonder why it's
>> written like that, without getting any answer.
> 
> I tried to explain that in the cover-letter but maybe I should add an
> additional comment in the code: checkpatch.pl now mentions the "Closes:"
> tag between parenthesis to express the fact it should be used only if it
> makes sense. I didn't find any other short ways to express that but I'm
> open to suggestions.
> 
> Now as discussed on patch 1/2, if the "Closes:" tag can be used with any
> public link, we should definitively remove the parenthesis here and
> probably below (see "Check for odd tags before a URI/URL") as well.

Well, ymmd, but if we go down that route I'd say this code should
suggest to use "Closes:" all the time (or primarily).

Ciao, Thorsten