threads / patch / 55764

patcht9001-send-email.sh: fix expected absolute paths on Windows

Subject: [PATCH] t9001-send-email.sh: fix expected absolute paths on Windows

## tl;dr

20 messages between May 24, 2021 and Jun 2, 2021. Diffs are folded; open one to read it.

replies: 19people: 7as markdown or json

Johannes Sixt· May 24, 2021, 19:38 UTC · lore

Git for Windows is a native Windows program that works with native absolute paths in the drive letter style C:\dir. The auxiliary infrastructure is based on MSYS2, which uses POSIX style /C/dir.

When we test for output of absolute paths produced by git.exe, we usally have to expect C:\dir style paths. To produce such expected paths, we have to use $(pwd) in the test scripts; the alternative, $PWD, produces a POSIX style path. ($PWD is a shell variable, and the shell is bash, an MSYS2 program, and operates in the POSIX realm.)

There are two recently added tests that were written to expect C:\dir paths. The output that is tested is produced by `git send-email`, but behind the scenes, this is a Perl script, which also works in the POSIX realm and produces /C/dir style output.

In the first test case that is changed here, replace $(pwd) by $PWD so that the expected path is constructed using /C/dir style.

The second test case sets core.hooksPath to an absolute path. Since the test script talks to native git.exe, it is supposed to place a C:/dir style path into the configuration; therefore, keep $(pwd). When this configuration value is consumed by the Perl script, it is transformed to /C/dir style by the MSYS2 layer and echoed back in this form in the error message. Hence, do use $PWD for the expected value.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 When I say "the configuration is transformed to /C/dir style", I am
 actually hand-waving: I can observe that a transformation must
 happen somewhere, but I actually do not know where the conversion
 really happens. "The MSYS2 layer" is my best qualified guess.
 t/t9001-send-email.sh | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)
Show changes to t/t9001-send-email.sh +3 −4
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 65b3035371..68bebc505b 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -539,15 +539,14 @@ 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 '"'"'$PWD/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" '
-	hooks_path="$(pwd)/my-hooks" &&
-	test_config core.hooksPath "$hooks_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>" \
@@ -558,7 +557,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 '"'"'$PWD/my-hooks/sendemail-validate'"'"' died with exit code 1
 	warning: no patches were sent
 	EOF
 	test_cmp expect actual

-- 
2.31.0.152.g120726e270
Jonathan Nieder· May 24, 2021, 20:26 UTC · re: Johannes Sixt · lore

Re: [PATCH] t9001-send-email.sh: fix expected absolute paths on Windows

Hi,
Johannes Sixt wrote:
> Git for Windows is a native Windows program that works with native
> absolute paths in the drive letter style C:\dir. The auxiliary
> infrastructure is based on MSYS2, which uses POSIX style /C/dir.
[nice explanation snipped]
Show 6 quoted lines
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---
>  When I say "the configuration is transformed to /C/dir style", I am
>  actually hand-waving: I can observe that a transformation must
>  happen somewhere, but I actually do not know where the conversion
>  really happens. "The MSYS2 layer" is my best qualified guess.

Thanks. The explanation is appreciated --- it helps avoid the feeling of randomness involved. Hopefully some day our test setup will allow doing everything at the "native Windows program" level (well, I can hope).

[...]
Show 11 quoted lines
> --- a/t/t9001-send-email.sh
> +++ b/t/t9001-send-email.sh
> @@ -539,15 +539,14 @@ 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 '"'"'$PWD/my-hooks/sendemail-validate'"'"' died with exit code 1
>  	warning: no patches were sent
>  	EOF
>  	test_cmp expect actual

Ideally we wouldn't have to check the exact output at all. Is there a reason we care about the absolute path being echoed on error?

[...]
Show 16 quoted lines
>  test_expect_success $PREREQ "--validate respects absolute core.hooksPath path" '
> -	hooks_path="$(pwd)/my-hooks" &&
> -	test_config core.hooksPath "$hooks_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>" \
> @@ -558,7 +557,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 '"'"'$PWD/my-hooks/sendemail-validate'"'"' died with exit code 1
>  	warning: no patches were sent
>  	EOF
>  	test_cmp expect actual
Likewise.
That said, the patch as is is
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Thanks.
Ævar Arnfjörð Bjarmason· May 24, 2021, 22:15 UTC · re: Johannes Sixt · lore

