Re: [PATCH v5 1/1] worktree add: sanitize worktree names
- From
Jeff King <peff@peff.net>
- Date
- Mar 11, 2019, 22:39 UTC
- Message-ID
- <20190311223936.GA24989@sigill.intra.peff.net>
- In-Reply-To
- <CACsJy8CqN=Uu-Fez7T9evazitVopXt2dkQ1rGzKwh94tdiUdvA@mail.gmail.com>
On Mon, Mar 11, 2019 at 04:24:13PM +0700, Duy Nguyen wrote:
Show 14 quoted lines
> > > I think this needs to be:
> > >
> > > strbuf_setlen(sanitized, sanitized->len - 1);
> > >
> > > to ensure that NUL-terminator ends up in the correct place if this "."
> > > is the very last character in 'refname'. (Otherwise, the NUL will
> > > remain after the second ".", thus ".." won't be collapsed to "." at
> > > all.)
> >
> > True. Why doesn't it do the similar "replace with -" it does for
> > other unfortunate characters, though?
>
> I think Jeff saw an opportunity to keep it cleaner ("." looks better
> than ".-") and took it.Yeah, that was one thing I was going to comment on your patch. The "rules" I made up were pretty ad-hoc as I was walking through the function (note it also drops ".lock" instead of sanitizing it into "-lock").
But it may make sense to make things more consistent (even if the result isn't entirely reversible).
Another option _is_ to actually make it reversible. I.e., use "%2e" instead of ".", which would also necessitate replacing "%". I don't know if that has a huge value for this use-case, but it's a nice property that two sanitized names can't collide (unless they originally identical).
-Peff