[PATCH v2 0/2] docs & checkpatch: allow Closes tags with links

[PATCH v2 0/2] docs & checkpatch: allow Closes tags with links

[Matthieu Baerts]

Since v6.3, checkpatch.pl now complains about the use of "Closes:" tags
followed by a link [1]. It also complains if a "Reported-by:" tag is
followed by a "Closes:" one [2].

As detailed in the first patch, this "Closes:" tag is used for a bit of
time, mainly by DRM and MPTCP subsystems. It is used by some bug
trackers to automate the closure of issues when a patch is accepted.
It is even planned to use this tag with bugzilla.kernel.org [3].

The first patch updates the documentation to explain what is this
"Closes:" tag and how/when to use it. The second patch modifies
checkpatch.pl to stop complaining about it.

The DRM maintainers and their mailing list have been added in Cc as they
are probably interested by these two patches as well.

[1] https://lore.kernel.org/all/3b036087d80b8c0e07a46a1dbaaf4ad0d018f8d5.1674217480.git.linux@leemhuis.info/
[2] https://lore.kernel.org/all/bb5dfd55ea2026303ab2296f4a6df3da7dd64006.1674217480.git.linux@leemhuis.info/
[3] https://lore.kernel.org/linux-doc/20230315181205.f3av7h6owqzzw64p@meerkat.local/

Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
---
Note: After having re-read the comments from the v1, it is still unclear
to me if this "Closes:" can be accepted or not. But because it seems
that the future Bugzilla bot for kernel.org is going to use it, I'm
sending here a v2. I'm sorry if I misunderstood the comments from v1.
Please ignore this v2 if I did.

Changes in v2:
- Patch 1/2:
  - Add Konstantin's Acked-by: even if the patch has changed a bit, the
    concept is still the same, I hope that's OK.
  - Mention "public" in "5.Posting.rst" file as well. (Jonathan Corbet)
  - Re-phrase the new text from "5.Posting.rst". (Bagas Sanjaya &
    Thorsten Leemhuis)
  - Clearly mention that private bug trackers and invalid URLs are
    forbidden (Linus Torvalds).
- Rebased on top of Linus' repo.
- Link to v1: https://lore.kernel.org/r/20230314-doc-checkpatch-closes-tag-v1-0-1b83072e9a9a@tessares.net

---
Matthieu Baerts (2):
      docs: process: allow Closes tags with links
      checkpatch: allow Closes tags with links

 Documentation/process/5.Posting.rst          |  9 +++++++++
 Documentation/process/submitting-patches.rst |  9 +++++++++
 scripts/checkpatch.pl                        | 16 ++++++++--------
 3 files changed, 26 insertions(+), 8 deletions(-)
---
base-commit: cb7f5b41f8341148050fe63e27cf52aa4f1519ad
change-id: 20230314-doc-checkpatch-closes-tag-1731b57556b1

Best regards,
-- 
Matthieu Baerts <matthieu.baerts@tessares.net>

[PATCH v2 1/2] docs: process: allow Closes tags with links

[Matthieu Baerts]

Making sure a bug tracker is up to date is not an easy task. For
example, a first version of a patch fixing a tracked issue can be sent a
long time after having created the issue. But also, it can take some
time to have this patch accepted upstream in its final form. When it is
done, someone -- probably not the person who accepted the patch -- has
to remember about closing the corresponding issue.

This task of closing and tracking the patch can be done automatically by
bug trackers like GitLab [1], GitHub [2] and hopefully soon [3]
bugzilla.kernel.org when the appropriated tag is used. The two first
ones accept multiple tags but it is probably better to pick one.

According to commit 76f381bb77a0 ("checkpatch: warn when unknown tags are used for links"),
the "Closes" tag seems to have been used in the past by a few people and
it is supported by popular bug trackers. Here is how it has been used in
the past:

 $ git log --no-merges --format=email -P --grep='^Closes: http' | \
       grep '^Closes: http' | cut -d/ -f3-5 | sort | uniq -c | sort -rn
    391 gitlab.freedesktop.org/drm/intel
     79 github.com/multipath-tcp/mptcp_net-next
      8 gitlab.freedesktop.org/drm/msm
      3 gitlab.freedesktop.org/drm/amd
      2 gitlab.freedesktop.org/mesa/mesa
      1 patchwork.freedesktop.org/series/73320
      1 gitlab.freedesktop.org/lima/linux
      1 gitlab.freedesktop.org/drm/nouveau
      1 github.com/ClangBuiltLinux/linux
      1 bugzilla.netfilter.org/show_bug.cgi?id=1579
      1 bugzilla.netfilter.org/show_bug.cgi?id=1543
      1 bugzilla.netfilter.org/show_bug.cgi?id=1436
      1 bugzilla.netfilter.org/show_bug.cgi?id=1427
      1 bugs.debian.org/625804