Re: [PATCH] t9001-send-email.sh: fix expected absolute paths on Windows

On Mon, May 24 2021, Johannes Sixt wrote:

Also CC-ing Robert Foss <robert.foss@linaro.org>, I last touched this code, but the fallout is ultimately from his c8243933c74 (git-send-email: Respect core.hooksPath setting, 2021-03-23).

Show 67 quoted lines
> Git for Windows is a native Windows program that works with native
> absolute paths in the drive letter style C:\dir. The auxiliary
> infrastructure is based on MSYS2, which uses POSIX style /C/dir.
>
> When we test for output of absolute paths produced by git.exe, we
> usally have to expect C:\dir style paths. To produce such expected
> paths, we have to use $(pwd) in the test scripts; the alternative,
> $PWD, produces a POSIX style path. ($PWD is a shell variable, and the
> shell is bash, an MSYS2 program, and operates in the POSIX realm.)
>
> There are two recently added tests that were written to expect C:\dir
> paths. The output that is tested is produced by `git send-email`, but
> behind the scenes, this is a Perl script, which also works in the
> POSIX realm and produces /C/dir style output.
>
> In the first test case that is changed here, replace $(pwd) by $PWD
> so that the expected path is constructed using /C/dir style.
>
> The second test case sets core.hooksPath to an absolute path. Since
> the test script talks to native git.exe, it is supposed to place a
> C:/dir style path into the configuration; therefore, keep $(pwd).
> When this configuration value is consumed by the Perl script, it is
> transformed to /C/dir style by the MSYS2 layer and echoed back in
> this form in the error message. Hence, do use $PWD for the expected
> value.
>
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---
>  When I say "the configuration is transformed to /C/dir style", I am
>  actually hand-waving: I can observe that a transformation must
>  happen somewhere, but I actually do not know where the conversion
>  really happens. "The MSYS2 layer" is my best qualified guess.
>
>  t/t9001-send-email.sh | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
> index 65b3035371..68bebc505b 100755
> --- a/t/t9001-send-email.sh
> +++ b/t/t9001-send-email.sh
> @@ -539,15 +539,14 @@ 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 '"'"'$PWD/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" '
> -	hooks_path="$(pwd)/my-hooks" &&
> -	test_config core.hooksPath "$hooks_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>" \
> @@ -558,7 +557,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 '"'"'$PWD/my-hooks/sendemail-validate'"'"' died with exit code 1
>  	warning: no patches were sent
>  	EOF
>  	test_cmp expect actual

Does this alternate patch[1] also fix the issue? I don't have a Windows system on which to test this, but it seems to me like it should.

I.e. the issue seems to me to me that we have an absolute path from --git-path, and one of Cwd.pm's or File::Spec.pm's ideas of what that absolute path should look like differs from ours.

We have a sprinkle of File::Spec->file_name_is_absolute($dir) in Git.pm for some other stuff to deal with the same scenario, but I don't see why we need the abs_path() here at all.

Either we have a relative path from "rev-parse --git-dir hooks", or an absolute one, in either case we feed it to Perl's system("some-relative-or-absolute-path").

I have a parallel series where I did some send-email changes by just extracting the relevant code from Git.pm, since there were objections to changing the "public API". But in this case there's been no release with this, so presumably it's fine to just change it.

1.
Show changes to 2 files +2 −3

perl/Git.pm, t/t9001-send-email.sh

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 65b30353719..9c518462c3e 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 '"'"'$(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
Ævar Arnfjörð Bjarmason· May 24, 2021, 23:15 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH] t9001-send-email.sh: fix expected absolute paths on Windows

