* bug report: cover letter is inheriting last patch's message ID with send-email
@ 2023-05-17 18:38 Emily Shaffer
2023-05-17 19:01 ` Junio C Hamano
0 siblings, 1 reply; 10+ messages in thread
From: Emily Shaffer @ 2023-05-17 18:38 UTC (permalink / raw)
To: Git List; +Cc: michael.strawbridge, dianders
[-- Attachment #1: Type: text/plain, Size: 3675 bytes --]
Following is a report from inside of Google:
**What did you do before the bug happened? (Steps to reproduce your issue)**
```
# With the attached patches, where all of the patches have a
# Message-Id but the cover letter doesn't.
git send-email *.patch
```
Specifically, you can see me doing it:
```
$ git send-email *.patch
0000-cover-letter.patch
0001-dt-bindings-interrupt-controller-arm-gic-v3-Add-quir.patch
0002-irqchip-gic-v3-Disable-pseudo-NMIs-on-Mediatek-devic.patch
0003-arm64-dts-mediatek-mt8183-Add-mediatek-gicr-save-qui.patch
0004-arm64-dts-mediatek-mt8186-Add-mediatek-gicr-save-qui.patch
0005-arm64-dts-mediatek-mt8192-Add-mediatek-gicr-save-qui.patch
0006-arm64-dts-mediatek-mt8195-Add-mediatek-gicr-save-qui.patch
To whom should the emails be sent (if anyone)?
Message-ID to be used as In-Reply-To for the first email (if any)?
(mbox) Adding cc: Douglas Anderson <dianders@chromium.org> from line
'From: Douglas Anderson <dianders@chromium.org>'
From: Douglas Anderson <dianders@chromium.org>
To:
Cc: Douglas Anderson <dianders@chromium.org>
Subject: [PATCH 0/6] irqchip/gic-v3: Disable pseudo NMIs on Mediatek
Chromebooks w/ bad FW
Date: Thu, 11 May 2023 15:25:55 -0700
Message-ID: <20230511151719.6.Ia0b6ebbaa351e3cd67e201355b9ae67783c7d718@changeid>
```
If you look at `0000-cover-letter.patch` you can see that it has no
Message-ID, but the above clearly shows that the cover letter is being
sent with a Message-ID (and the one from the last patch).
**What did you expect to happen? (Expected behavior)**
The cover letter should get an auto-generated Message-Id like it always used to.
**What happened instead? (Actual behavior)**
The cover letter ends up with the same Message-Id as the last patch.
**What's different between what you expected and what actually happened?**
See above.
**Anything else you want to add:**
This happens when using the `patman` tool to send patches with a cover
letter. The individual patches encode the "Change-Id" in the
Message-ID but the cover letter doesn't need any special Message-Id.
I didn't notice this until after I sent a patch series and thus:
https://lore.kernel.org/r/20230511150539.6.Ia0b6ebbaa351e3cd67e201355b9ae67783c7d718@changeid/
...refers to both my cover letter and the last patch in the series.
**Please review the rest of the bug report below.
You can delete any lines you don't wish to share.**
```
[System Info]
git version:
git version 2.40.1.606.ga4b1b128d6-goog
cpu: x86_64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
uname: Linux 6.1.15-1rodete3-amd64 #1 SMP PREEMPT_DYNAMIC Debian
6.1.15-1rodete3 (2023-03-28) x86_64
compiler info: gnuc: 12.2
libc info: glibc: 2.36
$SHELL (typically, interactive shell): /bin/bash
[Enabled Hooks]
commit-msg
pre-auto-gc
pre-commit
prepare-commit-msg
```
The report references an internal build based on 2.40.1, but we were
able to reproduce on upstream git version 2.41.0.rc0.4.g004e0f790f
(without using `patman`).
Note that the internal report did come with the output of `git
format-patch` as a zipped attachment, which I tried to attach and am
hoping won't get filtered. As described in the report, the patches
themselves already contain Message-IDs, but the cover letter does not.
At the mail linked in the bug, you can notice that patch 6 has two
Message-IDs for some reason, and the one that's clearly not generated
by the `patman` helper is appended to patch 6 instead of patch 0.
Bisecting shows that this bug was introduced at a8022c5f7b
(send-email: expose header information to git-send-email's
sendemail-validate hook, 2023-04-19), thanks Josh for bisecting it.
- Emily
[-- Attachment #2: gitpatches.tgz --]
[-- Type: application/x-compressed-tar, Size: 4705 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: bug report: cover letter is inheriting last patch's message ID with send-email
2023-05-17 18:38 bug report: cover letter is inheriting last patch's message ID with send-email Emily Shaffer
@ 2023-05-17 19:01 ` Junio C Hamano
2023-05-17 19:22 ` Junio C Hamano
2023-05-17 19:24 ` Doug Anderson
0 siblings, 2 replies; 10+ messages in thread
From: Junio C Hamano @ 2023-05-17 19:01 UTC (permalink / raw)
To: Emily Shaffer; +Cc: Git List, michael.strawbridge, dianders
Emily Shaffer <nasamuffin@google.com> writes:
> Following is a report from inside of Google:
>
> **What did you do before the bug happened? (Steps to reproduce your issue)**
>
> ```
> # With the attached patches, where all of the patches have a
> # Message-Id but the cover letter doesn't.
> git send-email *.patch
> ```
>
> Specifically, you can see me doing it:
>
> ```
> $ git send-email *.patch
> 0000-cover-letter.patch
> 0001-dt-bindings-interrupt-controller-arm-gic-v3-Add-quir.patch
> ...
> 0006-arm64-dts-mediatek-mt8195-Add-mediatek-gicr-save-qui.patch
> To whom should the emails be sent (if anyone)?
> Message-ID to be used as In-Reply-To for the first email (if any)?
> (mbox) Adding cc: Douglas Anderson <dianders@chromium.org> from line
> 'From: Douglas Anderson <dianders@chromium.org>'
>
> From: Douglas Anderson <dianders@chromium.org>
> To:
> Cc: Douglas Anderson <dianders@chromium.org>
> Subject: [PATCH 0/6] irqchip/gic-v3: Disable pseudo NMIs on Mediatek
> Chromebooks w/ bad FW
> Date: Thu, 11 May 2023 15:25:55 -0700
> Message-ID: <20230511151719.6.Ia0b6ebbaa351e3cd67e201355b9ae67783c7d718@changeid>
> ```
>
> If you look at `0000-cover-letter.patch` you can see that it has no
> Message-ID, but the above clearly shows that the cover letter is being
> sent with a Message-ID (and the one from the last patch).
It is correct that Message-ID needs to be assigned by send-email if
the outgoing message lacks one. I am not sure what is meant by
"from the last patch". Do you mean that Message-ID exists in
0006-*.patch but not in 0000-cover-letter.patch [*]? I suspect that
is the root cause of the problem; if 000[1-6]-*.patch already has
their own Message-ID: because --thread is used when running
git-format-patch, they would also have In-Reply-To: and References:,
but there is no way for them to reference 0000-cover-letter.patch
(because format-patch did not get a chance to generate Message-ID to
it), is there?
Is this because format-patch was used without --cover-letter but
with --thread to prepare 000[1-6]*.patch and the cover letter was
created separately, or something?
The simplest fix I can think of is to stop using Message-ID related
options when running format-patch, and let send-email do the
threading. It would avoid problems coming from mixing output from
multiple format-patch runs.
[Footnote]
* As a reproduction recipe, the report should tell how these files
were prepared (format-patch with what arguments to get there).
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: bug report: cover letter is inheriting last patch's message ID with send-email
2023-05-17 19:01 ` Junio C Hamano
@ 2023-05-17 19:22 ` Junio C Hamano
2023-05-17 20:14 ` Doug Anderson
2023-05-18 0:51 ` Michael Strawbridge
2023-05-17 19:24 ` Doug Anderson
1 sibling, 2 replies; 10+ messages in thread
From: Junio C Hamano @ 2023-05-17 19:22 UTC (permalink / raw)
To: Emily Shaffer; +Cc: Git List, michael.strawbridge, dianders
Junio C Hamano <gitster@pobox.com> writes:
>> # With the attached patches, where all of the patches have a
>> # Message-Id but the cover letter doesn't.
>> git send-email *.patch
I suspect this is a recent regression with the addition of the
pre_process_file step. 56adddaa (send-email: refactor header
generation functions, 2023-04-19) makes all messages parsed
before the first message is sent out, by calling a sub
"pre_process_file" before invoking the validate hook. The same sub
is called again for each message when it is sent out, as the
processing in that step is shared between the time the message gets
vetted and the time the message gets sent.
Unfortunately, $message_id variable is assigned to in that sub. So
it is very much understandable why this happens.
I wonder if it is just doing something silly like this?
--- >8 ---
Subject: [PATCH] send-email: clear the $message_id after validation
Recently git-send-email started parsing the same message twice, once
to validate _all_ the message before sending even the first one, and
then after the validation hook is happy and each message gets sent,
to read the contents to find out where to send to etc.
Unfortunately, the effect of reading the messages for validation
lingered even after the validation is done. Namely $message_id gets
assigned if exists in the input files but the variable is global,
and it is not cleared before pre_process_file runs. This causes
reading a message without a message-id followed by reading a message
with a message-id to misbehave---the sub reports as if the message
had the same id as the previously written one.
Clear the variable before starting to read the headers in
pre_process_file
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* I am not surprised at all if there are similar problems in this
function around variables other than $message_id; this patch is
merely reacting to the bug report and not systematically hunting
and fixing the bugs coming from the same root cause. If the
original author of the pre_process_file change is still around,
the second sets of eyes from them is very much appreciated.
git-send-email.perl | 1 +
1 file changed, 1 insertion(+)
diff --git c/git-send-email.perl w/git-send-email.perl
index 89d8237e89..889ef388c8 100755
--- c/git-send-email.perl
+++ w/git-send-email.perl
@@ -1771,6 +1771,7 @@ sub send_message {
sub pre_process_file {
my ($t, $quiet) = @_;
+ undef $message_id;
open my $fh, "<", $t or die sprintf(__("can't open file %s"), $t);
my $author = undef;
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: bug report: cover letter is inheriting last patch's message ID with send-email
2023-05-17 19:01 ` Junio C Hamano
2023-05-17 19:22 ` Junio C Hamano
@ 2023-05-17 19:24 ` Doug Anderson
2023-05-17 20:04 ` Junio C Hamano
1 sibling, 1 reply; 10+ messages in thread
From: Doug Anderson @ 2023-05-17 19:24 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Emily Shaffer, Git List, michael.strawbridge
Hi,
On Wed, May 17, 2023 at 12:01 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Emily Shaffer <nasamuffin@google.com> writes:
>
> > Following is a report from inside of Google:
> >
> > **What did you do before the bug happened? (Steps to reproduce your issue)**
> >
> > ```
> > # With the attached patches, where all of the patches have a
> > # Message-Id but the cover letter doesn't.
> > git send-email *.patch
> > ```
> >
> > Specifically, you can see me doing it:
> >
> > ```
> > $ git send-email *.patch
> > 0000-cover-letter.patch
> > 0001-dt-bindings-interrupt-controller-arm-gic-v3-Add-quir.patch
> > ...
> > 0006-arm64-dts-mediatek-mt8195-Add-mediatek-gicr-save-qui.patch
> > To whom should the emails be sent (if anyone)?
> > Message-ID to be used as In-Reply-To for the first email (if any)?
> > (mbox) Adding cc: Douglas Anderson <dianders@chromium.org> from line
> > 'From: Douglas Anderson <dianders@chromium.org>'
> >
> > From: Douglas Anderson <dianders@chromium.org>
> > To:
> > Cc: Douglas Anderson <dianders@chromium.org>
> > Subject: [PATCH 0/6] irqchip/gic-v3: Disable pseudo NMIs on Mediatek
> > Chromebooks w/ bad FW
> > Date: Thu, 11 May 2023 15:25:55 -0700
> > Message-ID: <20230511151719.6.Ia0b6ebbaa351e3cd67e201355b9ae67783c7d718@changeid>
> > ```
> >
> > If you look at `0000-cover-letter.patch` you can see that it has no
> > Message-ID, but the above clearly shows that the cover letter is being
> > sent with a Message-ID (and the one from the last patch).
>
> It is correct that Message-ID needs to be assigned by send-email if
> the outgoing message lacks one. I am not sure what is meant by
> "from the last patch". Do you mean that Message-ID exists in
> 0006-*.patch but not in 0000-cover-letter.patch [*]?
Yes. It exists in all of the patches except 0000-cover-letter.patch.
...but when the mail gets actually sent the cover letter and last
patch (0006 in the case I reported) end up sharing the same Change ID.
With older versions of git send-email the cover letter would get an
auto-generated Message-Id.
> I suspect that
> is the root cause of the problem; if 000[1-6]-*.patch already has
> their own Message-ID: because --thread is used when running
> git-format-patch, they would also have In-Reply-To: and References:,
> but there is no way for them to reference 0000-cover-letter.patch
> (because format-patch did not get a chance to generate Message-ID to
> it), is there?
The patches were generated with git-format-patch but the Message-ID
was added by patman [1]. The Message-ID encodes the local Change-Id
which can make it easier to associate one version of the same patch
with another (same reason gerrit uses Change-Id) [2]. There is no
Change-Id associated with the cover letter so patman doesn't bother
adding one there and has always just let it be auto-generated. We
could certainly change patman to make up a Message-Id for the cover
letter, but there is no real need.
[1] https://source.denx.de/u-boot/u-boot/-/blob/master/tools/patman/patman.rst
[2] https://source.denx.de/u-boot/u-boot/-/commit/833e4192cd791733ddc0106996a4f86f9269ceba
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: bug report: cover letter is inheriting last patch's message ID with send-email
2023-05-17 19:24 ` Doug Anderson
@ 2023-05-17 20:04 ` Junio C Hamano
2023-05-17 20:20 ` Doug Anderson
0 siblings, 1 reply; 10+ messages in thread
From: Junio C Hamano @ 2023-05-17 20:04 UTC (permalink / raw)
To: Doug Anderson; +Cc: Emily Shaffer, Git List, michael.strawbridge
Doug Anderson <dianders@chromium.org> writes:
> Yes. It exists in all of the patches except 0000-cover-letter.patch.
> ...but when the mail gets actually sent the cover letter and last
> patch (0006 in the case I reported) end up sharing the same Change ID.
> With older versions of git send-email the cover letter would get an
> auto-generated Message-Id.
Yeah, I think the patch I sent in the thread should help; I'd
appreciate it if you folks can test and verify.
>> I suspect that
>> is the root cause of the problem; if 000[1-6]-*.patch already has
>> their own Message-ID: because --thread is used when running
>> git-format-patch, they would also have In-Reply-To: and References:,
>> but there is no way for them to reference 0000-cover-letter.patch
>> (because format-patch did not get a chance to generate Message-ID to
>> it), is there?
>
> The patches were generated with git-format-patch but the Message-ID
> was added by patman [1]. The Message-ID encodes the local Change-Id
> which can make it easier to associate one version of the same patch
> with another (same reason gerrit uses Change-Id) [2]. There is no
> Change-Id associated with the cover letter so patman doesn't bother
> adding one there and has always just let it be auto-generated.
> We
> could certainly change patman to make up a Message-Id for the cover
> letter, but there is no real need.
This is a tangent, as I think the earlier patch should fix the
regression, but wouldn't a recipient of such a series have a hard
time to locate and group the patches in the same series with the
cover letter, without having In-Reply-To: or References: that links
the later message back to the initial message (i.e. cover letter)?
Assigning a Message-ID to the cover, and referencing it from the
patches via In-Reply-To:, is what is commonly done, I think, for
that kind of threading.
Thanks.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: bug report: cover letter is inheriting last patch's message ID with send-email
2023-05-17 19:22 ` Junio C Hamano
@ 2023-05-17 20:14 ` Doug Anderson
2023-05-17 20:21 ` Junio C Hamano
2023-05-18 0:51 ` Michael Strawbridge
1 sibling, 1 reply; 10+ messages in thread
From: Doug Anderson @ 2023-05-17 20:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Emily Shaffer, Git List, michael.strawbridge
Hi,
On Wed, May 17, 2023 at 12:22 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> >> # With the attached patches, where all of the patches have a
> >> # Message-Id but the cover letter doesn't.
> >> git send-email *.patch
>
> I suspect this is a recent regression with the addition of the
> pre_process_file step. 56adddaa (send-email: refactor header
> generation functions, 2023-04-19) makes all messages parsed
> before the first message is sent out, by calling a sub
> "pre_process_file" before invoking the validate hook. The same sub
> is called again for each message when it is sent out, as the
> processing in that step is shared between the time the message gets
> vetted and the time the message gets sent.
>
> Unfortunately, $message_id variable is assigned to in that sub. So
> it is very much understandable why this happens.
>
> I wonder if it is just doing something silly like this?
>
> --- >8 ---
> Subject: [PATCH] send-email: clear the $message_id after validation
>
> Recently git-send-email started parsing the same message twice, once
> to validate _all_ the message before sending even the first one, and
> then after the validation hook is happy and each message gets sent,
> to read the contents to find out where to send to etc.
>
> Unfortunately, the effect of reading the messages for validation
> lingered even after the validation is done. Namely $message_id gets
> assigned if exists in the input files but the variable is global,
> and it is not cleared before pre_process_file runs. This causes
> reading a message without a message-id followed by reading a message
> with a message-id to misbehave---the sub reports as if the message
> had the same id as the previously written one.
>
> Clear the variable before starting to read the headers in
> pre_process_file
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>
> * I am not surprised at all if there are similar problems in this
> function around variables other than $message_id; this patch is
> merely reacting to the bug report and not systematically hunting
> and fixing the bugs coming from the same root cause. If the
> original author of the pre_process_file change is still around,
> the second sets of eyes from them is very much appreciated.
>
> git-send-email.perl | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git c/git-send-email.perl w/git-send-email.perl
> index 89d8237e89..889ef388c8 100755
> --- c/git-send-email.perl
> +++ w/git-send-email.perl
> @@ -1771,6 +1771,7 @@ sub send_message {
> sub pre_process_file {
> my ($t, $quiet) = @_;
>
> + undef $message_id;
> open my $fh, "<", $t or die sprintf(__("can't open file %s"), $t);
>
> my $author = undef;
I can confirm this fixes the regression for me. Thus:
Tested-by: Douglas Anderson <dianders@chromium.org>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: bug report: cover letter is inheriting last patch's message ID with send-email
2023-05-17 20:04 ` Junio C Hamano
@ 2023-05-17 20:20 ` Doug Anderson
0 siblings, 0 replies; 10+ messages in thread
From: Doug Anderson @ 2023-05-17 20:20 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Emily Shaffer, Git List, michael.strawbridge
Hi,
On Wed, May 17, 2023 at 1:04 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Doug Anderson <dianders@chromium.org> writes:
>
> > Yes. It exists in all of the patches except 0000-cover-letter.patch.
> > ...but when the mail gets actually sent the cover letter and last
> > patch (0006 in the case I reported) end up sharing the same Change ID.
> > With older versions of git send-email the cover letter would get an
> > auto-generated Message-Id.
>
> Yeah, I think the patch I sent in the thread should help; I'd
> appreciate it if you folks can test and verify.
Yup, tested. It works!
> >> I suspect that
> >> is the root cause of the problem; if 000[1-6]-*.patch already has
> >> their own Message-ID: because --thread is used when running
> >> git-format-patch, they would also have In-Reply-To: and References:,
> >> but there is no way for them to reference 0000-cover-letter.patch
> >> (because format-patch did not get a chance to generate Message-ID to
> >> it), is there?
> >
> > The patches were generated with git-format-patch but the Message-ID
> > was added by patman [1]. The Message-ID encodes the local Change-Id
> > which can make it easier to associate one version of the same patch
> > with another (same reason gerrit uses Change-Id) [2]. There is no
> > Change-Id associated with the cover letter so patman doesn't bother
> > adding one there and has always just let it be auto-generated.
>
> > We
> > could certainly change patman to make up a Message-Id for the cover
> > letter, but there is no real need.
>
> This is a tangent, as I think the earlier patch should fix the
> regression, but wouldn't a recipient of such a series have a hard
> time to locate and group the patches in the same series with the
> cover letter, without having In-Reply-To: or References: that links
> the later message back to the initial message (i.e. cover letter)?
> Assigning a Message-ID to the cover, and referencing it from the
> patches via In-Reply-To:, is what is commonly done, I think, for
> that kind of threading.
It has always magically worked.
For instance, looking at a patch series I sent before the regression.
You can see the cover letter here with an automatically-assigned
Message-Id:
https://lore.kernel.org/linux-arm-kernel/20230504221349.1535669-1-dianders@chromium.org/
You can then look at patch #1, which had a Message-Id assigned to it
by patman (by simply adding a "Message-Id" line to the patch file
after git format-patch but before calling git send-email):
https://lore.kernel.org/linux-arm-kernel/20230504151100.v4.1.I8cbb2f4fa740528fcfade4f5439b6cdcdd059251@changeid/
You can see that it properly references the cover letter. Specifically
in the raw message you can see:
In-Reply-To: <20230504221349.1535669-1-dianders@chromium.org>
References: <20230504221349.1535669-1-dianders@chromium.org>
-Doug
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: bug report: cover letter is inheriting last patch's message ID with send-email
2023-05-17 20:14 ` Doug Anderson
@ 2023-05-17 20:21 ` Junio C Hamano
0 siblings, 0 replies; 10+ messages in thread
From: Junio C Hamano @ 2023-05-17 20:21 UTC (permalink / raw)
To: Doug Anderson; +Cc: Emily Shaffer, Git List, michael.strawbridge
Doug Anderson <dianders@chromium.org> writes:
> Hi,
>
> On Wed, May 17, 2023 at 12:22 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>> >> # With the attached patches, where all of the patches have a
>> >> # Message-Id but the cover letter doesn't.
>> >> git send-email *.patch
>>
>> I suspect this is a recent regression with the addition of the
>> pre_process_file step. 56adddaa (send-email: refactor header
>> generation functions, 2023-04-19) makes all messages parsed
>> before the first message is sent out, by calling a sub
>> "pre_process_file" before invoking the validate hook. The same sub
>> is called again for each message when it is sent out, as the
>> processing in that step is shared between the time the message gets
>> vetted and the time the message gets sent.
>>
>> Unfortunately, $message_id variable is assigned to in that sub. So
>> it is very much understandable why this happens.
>>
>> I wonder if it is just doing something silly like this?
>>
>> --- >8 ---
>> Subject: [PATCH] send-email: clear the $message_id after validation
>>
>> Recently git-send-email started parsing the same message twice, once
>> to validate _all_ the message before sending even the first one, and
>> then after the validation hook is happy and each message gets sent,
>> to read the contents to find out where to send to etc.
>>
>> Unfortunately, the effect of reading the messages for validation
>> lingered even after the validation is done. Namely $message_id gets
>> assigned if exists in the input files but the variable is global,
>> and it is not cleared before pre_process_file runs. This causes
>> reading a message without a message-id followed by reading a message
>> with a message-id to misbehave---the sub reports as if the message
>> had the same id as the previously written one.
>>
>> Clear the variable before starting to read the headers in
>> pre_process_file
>>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ---
>>
>> * I am not surprised at all if there are similar problems in this
>> function around variables other than $message_id; this patch is
>> merely reacting to the bug report and not systematically hunting
>> and fixing the bugs coming from the same root cause. If the
>> original author of the pre_process_file change is still around,
>> the second sets of eyes from them is very much appreciated.
>>
>> git-send-email.perl | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git c/git-send-email.perl w/git-send-email.perl
>> index 89d8237e89..889ef388c8 100755
>> --- c/git-send-email.perl
>> +++ w/git-send-email.perl
>> @@ -1771,6 +1771,7 @@ sub send_message {
>> sub pre_process_file {
>> my ($t, $quiet) = @_;
>>
>> + undef $message_id;
>> open my $fh, "<", $t or die sprintf(__("can't open file %s"), $t);
>>
>> my $author = undef;
>
> I can confirm this fixes the regression for me. Thus:
>
> Tested-by: Douglas Anderson <dianders@chromium.org>
Thanks.
Now I need to write (or trick somebody into writing) a test for this
;-)
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: bug report: cover letter is inheriting last patch's message ID with send-email
2023-05-17 19:22 ` Junio C Hamano
2023-05-17 20:14 ` Doug Anderson
@ 2023-05-18 0:51 ` Michael Strawbridge
2023-05-18 1:06 ` Junio C Hamano
1 sibling, 1 reply; 10+ messages in thread
From: Michael Strawbridge @ 2023-05-18 0:51 UTC (permalink / raw)
To: Junio C Hamano, Emily Shaffer; +Cc: Git List, dianders
On 2023-05-17 15:22, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>>> # With the attached patches, where all of the patches have a
>>> # Message-Id but the cover letter doesn't.
>>> git send-email *.patch
> I suspect this is a recent regression with the addition of the
> pre_process_file step. 56adddaa (send-email: refactor header
> generation functions, 2023-04-19) makes all messages parsed
> before the first message is sent out, by calling a sub
> "pre_process_file" before invoking the validate hook. The same sub
> is called again for each message when it is sent out, as the
> processing in that step is shared between the time the message gets
> vetted and the time the message gets sent.
>
> Unfortunately, $message_id variable is assigned to in that sub. So
> it is very much understandable why this happens.
>
> I wonder if it is just doing something silly like this?
>
> --- >8 ---
> Subject: [PATCH] send-email: clear the $message_id after validation
>
> Recently git-send-email started parsing the same message twice, once
> to validate _all_ the message before sending even the first one, and
> then after the validation hook is happy and each message gets sent,
> to read the contents to find out where to send to etc.
>
> Unfortunately, the effect of reading the messages for validation
> lingered even after the validation is done. Namely $message_id gets
> assigned if exists in the input files but the variable is global,
> and it is not cleared before pre_process_file runs. This causes
> reading a message without a message-id followed by reading a message
> with a message-id to misbehave---the sub reports as if the message
> had the same id as the previously written one.
>
> Clear the variable before starting to read the headers in
> pre_process_file
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>
> * I am not surprised at all if there are similar problems in this
> function around variables other than $message_id; this patch is
> merely reacting to the bug report and not systematically hunting
> and fixing the bugs coming from the same root cause. If the
> original author of the pre_process_file change is still around,
> the second sets of eyes from them is very much appreciated.
>
> git-send-email.perl | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git c/git-send-email.perl w/git-send-email.perl
> index 89d8237e89..889ef388c8 100755
> --- c/git-send-email.perl
> +++ w/git-send-email.perl
> @@ -1771,6 +1771,7 @@ sub send_message {
> sub pre_process_file {
> my ($t, $quiet) = @_;
>
> + undef $message_id;
> open my $fh, "<", $t or die sprintf(__("can't open file %s"), $t);
>
> my $author = undef;
Sorry I missed clearing $message_id in my initial patch. After going
through the variables again I believe it is the only one that is not
reset properly.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: bug report: cover letter is inheriting last patch's message ID with send-email
2023-05-18 0:51 ` Michael Strawbridge
@ 2023-05-18 1:06 ` Junio C Hamano
0 siblings, 0 replies; 10+ messages in thread
From: Junio C Hamano @ 2023-05-18 1:06 UTC (permalink / raw)
To: Michael Strawbridge; +Cc: Emily Shaffer, Git List, dianders
Michael Strawbridge <michael.strawbridge@amd.com> writes:
> Sorry I missed clearing $message_id in my initial patch. After going
> through the variables again I believe it is the only one that is not
> reset properly.
Excellent. Thank you very much for quickly checking.
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-05-18 1:06 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-17 18:38 bug report: cover letter is inheriting last patch's message ID with send-email Emily Shaffer
2023-05-17 19:01 ` Junio C Hamano
2023-05-17 19:22 ` Junio C Hamano
2023-05-17 20:14 ` Doug Anderson
2023-05-17 20:21 ` Junio C Hamano
2023-05-18 0:51 ` Michael Strawbridge
2023-05-18 1:06 ` Junio C Hamano
2023-05-17 19:24 ` Doug Anderson
2023-05-17 20:04 ` Junio C Hamano
2023-05-17 20:20 ` Doug Anderson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).