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

[Jeff King <peff@peff.net>]

On Tue, Feb 26, 2019 at 05:58:51PM +0700, Nguyễn Thái Ngọc Duy wrote:

> Worktree names are based on $(basename $GIT_WORK_TREE). They aren't
> significant until 3a3b9d8cde (refs: new ref types to make per-worktree
> refs visible to all worktrees - 2018-10-21), where worktree name could
> be part of a refname and must follow refname rules.
> 
> 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)
> +{
> +	return 0 <= ch && ch < ARRAY_SIZE(refname_disposition) &&
> +		refname_disposition[ch] == 0;
> +}

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).

Other than that, I didn't see anything objectionable in the patch.

-Peff