On Tue, May 25 2021, Ævar Arnfjörð Bjarmason wrote:
Show 76 quoted lines
> On Mon, May 24 2021, Johannes Sixt wrote:
>
> Also CC-ing Robert Foss <robert.foss@linaro.org>, I last touched this
> code, but the fallout is ultimately from his c8243933c74
> (git-send-email: Respect core.hooksPath setting, 2021-03-23).
>
>> Git for Windows is a native Windows program that works with native
>> absolute paths in the drive letter style C:\dir. The auxiliary
>> infrastructure is based on MSYS2, which uses POSIX style /C/dir.
>>
>> When we test for output of absolute paths produced by git.exe, we
>> usally have to expect C:\dir style paths. To produce such expected
>> paths, we have to use $(pwd) in the test scripts; the alternative,
>> $PWD, produces a POSIX style path. ($PWD is a shell variable, and the
>> shell is bash, an MSYS2 program, and operates in the POSIX realm.)
>>
>> There are two recently added tests that were written to expect C:\dir
>> paths. The output that is tested is produced by `git send-email`, but
>> behind the scenes, this is a Perl script, which also works in the
>> POSIX realm and produces /C/dir style output.
>>
>> In the first test case that is changed here, replace $(pwd) by $PWD
>> so that the expected path is constructed using /C/dir style.
>>
>> The second test case sets core.hooksPath to an absolute path. Since
>> the test script talks to native git.exe, it is supposed to place a
>> C:/dir style path into the configuration; therefore, keep $(pwd).
>> When this configuration value is consumed by the Perl script, it is
>> transformed to /C/dir style by the MSYS2 layer and echoed back in
>> this form in the error message. Hence, do use $PWD for the expected
>> value.
>>
>> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
>> ---
>>  When I say "the configuration is transformed to /C/dir style", I am
>>  actually hand-waving: I can observe that a transformation must
>>  happen somewhere, but I actually do not know where the conversion
>>  really happens. "The MSYS2 layer" is my best qualified guess.
>>
>>  t/t9001-send-email.sh | 7 +++----
>>  1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
>> index 65b3035371..68bebc505b 100755
>> --- a/t/t9001-send-email.sh
>> +++ b/t/t9001-send-email.sh
>> @@ -539,15 +539,14 @@ 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 '"'"'$PWD/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" '
>> -	hooks_path="$(pwd)/my-hooks" &&
>> -	test_config core.hooksPath "$hooks_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>" \
>> @@ -558,7 +557,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 '"'"'$PWD/my-hooks/sendemail-validate'"'"' died with exit code 1
>>  	warning: no patches were sent
>>  	EOF
>>  	test_cmp expect actual
>
> Does this alternate patch[1] also fix the issue? I don't have a Windows
> system on which to test this, but it seems to me like it should.

I've submitted that (in this thread) as https://lore.kernel.org/git/cover-0.2-00000000000-20210524T231047Z-avarab@gmail.com/, but as noted here I haven't tested it on Windows, so testing if it works would be most welcome...

Show 46 quoted lines
> I.e. the issue seems to me to me that we have an absolute path from
> --git-path, and one of Cwd.pm's or File::Spec.pm's ideas of what that
> absolute path should look like differs from ours.
>
> We have a sprinkle of File::Spec->file_name_is_absolute($dir) in Git.pm
> for some other stuff to deal with the same scenario, but I don't see why
> we need the abs_path() here at all.
>
> Either we have a relative path from "rev-parse --git-dir hooks", or an
> absolute one, in either case we feed it to Perl's
> system("some-relative-or-absolute-path").
>
> I have a parallel series where I did some send-email changes by just
> extracting the relevant code from Git.pm, since there were objections to
> changing the "public API". But in this case there's been no release with
> this, so presumably it's fine to just change it.
>
> 1.
>
> 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 65b30353719..9c518462c3e 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 '"'"'$(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
Ævar Arnfjörð Bjarmason· May 24, 2021, 23:14 UTC · re: Johannes Sixt · lore

[PATCH 0/2] send-email: pre-release fixes for v2.32.0

The 1/2 here fixes a bug I introduced with an error message going missing.

The 2/2 hopefully replaces <bb30fe2b-cd75-4782-24a6-08bb002a0367@kdbg.org>[1], but I have not tested it on Windows. I think improving the error message is better strategy here than working around the current Git.pm abs_path() behavior.

