Re: [PATCH 2/2] send-email: don't needlessly abs_path() the core.hooksPath

Ævar Arnfjörð Bjarmason [off-list ref] writes:

But I'll leave it to you, if you are convinced and do want to take this
2/2 after all I'll submit another trivial patch on top to remove the new
(then unused) repo_path function, which we expect to go away anyway.

Sounds good.

In c8243933c74 (git-send-email: Respect core.hooksPath setting,
2021-03-23) we started supporting core.hooksPath in "send-email". It's
been reported that on Windows[1] doing this by calling abs_path()
results in different canonicalizations of the absolute path.

This wasn't an issue in c8243933c74 itself, but was revealed by my
ea7811b37e0 (git-send-email: improve --validate error output,
2021-04-06) when we started emitting the path to the hook, which was
previously only internal to git-send-email.perl.

The just-landed 53753a37d09 (t9001-send-email.sh: fix expected
absolute paths on Windows, 2021-05-24) narrowly fixed this issue, but
I believe we can do better here. We should not be relying on whatever
changes Perl's abs_path() makes to the path "rev-parse --git-path
hooks" hands to us. Let's instead trust it, and hand it to Perl's
system() in git-send-email.perl. It will handle either a relative or
absolute path.

So let's revert most of 53753a37d09 and just have "hooks_path" return
what we get from "rev-parse" directly without modification. This has
the added benefit of making the error message friendlier in the common
case, we'll no longer print an absolute path for repository-local hook
errors.

1. http://lore.kernel.org/git/bb30fe2b-cd75-4782-24a6-08bb002a0367@kdbg.org

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 perl/Git.pm           | 3 +--
 t/t9001-send-email.sh | 7 ++++---
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index 73ebbf80cc6..df6280ebab5 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm

@@ -629,8 +629,7 @@ sub hooks_path {
 	my ($self) = @_;
 
 	my $dir = $self->command_oneline('rev-parse', '--git-path', 'hooks');
-	my $abs = abs_path($dir);
-	return $abs;
+	return $dir;
 }
 
 =item wc_path ()

diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index aa603cf4d07..3b7540050ca 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh

@@ -539,14 +539,15 @@ 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
+	fatal: command '"'"'my-hooks/sendemail-validate'"'"' died with exit code 1
 	warning: no patches were sent
 	EOF
 	test_cmp expect actual
 '
 
 test_expect_success $PREREQ "--validate respects absolute core.hooksPath path" '
-	test_config core.hooksPath "$(pwd)/my-hooks" &&
+	hooks_path="$(pwd)/my-hooks" &&
+	test_config core.hooksPath "$hooks_path" &&
 	test_when_finished "rm my-hooks.ran" &&
 	test_must_fail git send-email \
 		--from="Example <nobody@example.com>" \

@@ -557,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 '"'"'$PWD/my-hooks/sendemail-validate'"'"' died with exit code 1
+	fatal: command '"'"'$hooks_path/sendemail-validate'"'"' died with exit code 1
 	warning: no patches were sent
 	EOF
 	test_cmp expect actual

-- 
2.32.0.rc1.400.g0a5a93401d3

On Wed, May 26 2021, Junio C Hamano wrote:

Ævar Arnfjörð Bjarmason [off-list ref] writes:

quoted

But I'll leave it to you, if you are convinced and do want to take this
2/2 after all I'll submit another trivial patch on top to remove the new
(then unused) repo_path function, which we expect to go away anyway.

Sounds good.

Here that is, now on top of the new "master" which has the
t9001-send-email.sh changes you'd already merged down.

Ævar Arnfjörð Bjarmason (2):
  send-email: don't needlessly abs_path() the core.hooksPath
  send-email: move "hooks_path" invocation to git-send-email.perl

 git-send-email.perl   |  3 ++-
 perl/Git.pm           | 13 -------------
 t/t9001-send-email.sh |  7 ++++---
 3 files changed, 6 insertions(+), 17 deletions(-)

