Re: [PATCH v8 35/37] git-send-email: use 'git hook run' for 'sendemail-validate'

> 
> 
> On Thu, Mar 11 2021, Emily Shaffer wrote:
> 
> > By using the new 'git hook run' subcommand to run 'sendemail-validate',
> > we can reduce the boilerplate needed to run this hook in perl. Using
> > config-based hooks also allows us to run 'sendemail-validate' hooks that
> > were configured globally when running 'git send-email' from outside of a
> > Git directory, alongside other benefits like multihooks and
> > parallelization.
> >
> > Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> > ---
> >  git-send-email.perl   | 21 ++++-----------------
> >  t/t9001-send-email.sh | 11 +----------
> >  2 files changed, 5 insertions(+), 27 deletions(-)
> >
> > diff --git a/git-send-email.perl b/git-send-email.perl
> > index 1f425c0809..73e1e0b51a 100755
> > --- a/git-send-email.perl
> > +++ b/git-send-email.perl
> > @@ -1941,23 +1941,10 @@ sub unique_email_list {
> >  sub validate_patch {
> >  	my ($fn, $xfer_encoding) = @_;
> >  
> > -	if ($repo) {
> > -		my $validate_hook = catfile(catdir($repo->repo_path(), 'hooks'),
> > -					    'sendemail-validate');
> > -		my $hook_error;
> > -		if (-x $validate_hook) {
> > -			my $target = abs_path($fn);
> > -			# The hook needs a correct cwd and GIT_DIR.
> > -			my $cwd_save = cwd();
> > -			chdir($repo->wc_path() or $repo->repo_path())
> > -				or die("chdir: $!");
> > -			local $ENV{"GIT_DIR"} = $repo->repo_path();
> > -			$hook_error = "rejected by sendemail-validate hook"
> > -				if system($validate_hook, $target);
> > -			chdir($cwd_save) or die("chdir: $!");
> > -		}
> > -		return $hook_error if $hook_error;
> > -	}
> > +	my $target = abs_path($fn);
> > +	return "rejected by sendemail-validate hook"
> > +		if system(("git", "hook", "run", "sendemail-validate", "-a",
> > +				$target));
> 
> I see it's just moving code around, but since we're touching this:
> 
> This conflates the hook exit code with a general failure to invoke it,
> Perl's system().
> 
> Not a big deal in this case, but there's two other existing system()
> invocations which use the right blurb for it:
> 
> 
> 	system('sh', '-c', $editor.' "$@"', $editor, $_);
> 	if (($? & 127) || ($? >> 8)) {
> 		die(__("the editor exited uncleanly, aborting everything"));
> 	}
> 
> Makes sense to do something similar here for consistency. See "perldoc
> -f system" for an example.

> 
> >  
> >  	# Any long lines will be automatically fixed if we use a suitable transfer
> >  	# encoding.
> > diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
> > index 4eee9c3dcb..456b471c5c 100755
> > --- a/t/t9001-send-email.sh
> > +++ b/t/t9001-send-email.sh
> > @@ -2101,16 +2101,7 @@ test_expect_success $PREREQ 'invoke hook' '
> >  	mkdir -p .git/hooks &&
> >  
> >  	write_script .git/hooks/sendemail-validate <<-\EOF &&
> > -	# test that we have the correct environment variable, pwd, and
> > -	# argument
> > -	case "$GIT_DIR" in
> > -	*.git)
> > -		true
> > -		;;
> > -	*)
> > -		false
> > -		;;
> > -	esac &&
> > +	# test that we have the correct argument
> 
> This and getting rid of these Perl/Python/whatever special cases is very
> nice.