This has a trivial conflict with my outstanding [2] series which is easily solved, we just need to take the side introduced in this topic (i.e. abs_path isn't needed anymore).

Also, I said I'd CC Robert Foss in [3] but didn't, finally doing that here.

1. http://lore.kernel.org/git/bb30fe2b-cd75-4782-24a6-08bb002a0367@kdbg.org
2. https://lore.kernel.org/git/cover-00.13-00000000000-20210524T074932Z-avarab@gmail.com
3. http://lore.kernel.org/git/87im37ojrn.fsf@evledraar.gmail.com
Ævar Arnfjörð Bjarmason (2):
  send-email: fix missing error message regression
  send-email: don't needlessly abs_path() the core.hooksPath
 git-send-email.perl   | 12 +++++++++++-
 perl/Git.pm           |  3 +--
 t/t9001-send-email.sh | 25 ++++++++++++++++++++++---
 3 files changed, 34 insertions(+), 6 deletions(-)
-- 
2.32.0.rc1.385.g9db524b96f7
Ævar Arnfjörð Bjarmason· May 24, 2021, 23:14 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH 1/2] send-email: fix missing error message regression

Fix a regression with the "the editor exited uncleanly, aborting everything" error message going missing after my d21616c0394 (git-send-email: refactor duplicate $? checks into a function, 2021-04-06).

I introduced a $msg variable, but did not actually use it. This caused us to miss the optional error message supplied by the "do_edit" codepath. Fix that, and add tests to check that this works.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 git-send-email.perl   | 12 +++++++++++-
 t/t9001-send-email.sh | 23 +++++++++++++++++++++--
 2 files changed, 32 insertions(+), 3 deletions(-)
Show changes to 2 files +32 −3

git-send-email.perl, t/t9001-send-email.sh

diff --git a/git-send-email.perl b/git-send-email.perl
index 175da07d946..170f42fe210 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -219,8 +219,18 @@ sub system_or_msg {
 	my $exit_code = $? >> 8;
 	return unless $signalled or $exit_code;
 
+	my @sprintf_args = ($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
+		# "no warnings 'redundant'", since that category was
+		# introduced in perl 5.22, and asking for it will die
+		# on older perls.
+		no warnings;
+		return sprintf($msg, @sprintf_args);
+	}
 	return sprintf(__("fatal: command '%s' died with exit code %d"),
-		       $args->[0], $exit_code);
+		       @sprintf_args);
 }
 
 sub system_or_die {
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 65b30353719..2acf389837c 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -644,14 +644,33 @@ test_expect_success $PREREQ 'In-Reply-To with --chain-reply-to' '
 	test_cmp expect actual
 '
 
+test_set_editor "$(pwd)/fake-editor"
+
+test_expect_success $PREREQ 'setup erroring fake editor' '
+	write_script fake-editor <<-\EOF
+	echo >&2 "I am about to error"
+	exit 1
+	EOF
+'
+
+test_expect_success $PREREQ 'fake editor dies with error' '
+	clean_fake_sendmail &&
+	test_must_fail git send-email \
+		--compose --subject foo \
+		--from="Example <nobody@example.com>" \
+		--to=nobody@example.com \
+		--smtp-server="$(pwd)/fake.sendmail" \
+		$patches 2>err &&
+	grep "I am about to error" err &&
+	grep "the editor exited uncleanly, aborting everything" err
+'
+
 test_expect_success $PREREQ 'setup fake editor' '
 	write_script fake-editor <<-\EOF
 	echo fake edit >>"$1"
 	EOF
 '
 
-test_set_editor "$(pwd)/fake-editor"
-
 test_expect_success $PREREQ '--compose works' '
 	clean_fake_sendmail &&
 	git send-email \

-- 
2.32.0.rc1.385.g9db524b96f7
Ævar Arnfjörð Bjarmason· May 24, 2021, 23:14 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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.

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

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().

1. http://lore.kernel.org/git/bb30fe2b-cd75-4782-24a6-08bb002a0367@kdbg.org
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 perl/Git.pm           | 3 +--
 t/t9001-send-email.sh | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)
Show changes to 2 files +2 −3

perl/Git.pm, t/t9001-send-email.sh

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 2acf389837c..3b7540050ca 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 '"'"'$(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

-- 
2.32.0.rc1.385.g9db524b96f7
Junio C Hamano· May 25, 2021, 01:03 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
> 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.

I see the author of that patch CC'ed; the change in question explains why we switched from "the hooks directory immediately under $repo->repo_path()" to "ask 'rev-parse --git-path hooks'", but it does not say why we call abs_path() on the result. I guess that is because $repo->repo_path() has always been a result of applying the abs_path() function to something, so it was to safeguard the callers that expect an absolute path coming back from hooks_path?

