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

[PATCH 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.

Because this tag is used for a bit of time by different subsystems and
it looks like it makes sense and it is useful for them, I didn't bother
Linus to get his permission to continue using it. If you think this is
necessary to do that up front, please tell me and I will be happy to ask
for his agreement.

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/

Signed-off-by: Matthieu Baerts <matthieu.baerts@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 |  7 +++++++
 scripts/checkpatch.pl                        | 16 ++++++++--------
 3 files changed, 24 insertions(+), 8 deletions(-)
---
base-commit: 6015b1aca1a233379625385feb01dd014aca60b5
change-id: 20230314-doc-checkpatch-closes-tag-1731b57556b1

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

[PATCH 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] and GitHub [2] when the appropriated tag is
used. They both 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 [3].

checkpatch.pl script should then stop complaining about this "Closes"
tag. As suggested by Thorsten [4], 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.

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/all/CAHk-=wgs38ZrfPvy=nOwVkVzjpM3VFU1zobP37Fwd_h9iAD5JQ@mail.gmail.com/ [3]
Link: https://lore.kernel.org/all/688cd6cb-90ab-6834-a6f5-97080e39ca8e@leemhuis.info/ [4]
Link: https://github.com/multipath-tcp/mptcp_net-next/issues/373
Suggested-by: Thorsten Leemhuis <linux@leemhuis.info>
Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
---
 Documentation/process/5.Posting.rst          | 9 +++++++++
 Documentation/process/submitting-patches.rst | 7 +++++++
 2 files changed, 16 insertions(+)

diff --git a/Documentation/process/5.Posting.rst b/Documentation/process/5.Posting.rst
index 7a670a075ab6..582d67eb2420 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'.
 
+In the same category as linking web pages, a special tag is also used to close
+issues but only when the mentioned ticketing system can do this operation
+automatically::
+
+        Closes: https://example.com/issues/1234
+
+Please use this 'Closes:' tag only if it helps managing issues thanks to
+automations. If not, pick the 'Link:' one.
+
 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..88561a32b5cc 100644
--- a/Documentation/process/submitting-patches.rst
+++ b/Documentation/process/submitting-patches.rst
@@ -134,6 +134,13 @@ 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.
 
+Note that it might be interesting to use the 'Closes:' tag instead of 'Link:'
+if the URL points to an issue from public bug tracker that can automatically
+close tickets when such patches containing this tag is accepted upstream. For
+example::
+
+    Closes: https://example.com/issues/1234
+
 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

[PATCH 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 1/2] docs: process: allow Closes tags with links

[Konstantin Ryabitsev]

On Wed, Mar 15, 2023 at 06:44:56PM +0100, Matthieu Baerts wrote:
> 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.

We will soon gain this ability on bugzilla.kernel.org as one of the features
of the bugbot integration tool (which is still WIP). So, if it helps, I
support making this a recognized trailer.

Acked-by: Konstantin Ryabitsev <konstantin@linuxfoundation.org>

-K

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

[Jonathan Corbet]

Matthieu Baerts <matthieu.baerts@tessares.net> writes:

> +In the same category as linking web pages, a special tag is also used to close
> +issues but only when the mentioned ticketing system can do this operation
> +automatically::
> +
> +        Closes: https://example.com/issues/1234
> +
> +Please use this 'Closes:' tag only if it helps managing issues thanks to
> +automations. If not, pick the 'Link:' one.

So if there is a consensus for this, I can certainly apply the patch.

I do think, though, that if we accept this tag, we should ask that it
only be used for *public* trackers.  A bunch of tags referring to
internal trackers and such aren't going to be all that helpful.

jon

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

[Matthieu Baerts]

Hi Jon,

On 15/03/2023 19:19, Jonathan Corbet wrote:
> Matthieu Baerts <matthieu.baerts@tessares.net> writes:
> 
>> +In the same category as linking web pages, a special tag is also used to close
>> +issues but only when the mentioned ticketing system can do this operation
>> +automatically::
>> +
>> +        Closes: https://example.com/issues/1234
>> +
>> +Please use this 'Closes:' tag only if it helps managing issues thanks to
>> +automations. If not, pick the 'Link:' one.
> 
> So if there is a consensus for this, I can certainly apply the patch.
> 
> I do think, though, that if we accept this tag, we should ask that it
> only be used for *public* trackers.  A bunch of tags referring to
> internal trackers and such aren't going to be all that helpful.

Thank you for this feedback!

I agree, this should only refer to public bug trackers otherwise the
link is useless for most people.

In fact, that's what I wrote in submitting-patches.rst but I just
noticed I forgot to duplicate this into 5.Posting.rst. I can do that in
a v2 if there is no objection to allow this "Closes:" tag.

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

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

[Matthieu Baerts]

Hi Konstantin,

On 15/03/2023 19:12, Konstantin Ryabitsev wrote:
> On Wed, Mar 15, 2023 at 06:44:56PM +0100, Matthieu Baerts wrote:
>> 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.
> 
> We will soon gain this ability on bugzilla.kernel.org as one of the features
> of the bugbot integration tool (which is still WIP).

Good news! This feature will be helpful for bugzilla users!

> So, if it helps, I
> support making this a recognized trailer.
Thank you for your support!

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

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

[Andrew Morton]

On Wed, 15 Mar 2023 18:44:56 +0100 Matthieu Baerts <matthieu.baerts@tessares.net> wrote:

> +        Closes: https://example.com/issues/1234

I (and a few others) have been using "Addresses:" on occasion.  I think
"Addresses:" is a bit more general.  And more humble - "I tried to fix
it", not "there, I fixed it".

But whatever - both are good.

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

[Thorsten Leemhuis]

On 15.03.23 18:44, Matthieu Baerts wrote:
> 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.
> 
> Because this tag is used for a bit of time by different subsystems and
> it looks like it makes sense and it is useful for them, I didn't bother
> Linus to get his permission to continue using it. If you think this is
> necessary to do that up front, please tell me and I will be happy to ask
> for his agreement.

Due to how he reacted to some "invented" tags recently, I'd think it
would be appropriate to CC him on this patchset, as he then can speak up
if he wants to (and I assume a few more mails don't bother him).

> 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.

I liked Andrew's `have been using "Addresses:" on occasion. [...] more
humble [...]` comment.  Sadly that tag is not supported by GitLab and
GitHub. But well, "Resolves" is and also a bit more humble if you ask
me. How about using that instead? Assuming that Konstantin can work with
that tag, too, but I guess he can.

I also wonder if the texts for the documentation could be shorter.
Wouldn't something like this do?

`Instead of "Link:" feel free to use "Resolves:" with an URL instead, if
the issue was filed in a public bug tracker that will consider the issue
resolved when it noticed that tag.`

[s/Resolves/Closes/ if we stick to that]

Side note: makes we wonder if we should go "all in" here to avoid
confusion and allow "Resolves" everywhere, even for links to lore.

> [...]

Ciao, Thorsten

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

[Matthieu Baerts]

Hi Thorsten, Linus,

@Linus: in short, we would like to continue using the "Closes:" tag (or
similar, see below) with a URL in commit messages. They are useful to
have public bug trackers doing automated actions like closing a specific
ticket. Any objection from your side?

The full thread is visible there:

https://lore.kernel.org/linux-doc/20230314-doc-checkpatch-closes-tag-v1-0-1b83072e9a9a@tessares.net/T/


@Thorsten: thank you for your reply!

On 16/03/2023 10:22, Thorsten Leemhuis wrote:
> On 15.03.23 18:44, Matthieu Baerts wrote:
>> 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.
>>
>> Because this tag is used for a bit of time by different subsystems and
>> it looks like it makes sense and it is useful for them, I didn't bother
>> Linus to get his permission to continue using it. If you think this is
>> necessary to do that up front, please tell me and I will be happy to ask
>> for his agreement.
> 
> Due to how he reacted to some "invented" tags recently, I'd think it
> would be appropriate to CC him on this patchset, as he then can speak up
> if he wants to (and I assume a few more mails don't bother him).

Sure, just did with a short summary.

>> 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.
> 
> I liked Andrew's `have been using "Addresses:" on occasion. [...] more
> humble [...]` comment.  Sadly that tag is not supported by GitLab and
> GitHub. But well, "Resolves" is and also a bit more humble if you ask
> me. How about using that instead? Assuming that Konstantin can work with
> that tag, too, but I guess he can.

I don't mind changing the tag name but I still have a preference to use
'Closes:' simply because it was used ~500 times in the past.

If we want to change, it is probably the best time to do so but for me,
the fact we -- MPTCP subsystem -- use the same tag as the DRM subsystem
(and ClangBuiltLinux and Debian) without consulting each other -- if I'm
not mistaken -- is a sign it is a good tag :)

> I also wonder if the texts for the documentation could be shorter.
> Wouldn't something like this do?
> 
> `Instead of "Link:" feel free to use "Resolves:" with an URL instead, if
> the issue was filed in a public bug tracker that will consider the issue
> resolved when it noticed that tag.`
> 
> [s/Resolves/Closes/ if we stick to that]

Sure, I'm not used to write doc and I appreciate your suggestion to
improve that. I might change one or two words but I have no objection to
write this in the v2 once we agreed on the name of this tag.

Also, should I use the same text in both process/5.Posting.rst and
process/submitting-patches.rst?

> Side note: makes we wonder if we should go "all in" here to avoid
> confusion and allow "Resolves" everywhere, even for links to lore.

Personally, I would recommend that, it might even be useful for other
bots like regzbot: a patch can be linked to one discussion but not
fixing the issue and even fixing another one instead. It might be useful
for a bot to be able to distinguish the two without depending on a not
100% reliable AI ;-)

A concrete example: patch 1/2 of this series is linked to a bug report
[1]. The ticket can be closed only when patch 2/2 will be applied.

Cheers,
Matt

[1] https://github.com/multipath-tcp/mptcp_net-next/issues/373
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

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

[Konstantin Ryabitsev]

On Thu, Mar 16, 2023 at 10:22:18AM +0100, Thorsten Leemhuis wrote:
> I liked Andrew's `have been using "Addresses:" on occasion. [...] more
> humble [...]` comment.  Sadly that tag is not supported by GitLab and
> GitHub. But well, "Resolves" is and also a bit more humble if you ask
> me. How about using that instead? Assuming that Konstantin can work with
> that tag, too, but I guess he can.

There's a subtle difference between "Closes" and "Resolves" that may be
important to consider ("closes" doesn't really imply the bug is "fixed").

The Bugbot should eventually support a number of "if this, then that"
conditions once it's done, so which tag we look for will be a matter of
configuration. It's not yet at that stage, though I should have some initial
trials in the near future.

-K

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

[Linus Torvalds]

On Thu, Mar 16, 2023 at 4:43 AM Matthieu Baerts
<matthieu.baerts@tessares.net> wrote:
>
> @Linus: in short, we would like to continue using the "Closes:" tag (or
> similar, see below) with a URL in commit messages. They are useful to
> have public bug trackers doing automated actions like closing a specific
> ticket. Any objection from your side?

As long as it's a public link, I guess that just documents what the
drm people have been doing.

I'm not convinced "Closes" is actually any better than just "Link:",
though. I would very much hope and expect that the actual closing of
any bug report is actually done separately and verified, rather than
some kind of automated "well, the commit says it closes it, so.."

So honestly, I feel like "Link:" is just a better thing, and I worry
that "Closes:" is then going to be used for random internal crap.
We've very much seen people wanting to do that - having their own
private bug trackers, and then using the commit message to refer to
them, which I am *violently* against. If it's only useful to some
closed community, it shouldn't be in the public commits.

And while the current GPU people seem to use "Closes:" the right way
(and maybe some other groups do too - but it does seem to be mostly a
freedesktop thing), I really think it is amenable to mis-use in ways
"Link:" is not.

The point of "Link:" is explicitly two-fold:

 - it makes it quite obvious that you expect an actual valid web-link,
not some internal garbage

 - random people always want random extensions, and "Link:" is
_designed_ to counter-act that creeping "let's add a random new tag"
disease. It's very explicitly "any relevant link".

and I really question the value of adding new types of tags,
particularly ones that seem almost designed to be mis-used.

So I'm not violently against it, and 99% of the existing uses seem
fine. But I do note that some of the early "Closes:" tags in the
kernel were very much complete garbage, and exactly the kind of thing
that I absolutely detest.

What does

    Closes: 10437

mean? That's crazy talk. (And yes, in that case it was a
kernel.bugzilla.org number, which is perfectly fine, but I'm using it
as a very real example of how "Closes:" ends up being very naturally
to mis-use).

End result: I don't hate our current "Closes:" uses. But I'm very wary of it.

I'm not at all convinced that it really adds a lot of value over
"Link:", and I am, _very_ aware of how easily it can be then taken to
be a "let's use our own bug tracker cookies here".

So I will neither endorse nor condemn it, but if I see people using it
wrong, I will absolutely put my foot down.

                    Linus

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

[Bagas Sanjaya]

On 3/16/23 00:44, Matthieu Baerts wrote:
> +In the same category as linking web pages, a special tag is also used to close
> +issues but only when the mentioned ticketing system can do this operation
> +automatically::
> +
> +        Closes: https://example.com/issues/1234
> +
> +Please use this 'Closes:' tag only if it helps managing issues thanks to
> +automations. If not, pick the 'Link:' one.
> +

What about:

```
Similarly, there is also "Closes:" tag that can be used to close issues when
the underlying tracker can do this operation automatically. For example::

    Closes: <issue link>

For other trackers keep using "Link:" tag instead.
```
?

> +Note that it might be interesting to use the 'Closes:' tag instead of 'Link:'
> +if the URL points to an issue from public bug tracker that can automatically
> +close tickets when such patches containing this tag is accepted upstream. For
> +example::
> +
> +    Closes: https://example.com/issues/1234
> +

This one LGTM.

Thanks.

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

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

[Daniel Vetter]

On Thu, 16 Mar 2023 at 18:30, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Thu, Mar 16, 2023 at 4:43 AM Matthieu Baerts
> <matthieu.baerts@tessares.net> wrote:
> >
> > @Linus: in short, we would like to continue using the "Closes:" tag (or
> > similar, see below) with a URL in commit messages. They are useful to
> > have public bug trackers doing automated actions like closing a specific
> > ticket. Any objection from your side?
>
> As long as it's a public link, I guess that just documents what the
> drm people have been doing.
>
> I'm not convinced "Closes" is actually any better than just "Link:",
> though. I would very much hope and expect that the actual closing of
> any bug report is actually done separately and verified, rather than
> some kind of automated "well, the commit says it closes it, so.."
>
> So honestly, I feel like "Link:" is just a better thing, and I worry
> that "Closes:" is then going to be used for random internal crap.
> We've very much seen people wanting to do that - having their own
> private bug trackers, and then using the commit message to refer to
> them, which I am *violently* against. If it's only useful to some
> closed community, it shouldn't be in the public commits.

Yeah I think that's fine. The bot can then autogenerate a request in
the bug report to confirm that it's fixed, and ask the reporter to
close in that case. And then maybe if there's no message a few weeks
after the release, auto-close or something.

Bot needs to make sure it's only parsing tags for the instance it's
botting for anyway, so overloading Link: with all the meanings
(absolutely all themeanings!) is not really a problem since Closes:
has the same issue if different subsystems use it for different bug
tracking needs.

> And while the current GPU people seem to use "Closes:" the right way
> (and maybe some other groups do too - but it does seem to be mostly a
> freedesktop thing), I really think it is amenable to mis-use in ways
> "Link:" is not.

Huh I didn't realize this picked up. Way back we used Bugzilla: for
this sometimes, but I think just using Link: for everything and
letting instance-specific bots figure out whether it's relevant for
them should be perfectly fine. Humans should have no problem parsing
meaning out of a tag soup anyway (I mean we have Cc: stable meaning
backport after all, and I think that address is a blackhole).

I guess if you feel strongly we can percolate this a bit to
submaintainers and contributors in drm.
-Daniel

> The point of "Link:" is explicitly two-fold:
>
>  - it makes it quite obvious that you expect an actual valid web-link,
> not some internal garbage
>
>  - random people always want random extensions, and "Link:" is
> _designed_ to counter-act that creeping "let's add a random new tag"
> disease. It's very explicitly "any relevant link".
>
> and I really question the value of adding new types of tags,
> particularly ones that seem almost designed to be mis-used.
>
> So I'm not violently against it, and 99% of the existing uses seem
> fine. But I do note that some of the early "Closes:" tags in the
> kernel were very much complete garbage, and exactly the kind of thing
> that I absolutely detest.
>
> What does
>
>     Closes: 10437
>
> mean? That's crazy talk. (And yes, in that case it was a
> kernel.bugzilla.org number, which is perfectly fine, but I'm using it
> as a very real example of how "Closes:" ends up being very naturally
> to mis-use).
>
> End result: I don't hate our current "Closes:" uses. But I'm very wary of it.
>
> I'm not at all convinced that it really adds a lot of value over
> "Link:", and I am, _very_ aware of how easily it can be then taken to
> be a "let's use our own bug tracker cookies here".
>
> So I will neither endorse nor condemn it, but if I see people using it
> wrong, I will absolutely put my foot down.
>
>                     Linus



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Matthieu Baerts]

Hi Linus, Daniel, Konstantin,

@Linus, Daniel: Thank you both for your replies!

@Konstantin: I have one question for you at the end of this email if you
don't mind.

On 17/03/2023 17:58, Daniel Vetter wrote:
> On Thu, 16 Mar 2023 at 18:30, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>> On Thu, Mar 16, 2023 at 4:43 AM Matthieu Baerts
>> <matthieu.baerts@tessares.net> wrote:
>>>
>>> @Linus: in short, we would like to continue using the "Closes:" tag (or
>>> similar, see below) with a URL in commit messages. They are useful to
>>> have public bug trackers doing automated actions like closing a specific
>>> ticket. Any objection from your side?
>>
>> As long as it's a public link, I guess that just documents what the
>> drm people have been doing.
>>
>> I'm not convinced "Closes" is actually any better than just "Link:",
>> though. I would very much hope and expect that the actual closing of
>> any bug report is actually done separately and verified, rather than
>> some kind of automated "well, the commit says it closes it, so.."
>>
>> So honestly, I feel like "Link:" is just a better thing, and I worry
>> that "Closes:" is then going to be used for random internal crap.
>> We've very much seen people wanting to do that - having their own
>> private bug trackers, and then using the commit message to refer to
>> them, which I am *violently* against. If it's only useful to some
>> closed community, it shouldn't be in the public commits.
> 
> Yeah I think that's fine. The bot can then autogenerate a request in
> the bug report to confirm that it's fixed, and ask the reporter to
> close in that case. And then maybe if there's no message a few weeks
> after the release, auto-close or something.

That would be a nice behaviour indeed. That's just a shame it means we
cannot use the default behaviour of these bug trackers and we need a
dedicated bot instead. I don't know what's the behaviour with GitLab and
other bug trackers but with GitHub, when a commit is seen in a public
repo with a "Link:" tag pointing to an issue, a "special" comment is
added to the bug report but no notifications are sent. So if we don't
implement the bot you described, we will still have to do the tracking
manually.

I understand we can see that as an issue with the existing service but
it also means we cannot use their build-in automations.

Maybe it means we have to switch to Bugzilla and wait for the new bot :)
(but no, I don't want to add pressure on Konstantin ;) )


> Bot needs to make sure it's only parsing tags for the instance it's
> botting for anyway, so overloading Link: with all the meanings
> (absolutely all themeanings!) is not really a problem since Closes:
> has the same issue if different subsystems use it for different bug
> tracking needs.

Here, "Closes:" would be used exclusively with a URL to a specific bug
report, not just "Closes: #1234". Would this not work if different
subsystems use it?

An extra check could be added to checkpatch.pl to display a warning if
this "Closes:" tag is not used with a URL.

In the case of GitHub -- and GitLab if I'm not mistaken -- there are
some safeguards: the closure is only done if a commit having the
"Closes:" tag to the bug report is applied into a specific branch. In
other words, if someone applies the same patch elsewhere, the bug report
will not be closed automatically. Also in case of closure, a
notification is also sent and the bug report can be re-opened if
something wrong happened.

>> And while the current GPU people seem to use "Closes:" the right way
>> (and maybe some other groups do too - but it does seem to be mostly a
>> freedesktop thing), I really think it is amenable to mis-use in ways
>> "Link:" is not.
> 
> Huh I didn't realize this picked up. Way back we used Bugzilla: for
> this sometimes, but I think just using Link: for everything and
> letting instance-specific bots figure out whether it's relevant for
> them should be perfectly fine. Humans should have no problem parsing
> meaning out of a tag soup anyway (I mean we have Cc: stable meaning
> backport after all, and I think that address is a blackhole).
> 
> I guess if you feel strongly we can percolate this a bit to
> submaintainers and contributors in drm.

I understand the risks of being misused by some and I guess the main
point here is that we want to avoid exceptions.

On our side with MPTCP, if we can definitively no longer use the
"Closes:" tag, we will find alternatives. Probably by rewriting patches
containing them before sending patches to netdev. This way we can
continue to use the feature internally and when sent upstream, the
commits will contain Fixes tag instead :)


So correct me if I'm wrong but the conclusion is then to stop using the
"Closes:" tag to avoid misuses. In this case, we can of course drop this
series.

@Konstantin: would it be OK for your future Bugzilla bot to deal with
the generic "Link:" tag instead of the specific "Closes:" one?

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

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

[Konstantin Ryabitsev]

On Fri, Mar 17, 2023 at 07:41:04PM +0100, Matthieu Baerts wrote:
> @Konstantin: would it be OK for your future Bugzilla bot to deal with
> the generic "Link:" tag instead of the specific "Closes:" one?

Yes and no -- we can easily figure out that "it's a bugzilla link and it
points at this bug", but we can't make any decisions based on it. Just because
it's a bug that is mentioned in a commit doesn't really mean that the bug is
fixed and we should close it.

E.g. it could be something like:

    foo: initial workaround for bar

    This implements a workaround for problems with bar (see bug link below).
    It's not a complete fix, so further work is required to address all issues
    identified in the bug report.

    Link: https://bugzilla.kernel.org/show_bug.cgi?id=5551212

It would be wrong here to auto-close this bug, but we can certainly add a new
comment that says:

    This bug was mentioned in commit abcd1234:
    https://git.kernel.org/linus/abcd1234

-K

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

[Matthieu Baerts]

Hi Bagas,

On 17/03/2023 09:00, Bagas Sanjaya wrote:
> On 3/16/23 00:44, Matthieu Baerts wrote:
>> +In the same category as linking web pages, a special tag is also used to close
>> +issues but only when the mentioned ticketing system can do this operation
>> +automatically::
>> +
>> +        Closes: https://example.com/issues/1234
>> +
>> +Please use this 'Closes:' tag only if it helps managing issues thanks to
>> +automations. If not, pick the 'Link:' one.
>> +
> 
> What about:
> 
> ```
> Similarly, there is also "Closes:" tag that can be used to close issues when
> the underlying tracker can do this operation automatically. For example::
> 
>     Closes: <issue link>
> 
> For other trackers keep using "Link:" tag instead.
> ```
> ?

Thank you for your feedback! This suggestion looks better to me.

I might just have to mention "public bug tracker" to also address Jon's
comment: we don't want links to internal bug trackers.

Please note that it is still unclear to me if such exception for the
"Closes:" tag can be accepted: please see the discussions[1] on the
cover letter for more details about that.

Cheers,
Matt

[1]
https://lore.kernel.org/linux-doc/20230314-doc-checkpatch-closes-tag-v1-0-1b83072e9a9a@tessares.net/T/
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net