Likely here, the "Closes" tag was only properly used with GitLab and
GitHub. We can also see that it has been used quite a few times (and
still used recently) and this is then not a "random tag that makes no
sense" like it was the case with "BugLink" recently [4]. It has also
been misused but that was a long time ago, when it was common to use
many different random tags.

checkpatch.pl script should then stop complaining about this "Closes"
tag. As suggested by Thorsten [5], if this tag is accepted, it should
first be described in the documentation. This is what is done here in
this patch.

Note that thanks to this "Closes" tag, the mentioned bug trackers can
also locate where a patch has been applied in different branches and
repositories. If only the "Link" tag is used, the tracking can also be
done but the ticket will not be closed and a manual operation will be
needed. Also, these bug trackers have some safeguards: the closure is
only done if a commit having the "Closes:" tag is applied in a specific
branch. It will then not be closed if a random commit having the same
tag is published elsewhere. Also in case of closure, a notification is
sent to the owners.

Link: https://docs.gitlab.com/ee/user/project/issues/managing_issues.html#default-closing-pattern [1]
Link: https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/using-keywords-in-issues-and-pull-requests [2]
Link: https://lore.kernel.org/linux-doc/20230315181205.f3av7h6owqzzw64p@meerkat.local/ [3]
Link: https://lore.kernel.org/all/CAHk-=wgs38ZrfPvy=nOwVkVzjpM3VFU1zobP37Fwd_h9iAD5JQ@mail.gmail.com/ [4]
Link: https://lore.kernel.org/all/688cd6cb-90ab-6834-a6f5-97080e39ca8e@leemhuis.info/ [5]
Link: https://github.com/multipath-tcp/mptcp_net-next/issues/373
Suggested-by: Thorsten Leemhuis <linux@leemhuis.info>
Acked-by: Konstantin Ryabitsev <konstantin@linuxfoundation.org>
Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
---
 Documentation/process/5.Posting.rst          | 9 +++++++++
 Documentation/process/submitting-patches.rst | 9 +++++++++
 2 files changed, 18 insertions(+)

diff --git a/Documentation/process/5.Posting.rst b/Documentation/process/5.Posting.rst
index 7a670a075ab6..20f0b6b639b7 100644
--- a/Documentation/process/5.Posting.rst
+++ b/Documentation/process/5.Posting.rst
@@ -217,6 +217,15 @@ latest public review posting of the patch; often this is automatically done
 by tools like b4 or a git hook like the one described in
 'Documentation/maintainer/configure-git.rst'.
 
+Similarly, there is also the "Closes:" tag that can be used to close issues
+when the underlying public bug tracker can do this operation automatically.
+For example::
+
+	Closes: https://example.com/issues/1234
+
+Private bug trackers and invalid URLs are forbidden. For other public bug
+trackers not supporting automations, keep using the "Link:" tag instead.
+
 A third kind of tag is used to document who was involved in the development of
 the patch. Each of these uses this format::
 
diff --git a/Documentation/process/submitting-patches.rst b/Documentation/process/submitting-patches.rst
index 69ce64e03c70..759c99e34085 100644
--- a/Documentation/process/submitting-patches.rst
+++ b/Documentation/process/submitting-patches.rst
@@ -134,6 +134,15 @@ resources. In addition to giving a URL to a mailing list archive or bug,
 summarize the relevant points of the discussion that led to the
 patch as submitted.
 
+It might be interesting to use the 'Closes:' tag to close issues when the
+underlying public bug tracker can do this operation automatically. For
+example::
+
+	Closes: https://example.com/issues/1234
+
+Private bug trackers and invalid URLs are forbidden. For other public bug
+trackers not supporting automations, keep using the "Link:" tag instead.
+
 If your patch fixes a bug in a specific commit, e.g. you found an issue using
 ``git bisect``, please use the 'Fixes:' tag with the first 12 characters of
 the SHA-1 ID, and the one line summary.  Do not split the tag across multiple

