diff mbox series

[v2,08/13] send-email: use 'git hook run' for 'sendemail-validate'

Message ID patch-v2-08.13-6c4ebd68d56-20211015T093918Z-avarab@gmail.com (mailing list archive)
State Superseded
Headers show
Series hook.[ch]: new library to run hooks + simple hook conversion | expand

Commit Message

Ævar Arnfjörð Bjarmason Oct. 15, 2021, 9:43 a.m. UTC 
From: Emily Shaffer <emilyshaffer@google.com>

Change the "sendmail-validate" hook to be run via the "git hook run"
wrapper instead of via a direct invocation.

This is the smallest possibly change to get "send-email" using "git
hook run". We still check the hook itself with "-x", and set a
"GIT_DIR" variable, both of which are asserted by our tests. We'll
need to get rid of this special behavior if we start running N hooks,
but for now let's be as close to bug-for-bug compatible as possible.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 git-send-email.perl   | 22 ++++++++++++++--------
 t/t9001-send-email.sh |  4 ++--
 2 files changed, 16 insertions(+), 10 deletions(-)

Comments

Emily Shaffer Oct. 15, 2021, 5:07 p.m. UTC | #1 
On Fri, Oct 15, 2021 at 11:43:36AM +0200, Ævar Arnfjörð Bjarmason wrote:
> 
> 
> Change the "sendmail-validate" hook to be run via the "git hook run"
> wrapper instead of via a direct invocation.
> 
> This is the smallest possibly change to get "send-email" using "git
> hook run". We still check the hook itself with "-x", and set a
> "GIT_DIR" variable, both of which are asserted by our tests. We'll
> need to get rid of this special behavior if we start running N hooks,
> but for now let's be as close to bug-for-bug compatible as possible.
> 
> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

Someday we can delete a bunch more in there and on that day we can
celebrate. But the rationale for making the change minimal LGTM.

Reviewed-by: Emily Shaffer <emilyshaffer@google.com>

