{"thread":{"id":"59762","subject":"[PATCH] send-email: clear the $message_id after validation","startedAt":"2023-05-17T21:10:45Z","lastAt":"2023-05-18T01:11:34Z","messageCount":2,"participants":["Junio C Hamano","Michael Strawbridge"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"477487","messageId":"xmqqzg62oe9c.fsf@gitster.g","threadId":"59762","inReplyTo":null,"subject":"[PATCH] send-email: clear the $message_id after validation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-17T21:10:39Z","receivedAt":"2023-05-17T21:10:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Recently git-send-email started parsing the same message twice, once\nto validate _all_ the message before sending even the first one, and\nthen after the validation hook is happy and each message gets sent,\nto read the contents to find out where to send to etc.\n\nUnfortunately, the effect of reading the messages for validation\nlingered even after the validation is done.  Namely $message_id gets\nassigned if exists in the input files but the variable is global,\nand it is not cleared before pre_process_file runs.  This causes\nreading a message without a message-id followed by reading a message\nwith a message-id to misbehave---the sub reports as if the message\nhad the same id as the previously written one.\n\nClear the variable before starting to read the headers in\npre_process_file.\n\nTested-by: Douglas Anderson <dianders@chromium.org>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * This time with a minimum test.  I eyeballed what variables are\n   assigned in pre_process_file and it _appears_ to me that most of\n   them are cleared in the function before it processes one file\n   (except for $message_num that gets incremented per invocation for\n   obvious reasons---and it does get reset to 0 before the real loop\n   calls the function before sending each message).  So $message_id\n   may indeed be the only one that needs fixing.\n\n   But that can hardly qualify as an exhaustive verification X-<.\n\n git-send-email.perl   |  2 ++\n t/t9001-send-email.sh | 17 ++++++++++++++++-\n 2 files changed, 18 insertions(+), 1 deletion(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 10c450ef68..37dfd4b8c5 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -1768,6 +1768,8 @@ sub pre_process_file {\n \t$subject = $initial_subject;\n \t$message = \"\";\n \t$message_num++;\n+\tundef $message_id;\n+\n \t# First unfold multiline header fields\n \twhile(<$fh>) {\n \t\tlast if /^\\s*$/;\ndiff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\nindex 36bb85d6b4..8d49eff91a 100755\n--- a/t/t9001-send-email.sh\n+++ b/t/t9001-send-email.sh\n@@ -47,7 +47,7 @@ clean_fake_sendmail () {\n \n test_expect_success $PREREQ 'Extract patches' '\n \tpatches=$(git format-patch -s --cc=\"One <one@example.com>\" --cc=two@example.com -n HEAD^1) &&\n-\tthreaded_patches=$(git format-patch -o threaded -s --in-reply-to=\"format\" HEAD^1)\n+\tthreaded_patches=$(git format-patch -o threaded --thread=shallow -s --in-reply-to=\"format\" HEAD^1)\n '\n \n # Test no confirm early to ensure remaining tests will not hang\n@@ -588,6 +588,21 @@ test_expect_success $PREREQ \"--validate hook supports header argument\" '\n \t\toutdir/000?-*.patch\n '\n \n+test_expect_success $PREREQ 'clear message-id before parsing a new message' '\n+\tclean_fake_sendmail &&\n+\techo true | write_script my-hooks/sendemail-validate &&\n+\ttest_config core.hooksPath my-hooks &&\n+\tGIT_SEND_EMAIL_NOTTY=1 \\\n+\tgit send-email --validate --to=recipient@example.com \\\n+\t\t--smtp-server=\"$(pwd)/fake.sendmail\" \\\n+\t\t$patches $threaded_patches &&\n+\tid0=$(grep \"^Message-ID: \" $threaded_patches) &&\n+\tid1=$(grep \"^Message-ID: \" msgtxt1) &&\n+\tid2=$(grep \"^Message-ID: \" msgtxt2) &&\n+\ttest \"z$id0\" = \"z$id2\" &&\n+\ttest \"z$id1\" != \"z$id2\"\n+'\n+\n for enc in 7bit 8bit quoted-printable base64\n do\n \ttest_expect_success $PREREQ \"--transfer-encoding=$enc produces correct header\" '\n-- \n2.41.0-rc0-4-g004e0f790f\n\n"},{"id":"477507","messageId":"4f35223e-b7ec-8c0c-dc67-b419e47f7f5d@amd.com","threadId":"59762","inReplyTo":"xmqqzg62oe9c.fsf@gitster.g","subject":"Re: [PATCH] send-email: clear the $message_id after validation","fromName":"Michael Strawbridge","fromEmail":"michael.strawbridge@amd.com","sentAt":"2023-05-18T01:11:21Z","receivedAt":"2023-05-18T01:11:34Z","isPatch":true,"sender":{"key":"michael.strawbridge@amd.com","avatar":null},"body":"On 2023-05-17 17:10, Junio C Hamano wrote:\n> Recently git-send-email started parsing the same message twice, once\n> to validate _all_ the message before sending even the first one, and\n> then after the validation hook is happy and each message gets sent,\n> to read the contents to find out where to send to etc.\n>\n> Unfortunately, the effect of reading the messages for validation\n> lingered even after the validation is done.  Namely $message_id gets\n> assigned if exists in the input files but the variable is global,\n> and it is not cleared before pre_process_file runs.  This causes\n> reading a message without a message-id followed by reading a message\n> with a message-id to misbehave---the sub reports as if the message\n> had the same id as the previously written one.\n>\n> Clear the variable before starting to read the headers in\n> pre_process_file.\n>\n> Tested-by: Douglas Anderson <dianders@chromium.org>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>\n>  * This time with a minimum test.  I eyeballed what variables are\n>    assigned in pre_process_file and it _appears_ to me that most of\n>    them are cleared in the function before it processes one file\n>    (except for $message_num that gets incremented per invocation for\n>    obvious reasons---and it does get reset to 0 before the real loop\n>    calls the function before sending each message).  So $message_id\n>    may indeed be the only one that needs fixing.\n>\n>    But that can hardly qualify as an exhaustive verification X-<.\n\n\nAfter going through this again - I came to the same conclusion that\n$message_id seems to be the only one that must be fixed.\n\nIt is true that $in_reply_to, $references, and $message_num get set\noutside the pre_process_file function.  I suppose if we wanted to be\nmore robust, we could move a copy of those to:\n\n1) before the validation loop\n\n<<here\n\n    foreach my $r (@real_files) {\n        $ENV{GIT_SENDEMAIL_FILE_COUNTER} = \"$num\";\n        pre_process_file($r, 1);\n        validate_patch($r, $target_xfer_encoding);\n        $num += 1;\n    }\n\n2) before the process_file loop\n\n<<here\n\nforeach my $t (@files) {\n    while (!process_file($t)) {\n        # user edited the file\n    }\n}\n\nHowever, if we do that there becomes a few more cascading changes with\nthe declaration of the variables being after their use if we do the above.\n\nie.\n\n# Variables we set as part of the loop over files\nour ($message_id, %mail, $subject, $in_reply_to, $references, $message,\n    $needs_confirm, $message_num, $ask_default);\n\nI'm not sure the full repercussions of moving all that around.  There\ncould be further cascades. I think a minimal change here may be best.\n\nAcked-by: Michael Strawbridge <michael.strawbridge@amd.com>\n\n>  git-send-email.perl   |  2 ++\n>  t/t9001-send-email.sh | 17 ++++++++++++++++-\n>  2 files changed, 18 insertions(+), 1 deletion(-)\n>\n> diff --git a/git-send-email.perl b/git-send-email.perl\n> index 10c450ef68..37dfd4b8c5 100755\n> --- a/git-send-email.perl\n> +++ b/git-send-email.perl\n> @@ -1768,6 +1768,8 @@ sub pre_process_file {\n>  \t$subject = $initial_subject;\n>  \t$message = \"\";\n>  \t$message_num++;\n> +\tundef $message_id;\n> +\n>  \t# First unfold multiline header fields\n>  \twhile(<$fh>) {\n>  \t\tlast if /^\\s*$/;\n> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\n> index 36bb85d6b4..8d49eff91a 100755\n> --- a/t/t9001-send-email.sh\n> +++ b/t/t9001-send-email.sh\n> @@ -47,7 +47,7 @@ clean_fake_sendmail () {\n>  \n>  test_expect_success $PREREQ 'Extract patches' '\n>  \tpatches=$(git format-patch -s --cc=\"One <one@example.com>\" --cc=two@example.com -n HEAD^1) &&\n> -\tthreaded_patches=$(git format-patch -o threaded -s --in-reply-to=\"format\" HEAD^1)\n> +\tthreaded_patches=$(git format-patch -o threaded --thread=shallow -s --in-reply-to=\"format\" HEAD^1)\n>  '\n>  \n>  # Test no confirm early to ensure remaining tests will not hang\n> @@ -588,6 +588,21 @@ test_expect_success $PREREQ \"--validate hook supports header argument\" '\n>  \t\toutdir/000?-*.patch\n>  '\n>  \n> +test_expect_success $PREREQ 'clear message-id before parsing a new message' '\n> +\tclean_fake_sendmail &&\n> +\techo true | write_script my-hooks/sendemail-validate &&\n> +\ttest_config core.hooksPath my-hooks &&\n> +\tGIT_SEND_EMAIL_NOTTY=1 \\\n> +\tgit send-email --validate --to=recipient@example.com \\\n> +\t\t--smtp-server=\"$(pwd)/fake.sendmail\" \\\n> +\t\t$patches $threaded_patches &&\n> +\tid0=$(grep \"^Message-ID: \" $threaded_patches) &&\n> +\tid1=$(grep \"^Message-ID: \" msgtxt1) &&\n> +\tid2=$(grep \"^Message-ID: \" msgtxt2) &&\n> +\ttest \"z$id0\" = \"z$id2\" &&\n> +\ttest \"z$id1\" != \"z$id2\"\n> +'\n> +\n>  for enc in 7bit 8bit quoted-printable base64\n>  do\n>  \ttest_expect_success $PREREQ \"--transfer-encoding=$enc produces correct header\" '\n"}]}