-- 
2.39.2

Re: [PATCH v2 1/2] docs: process: allow Closes tags with links

[Bagas Sanjaya]

On 3/25/23 01:52, Matthieu Baerts wrote:
> diff --git a/Documentation/process/5.Posting.rst b/Documentation/process/5.Posting.rst
> index 7a670a075ab6..20f0b6b639b7 100644
> --- a/Documentation/process/5.Posting.rst
> +++ b/Documentation/process/5.Posting.rst
> @@ -217,6 +217,15 @@ latest public review posting of the patch; often this is automatically done
>  by tools like b4 or a git hook like the one described in
>  'Documentation/maintainer/configure-git.rst'.
>  
> +Similarly, there is also the "Closes:" tag that can be used to close issues
> +when the underlying public bug tracker can do this operation automatically.
> +For example::
> +
> +	Closes: https://example.com/issues/1234
> +
> +Private bug trackers and invalid URLs are forbidden. For other public bug
> +trackers not supporting automations, keep using the "Link:" tag instead.
> +
>  A third kind of tag is used to document who was involved in the development of
>  the patch. Each of these uses this format::
>  
> diff --git a/Documentation/process/submitting-patches.rst b/Documentation/process/submitting-patches.rst
> index 69ce64e03c70..759c99e34085 100644
> --- a/Documentation/process/submitting-patches.rst
> +++ b/Documentation/process/submitting-patches.rst
> @@ -134,6 +134,15 @@ resources. In addition to giving a URL to a mailing list archive or bug,
>  summarize the relevant points of the discussion that led to the
>  patch as submitted.
>  
> +It might be interesting to use the 'Closes:' tag to close issues when the
> +underlying public bug tracker can do this operation automatically. For
> +example::
> +
> +	Closes: https://example.com/issues/1234
> +
> +Private bug trackers and invalid URLs are forbidden. For other public bug
> +trackers not supporting automations, keep using the "Link:" tag instead.
> +
>  If your patch fixes a bug in a specific commit, e.g. you found an issue using
>  ``git bisect``, please use the 'Fixes:' tag with the first 12 characters of
>  the SHA-1 ID, and the one line summary.  Do not split the tag across multiple
> 

The doc LGTM, thanks!

Reviewed-by: Bagas Sanjaya <bagasdotme@gmail.com>

-- 
An old man doll... just what I always wanted! - Clara

Re: [PATCH v2 1/2] docs: process: allow Closes tags with links

[Thorsten Leemhuis]

On 24.03.23 19:52, Matthieu Baerts wrote:
> Making sure a bug tracker is up to date is not an easy task. For
> example, a first version of a patch fixing a tracked issue can be sent a
> long time after having created the issue. But also, it can take some
> time to have this patch accepted upstream in its final form. When it is
> done, someone -- probably not the person who accepted the patch -- has
> to remember about closing the corresponding issue.
> 
> This task of closing and tracking the patch can be done automatically by
> bug trackers like GitLab [1], GitHub [2] and hopefully soon [3]
> bugzilla.kernel.org when the appropriated tag is used. The two first
> ones accept multiple tags but it is probably better to pick one.
> 
> [...]
> 
> diff --git a/Documentation/process/5.Posting.rst b/Documentation/process/5.Posting.rst
> index 7a670a075ab6..20f0b6b639b7 100644
> --- a/Documentation/process/5.Posting.rst
> +++ b/Documentation/process/5.Posting.rst
> @@ -217,6 +217,15 @@ latest public review posting of the patch; often this is automatically done
>  by tools like b4 or a git hook like the one described in
>  'Documentation/maintainer/configure-git.rst'.
>  
> +Similarly, there is also the "Closes:" tag that can be used to close issues
> +when the underlying public bug tracker can do this operation automatically.
> +For example::
> +
> +	Closes: https://example.com/issues/1234
> +
> +Private bug trackers and invalid URLs are forbidden. For other public bug
> +trackers not supporting automations, keep using the "Link:" tag instead.
> [...]

This more and more seems half-hearted to me.

