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

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

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

[...]

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?

[...]

Likewise.

That said, the patch as is is
Reviewed-by: Jonathan Nieder <redacted>

Thanks.

On Mon, May 24 2021, Johannes Sixt wrote:

Also CC-ing Robert Foss [off-list ref], I last touched this
code, but the fallout is ultimately from his c8243933c74
(git-send-email: Respect core.hooksPath setting, 2021-03-23).

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

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

The 2/2 hopefully replaces
[off-list ref][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

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

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

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

On Mon, May 24 2021, Johannes Sixt wrote:

Also CC-ing Robert Foss [off-list ref], I last touched this
code, but the fallout is ultimately from his c8243933c74
(git-send-email: Respect core.hooksPath setting, 2021-03-23).

quoted

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

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

Thank you,
Dscho