| Message ID | 20230314-doc-checkpatch-closes-tag-v1-0-1b83072e9a9a@tessares.net (mailing list archive) |
|---|---|
| Headers | show |
| Series | docs & checkpatch: allow Closes tags with links | expand |
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
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
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
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
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
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
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
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,