Re: [PATCH] send-email: clear the $message_id after validation

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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" '



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux