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

On Fri, Mar 12, 2021 at 10:21:08AM +0100, Ævar Arnfjörð Bjarmason wrote:

On Thu, Mar 11 2021, Emily Shaffer wrote:

quoted

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

Oh cool, thanks. I'll do that.

quoted

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

I thought so too :D :D

 - Emily