One reason: it makes things unnecessarily complicated for developers, as
they'd then have to remember `is this a public bug tracker that is
supporting automations? Then use "Closes", otherwise "Link:"`.

Another reason: the resulting situation ignores my regression tracking
bot, which (among others) tracks emailed reports. It would benefit from
"Closes" as well to avoid the ambiguity problem Konstantin brought up
(the one about "Link: might just point to a report for background
information in patches that don't address the problem the link points
to"[1]. But FWIW, I'm not sure if this ambiguity is much of a problem in
practice, I have a feeling that it's rare and most of the time will
happen after the reported problem has been addressed or in the same
patch-set.

I thus think we should use either of these approaches:

* just stick to "Link: <url>"

* go "all-in" and tell developers to use "Closes: <url>"[2] all the time
when a patch is resolving an issue that was reported in public

I'm not sure which of them I prefer myself. Maybe I'm slightly leaning
towards the latter: it avoids the ambiguity, checkpatch.pl will yell if
it's used with something else than a URL, it makes things easier for
MPTCP & DRM developers, and (maybe most importantly) is something new
developers are often used to already from git forges.

Ciao, Thorsten

[1]
https://lore.kernel.org/linux-doc/20230317185637.ebxzsdxivhgzkqqw@meerkat.local/

[2] fwiw, I still prefer "Resolves:" over "Closes". Yes, I've seen
Konstantin's comment on the subtle difference between the two[3], but as
he said, Bugbot can work with it as well. But to me "Resolves" sounds
way friendlier and more descriptive to me; but well, I'm not a native
speaker, so I don't think my option should count much here.

[3]
https://lore.kernel.org/linux-doc/20230316162227.727rhima2tejdl5j@meerkat.local/

Re: [PATCH v2 1/2] docs: process: allow Closes tags with links

[Matthieu Baerts]

Hi Thorsten,

Thank you for your reply!

On 26/03/2023 13:28, Thorsten Leemhuis wrote:
> On 24.03.23 19:52, Matthieu Baerts wrote:
>> Making sure a bug tracker is up to date is not an easy task. For
>> example, a first version of a patch fixing a tracked issue can be sent a
>> long time after having created the issue. But also, it can take some
>> time to have this patch accepted upstream in its final form. When it is
>> done, someone -- probably not the person who accepted the patch -- has
>> to remember about closing the corresponding issue.
>>
>> This task of closing and tracking the patch can be done automatically by
>> bug trackers like GitLab [1], GitHub [2] and hopefully soon [3]
>> bugzilla.kernel.org when the appropriated tag is used. The two first
>> ones accept multiple tags but it is probably better to pick one.
>>
>> [...]
>>
>> diff --git a/Documentation/process/5.Posting.rst b/Documentation/process/5.Posting.rst
>> index 7a670a075ab6..20f0b6b639b7 100644
>> --- a/Documentation/process/5.Posting.rst
>> +++ b/Documentation/process/5.Posting.rst
>> @@ -217,6 +217,15 @@ latest public review posting of the patch; often this is automatically done
>>  by tools like b4 or a git hook like the one described in
>>  'Documentation/maintainer/configure-git.rst'.
>>  
>> +Similarly, there is also the "Closes:" tag that can be used to close issues
>> +when the underlying public bug tracker can do this operation automatically.
>> +For example::
>> +
>> +	Closes: https://example.com/issues/1234
>> +
>> +Private bug trackers and invalid URLs are forbidden. For other public bug
>> +trackers not supporting automations, keep using the "Link:" tag instead.
>> [...]
> 
> This more and more seems half-hearted to me.
> 
> One reason: it makes things unnecessarily complicated for developers, as
> they'd then have to remember `is this a public bug tracker that is
> supporting automations? Then use "Closes", otherwise "Link:"`.
> 
> Another reason: the resulting situation ignores my regression tracking
> bot, which (among others) tracks emailed reports. It would benefit from
> "Closes" as well to avoid the ambiguity problem Konstantin brought up
> (the one about "Link: might just point to a report for background
> information in patches that don't address the problem the link points
> to"[1]. But FWIW, I'm not sure if this ambiguity is much of a problem in
> practice, I have a feeling that it's rare and most of the time will
> happen after the reported problem has been addressed or in the same
> patch-set.

Even if they are rare, I think it might be good to avoid false-positives
that can be frustrating or create confusions. Using a dedicated tag plus
some safeguards help then be required. (And it is not compatible with
existing forges.)

> I thus think we should use either of these approaches:
> 
> * just stick to "Link: <url>"
> 
> * go "all-in" and tell developers to use "Closes: <url>"[2] all the time
> when a patch is resolving an issue that was reported in public
> 
> I'm not sure which of them I prefer myself. Maybe I'm slightly leaning
> towards the latter: it avoids the ambiguity, checkpatch.pl will yell if
> it's used with something else than a URL, it makes things easier for
> MPTCP & DRM developers, and (maybe most importantly) is something new
> developers are often used to already from git forges.

I think it makes sense not to restrict this tag to bug trackers with
automations as long as they are public of course. After having looked at
the comments from v1, I didn't feel like it would have been OK to extend
its usage but I can send a v3 taking this direction hoping to get more
feedback. After all, thanks to regzbot, we can also say that there are
some automations behind lore.kernel.org and other ML's :)

If we do that, would it be blocking to have this included in v6.3?

> [1]
> https://lore.kernel.org/linux-doc/20230317185637.ebxzsdxivhgzkqqw@meerkat.local/
> 
> [2] fwiw, I still prefer "Resolves:" over "Closes". Yes, I've seen
> Konstantin's comment on the subtle difference between the two[3], but as
> he said, Bugbot can work with it as well. But to me "Resolves" sounds
> way friendlier and more descriptive to me; but well, I'm not a native
> speaker, so I don't think my option should count much here.

As a non-native speaker, I'm open to use either of them. But as a
developer, I feel like I'm more used to see the "Closes:" tag than the
"Resolves" one.

When looking at the Git history, the "Closes:" tag with a link has been
used ~500 times, compared to ~14 times for "Resolves:". Maybe "Closes:"
is more natural for developers who first want to have their assigned
tickets being "closed" automatically than issues being "resolved"? :)

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: [PATCH v2 1/2] docs: process: allow Closes tags with links

[Thorsten Leemhuis]

On 27.03.23 15:05, Matthieu Baerts wrote:
> 
> Thank you for your reply!

Thank you for working on this!

> On 26/03/2023 13:28, Thorsten Leemhuis wrote:
>> On 24.03.23 19:52, Matthieu Baerts wrote:
>>> Making sure a bug tracker is up to date is not an easy task. For
>>> example, a first version of a patch fixing a tracked issue can be sent a
>>> long time after having created the issue. But also, it can take some
>>> time to have this patch accepted upstream in its final form. When it is
>>> done, someone -- probably not the person who accepted the patch -- has
>>> to remember about closing the corresponding issue.
>>>
>>> This task of closing and tracking the patch can be done automatically by
>>> bug trackers like GitLab [1], GitHub [2] and hopefully soon [3]
>>> bugzilla.kernel.org when the appropriated tag is used. The two first
>>> ones accept multiple tags but it is probably better to pick one.
>>>
>>> [...]
>>>
>>> diff --git a/Documentation/process/5.Posting.rst b/Documentation/process/5.Posting.rst
>>> index 7a670a075ab6..20f0b6b639b7 100644
>>> --- a/Documentation/process/5.Posting.rst
>>> +++ b/Documentation/process/5.Posting.rst
>>> @@ -217,6 +217,15 @@ latest public review posting of the patch; often this is automatically done
>>>  by tools like b4 or a git hook like the one described in
>>>  'Documentation/maintainer/configure-git.rst'.
>>>  
>>> +Similarly, there is also the "Closes:" tag that can be used to close issues
>>> +when the underlying public bug tracker can do this operation automatically.
>>> +For example::
>>> +
>>> +	Closes: https://example.com/issues/1234
>>> +
>>> +Private bug trackers and invalid URLs are forbidden. For other public bug
>>> +trackers not supporting automations, keep using the "Link:" tag instead.
>>> [...]
>>
>> This more and more seems half-hearted to me.
>>
>> One reason: it makes things unnecessarily complicated for developers, as
>> they'd then have to remember `is this a public bug tracker that is
>> supporting automations? Then use "Closes", otherwise "Link:"`.
>>
>> Another reason: the resulting situation ignores my regression tracking
>> bot, which (among others) tracks emailed reports. It would benefit from
>> "Closes" as well to avoid the ambiguity problem Konstantin brought up
>> (the one about "Link: might just point to a report for background
>> information in patches that don't address the problem the link points
>> to"[1]. But FWIW, I'm not sure if this ambiguity is much of a problem in
>> practice, I have a feeling that it's rare and most of the time will
>> happen after the reported problem has been addressed or in the same
>> patch-set.
> 
> Even if they are rare, I think it might be good to avoid false-positives
> that can be frustrating or create confusions. Using a dedicated tag plus
> some safeguards help then be required. (And it is not compatible with
> existing forges.)
Yeah, FWIW, I was all for such clear tags myself not that long ago (and
even twice proposed some), but due to the experience with regzbot and
Linus recent comment on Closes: I'm more in the neutral camp these days.

>> I thus think we should use either of these approaches:
>>
>> * just stick to "Link: <url>"
>>
>> * go "all-in" and tell developers to use "Closes: <url>"[2] all the time
>> when a patch is resolving an issue that was reported in public
>>
>> I'm not sure which of them I prefer myself. Maybe I'm slightly leaning
>> towards the latter: it avoids the ambiguity, checkpatch.pl will yell if
>> it's used with something else than a URL, it makes things easier for
>> MPTCP & DRM developers, and (maybe most importantly) is something new
>> developers are often used to already from git forges.
> 
> I think it makes sense not to restrict this tag to bug trackers with
> automations as long as they are public of course. After having looked at
> the comments from v1, I didn't feel like it would have been OK to extend
> its usage but I can send a v3 taking this direction hoping to get more
> feedback. After all, thanks to regzbot, we can also say that there are
> some automations behind lore.kernel.org and other ML's :)

:-D

> If we do that, would it be blocking to have this included in v6.3?

You mean if this still can go in for 6.3? Well, the patches afaics needs
to be ACKed by the right people first (Joe for checkpatch I guess, Jon
for docs). It likely also depends on how this discussion continues and
the opinion of the maintainer(s?) that picks up the patches.

>> [1]
>> https://lore.kernel.org/linux-doc/20230317185637.ebxzsdxivhgzkqqw@meerkat.local/
>>
>> [2] fwiw, I still prefer "Resolves:" over "Closes". Yes, I've seen
>> Konstantin's comment on the subtle difference between the two[3], but as
>> he said, Bugbot can work with it as well. But to me "Resolves" sounds
>> way friendlier and more descriptive to me; but well, I'm not a native
>> speaker, so I don't think my option should count much here.
> 
> As a non-native speaker, I'm open to use either of them. But as a
> developer, I feel like I'm more used to see the "Closes:" tag than the
> "Resolves" one.
> 
> When looking at the Git history, the "Closes:" tag with a link has been
> used ~500 times, compared to ~14 times for "Resolves:". Maybe "Closes:"
> is more natural for developers who first want to have their assigned
> tickets being "closed" automatically than issues being "resolved"? :)

Yeah, "developers are used to it" is a good argument. I'm not so sure
about the other argument, somehow "Resolves" feels more fitting to the
imperative language we use. Whatever, as I said, I don't care much (and
maybe thus shouldn't have written this paragraph :-D ).

Ciao, Thorsten

Re: [PATCH v2 1/2] docs: process: allow Closes tags with links

[Jonathan Corbet]

Thorsten Leemhuis <linux@leemhuis.info> writes:

>> If we do that, would it be blocking to have this included in v6.3?
>
> You mean if this still can go in for 6.3? Well, the patches afaics needs
> to be ACKed by the right people first (Joe for checkpatch I guess, Jon
> for docs). It likely also depends on how this discussion continues and
> the opinion of the maintainer(s?) that picks up the patches.

We're at -rc4, I wouldn't really consider this for 6.3 at this point.
There's no reason to try to rush it.

Thanks,

jon

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

[Matthieu Baerts]

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) {
 					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);
 			}
 		}
 

-- 
2.39.2

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

[Joe Perches]

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);
>  			}
>  		}
>  
> 

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

[Matthieu Baerts]

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
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

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

[Thorsten Leemhuis]

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

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

[Matthieu Baerts]

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
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

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

[Thorsten Leemhuis]

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