On 2023-05-17 17:10, Junio C Hamano wrote:
> 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.
>
> Tested-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
> Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx>
> ---
>
> * This time with a minimum test. I eyeballed what variables are
> assigned in pre_process_file and it _appears_ to me that most of
> them are cleared in the function before it processes one file
> (except for $message_num that gets incremented per invocation for
> obvious reasons---and it does get reset to 0 before the real loop
> calls the function before sending each message). So $message_id
> may indeed be the only one that needs fixing.
>
> But that can hardly qualify as an exhaustive verification X-<.
After going through this again - I came to the same conclusion that
$message_id seems to be the only one that must be fixed.
It is true that $in_reply_to, $references, and $message_num get set
outside the pre_process_file function. I suppose if we wanted to be
more robust, we could move a copy of those to:
1) before the validation loop
<<here
foreach my $r (@real_files) {
$ENV{GIT_SENDEMAIL_FILE_COUNTER} = "$num";
pre_process_file($r, 1);
validate_patch($r, $target_xfer_encoding);
$num += 1;
}
2) before the process_file loop
<<here
foreach my $t (@files) {
while (!process_file($t)) {
# user edited the file
}
}
However, if we do that there becomes a few more cascading changes with
the declaration of the variables being after their use if we do the above.
ie.
# Variables we set as part of the loop over files
our ($message_id, %mail, $subject, $in_reply_to, $references, $message,
$needs_confirm, $message_num, $ask_default);
I'm not sure the full repercussions of moving all that around. There
could be further cascades. I think a minimal change here may be best.
Acked-by: Michael Strawbridge <michael.strawbridge@xxxxxxx>
> git-send-email.perl | 2 ++
> t/t9001-send-email.sh | 17 ++++++++++++++++-
> 2 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 10c450ef68..37dfd4b8c5 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1768,6 +1768,8 @@ sub pre_process_file {
> $subject = $initial_subject;
> $message = "";
> $message_num++;
> + undef $message_id;
> +
> # First unfold multiline header fields
> while(<$fh>) {
> last if /^\s*$/;
> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
> index 36bb85d6b4..8d49eff91a 100755
> --- a/t/t9001-send-email.sh
> +++ b/t/t9001-send-email.sh
> @@ -47,7 +47,7 @@ clean_fake_sendmail () {
>
> test_expect_success $PREREQ 'Extract patches' '
> patches=$(git format-patch -s --cc="One <one@xxxxxxxxxxx>" --cc=two@xxxxxxxxxxx -n HEAD^1) &&
> - threaded_patches=$(git format-patch -o threaded -s --in-reply-to="format" HEAD^1)
> + threaded_patches=$(git format-patch -o threaded --thread=shallow -s --in-reply-to="format" HEAD^1)
> '
>
> # Test no confirm early to ensure remaining tests will not hang
> @@ -588,6 +588,21 @@ test_expect_success $PREREQ "--validate hook supports header argument" '
> outdir/000?-*.patch
> '
>
> +test_expect_success $PREREQ 'clear message-id before parsing a new message' '
> + clean_fake_sendmail &&
> + echo true | write_script my-hooks/sendemail-validate &&
> + test_config core.hooksPath my-hooks &&
> + GIT_SEND_EMAIL_NOTTY=1 \
> + git send-email --validate --to=recipient@xxxxxxxxxxx \
> + --smtp-server="$(pwd)/fake.sendmail" \
> + $patches $threaded_patches &&
> + id0=$(grep "^Message-ID: " $threaded_patches) &&
> + id1=$(grep "^Message-ID: " msgtxt1) &&
> + id2=$(grep "^Message-ID: " msgtxt2) &&
> + test "z$id0" = "z$id2" &&
> + test "z$id1" != "z$id2"
> +'
> +
> for enc in 7bit 8bit quoted-printable base64
> do
> test_expect_success $PREREQ "--transfer-encoding=$enc produces correct header" '
