Thread (581 messages) flat view 581 messages, 16 authors, 2021-10-12

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

From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2021-05-27 12:02:00

On Wed, May 26 2021, Emily Shaffer wrote:
quoted hunk ↗ jump to hunk
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   | 26 ++++++--------------------
 t/t9001-send-email.sh | 13 +------------
 2 files changed, 7 insertions(+), 32 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index 170f42fe21..b55687453e 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1958,26 +1958,12 @@ sub unique_email_list {
 sub validate_patch {
 	my ($fn, $xfer_encoding) = @_;
 
-	if ($repo) {
-		my $validate_hook = catfile($repo->hooks_path(),
-					    '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 = system_or_msg([$validate_hook, $target]);
-			chdir($cwd_save) or die("chdir: $!");
-		}
-		if ($hook_error) {
-			die sprintf(__("fatal: %s: rejected by sendemail-validate hook\n" .
-				       "%s\n" .
-				       "warning: no patches were sent\n"), $fn, $hook_error);
-		}
-	}
+	my $target = abs_path($fn);
+
+	system_or_die(["git", "hook", "run", "sendemail-validate", "-j1", "-a", $target],
How do we get the "benefit[ ... of ...] parallelization" here if -j1 is
hardcoded as parameter. Shouldn't that be removed so it'll use the
hook.jobs config like everything else?
quoted hunk ↗ jump to hunk
+		sprintf(__("fatal: %s: rejected by sendemail-validate hook\n" .
+			   "warning: no patches were sent\n"),
+		        $fn));
 
 	# 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 aa603cf4d0..bdf6472871 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -539,7 +539,6 @@ test_expect_success $PREREQ "--validate respects relative core.hooksPath path" '
 	test_path_is_file my-hooks.ran &&
 	cat >expect <<-EOF &&
 	fatal: longline.patch: rejected by sendemail-validate hook
-	fatal: command '"'"'$PWD/my-hooks/sendemail-validate'"'"' died with exit code 1
 	warning: no patches were sent
 	EOF
 	test_cmp expect actual
@@ -557,7 +556,6 @@ test_expect_success $PREREQ "--validate respects absolute core.hooksPath path" '
 	test_path_is_file my-hooks.ran &&
 	cat >expect <<-EOF &&
 	fatal: longline.patch: rejected by sendemail-validate hook
-	fatal: command '"'"'$PWD/my-hooks/sendemail-validate'"'"' died with exit code 1
 	warning: no patches were sent
 	EOF
 	test_cmp expect actual
Wouldn't it be better to keep the "died with exit code N" here. I.e. something like:

	fatal: longline.patch: rejected by sendemail-validate hook, 'git hook run' died with exit code %d

I.e. isn't that exit code carried forward by "git hook run"?
quoted hunk ↗ jump to hunk
@@ -2170,16 +2168,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
 	test -f 0001-add-main.patch &&
 	grep "add main" "$1"
 	EOF
We don't want to or need to check $GIT_DIR at all? Maybe not, but the
commit message doesn't say anything about it...
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help