Range-diff against v1:
1:  df3a2b8562d < -:  ----------- send-email: fix missing error message regression
2:  d097e7b0b81 ! 1:  ff01d4619ea send-email: don't needlessly abs_path() the core.hooksPath
    @@ Commit message
         2021-04-06) when we started emitting the path to the hook, which was
         previously only internal to git-send-email.perl.
     
    -    I think this change should let us have our cake and eat it too. We now
    -    emit a relative path for the common case where the hook is in the
    -    .git/hooks directory, but in the case it's an absolute path (there's
    -    another test for that, not seen here) we'll prefix it with $(pwd).
    +    The just-landed 53753a37d09 (t9001-send-email.sh: fix expected
    +    absolute paths on Windows, 2021-05-24) narrowly fixed this issue, but
    +    I believe we can do better here. We should not be relying on whatever
    +    changes Perl's abs_path() makes to the path "rev-parse --git-path
    +    hooks" hands to us. Let's instead trust it, and hand it to Perl's
    +    system() in git-send-email.perl. It will handle either a relative or
    +    absolute path.
     
    -    I hope that unlike the current implementation that $(pwd) v.s. $PWD
    -    difference won't matter on Windows, since now the absolute path is the
    -    one we get from rev-parse, not the one that's been passed through
    -    Perl's Cwd::abs_path().
    +    So let's revert most of 53753a37d09 and just have "hooks_path" return
    +    what we get from "rev-parse" directly without modification. This has
    +    the added benefit of making the error message friendlier in the common
    +    case, we'll no longer print an absolute path for repository-local hook
    +    errors.
     
         1. http://lore.kernel.org/git/bb30fe2b-cd75-4782-24a6-08bb002a0367@kdbg.org
     
    @@ t/t9001-send-email.sh: test_expect_success $PREREQ "--validate respects relative
      	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
    +-	fatal: command '"'"'$PWD/my-hooks/sendemail-validate'"'"' died with exit code 1
     +	fatal: command '"'"'my-hooks/sendemail-validate'"'"' died with exit code 1
      	warning: no patches were sent
      	EOF
      	test_cmp expect actual
    + '
    + 
    + test_expect_success $PREREQ "--validate respects absolute core.hooksPath path" '
    +-	test_config core.hooksPath "$(pwd)/my-hooks" &&
    ++	hooks_path="$(pwd)/my-hooks" &&
    ++	test_config core.hooksPath "$hooks_path" &&
    + 	test_when_finished "rm my-hooks.ran" &&
    + 	test_must_fail git send-email \
    + 		--from="Example [off-list ref]" \
    +@@ t/t9001-send-email.sh: 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
    ++	fatal: command '"'"'$hooks_path/sendemail-validate'"'"' died with exit code 1
    + 	warning: no patches were sent
    + 	EOF
    + 	test_cmp expect actual
-:  ----------- > 2:  cda41c36772 send-email: move "hooks_path" invocation to git-send-email.perl
-- 
2.32.0.rc1.400.g0a5a93401d3

Move the newly added "hooks_path" API in Git.pm to its only user in
git-send-email.perl. This was added in c8243933c74 (git-send-email:
Respect core.hooksPath setting, 2021-03-23), meaning that it hasn't
yet made it into a non-rc release of git.

The consensus with Git.pm is that we need to be considerate of
out-of-tree users who treat it as a public documented interface. We
should therefore be less willing to add new functionality to it, least
we be stuck supporting it after our own uses for it disappear.

In this case the git-send-email.perl hook invocation will probably be
replaced by a future "git hook run" command, and in the commit
preceding this one the "hooks_path" become nothing but a trivial
wrapper for "rev-parse --git-path hooks" anyway (with no
Cwd::abs_path() call), so let's just inline this command in
git-send-email.perl itself.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 git-send-email.perl |  3 ++-
 perl/Git.pm         | 12 ------------
 2 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 170f42fe210..25be2ebd2af 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl

@@ -1959,7 +1959,8 @@ sub validate_patch {
 	my ($fn, $xfer_encoding) = @_;
 
 	if ($repo) {
-		my $validate_hook = catfile($repo->hooks_path(),
+		my $hooks_path = $repo->command_oneline('rev-parse', '--git-path', 'hooks');
+		my $validate_hook = catfile($hooks_path,
 					    'sendemail-validate');
 		my $hook_error;
 		if (-x $validate_hook) {

diff --git a/perl/Git.pm b/perl/Git.pm
index df6280ebab5..02eacef0c2a 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm

@@ -619,18 +619,6 @@ 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');
-	return $dir;
-}
 
 =item wc_path ()
 

-- 
2.32.0.rc1.400.g0a5a93401d3