And that makes this change dubious, especially as a band-aid for a breakage immediately before the final release, doesn't it? Are we convinced that the callers are OK with seeing sometimes relative paths? Certainly the cases the tests J6t fixed are not negatively affected, but is that sufficient? To what directory is the configuration variable supposed to be relative to, and are we sure that the user will always invoke "git send-email" from that directory?

Puzzled.
Robert Foss· May 25, 2021, 06:10 UTC · re: Junio C Hamano · lore

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

Hey
On Tue, 25 May 2021 at 03:03, Junio C Hamano <gitster@pobox.com> wrote:
Show 15 quoted lines
>
> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
> > 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.
>
> I see the author of that patch CC'ed; the change in question
> explains why we switched from "the hooks directory immediately under
> $repo->repo_path()" to "ask 'rev-parse --git-path hooks'", but it
> does not say why we call abs_path() on the result.  I guess that is
> because $repo->repo_path() has always been a result of applying the
> abs_path() function to something, so it was to safeguard the callers
> that expect an absolute path coming back from hooks_path?

I don't think I have a good reason why abs_path() was used, most likely it was copied from some other snippet and kept for uniformity.

Show 9 quoted lines
>
> And that makes this change dubious, especially as a band-aid for a
> breakage immediately before the final release, doesn't it?  Are we
> convinced that the callers are OK with seeing sometimes relative
> paths?  Certainly the cases the tests J6t fixed are not negatively
> affected, but is that sufficient?  To what directory is the
> configuration variable supposed to be relative to, and are we sure
> that the user will always invoke "git send-email" from that
> directory?

Previously this functionality was entirely not working, so I don't think we'll have any regressions. With that being said I'm unable to test that it works well on windows.

As far as what the expected norms for paths are within git, I really don't have any answers.

>
> Puzzled.
>
Ævar Arnfjörð Bjarmason· May 25, 2021, 05:57 UTC · re: Junio C Hamano · lore

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

On Tue, May 25 2021, Junio C Hamano wrote:
Show 23 quoted lines
> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> 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.
>
> I see the author of that patch CC'ed; the change in question
> explains why we switched from "the hooks directory immediately under
> $repo->repo_path()" to "ask 'rev-parse --git-path hooks'", but it
> does not say why we call abs_path() on the result.  I guess that is
> because $repo->repo_path() has always been a result of applying the
> abs_path() function to something, so it was to safeguard the callers
> that expect an absolute path coming back from hooks_path?
>
> And that makes this change dubious, especially as a band-aid for a
> breakage immediately before the final release, doesn't it?  Are we
> convinced that the callers are OK with seeing sometimes relative
> paths?  Certainly the cases the tests J6t fixed are not negatively
> affected, but is that sufficient?  To what directory is the
> configuration variable supposed to be relative to, and are we sure
> that the user will always invoke "git send-email" from that
> directory?

The one caller is git-send-email.perl is fine with it, at least on *nix, this fix still needs testing on Windows.

The repo_path() function was introduced in c8243933c74, so it's never been in a release, thus I think it's fine to alter its behavior.

The code here doesn't need to concern itself with what needs to be relative to what, you run send-email in some working tree directory (or top-level), and depending on core.hooksPath we'll either return a relative path to the .git/hooks or an absolute one, the system() invocation will accept either.

Ævar Arnfjörð Bjarmason· May 25, 2021, 06:13 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

On Tue, May 25 2021, Ævar Arnfjörð Bjarmason wrote:
Show 37 quoted lines
> On Tue, May 25 2021, Junio C Hamano wrote:
>
>> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>>
>>> 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.
>>
>> I see the author of that patch CC'ed; the change in question
>> explains why we switched from "the hooks directory immediately under
>> $repo->repo_path()" to "ask 'rev-parse --git-path hooks'", but it
>> does not say why we call abs_path() on the result.  I guess that is
>> because $repo->repo_path() has always been a result of applying the
>> abs_path() function to something, so it was to safeguard the callers
>> that expect an absolute path coming back from hooks_path?
>>
>> And that makes this change dubious, especially as a band-aid for a
>> breakage immediately before the final release, doesn't it?  Are we
>> convinced that the callers are OK with seeing sometimes relative
>> paths?  Certainly the cases the tests J6t fixed are not negatively
>> affected, but is that sufficient?  To what directory is the
>> configuration variable supposed to be relative to, and are we sure
>> that the user will always invoke "git send-email" from that
>> directory?
>
> The one caller is git-send-email.perl is fine with it, at least on *nix,
> this fix still needs testing on Windows.
>
> The repo_path() function was introduced in c8243933c74, so it's never
> been in a release, thus I think it's fine to alter its behavior.
>
> The code here doesn't need to concern itself with what needs to be
> relative to what, you run send-email in some working tree directory (or
> top-level), and depending on core.hooksPath we'll either return a
> relative path to the .git/hooks or an absolute one, the system()
> invocation will accept either.

