From: Michael Strawbridge <michael.strawbridge@amd.com>
To: Junio C Hamano <gitster@pobox.com>, git@vger.kernel.org
Cc: Doug Anderson <dianders@chromium.org>,
Emily Shaffer <nasamuffin@google.com>
Subject: Re: [PATCH] send-email: clear the $message_id after validation
Date: Wed, 17 May 2023 21:11:21 -0400 [thread overview]
Message-ID: <4f35223e-b7ec-8c0c-dc67-b419e47f7f5d@amd.com> (raw)
In-Reply-To: <xmqqzg62oe9c.fsf@gitster.g>
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@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>
> 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" '
prev parent reply other threads:[~2023-05-18 1:11 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-17 21:10 [PATCH] send-email: clear the $message_id after validation Junio C Hamano
2023-05-18 1:11 ` Michael Strawbridge [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4f35223e-b7ec-8c0c-dc67-b419e47f7f5d@amd.com \
--to=michael.strawbridge@amd.com \
--cc=dianders@chromium.org \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=nasamuffin@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).