2021-11-22 07:33:48

by Thorsten Leemhuis

[permalink] [raw]
Subject: [RFC PATCH v1 1/1] docs: add the new commit-msg tags 'Reported:' and 'Reviewed:'

Introduce the tags 'Reported:' and 'Reviewed:' in addition to 'Link:',
as the latter is overloaded and hence doesn't indicate what the provided
URL is about. Documenting these also provides clarity, as a few
developers have used 'References:' to point to problem reports;
nevertheless 'Reported:' was chosen for this purpose, as it perfectly
matches up with the 'Reported-by:' tag commonly used already and needed
in this situation already.

Signed-off-by: Thorsten Leemhuis <[email protected]>
To: [email protected]
---
v1/RFC:
- first, *rough version* to see how this idea is received in the
community
---
Documentation/maintainer/configure-git.rst | 6 +--
Documentation/process/5.Posting.rst | 54 ++++++++++++++------
Documentation/process/submitting-patches.rst | 22 ++++----
3 files changed, 53 insertions(+), 29 deletions(-)

diff --git a/Documentation/maintainer/configure-git.rst b/Documentation/maintainer/configure-git.rst
index 80ae5030a590..8429d45d661c 100644
--- a/Documentation/maintainer/configure-git.rst
+++ b/Documentation/maintainer/configure-git.rst
@@ -40,12 +40,12 @@ Creating commit links to lore.kernel.org
The web site http://lore.kernel.org is meant as a grand archive of all mail
list traffic concerning or influencing the kernel development. Storing archives
of patches here is a recommended practice, and when a maintainer applies a
-patch to a subsystem tree, it is a good idea to provide a Link: tag with a
+patch to a subsystem tree, it is a good idea to provide a Reviewed: tag with a
reference back to the lore archive so that people that browse the commit
history can find related discussions and rationale behind a certain change.
The link tag will look like this:

- Link: https://lore.kernel.org/r/<message-id>
+ Reviewed: https://lore.kernel.org/r/<message-id>

This can be configured to happen automatically any time you issue ``git am``
by adding the following hook into your git:
@@ -56,7 +56,7 @@ by adding the following hook into your git:
$ cat >.git/hooks/applypatch-msg <<'EOF'
#!/bin/sh
. git-sh-setup
- perl -pi -e 's|^Message-Id:\s*<?([^>]+)>?$|Link: https://lore.kernel.org/r/$1|g;' "$1"
+ perl -pi -e 's|^Message-Id:\s*<?([^>]+)>?$|Reviewed: https://lore.kernel.org/r/$1|g;' "$1"
test -x "$GIT_DIR/hooks/commit-msg" &&
exec "$GIT_DIR/hooks/commit-msg" ${1+"$@"}
:
diff --git a/Documentation/process/5.Posting.rst b/Documentation/process/5.Posting.rst
index 855a70b80269..c5d7c982a373 100644
--- a/Documentation/process/5.Posting.rst
+++ b/Documentation/process/5.Posting.rst
@@ -197,53 +197,75 @@ the build process, for example, or editor backup files) in the patch. The
file "dontdiff" in the Documentation directory can help in this regard;
pass it to diff with the "-X" option.

-The tags mentioned above are used to describe how various developers have
-been associated with the development of this patch. They are described in
-detail in
-the :ref:`Documentation/process/submitting-patches.rst <submittingpatches>`
-document; what follows here is a brief summary. Each of these lines has
-the format:
+The tags already briefly mentioned above are used to provide insights how
+the patch came into being. They are described in detail in the
+:ref:`Documentation/process/submitting-patches.rst <submittingpatches>`
+document; what follows here is a brief summary.

-::
+One tag is used to refer to earlier commits which had problems fixed by
+the patch::
+
+ Fixes: 1f2e3d4c5b6a ("The first line of the commit specified by the first 12 characters of its SHA-1 ID")
+
+A second kind of tags is used to link webpages with additional details. There
+are three different tags of this sort which all use the following format::
+
+ Reported: https://example.com/somewhere.html optional-other-stuff
+
+The tags in common use are:
+
+ - ``Reported:`` points to a report of a problem fixed by this patch. The
+ provided URL thus might point to a entry in a bug tracker or a mail in a
+ mailing list archive. Typically this tag is followed by a "Reported-by:"
+ tag (see below).
+
+ - ``Link:`` points to websites providing additional backgrounds or details,
+ for example a document with a specification implemented by the patch.
+
+ - ``Reviewed:`` ignore this, as maintainers add it when applying a patch, to
+ make the commit point to the latest public review of the patch.
+
+A third kind of tags are used to document which developers were involved in
+the development of the patch. Each of these uses this format::

tag: Full Name <email address> optional-other-stuff

The tags in common use are:

- - Signed-off-by: this is a developer's certification that he or she has
+ - ``Signed-off-by:`` is a developer's certification that he or she has
the right to submit the patch for inclusion into the kernel. It is an
agreement to the Developer's Certificate of Origin, the full text of
which can be found in :ref:`Documentation/process/submitting-patches.rst <submittingpatches>`
Code without a proper signoff cannot be merged into the mainline.

- - Co-developed-by: states that the patch was co-created by several developers;
+ - ``Co-developed-by:`` states that the patch was co-created by several developers;
it is a used to give attribution to co-authors (in addition to the author
attributed by the From: tag) when multiple people work on a single patch.
Every Co-developed-by: must be immediately followed by a Signed-off-by: of
the associated co-author. Details and examples can be found in
:ref:`Documentation/process/submitting-patches.rst <submittingpatches>`.

