From: Johannes Schindelin via GitGitGadget <hidden> Date: 2021-06-10 09:14:45
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
From: Johannes Schindelin via GitGitGadget <hidden> Date: 2021-06-10 09:13:37
From: Johannes Schindelin <redacted>
In 22d550749361 (subtree: don't fuss with PATH, 2021-04-27), `git
subtree` was broken thoroughly on Windows.
The reason is that it assumes Unix semantics, where `PATH` is
colon-separated, and it assumes that `$GIT_EXEC_PATH:` is a verbatim
prefix of `$PATH`. Neither are true, the latter in particular because
`GIT_EXEC_PATH` is a Windows-style path, while `PATH` is a Unix-style
path list.
Let's keep the original spirit, and hack together something that
unbreaks the logic on Windows.
A more thorough fix would look at the inode of `$GIT_EXEC_PATH` and of
the first component of `$PATH`, to make sure that they are identical,
but that is even trickier to do in a portable way.
This fixes https://github.com/git-for-windows/git/issues/3260
Signed-off-by: Johannes Schindelin <redacted>
---
contrib/subtree/git-subtree.sh | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
@@ -5,7 +5,13 @@# Copyright (C) 2009 Avery Pennarun <apenwarr@gmail.com>#-iftest-z"$GIT_EXEC_PATH"||test"${PATH#"${GIT_EXEC_PATH}:"}"="$PATH"||!test-f"$GIT_EXEC_PATH/git-sh-setup"+iftest-z"$GIT_EXEC_PATH"||{+test"${PATH#"${GIT_EXEC_PATH}:"}"="$PATH"&&{+# On Windows, PATH might be Unix-style, GIT_EXEC_PATH not+!type-pcygpath>/dev/null2>&1||+test"${PATH#$(cygpath-au"$GIT_EXEC_PATH"):}"="$PATH"+}+}||!test-f"$GIT_EXEC_PATH/git-sh-setup"thenecho>&2'It looks like either your git installation or your'echo>&2'git-subtree installation is broken.'
From: Johannes Schindelin via GitGitGadget <hidden> Date: 2021-06-10 09:13:37
From: Johannes Schindelin <redacted>
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 <redacted>
---
contrib/subtree/git-subtree.sh | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
@@ -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 @@ thenecho>&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\`.">&2exit126fi
From: Luke Shumaker <hidden> Date: 2021-06-11 00:50:05
On Thu, 10 Jun 2021 03:13:30 -0600,
Johannes Schindelin via GitGitGadget wrote:
quoted hunk
From: Johannes Schindelin <redacted>
In 22d550749361 (subtree: don't fuss with PATH, 2021-04-27), `git
subtree` was broken thoroughly on Windows.
The reason is that it assumes Unix semantics, where `PATH` is
colon-separated, and it assumes that `$GIT_EXEC_PATH:` is a verbatim
prefix of `$PATH`. Neither are true, the latter in particular because
`GIT_EXEC_PATH` is a Windows-style path, while `PATH` is a Unix-style
path list.
Let's keep the original spirit, and hack together something that
unbreaks the logic on Windows.
A more thorough fix would look at the inode of `$GIT_EXEC_PATH` and of
the first component of `$PATH`, to make sure that they are identical,
but that is even trickier to do in a portable way.
This fixes https://github.com/git-for-windows/git/issues/3260
Signed-off-by: Johannes Schindelin <redacted>
---
contrib/subtree/git-subtree.sh | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
@@ -5,7 +5,13 @@# Copyright (C) 2009 Avery Pennarun <apenwarr@gmail.com>#-iftest-z"$GIT_EXEC_PATH"||test"${PATH#"${GIT_EXEC_PATH}:"}"="$PATH"||!test-f"$GIT_EXEC_PATH/git-sh-setup"+iftest-z"$GIT_EXEC_PATH"||{+test"${PATH#"${GIT_EXEC_PATH}:"}"="$PATH"&&{+# On Windows, PATH might be Unix-style, GIT_EXEC_PATH not+!type-pcygpath>/dev/null2>&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
From: Luke Shumaker <hidden> Date: 2021-06-11 01:02:18
On Thu, 10 Jun 2021 03:13:31 -0600,
Johannes Schindelin via GitGitGadget wrote:
quoted hunk
From: Johannes Schindelin <redacted>
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 <redacted>
---
contrib/subtree/git-subtree.sh | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
@@ -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
From: Johannes Schindelin <hidden> Date: 2021-06-11 10:19:33
Hi Luke,
On Thu, 10 Jun 2021, Luke Shumaker wrote:
On Thu, 10 Jun 2021 03:13:30 -0600,
Johannes Schindelin via GitGitGadget wrote:
quoted
From: Johannes Schindelin <redacted>
In 22d550749361 (subtree: don't fuss with PATH, 2021-04-27), `git
subtree` was broken thoroughly on Windows.
The reason is that it assumes Unix semantics, where `PATH` is
colon-separated, and it assumes that `$GIT_EXEC_PATH:` is a verbatim
prefix of `$PATH`. Neither are true, the latter in particular because
`GIT_EXEC_PATH` is a Windows-style path, while `PATH` is a Unix-style
path list.
Let's keep the original spirit, and hack together something that
unbreaks the logic on Windows.
A more thorough fix would look at the inode of `$GIT_EXEC_PATH` and of
the first component of `$PATH`, to make sure that they are identical,
but that is even trickier to do in a portable way.
This fixes https://github.com/git-for-windows/git/issues/3260
Signed-off-by: Johannes Schindelin <redacted>
---
contrib/subtree/git-subtree.sh | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
@@ -5,7 +5,13 @@# Copyright (C) 2009 Avery Pennarun <apenwarr@gmail.com>#-iftest-z"$GIT_EXEC_PATH"||test"${PATH#"${GIT_EXEC_PATH}:"}"="$PATH"||!test-f"$GIT_EXEC_PATH/git-sh-setup"+iftest-z"$GIT_EXEC_PATH"||{+test"${PATH#"${GIT_EXEC_PATH}:"}"="$PATH"&&{+# On Windows, PATH might be Unix-style, GIT_EXEC_PATH not+!type-pcygpath>/dev/null2>&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
From: Johannes Schindelin <hidden> Date: 2021-06-11 10:30:37
Hi Luke,
On Thu, 10 Jun 2021, Luke Shumaker wrote:
On Thu, 10 Jun 2021 03:13:29 -0600,
Johannes Schindelin via GitGitGadget wrote:
quoted
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
From: Johannes Schindelin <hidden> Date: 2021-06-11 10:36:05
Hi Luke,
On Thu, 10 Jun 2021, Luke Shumaker wrote:
On Thu, 10 Jun 2021 03:13:31 -0600,
Johannes Schindelin via GitGitGadget wrote:
quoted
From: Johannes Schindelin <redacted>
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 <redacted>
---
contrib/subtree/git-subtree.sh | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
@@ -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
From: Luke Shumaker <hidden> Date: 2021-06-11 13:41:41
On Fri, 11 Jun 2021 04:19:17 -0600,
Johannes Schindelin wrote:
Hi Luke,
On Thu, 10 Jun 2021, Luke Shumaker wrote:
quoted
On Thu, 10 Jun 2021 03:13:30 -0600,
Johannes Schindelin via GitGitGadget wrote:
quoted
-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.
quoted
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
From: Luke Shumaker <hidden> Date: 2021-06-11 13:46:07
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
From: Felipe Contreras <hidden> Date: 2021-06-11 15:50:27
Johannes Schindelin wrote:
On Thu, 10 Jun 2021, Luke Shumaker wrote:
quoted
On Thu, 10 Jun 2021 03:13:29 -0600,
Johannes Schindelin via GitGitGadget wrote:
quoted
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.
quoted
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
From: Luke Shumaker <hidden> Date: 2021-06-12 02:58:27
On Fri, 11 Jun 2021 09:50:18 -0600,
Felipe Contreras wrote:
Johannes Schindelin wrote:
quoted
On Thu, 10 Jun 2021, Luke Shumaker wrote:
quoted
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
From: Johannes Schindelin <hidden> Date: 2021-06-14 11:56:25
Hi Luke,
On Fri, 11 Jun 2021, Luke Shumaker wrote:
On Fri, 11 Jun 2021 04:19:17 -0600,
Johannes Schindelin wrote:
quoted
On Thu, 10 Jun 2021, Luke Shumaker wrote:
quoted
On Thu, 10 Jun 2021 03:13:30 -0600,
Johannes Schindelin via GitGitGadget wrote:
quoted
-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...
quoted
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!
quoted
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.
quoted
quoted
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
From: Johannes Schindelin via GitGitGadget <hidden> Date: 2021-06-14 12:42:12
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 [off-list ref]
## 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
From: Johannes Schindelin via GitGitGadget <hidden> Date: 2021-06-14 12:42:58
From: Johannes Schindelin <redacted>
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 <redacted>
---
contrib/subtree/git-subtree.sh | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
@@ -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 @@ thenecho>&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\`.">&2exit126fi
From: Johannes Schindelin via GitGitGadget <hidden> Date: 2021-06-14 12:43:14
From: Johannes Schindelin <redacted>
In 22d550749361 (subtree: don't fuss with PATH, 2021-04-27), `git
subtree` was broken thoroughly on Windows.
The reason is that it assumes Unix semantics, where `PATH` is
colon-separated, and it assumes that `$GIT_EXEC_PATH:` is a verbatim
prefix of `$PATH`. Neither are true, the latter in particular because
`GIT_EXEC_PATH` is a Windows-style path, while `PATH` is a Unix-style
path list.
Let's 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 <redacted>
---
contrib/subtree/git-subtree.sh | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
@@ -5,7 +5,10 @@# Copyright (C) 2009 Avery Pennarun <apenwarr@gmail.com>#-iftest-z"$GIT_EXEC_PATH"||test"${PATH#"${GIT_EXEC_PATH}:"}"="$PATH"||!test-f"$GIT_EXEC_PATH/git-sh-setup"+iftest-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+}thenecho>&2'It looks like either your git installation or your'echo>&2'git-subtree installation is broken.'