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

From: Jeff King <peff@peff.net>
Date: Nov 3, 2017, 18:29 UTC
Message-ID: <20171103182910.x7fbwodoegusyc37@sigill.intra.peff.net>
In-Reply-To: <alpine.DEB.2.21.1.1711031324110.6482@virtualbox>

On Fri, Nov 03, 2017 at 01:32:26PM +0100, Johannes Schindelin wrote:

Show 18 quoted lines

> > 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?

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.

Show 5 quoted lines

> 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.

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.

> 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.

> -- 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