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

From: Jeff King <peff@peff.net>
Date: Oct 20, 2023, 07:14 UTC
Message-ID: <20231020071402.GC1642714@coredump.intra.peff.net>
In-Reply-To: <20231020064525.GB1642714@coredump.intra.peff.net>

On Fri, Oct 20, 2023 at 02:45:25AM -0400, Jeff King wrote:

Show 10 quoted lines

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

Ah, your "address I wrote in the compose buffer" was the clue I needed.

I think this is actually an existing bug. If I use --compose and write:

  To: foo@example.com

in the editor, we read that back in and handle it in parse_header_line() like:

        my $addr_pat = join "|", qw(To Cc Bcc);

        foreach (split(/\n/, $lines)) {
                if (/^($addr_pat):\s*(.+)$/i) {
                        $parsed_line->{$1} = [ parse_address_line($2) ];
                } elsif (/^([^:]*):\s*(.+)\s*$/i) {
                        $parsed_line->{$1} = $2;
                }
        }

and there's your perl array ref (from the square brackets, which are necessary because we're sticking it in a hash value). But even before your patch, this seems to end up as garbage. The code which reads $parsed_line does not dereference the array.

The patch to fix it is only a few lines (well, more than that with some light editorializing in the comments):

diff --git a/git-send-email.perl b/git-send-email.perl
index 76589c7827..46a30088c9 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -918,7 +918,28 @@ sub get_patch_subject {
 	# Preserve unknown headers
 	foreach my $key (keys %parsed_email) {
 		next if $key eq 'body';
-		print $c2 "$key: $parsed_email{$key}";
+
+		# it seems like it would be easier to just look for
+		# $parsed_email{'To'} and so on. But we actually match
+		# these case-insenstively and preserve the user's spelling, so
+		# we might see $parsed_email{'to'}. Of course, the same bug
+		# exists for Subject, etc, above. Anyway, a "/i" regex here
+		# handles all cases.
+		#
+		# It kind of feels like all of this code would be much simpler
+		# if we just handled all of the headers while reading back the
+		# input, rather than stuffing them all into $parsed_email and
+		# then picking them out of it.
+		#
+		# It also really feels like these to/cc/bcc lines should be
+		# added to the regular ones? It is silly to make a cover letter
+		# that goes to some addresses, and then not send the patches to
+		# them, too.
+		if ($key =~ /^(To|Cc|Bcc)$/i) {
+			print $c2 "$key: ", join(', ', @{$parsed_email{$key}});
+		} else {
+			print $c2 "$key: $parsed_email{$key}";
+		}
 	}
 
 	if ($parsed_email{'body'}) {

I don't really think your patch makes things worse here. But it is
probably worth fixing it while we are here.

-Peff