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