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

Matthieu Baerts posted 2 patches 1 year, 2 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/multipath-tcp/mptcp_net-next tags/patchew/20230314-doc-checkpatch-closes-tag-v1-0-1b83072e9a9a@tessares.net
Maintainers: Jonathan Corbet <corbet@lwn.net>, Andy Whitcroft <apw@canonical.com>, Joe Perches <joe@perches.com>, Dwaipayan Ray <dwaipayanray1@gmail.com>, Lukas Bulwahn <lukas.bulwahn@gmail.com>, Thorsten Leemhuis <linux@leemhuis.info>, Andrew Morton <akpm@linux-foundation.org>, "Kai Wasserbäch" <kai@dev.carbon-project.org>
There is a newer version of this series
Documentation/process/5.Posting.rst          |  9 +++++++++
Documentation/process/submitting-patches.rst |  7 +++++++
scripts/checkpatch.pl                        | 16 ++++++++--------
3 files changed, 24 insertions(+), 8 deletions(-)
[PATCH 0/2] docs & checkpatch: allow Closes tags with links
Posted by Matthieu Baerts 1 year, 2 months ago
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>
Re: [PATCH 0/2] docs & checkpatch: allow Closes tags with links
Posted by Thorsten Leemhuis 1 year, 2 months ago
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
Posted by Konstantin Ryabitsev 1 year, 2 months ago
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
Posted by Matthieu Baerts 1 year, 2 months ago
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
Posted by Linus Torvalds 1 year, 2 months ago
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 0/2] docs & checkpatch: allow Closes tags with links
Posted by Daniel Vetter 1 year, 2 months ago
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
Posted by Matthieu Baerts 1 year, 2 months ago
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
Posted by Konstantin Ryabitsev 1 year, 2 months ago
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