...I think the one issue with my 2/2 is that it doesn't go far enough, we should just remove the repo_path() from Git.pm and instead use its:

    $self->command_oneline('rev-parse', '--git-path', 'hooks')

...in the one user in git-send-email.perl, as discussed in other serieses Git.pm is "public", so stuff we stick in there we can't alter. In this case we're doing so before a release, and nobody wanted this except git-send-email.perl.

That caller is likely to just go away if and when Emily's "git hook run" lands, so I think it would be best (but not strictly needed for the pre-rc fix) to just remove that API.

What do you / Robert Foss think (maybe he wanted this for something else...)?

Robert Foss· May 25, 2021, 06:21 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

On Tue, 25 May 2021 at 08:15, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
Show 46 quoted lines
>
>
> On Tue, May 25 2021, Ævar Arnfjörð Bjarmason wrote:
>
> > On Tue, May 25 2021, Junio C Hamano wrote:
> >
> >> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
> >>
> >>> 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.
> >>
> >> I see the author of that patch CC'ed; the change in question
> >> explains why we switched from "the hooks directory immediately under
> >> $repo->repo_path()" to "ask 'rev-parse --git-path hooks'", but it
> >> does not say why we call abs_path() on the result.  I guess that is
> >> because $repo->repo_path() has always been a result of applying the
> >> abs_path() function to something, so it was to safeguard the callers
> >> that expect an absolute path coming back from hooks_path?
> >>
> >> And that makes this change dubious, especially as a band-aid for a
> >> breakage immediately before the final release, doesn't it?  Are we
> >> convinced that the callers are OK with seeing sometimes relative
> >> paths?  Certainly the cases the tests J6t fixed are not negatively
> >> affected, but is that sufficient?  To what directory is the
> >> configuration variable supposed to be relative to, and are we sure
> >> that the user will always invoke "git send-email" from that
> >> directory?
> >
> > The one caller is git-send-email.perl is fine with it, at least on *nix,
> > this fix still needs testing on Windows.
> >
> > The repo_path() function was introduced in c8243933c74, so it's never
> > been in a release, thus I think it's fine to alter its behavior.
> >
> > The code here doesn't need to concern itself with what needs to be
> > relative to what, you run send-email in some working tree directory (or
> > top-level), and depending on core.hooksPath we'll either return a
> > relative path to the .git/hooks or an absolute one, the system()
> > invocation will accept either.
>
> ...I think the one issue with my 2/2 is that it doesn't go far enough,
> we should just remove the repo_path() from Git.pm and instead use its:
>
>     $self->command_oneline('rev-parse', '--git-path', 'hooks')
This looks good to me.
Show 9 quoted lines
>
> ...in the one user in git-send-email.perl, as discussed in other
> serieses Git.pm is "public", so stuff we stick in there we can't
> alter. In this case we're doing so before a release, and nobody wanted
> this except git-send-email.perl.
>
> That caller is likely to just go away if and when Emily's "git hook run"
> lands, so I think it would be best (but not strictly needed for the
> pre-rc fix) to just remove that API.

That sounds like a sane approach. If core.hooksPath is needed in more locations in the future, it can be added then.

>
> What do you / Robert Foss think (maybe he wanted this for something
> else...)?
Junio C Hamano· May 25, 2021, 06:21 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 8 quoted lines
> The repo_path() function was introduced in c8243933c74, so it's never
> been in a release, thus I think it's fine to alter its behavior.
>
> The code here doesn't need to concern itself with what needs to be
> relative to what, you run send-email in some working tree directory (or
> top-level), and depending on core.hooksPath we'll either return a
> relative path to the .git/hooks or an absolute one, the system()
> invocation will accept either.

