Re: git tries to stat //HEAD when searching for a repo, leading to huge delays on Cygwin

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Peff,


On Thu, 2 Nov 2017, Jeff King wrote:

> On Thu, Nov 02, 2017 at 11:45:55PM +0000, Andrew Baumann wrote:
> 
> > 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.
> 
> 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:
> 
> diff --git a/setup.c b/setup.c
> index 27a31de33f..5d0b6a88e3 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -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.

Signed-off-by: Jeff King <peff@peff.net>
Acked-by: Johannes Schindelin <johannes.schindelin@gmx.de>