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

["Strawbridge, Michael" <Michael.Strawbridge@amd.com>]

On Tue, Jan 17 2023, Ævar Arnfjörð Bjarmason wrote:
>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 general I understand your point.  However, I found in
git-send-email that code outside of functions is sprinkled in between
function declarations in several areas.  Lines 790-797 and 1011-1058 are
examples of some of what I mean.  Also, the original process_file call is
actually out of place (roughly line 2011-2015). Unfortunately some of this
sprinkled code has an impact on the header variables and so I needed to
move the validation code lower, closer to the process_file section but
before the intialization of in_reply_to, references, and message_num.
I was attempting to reduce the number of code changes.

You do bring up a good point about printing of the names of files
before validation causing a difference in output. I can attempt to move
some more of this code as well, if the change of output is deemed
important enough.
 
>>  $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...

I was trying to follow the convention that the original hook was using.
I'm not against changing this if the out of tree patches you speak of
are going to be rolled in soon.  However, I'd prefer not to delay this
patch if these other patches are far off. Thanks.

-- 
2.34.1