- - Acked-by: indicates an agreement by another developer (often a
+ - ``Acked-by:`` indicates an agreement by another developer (often a
maintainer of the relevant code) that the patch is appropriate for
inclusion into the kernel.

- - Tested-by: states that the named person has tested the patch and found
+ - ``Tested-by:`` states that the named person has tested the patch and found
it to work.

- - Reviewed-by: the named developer has reviewed the patch for correctness;
+ - ``Reviewed-by:`` the named developer has reviewed the patch for correctness;
see the reviewer's statement in :ref:`Documentation/process/submitting-patches.rst <submittingpatches>`
for more detail.

- - Reported-by: names a user who reported a problem which is fixed by this
+ - ``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.

- - Cc: the named person received a copy of the patch and had the
+ - ``Cc:`` the named person received a copy of the patch and had the
opportunity to comment on it.

-Be careful in the addition of tags to your patches: only Cc: is appropriate
-for addition without the explicit permission of the person named.
+Be careful when adding this sort of tags to your patches: only Cc: is
+appropriate for addition without the explicit permission of the person named.


Sending the patch
diff --git a/Documentation/process/submitting-patches.rst b/Documentation/process/submitting-patches.rst
index da085d63af9b..b7c9155da9bd 100644
--- a/Documentation/process/submitting-patches.rst
+++ b/Documentation/process/submitting-patches.rst
@@ -112,20 +112,22 @@ collisions with shorter IDs a real possibility. Bear in mind that, even if
there is no collision with your six-character ID now, that condition may
change five years from now.

-If related discussions or any other background information behind the change
-can be found on the web, add 'Link:' tags pointing to it. In case your patch
-fixes a bug, for example, add a tag with a URL referencing the report in the
-mailing list archives or a bug tracker; if the patch is a result of some
-earlier mailing list discussion or something documented on the web, point to
-it.
+Add tags linking to any related discussions or background information behind
+the change on the web. For example, if your patch fixes a bug, add a
+`Reported:` tag pointing to the report in the mailing list archives or a bug
+tracker::

-When linking to mailing list archives, preferably use the lore.kernel.org
-message archiver service. To create the link URL, use the contents of the
-``Message-Id`` header of the message without the surrounding angle brackets.
-For example::
+ Reported: https://lore.kernel.org/r/[email protected]/
+
+If the patch is a related to some earlier mailing list discussion or something
+documented on the web, point to it using a `Link:` tag::

Link: https://lore.kernel.org/r/[email protected]/

+When linking to mailing list archives, preferably use the lore.kernel.org
+message archiver service. To create the link URL, use the contents of the
+``Message-Id`` header of the message without the surrounding angle brackets,
+e.g. "[email protected]" in the two examples above.
Please check the link to make sure that it is actually working and points
to the relevant message.

--
2.31.1



2021-11-22 16:29:20

by Steven Rostedt

[permalink] [raw]
Subject: Re: [RFC PATCH v1 1/1] docs: add the new commit-msg tags 'Reported:' and 'Reviewed:'

On Mon, 22 Nov 2021 08:33:42 +0100
Thorsten Leemhuis <[email protected]> wrote:

> Introduce the tags 'Reported:' and 'Reviewed:' in addition to 'Link:',
> as the latter is overloaded and hence doesn't indicate what the provided
> URL is about. Documenting these also provides clarity, as a few
> developers have used 'References:' to point to problem reports;
> nevertheless 'Reported:' was chosen for this purpose, as it perfectly
> matches up with the 'Reported-by:' tag commonly used already and needed
> in this situation already.

I like the differences between "Reorted:" and "Reviewed:", although I may
keep the "Link" instead of Reviewed because my automate scripts just give
the link of the patch, and there's seldom a review attached to it :-/

That said, I would like a way to have versions show a link to the last
version that was reviewed.

v1: has no tags

v2: has a Reviewed: tag to v1.

v3: has a Reviewed: tag to v2

[...]

Then the final commit could have a "Link" or "Reviewed" tag to v3, even
though there may not be any reviews to v3, but v3 has the link to v2, and
v2 has the link to v1, etc.

-- Steve

2021-11-22 18:50:47

by Thorsten Leemhuis

[permalink] [raw]
Subject: Re: [RFC PATCH v1 1/1] docs: add the new commit-msg tags 'Reported:' and 'Reviewed:'

On 22.11.21 17:29, Steven Rostedt wrote:
> On Mon, 22 Nov 2021 08:33:42 +0100
> Thorsten Leemhuis <[email protected]> wrote:
>
>> Introduce the tags 'Reported:' and 'Reviewed:' in addition to 'Link:',
>> as the latter is overloaded and hence doesn't indicate what the provided
>> URL is about. Documenting these also provides clarity, as a few
>> developers have used 'References:' to point to problem reports;
>> nevertheless 'Reported:' was chosen for this purpose, as it perfectly
>> matches up with the 'Reported-by:' tag commonly used already and needed
>> in this situation already.
>
> I like the differences between "Reorted:" and "Reviewed:", although I may
> keep the "Link" instead of Reviewed because my automate scripts just give
> the link of the patch, and there's seldom a review attached to it :-/

/me wonders if "Merge Request:" would be a better tag, but at least for
now settles on 'it's nice, as it's close to what people are used from
git forges, but OTOH it somehow feels wrong'

> That said, I would like a way to have versions show a link to the last
> version that was reviewed.
>
> v1: has no tags
>
> v2: has a Reviewed: tag to v1.
>
> v3: has a Reviewed: tag to v2
>
> [...]
>
> Then the final commit could have a "Link" or "Reviewed" tag to v3, even
> though there may not be any reviews to v3, but v3 has the link to v2, and
> v2 has the link to v1, etc.

Is that really worth it? Isn't it sufficient if the commit links to the
last public review posting, as that already should link to all earlier
review postings. Sure, not everybody is doing this right now, but maybe
just educating people to do so is better than creating something new.

Ciao, Thorsten

2021-11-22 20:24:34

by Steven Rostedt

[permalink] [raw]
Subject: Re: [RFC PATCH v1 1/1] docs: add the new commit-msg tags 'Reported:' and 'Reviewed:'

On Mon, 22 Nov 2021 19:50:35 +0100
Thorsten Leemhuis <[email protected]> wrote:

> > That said, I would like a way to have versions show a link to the last
> > version that was reviewed.
> >
> > v1: has no tags
> >
> > v2: has a Reviewed: tag to v1.
> >
> > v3: has a Reviewed: tag to v2
> >
> > [...]
> >
> > Then the final commit could have a "Link" or "Reviewed" tag to v3, even
> > though there may not be any reviews to v3, but v3 has the link to v2, and
> > v2 has the link to v1, etc.
>
> Is that really worth it? Isn't it sufficient if the commit links to the
> last public review posting, as that already should link to all earlier
> review postings. Sure, not everybody is doing this right now, but maybe
> just educating people to do so is better than creating something new.

Isn't "as that already should link to all earlier review postings" what I'm
suggesting above? I haven't seen many people do that yet.

-- Steve



2021-11-23 08:53:41

by Thorsten Leemhuis

[permalink] [raw]
Subject: Re: [RFC PATCH v1 1/1] docs: add the new commit-msg tags 'Reported:' and 'Reviewed:'



On 22.11.21 21:24, Steven Rostedt wrote:
> On Mon, 22 Nov 2021 19:50:35 +0100
> Thorsten Leemhuis <[email protected]> wrote:
>
>>> That said, I would like a way to have versions show a link to the last
>>> version that was reviewed.
>>>
>>> v1: has no tags
>>>
>>> v2: has a Reviewed: tag to v1.
>>>
>>> v3: has a Reviewed: tag to v2
>>>
>>> [...]
>>>
>>> Then the final commit could have a "Link" or "Reviewed" tag to v3, even
>>> though there may not be any reviews to v3, but v3 has the link to v2, and
>>> v2 has the link to v1, etc.
>>
>> Is that really worth it? Isn't it sufficient if the commit links to the
>> last public review posting, as that already should link to all earlier
>> review postings. Sure, not everybody is doing this right now, but maybe
>> just educating people to do so is better than creating something new.
>
> Isn't "as that already should link to all earlier review postings" what I'm
> suggesting above? I haven't seen many people do that yet.

Yeah, you are right, sorry, my perception was wrong.

Any maybe I got your suggestion wrong, but what you suggested sounded to
me like "each patch should link to the previous submission of the
patch". I just wonder if it's way easier and sufficient if just the
cover letter links to the previous or all earlier submissions of the
series in its revision history (sorry, I didn't should have made this
more obvious in my earlier mail); for patches without a cover letter
this obligation obviously would shift to the patch.

IOW: I have something in mind like in this submission:
https://lore.kernel.org/lkml/[email protected]/

Only the cover letter links to the earlier version, not the individual
patches.

But I have no strong feeling here, I don't care much about this.

Ciao, Thorsten

2021-11-23 18:52:40

by Eric Wong

[permalink] [raw]
Subject: Re: [RFC PATCH v1 1/1] docs: add the new commit-msg tags 'Reported:' and 'Reviewed:'

Thorsten Leemhuis <[email protected]> wrote:
> diff --git a/Documentation/maintainer/configure-git.rst b/Documentation/maintainer/configure-git.rst
> index 80ae5030a590..8429d45d661c 100644
> --- a/Documentation/maintainer/configure-git.rst
> +++ b/Documentation/maintainer/configure-git.rst

<snip>, +cc git@vger

> @@ -56,7 +56,7 @@ by adding the following hook into your git:
> $ cat >.git/hooks/applypatch-msg <<'EOF'
> #!/bin/sh
> . git-sh-setup
> - perl -pi -e 's|^Message-Id:\s*<?([^>]+)>?$|Link: https://lore.kernel.org/r/$1|g;' "$1"
> + perl -pi -e 's|^Message-Id:\s*<?([^>]+)>?$|Reviewed: https://lore.kernel.org/r/$1|g;' "$1"

Side note: that regexp should match "Message-ID" case-insensitively.
git send-email is an outlier in its capitalization of "Message-Id",
most RFCs capitalize it "Message-ID", as do common MUAs.

git send-email's capitalization does annoy me and I've looked
into changing it; but there's a bunch of tests and probably
dependent code that also need to be updated...

2021-11-24 01:37:21

by Junio C Hamano

[permalink] [raw]
Subject: Re: [RFC PATCH v1 1/1] docs: add the new commit-msg tags 'Reported:' and 'Reviewed:'

Eric Wong <[email protected]> writes:

> git send-email's capitalization does annoy me and I've looked
> into changing it; but there's a bunch of tests and probably
> dependent code that also need to be updated...

It does annoy me, too, and I do not mind if it gets "fixed", but I
do not know if the cost for vetting a bulk update like that is worth
it. There is another one outside send-email in log-tree.c that is
responsible for output from format-patch.

Here is the extent of damage in my trial change that seems to pass
the tests locally.

Documentation/MyFirstContribution.txt | 14 ++++-----
Documentation/git-format-patch.txt | 4 +--
Documentation/git-send-email.txt | 2 +-
git-send-email.perl | 4 +--
log-tree.c | 2 +-
mailinfo.c | 4 +--
t/t4014-format-patch.sh | 56 +++++++++++++++++------------------
t/t4150-am.sh | 8 ++---
t/t4258/mbox | 2 +-
t/t5100/msg0002 | 2 +-
t/t5100/msg0003 | 2 +-
t/t5100/msg0012--message-id | 2 +-
t/t5100/quoted-cr.mbox | 4 +--
t/t5100/sample.mbox | 6 ++--
t/t9001-send-email.sh | 38 ++++++++++++------------
15 files changed, 75 insertions(+), 75 deletions(-)

diff --git i/Documentation/MyFirstContribution.txt w/Documentation/MyFirstContribution.txt
index b20bc8e914..91cb204d52 100644
--- i/Documentation/MyFirstContribution.txt
+++ w/Documentation/MyFirstContribution.txt
@@ -1071,21 +1071,21 @@ between your last version and now, if it's something significant. You do not
need the exact same body in your second cover letter; focus on explaining to
reviewers the changes you've made that may not be as visible.

-You will also need to go and find the Message-Id of your previous cover letter.
+You will also need to go and find the Message-ID of your previous cover letter.
You can either note it when you send the first series, from the output of `git
send-email`, or you can look it up on the
https://lore.kernel.org/git[mailing list]. Find your cover letter in the
-archives, click on it, then click "permalink" or "raw" to reveal the Message-Id
+archives, click on it, then click "permalink" or "raw" to reveal the Message-ID
header. It should match:

----
-Message-Id: <[email protected]>
+Message-ID: <[email protected]>
----

-Your Message-Id is `<[email protected]>`. This example will be used
-below as well; make sure to replace it with the correct Message-Id for your
-**previous cover letter** - that is, if you're sending v2, use the Message-Id
-from v1; if you're sending v3, use the Message-Id from v2.
+Your Message-ID is `<[email protected]>`. This example will be used
+below as well; make sure to replace it with the correct Message-ID for your
+**previous cover letter** - that is, if you're sending v2, use the Message-ID
+from v1; if you're sending v3, use the Message-ID from v2.

While you're looking at the email, you should also note who is CC'd, as it's
common practice in the mailing list to keep all CCs on a thread. You can add
diff --git i/Documentation/git-format-patch.txt w/Documentation/git-format-patch.txt
index 113eabc107..daf911f249 100644
--- i/Documentation/git-format-patch.txt
+++ w/Documentation/git-format-patch.txt
@@ -99,7 +99,7 @@ To omit patch numbers from the subject, use `-N`.

If given `--thread`, `git-format-patch` will generate `In-Reply-To` and
`References` headers to make the second and subsequent patch mails appear
-as replies to the first mail; this also generates a `Message-Id` header to
+as replies to the first mail; this also generates a `Message-ID` header to
reference.

OPTIONS
@@ -163,7 +163,7 @@ include::diff-options.txt[]
--no-thread::
Controls addition of `In-Reply-To` and `References` headers to
make the second and subsequent mails appear as replies to the
- first. Also controls generation of the `Message-Id` header to
+ first. Also controls generation of the `Message-ID` header to
reference.
+
The optional <style> argument can be either `shallow` or `deep`.
diff --git i/Documentation/git-send-email.txt w/Documentation/git-send-email.txt
index 3db4eab4ba..086f132229 100644
--- i/Documentation/git-send-email.txt
+++ w/Documentation/git-send-email.txt
@@ -91,7 +91,7 @@ See the CONFIGURATION section for `sendemail.multiEdit`.

--in-reply-to=<identifier>::
Make the first mail (or all the mails with `--no-thread`) appear as a
- reply to the given Message-Id, which avoids breaking threads to
+ reply to the given Message-ID, which avoids breaking threads to
provide a new patch series.
The second and subsequent emails will be sent as replies according to
the `--[no-]chain-reply-to` setting.
diff --git i/git-send-email.perl w/git-send-email.perl
index 5262d88ee3..a61134c7d3 100755
--- i/git-send-email.perl
+++ w/git-send-email.perl
@@ -1494,7 +1494,7 @@ sub send_message {
To: $to${ccline}
Subject: $subject
Date: $date
-Message-Id: $message_id
+Message-ID: $message_id
";
if ($use_xmailer) {
$header .= "X-Mailer: git-send-email $gitversion\n";
@@ -1789,7 +1789,7 @@ sub process_file {
$has_mime_version = 1;
push @xh, $_;
}
- elsif (/^Message-Id: (.*)/i) {
+ elsif (/^Message-ID: (.*)/i) {
$message_id = $1;
}
elsif (/^Content-Transfer-Encoding: (.*)/i) {
diff --git i/log-tree.c w/log-tree.c
index 644893fd8c..818cea5f12 100644
--- i/log-tree.c
+++ w/log-tree.c
@@ -428,7 +428,7 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,
fprintf(opt->diffopt.file, "From %s Mon Sep 17 00:00:00 2001\n", name);
graph_show_oneline(opt->graph);
if (opt->message_id) {
- fprintf(opt->diffopt.file, "Message-Id: <%s>\n", opt->message_id);
+ fprintf(opt->diffopt.file, "Message-ID: <%s>\n", opt->message_id);
graph_show_oneline(opt->graph);
}
if (opt->ref_message_ids && opt->ref_message_ids->nr > 0) {
diff --git i/mailinfo.c w/mailinfo.c
index 02f6f95357..855349cc0e 100644
--- i/mailinfo.c
+++ w/mailinfo.c
@@ -597,7 +597,7 @@ static int check_header(struct mailinfo *mi,
ret = 1;
goto check_header_out;
}
- if (parse_header(line, "Message-Id", mi, &sb)) {
+ if (parse_header(line, "Message-ID", mi, &sb)) {
if (mi->add_message_id)
mi->message_id = strbuf_detach(&sb, NULL);
ret = 1;
@@ -829,7 +829,7 @@ static int handle_commit_msg(struct mailinfo *mi, struct strbuf *line)
if (patchbreak(line)) {
if (mi->message_id)
strbuf_addf(&mi->log_message,
- "Message-Id: %s\n", mi->message_id);
+ "Message-ID: %s\n", mi->message_id);
return 1;
}

diff --git i/t/t4014-format-patch.sh w/t/t4014-format-patch.sh
index 712d4b5ddf..973899d165 100755
--- i/t/t4014-format-patch.sh
+++ w/t/t4014-format-patch.sh
@@ -445,13 +445,13 @@ test_expect_success 'no threading' '

cat >expect.thread <<EOF
---
-Message-Id: <0>
+Message-ID: <0>
---
-Message-Id: <1>
+Message-ID: <1>
In-Reply-To: <0>
References: <0>
---
-Message-Id: <2>
+Message-ID: <2>
In-Reply-To: <0>
References: <0>
EOF
@@ -462,15 +462,15 @@ test_expect_success 'thread' '

cat >expect.in-reply-to <<EOF
---
-Message-Id: <0>
+Message-ID: <0>
In-Reply-To: <1>
References: <1>
---
-Message-Id: <2>
+Message-ID: <2>
In-Reply-To: <1>
References: <1>
---
-Message-Id: <3>
+Message-ID: <3>
In-Reply-To: <1>
References: <1>
EOF
@@ -482,17 +482,17 @@ test_expect_success 'thread in-reply-to' '

cat >expect.cover-letter <<EOF
---
-Message-Id: <0>
+Message-ID: <0>
---
-Message-Id: <1>
+Message-ID: <1>
In-Reply-To: <0>
References: <0>
---
-Message-Id: <2>
+Message-ID: <2>
In-Reply-To: <0>
References: <0>
---
-Message-Id: <3>
+Message-ID: <3>
In-Reply-To: <0>
References: <0>
EOF
@@ -503,21 +503,21 @@ test_expect_success 'thread cover-letter' '

cat >expect.cl-irt <<EOF
---
-Message-Id: <0>
+Message-ID: <0>
In-Reply-To: <1>
References: <1>
---
-Message-Id: <2>
+Message-ID: <2>
In-Reply-To: <0>
References: <1>
<0>
---
-Message-Id: <3>
+Message-ID: <3>
In-Reply-To: <0>
References: <1>
<0>
---
-Message-Id: <4>
+Message-ID: <4>
In-Reply-To: <0>
References: <1>
<0>
@@ -535,13 +535,13 @@ test_expect_success 'thread explicit shallow' '

cat >expect.deep <<EOF
---
-Message-Id: <0>
+Message-ID: <0>
---
-Message-Id: <1>
+Message-ID: <1>
In-Reply-To: <0>
References: <0>
---
-Message-Id: <2>
+Message-ID: <2>
In-Reply-To: <1>
References: <0>
<1>
@@ -553,16 +553,16 @@ test_expect_success 'thread deep' '

cat >expect.deep-irt <<EOF
---
-Message-Id: <0>
+Message-ID: <0>
In-Reply-To: <1>
References: <1>
---
-Message-Id: <2>
+Message-ID: <2>
In-Reply-To: <0>
References: <1>
<0>
---
-Message-Id: <3>
+Message-ID: <3>
In-Reply-To: <2>
References: <1>
<0>
@@ -576,18 +576,18 @@ test_expect_success 'thread deep in-reply-to' '

cat >expect.deep-cl <<EOF
---
-Message-Id: <0>
+Message-ID: <0>
---
-Message-Id: <1>
+Message-ID: <1>
In-Reply-To: <0>
References: <0>
---
-Message-Id: <2>
+Message-ID: <2>
In-Reply-To: <1>
References: <0>
<1>
---
-Message-Id: <3>
+Message-ID: <3>
In-Reply-To: <2>
References: <0>
<1>
@@ -600,22 +600,22 @@ test_expect_success 'thread deep cover-letter' '

cat >expect.deep-cl-irt <<EOF
---
-Message-Id: <0>
+Message-ID: <0>
In-Reply-To: <1>
References: <1>
---
-Message-Id: <2>
+Message-ID: <2>
In-Reply-To: <0>
References: <1>
<0>
---
-Message-Id: <3>
+Message-ID: <3>
In-Reply-To: <2>
References: <1>
<0>
<2>
---
-Message-Id: <4>
+Message-ID: <4>
In-Reply-To: <3>
References: <1>
<0>
diff --git i/t/t4150-am.sh w/t/t4150-am.sh
index 2aaaa0d7de..f41418c6e9 100755
--- i/t/t4150-am.sh
+++ w/t/t4150-am.sh
@@ -103,7 +103,7 @@ test_expect_success setup '

git format-patch --stdout first >patch1 &&
{
- echo "Message-Id: <[email protected]>" &&
+ echo "Message-ID: <[email protected]>" &&
echo "X-Fake-Field: Line One" &&
echo "X-Fake-Field: Line Two" &&
echo "X-Fake-Field: Line Three" &&
@@ -916,7 +916,7 @@ test_expect_success 'am --message-id really adds the message id' '
git am --message-id patch1.eml &&
test_path_is_missing .git/rebase-apply &&
git cat-file commit HEAD | tail -n1 >actual &&
- grep Message-Id patch1.eml >expected &&
+ grep Message-ID patch1.eml >expected &&
test_cmp expected actual
'

@@ -928,7 +928,7 @@ test_expect_success 'am.messageid really adds the message id' '
git am patch1.eml &&
test_path_is_missing .git/rebase-apply &&
git cat-file commit HEAD | tail -n1 >actual &&
- grep Message-Id patch1.eml >expected &&
+ grep Message-ID patch1.eml >expected &&
test_cmp expected actual
'

@@ -939,7 +939,7 @@ test_expect_success 'am --message-id -s signs off after the message id' '
git am -s --message-id patch1.eml &&
test_path_is_missing .git/rebase-apply &&
git cat-file commit HEAD | tail -n2 | head -n1 >actual &&
- grep Message-Id patch1.eml >expected &&
+ grep Message-ID patch1.eml >expected &&
test_cmp expected actual
'

diff --git i/t/t4258/mbox w/t/t4258/mbox
index c62819f3d2..1ae528ba78 100644
--- i/t/t4258/mbox
+++ w/t/t4258/mbox
@@ -2,7 +2,7 @@ From: A U Thor <[email protected]>
To: [email protected]
Subject: [PATCH v2] sample
Date: Mon, 3 Aug 2020 22:40:55 +0700
-Message-Id: <[email protected]>
+Message-ID: <[email protected]>
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: base64

diff --git i/t/t5100/msg0002 w/t/t5100/msg0002
index e2546ec733..1089382425 100644
--- i/t/t5100/msg0002
+++ w/t/t5100/msg0002
@@ -3,7 +3,7 @@ message:

From: Nit Picker <[email protected]>
Subject: foo is too old
-Message-Id: <[email protected]>
+Message-ID: <[email protected]>

Hopefully this would fix the problem stated there.

diff --git i/t/t5100/msg0003 w/t/t5100/msg0003
index 1ac68101b1..3402b534a6 100644
--- i/t/t5100/msg0003
+++ w/t/t5100/msg0003
@@ -3,7 +3,7 @@ message:

From: Nit Picker <[email protected]>
Subject: foo is too old
-Message-Id: <[email protected]>
+Message-ID: <[email protected]>

Hopefully this would fix the problem stated there.

diff --git i/t/t5100/msg0012--message-id w/t/t5100/msg0012--message-id
index 376e26e9ae..44482958ce 100644
--- i/t/t5100/msg0012--message-id
+++ w/t/t5100/msg0012--message-id
@@ -5,4 +5,4 @@ docutils заменён на python-docutils
python-docutils. В то время как сам rest2web не нужен.

Signed-off-by: Dmitriy Blinov <[email protected]>
-Message-Id: <[email protected]>
+Message-ID: <[email protected]>
diff --git i/t/t5100/quoted-cr.mbox w/t/t5100/quoted-cr.mbox
index 909021bb7a..a529d4de08 100644
--- i/t/t5100/quoted-cr.mbox
+++ w/t/t5100/quoted-cr.mbox
@@ -3,7 +3,7 @@ From: A U Thor <[email protected]>
To: [email protected]
Subject: [PATCH v2] sample
Date: Mon, 3 Aug 2020 22:40:55 +0700
-Message-Id: <[email protected]>
+Message-ID: <[email protected]>
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: base64

@@ -27,7 +27,7 @@ From: A U Thor <[email protected]>
To: [email protected]
Subject: [PATCH v2] sample
Date: Mon, 3 Aug 2020 22:40:55 +0700
-Message-Id: <[email protected]>
+Message-ID: <[email protected]>
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: base64

diff --git i/t/t5100/sample.mbox w/t/t5100/sample.mbox
index 6d4d0e4474..4a54ee5171 100644
--- i/t/t5100/sample.mbox
+++ w/t/t5100/sample.mbox
@@ -35,7 +35,7 @@ message:

From: Nit Picker <[email protected]>
Subject: foo is too old
-Message-Id: <[email protected]>
+Message-ID: <[email protected]>

Hopefully this would fix the problem stated there.

@@ -78,7 +78,7 @@ message:

From: Nit Picker <[email protected]>
Subject: foo is too old
-Message-Id: <[email protected]>
+Message-ID: <[email protected]>

Hopefully this would fix the problem stated there.

@@ -508,7 +508,7 @@ From [email protected] Wed Nov 12 17:54:41 2008
From: Dmitriy Blinov <[email protected]>
To: [email protected]
Date: Wed, 12 Nov 2008 17:54:41 +0300
-Message-Id: <[email protected]>
+Message-ID: <[email protected]>
X-Mailer: git-send-email 1.5.6.5
MIME-Version: 1.0
Content-Type: text/plain;
diff --git i/t/t9001-send-email.sh w/t/t9001-send-email.sh
index aa0c20499b..ce09cf1fe3 100755
--- i/t/t9001-send-email.sh
+++ w/t/t9001-send-email.sh
@@ -11,7 +11,7 @@ PREREQ="PERL"

replace_variable_fields () {
sed -e "s/^\(Date:\).*/\1 DATE-STRING/" \
- -e "s/^\(Message-Id:\).*/\1 MESSAGE-ID-STRING/" \
+ -e "s/^\(Message-ID:\).*/\1 MESSAGE-ID-STRING/" \
-e "s/^\(X-Mailer:\).*/\1 X-MAILER-STRING/"
}

@@ -224,7 +224,7 @@ Cc: [email protected],
[email protected]
Subject: [PATCH 1/1] Second.
Date: DATE-STRING
-Message-Id: MESSAGE-ID-STRING
+Message-ID: MESSAGE-ID-STRING
X-Mailer: X-MAILER-STRING
In-Reply-To: <[email protected]>
References: <[email protected]>
@@ -616,7 +616,7 @@ test_expect_success $PREREQ 'In-Reply-To without --chain-reply-to' '
sed -n -e "s/^In-Reply-To: *\(.*\)/\1/p" msgtxt1 >actual &&
test_cmp expect actual &&
# Second and subsequent messages are replies to the first one
- sed -n -e "s/^Message-Id: *\(.*\)/\1/p" msgtxt1 >expect &&
+ sed -n -e "s/^Message-ID: *\(.*\)/\1/p" msgtxt1 >expect &&
sed -n -e "s/^In-Reply-To: *\(.*\)/\1/p" msgtxt2 >actual &&
test_cmp expect actual &&
sed -n -e "s/^In-Reply-To: *\(.*\)/\1/p" msgtxt3 >actual &&
@@ -636,10 +636,10 @@ test_expect_success $PREREQ 'In-Reply-To with --chain-reply-to' '
2>errors &&
sed -n -e "s/^In-Reply-To: *\(.*\)/\1/p" msgtxt1 >actual &&
test_cmp expect actual &&
- sed -n -e "s/^Message-Id: *\(.*\)/\1/p" msgtxt1 >expect &&
+ sed -n -e "s/^Message-ID: *\(.*\)/\1/p" msgtxt1 >expect &&
sed -n -e "s/^In-Reply-To: *\(.*\)/\1/p" msgtxt2 >actual &&
test_cmp expect actual &&
- sed -n -e "s/^Message-Id: *\(.*\)/\1/p" msgtxt2 >expect &&
+ sed -n -e "s/^Message-ID: *\(.*\)/\1/p" msgtxt2 >expect &&
sed -n -e "s/^In-Reply-To: *\(.*\)/\1/p" msgtxt3 >actual &&
test_cmp expect actual
'
@@ -712,7 +712,7 @@ Cc: [email protected],
[email protected]
Subject: [PATCH 1/1] Second.
Date: DATE-STRING
-Message-Id: MESSAGE-ID-STRING
+Message-ID: MESSAGE-ID-STRING
X-Mailer: X-MAILER-STRING
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
@@ -758,7 +758,7 @@ Cc: A <[email protected]>,
[email protected]
Subject: [PATCH 1/1] Second.
Date: DATE-STRING
-Message-Id: MESSAGE-ID-STRING
+Message-ID: MESSAGE-ID-STRING
X-Mailer: X-MAILER-STRING
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
@@ -795,7 +795,7 @@ Cc: A <[email protected]>,
C O Mitter <[email protected]>
Subject: [PATCH 1/1] Second.
Date: DATE-STRING
-Message-Id: MESSAGE-ID-STRING
+Message-ID: MESSAGE-ID-STRING
X-Mailer: X-MAILER-STRING
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
@@ -823,7 +823,7 @@ From: Example <[email protected]>
To: [email protected]
Subject: [PATCH 1/1] Second.
Date: DATE-STRING
-Message-Id: MESSAGE-ID-STRING
+Message-ID: MESSAGE-ID-STRING
X-Mailer: X-MAILER-STRING
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
@@ -859,7 +859,7 @@ Cc: A <[email protected]>,
[email protected]
Subject: [PATCH 1/1] Second.
Date: DATE-STRING
-Message-Id: MESSAGE-ID-STRING
+Message-ID: MESSAGE-ID-STRING
X-Mailer: X-MAILER-STRING
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
@@ -892,7 +892,7 @@ Cc: A <[email protected]>,
[email protected]
Subject: [PATCH 1/1] Second.
Date: DATE-STRING
-Message-Id: MESSAGE-ID-STRING
+Message-ID: MESSAGE-ID-STRING
X-Mailer: X-MAILER-STRING
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
@@ -925,7 +925,7 @@ Cc: A <[email protected]>,
[email protected]
Subject: [PATCH 1/1] Second.
Date: DATE-STRING
-Message-Id: MESSAGE-ID-STRING
+Message-ID: MESSAGE-ID-STRING
X-Mailer: X-MAILER-STRING
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
@@ -962,7 +962,7 @@ Cc: A <[email protected]>,
C O Mitter <[email protected]>
Subject: [PATCH 1/1] Second.
Date: DATE-STRING
-Message-Id: MESSAGE-ID-STRING
+Message-ID: MESSAGE-ID-STRING
X-Mailer: X-MAILER-STRING
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
@@ -992,7 +992,7 @@ Cc: A <[email protected]>,
C O Mitter <[email protected]>
Subject: [PATCH 1/1] Second.
Date: DATE-STRING
-Message-Id: MESSAGE-ID-STRING
+Message-ID: MESSAGE-ID-STRING
X-Mailer: X-MAILER-STRING
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
@@ -1477,7 +1477,7 @@ test_expect_success $PREREQ 'To headers from files reset each patch' '
test_expect_success $PREREQ 'setup expect' '
cat >email-using-8bit <<\EOF
From fe6ecc66ece37198fe5db91fa2fc41d9f4fe5cc4 Mon Sep 17 00:00:00 2001
-Message-Id: <[email protected]>
+Message-ID: <[email protected]>
From: [email protected]
Date: Sat, 12 Jun 2010 15:53:58 +0200
Subject: subject goes here
@@ -1563,7 +1563,7 @@ test_expect_success $PREREQ '--8bit-encoding overrides sendemail.8bitEncoding' '
test_expect_success $PREREQ 'setup expect' '
cat >email-using-8bit <<-\EOF
From fe6ecc66ece37198fe5db91fa2fc41d9f4fe5cc4 Mon Sep 17 00:00:00 2001
- Message-Id: <[email protected]>
+ Message-ID: <[email protected]>
From: [email protected]
Date: Sat, 12 Jun 2010 15:53:58 +0200
Subject: Dieser Betreff enthält auch einen Umlaut!
@@ -1592,7 +1592,7 @@ test_expect_success $PREREQ '--8bit-encoding also treats subject' '
test_expect_success $PREREQ 'setup expect' '
cat >email-using-8bit <<-\EOF
From fe6ecc66ece37198fe5db91fa2fc41d9f4fe5cc4 Mon Sep 17 00:00:00 2001
- Message-Id: <[email protected]>
+ Message-ID: <[email protected]>
From: A U Thor <[email protected]>
Date: Sat, 12 Jun 2010 15:53:58 +0200
Content-Type: text/plain; charset=UTF-8
@@ -1673,7 +1673,7 @@ test_expect_success $PREREQ '8-bit and sendemail.transferencoding=base64' '
test_expect_success $PREREQ 'setup expect' '
cat >email-using-qp <<-\EOF
From fe6ecc66ece37198fe5db91fa2fc41d9f4fe5cc4 Mon Sep 17 00:00:00 2001
- Message-Id: <[email protected]>
+ Message-ID: <[email protected]>
From: A U Thor <[email protected]>
Date: Sat, 12 Jun 2010 15:53:58 +0200
MIME-Version: 1.0
@@ -1699,7 +1699,7 @@ test_expect_success $PREREQ 'convert from quoted-printable to base64' '
test_expect_success $PREREQ 'setup expect' "
tr -d '\\015' | tr '%' '\\015' >email-using-crlf <<EOF
From fe6ecc66ece37198fe5db91fa2fc41d9f4fe5cc4 Mon Sep 17 00:00:00 2001
-Message-Id: <[email protected]>
+Message-ID: <[email protected]>
From: A U Thor <[email protected]>
Date: Sat, 12 Jun 2010 15:53:58 +0200
Content-Type: text/plain; charset=UTF-8

Subject: Re: [RFC PATCH v1 1/1] docs: add the new commit-msg tags 'Reported:' and 'Reviewed:'


On Tue, Nov 23 2021, Eric Wong wrote:

> Thorsten Leemhuis <[email protected]> wrote:
>> diff --git a/Documentation/maintainer/configure-git.rst b/Documentation/maintainer/configure-git.rst
>> index 80ae5030a590..8429d45d661c 100644
>> --- a/Documentation/maintainer/configure-git.rst
>> +++ b/Documentation/maintainer/configure-git.rst
>
> <snip>, +cc git@vger
>
>> @@ -56,7 +56,7 @@ by adding the following hook into your git:
>> $ cat >.git/hooks/applypatch-msg <<'EOF'
>> #!/bin/sh
>> . git-sh-setup
>> - perl -pi -e 's|^Message-Id:\s*<?([^>]+)>?$|Link: https://lore.kernel.org/r/$1|g;' "$1"
>> + perl -pi -e 's|^Message-Id:\s*<?([^>]+)>?$|Reviewed: https://lore.kernel.org/r/$1|g;' "$1"
>
> Side note: that regexp should match "Message-ID" case-insensitively.
> git send-email is an outlier in its capitalization of "Message-Id",
> most RFCs capitalize it "Message-ID", as do common MUAs.
>
> git send-email's capitalization does annoy me and I've looked
> into changing it; but there's a bunch of tests and probably
> dependent code that also need to be updated...

"git format-patch" does it without send-email, but I see that send-email
will then parse its output, and would turn any capitalization into the
"Message-Id" form again.

We could probably just have it preserve whatever capitalization it finds
if there's an existing header, we wouldn't fix anything, but we'd move
the blame around a bit :)

2021-11-24 06:12:14

by Eric Wong

[permalink] [raw]
Subject: Re: [RFC PATCH v1 1/1] docs: add the new commit-msg tags 'Reported:' and 'Reviewed:'

Junio C Hamano <[email protected]> wrote:
> Eric Wong <[email protected]> writes:
>
> > git send-email's capitalization does annoy me and I've looked
> > into changing it; but there's a bunch of tests and probably
> > dependent code that also need to be updated...
>
> It does annoy me, too, and I do not mind if it gets "fixed", but I
> do not know if the cost for vetting a bulk update like that is worth
> it. There is another one outside send-email in log-tree.c that is
> responsible for output from format-patch.

I support updating the documentation in git, first; then perhaps
making tests case-insensitive (which could be a complex
change...).

Unfortunately, changing the send-email + log-tree.c code would
break existing users of the applypatch-msg hook sample in
linux/Documentation/maintainer/configure-git.rst which has
been case-sensitive for around for 2 years:
https://lore.kernel.org/r/[email protected]

:<

2021-11-26 07:31:12

by Thorsten Leemhuis

[permalink] [raw]
Subject: Re: [RFC PATCH v1 1/1] docs: add the new commit-msg tags 'Reported:' and 'Reviewed:'

Ccing Linus Walleij, who added this, and Kees, who apparently came up
with this originally.

On 23.11.21 19:52, Eric Wong wrote:
> Thorsten Leemhuis <[email protected]> wrote:
>> diff --git a/Documentation/maintainer/configure-git.rst b/Documentation/maintainer/configure-git.rst
>> index 80ae5030a590..8429d45d661c 100644
>> --- a/Documentation/maintainer/configure-git.rst
>> +++ b/Documentation/maintainer/configure-git.rst
>
> <snip>, +cc git@vger
>
>> @@ -56,7 +56,7 @@ by adding the following hook into your git:
>> $ cat >.git/hooks/applypatch-msg <<'EOF'
>> #!/bin/sh
>> . git-sh-setup
>> - perl -pi -e 's|^Message-Id:\s*<?([^>]+)>?$|Link: https://lore.kernel.org/r/$1|g;' "$1"
>> + perl -pi -e 's|^Message-Id:\s*<?([^>]+)>?$|Reviewed: https://lore.kernel.org/r/$1|g;' "$1"
>
> Side note: that regexp should match "Message-ID" case-insensitively.
> git send-email is an outlier in its capitalization of "Message-Id",
> most RFCs capitalize it "Message-ID", as do common MUAs.

Argh :-/

It's still totally unclear if that or a similar patch will be accepted.
And even if it is: the "don't do two different things in one commit"
rule might not be that strict enforced when it comes to the Linux
kernel's docs, but changing this regexp as part of another patch crosses
the line.

IOW: we afaics need a separate patch to make the regexp
case-insensitively. Eric, do you want to submit one, as you brought it
up? Or are there any other volunteers?

Ciao, Thorsten

Subject: Re: [RFC PATCH v1 1/1] docs: add the new commit-msg tags 'Reported:' and 'Reviewed:'


On Tue, Nov 23 2021, Junio C Hamano wrote:

> Eric Wong <[email protected]> writes:
>
>> git send-email's capitalization does annoy me and I've looked
>> into changing it; but there's a bunch of tests and probably
>> dependent code that also need to be updated...
> [...]
> diff --git i/git-send-email.perl w/git-send-email.perl
> index 5262d88ee3..a61134c7d3 100755
> --- i/git-send-email.perl
> +++ w/git-send-email.perl
> @@ -1494,7 +1494,7 @@ sub send_message {
> To: $to${ccline}
> Subject: $subject
> Date: $date
> -Message-Id: $message_id
> +Message-ID: $message_id
> ";
> if ($use_xmailer) {
> $header .= "X-Mailer: git-send-email $gitversion\n";

Perhaps one way to split this & make it more readable is to split this,
i.e. the mesage-id's send-email itself generates & tests, usually it
passes along format-patch's.

> @@ -1789,7 +1789,7 @@ sub process_file {
> $has_mime_version = 1;
> push @xh, $_;
> }
> - elsif (/^Message-Id: (.*)/i) {
> + elsif (/^Message-ID: (.*)/i) {
> $message_id = $1;
> }
> elsif (/^Content-Transfer-Encoding: (.*)/i) {

Not strictly needed due to the /i, maybe splitting out cosmetic changes
would be better?

I also notice we have various hits for "git grep message-id", including
regex checks you didn't update here.

2021-11-26 17:13:44

by Eric Wong

[permalink] [raw]
Subject: Re: [RFC PATCH v1 1/1] docs: add the new commit-msg tags 'Reported:' and 'Reviewed:'

Thorsten Leemhuis <[email protected]> wrote:
> Ccing Linus Walleij, who added this, and Kees, who apparently came up
> with this originally.
>
> On 23.11.21 19:52, Eric Wong wrote:
> > Thorsten Leemhuis <[email protected]> wrote:
> >> diff --git a/Documentation/maintainer/configure-git.rst b/Documentation/maintainer/configure-git.rst
> >> index 80ae5030a590..8429d45d661c 100644
> >> --- a/Documentation/maintainer/configure-git.rst
> >> +++ b/Documentation/maintainer/configure-git.rst
> >
> > <snip>, +cc git@vger
> >
> >> @@ -56,7 +56,7 @@ by adding the following hook into your git:
> >> $ cat >.git/hooks/applypatch-msg <<'EOF'
> >> #!/bin/sh
> >> . git-sh-setup
> >> - perl -pi -e 's|^Message-Id:\s*<?([^>]+)>?$|Link: https://lore.kernel.org/r/$1|g;' "$1"
> >> + perl -pi -e 's|^Message-Id:\s*<?([^>]+)>?$|Reviewed: https://lore.kernel.org/r/$1|g;' "$1"
> >
> > Side note: that regexp should match "Message-ID" case-insensitively.
> > git send-email is an outlier in its capitalization of "Message-Id",
> > most RFCs capitalize it "Message-ID", as do common MUAs.
>
> Argh :-/
>
> It's still totally unclear if that or a similar patch will be accepted.
> And even if it is: the "don't do two different things in one commit"
> rule might not be that strict enforced when it comes to the Linux
> kernel's docs, but changing this regexp as part of another patch crosses
> the line.
>
> IOW: we afaics need a separate patch to make the regexp
> case-insensitively. Eric, do you want to submit one, as you brought it
> up? Or are there any other volunteers?

I suggest you turn this into a 2 patch series to avoid conflicts
for a trivial change. I don't even have a kernel worktree handy
at the moment (ENOSPC :x)

2021-11-27 19:34:45

by Thorsten Leemhuis

[permalink] [raw]
Subject: Re: [RFC PATCH v1 1/1] docs: add the new commit-msg tags 'Reported:' and 'Reviewed:'

On 26.11.21 18:11, Eric Wong wrote:
> Thorsten Leemhuis <[email protected]> wrote:
>> Ccing Linus Walleij, who added this, and Kees, who apparently came up
>> with this originally.
>>
>> On 23.11.21 19:52, Eric Wong wrote:
>>> Thorsten Leemhuis <[email protected]> wrote:
>>>> diff --git a/Documentation/maintainer/configure-git.rst b/Documentation/maintainer/configure-git.rst
>>>> index 80ae5030a590..8429d45d661c 100644
>>>> --- a/Documentation/maintainer/configure-git.rst
>>>> +++ b/Documentation/maintainer/configure-git.rst
>>>
>>> <snip>, +cc git@vger
>>>
>>>> @@ -56,7 +56,7 @@ by adding the following hook into your git:
>>>> $ cat >.git/hooks/applypatch-msg <<'EOF'
>>>> #!/bin/sh
>>>> . git-sh-setup
>>>> - perl -pi -e 's|^Message-Id:\s*<?([^>]+)>?$|Link: https://lore.kernel.org/r/$1|g;' "$1"
>>>> + perl -pi -e 's|^Message-Id:\s*<?([^>]+)>?$|Reviewed: https://lore.kernel.org/r/$1|g;' "$1"
>>>
>>> Side note: that regexp should match "Message-ID" case-insensitively.
>>> git send-email is an outlier in its capitalization of "Message-Id",
>>> most RFCs capitalize it "Message-ID", as do common MUAs.
>>
>> Argh :-/
>>
>> It's still totally unclear if that or a similar patch will be accepted.
>> And even if it is: the "don't do two different things in one commit"
>> rule might not be that strict enforced when it comes to the Linux
>> kernel's docs, but changing this regexp as part of another patch crosses
>> the line.
>>
>> IOW: we afaics need a separate patch to make the regexp
>> case-insensitively. Eric, do you want to submit one, as you brought it
>> up? Or are there any other volunteers?
>
> I suggest you turn this into a 2 patch series to avoid conflicts
> for a trivial change. I don't even have a kernel worktree handy
> at the moment (ENOSPC :x)

:-D

Will do this in a couple of days, unless Linus or Kees speak up.

Just to be sure I'll do what you expect to be done: I assume you want to see
it changed like this?

- perl -pi -e 's|^Message-Id:\s*<?([^>]+)>?$|Link: https://lore.kernel.org/r/$1|g;' "$1"
+ perl -pi -e 's|^Message-I[dD]:\s*<?([^>]+)>?$|Link: https://lore.kernel.org/r/$1|g;' "$1"

Or are there even more variants of Message-ID out there known that
need to be taken into account?

Ciao, Thorsten

2021-11-27 19:54:37

by Eric Wong

[permalink] [raw]
Subject: Re: [RFC PATCH v1 1/1] docs: add the new commit-msg tags 'Reported:' and 'Reviewed:'

Thorsten Leemhuis <[email protected]> wrote:
> Just to be sure I'll do what you expect to be done: I assume you want to see
> it changed like this?
>
> - perl -pi -e 's|^Message-Id:\s*<?([^>]+)>?$|Link: https://lore.kernel.org/r/$1|g;' "$1"
> + perl -pi -e 's|^Message-I[dD]:\s*<?([^>]+)>?$|Link: https://lore.kernel.org/r/$1|g;' "$1"
>
> Or are there even more variants of Message-ID out there known that
> need to be taken into account?

The entire match should be case-insensitive[1], so I'd add `i'
at the end:

perl -pi -e 's|^Message-ID:\s*<?([^>]+)>?$|Link: https://lore.kernel.org/r/$1|gi;' "$1"

Fwiw, every mail and HTTP/1.x header parser I've looked at works
case-insensitively. Also, I'm not sure if `g' is needed, actually...

[1] https://datatracker.ietf.org/doc/html/rfc822#section-3.4.7

2021-11-27 20:22:27

by Junio C Hamano

[permalink] [raw]
Subject: Re: [RFC PATCH v1 1/1] docs: add the new commit-msg tags 'Reported:' and 'Reviewed:'

Eric Wong <[email protected]> writes:

> Thorsten Leemhuis <[email protected]> wrote:
>> Just to be sure I'll do what you expect to be done: I assume you want to see
>> it changed like this?
>>
>> - perl -pi -e 's|^Message-Id:\s*<?([^>]+)>?$|Link: https://lore.kernel.org/r/$1|g;' "$1"
> ...
> The entire match should be case-insensitive[1], so I'd add `i'
> at the end:
>
> perl -pi -e 's|^Message-ID:\s*<?([^>]+)>?$|Link: https://lore.kernel.org/r/$1|gi;' "$1"
>
> Fwiw, every mail and HTTP/1.x header parser I've looked at works
> case-insensitively. Also, I'm not sure if `g' is needed, actually...

It is left anchored with "^" so it would be hard to match more than
once on the same line ;-)

I agree that it is the right solution to make the whole thing
case-insensitive by adding 'i' at the end.

FWIW, the RFC first says this:

1.2.2. Syntactic notation

This standard uses the Augmented Backus-Naur Form (ABNF) notation
specified in [RFC2234] for the formal definitions of the syntax of
messages. Characters will be specified either by a decimal value
(e.g., the value %d65 for uppercase A and %d97 for lowercase A) or by
a case-insensitive literal value enclosed in quotation marks (e.g.,
"A" for either uppercase or lowercase A).

and then goes on to define how message-id should look like.

3.6.4. Identification fields

message-id = "Message-ID:" msg-id CRLF


But if you go the "add /i at the end" route, you do not have to
upcase "d" to "D" and that may reduce the patch noise (it only
matters if the patch viewer highlights letter-by-letter changes for
your recipients).

HTH

2021-11-29 12:05:30

by Jani Nikula

[permalink] [raw]
Subject: Re: [RFC PATCH v1 1/1] docs: add the new commit-msg tags 'Reported:' and 'Reviewed:'

On Sat, 27 Nov 2021, Junio C Hamano <[email protected]> wrote:
> Eric Wong <[email protected]> writes:
>
>> Thorsten Leemhuis <[email protected]> wrote:
>>> Just to be sure I'll do what you expect to be done: I assume you want to see
>>> it changed like this?
>>>
>>> - perl -pi -e 's|^Message-Id:\s*<?([^>]+)>?$|Link: https://lore.kernel.org/r/$1|g;' "$1"
>> ...
>> The entire match should be case-insensitive[1], so I'd add `i'
>> at the end:
>>
>> perl -pi -e 's|^Message-ID:\s*<?([^>]+)>?$|Link: https://lore.kernel.org/r/$1|gi;' "$1"
>>
>> Fwiw, every mail and HTTP/1.x header parser I've looked at works
>> case-insensitively. Also, I'm not sure if `g' is needed, actually...
>
> It is left anchored with "^" so it would be hard to match more than
> once on the same line ;-)
>
> I agree that it is the right solution to make the whole thing
> case-insensitive by adding 'i' at the end.
>
> FWIW, the RFC first says this:
>
> 1.2.2. Syntactic notation
>
> This standard uses the Augmented Backus-Naur Form (ABNF) notation
> specified in [RFC2234] for the formal definitions of the syntax of
> messages. Characters will be specified either by a decimal value
> (e.g., the value %d65 for uppercase A and %d97 for lowercase A) or by
> a case-insensitive literal value enclosed in quotation marks (e.g.,
> "A" for either uppercase or lowercase A).
>
> and then goes on to define how message-id should look like.
>
> 3.6.4. Identification fields
>
> message-id = "Message-ID:" msg-id CRLF
>
>
> But if you go the "add /i at the end" route, you do not have to
> upcase "d" to "D" and that may reduce the patch noise (it only
> matters if the patch viewer highlights letter-by-letter changes for
> your recipients).

From the RFC nitpicking department, msg-id is allowed to contain CFWS
(comments and folding white space) outside the angle brackets, which
means you could have RFC compliant Message-ID header field:

Message-ID:
<[email protected]>

or

Message-ID: (comment)
<[email protected]>

or even worse, really.

The moral of the story is that you should always offload the header
parsing to some tool or library designed to do that.


BR,
Jani.


--
Jani Nikula, Intel Open Source Graphics Center

2021-11-29 17:20:38

by Junio C Hamano

[permalink] [raw]
Subject: Re: [RFC PATCH v1 1/1] docs: add the new commit-msg tags 'Reported:' and 'Reviewed:'

Jani Nikula <[email protected]> writes:

> From the RFC nitpicking department, ...
>
> Message-ID: (comment)
> <[email protected]>

Thanks for a fun piece; the (comment) is quite interesting.

I wasn't having fun with RFC nitpicking, though. I was reacting to
this part of the message I was responding to ...

>>> Fwiw, every mail and HTTP/1.x header parser I've looked at works
>>> case-insensitively. Also, I'm not sure if `g' is needed, actually...

... to say that "works case-insensitively" may not just be
empirically correct, but RFC backs him up, to Eric.

2021-11-29 17:28:25

by Eric Wong

[permalink] [raw]
Subject: Re: [RFC PATCH v1 1/1] docs: add the new commit-msg tags 'Reported:' and 'Reviewed:'

Jani Nikula <[email protected]> wrote:
> From the RFC nitpicking department, msg-id is allowed to contain CFWS
> (comments and folding white space) outside the angle brackets, which
> means you could have RFC compliant Message-ID header field:
>
> Message-ID:
> <[email protected]>
>
> or
>
> Message-ID: (comment)
> <[email protected]>
>
> or even worse, really.
>
> The moral of the story is that you should always offload the header
> parsing to some tool or library designed to do that.

It's a bit much for common cases with git-send-email and
reasonable MUAs, I think. I don't know if formail is commonly
installed, nowadays...

Fwiw, the code running lore uses something like this:

/^Message-ID:[ \t]*([^\n]*\r?\n # 1st line
# continuation lines:
(?:[^:\n]*?[ \t]+[^\n]*\r?\n)*)
/ismx

I'm fine with this non-trivial regexp being included with
GPL-2.0 code; but it could be too big for a one-liner *shrug*

... And <([^>]+)>/s to extract Message-IDs, but ISTR the code
behind lore doesn't handle spaces inside <> properly, but I'm
not sure if there's enough valid, non-spam messages with them...

2021-11-29 19:22:33

by Jani Nikula

[permalink] [raw]
Subject: Re: [RFC PATCH v1 1/1] docs: add the new commit-msg tags 'Reported:' and 'Reviewed:'

On Mon, 29 Nov 2021, Junio C Hamano <[email protected]> wrote:
> I wasn't having fun with RFC nitpicking, though.

I didn't mean to imply you were, I was saying I was!

BR,
Jani.


--
Jani Nikula, Intel Open Source Graphics Center

2021-11-29 19:27:07

by Jani Nikula

[permalink] [raw]
Subject: Re: [RFC PATCH v1 1/1] docs: add the new commit-msg tags 'Reported:' and 'Reviewed:'

On Mon, 29 Nov 2021, Eric Wong <[email protected]> wrote:
> Jani Nikula <[email protected]> wrote:
>> The moral of the story is that you should always offload the header
>> parsing to some tool or library designed to do that.
>
> It's a bit much for common cases with git-send-email and
> reasonable MUAs, I think.

I think you can have unreasonable MDAs in between, though!

BR,
Jani.


--
Jani Nikula, Intel Open Source Graphics Center

2021-11-29 20:34:45

by Steven Rostedt

[permalink] [raw]
Subject: Re: [RFC PATCH v1 1/1] docs: add the new commit-msg tags 'Reported:' and 'Reviewed:'

On Mon, 29 Nov 2021 14:03:09 +0200
Jani Nikula <[email protected]> wrote:

> >From the RFC nitpicking department, msg-id is allowed to contain CFWS
> (comments and folding white space) outside the angle brackets, which
> means you could have RFC compliant Message-ID header field:
>
> Message-ID:
> <[email protected]>

My scripts have already been hit by this. (I've been lazy and not fixed it,
but instead, just edit the file that it is parsing manually, to be on one
line :-p)

-- Steve

2021-11-29 22:18:07

by Jonathan Corbet

[permalink] [raw]
Subject: Re: [RFC PATCH v1 1/1] docs: add the new commit-msg tags 'Reported:' and 'Reviewed:'

Thorsten Leemhuis <[email protected]> writes:

> Introduce the tags 'Reported:' and 'Reviewed:' in addition to 'Link:',
> as the latter is overloaded and hence doesn't indicate what the provided
> URL is about. Documenting these also provides clarity, as a few
> developers have used 'References:' to point to problem reports;
> nevertheless 'Reported:' was chosen for this purpose, as it perfectly
> matches up with the 'Reported-by:' tag commonly used already and needed
> in this situation already.
>
> Signed-off-by: Thorsten Leemhuis <[email protected]>
> To: [email protected]

Thanks for flooding my inbox during a holiday week :) Just looking at
this now.

> v1/RFC:
> - first, *rough version* to see how this idea is received in the
> community
> ---
> Documentation/maintainer/configure-git.rst | 6 +--
> Documentation/process/5.Posting.rst | 54 ++++++++++++++------
> Documentation/process/submitting-patches.rst | 22 ++++----
> 3 files changed, 53 insertions(+), 29 deletions(-)
>
> diff --git a/Documentation/maintainer/configure-git.rst b/Documentation/maintainer/configure-git.rst
> index 80ae5030a590..8429d45d661c 100644
> --- a/Documentation/maintainer/configure-git.rst
> +++ b/Documentation/maintainer/configure-git.rst
> @@ -40,12 +40,12 @@ Creating commit links to lore.kernel.org
> The web site http://lore.kernel.org is meant as a grand archive of all mail
> list traffic concerning or influencing the kernel development. Storing archives
> of patches here is a recommended practice, and when a maintainer applies a
> -patch to a subsystem tree, it is a good idea to provide a Link: tag with a
> +patch to a subsystem tree, it is a good idea to provide a Reviewed: tag with a
> reference back to the lore archive so that people that browse the commit
> history can find related discussions and rationale behind a certain change.
> The link tag will look like this:
>
> - Link: https://lore.kernel.org/r/<message-id>
> + Reviewed: https://lore.kernel.org/r/<message-id>

The *link* tag will look like that?

[...]

> +The tags in common use are:
> +
> + - ``Reported:`` points to a report of a problem fixed by this patch. The
> + provided URL thus might point to a entry in a bug tracker or a mail in a
> + mailing list archive. Typically this tag is followed by a "Reported-by:"
> + tag (see below).
> +
> + - ``Link:`` points to websites providing additional backgrounds or details,
> + for example a document with a specification implemented by the patch.

So this is a serious change from how Link: is used now, and runs counter
to the scripts used by a lot of maintainers. I suspect that this thread
is only as short as it is because a lot of people haven't seen this yet;
it could be a hard change to sell.

Also, I think that documents like specs should be called out separately
in the changelog, with text saying what they actually are.

> + - ``Reviewed:`` ignore this, as maintainers add it when applying a patch, to
> + make the commit point to the latest public review of the patch.

Another question would be: what's the interplay between the (quite
similar) "Reviewed" and "Reviewed-by" tags (and the same for the report
tags). If there's a "Reviewed" do we still need "Reviewed-by"? That
should be spelled out, whichever way is wanted.

I do worry that the similarity is going to lead to a certain amount of
confusion and use of the wrong tag. People have a hard time getting all
the tags we have now right; adding more that look almost like the
existing ones seems like a recipe for trouble.

For these reasons, I would be more inclined toward Konstantin's
suggestion of adding notes to the existing Link: tags.

> +A third kind of tags are used to document which developers were involved in
> +the development of the patch. Each of these uses this format::
>
> tag: Full Name <email address> optional-other-stuff
>
> The tags in common use are:
>
> - - Signed-off-by: this is a developer's certification that he or she has
> + - ``Signed-off-by:`` is a developer's certification that he or she has

So this markup addition is a separate change that would belong in its
own patch. Do we really need it, though? It clutters the text and
irritates the anti-RST minority (which has been mercifully quiet
recently) without really adding any benefit.

Thanks,

jon

2021-11-30 08:24:29

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [RFC PATCH v1 1/1] docs: add the new commit-msg tags 'Reported:' and 'Reviewed:'

Hi Eric,

On Mon, Nov 29, 2021 at 11:29 PM Eric Wong <[email protected]> wrote:
> It's a bit much for common cases with git-send-email and
> reasonable MUAs, I think. I don't know if formail is commonly
> installed, nowadays...

Of course ;-) You need it to run checkpatch on patch series obtained
through "b4 am", before you apply them to your tree:

$ cat *mbx | formail -s scripts/checkpatch.pl

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2021-11-30 13:10:34

by Thorsten Leemhuis

[permalink] [raw]
Subject: Re: [RFC PATCH v1 1/1] docs: add the new commit-msg tags 'Reported:' and 'Reviewed:'

On 29.11.21 23:16, Jonathan Corbet wrote:
> Thorsten Leemhuis <[email protected]> writes:
>
>> Introduce the tags 'Reported:' and 'Reviewed:' in addition to 'Link:',
>> as the latter is overloaded and hence doesn't indicate what the provided
>> URL is about. Documenting these also provides clarity, as a few
>> developers have used 'References:' to point to problem reports;
>> nevertheless 'Reported:' was chosen for this purpose, as it perfectly
>> matches up with the 'Reported-by:' tag commonly used already and needed
>> in this situation already.
>>
>> Signed-off-by: Thorsten Leemhuis <[email protected]>
>> To: [email protected]
>
> Thanks for flooding my inbox during a holiday week :)

We didn't have one here. :-D But hey, it wasn't out of envy, I'm a
little ignorant to local holiday/festival seasons, too.

> Just looking at this now.
>
>> v1/RFC:
>> - first, *rough version* to see how this idea is received in the
>> community
>> ---
>> Documentation/maintainer/configure-git.rst | 6 +--
>> Documentation/process/5.Posting.rst | 54 ++++++++++++++------
>> Documentation/process/submitting-patches.rst | 22 ++++----
>> 3 files changed, 53 insertions(+), 29 deletions(-)
>>
>> diff --git a/Documentation/maintainer/configure-git.rst b/Documentation/maintainer/configure-git.rst
>> index 80ae5030a590..8429d45d661c 100644
>> --- a/Documentation/maintainer/configure-git.rst
>> +++ b/Documentation/maintainer/configure-git.rst
>> @@ -40,12 +40,12 @@ Creating commit links to lore.kernel.org
>> The web site http://lore.kernel.org is meant as a grand archive of all mail
>> list traffic concerning or influencing the kernel development. Storing archives
>> of patches here is a recommended practice, and when a maintainer applies a
>> -patch to a subsystem tree, it is a good idea to provide a Link: tag with a
>> +patch to a subsystem tree, it is a good idea to provide a Reviewed: tag with a
>> reference back to the lore archive so that people that browse the commit
>> history can find related discussions and rationale behind a certain change.
>> The link tag will look like this:
>>
>> - Link: https://lore.kernel.org/r/<message-id>
>> + Reviewed: https://lore.kernel.org/r/<message-id>
>
> The *link* tag will look like that?

Ha, good point, will fix that.

> [...]
>> +The tags in common use are:
>> +
>> + - ``Reported:`` points to a report of a problem fixed by this patch. The
>> + provided URL thus might point to a entry in a bug tracker or a mail in a
>> + mailing list archive. Typically this tag is followed by a "Reported-by:"
>> + tag (see below).
>> +
>> + - ``Link:`` points to websites providing additional backgrounds or details,
>> + for example a document with a specification implemented by the patch.
>
> So this is a serious change from how Link: is used now, and runs counter
> to the scripts used by a lot of maintainers. I suspect that this thread
> is only as short as it is because a lot of people haven't seen this yet;
> it could be a hard change to sell.

Yeah, I'm aware of that. And to be honest: I don't have a strong
interest in this, just think it might be the right thing to do. And I
just got the impression that regzbot's dependence on the Link: tag for
linking to regression reports is making the ambiguity of the tag worse.
That lead to the thought: well, simply bring it up now and see what
people think; if they don't like it, I can tell myself "well, I tried to
improve it, but it was not welcomed" and sleep well at night. At least
as long as my cat allows me to. :-)

> Also, I think that documents like specs should be called out separately
> in the changelog, with text saying what they actually are.

I wonder a little if that is worth the trouble, but hey, why not, fine
with me.

>> + - ``Reviewed:`` ignore this, as maintainers add it when applying a patch, to
>> + make the commit point to the latest public review of the patch.
>
> Another question would be: what's the interplay between the (quite
> similar) "Reviewed" and "Reviewed-by" tags (and the same for the report
> tags).

Hmmm, I liked the interplay for Reported/Reported-by, but yeah, for
Reviewed/Reviewed-by I see the problem now.

> If there's a "Reviewed" do we still need "Reviewed-by"? That
> should be spelled out, whichever way is wanted.

I didn't want to undermine or obsolete "Reviewed-by" at all. I sometimes
wonder if this and "Tested-by" should be stored somewhere else (in "git
notes" or something), so they can be extended after a change got
committed -- but that's a whole different topic and something I'm even
less interested in driving forward. :-D

Maybe "Reviewed" was simply the wrong term. Maybe "Review:", "Posted:",
or "MergeRequest:" would be better in general and avoid this problem.

> I do worry that the similarity is going to lead to a certain amount of
> confusion and use of the wrong tag. People have a hard time getting all
> the tags we have now right; adding more that look almost like the
> existing ones seems like a recipe for trouble.
>
> For these reasons, I would be more inclined toward Konstantin's
> suggestion of adding notes to the existing Link: tags.

Yeah, maybe, but that results in long lines and forces people to type more.

>> +A third kind of tags are used to document which developers were involved in
>> +the development of the patch. Each of these uses this format::
>>
>> tag: Full Name <email address> optional-other-stuff
>>
>> The tags in common use are:
>>
>> - - Signed-off-by: this is a developer's certification that he or she has
>> + - ``Signed-off-by:`` is a developer's certification that he or she has
>
> So this markup addition is a separate change that would belong in its
> own patch.

Okay.

> Do we really need it, though? It clutters the text and
> irritates the anti-RST minority (which has been mercifully quiet
> recently) without really adding any benefit.

I'm not strongly attached to it. I added it after I noticed that it's at
least for me slightly unclear if the colon is part of the tag or used to
precede the explanation for the tag (in the 'Signed-off-by' case it's
both at the same time). And after adding the proposed tags the html view
imho looked a lot better.

Thx for the feedback!

Ciao, Thorsten

2021-12-01 12:25:10

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [RFC PATCH v1 1/1] docs: add the new commit-msg tags 'Reported:' and 'Reviewed:'

Hi Thorsten,

On Wed, Dec 1, 2021 at 7:32 AM Thorsten Leemhuis <[email protected]> wrote:
> On 29.11.21 23:16, Jonathan Corbet wrote:
> > Thorsten Leemhuis <[email protected]> writes:
> >> Introduce the tags 'Reported:' and 'Reviewed:' in addition to 'Link:',
> >> as the latter is overloaded and hence doesn't indicate what the provided
> >> URL is about. Documenting these also provides clarity, as a few
> >> developers have used 'References:' to point to problem reports;
> >> nevertheless 'Reported:' was chosen for this purpose, as it perfectly
> >> matches up with the 'Reported-by:' tag commonly used already and needed
> >> in this situation already.
> >>
> >> Signed-off-by: Thorsten Leemhuis <[email protected]>

> > [...]
> >> +The tags in common use are:
> >> +
> >> + - ``Reported:`` points to a report of a problem fixed by this patch. The
> >> + provided URL thus might point to a entry in a bug tracker or a mail in a
> >> + mailing list archive. Typically this tag is followed by a "Reported-by:"
> >> + tag (see below).
> >> +
> >> + - ``Link:`` points to websites providing additional backgrounds or details,
> >> + for example a document with a specification implemented by the patch.
> >
> > So this is a serious change from how Link: is used now, and runs counter
> > to the scripts used by a lot of maintainers. I suspect that this thread
> > is only as short as it is because a lot of people haven't seen this yet;
> > it could be a hard change to sell.

I saw it, but decided to wait a bit for other input...

> Yeah, I'm aware of that. And to be honest: I don't have a strong
> interest in this, just think it might be the right thing to do. And I
> just got the impression that regzbot's dependence on the Link: tag for
> linking to regression reports is making the ambiguity of the tag worse.
> That lead to the thought: well, simply bring it up now and see what
> people think; if they don't like it, I can tell myself "well, I tried to
> improve it, but it was not welcomed" and sleep well at night. At least
> as long as my cat allows me to. :-)
>
> > Also, I think that documents like specs should be called out separately
> > in the changelog, with text saying what they actually are.
>
> I wonder a little if that is worth the trouble, but hey, why not, fine
> with me.
>
> >> + - ``Reviewed:`` ignore this, as maintainers add it when applying a patch, to
> >> + make the commit point to the latest public review of the patch.
> >
> > Another question would be: what's the interplay between the (quite
> > similar) "Reviewed" and "Reviewed-by" tags (and the same for the report
> > tags).
>
> Hmmm, I liked the interplay for Reported/Reported-by, but yeah, for
> Reviewed/Reviewed-by I see the problem now.
>
> > If there's a "Reviewed" do we still need "Reviewed-by"? That
> > should be spelled out, whichever way is wanted.
>
> I didn't want to undermine or obsolete "Reviewed-by" at all. I sometimes
> wonder if this and "Tested-by" should be stored somewhere else (in "git
> notes" or something), so they can be extended after a change got
> committed -- but that's a whole different topic and something I'm even
> less interested in driving forward. :-D
>
> Maybe "Reviewed" was simply the wrong term. Maybe "Review:", "Posted:",
> or "MergeRequest:" would be better in general and avoid this problem.
>
> > I do worry that the similarity is going to lead to a certain amount of
> > confusion and use of the wrong tag. People have a hard time getting all
> > the tags we have now right; adding more that look almost like the
> > existing ones seems like a recipe for trouble.
> >
> > For these reasons, I would be more inclined toward Konstantin's
> > suggestion of adding notes to the existing Link: tags.

Exactly. The power of the "Link" tag is that it can refer to a
variety of related content. I.e. the meaning is derived from the
link target, which can be an email discussion, a bug report, a bug
tracker page, ...

A proliferation of tags complicates life for patch authors and commit
analyzers. IMHO adding tags should only be done as a last resort, as
it doesn't come without a cost.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2021-12-08 13:41:53

by Thorsten Leemhuis

[permalink] [raw]
Subject: Re: [RFC PATCH v1 1/1] docs: add the new commit-msg tags 'Reported:' and 'Reviewed:'

Hi Eric!

On 30.11.21 09:24, Geert Uytterhoeven wrote:
> On Mon, Nov 29, 2021 at 11:29 PM Eric Wong <[email protected]> wrote:
>> It's a bit much for common cases with git-send-email and
>> reasonable MUAs, I think. I don't know if formail is commonly
>> installed, nowadays...

Well, after your earlier suggestion I considered to go with this:

- perl -pi -e 's|^Message-Id:\s*<?([^>]+)>?$|Link:
https://lore.kernel.org/r/$1|g;' "$1"
+ perl -pi -e 's|^Message-ID:\s*<?([^>]+)>?$|Link:
https://lore.kernel.org/r/$1|i;' "$1"

But...

> Of course ;-) You need it to run checkpatch on patch series obtained
> through "b4 am", before you apply them to your tree:
>
> $ cat *mbx | formail -s scripts/checkpatch.pl

...this made me wonder if formail would be the better solution. I came
up with this:

formail -A "Link: https://lore.kernel.org/r/`formail -c -x Message-ID <
"${1}" | sed 's!.*<\(.*\)>!\1!'`" < "${1}" | sponge "${1}"

Downsides: instead of perl it requires sed and sponge (part of
moreutils, which I guess not everyone has installed; but I tried to
avoid a big here document or moving files around).

Is that worth it? Or is there a way to realize this in a more elegant
fashion with tools everyone has installed?

Ciao, Thorsten

2021-12-08 17:02:31

by Eric Wong

[permalink] [raw]
Subject: Re: [RFC PATCH v1 1/1] docs: add the new commit-msg tags 'Reported:' and 'Reviewed:'

Thorsten Leemhuis <[email protected]> wrote:
> On 30.11.21 09:24, Geert Uytterhoeven wrote:
> > On Mon, Nov 29, 2021 at 11:29 PM Eric Wong <[email protected]> wrote:
> >> It's a bit much for common cases with git-send-email and
> >> reasonable MUAs, I think. I don't know if formail is commonly
> >> installed, nowadays...
>
> Well, after your earlier suggestion I considered to go with this:
>
> - perl -pi -e 's|^Message-Id:\s*<?([^>]+)>?$|Link:
> https://lore.kernel.org/r/$1|g;' "$1"
> + perl -pi -e 's|^Message-ID:\s*<?([^>]+)>?$|Link:
> https://lore.kernel.org/r/$1|i;' "$1"
>
> But...
>
> > Of course ;-) You need it to run checkpatch on patch series obtained
> > through "b4 am", before you apply them to your tree:
> >
> > $ cat *mbx | formail -s scripts/checkpatch.pl
>
> ...this made me wonder if formail would be the better solution. I came
> up with this:
>
> formail -A "Link: https://lore.kernel.org/r/`formail -c -x Message-ID <
> "${1}" | sed 's!.*<\(.*\)>!\1!'`" < "${1}" | sponge "${1}"
>
> Downsides: instead of perl it requires sed and sponge (part of
> moreutils, which I guess not everyone has installed; but I tried to
> avoid a big here document or moving files around).

As Geert noted, formail is probably reasonable, but I certainly
don't have moreutils across all the systems I'm using right now.

> Is that worth it? Or is there a way to realize this in a more elegant
> fashion with tools everyone has installed?

*shrug* Since newlines after ':' are a concern and it's (probably :P)
safe to slurp entire contents of emails into memory nowadays;
some minor tweaks to the original perl invocation should work:

* use `$/ = undef' to force Perl to operate on the entire input at once
* use `m' RE modifier to ensure `^' and `$' still match SOL/EOL
($/ is only the input record separator, it doesn't change
Perl's definition of "lines" for `^' and `$')

perl -i -p -e 'BEGIN{$/=undef};s|^Message-ID:\s*<?([^>]+)>?$|Link:
https://lore.kernel.org/r/$1|im;'