On Mon, Mar 11, 2019 at 04:24:13PM +0700, Duy Nguyen wrote:
> > > 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
