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@xxxxxxxxxxx,bar@xxxxxxxxxxx.
>
> 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