> ---
>  git-send-email.perl   | 22 ++++++++++++++--------
>  t/t9001-send-email.sh |  4 ++--
>  2 files changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 5262d88ee32..4c20c0bbb79 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -195,13 +195,13 @@ sub format_2822_time {
>  my $editor;
>  
>  sub system_or_msg {
> -	my ($args, $msg) = @_;
> +	my ($args, $msg, $cmd_name) = @_;
>  	system(@$args);
>  	my $signalled = $? & 127;
>  	my $exit_code = $? >> 8;
>  	return unless $signalled or $exit_code;
>  
> -	my @sprintf_args = ($args->[0], $exit_code);
> +	my @sprintf_args = ($cmd_name ? $cmd_name : $args->[0], $exit_code);
>  	if (defined $msg) {
>  		# Quiet the 'redundant' warning category, except we
>  		# need to support down to Perl 5.8, so we can't do a
> @@ -2039,10 +2039,10 @@ sub validate_patch {
>  	my ($fn, $xfer_encoding) = @_;
>  
>  	if ($repo) {
> +		my $hook_name = 'sendemail-validate';
>  		my $hooks_path = $repo->command_oneline('rev-parse', '--git-path', 'hooks');
>  		require File::Spec;
> -		my $validate_hook = File::Spec->catfile($hooks_path,
> -					    'sendemail-validate');
> +		my $validate_hook = File::Spec->catfile($hooks_path, $hook_name);
>  		my $hook_error;
>  		if (-x $validate_hook) {
>  			require Cwd;
> @@ -2052,13 +2052,19 @@ sub validate_patch {
>  			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]);
> +			my @cmd = ("git", "hook", "run", "--ignore-missing",
> +				    $hook_name, "--");
> +			my @cmd_msg = (@cmd, "<patch>");
> +			my @cmd_run = (@cmd, $target);
> +			$hook_error = system_or_msg(\@cmd_run, undef, "@cmd_msg");
>  			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);
> +			$hook_error = sprintf(__("fatal: %s: rejected by %s hook\n" .
> +						 $hook_error . "\n" .
> +						 "warning: no patches were sent\n"),
> +					      $fn, $hook_name);
> +			die $hook_error;
>  		}
>  	}
>  
> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
> index aa0c20499ba..84d0f40d76a 100755
> --- a/t/t9001-send-email.sh
> +++ b/t/t9001-send-email.sh
> @@ -539,7 +539,7 @@ 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 '"'"'my-hooks/sendemail-validate'"'"' died with exit code 1
> +	fatal: command '"'"'git hook run --ignore-missing sendemail-validate -- <patch>'"'"' died with exit code 1
>  	warning: no patches were sent
>  	EOF
>  	test_cmp expect actual
> @@ -558,7 +558,7 @@ 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 '"'"'$hooks_path/sendemail-validate'"'"' died with exit code 1
> +	fatal: command '"'"'git hook run --ignore-missing sendemail-validate -- <patch>'"'"' died with exit code 1
>  	warning: no patches were sent
>  	EOF
>  	test_cmp expect actual
> -- 
> 2.33.1.1338.g20da966911a
>
diff mbox series

Patch

diff --git a/git-send-email.perl b/git-send-email.perl
index 5262d88ee32..4c20c0bbb79 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -195,13 +195,13 @@  sub format_2822_time {
 my $editor;
 
 sub system_or_msg {
-	my ($args, $msg) = @_;
+	my ($args, $msg, $cmd_name) = @_;
 	system(@$args);
 	my $signalled = $? & 127;
 	my $exit_code = $? >> 8;
 	return unless $signalled or $exit_code;
 
-	my @sprintf_args = ($args->[0], $exit_code);
+	my @sprintf_args = ($cmd_name ? $cmd_name : $args->[0], $exit_code);
 	if (defined $msg) {
 		# Quiet the 'redundant' warning category, except we
 		# need to support down to Perl 5.8, so we can't do a
@@ -2039,10 +2039,10 @@  sub validate_patch {
 	my ($fn, $xfer_encoding) = @_;
 
 	if ($repo) {
+		my $hook_name = 'sendemail-validate';
 		my $hooks_path = $repo->command_oneline('rev-parse', '--git-path', 'hooks');
 		require File::Spec;
-		my $validate_hook = File::Spec->catfile($hooks_path,
-					    'sendemail-validate');
+		my $validate_hook = File::Spec->catfile($hooks_path, $hook_name);
 		my $hook_error;
 		if (-x $validate_hook) {
 			require Cwd;
@@ -2052,13 +2052,19 @@  sub validate_patch {
 			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]);
+			my @cmd = ("git", "hook", "run", "--ignore-missing",
+				    $hook_name, "--");
+			my @cmd_msg = (@cmd, "<patch>");
+			my @cmd_run = (@cmd, $target);
+			$hook_error = system_or_msg(\@cmd_run, undef, "@cmd_msg");
 			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);
+			$hook_error = sprintf(__("fatal: %s: rejected by %s hook\n" .
+						 $hook_error . "\n" .
+						 "warning: no patches were sent\n"),
+					      $fn, $hook_name);
+			die $hook_error;
 		}
 	}
 
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index aa0c20499ba..84d0f40d76a 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -539,7 +539,7 @@  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 '"'"'my-hooks/sendemail-validate'"'"' died with exit code 1
+	fatal: command '"'"'git hook run --ignore-missing sendemail-validate -- <patch>'"'"' died with exit code 1
 	warning: no patches were sent
 	EOF
 	test_cmp expect actual
@@ -558,7 +558,7 @@  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 '"'"'$hooks_path/sendemail-validate'"'"' died with exit code 1
+	fatal: command '"'"'git hook run --ignore-missing sendemail-validate -- <patch>'"'"' died with exit code 1
 	warning: no patches were sent
 	EOF
 	test_cmp expect actual