Re: [PATCH] send-email: move validation code below process_address_list

[Michael Strawbridge <michael.strawbridge@amd.com>]

On 10/25/23 02:50, Jeff King wrote:
> On Tue, Oct 24, 2023 at 04:19:43PM -0400, Michael Strawbridge wrote:
> 
>> Move validation logic below processing of email address lists so that
>> email validation gets the proper email addresses.
>>
>> This fixes email address validation errors when the optional
>> perl module Email::Valid is installed and multiple addresses are passed
>> in on a single to/cc argument like --to=foo@example.com,bar@example.com.
> 
> Is there a test we can include here?
> 
>> @@ -2023,6 +1999,30 @@ sub process_file {
>>  	return 1;
>>  }
>>  
>> +if ($validate) {
> 
> So the new spot is right before we call process_file() on each of the
> input files. It is a little hard to follow because of the number of
> functions defined inline in the middle of the script, but I think that
> is a reasonable spot. It is after we have called process_address_list()
> on to/cc/bcc, which I think fixes the regression. But it is also after
> we sanitize $reply_to, etc, which seems like a good idea.
> 
> But I think putting it down that far is the source of the test failures.
> 
> The culprit seems not to be the call to validate_patch() in the loop you
> moved, but rather pre_process_file(), which was added in your earlier
> a8022c5f7b (send-email: expose header information to git-send-email's
> sendemail-validate hook, 2023-04-19).
> 
> It looks like the issue is the global $message_num variable which is
> incremented by pre_process_file(). On line 1755 (on the current tip of
> master), we set it to 0. And your patch moves the validation across
> there (from line ~799 to ~2023).
> 
> And that's why the extra increments didn't matter when you added the
> calls to pre_process_file() in your earlier patch; they all happened
> before we reset $message_num to 0. But now they happen after.
> 
> To be fair, this is absolutely horrific perl code. There's over a
> thousand lines of function definitions, and then hidden in the middle
> are some global variable assignments!

I agree. Following where things are initialized seems to be especially troublesome.
> 
> So we have a few options, I think:
> 
>   1. Reset $message_num to 0 after validating (maybe we also need
>      to reset $in_reply_to, etc, set at the same time? I didn't check).
>      This feels like a hack.
> 
>   2. Move the validation down, but not so far down. Like maybe right
>      after we set up the @files list with the $compose.final name. This
>      is the smallest diff, but feels like we haven't really made the
>      world a better place.
> 
>   3. Move the $message_num, etc, initialization to right before we call
>      the process_file() loop, which is what expects to use them. Like
>      this (squashed into your patch):
> 
> diff --git a/git-send-email.perl b/git-send-email.perl
> index a898dbc76e..d44db14223 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1730,10 +1730,6 @@ sub send_message {
>  	return 1;
>  }
>  
> -$in_reply_to = $initial_in_reply_to;
> -$references = $initial_in_reply_to || '';
> -$message_num = 0;
> -
>  sub pre_process_file {
>  	my ($t, $quiet) = @_;
>  
> @@ -2023,6 +2019,10 @@ sub process_file {
>  	delete $ENV{GIT_SENDEMAIL_FILE_TOTAL};
>  }
>  
> +$in_reply_to = $initial_in_reply_to;
> +$references = $initial_in_reply_to || '';
> +$message_num = 0;
> +
>  foreach my $t (@files) {
>  	while (!process_file($t)) {
>  		# user edited the file
> 
The above patch was a great place to start.  Thank you!  In order to address
the fact that validation and actually sending the emails should have the same
initial conditions I created a new function to set the variables and call it
instead.
> That seems to make the test failures go away. It is still weird that the
> validation code is calling pre_process_file(), which increments
> $message_num, without us having set it up in any meaningful way. I'm not
> sure if there are bugs lurking there or not. I'm not impressed by the
> general quality of this code, and I'm kind of afraid to keep looking
> deeper.
> 
> -Peff