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

>On Tue, Jan 10 2023, Strawbridge, Michael wrote:
>
>> To allow further flexibility in the git hook, the SMTP header
>> information of the email that git-send-email intends to send, is now
>> passed as a 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.
>
>Okey, but...
>
>> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
>> index a16e62bc8c..2b5c6640cc 100644
>> --- a/Documentation/githooks.txt
>> +++ b/Documentation/githooks.txt
>> @@ -583,10 +583,19 @@ processed by rebase.
>>  sendemail-validate
>>  ~~~~~~~~~~~~~~~~~~
>>  
>> -This hook is invoked by linkgit:git-send-email[1].  It takes a single parameter,
>> -the name of the file that holds the e-mail to be sent.  Exiting with a
>> -non-zero status causes `git send-email` to abort before sending any
>> -e-mails.
>> +This hook is invoked by linkgit:git-send-email[1].
>> +
>> +It takes these command line arguments:
>> +1. the name of the file that holds the e-mail to be sent.
>> +2. the name of the file that holds the SMTP headers to be used.
>> +
>> +The hook doesn't need to support multiple header names (for example only Cc
>> +is passed). However, it does need to understand that lines beginning with
>> +whitespace belong to the previous header.  The header information follows
>> +the same format as the confirmation given at the end of send-email.
>> +
>> +Exiting with a non-zero status causes `git send-email` to abort
>> +before sending any e-mails.
>>  
>>  fsmonitor-watchman
>>  ~~~~~~~~~~~~~~~~~~
>> diff --git a/git-send-email.perl b/git-send-email.perl
>> index 810dd1f1ce..b2adca515e 100755
>> --- a/git-send-email.perl
>> +++ b/git-send-email.perl
>> @@ -787,14 +787,6 @@ sub is_format_patch_arg {
>>  
>>  @files = handle_backup_files(@files);
>>  
>> -if ($validate) {
>> -	foreach my $f (@files) {
>> -		unless (-p $f) {
>> -			validate_patch($f, $target_xfer_encoding);
>> -		}
>> -	}
>> -}
>> -
>>  if (@files) {
>>  	unless ($quiet) {
>>  		print $_,"\n" for (@files);
>> @@ -1738,6 +1730,16 @@ sub send_message {
>>  	return 1;
>>  }
>>  
>> +if ($validate) {
>> +	foreach my $f (@files) {
>> +		unless (-p $f) {
>> +		        pre_process_file($f, 1);
>> +
>> +			validate_patch($f, $target_xfer_encoding);
>> +		}
>> +	}
>> +}
>
>...here we have the seemingly unrelated change of first doing the
>validation before this, and if we pass it we'll print the names of the
>files we're sending unless --quiet.
>
>Now we'll do it the other way around, maybe that's good, or maybe not,
>but your updated docs don't say.
>
>Also (and I didn't look at this all that carefully), why are you moving
>the control logic to between the later function declarations?
>
>Perl isn't a language where you need to arrange your source in that way
>(unless a bareword is involved, or if this happens at BEGIN time or
>whatever). The current structure is:
>
>	<use & imports>
>	<basic setup (getopts etc)>
>	<main logic>
>	<helper function>
>
>Here you're moving part of the main logic to in-between two helper
>function, why?
>

>>  $in_reply_to = $initial_in_reply_to;
>>  $references = $initial_in_reply_to || '';
>>  $message_num = 0;
>> @@ -2101,11 +2103,20 @@ sub validate_patch {
>>  			chdir($repo->wc_path() or $repo->repo_path())
>>  				or die("chdir: $!");
>>  			local $ENV{"GIT_DIR"} = $repo->repo_path();
>> +
>> +			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 know "git hook run" doesn't support input on stdin yet, but isn't this
>just working around it not supporting that? That seems like a much
>better & natural interface than what we're doing here.
>
>I have out-of-tree patches for that (or rather, a re-roll of Emily's
>patches to do that), if that landed in-tree could this use that
>interface, do you think?
>
>I'd rather that we didn't forever codify a strange interface here due to
>a temporary limitation in "git hook" and the hook API...