Web lists-archives.com

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

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

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

> 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

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