Re: [PATCH] send-email: move process_address_list earlier to avoid, uninitialized address error

[Jeff King <peff@peff.net>]

On Fri, Oct 13, 2023 at 04:25:57PM -0400, Michael Strawbridge wrote:

> > I did not look carefully at the flow of send-email, so this may or may
> > not be an issue. But what I think would be _really_ annoying is if you
> > asked to write a cover letter, went through the trouble of writing it,
> > and then send-email bailed due to some validation failure that could
> > have been checked earlier.
> >
> > There is probably a way to recover your work (presumably we leave it in
> > a temporary file somewhere), but it may not be entirely trivial,
> > especially for users who are not comfortable with advanced usage of
> > their editor. ;)
>
> As I was looking at covering the case of interactive input (--compose)
> to the fix I noticed that this seems to be at least partly handled by
> the $compose_filename code.  There is a nice output message telling
> you exactly where the intermediate version of the email you are
> composing is located if there are errors.  I took a quick look inside
> and can verify that any lost work should be minimal as long as someone
> knows how to edit files with their editor of choice.

OK, that makes me feel better about just moving the validation code. A
more complicated solution could be do to do _some_ basic checks up
front, and then more complete validation later. But even if we wanted to
do that, moving the bulk of the validation (as discussed in this thread)
would probably be the first step anyway (and if nobody complains, maybe
we can avoid doing the extra work).

I do wonder if we might find other interesting corner cases where
the validation code (or somebody's hook) isn't happy with seeing the
more "full" picture (i.e., with the extra addresses from interactive and
command-line input). But arguably any such case would be indicative of a
bug, and smoking it out would be a good thing.

> I have been looking into handling the interactive input cases while
> solving this issue, but have yet to make a breakthrough.  Simply
> moving the validation code below the original process_address_list
> code results in a a scenario where I get the email address being seen
> as something like "ARRAY (0x55ddb951d768)" rather than the email
> address I wrote in the compose buffer.

Sounds like something is making a perl ref that shouldn't (or something
that should be dereferencing it not doing so). If you post your patch
and a reproduction command, I might be able to help debug.

But just blindly moving the validation code down, like:

diff --git a/git-send-email.perl b/git-send-email.perl
index 288ea1ae80..76589c7827 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -799,30 +799,6 @@ sub is_format_patch_arg {
 
 $time = time - scalar $#files;
 
-if ($validate) {
-	# FIFOs can only be read once, exclude them from validation.
-	my @real_files = ();
-	foreach my $f (@files) {
-		unless (-p $f) {
-			push(@real_files, $f);
-		}
-	}
-
-	# Run the loop once again to avoid gaps in the counter due to FIFO
-	# arguments provided by the user.
-	my $num = 1;
-	my $num_files = scalar @real_files;
-	$ENV{GIT_SENDEMAIL_FILE_TOTAL} = "$num_files";
-	foreach my $r (@real_files) {
-		$ENV{GIT_SENDEMAIL_FILE_COUNTER} = "$num";
-		pre_process_file($r, 1);
-		validate_patch($r, $target_xfer_encoding);
-		$num += 1;
-	}
-	delete $ENV{GIT_SENDEMAIL_FILE_COUNTER};
-	delete $ENV{GIT_SENDEMAIL_FILE_TOTAL};
-}
-
 @files = handle_backup_files(@files);
 
 if (@files) {
@@ -1121,6 +1097,30 @@ sub expand_one_alias {
 	$reply_to = sanitize_address($reply_to);
 }
 
+if ($validate) {
+	# FIFOs can only be read once, exclude them from validation.
+	my @real_files = ();
+	foreach my $f (@files) {
+		unless (-p $f) {
+			push(@real_files, $f);
+		}
+	}
+
+	# Run the loop once again to avoid gaps in the counter due to FIFO
+	# arguments provided by the user.
+	my $num = 1;
+	my $num_files = scalar @real_files;
+	$ENV{GIT_SENDEMAIL_FILE_TOTAL} = "$num_files";
+	foreach my $r (@real_files) {
+		$ENV{GIT_SENDEMAIL_FILE_COUNTER} = "$num";
+		pre_process_file($r, 1);
+		validate_patch($r, $target_xfer_encoding);
+		$num += 1;
+	}
+	delete $ENV{GIT_SENDEMAIL_FILE_COUNTER};
+	delete $ENV{GIT_SENDEMAIL_FILE_TOTAL};
+}
+
 if (!defined $sendmail_cmd && !defined $smtp_server) {
 	my @sendmail_paths = qw( /usr/sbin/sendmail /usr/lib/sendmail );
 	push @sendmail_paths, map {"$_/sendmail"} split /:/, $ENV{PATH};

seems to fix the problem from this thread and passes the existing tests.
Manually inspecting the result (and what's fed to the validation hook) I
don't see anything odd (like "ARRAY (...)").

-Peff