Re: [PATCH 1/1] mingw: work around t2300's assuming non-Windows paths
Hi Junio, On Wed, 22 Jun 2016, Junio C Hamano wrote: > Junio C Hamano 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] mingw: work around t2300's assuming non-Windows paths
Junio C Hamano 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 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 Signed-off-by: Junio C Hamano --- 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] mingw: work around t2300's assuming non-Windows paths
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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] mingw: work around t2300's assuming non-Windows paths
Johannes Schindelin 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] mingw: work around t2300's assuming non-Windows paths
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 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 resp
Re: [PATCH 1/1] mingw: work around t2300's assuming non-Windows paths
Junio C Hamano 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] mingw: work around t2300's assuming non-Windows paths
Johannes Schindelin 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%). -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] mingw: work around t2300's assuming non-Windows paths
Hi Junio, On Mon, 20 Jun 2016, Junio C Hamano wrote: > Johannes Schindelin 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] mingw: work around t2300's assuming non-Windows paths
Johannes Schindelin 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 > --- > 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" ] -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/1] mingw: work around t2300's assuming non-Windows paths
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 --- 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html