Re: [PATCH v3 1/1] worktree add: sanitize worktree names

[Duy Nguyen <pclouds@gmail.com>]

On Wed, Feb 27, 2019 at 11:05 PM Jeff King <peff@peff.net> wrote:
>
> On Wed, Feb 27, 2019 at 09:23:33AM -0500, Eric Sunshine wrote:
>
> > > If we just cared about saying "is this worktree name valid", I'd suggest
> > > actually constructing a sample refname with the worktree name embedded
> > > in it and feeding that to check_refname_format(). But because you want
> > > to actually sanitize, I don't think there's an easy way to reuse it.
> > >
> > > So this approach is probably the best we can do, though I do still think
> > > it's worth renaming that function (and/or putting a big warning comment
> > > in front of it).
> >
> > The above arguments seem to suggest the introduction of a companion to
> > check_refname_format() for sanitizing, perhaps named
> > sanitize_refname_format(), in ref.[hc]. The potential difficulty with
> > that is defining exactly what "sanitize" means. Will it be contextual?
> > (That is, will git-worktree have differently sanitation needs than
> > some other facility?) If so, perhaps a 'flags' argument could control
> > how sanitization is done.
>
> I agree that sanitize_refname_format() would be nice, but I'm pretty
> sure it's going to end up having to duplicate many of the rules from
> check_refname_format(). Which is ugly if the two ever get out of sync.
>
> But if we could write it in a way that keeps the actual policy logic in
> one factored-out portion, I think it would be worth doing.

I think we could make check_refname_format() returns the bad position
and several different error codes depending on context. Then
sanitize_.. can just repeatedly call check_refname_format and fix up
whatever error it reports. Performance goes straight to hell but I
don't think that's a big deal for git-worktree, and it keeps
check_refname_format() simple (relatively speaking).

The second option is make check_refname_format() call some callback
instead of returning error. This allows sanitize_ to fix up in one go
(inside the callback), but check_refname_format could be a lot uglier,
and verifying all refs (I think pack-refs does this?) could also be
slowed down.
-- 
Duy