[PATCH 0/2] Fix git subtree on Windows

[PATCH 0/2] Fix git subtree on Windows

[Johannes Schindelin via GitGitGadget]

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

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

[Johannes Schindelin via GitGitGadget]

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(-)

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"
+	}
+} || ! test -f "$GIT_EXEC_PATH/git-sh-setup"
 then
 	echo >&2 'It looks like either your git installation or your'
 	echo >&2 'git-subtree installation is broken.'
-- 
gitgitgadget

[PATCH 2/2] subtree: fix assumption about the directory separator

[Johannes Schindelin via GitGitGadget]

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(-)

diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index 6bd689a6bb92..d11ac56f9eb8 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -13,6 +13,8 @@ if test -z "$GIT_EXEC_PATH" || {
 	}
 } || ! test -f "$GIT_EXEC_PATH/git-sh-setup"
 then
+	base=${0##*/}
+	base=${base##*\\}
 	echo >&2 'It looks like either your git installation or your'
 	echo >&2 'git-subtree installation is broken.'
 	echo >&2
@@ -20,10 +22,10 @@ then
 	echo >&2 " - If \`git --exec-path\` does not print the correct path to"
 	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 "   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
 	exit 126
 fi
 
-- 
gitgitgadget

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

[Luke Shumaker]

On Thu, 10 Jun 2021 03:13:30 -0600,
Johannes Schindelin via GitGitGadget wrote:
> 
> 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(-)
> 
> 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"

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?

-- 
Happy hacking,
~ Luke Shumaker

Re: [PATCH 0/2] Fix git subtree on Windows

[Luke Shumaker]

On Thu, 10 Jun 2021 03:13:29 -0600,
Johannes Schindelin via GitGitGadget wrote:
> This particular condition, however, was never tested on Windows,

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

-- 
Happy hacking,
~ Luke Shumaker

Re: [PATCH 2/2] subtree: fix assumption about the directory separator

[Luke Shumaker]

On Thu, 10 Jun 2021 03:13:31 -0600,
Johannes Schindelin via GitGitGadget wrote:
> 
> 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(-)
> 
> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
> index 6bd689a6bb92..d11ac56f9eb8 100755
> --- a/contrib/subtree/git-subtree.sh
> +++ b/contrib/subtree/git-subtree.sh
> @@ -13,6 +13,8 @@ if test -z "$GIT_EXEC_PATH" || {
>  	}
>  } || ! test -f "$GIT_EXEC_PATH/git-sh-setup"
>  then
> +	base=${0##*/}
> +	base=${base##*\\}

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?)

-- 
Happy hacking,
~ Luke Shumaker

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

[Junio C Hamano]

Luke Shumaker <lukeshu@lukeshu.com> writes:

>> +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"
>
> But no need to re-roll for just that.

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.

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

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

[Luke Shumaker]

On Thu, 10 Jun 2021 19:37:12 -0600,
Junio C Hamano wrote:
> Luke Shumaker <lukeshu@lukeshu.com> writes:
> 
> >> +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"
> >
> > But no need to re-roll for just that.
> 
> 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.

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.

-- 
Happy hacking,
~ Luke Shumaker

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

[Johannes Schindelin]

Hi Luke,

On Thu, 10 Jun 2021, Luke Shumaker wrote:

> On Thu, 10 Jun 2021 03:13:30 -0600,
> Johannes Schindelin via GitGitGadget wrote:
> >
> > 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(-)
> >
> > 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

Re: [PATCH 0/2] Fix git subtree on Windows

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 2430 bytes --]

Hi Luke,

On Thu, 10 Jun 2021, Luke Shumaker wrote:

> On Thu, 10 Jun 2021 03:13:29 -0600,
> Johannes Schindelin via GitGitGadget wrote:
> > This particular condition, however, was never tested on Windows,
>
> 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.

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

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

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

>
> [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/
>
> --
> Happy hacking,
> ~ Luke Shumaker
>

Re: [PATCH 2/2] subtree: fix assumption about the directory separator

[Johannes Schindelin]

Hi Luke,

On Thu, 10 Jun 2021, Luke Shumaker wrote:

> On Thu, 10 Jun 2021 03:13:31 -0600,
> Johannes Schindelin via GitGitGadget wrote:
> >
> > 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(-)
> >
> > diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
> > index 6bd689a6bb92..d11ac56f9eb8 100755
> > --- a/contrib/subtree/git-subtree.sh
> > +++ b/contrib/subtree/git-subtree.sh
> > @@ -13,6 +13,8 @@ if test -z "$GIT_EXEC_PATH" || {
> >  	}
> >  } || ! test -f "$GIT_EXEC_PATH/git-sh-setup"
> >  then
> > +	base=${0##*/}
> > +	base=${base##*\\}
>
> This might be more clearly written as
>
>     base=${0##*[/\\]}
>
> ?

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.

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

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

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

[Luke Shumaker]

On Fri, 11 Jun 2021 04:19:17 -0600,
Johannes Schindelin wrote:
> 
> Hi Luke,
> 
> On Thu, 10 Jun 2021, Luke Shumaker wrote:
> 
> > On Thu, 10 Jun 2021 03:13:30 -0600,
> > Johannes Schindelin via GitGitGadget wrote:
> > > -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.

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

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

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

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

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

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.

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

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

-- 
Happy hacking,
~ Luke Shumaker

Re: [PATCH 0/2] Fix git subtree on Windows

[Luke Shumaker]

On Fri, 11 Jun 2021 04:30:21 -0600,
Johannes Schindelin wrote:
> 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.

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

-- 
Happy hacking,
~ Luke Shumaker

Re: [PATCH 0/2] Fix git subtree on Windows

[Felipe Contreras]

Johannes Schindelin wrote:
> On Thu, 10 Jun 2021, Luke Shumaker wrote:
> 
> > On Thu, 10 Jun 2021 03:13:29 -0600,
> > Johannes Schindelin via GitGitGadget wrote:
> > > This particular condition, however, was never tested on Windows,
> >
> > 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.
> 
> Right, I am sorry about that, it did bite me recently, too.
> 
> > 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.
> 
> I am not so sure about running tests in contrib/ as part of the CI.

But we already do run contrib tests:

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

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

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.

-- 
Felipe Contreras

Re: [PATCH 0/2] Fix git subtree on Windows

[Luke Shumaker]

On Fri, 11 Jun 2021 09:50:18 -0600,
Felipe Contreras wrote:
> Johannes Schindelin wrote:
> > On Thu, 10 Jun 2021, Luke Shumaker wrote:
> > > 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.
> > 
> > I am not so sure about running tests in contrib/ as part of the CI.
> 
> But we already do run contrib tests:
> 
>   t1021-rerere-in-workdir.sh
>   t3000-ls-files-others.sh
>   t9902-completion.sh
>   t9903-bash-prompt.sh

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.

-- 
Happy hacking,
~ Luke Shumaker

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

[Johannes Schindelin]

Hi Luke,

On Fri, 11 Jun 2021, Luke Shumaker wrote:

> On Fri, 11 Jun 2021 04:19:17 -0600,
> Johannes Schindelin wrote:
> >
> > On Thu, 10 Jun 2021, Luke Shumaker wrote:
> >
> > > On Thu, 10 Jun 2021 03:13:30 -0600,
> > > Johannes Schindelin via GitGitGadget wrote:
> > > > -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.
>
> 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

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.

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

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!

> > 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.
>
> 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.
>
> > > 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`.
>
> Did you mean to write `PATH` here instead of `GIT_EXEC_PATH`?  Because
> if not, then I'm confused.

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

[PATCH v2 0/2] Fix git subtree on Windows

[Johannes Schindelin via GitGitGadget]

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
       

-- 
gitgitgadget

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

[Johannes Schindelin via GitGitGadget]

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(-)

diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index b06782bc7955..3935cea7dd13 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -5,7 +5,10 @@
 # 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 -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.'
-- 
gitgitgadget

[PATCH v2 2/2] subtree: fix assumption about the directory separator

[Johannes Schindelin via GitGitGadget]

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(-)

diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index 3935cea7dd13..7f767b5c38fe 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -10,6 +10,7 @@ if test -z "$GIT_EXEC_PATH" || ! test -f "$GIT_EXEC_PATH/git-sh-setup" || {
 	test ! "$GIT_EXEC_PATH" -ef "${PATH%%:*}" 2>/dev/null
 }
 then
+	basename=${0##*[/\\]}
 	echo >&2 'It looks like either your git installation or your'
 	echo >&2 'git-subtree installation is broken.'
 	echo >&2
@@ -17,10 +18,10 @@ then
 	echo >&2 " - If \`git --exec-path\` does not print the correct path to"
 	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 \`$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 ${basename#git-}\`,"
+	echo >&2 "   not as \`$basename\`." >&2
 	exit 126
 fi
 
-- 
gitgitgadget

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

[Junio C Hamano]

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

>> `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.
>
> 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!

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

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

[Junio C Hamano]

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

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

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

Will queue.

Thanks.

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

[Jeff King]

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

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> `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.
> >
> > 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!
> 
> So,... is contrib/subtree for Windows only?

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

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

[Bagas Sanjaya]

Hi Jeff,

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

What is the POSIX equivalent then of?

-- 
An old man doll... just what I always wanted! - Clara

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

[Jeff King]

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

> Hi Jeff,
> 
> > 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
> > 
> 
> What is the POSIX equivalent then of?

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

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

[Johannes Schindelin]

Hi,

On Tue, 15 Jun 2021, Jeff King wrote:

> On Tue, Jun 15, 2021 at 06:05:08PM +0700, Bagas Sanjaya wrote:
>
> > > 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
> > >
> >
> > What is the POSIX equivalent then of?
>
> 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).

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> So,... is contrib/subtree for Windows only?
>
> 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".

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.

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

[Jeff King]

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

> Jeff King <peff@peff.net> writes:
> 
> >> So,... is contrib/subtree for Windows only?
> >
> > 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".
> 
> 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.

Yep, thanks for saying that last part out loud.

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

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