From: Konstantin Kharlamov Date: Thu, 21 Feb 2019 11:44:48 GMT Subject: Re: [PATCH] worktree add: sanitize worktree names Message-ID: <1550749488.30307.2@yandex.ru> In-Reply-To: On Чт, Feb 21, 2019 at 2:38 PM, Duy Nguyen wrote: > On Thu, Feb 21, 2019 at 6:28 PM Konstantin Kharlamov > wrote: >> >> >> >> On Чт, Feb 21, 2019 at 2:00 PM, >> =?UTF-8?b?Tmd1eeG7hW4gVGjDoWkgTmfhu41j?= 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. >> > >> > Reported-by: hi-angel@yandex.ru >> > Signed-off-by: Nguyễn Thái Ngọc Duy >> > --- >> > builtin/worktree.c | 47 >> > ++++++++++++++++++++++++++++++++++++++++- >> > t/t2025-worktree-add.sh | 5 +++++ >> > 2 files changed, 51 insertions(+), 1 deletion(-) >> > >> > diff --git a/builtin/worktree.c b/builtin/worktree.c >> > index 3f9907fcc9..ff36838a33 100644 >> > --- a/builtin/worktree.c >> > +++ b/builtin/worktree.c >> > @@ -262,6 +262,46 @@ static void validate_worktree_add(const char >> > *path, const struct add_opts *opts) >> > free_worktrees(worktrees); >> > } >> > >> > +/* >> > + * worktree name is part of refname and has to pass >> > + * check_refname_component(). Remove unallowed characters to >> make it >> > + * valid. >> > + */ >> > +static void sanitize_worktree_name(struct strbuf *name) >> > +{ >> > + int i; >> > + >> > + /* no ending with .lock */ >> > + if (ends_with(name->buf, ".lock")) >> > + strbuf_remove(name, name->len - strlen(".lock"), >> > + strlen(".lock")); >> > + >> > + /* >> > + * All special chars replaced with dashes. See >> > + * check_refname_component() for reference. >> > + */ >> > + for (i = 0; i < name->len; i++) { >> > + if (strchr(":?[]\\~ \t@{}*/.", name->buf[i])) >> > + name->buf[i] = '-'; >> > + } >> > + >> > + /* remove consecutive dashes, leading or trailing dashes */ >> > + for (i = 0; i < name->len; i++) { >> > + while (name->buf[i] == '-' && >> > + (i == 0 || >> > + i == name->len - 1 || >> > + (i < name->len - 1 && name->buf[i + 1] == >> '-'))) >> > + strbuf_remove(name, i, 1); >> > + } >> > + >> > + /* last resort, should never ever happen in practice */ >> > + if (name->len == 0) >> > + strbuf_addstr(name, "worktree"); >> >> I assume this means a user have passed a zero-sized worktree name? >> But >> zero-sized file/directory names are not possible anyway, would it >> make >> sense to just return an error in this case? > > It could happen if you do "git worktree add .lock". The ".lock" part > will be stripped out, leaving us with an empty string. Ah, I see. Then, would it maybe make sense to just sanitize the ".lock" out the same way as you did with special symbols, i.e. with dashes? (I am not a git developer, so not sure if that's a good question, but I would also question why ".lock" needs to be deleted. I guess git uses the postfix internally, but why can't it be okay with "name.lock.lock")