[PATCH v2] git-send-email: Respect core.hooksPath setting

[PATCH v2] git-send-email: Respect core.hooksPath setting

[Robert Foss]

get-send-email currently makes the assumption that the
'sendemail-validate' hook exists inside of the repository.

Since the introduction of 'core.hooksPath' configuration option in
v2.9, this is no longer true.

Instead of assuming a hardcoded repo relative path, query
git for the actual path of the hooks directory.

Signed-off-by: Robert Foss <robert.foss@linaro.org>
---

Changes since v1:
 - Ævar: Add unit test
 - Ævar: Add support for getting the hooks_path() from Git perl module
 - Ævar: Use new hooks_path() instread of issuing git rev-parse in
         git-send-email.perl


Note: The test currently leaves a file in a mktemp directory
in /tmp.

 git-send-email.perl   |  4 ++--
 perl/Git.pm           |  9 +++++++++
 t/t9001-send-email.sh | 16 ++++++++++++++++
 3 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 1f425c0809..c3dd825322 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1942,8 +1942,8 @@ sub validate_patch {
 	my ($fn, $xfer_encoding) = @_;
 
 	if ($repo) {
-		my $validate_hook = catfile(catdir($repo->repo_path(), 'hooks'),
-					    'sendemail-validate');
+		my $hook_path = $repo->hooks_path();
+		my $validate_hook = catfile($hook_path, 'sendemail-validate');
 		my $hook_error;
 		if (-x $validate_hook) {
 			my $target = abs_path($fn);
diff --git a/perl/Git.pm b/perl/Git.pm
index 02eacef0c2..ac1fabff28 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -226,6 +226,8 @@ sub repository {
 			$opts{Repository} = abs_path($dir);
 		}
 
+                $opts{HooksPath} = $search->command_oneline('rev-parse', '--git-path', 'hooks');
+
 		delete $opts{Directory};
 	}
 
@@ -619,6 +621,13 @@ sub _prompt {
 
 sub repo_path { $_[0]->{opts}->{Repository} }
 
+=item hooks_path ()
+
+Return path to the hooks directory. Must be called on a repository instance.
+
+=cut
+
+sub hooks_path { $_[0]->{opts}->{HooksPath} }
 
 =item wc_path ()
 
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 4eee9c3dcb..73b3bc1ce6 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -513,6 +513,22 @@ do
 
 done
 
+test_expect_success $PREREQ "--validate respects core.hooksPath setting" '
+	clean_fake_sendmail &&
+	tmp_dir=$(mktemp -d -t ci-XXXXXXXXXX) &&
+	printf "#!/bin/sh" >> $tmp_dir/sendemail-validate &&
+	printf "return 1" >> $tmp_dir/sendemail-validate &&
+	chmod a+x $tmp_dir/sendemail-validate &&
+	git -c core.hooksPath=$tmp_dir send-email \
+		--from="Example <nobody@example.com>" \
+		--to=nobody@example.com \
+		--smtp-server="$(pwd)/fake.sendmail" \
+		--validate \
+		longline.patch \
+		2>&1 >/dev/null | \
+	grep "rejected by sendemail-validate"
+'
+
 for enc in 7bit 8bit quoted-printable base64
 do
 	test_expect_success $PREREQ "--transfer-encoding=$enc produces correct header" '
-- 
2.31.0.30.g398dba342d.dirty

[PATCH v3] git-send-email: Respect core.hooksPath setting

[Ævar Arnfjörð Bjarmason]

From: Robert Foss <robert.foss@linaro.org>

get-send-email currently makes the assumption that the
'sendemail-validate' hook exists inside of the repository.

Since the introduction of 'core.hooksPath' configuration option in
867ad08a261 (hooks: allow customizing where the hook directory is,
2016-05-04), this is no longer true.

Instead of assuming a hardcoded repo relative path, query
git for the actual path of the hooks directory.

Signed-off-by: Robert Foss <robert.foss@linaro.org>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

Here's a v3 that fixes various issues with Robert's v2. Range-diff &
updated patch below.

The advice I had in the v1 feedback about GetHooksPath was bad, just
having it be a new accessor is better. It's not like anyone is calling
this in a loop.

Range-diff:
1:  56c181cf091 ! 1:  cc0ba73974a git-send-email: Respect core.hooksPath setting
    @@ Commit message
         'sendemail-validate' hook exists inside of the repository.
     
         Since the introduction of 'core.hooksPath' configuration option in
    -    v2.9, this is no longer true.
    +    867ad08a261 (hooks: allow customizing where the hook directory is,
    +    2016-05-04), this is no longer true.
     
         Instead of assuming a hardcoded repo relative path, query
         git for the actual path of the hooks directory.
     
         Signed-off-by: Robert Foss <robert.foss@linaro.org>
    +    Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## git-send-email.perl ##
     @@ git-send-email.perl: sub validate_patch {
    @@ git-send-email.perl: sub validate_patch {
      
      	if ($repo) {
     -		my $validate_hook = catfile(catdir($repo->repo_path(), 'hooks'),
    --					    'sendemail-validate');
    -+		my $hook_path = $repo->hooks_path();
    -+		my $validate_hook = catfile($hook_path, 'sendemail-validate');
    ++		my $validate_hook = catfile($repo->hooks_path(),
    + 					    'sendemail-validate');
      		my $hook_error;
      		if (-x $validate_hook) {
    - 			my $target = abs_path($fn);
     
      ## perl/Git.pm ##
    -@@ perl/Git.pm: sub repository {
    - 			$opts{Repository} = abs_path($dir);
    - 		}
    - 
    -+                $opts{HooksPath} = $search->command_oneline('rev-parse', '--git-path', 'hooks');
    -+
    - 		delete $opts{Directory};
    - 	}
    - 
     @@ perl/Git.pm: sub _prompt {
      
      sub repo_path { $_[0]->{opts}->{Repository} }
    @@ perl/Git.pm: sub _prompt {
     +
     +=cut
     +
    -+sub hooks_path { $_[0]->{opts}->{HooksPath} }
    ++sub hooks_path {
    ++	my ($self) = @_;
    ++
    ++	my $dir = $self->command_oneline('rev-parse', '--git-path', 'hooks');
    ++	my $abs = abs_path($dir);
    ++	return $abs;
    ++}
      
      =item wc_path ()
      
    @@ t/t9001-send-email.sh: do
      
      done
      
    -+test_expect_success $PREREQ "--validate respects core.hooksPath path" '
    ++test_expect_success $PREREQ "--validate respects relative core.hooksPath path" '
     +	clean_fake_sendmail &&
    -+	tmp_dir=$(mktemp -d -t ci-XXXXXXXXXX) &&
    -+	printf "#!/bin/sh" >> $tmp_dir/sendemail-validate &&
    -+	printf "return 1" >> $tmp_dir/sendemail-validate &&
    -+	chmod a+x $tmp_dir/sendemail-validate &&
    -+	git -c core.hooksPath=$tmp_dir send-email \
    ++	mkdir my-hooks &&
    ++	test_when_finished "rm my-hooks.ran" &&
    ++	write_script my-hooks/sendemail-validate <<-\EOF &&
    ++	>my-hooks.ran
    ++	exit 1
    ++	EOF
    ++	test_config core.hooksPath "my-hooks" &&
    ++	test_must_fail git send-email \
    ++		--from="Example <nobody@example.com>" \
    ++		--to=nobody@example.com \
    ++		--smtp-server="$(pwd)/fake.sendmail" \
    ++		--validate \
    ++		longline.patch 2>err &&
    ++	test_path_is_file my-hooks.ran &&
    ++	grep "rejected by sendemail-validate" err
    ++'
    ++
    ++test_expect_success $PREREQ "--validate respects absolute core.hooksPath path" '
    ++	test_config core.hooksPath "$(pwd)/my-hooks" &&
    ++	test_when_finished "rm my-hooks.ran" &&
    ++	test_must_fail git send-email \
     +		--from="Example <nobody@example.com>" \
     +		--to=nobody@example.com \
     +		--smtp-server="$(pwd)/fake.sendmail" \
     +		--validate \
    -+		longline.patch \
    -+		2>&1 >/dev/null | \
    -+	grep "rejected by sendemail-validate"
    ++		longline.patch 2>err &&
    ++	test_path_is_file my-hooks.ran &&
    ++	grep "rejected by sendemail-validate" err
     +'
     +
      for enc in 7bit 8bit quoted-printable base64

 git-send-email.perl   |  2 +-
 perl/Git.pm           | 13 +++++++++++++
 t/t9001-send-email.sh | 32 ++++++++++++++++++++++++++++++++
 3 files changed, 46 insertions(+), 1 deletion(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 1f425c08091..f5bbf1647e3 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1942,7 +1942,7 @@ sub validate_patch {
 	my ($fn, $xfer_encoding) = @_;
 
 	if ($repo) {
-		my $validate_hook = catfile(catdir($repo->repo_path(), 'hooks'),
+		my $validate_hook = catfile($repo->hooks_path(),
 					    'sendemail-validate');
 		my $hook_error;
 		if (-x $validate_hook) {
diff --git a/perl/Git.pm b/perl/Git.pm
index 02eacef0c2a..73ebbf80cc6 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -619,6 +619,19 @@ sub _prompt {
 
 sub repo_path { $_[0]->{opts}->{Repository} }
 
+=item hooks_path ()
+
+Return path to the hooks directory. Must be called on a repository instance.
+
+=cut
+
+sub hooks_path {
+	my ($self) = @_;
+
+	my $dir = $self->command_oneline('rev-parse', '--git-path', 'hooks');
+	my $abs = abs_path($dir);
+	return $abs;
+}
 
 =item wc_path ()
 
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 4eee9c3dcb5..1a1caf8f2ed 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -513,6 +513,38 @@ do
 
 done
 
+test_expect_success $PREREQ "--validate respects relative core.hooksPath path" '
+	clean_fake_sendmail &&
+	mkdir my-hooks &&
+	test_when_finished "rm my-hooks.ran" &&
+	write_script my-hooks/sendemail-validate <<-\EOF &&
+	>my-hooks.ran
+	exit 1
+	EOF
+	test_config core.hooksPath "my-hooks" &&
+	test_must_fail git send-email \
+		--from="Example <nobody@example.com>" \
+		--to=nobody@example.com \
+		--smtp-server="$(pwd)/fake.sendmail" \
+		--validate \
+		longline.patch 2>err &&
+	test_path_is_file my-hooks.ran &&
+	grep "rejected by sendemail-validate" err
+'
+
+test_expect_success $PREREQ "--validate respects absolute core.hooksPath path" '
+	test_config core.hooksPath "$(pwd)/my-hooks" &&
+	test_when_finished "rm my-hooks.ran" &&
+	test_must_fail git send-email \
+		--from="Example <nobody@example.com>" \
+		--to=nobody@example.com \
+		--smtp-server="$(pwd)/fake.sendmail" \
+		--validate \
+		longline.patch 2>err &&
+	test_path_is_file my-hooks.ran &&
+	grep "rejected by sendemail-validate" err
+'
+
 for enc in 7bit 8bit quoted-printable base64
 do
 	test_expect_success $PREREQ "--transfer-encoding=$enc produces correct header" '
-- 
2.31.0.366.ga80606b22c1

Re: [PATCH v3] git-send-email: Respect core.hooksPath setting

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> From: Robert Foss <robert.foss@linaro.org>
>
> get-send-email currently makes the assumption that the
> 'sendemail-validate' hook exists inside of the repository.
>
> Since the introduction of 'core.hooksPath' configuration option in
> 867ad08a261 (hooks: allow customizing where the hook directory is,
> 2016-05-04), this is no longer true.
>
> Instead of assuming a hardcoded repo relative path, query
> git for the actual path of the hooks directory.
>
> Signed-off-by: Robert Foss <robert.foss@linaro.org>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>
> Here's a v3 that fixes various issues with Robert's v2. Range-diff &
> updated patch below.
>
> The advice I had in the v1 feedback about GetHooksPath was bad, just
> having it be a new accessor is better. It's not like anyone is calling
> this in a loop.

How urgent is this "fix".  I am wondering if Emily's "git hook"
automatically fix this for us when it comes.

Re: [PATCH v3] git-send-email: Respect core.hooksPath setting

[Ævar Arnfjörð Bjarmason]

On Tue, Mar 23 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> From: Robert Foss <robert.foss@linaro.org>
>>
>> get-send-email currently makes the assumption that the
>> 'sendemail-validate' hook exists inside of the repository.
>>
>> Since the introduction of 'core.hooksPath' configuration option in
>> 867ad08a261 (hooks: allow customizing where the hook directory is,
>> 2016-05-04), this is no longer true.
>>
>> Instead of assuming a hardcoded repo relative path, query
>> git for the actual path of the hooks directory.
>>
>> Signed-off-by: Robert Foss <robert.foss@linaro.org>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>
>> Here's a v3 that fixes various issues with Robert's v2. Range-diff &
>> updated patch below.
>>
>> The advice I had in the v1 feedback about GetHooksPath was bad, just
>> having it be a new accessor is better. It's not like anyone is calling
>> this in a loop.
>
> How urgent is this "fix".  I am wondering if Emily's "git hook"
> automatically fix this for us when it comes.

We've had iterations of that topic for almost a year now (since 2019
counting RFC discussions).

While I'd like to see it land I'm skeptical of parts of that approach[1]
and expect we'll have more re-rolls of it, and in any case the conflict
in send-email[2] will be trivial to resolve. So I think it makes sense
to queue up this narrow fix and not have this wait on the larger topic.

1. https://lore.kernel.org/git/87mtv8fww3.fsf@evledraar.gmail.com/
2. https://lore.kernel.org/git/20210311021037.3001235-36-emilyshaffer@google.com/

Re: [PATCH v3] git-send-email: Respect core.hooksPath setting

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> While I'd like to see it land I'm skeptical of parts of that approach[1]
> and expect we'll have more re-rolls of it, and in any case the conflict
> in send-email[2] will be trivial to resolve.
> So I think it makes sense
> to queue up this narrow fix and not have this wait on the larger topic.

Well, that is what I am doing for now, queue up this narrow change,
while resolving the conflict to use Emily's topic.  Which means that
the new code this topic adds is never exercised while the other topic
is still cooking in tree.

> 1. https://lore.kernel.org/git/87mtv8fww3.fsf@evledraar.gmail.com/
> 2. https://lore.kernel.org/git/20210311021037.3001235-36-emilyshaffer@google.com/