Re: [PATCH v4 1/1] Expose header information to git-send-email's sendemail-validate hook

[Luben Tuikov <luben.tuikov@amd.com>]

Looks good to me.

Reviewed-by: Luben Tuikov <luben.tuikov@amd.com>

Regards,
Luben

On 2023-01-06 16:51, 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.  A new t9001
> test was added to test this 2nd arg and docs are also updated.
> 
> As an example, this can be useful for acting upon keywords in the
> subject or specific email addresses.
> 
> Cc: Luben Tuikov <luben.tuikov@amd.com>
> Cc: brian m. carlson <sandals@crustytoothpaste.net>
> Signed-off-by: Michael Strawbridge <michael.strawbridge@amd.com>
> ---
>  Documentation/githooks.txt |  8 +++---
>  git-send-email.perl        | 55 +++++++++++++++++++++++++-------------
>  t/t9001-send-email.sh      | 25 +++++++++++++++++
>  3 files changed, 65 insertions(+), 23 deletions(-)
> 
> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
> index a16e62bc8c..346e536cbe 100644
> --- a/Documentation/githooks.txt
> +++ b/Documentation/githooks.txt
> @@ -583,10 +583,10 @@ 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 two parameters,
> +the name of a file that holds the patch and the name of a file that holds the
> +SMTP headers.  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 5861e99a6e..5a626a4238 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);
> @@ -1495,16 +1487,7 @@ sub file_name_is_absolute {
>  	return File::Spec::Functions::file_name_is_absolute($path);
>  }
>  
> -# Prepares the email, then asks the user what to do.
> -#
> -# If the user chooses to send the email, it's sent and 1 is returned.
> -# If the user chooses not to send the email, 0 is returned.
> -# If the user decides they want to make further edits, -1 is returned and the
> -# caller is expected to call send_message again after the edits are performed.
> -#
> -# If an error occurs sending the email, this just dies.
> -
> -sub send_message {
> +sub gen_header {
>  	my @recipients = unique_email_list(@to);
>  	@cc = (grep { my $cc = extract_valid_address_or_die($_);
>  		      not grep { $cc eq $_ || $_ =~ /<\Q${cc}\E>$/ } @recipients
> @@ -1546,6 +1529,22 @@ sub send_message {
>  	if (@xh) {
>  		$header .= join("\n", @xh) . "\n";
>  	}
> +	my $recipients_ref = \@recipients;
> +	return ($recipients_ref, $to, $date, $gitversion, $cc, $ccline, $header);
> +}
> +
> +# Prepares the email, then asks the user what to do.
> +#
> +# If the user chooses to send the email, it's sent and 1 is returned.
> +# If the user chooses not to send the email, 0 is returned.
> +# If the user decides they want to make further edits, -1 is returned and the
> +# caller is expected to call send_message again after the edits are performed.
> +#
> +# If an error occurs sending the email, this just dies.
> +
> +sub send_message {
> +	my ($recipients_ref, $to, $date, $gitversion, $cc, $ccline, $header) = gen_header();
> +	my @recipients = @$recipients_ref;
>  
>  	my @sendmail_parameters = ('-i', @recipients);
>  	my $raw_from = $sender;
> @@ -1955,6 +1954,15 @@ sub process_file {
>  		}
>  	}
>  
> +
> +	if ($validate) {
> +		foreach my $f (@files) {
> +			unless (-p $f) {
> +				validate_patch($f, $target_xfer_encoding);
> +			}
> +		}
> +	}
> +
>  	my $message_was_sent = send_message();
>  	if ($message_was_sent == -1) {
>  		do_edit($t);
> @@ -2088,11 +2096,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_run = (@cmd, $target, $header_filename);
>  			$hook_error = system_or_msg(\@cmd_run, undef, "@cmd_msg");
> +			unlink($header_filehandle);
>  			chdir($cwd_save) or die("chdir: $!");
>  		}
>  		if ($hook_error) {
> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
> index 1130ef21b3..11e68f9c18 100755
> --- a/t/t9001-send-email.sh
> +++ b/t/t9001-send-email.sh
> @@ -565,6 +565,31 @@ test_expect_success $PREREQ "--validate respects absolute core.hooksPath path" '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success $PREREQ "--validate hook supports header argument" '
> +	test_when_finished "rm my-hooks.ran" &&
> +	write_script my-hooks/sendemail-validate <<-\EOF &&
> +	filesize=$(stat -c%s "$2")
> +	if [ "$filesize" != "0" ]; then
> +	>my-hooks.ran
> +	fi
> +	exit 1
> +	EOF
> +	test_config core.hooksPath "my-hooks" &&
> +	test_must_fail git send-email \
> +		--from="Example <nobody@example.com>" \
> +		--to=nobody@example.com \
> +		--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>'"'"' died with exit code 1
> +	warning: no patches were sent
> +	EOF
> +	test_cmp expect actual
> +'
> +
>  for enc in 7bit 8bit quoted-printable base64
>  do
>  	test_expect_success $PREREQ "--transfer-encoding=$enc produces correct header" '