{"thread":{"id":"47105","subject":"git tries to stat //HEAD when searching for a repo, leading to huge delays on Cygwin","startedAt":"2017-11-02T23:46:03Z","lastAt":"2017-11-03T18:29:21Z","messageCount":6,"participants":["Andrew Baumann","Jeff King","SZEDER Gábor","Johannes Schindelin"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"331749","messageId":"MWHPR21MB0159089DB91A30E5E5CFCB479E5C0@MWHPR21MB0159.namprd21.prod.outlook.com","threadId":"47105","inReplyTo":null,"subject":"git tries to stat //HEAD when searching for a repo, leading to huge delays on Cygwin","fromName":"Andrew Baumann","fromEmail":"andrew.baumann@microsoft.com","sentAt":"2017-11-02T23:45:55Z","receivedAt":"2017-11-02T23:46:03Z","isPatch":false,"sender":{"key":"andrew.baumann@microsoft.com","avatar":null},"body":"Hi,\n\nI have a workaround for this, but someone on stack overflow [1] suggested reporting it upstream, so here you go:\n\nI 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:\n\n/home/me/.git\n/home/me/.git/HEAD\n/home/me/HEAD\n/home\n/home/.git\n/home/.git/HEAD\n/home/HEAD\n/\n/.git\n/.git/HEAD\n//HEAD\n\nThe 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.\n\nI've tested with the latest Cygwin git (2.15.0); this was also present in a prior version.\n\nCheers,\nAndrew\n\n[1] https://stackoverflow.com/questions/47084672\n"},{"id":"331752","messageId":"20171103010300.3jwme4d6nbxnj6od@sigill.intra.peff.net","threadId":"47105","inReplyTo":"MWHPR21MB0159089DB91A30E5E5CFCB479E5C0@MWHPR21MB0159.namprd21.prod.outlook.com","subject":"Re: git tries to stat //HEAD when searching for a repo, leading to huge delays on Cygwin","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-11-03T01:03:01Z","receivedAt":"2017-11-03T01:03:07Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 02, 2017 at 11:45:55PM +0000, Andrew Baumann wrote:\n\n> I have a workaround for this, but someone on stack overflow [1]\n> suggested reporting it upstream, so here you go:\n> \n> I have a fancy shell prompt that executes \"git rev-parse\n> --is-inside-work-tree\" to determine whether we're currently inside a\n> working directory. This causes git to walk up the directory hierarchy\n> looking for a containing git repo. For example, when invoked from my\n> home directory, it stats the following paths, in order:\n> \n> /home/me/.git\n> /home/me/.git/HEAD\n> /home/me/HEAD\n> /home\n> /home/.git\n> /home/.git/HEAD\n> /home/HEAD\n> /\n> /.git\n> /.git/HEAD\n> //HEAD\n> \n> The last name (//HEAD) interacts badly with Cygwin, which interprets\n> it as a UNC file share, and so demand-loads a bunch of extra DLLs and\n> attempts to resolve/contact the server named HEAD. This obviously\n> doesn't work too well, especially over a slow network link.\n> \n> I've tested with the latest Cygwin git (2.15.0); this was also present\n> in a prior version.\n\nInteresting. I can reproduce on Linux (but of course \"//HEAD\" is cheap\nto look at there). It bisects to ce9b8aab5d (setup_git_directory_1():\navoid changing global state, 2017-03-13). Before that, the end of the\nstrace for \"git rev-parse --git-dir\" looks like:\n\n chdir(\"..\") = 0\n stat(\".git\", 0x7fffba398e00) = -1 ENOENT (No such file or directory)\n lstat(\".git/HEAD\", 0x7fffba398dd0) = -1 ENOENT (No such file or directory)\n lstat(\"./HEAD\", 0x7fffba398dd0) = -1 ENOENT (No such file or directory)\n write(2, \"fatal: Not a git repository (or \"..., 69) = 69\n\nand after:\n\n stat(\"/.git\", 0x7ffdb28b7eb0) = -1 ENOENT (No such file or directory)\n lstat(\"/.git/HEAD\", 0x7ffdb28b7e80) = -1 ENOENT (No such file or directory)\n lstat(\"//HEAD\", 0x7ffdb28b7e80) = -1 ENOENT (No such file or directory)\n write(2, \"fatal: Not a git repository (or \"..., 69) = 69\n\nSwitching to using absolute paths rather than chdir-ing around is\nintentional for that commit, but it looks like we just need to\nspecial-case the construction of the root path.\n\nLike this, perhaps:\n\ndiff --git a/setup.c b/setup.c\nindex 27a31de33f..5d0b6a88e3 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -283,7 +283,9 @@ int is_git_directory(const char *suspect)\n \tsize_t len;\n \n \t/* Check worktree-related signatures */\n-\tstrbuf_addf(&path, \"%s/HEAD\", suspect);\n+\tstrbuf_addstr(&path, suspect);\n+\tstrbuf_complete(&path, '/');\n+\tstrbuf_addstr(&path, \"HEAD\");\n \tif (validate_headref(path.buf))\n \t\tgoto done;\n \n\n-Peff\n"},{"id":"331753","messageId":"20171103010645.25499-1-szeder.dev@gmail.com","threadId":"47105","inReplyTo":"MWHPR21MB0159089DB91A30E5E5CFCB479E5C0@MWHPR21MB0159.namprd21.prod.outlook.com","subject":"[PATCH] setup.c: don't try to access '//HEAD' during repo discovery","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-11-03T01:06:45Z","receivedAt":"2017-11-03T01:06:54Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Commit ce9b8aab5 (setup_git_directory_1(): avoid changing global state,\n2017-03-13) changed how the git directory is discovered, and as a side\neffect when the discovery reaches the root directory Git tries to\naccess paths like '//HEAD' and '//objects'. This interacts badly with\nCygwin, which interprets it as a UNC file share, and so demand-loads a\nbunch of extra DLLs and attempts to resolve/contact the server named\nHEAD. This obviously doesn't work too well, especially over a slow\nnetwork link.\n\nSpecial case the root directory in is_git_directory() to prevent\naccessing paths with leading double slashes.\n\nReported-by: Andrew Baumann \nSigned-off-by: SZEDER Gábor \n---\n\nI'm not quite sure whether this is the right or complete fix. I can't\ntest it on Cygwin, and I don't know whether Git for Windows is\naffected with it's drive prefixes in paths. Anyway, at least strace\noutput on Linux looks good to me.\n\n setup.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/setup.c b/setup.c\nindex 03f51e056..0cfc5e676 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -311,6 +311,10 @@ int is_git_directory(const char *suspect)\n \tint ret = 0;\n \tsize_t len;\n \n+\t/* To avoid accessing '//HEAD' & co when checking the root directory */\n+\tif (!strcmp(suspect, \"/\"))\n+\t\tsuspect = \"\";\n+\n \t/* Check worktree-related signatures */\n \tstrbuf_addf(&path, \"%s/HEAD\", suspect);\n \tif (validate_headref(path.buf))\n-- \n2.15.0.67.gb67a46776\n\n"},{"id":"331773","messageId":"alpine.DEB.2.21.1.1711031324110.6482@virtualbox","threadId":"47105","inReplyTo":"20171103010300.3jwme4d6nbxnj6od@sigill.intra.peff.net","subject":"Re: git tries to stat //HEAD when searching for a repo, leading to huge delays on Cygwin","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-11-03T12:32:26Z","receivedAt":"2017-11-03T12:33:00Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\n\nOn Thu, 2 Nov 2017, Jeff King wrote:\n\n> On Thu, Nov 02, 2017 at 11:45:55PM +0000, Andrew Baumann wrote:\n> \n> > I have a workaround for this, but someone on stack overflow [1]\n> > suggested reporting it upstream, so here you go:\n> > \n> > I have a fancy shell prompt that executes \"git rev-parse\n> > --is-inside-work-tree\" to determine whether we're currently inside a\n> > working directory. This causes git to walk up the directory hierarchy\n> > looking for a containing git repo. For example, when invoked from my\n> > home directory, it stats the following paths, in order:\n> > \n> > /home/me/.git\n> > /home/me/.git/HEAD\n> > /home/me/HEAD\n> > /home\n> > /home/.git\n> > /home/.git/HEAD\n> > /home/HEAD\n> > /\n> > /.git\n> > /.git/HEAD\n> > //HEAD\n> > \n> > The last name (//HEAD) interacts badly with Cygwin, which interprets\n> > it as a UNC file share, and so demand-loads a bunch of extra DLLs and\n> > attempts to resolve/contact the server named HEAD. This obviously\n> > doesn't work too well, especially over a slow network link.\n> > \n> > I've tested with the latest Cygwin git (2.15.0); this was also present\n> > in a prior version.\n> \n> Interesting. I can reproduce on Linux (but of course \"//HEAD\" is cheap\n> to look at there). It bisects to ce9b8aab5d (setup_git_directory_1():\n> avoid changing global state, 2017-03-13). Before that, the end of the\n> strace for \"git rev-parse --git-dir\" looks like:\n> \n> chdir(\"..\") = 0\n> stat(\".git\", 0x7fffba398e00) = -1 ENOENT (No such file or directory)\n> lstat(\".git/HEAD\", 0x7fffba398dd0) = -1 ENOENT (No such file or directory)\n> lstat(\"./HEAD\", 0x7fffba398dd0) = -1 ENOENT (No such file or directory)\n> write(2, \"fatal: Not a git repository (or \"..., 69) = 69\n> \n> and after:\n> \n> stat(\"/.git\", 0x7ffdb28b7eb0) = -1 ENOENT (No such file or directory)\n> lstat(\"/.git/HEAD\", 0x7ffdb28b7e80) = -1 ENOENT (No such file or directory)\n> lstat(\"//HEAD\", 0x7ffdb28b7e80) = -1 ENOENT (No such file or directory)\n> write(2, \"fatal: Not a git repository (or \"..., 69) = 69\n> \n> Switching to using absolute paths rather than chdir-ing around is\n> intentional for that commit, but it looks like we just need to\n> special-case the construction of the root path.\n> \n> Like this, perhaps:\n> \n> diff --git a/setup.c b/setup.c\n> index 27a31de33f..5d0b6a88e3 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -283,7 +283,9 @@ int is_git_directory(const char *suspect)\n> \tsize_t len;\n> \n> \t/* Check worktree-related signatures */\n> -\tstrbuf_addf(&path, \"%s/HEAD\", suspect);\n> +\tstrbuf_addstr(&path, suspect);\n> +\tstrbuf_complete(&path, '/');\n> +\tstrbuf_addstr(&path, \"HEAD\");\n> \tif (validate_headref(path.buf))\n> \t\tgoto done;\n\nYes, that would work around the issue. TBH I expected `/` to not be a\nvalid bare repository path (and therefore I thought that `suspect` could\nnever be just a single slash), but people do all kinds of crazy stuff, right?\n\nI note also that there are tons of `strbuf_addstr(...);\nstrbuf_complete(..., '/');` patterns in our code, as well as\n`strbuf(\"%s/blub\", dir)`, which probably should all be condensed into\nsingle function calls both for semantic clarity as well as to avoid double\nslashes in the middle of paths.\n\nIn the short run, though, let's take your patch. Maybe with a commit\nmessage like this?\n\n-- snipsnap --\nsetup: avoid double slashes when looking for HEAD\n\nAndrew Baumann reported that when called outside of any Git worktree, `git\nrev-parse --is-inside-work-tree` eventually tries to access `//HEAD`, i.e.\nany `HEAD` file in the root directory, but with a double slash.\n\nThis double slash is not only unintentional, but is allowed by the POSIX\nstandard to have a special meaning. And most notably on Windows, it does,\nwhere it refers to a UNC path of the form `//server/share/`.\n\nAs a consequence, afore-mentioned `rev-parse` call not only looks for the\nwrong thing, but it also causes serious delays, as Windows will try to\naccess a server called `HEAD`.\n\nLet's simply avoid the unintended double slash.\n\nSigned-off-by: Jeff King \nAcked-by: Johannes Schindelin \n"},{"id":"331774","messageId":"alpine.DEB.2.21.1.1711031334000.6482@virtualbox","threadId":"47105","inReplyTo":"20171103010645.25499-1-szeder.dev@gmail.com","subject":"Re: [PATCH] setup.c: don't try to access '//HEAD' during repo discovery","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-11-03T12:36:45Z","receivedAt":"2017-11-03T12:36:55Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Gábor,\n\nOn Fri, 3 Nov 2017, SZEDER Gábor wrote:\n\n> Commit ce9b8aab5 (setup_git_directory_1(): avoid changing global state,\n> 2017-03-13) changed how the git directory is discovered, and as a side\n> effect when the discovery reaches the root directory Git tries to\n> access paths like '//HEAD' and '//objects'. This interacts badly with\n> Cygwin, which interprets it as a UNC file share, and so demand-loads a\n> bunch of extra DLLs and attempts to resolve/contact the server named\n> HEAD. This obviously doesn't work too well, especially over a slow\n> network link.\n> \n> Special case the root directory in is_git_directory() to prevent\n> accessing paths with leading double slashes.\n\nI would rather not special-case the (Unix) root directory. The underlying\nproblem, as Peff pointed out, is that an extra slash is appended if the\ndirectory. And as `is_git_directory()` is not marked as static, we must\nassume that there are other callers that may pass in directory paths\nending in a slash already.\n\nCiao,\nDscho"},{"id":"331795","messageId":"20171103182910.x7fbwodoegusyc37@sigill.intra.peff.net","threadId":"47105","inReplyTo":"alpine.DEB.2.21.1.1711031324110.6482@virtualbox","subject":"Re: git tries to stat //HEAD when searching for a repo, leading to huge delays on Cygwin","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-11-03T18:29:10Z","receivedAt":"2017-11-03T18:29:21Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 03, 2017 at 01:32:26PM +0100, Johannes Schindelin wrote:\n\n> > diff --git a/setup.c b/setup.c\n> > index 27a31de33f..5d0b6a88e3 100644\n> > --- a/setup.c\n> > +++ b/setup.c\n> > @@ -283,7 +283,9 @@ int is_git_directory(const char *suspect)\n> > \tsize_t len;\n> > \n> > \t/* Check worktree-related signatures */\n> > -\tstrbuf_addf(&path, \"%s/HEAD\", suspect);\n> > +\tstrbuf_addstr(&path, suspect);\n> > +\tstrbuf_complete(&path, '/');\n> > +\tstrbuf_addstr(&path, \"HEAD\");\n> > \tif (validate_headref(path.buf))\n> > \t\tgoto done;\n> \n> Yes, that would work around the issue. TBH I expected `/` to not be a\n> valid bare repository path (and therefore I thought that `suspect` could\n> never be just a single slash), but people do all kinds of crazy stuff, right?\n\nHeh. People _do_ do crazy stuff, but I think here it is just the tool\ndouble-checking if people are doing crazy stuff (which they mostly\naren't) by walking up to the root.\n\n> I note also that there are tons of `strbuf_addstr(...);\n> strbuf_complete(..., '/');` patterns in our code, as well as\n> `strbuf(\"%s/blub\", dir)`, which probably should all be condensed into\n> single function calls both for semantic clarity as well as to avoid double\n> slashes in the middle of paths.\n\nYeah, I had the same thought. This _seems_ like the kind of thing\nmkpathdup() would handle, but it doesn't (and as far as I can tell never\ndid).\n\nGrepping around for calls to strbuf_complete() with '/' shows that most\ncallers wouldn't benefit. Many do trickier things like setting up a path\nending in slash, and then appending the final element repeatedly while\niterating over a list. Some have a strbuf already and just want to\nappend the final element to it.\n\nOn the other hand, I suspect these are only the cases that are already\nconscientious about avoiding double-slashes. There are probably a ton of\nxstrfmt(\"%s/%s\", dir, file) equivalents that just aren't careful.\n\n> In the short run, though, let's take your patch. Maybe with a commit\n> message like this?\n\nAgreed. There are enough pitfalls to a general path-constructing helper\nthat I don't think we should hold up a fix while on it.\n\n> -- snipsnap --\n> setup: avoid double slashes when looking for HEAD\n\nI see you posted this separately, so I'll review there.\n\nThanks for finishing it off (I had intended to circle back to it this\nmorning myself).\n\n-Peff\n"}]}