[PATCH 0/1] Fix testing of `master` on Windows

[PATCH 0/1] Fix testing of `master` on Windows

[Johannes Schindelin]

Sorry, I missed this (trivially-fixed) test breakage... FWIW this is
*not* part of the rebase--helper patches ;-)


Johannes Schindelin (1):
  mingw: work around t2300's assuming non-Windows paths

 t/t2300-cd-to-toplevel.sh | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

Published-As: https://github.com/dscho/git/releases/tag/mingw-t2300-v1
-- 
2.9.0.118.gce770ba.dirty

base-commit: 05219a1276341e72d8082d76b7f5ed394b7437a4

[PATCH 1/1] mingw: work around t2300's assuming non-Windows paths

[Johannes Schindelin]

On Windows, we have to juggle two different schemes of representing
paths: the native, Windows paths (the only ones known to the main
Git executable) on the one hand, and POSIX-ish ones used by the Bash
through MSYS2's POSIX emulation layer on the other hand.

A Windows path looks like this: C:\git-sdk-64\usr\src\git. In modern
Windows, it is almost always legal to use forward slashes as directory
separators, which is the reason why the Git executable itself would use
the path C:/git-sdk-64/usr/src/git instead. The equivalent POSIX-ish
path would be: /c/git-sdk-64/usr/src/git.

This patch works around the assumption of t2300-cd-to-toplevel.sh that
`git --exec-path` spits out a POSIX-ish path, by converting the output
accordingly.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t2300-cd-to-toplevel.sh | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/t/t2300-cd-to-toplevel.sh b/t/t2300-cd-to-toplevel.sh
index cccd7d9..c8de6d8 100755
--- a/t/t2300-cd-to-toplevel.sh
+++ b/t/t2300-cd-to-toplevel.sh
@@ -4,11 +4,19 @@ test_description='cd_to_toplevel'
 
 . ./test-lib.sh
 
