git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH v2] git-send-email: Respect core.hooksPath setting
@ 2021-03-23 16:27 Robert Foss
  2021-03-23 17:33 ` [PATCH v3] " Ævar Arnfjörð Bjarmason
  0 siblings, 1 reply; 5+ messages in thread
From: Robert Foss @ 2021-03-23 16:27 UTC (permalink / raw)
  To: Junio C Hamano, git, Ævar Arnfjörð Bjarmason
  Cc: Drew DeVault, Rafael Aquini, Marcelo Arenas Belón, 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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH v3] git-send-email: Respect core.hooksPath setting
  2021-03-23 16:27 [PATCH v2] git-send-email: Respect core.hooksPath setting Robert Foss
@ 2021-03-23 17:33 ` Ævar Arnfjörð Bjarmason
  2021-03-23 22:40   ` Junio C Hamano
  0 siblings, 1 reply; 5+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-03-23 17:33 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Robert Foss, Drew DeVault, Rafael Aquini,
	Marcelo Arenas Belón,
	Æ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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v3] git-send-email: Respect core.hooksPath setting
  2021-03-23 17:33 ` [PATCH v3] " Ævar Arnfjörð Bjarmason
@ 2021-03-23 22:40   ` Junio C Hamano
  2021-03-24  0:44     ` Ævar Arnfjörð Bjarmason
  0 siblings, 1 reply; 5+ messages in thread
From: Junio C Hamano @ 2021-03-23 22:40 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason, Emily Shaffer
  Cc: git, Robert Foss, Drew DeVault, Rafael Aquini, Marcelo Arenas Belón

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v3] git-send-email: Respect core.hooksPath setting
  2021-03-23 22:40   ` Junio C Hamano
@ 2021-03-24  0:44     ` Ævar Arnfjörð Bjarmason
  2021-03-24  1:17       ` Junio C Hamano
  0 siblings, 1 reply; 5+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-03-24  0:44 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Emily Shaffer, git, Robert Foss, Drew DeVault, Rafael Aquini,
	Marcelo Arenas Belón


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/

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v3] git-send-email: Respect core.hooksPath setting
  2021-03-24  0:44     ` Ævar Arnfjörð Bjarmason
@ 2021-03-24  1:17       ` Junio C Hamano
  0 siblings, 0 replies; 5+ messages in thread
From: Junio C Hamano @ 2021-03-24  1:17 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: Emily Shaffer, git, Robert Foss, Drew DeVault, Rafael Aquini,
	Marcelo Arenas Belón

Æ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/

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-03-24  1:18 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-23 16:27 [PATCH v2] git-send-email: Respect core.hooksPath setting Robert Foss
2021-03-23 17:33 ` [PATCH v3] " Ævar Arnfjörð Bjarmason
2021-03-23 22:40   ` Junio C Hamano
2021-03-24  0:44     ` Ævar Arnfjörð Bjarmason
2021-03-24  1:17       ` Junio C Hamano

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).