Re: [PATCH RESEND] hooks: add sendemail-validate-series
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Apr 3, 2023, 14:09 UTC
- 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:
Show 13 quoted lines
> 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> > ---
Show 11 quoted lines
> +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.
Show 17 quoted lines
> +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?
Show 16 quoted lines
> }
>
> 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).
Show 12 quoted lines
> + 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
Show 21 quoted lines
> + 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);