| Message ID | 20230314-doc-checkpatch-closes-tag-v3-0-d1bdcf31c71c@tessares.net (mailing list archive) |
|---|---|
| Headers | show |
| Series | docs & checkpatch: allow Closes tags with links | expand |
On 30.03.23 20:13, 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. > 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> Maybe it's just me, but I think those changes do not make it clear enough when to use Link: and when to use Closes. Find below an alternative proposal how I'd do it for consideration that goes 'all-in' for the sake of simplicity. [untested -- and I hope thunderbird won't mangle the patch] Ciao, Thorsten diff --git a/Documentation/process/5.Posting.rst b/Documentation/process/5.Posting.rst index 7a670a075ab6..fc194b4d1674 100644 --- a/Documentation/process/5.Posting.rst +++ b/Documentation/process/5.Posting.rst @@ -207,11 +207,17 @@ the patch:: Fixes: 1f2e3d4c5b6a ("The first line of the commit specified by the first 12 characters of its SHA-1 ID") Another tag is used for linking web pages with additional backgrounds or -details, for example a report about a bug fixed by the patch or a document +details, for example earlier discussion which lead to the patch or a document with a specification implemented by the patch:: Link: https://example.com/somewhere.html optional-other-stuff +If the URL points to a report about a bug fixed by the patch, use this instead:: + + Closes: https://example.com/somewhere.html optional-other-stuff + +Ensure any such links are publicly accessible. + Many maintainers when applying a patch also add this tag to link to the 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 @@ -251,7 +257,7 @@ The tags in common use are: - Reported-by: names a user who reported a problem which is fixed by this patch; this tag is used to give credit to the (often underappreciated) people who test our code and let us know when things do not work - correctly. Note, this tag should be followed by a Link: tag pointing to the + correctly. Note, this tag should be followed by a Closes: tag pointing to the report, unless the report is not available on the web. - Cc: the named person received a copy of the patch and had the diff --git a/Documentation/process/submitting-patches.rst b/Documentation/process/submitting-patches.rst index 69ce64e03c70..73611cf1c372 100644 --- a/Documentation/process/submitting-patches.rst +++ b/Documentation/process/submitting-patches.rst @@ -126,8 +126,10 @@ For example:: Link: https://lore.kernel.org/r/30th.anniversary.repost@klaava.Helsinki.FI/ -Please check the link to make sure that it is actually working and points -to the relevant message. +If the URL points to a bug report that is fixed by the patch, use 'Closes:' +instead. + +Ensure any such links are publicly accessible. However, try to make your explanation understandable without external resources. In addition to giving a URL to a mailing list archive or bug, @@ -498,7 +500,7 @@ Using Reported-by:, Tested-by:, Reviewed-by:, Suggested-by: and Fixes: The Reported-by tag gives credit to people who find bugs and report them and it hopefully inspires them to help us again in the future. The tag is intended for bugs; please do not use it to credit feature requests. The tag should be -followed by a Link: tag pointing to the report, unless the report is not +followed by a Closes: tag pointing to the report, unless the report is not available on the web. Please note that if the bug was reported in private, then ask for permission first before using the Reported-by tag. diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index bd44d12965c9..f9a7c2b856ae 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 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 Closes: to the report\n" . $herecurr . $rawlines[$linenr] . "\n"); + } elsif ($rawlines[$linenr] !~ m{^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 Closes: with a URL to the report\n" . $herecurr . $rawlines[$linenr] . "\n"); } } } @@ -3266,13 +3266,13 @@ sub process { # Check for odd tags before a URI/URL if ($in_commit_log && - $line =~ /^\s*(\w+):\s*http/ && $1 ne 'Link') { + $line =~ /^\s*(\w+):\s*http/ && $1 ne 'Link' && $1 ne 'Closes') { if ($1 =~ /^v(?:ersion)?\d+/i) { WARN("COMMIT_LOG_VERSIONING", "Patch version information should be after the --- line\n" . $herecurr); } else { WARN("COMMIT_LOG_USE_LINK", - "Unknown link reference '$1:', use 'Link:' instead\n" . $herecurr); + "Unknown link reference '$1:', use 'Link:' or 'Closes:' instead\n" . $herecurr); } }
On Fri, Mar 31, 2023 at 11:39:22AM +0200, Thorsten Leemhuis wrote: > -Please check the link to make sure that it is actually working and points > -to the relevant message. > +If the URL points to a bug report that is fixed by the patch, use 'Closes:' > +instead. This is not specifically a comment about your additional diff, but this sprang to mind (again) while reading it. I have been wondering if this sort of thing will lead to inconsistency. Reports sometimes report more than one issue at once. Other times a patch that is (intentionally) not a complete fix for the problem. Using Closes: in those cases is not really true, as it does not close the report. Having a series of N patches, each of which purport to close an issue, also doesn't seem quite right. The word Closes has a meaning and "forcing" the use of Closes: for reports implies meaning that may not be present. I suppose it is true that just because documentation or checkpatch says to do something, doesn't mean that you **have** to do it but I don't want to be the one on the Rx side of a rant...
Hi Thorsten, On 31/03/2023 11:39, Thorsten Leemhuis wrote: > On 30.03.23 20:13, 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. >> 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> > > Maybe it's just me, but I think those changes do not make it clear > enough when to use Link: and when to use Closes. Find below an > alternative proposal how I'd do it for consideration that goes > 'all-in' for the sake of simplicity. Thank you for the new proposition. I like your approach of forcing people to use the "Closes:" tag, I didn't think it would have been OK. I will wait a bit before sending a v4 just to get more feedback about that. The good thing with this approach is that it makes things clear. The "Closes:" tag is then no longer an alternative to "Link:" but a different tag, e.g. to be used after "Reported-by" as you did in your patch. I guess as any warnings from checkpatch.pl, it needs to be interpreted, e.g. if multiple bugs are reported in the same report as Conor mentioned. Cheers, Matt
On 31.03.23 12:08, Conor Dooley wrote: > On Fri, Mar 31, 2023 at 11:39:22AM +0200, Thorsten Leemhuis wrote: > >> -Please check the link to make sure that it is actually working and points >> -to the relevant message. >> +If the URL points to a bug report that is fixed by the patch, use 'Closes:' >> +instead. > > This is not specifically a comment about your additional diff, but this > sprang to mind (again) while reading it. > I have been wondering if this sort of thing will lead to inconsistency. > Reports sometimes report more than one issue at once. Other times a > patch that is (intentionally) not a complete fix for the problem. > Using Closes: in those cases is not really true, as it does not close > the report. > > Having a series of N patches, each of which purport to close an issue, > also doesn't seem quite right. > The word Closes has a meaning and "forcing" the use of Closes: for > reports implies meaning that may not be present. > > I suppose it is true that just because documentation or checkpatch says > to do something, doesn't mean that you **have** to do it but I don't > want to be the one on the Rx side of a rant... Yeah, maybe checkpath.pl should allow a "Link" after a "Reported-by" for cases like this, then developers could save "Closes" for the patch that addresses the last of the issues the report is about. OTOH checkpatch.pl currently just prints a warning, so developers could ignore this and do the above already now, as you say. Guess it depends on how often we expect "one report with multiple issue" to happen. Maybe this is an indicator that we are on the wrong track in general and should not do any of this and just stick to "Link:". Ciao, Thorsten
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 and regzbot would like to use it as well, I'm sending here new versions. I'm sorry if I misunderstood the comments from v1. Please tell me if I did. Changes in v3: - Patch 1/4 now allow using the "Closes" tag with any kind of bug reports, as long as the link is public. (Thorsten) - The former patch 2/2 has been split in two: first to use a list for the different "link" tags (Joe). Then to allow the 'Closes' tag. - A new patch has been added to let checkpatch.pl checking if "Closes" and "Links" are used with a URL. - Link to v2: https://lore.kernel.org/r/20230314-doc-checkpatch-closes-tag-v2-0-f4a417861f6d@tessares.net Changes in v2: - The text on patch 1/2 has been reworked thanks to Jon, Bagas and Thorsten. See the individual changelog on the patch for more details. - Private bug trackers and invalid URLs are clearly marked as forbidden to avoid being misused. (Linus) - 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 (4): docs: process: allow Closes tags with links checkpatch: use a list of "link" tags checkpatch: allow Closes tags with links checkpatch: check for misuse of the link tags Documentation/process/5.Posting.rst | 10 +++++++ Documentation/process/submitting-patches.rst | 10 +++++++ scripts/checkpatch.pl | 43 ++++++++++++++++++++++------ 3 files changed, 55 insertions(+), 8 deletions(-) --- base-commit: ffe78bbd512166e0ef1cc4858010b128c510ed7d change-id: 20230314-doc-checkpatch-closes-tag-1731b57556b1 Best regards,