[v3,0/4] docs & checkpatch: allow Closes tags with links

  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,
  

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