+EXEC_PATH="$(git --exec-path)"
+test_have_prereq !MINGW ||
+case "$EXEC_PATH" in
+[A-Za-z]:/*)
+	EXEC_PATH="/${EXEC_PATH%%:*}${EXEC_PATH#?:}"
+	;;
+esac
+
 test_cd_to_toplevel () {
 	test_expect_success $3 "$2" '
 		(
 			cd '"'$1'"' &&
-			PATH="$(git --exec-path):$PATH" &&
+			PATH="$EXEC_PATH:$PATH" &&
 			. git-sh-setup &&
 			cd_to_toplevel &&
 			[ "$(pwd -P)" = "$TOPLEVEL" ]
-- 
2.9.0.118.gce770ba.dirty

Re: [PATCH 1/1] mingw: work around t2300's assuming non-Windows paths

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> On Windows, we have to juggle two different schemes of representing
> paths: the native, Windows paths (the only ones known to the main
> Git executable) on the one hand, and POSIX-ish ones used by the Bash
> through MSYS2's POSIX emulation layer on the other hand.
>
> A Windows path looks like this: C:\git-sdk-64\usr\src\git. In modern
> Windows, it is almost always legal to use forward slashes as directory
> separators, which is the reason why the Git executable itself would use
> the path C:/git-sdk-64/usr/src/git instead. The equivalent POSIX-ish
> path would be: /c/git-sdk-64/usr/src/git.
>
> This patch works around the assumption of t2300-cd-to-toplevel.sh that
> `git --exec-path` spits out a POSIX-ish path, by converting the output
> accordingly.

Hmm, I am confused.  `git --exec-path` _is_ meant to "spit out" a
path that is usable when prepended/appended to $PATH [1], and it
does _not_ have to be POSIX-ish path.  It is totally up to the port
to adjust it to the platform's convention how the $PATH environment
variable is understood.

If $PATH cannot take C:/git-sdk-64/usr/src/git but does understand
/c/git-sdk-64/usr/src/git, perhaps "git --exec-path" should be
emitting the latter in the first place?


[Footnote]

*1* That after all was how we handled the painful 1.6 "'git-cmd' to
'git cmd'" transition (cf. $gmane/93793).



> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  t/t2300-cd-to-toplevel.sh | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/t/t2300-cd-to-toplevel.sh b/t/t2300-cd-to-toplevel.sh
> index cccd7d9..c8de6d8 100755
> --- a/t/t2300-cd-to-toplevel.sh
> +++ b/t/t2300-cd-to-toplevel.sh
> @@ -4,11 +4,19 @@ test_description='cd_to_toplevel'
>  
>  . ./test-lib.sh
>  
> +EXEC_PATH="$(git --exec-path)"
> +test_have_prereq !MINGW ||
> +case "$EXEC_PATH" in
> +[A-Za-z]:/*)
> +	EXEC_PATH="/${EXEC_PATH%%:*}${EXEC_PATH#?:}"
> +	;;
> +esac
> +
>  test_cd_to_toplevel () {
>  	test_expect_success $3 "$2" '
>  		(
>  			cd '"'$1'"' &&
> -			PATH="$(git --exec-path):$PATH" &&
> +			PATH="$EXEC_PATH:$PATH" &&
>  			. git-sh-setup &&
>  			cd_to_toplevel &&
>  			[ "$(pwd -P)" = "$TOPLEVEL" ]

Re: [PATCH 1/1] mingw: work around t2300's assuming non-Windows paths

[Johannes Schindelin]

Hi Junio,

On Mon, 20 Jun 2016, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > On Windows, we have to juggle two different schemes of representing
> > paths: the native, Windows paths (the only ones known to the main Git
> > executable) on the one hand, and POSIX-ish ones used by the Bash
> > through MSYS2's POSIX emulation layer on the other hand.
> >
> > A Windows path looks like this: C:\git-sdk-64\usr\src\git. In modern
> > Windows, it is almost always legal to use forward slashes as directory
> > separators, which is the reason why the Git executable itself would
> > use the path C:/git-sdk-64/usr/src/git instead. The equivalent
> > POSIX-ish path would be: /c/git-sdk-64/usr/src/git.
> >
> > This patch works around the assumption of t2300-cd-to-toplevel.sh that
> > `git --exec-path` spits out a POSIX-ish path, by converting the output
> > accordingly.
> 
> Hmm, I am confused.  `git --exec-path` _is_ meant to "spit out" a
> path that is usable when prepended/appended to $PATH [1], and it
> does _not_ have to be POSIX-ish path.  It is totally up to the port
> to adjust it to the platform's convention how the $PATH environment
> variable is understood.

The port does exactly what it is supposed to do: the output of `git
--exec-path` can be prepended/appended to %PATH%. The point here is:
%PATH% is *semicolon*-delimited.

All problems start when we do not use scripting native to Windows, but the
Bash. In the Bash, we *assume* that $PATH is *colon*-delimited. All the
time. And that is part of what makes a POSIX emulation layer on Windows so
challenging.

> If $PATH cannot take C:/git-sdk-64/usr/src/git but does understand
> /c/git-sdk-64/usr/src/git, perhaps "git --exec-path" should be
> emitting the latter in the first place?

Again, %PATH% can take C:/... just fine. But the Bash gets the
POSIX-layer-munged $PATH which has POSIX-ish paths so that it can
colon-delimit its PATH and all shell scripts can continue to work.

So the root cause for the problem which requires this work-around is that
our test suite is written in shell script, which is inherently not as
portable as we think it is.

Does that address your puzzlement? If so, would you kindly advise how to
improve the commit message (or the patch)?

Ciao,
Dscho

Re: [PATCH 1/1] mingw: work around t2300's assuming non-Windows paths

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> Hmm, I am confused.  `git --exec-path` _is_ meant to "spit out" a
>> path that is usable when prepended/appended to $PATH [1], and it
>> does _not_ have to be POSIX-ish path.  It is totally up to the port
>> to adjust it to the platform's convention how the $PATH environment
>> variable is understood.
>
> The port does exactly what it is supposed to do: the output of `git
> --exec-path` can be prepended/appended to %PATH%. The point here is:
> %PATH% is *semicolon*-delimited.

I said $PATH because --exec-path does not care what you do with
%PATH% but it deeply cares that its output is usable in $PATH.

I think you would need something similar to "pwd -W", that is, leave
"git --exec-path" as a way to give shell scripts people have written
over the years that allows them to say "git-cmd" as long as they do
PATH="$(git --exec-path):$PATH" upfront.  And for Windows scripts,
introduce a new option "git --exec-path-windows" that can give
C:/git-sdk-64/usr/src/git (or even using backslash).

I do not mean to say that exec_cmd.c::git_exec_path() function must
return a string /c/git-sdk-64/usr/src/git by the above.  It should
keep returning C:/git-sdk-64/usr/src/git, I think.  Its use in
exec_cmd.c::add_path() to use PATH_SEP to do the equivalent
internally for the platform would want C:/git-sdk-64/usr/src/git
there.  Also I think exec_cmd.c::argv_exec_path should keep using
platform native format.

Perhaps you can do the conversion you are doing with this
"let's change t2300" patch instead in C where the return value of
git_exec_path() is fed to puts() in git.c::handle_options(), or
better yet make a helper function git_exec_path_for_shell() that
calls git_exec_path() and does the conversion on Windows (and passes
the result from git_exec_path() intact on other platforms).

I say all of the above because I see this in hits from "git grep -e
--exec-path":

contrib/subtree/git-subtree.sh:PATH=$PATH:$(git --exec-path)

and I would imagine there are countless other shell scripts that
follow the "Output from 'git --exec-path' can be prepended/appended
to PATH to allow you write 'git-cmd' instead of 'git cmd'".  They
would not work unless your "git --exec-path" gives an output that is
usable by shell in $PATH (and not %PATH%).

Re: [PATCH 1/1] mingw: work around t2300's assuming non-Windows paths

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I think you would need something similar to "pwd -W", that is, leave
> "git --exec-path" as a way to give shell scripts people have written
> over the years that allows them to say "git-cmd" as long as they do
> PATH="$(git --exec-path):$PATH" upfront.  And for Windows scripts,
> introduce a new option "git --exec-path-windows" that can give
> C:/git-sdk-64/usr/src/git (or even using backslash).

Of course we could go the other way.  We can declare that the output
of "git --exec-path" is the format that is platform-native pathname
to the directory [*1*], introduce a new option that is better
named than "--exec-path-to-include-in-PATH-in-shell-scripts" that
does the "convert C:/ to /c/ on Windows before showing" thing, and
rewrite all the references that does PATH=$(git --exec-path):$PATH
in scripts to use that new option.

The fact remains that on some platforms two variants are needed.  We
can update our test scripts with "convert C:/ to /c/ on Windows" to
work around a test failure like the patch under discussion did, but
that approach would not scale to fix real world scripts that people
already have, which is what I am trying to see if we can address in
these two messages.


[Footnote]

*1* ...instead of "it is suitable for PATH=$(git --exec-path):$PATH
in your shell scripts", which was the definition I used in the
message I am responding to.  "The name '--exec-path' implies it is a
path to the directory, and it is more natural for that to be platform
native" is a valid argument (and that is why I am saying "we could
go the other way" here), but I am not convinced that the conclusion
that the argument leads to is a better one in the practical sense.

Re: [PATCH 1/1] mingw: work around t2300's assuming non-Windows paths

[Johannes Schindelin]

Hi Junio,

On Tue, 21 Jun 2016, Junio C Hamano wrote:

> I said $PATH because --exec-path does not care what you do with
> %PATH% but it deeply cares that its output is usable in $PATH.

The really, really, really important part to keep in mind is that there is
no $PATH on Windows.

Seriously, there is not.

There is a %PATH% because the scripting language native to Windows is the
Windows batch language, which works with native Windows paths.

The same holds true for Git, BTW: all of the C code we have handles
Windows paths just fine, as tedious as it is to keep this support intact.

Now, all of the troubles enter with Git's relying on shell and Perl
scripts. That is not a fault of the platform, of course, it is Git's
fault. It is cute and convenient when your entire world is the Linux
command-line, and when you do not really care about performance, either.

And these shell scripts have a different idea about paths than Windows, or
even the Git executable for that matter!

Now, the big question is: how much do you want to cast this reliance on
shell and Perl scripts in stone? That is an important question, and
affects directly the answer to the related question: Should `git
--exec-path` cater to POSIX shell scripts, or to scripts native to the
current platform?

Personally, I am not a fan of casting this in stone so much.

You see, shell scripts are as foreign to Windows as Windows batch scripts
are to Linux. I doubt that you would accept any patch that would change
all scripts to accept Windows-style paths even on Linux. Yet, the reverse
is true: Git for Windows is expected to accept non-Windows paths.

And all this talk did not even scratch the most important point. When I
say "POSIX paths" I know fully well that this is incorrect. POSIX paths
are not required to use a forward slash as directory separator and a colon
as path separator.

Think VAX.

I see the discussion veering into the wrong direction here. I am not
prepared to accept the notion of shell scripting as portable, certainly
not when we rely on so many Unix-isms.

So unless we come up with a truly portable way to test Git, and to
implement scripted parts of Git, I am convinced that our best course of
action is to continue to work around Git's erroneous assumption that shell
scripting is as native to the current platform as the Git executable
itself, and to continue to introduce glue code to keep things running
relatively smoothly.

Of course, if you have a splendid idea how to fix the underlying issue, I
am more than just eager to go there.

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > I think you would need something similar to "pwd -W", that is, leave
> > "git --exec-path" as a way to give shell scripts people have written
> > over the years that allows them to say "git-cmd" as long as they do
> > PATH="$(git --exec-path):$PATH" upfront.  And for Windows scripts,
> > introduce a new option "git --exec-path-windows" that can give
> > C:/git-sdk-64/usr/src/git (or even using backslash).
> 
> Of course we could go the other way.  We can declare that the output
> of "git --exec-path" is the format that is platform-native pathname
> to the directory [*1*], introduce a new option that is better
> named than "--exec-path-to-include-in-PATH-in-shell-scripts" that
> does the "convert C:/ to /c/ on Windows before showing" thing, and
> rewrite all the references that does PATH=$(git --exec-path):$PATH
> in scripts to use that new option.

I see your smirking when you wrote this.

> The fact remains that on some platforms two variants are needed.  We
> can update our test scripts with "convert C:/ to /c/ on Windows" to
> work around a test failure like the patch under discussion did, but
> that approach would not scale to fix real world scripts that people
> already have, which is what I am trying to see if we can address in
> these two messages.

This overstates the importance of shell scripting in general Git usage,
methinks, and certainly the importance of --exec-path with regards to the
PATH variable.

Git's own shell scripts do not need to extend the PATH to include Git's
exec path (when they are called, the PATH is already extended
appropriately). And neither do other shell scripts: the dashed
invocations are deprecated for way too long a time already.

Performance characteristics as well as quite serious portability problems
(just how much time do we still have to spend just to bend over backwards
for KSH, for example?) are simply not in favor of using shell scripting
for any serious Git operation.

Of course, it is not as bad for Linux users because shell scripts have
decent performance characteristics there (at least if you work with
projects at most the size of the Linux kernel), and you always have
at least a Dash, if not a Bash. But already going to MacOSX, shell scripts
are not so much fun anymore.

> *1* ...instead of "it is suitable for PATH=$(git --exec-path):$PATH
> in your shell scripts", which was the definition I used in the
> message I am responding to.  "The name '--exec-path' implies it is a
> path to the directory, and it is more natural for that to be platform
> native" is a valid argument (and that is why I am saying "we could
> go the other way" here), but I am not convinced that the conclusion
> that the argument leads to is a better one in the practical sense.

Yes, the practical issue at hand is: we are pretty much stuck with shell
scripting for Git's test suite (which is not nice for me: on the same
machine where the test suite requires an already painful 10 minutes to
complete in a Linux VM, it requires 45 minutes to complete in the
POSIX-emulating Bash).

And I would rather keep this test suite working on Windows using
work-arounds such as the patch in question than to spend a lot of effort
to support portable scripting, when scripting does not really matter all
that much in practice to begin with.

Ciao,
Dscho

Re: [PATCH 1/1] mingw: work around t2300's assuming non-Windows paths

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Tue, 21 Jun 2016, Junio C Hamano wrote:
>
>> I said $PATH because --exec-path does not care what you do with
>> %PATH% but it deeply cares that its output is usable in $PATH.
>
> The really, really, really important part to keep in mind is that there is
> no $PATH on Windows.

I think I know that well enough; please sanity check.  My
understanding is:

 * Your (emulated) getenv(3) reads %PATH% which may look like
   "c:\a\b;c:\c\d", i.e. Windows style.

 * Your argv_exec_path also looks like "c:\e\f", i.e. Windows
   style.

 * Your setup_path() would yield "c:\e\f;c:\a\b;c:\c\d" because it
   concatenates the above two with PATH_SEP, i.e. Windows style, and
   your setenv(3) will set that to %PATH%.

 * After all that happens, your run_command(), execv_git_cmd(),
   etc. would honor that %PATH%.

 * In all of the above, a back-slash can be (and may indeed be) a
   forward-slash, as library functions and system calls are prepared
   to take both since the good old DOS days.  I.e. "c:\a\b" can be
   "c:/a/b".  It cannot be "/c/a/b", however.

 * When bash gets spawned (perhaps because a hook is written in that
   language, perhaps because child_process.use_shell is set when
   executing an alias "!cmd", running a pager, or running an
   editor), the value of %PATH% derived by the above sequence is not
   exposed as $PATH.  There is the "rewrite leading C: with /C/"
   outside us (i.e. in bash).

And that is why I suggested to keep that "internal paths are in
platform native format" and apply the same conversion as what bash
does immediately before the returned value from git_exec_path() is
fed to puts().  That way, "$PATH" (not %PATH%) can be modified with
"$(git --exec-path)" in scripts the same way on all the platforms
and you do not have to break people's hooks.

Having said that, I realize that I missed one huge thing to take
into consideration.  I assume that you have been shipping Git for
Windows with this "'git --exec-path' gives c:\a\b, not /c/a/b"
feature for a long time, so existing Windows users will be broken if
we "fix" this (which would allow them to use shell scripts their
friends use on other platforms without modification).  So from that
point alone, "PATH=$(git --exec-path):$PATH in shell should work on
any platform and "git --exec-path" should be fixed" would not fly.
This simply is too late to fix.

The patch under discussion is the only door left for that test, and
a similar trickery is needed for any end-user scripts used for hooks
and aliases that use 'git --exec-path', if they ever want to be
cross-platform.

So let's take that "if Windows do this" change to t2300 as-is.

Re: [PATCH 1/1] mingw: work around t2300's assuming non-Windows paths

[Johannes Schindelin]

Hi Junio,

On Wed, 22 Jun 2016, Junio C Hamano wrote:

> So let's take that "if Windows do this" change to t2300 as-is.

Thanks!
Dscho

Re: [PATCH 1/1] mingw: work around t2300's assuming non-Windows paths

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I think I know that well enough; please sanity check.  My
> understanding is:
> ...
> The patch under discussion is the only door left for that test, and
> a similar trickery is needed for any end-user scripts used for hooks
> and aliases that use 'git --exec-path', if they ever want to be
> cross-platform.
>
> So let's take that "if Windows do this" change to t2300 as-is.

Assuming that the sanity check passes, here is a suggested rewrite
to explain the real problem better.

-- >8 --
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Sat, 18 Jun 2016 12:49:11 +0200
Subject: [PATCH] t2300: "git --exec-path" is not usable in $PATH on Windows as-is

The "git" command itself has an internal logic to prepend the
exec-path to the PATH environment variable for processes it spawns.
That is how ". git-sh-setup" in our scripted Porcelains can find the
dot-sourced file in $GIT_EXEC_PATH that is not usually on user's
PATH.

When t2300 runs, because it is not spawned by the "git" command, the
scriptlet being tested did not run with a realistic setting of PATH
environment.  It lacked the exec-path on the PATH, and failed to
find the dot-sourced file.  A recent update to t2300 attempted to do
so, with "PATH=$(git --exec-path):$PATH", which was the recommended
way around v1.6.0 days (a script whose original was written before
that release that survives to this day is likely to have such a line).

However, the "git --exec-path" command outputs C:\path\to\exec\dir
(not /c/path/to/exec/dir) on Windows; the recent update failed to
consider the problem that comes from it.

Even though Git itself, when doing the equivalent internally, does
so in a platform native way (i.e. on Windows, C:\path\to\exec\dir is
prepended to the existing value of %PATH% using ';' as a component
separator), the result is further massaged by bash and gets turned
into $PATH that uses /c/path/to/exec/dir with ':' separating the
components, which is the form understood by bash, so scripted
Porcelains finds commands from PATH correctly even on Windows.

An end user script written in shell, however, cannot prepend
"C:\path\to\exec\dir:" to the existing value of $PATH and expect
bash to magically turn it into the form it understands.  In other
words, "PATH=$(git --exec-path):$PATH" does not work as an emulation
of what "Git" internally does to the PATH on Windows.

To correctly emulate how exec-path is prepended to the PATH
environment internally on Windows, we'd need to convert
C:\git-sdk-64\usr\src\git to at least /c\git-sdk-64\usr\src\git
ourselves before prepending it to PATH.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t2300-cd-to-toplevel.sh | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/t/t2300-cd-to-toplevel.sh b/t/t2300-cd-to-toplevel.sh
index cccd7d9..c8de6d8 100755
--- a/t/t2300-cd-to-toplevel.sh
+++ b/t/t2300-cd-to-toplevel.sh
@@ -4,11 +4,19 @@ test_description='cd_to_toplevel'
 
 . ./test-lib.sh
 
+EXEC_PATH="$(git --exec-path)"
+test_have_prereq !MINGW ||
+case "$EXEC_PATH" in
+[A-Za-z]:/*)
+	EXEC_PATH="/${EXEC_PATH%%:*}${EXEC_PATH#?:}"
+	;;
+esac
+
 test_cd_to_toplevel () {
 	test_expect_success $3 "$2" '
 		(
 			cd '"'$1'"' &&
-			PATH="$(git --exec-path):$PATH" &&
+			PATH="$EXEC_PATH:$PATH" &&
 			. git-sh-setup &&
 			cd_to_toplevel &&
 			[ "$(pwd -P)" = "$TOPLEVEL" ]
-- 
2.9.0-346-g30ec1fd

Re: [PATCH 1/1] mingw: work around t2300's assuming non-Windows paths

[Johannes Schindelin]

Hi Junio,

On Wed, 22 Jun 2016, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > I think I know that well enough; please sanity check.  My
> > understanding is:
> > ...
> > The patch under discussion is the only door left for that test, and
> > a similar trickery is needed for any end-user scripts used for hooks
> > and aliases that use 'git --exec-path', if they ever want to be
> > cross-platform.
> >
> > So let's take that "if Windows do this" change to t2300 as-is.
> 
> Assuming that the sanity check passes, here is a suggested rewrite
> to explain the real problem better.

Yes, the sanity check passes, and your commit message is much better than
mine.

Thanks!
Dscho