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

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

[Johannes Sixt]

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
-- 
2.31.0.152.g120726e270

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

[Jonathan Nieder]

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

[...]
> --- 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?

[...]
>  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.

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

[Ævar Arnfjörð Bjarmason]

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

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

[Ævar Arnfjörð Bjarmason]

On Tue, May 25 2021, Ævar Arnfjörð Bjarmason wrote:

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

> 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

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

[Ævar Arnfjörð Bjarmason]

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

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

[Ævar Arnfjörð Bjarmason]

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

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

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

[Ævar Arnfjörð Bjarmason]

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

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

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

[Junio C Hamano]

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

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

[Ævar Arnfjörð Bjarmason]

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.

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

[Ævar Arnfjörð Bjarmason]

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

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

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

[Robert Foss]

On Tue, 25 May 2021 at 08:15, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>
>
> 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.

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

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

[Junio C Hamano]

Æ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 ;-)

Thanks.

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

[Ævar Arnfjörð Bjarmason]

On Tue, May 25 2021, Junio C Hamano wrote:

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

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

[Junio C Hamano]

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

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

[Ævar Arnfjörð Bjarmason]

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

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

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

[Ævar Arnfjörð Bjarmason]

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

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

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

[Ævar Arnfjörð Bjarmason]

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

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

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

[Felipe Contreras]

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

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

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

[Robert Foss]

Hey

On Tue, 25 May 2021 at 03:03, Junio C Hamano <gitster@pobox.com> 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?

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.

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

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 5163 bytes --]

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.

> 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

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