Re: [PATCH] worktree add: sanitize worktree names

From: Duy Nguyen <pclouds@gmail.com>
Date: Feb 21, 2019, 11:52 UTC
Message-ID: <CACsJy8CWH-b18uaRvn-bXdsRbn+6QnJ6GNekqG2khGeJUa8S3w@mail.gmail.com>
In-Reply-To: <1550749488.30307.2@yandex.ru>

On Thu, Feb 21, 2019 at 6:44 PM Konstantin Kharlamov <hi-angel@yandex.ru> wrote:

Show 16 quoted lines

> >>  > +
> >>  > +     /* last resort, should never ever happen in practice */
> >>  > +     if (name->len == 0)
> >>  > +             strbuf_addstr(name, "worktree");
> >>
> >>  I assume this means a user have passed a zero-sized worktree name?
> >> But
> >>  zero-sized file/directory names are not possible anyway, would it
> >> make
> >>  sense to just return an error in this case?
> >
> > It could happen if you do "git worktree add .lock". The ".lock" part
> > will be stripped out, leaving us with an empty string.
>
> Ah, I see. Then, would it maybe make sense to just sanitize the ".lock"
> out the same way as you did with special symbols, i.e. with dashes?

Hmm.. I actually did not think of that. Then we could return the error if "name" is empty.

> (I am not a git developer, so not sure if that's a good question, but I
> would also question why ".lock" needs to be deleted. I guess git uses

It's because "foo.lock" is usually a temporary file to prepare things before we do an atomic update to "foo". And the "refs guys" were just being careful when they designed reference names so they reject any reference names that end with .lock. You can try to create a branch named something.lock, it will not go through. This is actually documented in "git help check-ref-format".

> the postfix internally, but why can't it be okay with "name.lock.lock")

Uh oh I miss this case. I only delete ".lock" once, "name.lock" would still be rejected. Thanks for noticing.

-- 
Duy