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

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

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