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@xxxxxx> > > > > 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@xxxxxx> > > --- > > 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@xxxxxxxxx> > > # > > > > -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
