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

Commit Message

Johannes Schindelin June 10, 2021, 9:13 a.m. UTC

From: Johannes Schindelin <[e-mail]>

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 <[e-mail]>
---
 contrib/subtree/git-subtree.sh | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Luke Shumaker June 11, 2021, 12:40 a.m. UTC | #1

On Thu, 10 Jun 2021 03:13:30 -0600,
Johannes Schindelin via GitGitGadget wrote:
> 
> From: Johannes Schindelin <[e-mail]>
> 
> 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 <[e-mail]>
> ---
>  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 <[e-mail]>
>  #
>  
> -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?

Junio C Hamano June 11, 2021, 1:37 a.m. UTC | #2

Luke Shumaker <[e-mail]> 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?

Luke Shumaker June 11, 2021, 4:31 a.m. UTC | #3

On Thu, 10 Jun 2021 19:37:12 -0600,
Junio C Hamano wrote:
> Luke Shumaker <[e-mail]> 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.

Johannes Schindelin June 11, 2021, 10:19 a.m. UTC | #4

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 <[e-mail]>
> >
> > 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 <[e-mail]>
> > ---
> >  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 <[e-mail]>
> >  #
> >
> > -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

Luke Shumaker June 11, 2021, 1:41 p.m. UTC | #5

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.

Johannes Schindelin June 14, 2021, 11:56 a.m. UTC | #6

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

Junio C Hamano June 15, 2021, 2:33 a.m. UTC | #7

Johannes Schindelin <[e-mail]> 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?

Jeff King June 15, 2021, 10:56 a.m. UTC | #8

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

> Johannes Schindelin <[e-mail]> 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

Bagas Sanjaya June 15, 2021, 11:05 a.m. UTC | #9

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?

Jeff King June 15, 2021, 11:18 a.m. UTC | #10

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

Johannes Schindelin June 15, 2021, 11:27 a.m. UTC | #11

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

Junio C Hamano June 16, 2021, 12:52 a.m. UTC | #12

Jeff King <[e-mail]> 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.

Jeff King June 16, 2021, 7:49 a.m. UTC | #13

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

> Jeff King <[e-mail]> 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

Patch

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 <[e-mail]>
 #
 
-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.'