Re: [PATCH v8 2/2] send-email: expose header information to git-send-email's sendemail-validate hook

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Wed, Jan 18 2023, Michael Strawbridge wrote:

> To allow further flexibility in the Git hook, the SMTP header
> information of the email which git-send-email intends to send, is now
> passed as the 2nd argument to the sendemail-validate hook.
>
> As an example, this can be useful for acting upon keywords in the
> subject or specific email addresses.
>
> As a consequence of needing all the header data, validation has been
> moved later in the sequence to right before sending the emails instead
> of at the beginning.

Ah, I see. I tested this (i.e. moving it back to the previous behavior)
and you did this change because you don't have the $sender variable yet.

I tried this quickly on top, which seems to work, i.e. now we do this in
the same order as before, but we just move the $sender code earlier:
	
	diff --git a/git-send-email.perl b/git-send-email.perl
	index d123dfd33d5..7e7681116bb 100755
	--- a/git-send-email.perl
	+++ b/git-send-email.perl
	@@ -787,6 +787,28 @@ sub is_format_patch_arg {
	 
	 @files = handle_backup_files(@files);
	 
	+if (defined $sender) {
	+	$sender =~ s/^\s+|\s+$//g;
	+	($sender) = expand_aliases($sender);
	+} else {
	+	$sender = $repoauthor->() || $repocommitter->() || '';
	+}
	+
	+# $sender could be an already sanitized address
	+# (e.g. sendemail.from could be manually sanitized by user).
	+# But it's a no-op to run sanitize_address on an already sanitized address.
	+$sender = sanitize_address($sender);
	+
	+if ($validate) {
	+	foreach my $f (@files) {
	+		unless (-p $f) {
	+		        pre_process_file($f, 1);
	+
	+			validate_patch($f, $target_xfer_encoding);
	+		}
	+	}
	+}
	+
	 if (@files) {
	 	unless ($quiet) {
	 		print $_,"\n" for (@files);
	@@ -1035,18 +1057,6 @@ sub file_declares_8bit_cte {
	 	}
	 }
	 
	-if (defined $sender) {
	-	$sender =~ s/^\s+|\s+$//g;
	-	($sender) = expand_aliases($sender);
	-} else {
	-	$sender = $repoauthor->() || $repocommitter->() || '';
	-}
	-
	-# $sender could be an already sanitized address
	-# (e.g. sendemail.from could be manually sanitized by user).
	-# But it's a no-op to run sanitize_address on an already sanitized address.
	-$sender = sanitize_address($sender);
	-
	 my $to_whom = __("To whom should the emails be sent (if anyone)?");
	 my $prompting = 0;
	 if (!@initial_to && !defined $to_cmd) {
	@@ -1120,16 +1130,6 @@ sub expand_one_alias {
	 
	 $time = time - scalar $#files;
	 
	-if ($validate) {
	-	foreach my $f (@files) {
	-		unless (-p $f) {
	-		        pre_process_file($f, 1);
	-
	-			validate_patch($f, $target_xfer_encoding);
	-		}
	-	}
	-}
	-
	 $in_reply_to = $initial_in_reply_to;
	 $references = $initial_in_reply_to || '';
	 $message_num = 0;

All tests pass with that, which is less good than it sounds, because
shouldn't your tests be checking whether we have this non--quiet
print-out of the files as expected before or after the validation hook
runs?

> +
> +			my ($recipients_ref, $to, $date, $gitversion, $cc, $ccline, $header) = gen_header();
> +
> +			require File::Temp;
> +			my ($header_filehandle, $header_filename) = File::Temp::tempfile(
> +                            ".gitsendemail.header.XXXXXX", DIR => $repo->repo_path());
> +			print $header_filehandle $header;
> +
>  			my @cmd = ("git", "hook", "run", "--ignore-missing",
>  				    $hook_name, "--");
> -			my @cmd_msg = (@cmd, "<patch>");
> -			my @cmd_run = (@cmd, $target);
> +			my @cmd_msg = (@cmd, "<patch>", "<header>");
> +			my @cmd_run = (@cmd, $target, $header_filename);
>  			$hook_error = system_or_msg(\@cmd_run, undef, "@cmd_msg");
> +			unlink($header_filehandle);
>  			chdir($cwd_save) or die("chdir: $!");

I'm still curious about the "stdin" question I asked in the last round.