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/[email protected]/
Signed-off-by: Matthieu Baerts <[email protected]>
---
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,
--
Matthieu Baerts <[email protected]>
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], GitHub [2] and hopefully soon [3]
bugzilla.kernel.org when the appropriated tag is used. The two first
ones 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 [4]. It has also
been misused but that was a long time ago, when it was common to use
many different random tags.
checkpatch.pl script should then stop complaining about this "Closes"
tag. As suggested by Thorsten [5], if this tag is accepted, it should
first be described in the documentation. This is what is done here in
this patch.
To avoid confusion, the "Closes" can be used with any public links. No
need to check if the underlying bug tracker supports automations.
Having this tag with any kind of public bug reports allows bots like
regzbot to clearly identify patches fixing a specific bug and avoid
false-positives, e.g. patches mentioning it is related to an issue but
not fixing it.
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. Also, these bug trackers have some safeguards: the closure is
only done if a commit having the "Closes:" tag is applied in a specific
branch. It will then not be closed if a random commit having the same
tag is published elsewhere. Also in case of closure, a notification is
sent to the owners.
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/linux-doc/[email protected]/ [3]
Link: https://lore.kernel.org/all/CAHk-=wgs38ZrfPvy=nOwVkVzjpM3VFU1zobP37Fwd_h9iAD5JQ@mail.gmail.com/ [4]
Link: https://lore.kernel.org/all/[email protected]/ [5]
Link: https://github.com/multipath-tcp/mptcp_net-next/issues/373
Suggested-by: Thorsten Leemhuis <[email protected]>
Acked-by: Konstantin Ryabitsev <[email protected]>
Signed-off-by: Matthieu Baerts <[email protected]>
---
v3:
- Allow using the "Closes" tag with any bug reports, not only the ones
supporting automations, useful for regzbot, etc. (Thorsten Leemhuis)
v2:
- Add Konstantin's Acked-by: even if the patch has changed a bit, the
concept is still the same, I hope that's OK.
- Mention "public" in "5.Posting.rst" file as well. (Jonathan Corbet)
- Re-phrase the new text from "5.Posting.rst". (Bagas Sanjaya &
Thorsten Leemhuis)
- Clearly mention that private bug trackers and invalid URLs are
forbidden (Linus Torvalds).
---
Documentation/process/5.Posting.rst | 10 ++++++++++
Documentation/process/submitting-patches.rst | 10 ++++++++++
2 files changed, 20 insertions(+)
diff --git a/Documentation/process/5.Posting.rst b/Documentation/process/5.Posting.rst
index 7a670a075ab6..c755ca795f15 100644
--- a/Documentation/process/5.Posting.rst
+++ b/Documentation/process/5.Posting.rst
@@ -217,6 +217,16 @@ 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'.
+Similarly, there is also the "Closes:" tag that can be used to mark any
+kind of public bug report as closed. For example::
+
+ Closes: https://example.com/issues/1234
+
+Some bug trackers have the ability to close issues automatically when a
+commit with such a tag is applied. Some bots monitoring mailing lists can
+also track such tags and take certain actions. Private bug trackers and
+invalid URLs are forbidden.
+
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..b0ea03f18bad 100644
--- a/Documentation/process/submitting-patches.rst
+++ b/Documentation/process/submitting-patches.rst
@@ -134,6 +134,16 @@ 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.
+It might be interesting to use the 'Closes:' tag to mark any kind of public
+bug report as closed. For example::
+
+ Closes: https://example.com/issues/1234
+
+Some bug trackers have the ability to close issues automatically when a
+commit with such a tag is applied. Some bots monitoring mailing lists can
+also track such tags and take certain actions. Private bug trackers and
+invalid URLs are forbidden.
+
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
The following commit will allow the use of a similar "link" tag.
Because there is a possibility that other similar tags will be added in
the future and to reduce the number of places where the code will be
modified to allow this new tag, a list with all these "link" tags is now
used.
Two variables are created from it: one to search for such tags and one
to print all tags in a warning message.
Suggested-by: Joe Perches <[email protected]>
Signed-off-by: Matthieu Baerts <[email protected]>
---
scripts/checkpatch.pl | 32 ++++++++++++++++++++++++--------
1 file changed, 24 insertions(+), 8 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index bd44d12965c9..9d092ff4fc16 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -620,6 +620,22 @@ our $signature_tags = qr{(?xi:
Cc:
)};
+our @link_tags = qw(Link);
+
+#Create a search and print patterns for all these strings to be used directly below
+our $link_tags_search = "";
+our $link_tags_print = "";
+foreach my $entry (@link_tags) {
+ if ($link_tags_search ne "") {
+ $link_tags_search .= '|';
+ $link_tags_print .= ' or ';
+ }
+ $entry .= ':';
+ $link_tags_search .= $entry;
+ $link_tags_print .= "'$entry'";
+}
+$link_tags_search = "(?:${link_tags_search})";
+
our $tracing_logging_tags = qr{(?xi:
[=-]*> |
<[=-]* |
@@ -3158,14 +3174,14 @@ sub process {
}
}
-# check if Reported-by: is followed by a Link:
+# check if Reported-by: is followed by a link 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_tags_print to the report\n" . $herecurr . $rawlines[$linenr] . "\n");
+ } elsif ($rawlines[$linenr] !~ m{^$link_tags_search\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_tags_print with a URL to the report\n" . $herecurr . $rawlines[$linenr] . "\n");
}
}
}
@@ -3250,8 +3266,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_tags_search|$signature_tags)/i ||
+ # A Fixes:, link 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 +3282,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 !~ /^$link_tags_search$/) {
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_tags_print instead\n" . $herecurr);
}
}
--
2.39.2
As a follow-up of a previous patch modifying the documentation to
allow using the "Closes:" tag, checkpatch.pl is updated accordingly.
checkpatch.pl now no longer complain when the "Closes:" tag is used by
itself or after the "Reported-by:" 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 <[email protected]>
---
v3:
- split into 2 patches: the previous one adds a list with all the
"link" tags. This one only allows the "Closes" tag. (Joe Perches)
- "Closes" is no longer printed between parenthesis. (Thorsten
Leemhuis)
---
scripts/checkpatch.pl | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 9d092ff4fc16..ca58c734ff22 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -620,7 +620,7 @@ our $signature_tags = qr{(?xi:
Cc:
)};
-our @link_tags = qw(Link);
+our @link_tags = qw(Link Closes);
#Create a search and print patterns for all these strings to be used directly below
our $link_tags_search = "";
--
2.39.2
"Link:" and "Closes:" tags have to be used with public URLs.
It is difficult to make sure the link is public but at least we can
verify the tag is followed by 'http(s):'.
With that, we avoid such a tag that is not allowed [1]:
Closes: <number>
Link: https://lore.kernel.org/linux-doc/CAHk-=wh0v1EeDV3v8TzK81nDC40=XuTdY2MCr0xy3m3FiBV3+Q@mail.gmail.com/ [1]
Signed-off-by: Matthieu Baerts <[email protected]>
---
v3:
- new patch addressing Linus' concerns.
---
scripts/checkpatch.pl | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index ca58c734ff22..e3cafd2cb77a 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3292,6 +3292,17 @@ sub process {
}
}
+# Check for misuse of the link tags
+ if ($in_commit_log &&
+ $line =~ /^\s*(\w+:)\s*(\S+)/) {
+ my $tag = $1;
+ my $value = $2;
+ if ($tag =~ /^$link_tags_search$/ && $value !~ /^https?:/) {
+ WARN("COMMIT_LOG_WRONG_LINK",
+ "'$tag' should be followed by a public http(s) link\n" . $herecurr);
+ }
+ }
+
# Check for lines starting with a #
if ($in_commit_log && $line =~ /^#/) {
if (WARN("COMMIT_COMMENT_SYMBOL",
--
2.39.2
On Thu, 2023-03-30 at 20:13 +0200, Matthieu Baerts wrote:
> As a follow-up of a previous patch modifying the documentation to
> allow using the "Closes:" tag, checkpatch.pl is updated accordingly.
>
> checkpatch.pl now no longer complain when the "Closes:" tag is used by
> itself or after the "Reported-by:" tag.
>
> Fixes: 76f381bb77a0 ("checkpatch: warn when unknown tags are used for links")
> Fixes: d7f1d71e5ef6 ("checkpatch: warn when Reported-by: is not followed by Link:")
I don't think this _fixes_ anything.
I believe it's merely a new capability.
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/373
> Signed-off-by: Matthieu Baerts <[email protected]>
> ---
> v3:
> - split into 2 patches: the previous one adds a list with all the
> "link" tags. This one only allows the "Closes" tag. (Joe Perches)
> - "Closes" is no longer printed between parenthesis. (Thorsten
> Leemhuis)
> ---
> scripts/checkpatch.pl | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 9d092ff4fc16..ca58c734ff22 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -620,7 +620,7 @@ our $signature_tags = qr{(?xi:
> Cc:
> )};
>
> -our @link_tags = qw(Link);
> +our @link_tags = qw(Link Closes);
>
> #Create a search and print patterns for all these strings to be used directly below
> our $link_tags_search = "";
>
On 30.03.23 20:13, Matthieu Baerts wrote:
> "Link:" and "Closes:" tags have to be used with public URLs.
>
> It is difficult to make sure the link is public but at least we can
> verify the tag is followed by 'http(s):'.
>
> With that, we avoid such a tag that is not allowed [1]:
>
> Closes: <number>
>
> Link: https://lore.kernel.org/linux-doc/CAHk-=wh0v1EeDV3v8TzK81nDC40=XuTdY2MCr0xy3m3FiBV3+Q@mail.gmail.com/ [1]
> Signed-off-by: Matthieu Baerts <[email protected]>
> [...]
> +# Check for misuse of the link tags
> + if ($in_commit_log &&
> + $line =~ /^\s*(\w+:)\s*(\S+)/) {
> + my $tag = $1;
> + my $value = $2;
> + if ($tag =~ /^$link_tags_search$/ && $value !~ /^https?:/) {
> + WARN("COMMIT_LOG_WRONG_LINK",
> + "'$tag' should be followed by a public http(s) link\n" . $herecurr);
> + }
> + }
> +
I must be missing something here, but it looks to me like this is
checked twice now. See this line in patch2 (which is changed there, but
the check itself remains):
> } elsif ($rawlines[$linenr] !~ m{^link:\s*https?://}i) {
Ciao, Thorsten
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/[email protected]/
>
> Signed-off-by: Matthieu Baerts <[email protected]>
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/[email protected]/
-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);
}
}
Hi Joe,
Thank you for this review.
On 31/03/2023 00:43, Joe Perches wrote:
> On Thu, 2023-03-30 at 20:13 +0200, Matthieu Baerts wrote:
>> As a follow-up of a previous patch modifying the documentation to
>> allow using the "Closes:" tag, checkpatch.pl is updated accordingly.
>>
>> checkpatch.pl now no longer complain when the "Closes:" tag is used by
>> itself or after the "Reported-by:" tag.
>>
>> Fixes: 76f381bb77a0 ("checkpatch: warn when unknown tags are used for links")
>> Fixes: d7f1d71e5ef6 ("checkpatch: warn when Reported-by: is not followed by Link:")
>
> I don't think this _fixes_ anything.
> I believe it's merely a new capability.
When we first saw the new warnings checkpatch.pl was producing on a pre
Linux 6.3-rc1, we thought it was an issue with checkpatch: the "Closes:"
tag with a URL has been used for years without any complaints from
checkpatch and people as far as I know. At some point we had to stop
using it to please checkpatch and that's why we thought something had to
be fixed: we initially [1] thought the "Closes:" tag case had been
forgotten when the two mentioned commits had been created.
But I'm fine to see these "Fixes" tags removed, I understand the
"Closes:" tags were tolerated before but just not documented. Do I send
a v4 without these two "Fixes" tags? It means only the v6.3 would not
accept "Closes:" tags but we can work around that.
[1]
https://lore.kernel.org/all/[email protected]/
Cheers,
Matt
--
Tessares | Belgium | Hybrid Access Solutions
http://www.tessares.net
Hi Thorsten,
On 31/03/2023 10:57, Thorsten Leemhuis wrote:
> On 30.03.23 20:13, Matthieu Baerts wrote:
>> "Link:" and "Closes:" tags have to be used with public URLs.
>>
>> It is difficult to make sure the link is public but at least we can
>> verify the tag is followed by 'http(s):'.
>>
>> With that, we avoid such a tag that is not allowed [1]:
>>
>> Closes: <number>
>>
>> Link: https://lore.kernel.org/linux-doc/CAHk-=wh0v1EeDV3v8TzK81nDC40=XuTdY2MCr0xy3m3FiBV3+Q@mail.gmail.com/ [1]
>> Signed-off-by: Matthieu Baerts <[email protected]>
>> [...]
>> +# Check for misuse of the link tags
>> + if ($in_commit_log &&
>> + $line =~ /^\s*(\w+:)\s*(\S+)/) {
>> + my $tag = $1;
>> + my $value = $2;
>> + if ($tag =~ /^$link_tags_search$/ && $value !~ /^https?:/) {
>> + WARN("COMMIT_LOG_WRONG_LINK",
>> + "'$tag' should be followed by a public http(s) link\n" . $herecurr);
>> + }
>> + }
>> +
>
> I must be missing something here, but it looks to me like this is
> checked twice now. See this line in patch2 (which is changed there, but
> the check itself remains):
>
>> } elsif ($rawlines[$linenr] !~ m{^link:\s*https?://}i) {
If I'm not mistaken, we had the following checks:
- after Reported-by, there is a link tag (Link:|Closes:)
- (link tags can take more than 75 chars)
- tags followed by "http(s)://" are restricted to link ones
Then not: link tags (Link:|Closes:) are followed by "http(s):".
But maybe I missed something, the checkpatch.pl script is quite big, I
would not be surprised :-)
Cheers,
Matt
--
Tessares | Belgium | Hybrid Access Solutions
http://www.tessares.net
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...
On 31.03.23 11:44, Matthieu Baerts wrote:
> Hi Thorsten,
>
> On 31/03/2023 10:57, Thorsten Leemhuis wrote:
>> On 30.03.23 20:13, Matthieu Baerts wrote:
>>> "Link:" and "Closes:" tags have to be used with public URLs.
>>>
>>> It is difficult to make sure the link is public but at least we can
>>> verify the tag is followed by 'http(s):'.
>>>
>>> With that, we avoid such a tag that is not allowed [1]:
>>>
>>> Closes: <number>
>>>
>>> Link: https://lore.kernel.org/linux-doc/CAHk-=wh0v1EeDV3v8TzK81nDC40=XuTdY2MCr0xy3m3FiBV3+Q@mail.gmail.com/ [1]
>>> Signed-off-by: Matthieu Baerts <[email protected]>
>>> [...]
>>> +# Check for misuse of the link tags
>>> + if ($in_commit_log &&
>>> + $line =~ /^\s*(\w+:)\s*(\S+)/) {
>>> + my $tag = $1;
>>> + my $value = $2;
>>> + if ($tag =~ /^$link_tags_search$/ && $value !~ /^https?:/) {
>>> + WARN("COMMIT_LOG_WRONG_LINK",
>>> + "'$tag' should be followed by a public http(s) link\n" . $herecurr);
>>> + }
>>> + }
>>> +
>>
>> I must be missing something here, but it looks to me like this is
>> checked twice now. See this line in patch2 (which is changed there, but
>> the check itself remains):
>>
>>> } elsif ($rawlines[$linenr] !~ m{^link:\s*https?://}i) {
>
> If I'm not mistaken, we had the following checks:
>
> - after Reported-by, there is a link tag (Link:|Closes:)
>
> - (link tags can take more than 75 chars)
> - tags followed by "http(s)://" are restricted to link ones
>
> Then not: link tags (Link:|Closes:) are followed by "http(s):".
Not in general, afaics -- and ensuring that is likely wise, so thx for
this. But for Link: and Closes: tags after a Reported-by it is already
checked, that's what I meant (and didn't communicate well, sorry). It's
just a detail, but might be wise to do this in patch 4:
- } elsif ($rawlines[$linenr] !~ m{^$link_tags_search\s*https?://}i) {
+ } elsif ($rawlines[$linenr] !~ m{^$link_tags_search}i) {
(that's a line changed in patch2)
Ciao, Thorsten
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/[email protected]/
>>
>> Signed-off-by: Matthieu Baerts <[email protected]>
>
> 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
--
Tessares | Belgium | Hybrid Access Solutions
http://www.tessares.net
Hi Thorsten,
On 31/03/2023 12:09, Thorsten Leemhuis wrote:
>
>
> On 31.03.23 11:44, Matthieu Baerts wrote:
>> Hi Thorsten,
>>
>> On 31/03/2023 10:57, Thorsten Leemhuis wrote:
>>> On 30.03.23 20:13, Matthieu Baerts wrote:
>>>> "Link:" and "Closes:" tags have to be used with public URLs.
>>>>
>>>> It is difficult to make sure the link is public but at least we can
>>>> verify the tag is followed by 'http(s):'.
>>>>
>>>> With that, we avoid such a tag that is not allowed [1]:
>>>>
>>>> Closes: <number>
>>>>
>>>> Link: https://lore.kernel.org/linux-doc/CAHk-=wh0v1EeDV3v8TzK81nDC40=XuTdY2MCr0xy3m3FiBV3+Q@mail.gmail.com/ [1]
>>>> Signed-off-by: Matthieu Baerts <[email protected]>
>>>> [...]
>>>> +# Check for misuse of the link tags
>>>> + if ($in_commit_log &&
>>>> + $line =~ /^\s*(\w+:)\s*(\S+)/) {
>>>> + my $tag = $1;
>>>> + my $value = $2;
>>>> + if ($tag =~ /^$link_tags_search$/ && $value !~ /^https?:/) {
>>>> + WARN("COMMIT_LOG_WRONG_LINK",
>>>> + "'$tag' should be followed by a public http(s) link\n" . $herecurr);
>>>> + }
>>>> + }
>>>> +
>>>
>>> I must be missing something here, but it looks to me like this is
>>> checked twice now. See this line in patch2 (which is changed there, but
>>> the check itself remains):
>>>
>>>> } elsif ($rawlines[$linenr] !~ m{^link:\s*https?://}i) {
>>
>> If I'm not mistaken, we had the following checks:
>>
>> - after Reported-by, there is a link tag (Link:|Closes:)
>>
>> - (link tags can take more than 75 chars)
>> - tags followed by "http(s)://" are restricted to link ones
>>
>> Then not: link tags (Link:|Closes:) are followed by "http(s):".
>
> Not in general, afaics -- and ensuring that is likely wise, so thx for
> this. But for Link: and Closes: tags after a Reported-by it is already
> checked, that's what I meant (and didn't communicate well, sorry). It's
> just a detail, but might be wise to do this in patch 4:
>
> - } elsif ($rawlines[$linenr] !~ m{^$link_tags_search\s*https?://}i) {
> + } elsif ($rawlines[$linenr] !~ m{^$link_tags_search}i) {
>
> (that's a line changed in patch2)
OK thank you. Sorry I didn't get that. Indeed, it should be enough to
just check for the tags, not for the "http(s)://" part.
Cheers,
Matt
--
Tessares | Belgium | Hybrid Access Solutions
http://www.tessares.net
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