Re: [PATCH 1/2] subtree: fix the GIT_EXEC_PATH sanity check to work on Windows

Hi Luke,

On Thu, 10 Jun 2021, Luke Shumaker wrote:

On Thu, 10 Jun 2021 03:13:30 -0600,
Johannes Schindelin via GitGitGadget wrote:

quoted

From: Johannes Schindelin <redacted>

In 22d550749361 (subtree: don't fuss with PATH, 2021-04-27), `git
subtree` was broken thoroughly on Windows.

The reason is that it assumes Unix semantics, where `PATH` is
colon-separated, and it assumes that `$GIT_EXEC_PATH:` is a verbatim
prefix of `$PATH`. Neither are true, the latter in particular because
`GIT_EXEC_PATH` is a Windows-style path, while `PATH` is a Unix-style
path list.

Let's keep the original spirit, and hack together something that
unbreaks the logic on Windows.

A more thorough fix would look at the inode of `$GIT_EXEC_PATH` and of
the first component of `$PATH`, to make sure that they are identical,
but that is even trickier to do in a portable way.

This fixes https://github.com/git-for-windows/git/issues/3260

Signed-off-by: Johannes Schindelin <redacted>
---
 contrib/subtree/git-subtree.sh | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index b06782bc7955..6bd689a6bb92 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh

@@ -5,7 +5,13 @@
 # Copyright (C) 2009 Avery Pennarun <apenwarr@gmail.com>
 #

-if test -z "$GIT_EXEC_PATH" || test "${PATH#"${GIT_EXEC_PATH}:"}" = "$PATH" || ! test -f "$GIT_EXEC_PATH/git-sh-setup"
+if test -z "$GIT_EXEC_PATH" || {
+	test "${PATH#"${GIT_EXEC_PATH}:"}" = "$PATH" && {
+		# On Windows, PATH might be Unix-style, GIT_EXEC_PATH not
+		! type -p cygpath >/dev/null 2>&1 ||
+		test "${PATH#$(cygpath -au "$GIT_EXEC_PATH"):}" = "$PATH"

Nit: That should have a couple more `"` in it:

    test "${PATH#"$(cygpath -au "$GIT_EXEC_PATH"):"}" = "$PATH"

Are you sure about that?

	$ P='*:hello'; echo "${P#$(echo '*'):}"
	hello

As you can see, there is no problem with that `echo '*'` producing a
wildcard character.

In any case, neither '*' nor '?' are valid filename characters on Windows,
therefore there is little danger here.

To be honest, I was looking more for reviews focusing on
potentially-better solutions, such as looking at the inodes, or even
comparing the contents of `$GIT_EXEC_PATH/git-subtree` and
`${PATH%%:*}/git-subtree`, and complaining if they're not identical.

Those two ideas look a bit ham-handed to me, though, the latter because it
reads the file twice, for _every_ `git subtree` invocation, and the fomer
because there simply is no easy portable way to look at the inode of a
file (stat(1) has different semantics depending whether it is the GNU or
the BSD flavor, and it might not even be present to begin with).

I was also looking forward to hear whether there are opinions about maybe
dropping this check altogether because there were indications that this
condition is not even common anymore.

But no need to re-roll for just that.

Do we also need to handle the reverse case, where PATH uses
backslashes but GIT_EXEC_PATH uses forward slashes?

In Git for Windows, we ensure to use forward slashes in `GIT_EXEC_PATH`.

Ciao,
Dscho