patch, 2 partssubtree: fix the GIT_EXEC_PATH sanity check to work on Windows

In the topic branch ls/subtree, we saw a lot of improvements of the git subtree command, and some cleaning up, too. For example, 22d550749361 (subtree: don't fuss with PATH, 2021-04-27) carefully laid out a history of changes intended to work around issues where the git-subtree script was not in the intended location, and removed a statement modifying PATH in favor of a conditional warning (contingent on the PATH being in an unexpected shape).

This particular condition, however, was never tested on Windows, and it broke git subtree in Git for Windows v2.32.0, as reported here [https://github.com/git-for-windows/git/issues/3260]. Now, every invocation of git subtree, with or without command-line arguments, results in output like this:

It looks like either your git installation or your git-subtree installation is broken.

Tips:
 - If `git --exec-path` does not print the correct path to
   your git install directory, then set the GIT_EXEC_PATH
   environment variable to the correct directory.
 - Make sure that your `git-core\git-subtree` file is either in your
   PATH or in your git exec path (`C:/Users/harry/Downloads/PortableGit/mingw64/libexec/git-core`).
 - You should run git-subtree as `git core\git-subtree`,
   not as `git-core\git-subtree`.

This patch series provides a band-aid to that symptom, and is based on ls/subtree.

Johannes Schindelin (2):
  subtree: fix the GIT_EXEC_PATH sanity check to work on Windows
  subtree: fix assumption about the directory separator

 contrib/subtree/git-subtree.sh | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

base-commit: 9a3e3ca2ba869f9fef9f8be390ed45457565ccd1
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-978%2Fdscho%2Ffix-subtree-on-windows-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-978/dscho/fix-subtree-on-windows-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/978

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

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 <johannes.schindelin@gmx.de>
---
 contrib/subtree/git-subtree.sh | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

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

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

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

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?

Luke Shumaker <lukeshu@lukeshu.com> writes:

Does the nit purely cosmetic, or does it affect correctness? I'd assume the former, as it would not make sense to say "no need to reroll" if leaving it as-is would mean a breakage, but trying to make sure.

Thanks.

On Thu, 10 Jun 2021 19:37:12 -0600, Junio C Hamano wrote:

Quoting in that shell construct can in theory affect correctness, but my intuition was that it can't affect this specific case. However, upon thinking about it a bit more, I realize that I was mistaken. If the git exec path contains a shell glob that is not self-matching, then it will break in that `git subtree` will refuse to run even though the install is fine.

For instance,

    GIT_EXEC_PATH => \Home\Tricky[x]User\git-core
    PATH          => /Home/Tricky[x]User/git-core:/bin

I'd think that this type of thing would be unlikely to happen in the wild, but yeah, it needs to be fixed. So a reroll is needed.

It is also broken in the other direction (it might run even though the install is broken), but only in situations that I don't think I actually care about. It would happen if the glob is self-matching, your GIT_EXEC_PATH and PATH disagree, and the glob matches PATH. The point of the check is to look for ways that installs are actually accidentally broken in the wild, I'm pretty sure the only way all 3 of those things can happen together is if you're actively trying to break it. And if you're actively trying to break a shell script, you can do so a lot simpler by just setting PATH to something silly.

Hi Luke,

On Thu, 10 Jun 2021, Luke Shumaker wrote:

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.

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

Ciao, Dscho

On Fri, 11 Jun 2021 04:19:17 -0600, Johannes Schindelin wrote:

In the other email (the reply to Junio), I specified that it's only a problem if the glob isn't self-matching. So * and ? are fine, but [charset] probably isn't.

    $ P='f[o]o:bar'; echo "${P#$(echo 'f[o]o'):}"
    f[o]o:bar

    $ P='f[o]o:bar'; echo "${P#"$(echo 'f[o]o'):"}"
    bar

So the check right now is gross, but I don't know what would be better. The point of the check is more to check "is the environment set up the way that `git` sets it up for us", not so much to actually check the filesystem.

Plus, it shouldn't actually care if it's installed in `$GIT_EXEC_PATH` or not, it should be totally happy for $GIT_EXEC_PATH/git-subtree to not exist and for git-subtree to be elsewhere in the PATH. So an inode or content check would be wrong. Perhaps checking git-sh-setup instead of git-subtree though...

`test FILE1 -ef FILE2` checks wether the inode is the same. And it's POSIX, so I'm assuming that it's sufficiently portable, though I haven't actually tested whether things other than Bash implement it.

I think it would be good for it to eventually go away. But having removed the hacks that allowed it to work in broken setups, I have no way of knowing how many people had setups like that unless they tell me now that it's telling them, and if those users are now broken, I don't want them to be *silently* broken. So I think we do need to have the check for a longish period of time.

Did you mean to write `PATH` here instead of `GIT_EXEC_PATH`? Because if not, then I'm confused.

Hi Luke,

On Fri, 11 Jun 2021, Luke Shumaker wrote:

Thank you for clarifying.

This is actually a valid concern also on Windows because according to https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file the brackets _are_ valid file name characters.

It's not POSIX. From https://pubs.opengroup.org/onlinepubs/009695399/utilities/test.html:

	Some additional primaries newly invented or from the KornShell
	appeared in an early proposal as part of the conditional command
	([[]]): s1 > s2, s1 < s2, str = pattern, str != pattern,
	f1 -nt f2, f1 -ot f2, and f1 -ef f2.

Having said that, it appears that Bash implements it (what non-standard behavior _doesn't_ it implement ;-))

And since Git for Windows ships with Bash, we can actually use it!

Ah, I thought it was clear that the `PATH` variable is already _not_ the standard Windows version (which contains backslashes and _semicolons_), but it is adjusted automatically by the MSYS2 runtime to look more Unix-like (with forward slashes and _colons_).

Ciao, Dscho

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

So,... is contrib/subtree for Windows only?

On Tue, Jun 15, 2021 at 11:33:58AM +0900, Junio C Hamano wrote:

I read it as "this workaround is needed only on Windows, and will kick in only there; on other platforms, the "-ef" code will not run at all, so we don't have to worry about its portability".

But having seen the earlier part of the thread, it looks like "are we on Windows" is predicated on "! type -p cygpath", which seems a bit loose. I also think "-p" is a bash-ism, so we'd want to avoid it before determining whether we're on Windows to avoid a chicken-and-egg on other platforms.

-Peff

Hi Jeff,

What is the POSIX equivalent then of?

On Tue, Jun 15, 2021 at 06:05:08PM +0700, Bagas Sanjaya wrote:

I don't think there is an equivalent for "-p". But regular "type" is probably sufficient for this use (the "-p" is just suppressing aliases and functions).

It would be nice if there was a more robust test in general, though (after all, I could have something called "cygpath" on a non-Windows system). I don't know what options there are to get info from bash, though.

(I'd also clarify that I haven't been carefully following this thread, so take any suggestion or comments from me with a grain of salt. I mostly jumped in because it looked like there was a communication confusion).

-Peff

Hi,

On Tue, 15 Jun 2021, Jeff King wrote:

Please don't worry about `type` vs `type -p` here, as that is no longer used in v2.

Please also don't worry about perceived brittleness of relying on `-ef`: The only thing my patch now does is to _fall back_ after the previously-already-existing test verifying that `$PATH` starts with `$GIT_EXEC_PREFIX:`.

In the worst case, this will simply behave the same. In the best case (which is the case on Windows), it will rely on Git for Windows' Bash to _have_ support for `-ef` and no longer do the wrong thing.

Ciao, Dscho

Jeff King <peff@peff.net> writes:

Yes, in the latest round that I queued yesterday, it is clear that the use of non-POSIX "test" comes after the original condition followed by "||", and even if "test" may sometimes be a builtin, I do not think we will trigger an error at parse-time [*1*], so it is a safe change.

Thanks.

[Footnote]

*1* I recall we had one interesting breakage of a script that tried
    to do what is essentially:

        if are we running a shell with that funky feature?
        then
            shell commands that use the funky feature
        else
            portable POSIX shell feature
        fi

    but the part that were supposed to be excluded from the
    "portable" codepath by being between "then" and "else" still
    were parsed and caused a parse error.

On Wed, Jun 16, 2021 at 09:52:34AM +0900, Junio C Hamano wrote:

Yep, thanks for saying that last part out loud.

IIRC, one instance was trying to redirect descriptors over 9. That works in bash but not elsewhere, but we ran into shells that choke on the parsing.

So yeah, that is an important distinction in any portability workarounds. Sometimes an extra layer of "eval" can get around it, though.

-Peff

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

On Windows, both forward and backslash are valid separators. In 22d550749361 (subtree: don't fuss with PATH, 2021-04-27), however, we added code that assumes that it can only be the forward slash.

Let's fix that.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 contrib/subtree/git-subtree.sh | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

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

This might be more clearly written as

    base=${0##*[/\\]}

?

(And then maybe it doesn't need a separate variable, and can still be written in-line?)

Hi Luke,

On Thu, 10 Jun 2021, Luke Shumaker wrote:

True. For some reason, this did not work in my tests (probably because I confused `#` with `%` or put the wildcard on the wrong side, or something).

Will fix.

I am not in favor of inlining here. Three times. It is too complicated a construct for a casual contributor. At least giving it a name (where "basename" would make even more sense than "base", I just realized) alleviates this problem _somewhat_.

Ciao, Dscho

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

I was going to say that I do have CI set up[0] to run the subtree tests on Windows, and was going to ask for assistance in figuring out how to set up CI's Windows to match the behavior that you're seeing with Cygwin.

However, upon closer inspection: Because of how run-test-slice.sh works, it wasn't actually running the subtree test on Windows. Oops.

Now, that CI setup was just on my personal copies and hadn't been submitted upstream, since I wasn't sure if it was welcome[1]. I never really got a reply to that part, but but now that we've got buy-in from Ævar for running the tests in contrib/,[2] I might spend some more time on it and try to get that submitted. That is, if Felipe doesn't beat me to it[3].

[0]: https://github.com/LukeShu/git/commit/359c71be69749621125181b9d8c109baba6bf745 [1]: https://lore.kernel.org/git/20210426174525.3937858-1-lukeshu@lukeshu.com/ [2]: https://lore.kernel.org/git/87o8d0o2or.fsf@evledraar.gmail.com/ [3]: https://lore.kernel.org/git/60abe0b32dfa1_1b2092081d@natae.notmuch/#t

Hi Luke,

On Thu, 10 Jun 2021, Luke Shumaker wrote:

Right, I am sorry about that, it did bite me recently, too.

I am not so sure about running tests in contrib/ as part of the CI. Those bits and pieces are in contrib/ to indicate that they should _not_ be considered as adding to Junio's maintenance burden, and adding them to CI would contradict that.

Having said that, in _Git for Windows_, we do bundle `git subtree`. Therefore, I would be interested in running its tests as part of CI because it is part of _my_ maintenance burden.

As to your 30-strong patch series: Sorry, I really cannot afford even looking at such a large patch series. There are only 24h in a day, I need to sleep from time to time, and there is a pandemic raging on, and the Git mailing list's current atmosphere requires more self-care time from me that I need to spend elsewhere.

But _if_ I had time to even look at it, I would suggest to first spend time to turn it into a full-fledged built-in in Git. I.e. move it from contrib/ to builtin/, rewriting it in C in the process.

Please take that suggestion with a large rock of salt, though: as I said, I don't have time to assist in that endeavor, unfortunately not even as a reviewer.

Ciao, Dscho

On Fri, 11 Jun 2021 04:30:21 -0600, Johannes Schindelin wrote:

That's definitely the direction I want to go, yes.

Johannes Schindelin wrote:

But we already do run contrib tests:

  t1021-rerere-in-workdir.sh
  t3000-ls-files-others.sh
  t9902-completion.sh
  t9903-bash-prompt.sh

People rely on contrib and distributions build packages enabling what is in contrib.

Part of a successful release is contrib not being broken, so Junio can't just ignore the status of contrib.

I agree this is not goood, but that's the current situation we are in, and that's because we have allowed perfectly widely used and stable software on the same cohort as unmaintained waste nobody cares about.

On Fri, 11 Jun 2021 09:50:18 -0600, Felipe Contreras wrote:

Lovely, I didn't realize that it would be OK to move t7900-subtree.sh out of contrib/ without moving all of subtree out of contrib/. That'll make things easier, I think.

In the topic branch ls/subtree, we saw a lot of improvements of the git subtree command, and some cleaning up, too. For example, 22d550749361 (subtree: don't fuss with PATH, 2021-04-27) carefully laid out a history of changes intended to work around issues where the git-subtree script was not in the intended location, and removed a statement modifying PATH in favor of a conditional warning (contingent on the PATH being in an unexpected shape).

This particular condition, however, was never tested on Windows, and it broke git subtree in Git for Windows v2.32.0, as reported here [https://github.com/git-for-windows/git/issues/3260]. Now, every invocation of git subtree, with or without command-line arguments, results in output like this:

It looks like either your git installation or your git-subtree installation is broken.

Tips:
 - If `git --exec-path` does not print the correct path to
   your git install directory, then set the GIT_EXEC_PATH
   environment variable to the correct directory.
 - Make sure that your `git-core\git-subtree` file is either in your
   PATH or in your git exec path (`C:/Users/harry/Downloads/PortableGit/mingw64/libexec/git-core`).
 - You should run git-subtree as `git core\git-subtree`,
   not as `git-core\git-subtree`.

This patch series provides a fix for that symptom, and is based on ls/subtree.

Changes since v1:

 * Instead of using the Windows-specific cygpath construct, we now instead
   fall back to verify that GIT_EXEC_PATH and the first component of PATH
   refer to the same entity (via the -ef operator, which compares inodes).
   Since the bug affects only Windows, as far as we know, the
   non-portability of the -ef operator does not matter because Git for
   Windows' Bash does have support for it.

Johannes Schindelin (2):
  subtree: fix the GIT_EXEC_PATH sanity check to work on Windows
  subtree: fix assumption about the directory separator

 contrib/subtree/git-subtree.sh | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

base-commit: 9a3e3ca2ba869f9fef9f8be390ed45457565ccd1
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-978%2Fdscho%2Ffix-subtree-on-windows-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-978/dscho/fix-subtree-on-windows-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/978

Range-diff vs v1:

 1:  a91ac6c18938 ! 1:  5f2d9434b4eb subtree: fix the GIT_EXEC_PATH sanity check to work on Windows
     @@ Commit message
          `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.
     +    Let's make extra certain that `$GIT_EXEC_PATH` and the first component
     +    of `$PATH` refer to different entities before erroring out.
      
     -    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.
     +    We do that by using the `test <path1> -ef <path2>` command that verifies
     +    that the inode of `<path1>` and of `<path2>` is the same.
     +
     +    Sadly, this construct is non-portable, according to
     +    https://pubs.opengroup.org/onlinepubs/009695399/utilities/test.html.
     +    However, it does not matter in practice because we still first look
     +    whether `$GIT_EXEC_PREFIX` is string-identical to the first component of
     +    `$PATH`. This will give us the expected result everywhere but in Git for
     +    Windows, and Git for Windows' own Bash _does_ handle the `-ef` operator.
     +
     +    Just in case that we _do_ need to show the error message _and_ are
     +    running in a shell that lacks support for `-ef`, we simply suppress the
     +    error output for that part.
      
          This fixes https://github.com/git-for-windows/git/issues/3260
      
     @@ contrib/subtree/git-subtree.sh
       #
       
      -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"
     -+	}
     -+} || ! test -f "$GIT_EXEC_PATH/git-sh-setup"
     ++if test -z "$GIT_EXEC_PATH" || ! test -f "$GIT_EXEC_PATH/git-sh-setup" || {
     ++	test "${PATH#"${GIT_EXEC_PATH}:"}" = "$PATH" &&
     ++	test ! "$GIT_EXEC_PATH" -ef "${PATH%%:*}" 2>/dev/null
     ++}
       then
       	echo >&2 'It looks like either your git installation or your'
       	echo >&2 'git-subtree installation is broken.'
 2:  4e1a569c9fa4 ! 2:  a6f7aa40485f subtree: fix assumption about the directory separator
     @@ Commit message
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
       ## contrib/subtree/git-subtree.sh ##
     -@@ contrib/subtree/git-subtree.sh: if test -z "$GIT_EXEC_PATH" || {
     - 	}
     - } || ! test -f "$GIT_EXEC_PATH/git-sh-setup"
     +@@ contrib/subtree/git-subtree.sh: if test -z "$GIT_EXEC_PATH" || ! test -f "$GIT_EXEC_PATH/git-sh-setup" || {
     + 	test ! "$GIT_EXEC_PATH" -ef "${PATH%%:*}" 2>/dev/null
     + }
       then
     -+	base=${0##*/}
     -+	base=${base##*\\}
     ++	basename=${0##*[/\\]}
       	echo >&2 'It looks like either your git installation or your'
       	echo >&2 'git-subtree installation is broken.'
       	echo >&2
     @@ contrib/subtree/git-subtree.sh: then
       	echo >&2 "   your git install directory, then set the GIT_EXEC_PATH"
       	echo >&2 "   environment variable to the correct directory."
      -	echo >&2 " - Make sure that your \`${0##*/}\` file is either in your"
     -+	echo >&2 " - Make sure that your \`$base\` file is either in your"
     ++	echo >&2 " - Make sure that your \`$basename\` file is either in your"
       	echo >&2 "   PATH or in your git exec path (\`$(git --exec-path)\`)."
      -	echo >&2 " - You should run git-subtree as \`git ${0##*/git-}\`,"
      -	echo >&2 "   not as \`${0##*/}\`." >&2
     -+	echo >&2 " - You should run git-subtree as \`git ${base#git-}\`,"
     -+	echo >&2 "   not as \`$base\`." >&2
     ++	echo >&2 " - You should run git-subtree as \`git ${basename#git-}\`,"
     ++	echo >&2 "   not as \`$basename\`." >&2
       	exit 126
       fi
       

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

On Windows, both forward and backslash are valid separators. In 22d550749361 (subtree: don't fuss with PATH, 2021-04-27), however, we added code that assumes that it can only be the forward slash.

Let's fix that.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 contrib/subtree/git-subtree.sh | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

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

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 make extra certain that `$GIT_EXEC_PATH` and the first component of `$PATH` refer to different entities before erroring out.

We do that by using the `test <path1> -ef <path2>` command that verifies that the inode of `<path1>` and of `<path2>` is the same.

Sadly, this construct is non-portable, according to https://pubs.opengroup.org/onlinepubs/009695399/utilities/test.html. However, it does not matter in practice because we still first look whether `$GIT_EXEC_PREFIX` is string-identical to the first component of `$PATH`. This will give us the expected result everywhere but in Git for Windows, and Git for Windows' own Bash _does_ handle the `-ef` operator.

Just in case that we _do_ need to show the error message _and_ are running in a shell that lacks support for `-ef`, we simply suppress the error output for that part.

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

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 contrib/subtree/git-subtree.sh | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

OK. That's somewhat subtle and possibly brittle, but let's see how it goes.

Will queue.

Thanks.