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

[Eric Sunshine <sunshine@sunshineco.com>]

On Wed, Feb 27, 2019 at 7:09 AM Jeff King <peff@peff.net> wrote:
> On Tue, Feb 26, 2019 at 05:58:51PM +0700, Nguyễn Thái Ngọc Duy wrote:
> > Update 'worktree add' code to remove special characters to follow
> > these rules. The code could replace chars with '-' more than
> > necessary, but it keeps the code simple. In the future the user will
> > be able to specify the worktree name by themselves if they're not
> > happy with this dumb character substitution.
>
> So notably this gets around ".." and ".lock" by just disallowing "."
> entirely. I think I'm OK with that for worktrees. It does make me a
> little nervous to see this new public function, though:
>
> > +int char_allowed_in_refname(int ch) [...]
>
> because it's not entirely accurate, as you noted above. I wonder if we
> could name this differently to warn people that the refname rules are
> not so simple.
>
> 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.