[v2] send-email: move validation code below process_address_list

Commit Message

Michael Strawbridge Oct. 25, 2023, 6:51 p.m. UTC

From 67223238d9b1977d20b1286055d7f197e4d746e9 Mon Sep 17 00:00:00 2001
From: Michael Strawbridge <[e-mail]>
Date: Wed, 11 Oct 2023 16:13:13 -0400
Subject: [PATCH v2] send-email: move validation code below
 process_address_list

Move validation logic below processing of email address lists so that
email validation gets the proper email addresses.  As a side effect,
some initialization needed to be moved down.  In order for validation
and the actual email sending to have the same initial state, the
initialized variables that get modified by pre_process_file are
encapsulated in a new function.

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=[e-mail],[e-mail].
A new test was added to t9001 to expose failures with this case in the
future.

Fixes: a8022c5f7b67 ("send-email: expose header information to git-send-email's sendemail-validate hook")
Reported-by: Bagas Sanjaya <[e-mail]>
Signed-off-by: Michael Strawbridge <[e-mail]>
---
 git-send-email.perl   | 60 +++++++++++++++++++++++--------------------
 t/t9001-send-email.sh | 19 ++++++++++++++
 2 files changed, 51 insertions(+), 28 deletions(-)

Comments

Junio C Hamano Oct. 26, 2023, 12:44 p.m. UTC | #1

Michael Strawbridge <[e-mail]> writes:

> From 67223238d9b1977d20b1286055d7f197e4d746e9 Mon Sep 17 00:00:00 2001
> From: Michael Strawbridge <[e-mail]>
> Date: Wed, 11 Oct 2023 16:13:13 -0400
> Subject: [PATCH v2] send-email: move validation code below
>  process_address_list

Why do these in-body headers to lie about the author date?

By the way, the in-body header does seem to support the header line
folding (see the "subject" one here).

Michael Strawbridge Oct. 26, 2023, 1:11 p.m. UTC | #2

On 10/26/23 08:44, Junio C Hamano wrote:
> Michael Strawbridge <[e-mail]> writes:
> 
>> From 67223238d9b1977d20b1286055d7f197e4d746e9 Mon Sep 17 00:00:00 2001
>> From: Michael Strawbridge <[e-mail]>
>> Date: Wed, 11 Oct 2023 16:13:13 -0400
>> Subject: [PATCH v2] send-email: move validation code below
>>  process_address_list
> 
> Why do these in-body headers to lie about the author date?

Sorry.  They weren't meant to.  I have been amending my local commit
rather than making new ones for every version.  It seems that when
I do that, the author date stays at the first time the commit was
created.  I wasn't aware of that unintended side effect.
> 
> By the way, the in-body header does seem to support the header line
> folding (see the "subject" one here).

Patch

diff --git a/git-send-email.perl b/git-send-email.perl
index 288ea1ae80..ce22a5e06d 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) {
@@ -1754,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 +1995,38 @@  sub process_file {
 	return 1;
 }
 
+sub initialize_modified_loop_vars {
+	$in_reply_to = $initial_in_reply_to;
+	$references = $initial_in_reply_to || '';
+	$message_num = 0;
+}
+
+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";
+	initialize_modified_loop_vars();
+	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};
+}
+
+initialize_modified_loop_vars();
 foreach my $t (@files) {
 	while (!process_file($t)) {
 		# user edited the file
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 263db3ad17..ccff2ad647 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -633,6 +633,25 @@  test_expect_success $PREREQ "--validate respects absolute core.hooksPath path" '
 	test_cmp expect actual
 '
 
+test_expect_success $PREREQ "--validate hook supports multiple addresses in arguments" '
+	hooks_path="$(pwd)/my-hooks" &&
+	test_config core.hooksPath "$hooks_path" &&
+	test_when_finished "rm my-hooks.ran" &&
+	test_must_fail git send-email \
+		--from="Example <[e-mail]>" \
+		--to=[e-mail],[e-mail] \
+		--smtp-server="$(pwd)/fake.sendmail" \
+		--validate \
+		longline.patch 2>actual &&
+	test_path_is_file my-hooks.ran &&
+	cat >expect <<-EOF &&
+	fatal: longline.patch: rejected by sendemail-validate hook
+	fatal: command '"'"'git hook run --ignore-missing sendemail-validate -- <patch> <header>'"'"' died with exit code 1
+	warning: no patches were sent
+	EOF
+	test_cmp expect actual
+'
+
 test_expect_success $PREREQ "--validate hook supports header argument" '
 	write_script my-hooks/sendemail-validate <<-\EOF &&
 	if test "$#" -ge 2