Re: [PATCH] send-email: clear the $message_id after validation
- From
- Michael Strawbridge <michael.strawbridge@amd.com>
- Date
- May 18, 2023, 01:11 UTC
- Message-ID
- <4f35223e-b7ec-8c0c-dc67-b419e47f7f5d@amd.com>
- In-Reply-To
- <xmqqzg62oe9c.fsf@gitster.g>
On 2023-05-17 17:10, Junio C Hamano wrote:
Show 29 quoted lines
> 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@chromium.org> > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > > * 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@amd.com>
Show 52 quoted lines
> 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@example.com>" --cc=two@example.com -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@example.com \
> + --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" '