Re: bug report: cover letter is inheriting last patch's message ID with send-email

From: Junio C Hamano <gitster@pobox.com>
Date: May 17, 2023, 19:22 UTC
Message-ID: <xmqqjzx6pxuu.fsf@gitster.g>
In-Reply-To: <xmqqo7mipyt0.fsf@gitster.g>

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;