From: Phillip Wood <phillip.wood123@gmail.com>
Date: Mon, 03 Apr 2023 14:09:32 GMT
Subject: Re: [PATCH RESEND] hooks: add sendemail-validate-series
Message-ID: <66099367-4ea0-7d2a-a089-7a88e27f695e@dunelm.org.uk>
In-Reply-To: <20230402185635.302653-1-robin@jarry.cc>

Hi Robin

On 02/04/2023 19:56, Robin Jarry wrote:
> When sending patch series (with a cover-letter or not)
> sendemail-validate is called with every email/patch file independently
> from the others. When one of the patches depends on a previous one, it
> may not be possible to use this hook in a meaningful way.
> 
> Changing sendemail-validate to take all patches as arguments would break
> backward compatibility.
> 
> Add a new hook to allow validating patch series instead of patch by
> patch. The patch files are provided in the hook script standard input.
> 
> git hook run cannot be used since it closes the hook standard input. Run
> the hook directly.

I've left some comments about this lower down as "git hook run" now has 
a --to-stdin option.

> Signed-off-by: Robin Jarry <robin@jarry.cc>
> ---

> +sendemail-validate-series
> +~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +This hook is invoked by linkgit:git-send-email[1].  It allows performing
> +validation on a complete patch series at once, instead of patch by patch with
> +`sendemail-validate`.
> +
> +`sendemail-validate-series` takes no arguments, but for each e-mail to be sent
> +it receives on standard input a line of the format:
> +
> +  <patch-file> LF

Usually git commands that produce or consume paths either use quoted 
paths terminated by LF or unquoted paths terminated by NUL. That way 
there is no ambiguity when a path contains LF.

> +where `<patch-file>` is a name of a file that holds an e-mail to be sent,
> +
> +If the hook exits with non-zero status, `git send-email` will abort before
> +sending any e-mails.
> +
>   fsmonitor-watchman
>   ~~~~~~~~~~~~~~~~~~
>   
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 07f2a0cbeaad..bec4d0f4ab47 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -800,6 +800,7 @@ sub is_format_patch_arg {
>   			validate_patch($f, $target_xfer_encoding);
>   		}
>   	}
> +	validate_patch_series(@files)

This happens fairly early, before the user has had a chance to edit the 
patches and before we have added all the recipient and in-reply-to 
headers to the patch files. Would it be more useful to validate what 
will actually be sent?

>   }
>   
>   if (@files) {
> @@ -2125,6 +2126,47 @@ sub validate_patch {
>   	return;
>   }
>   
> +sub validate_patch_series {
> +	my @files = @_;
> +
> +	unless ($repo) {
> +		return;
> +	}
> +
> +	my $hook_name = 'sendemail-validate-series';
> +	my $hooks_path = $repo->command_oneline('rev-parse', '--git-path', 'hooks');

$hooks_path maybe a relative path, this is problematic because we change 
directory before executing the hook (using "git hook run" would avoid this).

> +	require File::Spec;
> +	my $validate_hook = File::Spec->catfile($hooks_path, $hook_name);
> +	my $hook_error;
> +	unless (-x $validate_hook) {
> +		return;
> +	}
> +
> +	# The hook needs a correct cwd and GIT_DIR.
> +	require Cwd;
> +	my $cwd_save = Cwd::getcwd();
> +	chdir($repo->wc_path() or $repo->repo_path()) or die("chdir: $!");
> +	local $ENV{"GIT_DIR"} = $repo->repo_path();

This looks like it is copied from the existing code but why do we need 
to do this? I'm struggling to come up with a scenario where "git 
send-email" can find the repository but the hook cannot.

> +	# cannot use git hook run, it closes stdin before forking the hook
> +	open(my $stdin, "|-", $validate_hook) or die("fork: $!");

This passes $validate_hook to the shell to execute which is not what we 
want as it will split the hook path on whitespace etc. I think it would 
be better to use "git hook run --to-stdin" (see 0414b3891c (hook: 
support a --to-stdin=<path> option, 2023-02-08))

Best Wishes

Phillip

> +	chdir($cwd_save) or die("chdir: $!");
> +	for my $fn (@files) {
> +		unless (-p $fn) {
> +			$fn = Cwd::abs_path($fn);
> +			$stdin->print("$fn\n");
> +		}
> +	}
> +	close($stdin); # calls waitpid
> +	if ($? & 0x7f) {
> +		my $sig = $? & 0x7f;
> +		die("fatal: hook $hook_name killed by signal $sig");
> +	} elsif ($? >> 8) {
> +		my $err = $? >> 8;
> +		die("fatal: hook $hook_name rejected patch series (exit code $err)");
> +	}
> +	return;
> +}
> +
>   sub handle_backup {
>   	my ($last, $lastlen, $file, $known_suffix) = @_;
>   	my ($suffix, $skip);