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!
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
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
