git tries to stat //HEAD when searching for a repo, leading to huge delays on Cygwin
Hi,
I have a workaround for this, but someone on stack overflow [1] suggested reporting it upstream, so here you go:
I have a fancy shell prompt that executes "git rev-parse --is-inside-work-tree" to determine whether we're currently inside a working directory. This causes git to walk up the directory hierarchy looking for a containing git repo. For example, when invoked from my home directory, it stats the following paths, in order:
/home/me/.git/home/me/.git/HEAD/home/me/HEAD
/home/home/.git/home/.git/HEAD/home/HEAD
/
/.git
/.git/HEAD
//HEAD
The last name (//HEAD) interacts badly with Cygwin, which interprets it as a UNC file share, and so demand-loads a bunch of extra DLLs and attempts to resolve/contact the server named HEAD. This obviously doesn't work too well, especially over a slow network link.
I've tested with the latest Cygwin git (2.15.0); this was also present in a prior version.
Cheers,
Andrew
Quoted 21 lines · Andrew Baumann
I have a workaround for this, but someone on stack overflow [1] suggested reporting it upstream, so here you go:
I have a fancy shell prompt that executes "git rev-parse --is-inside-work-tree" to determine whether we're currently inside a working directory. This causes git to walk up the directory hierarchy looking for a containing git repo. For example, when invoked from my home directory, it stats the following paths, in order:
/home/me/.git/home/me/.git/HEAD/home/me/HEAD
/home/home/.git/home/.git/HEAD/home/HEAD
/
/.git
/.git/HEAD
//HEAD
Quoted 7 lines
The last name (//HEAD) interacts badly with Cygwin, which interprets it as a UNC file share, and so demand-loads a bunch of extra DLLs and attempts to resolve/contact the server named HEAD. This obviously doesn't work too well, especially over a slow network link.
I've tested with the latest Cygwin git (2.15.0); this was also present in a prior version.
Interesting. I can reproduce on Linux (but of course "//HEAD" is cheap to look at there). It bisects to ce9b8aab5d (setup_git_directory_1():
avoid changing global state, 2017-03-13). Before that, the end of the strace for "git rev-parse --git-dir" looks like:
chdir("..") = 0
stat(".git", 0x7fffba398e00) = -1 ENOENT (No such file or directory)
lstat(".git/HEAD", 0x7fffba398dd0) = -1 ENOENT (No such file or directory)
lstat("./HEAD", 0x7fffba398dd0) = -1 ENOENT (No such file or directory)write(2, "fatal: Not a git repository (or "..., 69) = 69
and after:
stat("/.git", 0x7ffdb28b7eb0) = -1 ENOENT (No such file or directory)
lstat("/.git/HEAD", 0x7ffdb28b7e80) = -1 ENOENT (No such file or directory)
lstat("//HEAD", 0x7ffdb28b7e80) = -1 ENOENT (No such file or directory)write(2, "fatal: Not a git repository (or "..., 69) = 69
Switching to using absolute paths rather than chdir-ing around is intentional for that commit, but it looks like we just need to special-case the construction of the root path.
Like this, perhaps:
setup.c +3 -1
@@ -283,7 +283,9 @@ int is_git_directory(const char *suspect)size_t len;/* Check worktree-related signatures */ -strbuf_addf (&path,"%s/HEAD" , suspect); +strbuf_addstr (&path, suspect); +strbuf_complete (&path,'/' ); +strbuf_addstr (&path,"HEAD" );if (validate_headref (path.buf)) goto done;
-Peff
Johannes Schindelin
Acked-byHi Peff,
Quoted 60 lines · Jeff King
Quoted 21 lines · Andrew Baumann
I have a workaround for this, but someone on stack overflow [1] suggested reporting it upstream, so here you go:
I have a fancy shell prompt that executes "git rev-parse --is-inside-work-tree" to determine whether we're currently inside a working directory. This causes git to walk up the directory hierarchy looking for a containing git repo. For example, when invoked from my home directory, it stats the following paths, in order:
/home/me/.git/home/me/.git/HEAD/home/me/HEAD
/home/home/.git/home/.git/HEAD/home/HEAD
/
/.git
/.git/HEAD
//HEADQuoted 7 lines
The last name (//HEAD) interacts badly with Cygwin, which interprets it as a UNC file share, and so demand-loads a bunch of extra DLLs and attempts to resolve/contact the server named HEAD. This obviously doesn't work too well, especially over a slow network link.
I've tested with the latest Cygwin git (2.15.0); this was also present in a prior version.
Interesting. I can reproduce on Linux (but of course "//HEAD" is cheap to look at there). It bisects to
ce9b8aab5d(setup_git_directory_1():
avoid changing global state, 2017-03-13). Before that, the end of the strace for "git rev-parse --git-dir" looks like:chdir("..") = 0 stat(".git", 0x7fffba398e00) = -1 ENOENT (No such file or directory) lstat(".git/HEAD", 0x7fffba398dd0) = -1 ENOENT (No such file or directory) lstat("./HEAD", 0x7fffba398dd0) = -1 ENOENT (No such file or directory)write(2, "fatal: Not a git repository (or "..., 69) = 69
and after:
stat("/.git", 0x7ffdb28b7eb0) = -1 ENOENT (No such file or directory) lstat("/.git/HEAD", 0x7ffdb28b7e80) = -1 ENOENT (No such file or directory) lstat("//HEAD", 0x7ffdb28b7e80) = -1 ENOENT (No such file or directory)write(2, "fatal: Not a git repository (or "..., 69) = 69
Switching to using absolute paths rather than chdir-ing around is intentional for that commit, but it looks like we just need to special-case the construction of the root path.
Like this, perhaps:
setup.c +0
-0
setup.c int is_git_directory(const char *suspect) · line 290
Quoted 9 lines
setup.c +3
-1@@ -283,7 +283,9 @@ int is_git_directory(const char *suspect)size_t len;/* Check worktree-related signatures */ -strbuf_addf (&path,"%s/HEAD" , suspect); +strbuf_addstr (&path, suspect); +strbuf_complete (&path,'/' ); +strbuf_addstr (&path,"HEAD" );if (validate_headref (path.buf)) goto done;
Yes, that would work around the issue. TBH I expected / to not be a valid bare repository path (and therefore I thought that suspect could never be just a single slash), but people do all kinds of crazy stuff, right?
I note also that there are tons of strbuf_addstr(...); strbuf_complete(..., '/'); patterns in our code, as well as strbuf("%s/blub", dir), which probably should all be condensed into single function calls both for semantic clarity as well as to avoid double slashes in the middle of paths.
In the short run, though, let's take your patch. Maybe with a commit message like this?
-- snipsnap --
setup: avoid double slashes when looking for HEAD
Andrew Baumann reported that when called outside of any Git worktree, git rev-parse --is-inside-work-tree eventually tries to access //HEAD, i.e. any HEAD file in the root directory, but with a double slash.
This double slash is not only unintentional, but is allowed by the POSIX standard to have a special meaning. And most notably on Windows, it does, where it refers to a UNC path of the form //server/share/.
As a consequence, afore-mentioned rev-parse call not only looks for the wrong thing, but it also causes serious delays, as Windows will try to access a server called HEAD.
Let's simply avoid the unintended double slash.
Quoted 7 lines · Johannes Schindelin
Quoted 7 lines
setup.c +0
-0@@ -283,7 +283,9 @@ int is_git_directory(const char *suspect)size_t len;
Quoted 11 lines
setup.c int is_git_directory(const char *suspect) · line 290
Quoted 7 lines
setup.c +3
-1/* Check worktree-related signatures */ -strbuf_addf (&path,"%s/HEAD" , suspect); +strbuf_addstr (&path, suspect); +strbuf_complete (&path,'/' ); +strbuf_addstr (&path,"HEAD" );if (validate_headref (path.buf)) goto done;Yes, that would work around the issue. TBH I expected
/to not be a valid bare repository path (and therefore I thought thatsuspectcould never be just a single slash), but people do all kinds of crazy stuff, right?
Heh. People _do_ do crazy stuff, but I think here it is just the tool double-checking if people are doing crazy stuff (which they mostly aren't) by walking up to the root.
Quoted 5 lines
I note also that there are tons of
strbuf_addstr(...); strbuf_complete(..., '/');patterns in our code, as well asstrbuf("%s/blub", dir), which probably should all be condensed into single function calls both for semantic clarity as well as to avoid double slashes in the middle of paths.
Yeah, I had the same thought. This _seems_ like the kind of thing mkpathdup() would handle, but it doesn't (and as far as I can tell never did).
Grepping around for calls to strbuf_complete() with '/' shows that most callers wouldn't benefit. Many do trickier things like setting up a path ending in slash, and then appending the final element repeatedly while iterating over a list. Some have a strbuf already and just want to append the final element to it.
On the other hand, I suspect these are only the cases that are already conscientious about avoiding double-slashes. There are probably a ton of xstrfmt("%s/%s", dir, file) equivalents that just aren't careful.
Quoted 2 lines
In the short run, though, let's take your patch. Maybe with a commit message like this?
Agreed. There are enough pitfalls to a general path-constructing helper that I don't think we should hold up a fix while on it.
Quoted 2 lines
-- snipsnap --
setup: avoid double slashes when looking for HEAD
I see you posted this separately, so I'll review there.
Thanks for finishing it off (I had intended to circle back to it this morning myself).
-Peff
PATCH 1/1 setup.c: don't try to access '//HEAD' during repo discovery
In reply to Andrew BaumannCommit ce9b8aab5 (setup_git_directory_1(): avoid changing global state, 2017-03-13) changed how the git directory is discovered, and as a side effect when the discovery reaches the root directory Git tries to access paths like '//HEAD' and '//objects'. This interacts badly with Cygwin, which interprets it as a UNC file share, and so demand-loads a bunch of extra DLLs and attempts to resolve/contact the server named HEAD. This obviously doesn't work too well, especially over a slow network link.
Special case the root directory in is_git_directory() to prevent accessing paths with leading double slashes.
---
I'm not quite sure whether this is the right or complete fix. I can't test it on Cygwin, and I don't know whether Git for Windows is affected with it's drive prefixes in paths. Anyway, at least strace output on Linux looks good to me.
setup.c | 4 ++++ 1 file changed, 4 insertions(+)
setup.c +4 -0
@@ -311,6 +311,10 @@ int is_git_directory(const char *suspect)int ret =0 ;size_t len; +/* To avoid accessing '//HEAD' & co when checking the root directory */ +if (!strcmp (suspect,"/" )) + suspect ="" ; +/* Check worktree-related signatures */ strbuf_addf (&path,"%s/HEAD" , suspect);if (validate_headref (path.buf))
Signature · 1 line
2.15.0.67.gb67a46776
Re: [PATCH] setup.c: don't try to access '//HEAD' during repo discovery
Hi Gábor,
Quoted 11 lines · SZEDER Gábor
Commit
ce9b8aab5(setup_git_directory_1(): avoid changing global state, 2017-03-13) changed how the git directory is discovered, and as a side effect when the discovery reaches the root directory Git tries to access paths like '//HEAD' and '//objects'. This interacts badly with Cygwin, which interprets it as a UNC file share, and so demand-loads a bunch of extra DLLs and attempts to resolve/contact the server named HEAD. This obviously doesn't work too well, especially over a slow network link.Special case the root directory in
is_git_directory()to prevent accessing paths with leading double slashes.
I would rather not special-case the (Unix) root directory. The underlying problem, as Peff pointed out, is that an extra slash is appended if the directory. And as is_git_directory() is not marked as static, we must assume that there are other callers that may pass in directory paths ending in a slash already.
Ciao,
Dscho