All of that are convincing, but I'd rather not risk it. I'll take your 1/2 with J6t's fix, both of which are obvious and no brainer, but I am willing to take this 2/2 as a post-release clean-up next monght, though ;-)

Thanks.
Ævar Arnfjörð Bjarmason· May 25, 2021, 12:09 UTC · re: Junio C Hamano · lore

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

On Tue, May 25 2021, Junio C Hamano wrote:
Show 15 quoted lines
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> The repo_path() function was introduced in c8243933c74, so it's never
>> been in a release, thus I think it's fine to alter its behavior.
>>
>> The code here doesn't need to concern itself with what needs to be
>> relative to what, you run send-email in some working tree directory (or
>> top-level), and depending on core.hooksPath we'll either return a
>> relative path to the .git/hooks or an absolute one, the system()
>> invocation will accept either.
>
> All of that are convincing, but I'd rather not risk it.  I'll take
> your 1/2 with J6t's fix, both of which are obvious and no brainer,
> but I am willing to take this 2/2 as a post-release clean-up next
> monght, though ;-)

Fair enough. I get that we're in the rc phase, but FWIW I still disagree.

I do think supporting whatever edge case we're exposing by round-tripping through abs_path() and its subtly different behavior is worse than a change to make us rely on the well-understood rev-parse behavior directly.

Especially since the fix is to something that didn't work to begin with before the v2.32.0 release is out (core.hooksPath + send-email), but perhaps that's more of an argument for reverting the topic during RC + try to have it land in v2.33.0?

And that + what seems to be the prevailing (IMO nonsensical) opinion on Git.pm's stability promise, meaning that once this new function is out in a release we'll be stuck supporting it makes me want to change this pre-release.

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.

Junio C Hamano· May 25, 2021, 19:28 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

Ævar Arnfjörð Bjarmason <avarab@gmail.com> 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.
Ævar Arnfjörð Bjarmason· May 26, 2021, 11:21 UTC · re: Junio C Hamano · lore

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

On Wed, May 26 2021, Junio C Hamano wrote:
Show 7 quoted lines
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> 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.

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 <nobody@example.com>" \
    +@@ 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
Ævar Arnfjörð Bjarmason· May 26, 2021, 11:21 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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 <avarab@gmail.com>
---
 perl/Git.pm           | 3 +--
 t/t9001-send-email.sh | 7 ++++---
 2 files changed, 5 insertions(+), 5 deletions(-)
Show changes to 2 files +5 −5

perl/Git.pm, t/t9001-send-email.sh

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
Ævar Arnfjörð Bjarmason· May 26, 2021, 11:21 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH v2 2/2] send-email: move "hooks_path" invocation to git-send-email.perl

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 <avarab@gmail.com>
---
 git-send-email.perl |  3 ++-
 perl/Git.pm         | 12 ------------
 2 files changed, 2 insertions(+), 13 deletions(-)
Show changes to 2 files +2 −13

git-send-email.perl, perl/Git.pm

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
Felipe Contreras· May 26, 2021, 01:22 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

Ævar Arnfjörð Bjarmason wrote:
Show 28 quoted lines
> On Tue, May 25 2021, Junio C Hamano wrote:
> 
> > Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
> >
> >> 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.
> >
> > I see the author of that patch CC'ed; the change in question
> > explains why we switched from "the hooks directory immediately under
> > $repo->repo_path()" to "ask 'rev-parse --git-path hooks'", but it
> > does not say why we call abs_path() on the result.  I guess that is
> > because $repo->repo_path() has always been a result of applying the
> > abs_path() function to something, so it was to safeguard the callers
> > that expect an absolute path coming back from hooks_path?
> >
> > And that makes this change dubious, especially as a band-aid for a
> > breakage immediately before the final release, doesn't it?  Are we
> > convinced that the callers are OK with seeing sometimes relative
> > paths?  Certainly the cases the tests J6t fixed are not negatively
> > affected, but is that sufficient?  To what directory is the
> > configuration variable supposed to be relative to, and are we sure
> > that the user will always invoke "git send-email" from that
> > directory?
> 
> The one caller is git-send-email.perl is fine with it, at least on *nix,
> this fix still needs testing on Windows.

While I understand the reluctance to test things on Windows, sometimes we waste more time discussing about what couldn't potentially break there than the time it takes to setup a VM and just make sure.

Am I the only one that has setup a Windows VM just for tests?
Also, there's ways to trigger CI runs to test this stuff aren't there?
Seems like a much less painful way to leapfrog this roadblock.
Cheers.
-- 
Felipe Contreras
Johannes Schindelin· Jun 2, 2021, 11:40 UTC · re: Johannes Sixt · lore

Re: [PATCH] t9001-send-email.sh: fix expected absolute paths on Windows

Hi Hannes,
On Mon, 24 May 2021, Johannes Sixt wrote:
> Git for Windows is a native Windows program that works with native
> absolute paths in the drive letter style C:\dir. The auxiliary
> infrastructure is based on MSYS2, which uses POSIX style /C/dir.

As far as I remember, VMS is also POSIX, and it has a different path style. Therefore I would probably use the term "Unix style" here instead of "POSIX style".

But that has nothing to do with the validity of your point: it is still a correct and important observation.

Show 28 quoted lines
> When we test for output of absolute paths produced by git.exe, we
> usally have to expect C:\dir style paths. To produce such expected
> paths, we have to use $(pwd) in the test scripts; the alternative,
> $PWD, produces a POSIX style path. ($PWD is a shell variable, and the
> shell is bash, an MSYS2 program, and operates in the POSIX realm.)
>
> There are two recently added tests that were written to expect C:\dir
> paths. The output that is tested is produced by `git send-email`, but
> behind the scenes, this is a Perl script, which also works in the
> POSIX realm and produces /C/dir style output.
>
> In the first test case that is changed here, replace $(pwd) by $PWD
> so that the expected path is constructed using /C/dir style.
>
> The second test case sets core.hooksPath to an absolute path. Since
> the test script talks to native git.exe, it is supposed to place a
> C:/dir style path into the configuration; therefore, keep $(pwd).
> When this configuration value is consumed by the Perl script, it is
> transformed to /C/dir style by the MSYS2 layer and echoed back in
> this form in the error message. Hence, do use $PWD for the expected
> value.
>
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---
>  When I say "the configuration is transformed to /C/dir style", I am
>  actually hand-waving: I can observe that a transformation must
>  happen somewhere, but I actually do not know where the conversion
>  really happens. "The MSYS2 layer" is my best qualified guess.

Indeed, it is the MSYS2 runtime that performs this conversion. Concretely, whenever you call a non-MSYS2 program from within MSYS2, command-line arguments that look like Unix paths are converted by replacing forward slashes with backslashes and by prefixing absolute paths with MSYS2' root directory (as a Windows style path, of course).

However, in this instance, it is a different problem, I think.

Perl cannot handle Windows style paths. At least _Git's_ Perl scripts cannot.

For example, the `PATH` variable is assumed to contain colon-separated directory paths in our scripts. But that is not true on Windows: the colon already separates the drive letter from the rest of the path, and therefore the separator used in `PATH` is a _semicolon_.

To help with this, the MSYS2 runtime converts the command-line arguments and environment variables that look like path lists (such as `PATH`) and paths (such as `SYSTEMROOT`) from Windows style to Unix style when it detects that, say, MSYS2's Perl is started from a non-MSYS2 program such as `git.exe`.

Which means that the Perl code executed in Ævar's tests spits out Unix style paths.

Happily for us, the MSYS2 Bash with which Git's test suite is expected to be executed on Windows understands those Unix style paths very well! All we need to do is to use them here, and that is what your patch does, therefore:

Acked-by: Johannes Schindelin <johannes.schindelin@gmx.de>

Thank you, Dscho

Show 39 quoted lines
>
>  t/t9001-send-email.sh | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
> index 65b3035371..68bebc505b 100755
> --- a/t/t9001-send-email.sh
> +++ b/t/t9001-send-email.sh
> @@ -539,15 +539,14 @@ 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 '"'"'$PWD/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" '
> -	hooks_path="$(pwd)/my-hooks" &&
> -	test_config core.hooksPath "$hooks_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>" \
> @@ -558,7 +557,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 '"'"'$PWD/my-hooks/sendemail-validate'"'"' died with exit code 1
>  	warning: no patches were sent
>  	EOF
>  	test_cmp expect actual
> --
> 2.31.0.152.g120726e270
>
>

← back to recent threads