{"thread":{"id":"50533","subject":"git gc fails with \"unable to resolve reference\" for worktree","startedAt":"2019-02-18T14:43:22Z","lastAt":"2019-03-12T06:45:48Z","messageCount":41,"participants":["hi-angel@yandex.ru","Duy Nguyen","Nguyễn Thái Ngọc Duy","Konstantin Kharlamov","Jeff King","Ramsay Jones","Eric Sunshine","Junio C Hamano","Johannes Schindelin"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"369528","messageId":"1550500586.2865.0@yandex.ru","threadId":"50533","inReplyTo":null,"subject":"git gc fails with \"unable to resolve reference\" for worktree","fromName":"","fromEmail":"hi-angel@yandex.ru","sentAt":"2019-02-18T14:36:26Z","receivedAt":"2019-02-18T14:43:22Z","isPatch":false,"sender":{"key":"hi-angel@yandex.ru","avatar":null},"body":"# Steps to reproduce (in terms of terminal commands)\n\n $ mkdir foo\n $ cd foo\n $ git init\n Initialized empty Git repository in /tmp/foo/.git/\n $ echo hello > testfile\n $ git add testfile && git commit -m \"my commit1\"\n [master (root-commit) d5f0b47] my commit1\n 1 file changed, 1 insertion(+)\n create mode 100644 testfile\n $ git checkout -b bar\n Switched to a new branch 'bar'\n $ git worktree add ../bar\\ \\(worktree\\) master\n Preparing worktree (checking out 'master')\n HEAD is now at d5f0b47 my commit1\n $ git gc\n error: cannot lock ref 'worktrees/bar (worktree)/HEAD': unable to \nresolve reference 'worktrees/bar (worktree)/HEAD': Invalid argument\n fatal: failed to run reflog\n\n# Expected\n\nNo errors\n\n# Actual\n\nerror: cannot lock ref 'worktrees/bar (worktree)/HEAD': unable to \nresolve reference 'worktrees/bar (worktree)/HEAD': Invalid argument\n\n\n"},{"id":"369529","messageId":"CACsJy8Bjryv5Of0kN-wwiQs5S3Km=z=WRDTPcBD_Sgsm6Mvjag@mail.gmail.com","threadId":"50533","inReplyTo":"1550500586.2865.0@yandex.ru","subject":"Re: git gc fails with \"unable to resolve reference\" for worktree","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-02-18T15:02:34Z","receivedAt":"2019-02-18T15:03:03Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Feb 18, 2019 at 9:44 PM wrote:\n>\n> # Steps to reproduce (in terms of terminal commands)\n>\n> $ mkdir foo\n> $ cd foo\n> $ git init\n> Initialized empty Git repository in /tmp/foo/.git/\n> $ echo hello > testfile\n> $ git add testfile && git commit -m \"my commit1\"\n> [master (root-commit) d5f0b47] my commit1\n> 1 file changed, 1 insertion(+)\n> create mode 100644 testfile\n> $ git checkout -b bar\n> Switched to a new branch 'bar'\n> $ git worktree add ../bar\\ \\(worktree\\) master\n> Preparing worktree (checking out 'master')\n> HEAD is now at d5f0b47 my commit1\n> $ git gc\n> error: cannot lock ref 'worktrees/bar (worktree)/HEAD': unable to\n> resolve reference 'worktrees/bar (worktree)/HEAD': Invalid argument\n\nThanks for reporting. This is not a valid reference and causes the\nproblem. The worktree's name has to sanitized. I'll fix it tomorrow.\n\n> fatal: failed to run reflog\n>\n> # Expected\n>\n> No errors\n>\n> # Actual\n>\n> error: cannot lock ref 'worktrees/bar (worktree)/HEAD': unable to\n> resolve reference 'worktrees/bar (worktree)/HEAD': Invalid argument\n>\n>\n\n\n-- \nDuy\n"},{"id":"369530","messageId":"1550502546.2865.1@yandex.ru","threadId":"50533","inReplyTo":"CACsJy8Bjryv5Of0kN-wwiQs5S3Km=z=WRDTPcBD_Sgsm6Mvjag@mail.gmail.com","subject":"Re: git gc fails with \"unable to resolve reference\" for worktree","fromName":"","fromEmail":"hi-angel@yandex.ru","sentAt":"2019-02-18T15:09:06Z","receivedAt":"2019-02-18T15:09:12Z","isPatch":false,"sender":{"key":"hi-angel@yandex.ru","avatar":null},"body":"\n\nOn Пн, Feb 18, 2019 at 6:02 PM, Duy Nguyen wrote:\n> On Mon, Feb 18, 2019 at 9:44 PM wrote:\n>> \n>> # Steps to reproduce (in terms of terminal commands)\n>> \n>> $ mkdir foo\n>> $ cd foo\n>> $ git init\n>> Initialized empty Git repository in /tmp/foo/.git/\n>> $ echo hello > testfile\n>> $ git add testfile && git commit -m \"my commit1\"\n>> [master (root-commit) d5f0b47] my commit1\n>> 1 file changed, 1 insertion(+)\n>> create mode 100644 testfile\n>> $ git checkout -b bar\n>> Switched to a new branch 'bar'\n>> $ git worktree add ../bar\\ \\(worktree\\) master\n>> Preparing worktree (checking out 'master')\n>> HEAD is now at d5f0b47 my commit1\n>> $ git gc\n>> error: cannot lock ref 'worktrees/bar (worktree)/HEAD': unable \n>> to\n>> resolve reference 'worktrees/bar (worktree)/HEAD': Invalid argument\n> \n> Thanks for reporting. This is not a valid reference and causes the\n> problem. The worktree's name has to sanitized. I'll fix it tomorrow.\n>> \n\nYou mean, you want to prohibit such directory names as a worktree? But \nit's a proper directory naming, can perhaps git do the sanitizing \ntransparently for end-user?\n\n\n"},{"id":"369532","messageId":"CACsJy8BLERwq_oSnox7f0f_VPZy9NMjpM=nym_sJ-i8k-0DTKg@mail.gmail.com","threadId":"50533","inReplyTo":"1550502546.2865.1@yandex.ru","subject":"Re: git gc fails with \"unable to resolve reference\" for worktree","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-02-18T15:18:20Z","receivedAt":"2019-02-18T15:18:49Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Feb 18, 2019 at 10:09 PM wrote:\n>\n>\n>\n> On Пн, Feb 18, 2019 at 6:02 PM, Duy Nguyen wrote:\n> > On Mon, Feb 18, 2019 at 9:44 PM wrote:\n> >>\n> >> # Steps to reproduce (in terms of terminal commands)\n> >>\n> >> $ mkdir foo\n> >> $ cd foo\n> >> $ git init\n> >> Initialized empty Git repository in /tmp/foo/.git/\n> >> $ echo hello > testfile\n> >> $ git add testfile && git commit -m \"my commit1\"\n> >> [master (root-commit) d5f0b47] my commit1\n> >> 1 file changed, 1 insertion(+)\n> >> create mode 100644 testfile\n> >> $ git checkout -b bar\n> >> Switched to a new branch 'bar'\n> >> $ git worktree add ../bar\\ \\(worktree\\) master\n> >> Preparing worktree (checking out 'master')\n> >> HEAD is now at d5f0b47 my commit1\n> >> $ git gc\n> >> error: cannot lock ref 'worktrees/bar (worktree)/HEAD': unable\n> >> to\n> >> resolve reference 'worktrees/bar (worktree)/HEAD': Invalid argument\n> >\n> > Thanks for reporting. This is not a valid reference and causes the\n> > problem. The worktree's name has to sanitized. I'll fix it tomorrow.\n> >>\n>\n> You mean, you want to prohibit such directory names as a worktree? But\n> it's a proper directory naming, can perhaps git do the sanitizing\n> transparently for end-user?\n\nNo, not inhibiting. When you do \"git add ../(abc)\" then the internal\nname could be simply \"abc\" or something like that instead of \"(abc)\"\nwhich is invalid to reflog and other commands. The worktree's location\nis still '(abc)'. There's also a work-in-progress option to let the\nuser control this worktree name directly.\n-- \nDuy\n"},{"id":"369720","messageId":"1550673274.30738.0@yandex.ru","threadId":"50533","inReplyTo":"CACsJy8BLERwq_oSnox7f0f_VPZy9NMjpM=nym_sJ-i8k-0DTKg@mail.gmail.com","subject":"Re: git gc fails with \"unable to resolve reference\" for worktree","fromName":"","fromEmail":"hi-angel@yandex.ru","sentAt":"2019-02-20T14:34:34Z","receivedAt":"2019-02-20T14:34:41Z","isPatch":false,"sender":{"key":"hi-angel@yandex.ru","avatar":null},"body":"I see, thanks!\n\nOn Пн, Feb 18, 2019 at 6:18 PM, Duy Nguyen wrote:\n> On Mon, Feb 18, 2019 at 10:09 PM wrote:\n>> \n>> \n>> \n>> On Пн, Feb 18, 2019 at 6:02 PM, Duy Nguyen \n>> wrote:\n>> > On Mon, Feb 18, 2019 at 9:44 PM wrote:\n>> >>\n>> >> # Steps to reproduce (in terms of terminal commands)\n>> >>\n>> >> $ mkdir foo\n>> >> $ cd foo\n>> >> $ git init\n>> >> Initialized empty Git repository in /tmp/foo/.git/\n>> >> $ echo hello > testfile\n>> >> $ git add testfile && git commit -m \"my commit1\"\n>> >> [master (root-commit) d5f0b47] my commit1\n>> >> 1 file changed, 1 insertion(+)\n>> >> create mode 100644 testfile\n>> >> $ git checkout -b bar\n>> >> Switched to a new branch 'bar'\n>> >> $ git worktree add ../bar\\ \\(worktree\\) master\n>> >> Preparing worktree (checking out 'master')\n>> >> HEAD is now at d5f0b47 my commit1\n>> >> $ git gc\n>> >> error: cannot lock ref 'worktrees/bar (worktree)/HEAD': \n>> unable\n>> >> to\n>> >> resolve reference 'worktrees/bar (worktree)/HEAD': Invalid \n>> argument\n>> >\n>> > Thanks for reporting. This is not a valid reference and causes the\n>> > problem. The worktree's name has to sanitized. I'll fix it \n>> tomorrow.\n>> >>\n>> \n>> You mean, you want to prohibit such directory names as a worktree? \n>> But\n>> it's a proper directory naming, can perhaps git do the sanitizing\n>> transparently for end-user?\n> \n> No, not inhibiting. When you do \"git add ../(abc)\" then the internal\n> name could be simply \"abc\" or something like that instead of \"(abc)\"\n> which is invalid to reflog and other commands. The worktree's location\n> is still '(abc)'. There's also a work-in-progress option to let the\n> user control this worktree name directly.\n> --\n> Duy\n\n\n"},{"id":"369768","messageId":"20190221110026.23135-1-pclouds@gmail.com","threadId":"50533","inReplyTo":"1550500586.2865.0@yandex.ru","subject":"[PATCH] worktree add: sanitize worktree names","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2019-02-21T11:00:26Z","receivedAt":"2019-02-21T11:00:42Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Worktree names are based on $(basename $GIT_WORK_TREE). They aren't\nsignificant until 3a3b9d8cde (refs: new ref types to make per-worktree\nrefs visible to all worktrees - 2018-10-21), where worktree name could\nbe part of a refname and must follow refname rules.\n\nUpdate 'worktree add' code to remove special characters to follow\nthese rules. The code could replace chars with '-' more than\nnecessary, but it keeps the code simple. In the future the user will\nbe able to specify the worktree name by themselves if they're not\nhappy with this dumb character substitution.\n\nReported-by: hi-angel@yandex.ru\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/worktree.c | 47 ++++++++++++++++++++++++++++++++++++++++-\n t/t2025-worktree-add.sh | 5 +++++\n 2 files changed, 51 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 3f9907fcc9..ff36838a33 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -262,6 +262,46 @@ static void validate_worktree_add(const char *path, const struct add_opts *opts)\n \tfree_worktrees(worktrees);\n }\n \n+/*\n+ * worktree name is part of refname and has to pass\n+ * check_refname_component(). Remove unallowed characters to make it\n+ * valid.\n+ */\n+static void sanitize_worktree_name(struct strbuf *name)\n+{\n+\tint i;\n+\n+\t/* no ending with .lock */\n+\tif (ends_with(name->buf, \".lock\"))\n+\t\tstrbuf_remove(name, name->len - strlen(\".lock\"),\n+\t\t\t strlen(\".lock\"));\n+\n+\t/*\n+\t * All special chars replaced with dashes. See\n+\t * check_refname_component() for reference.\n+\t */\n+\tfor (i = 0; i < name->len; i++) {\n+\t\tif (strchr(\":?[]\\\\~ \\t@{}*/.\", name->buf[i]))\n+\t\t\tname->buf[i] = '-';\n+\t}\n+\n+\t/* remove consecutive dashes, leading or trailing dashes */\n+\tfor (i = 0; i < name->len; i++) {\n+\t\twhile (name->buf[i] == '-' &&\n+\t\t (i == 0 ||\n+\t\t\ti == name->len - 1 ||\n+\t\t\t(i < name->len - 1 && name->buf[i + 1] == '-')))\n+\t\t\tstrbuf_remove(name, i, 1);\n+\t}\n+\n+\t/* last resort, should never ever happen in practice */\n+\tif (name->len == 0)\n+\t\tstrbuf_addstr(name, \"worktree\");\n+\n+\tif (check_refname_format(name->buf, REFNAME_ALLOW_ONELEVEL))\n+\t\tBUG(\"worktree name '%s' is not a valid refname\", name->buf);\n+}\n+\n static int add_worktree(const char *path, const char *refname,\n \t\t\tconst struct add_opts *opts)\n {\n@@ -275,6 +315,7 @@ static int add_worktree(const char *path, const char *refname,\n \tstruct strbuf symref = STRBUF_INIT;\n \tstruct commit *commit = NULL;\n \tint is_branch = 0;\n+\tstruct strbuf sb_name = STRBUF_INIT;\n \n \tvalidate_worktree_add(path, opts);\n \n@@ -290,7 +331,10 @@ static int add_worktree(const char *path, const char *refname,\n \t\tdie(_(\"invalid reference: %s\"), refname);\n \n \tname = worktree_basename(path, &len);\n-\tgit_path_buf(&sb_repo, \"worktrees/%.*s\", (int)(path + len - name), name);\n+\tstrbuf_add(&sb_name, name, path + len - name);\n+\tsanitize_worktree_name(&sb_name);\n+\tname = sb_name.buf;\n+\tgit_path_buf(&sb_repo, \"worktrees/%s\", name);\n \tlen = sb_repo.len;\n \tif (safe_create_leading_directories_const(sb_repo.buf))\n \t\tdie_errno(_(\"could not create leading directories of '%s'\"),\n@@ -415,6 +459,7 @@ static int add_worktree(const char *path, const char *refname,\n \tstrbuf_release(&symref);\n \tstrbuf_release(&sb_repo);\n \tstrbuf_release(&sb_git);\n+\tstrbuf_release(&sb_name);\n \treturn ret;\n }\n \ndiff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh\nindex 286bba35d8..0d465adb54 100755\n--- a/t/t2025-worktree-add.sh\n+++ b/t/t2025-worktree-add.sh\n@@ -570,4 +570,9 @@ test_expect_success '\"add\" an existing locked but missing worktree' '\n \tgit worktree add --force --force --detach gnoo\n '\n \n+test_expect_success 'sanitize generated worktree name' '\n+\tgit worktree add --detach \". weird*..?.lock\" &&\n+\ttest -d .git/worktrees/weird\n+'\n+\n test_done\n-- \n2.21.0.rc1.337.gdf7f8d0522\n\n"},{"id":"369793","messageId":"1550748525.30307.1@yandex.ru","threadId":"50533","inReplyTo":"20190221110026.23135-1-pclouds@gmail.com","subject":"Re: [PATCH] worktree add: sanitize worktree names","fromName":"Konstantin Kharlamov","fromEmail":"hi-angel@yandex.ru","sentAt":"2019-02-21T11:28:45Z","receivedAt":"2019-02-21T11:28:55Z","isPatch":true,"sender":{"key":"hi-angel@yandex.ru","avatar":null},"body":"\n\nOn Чт, Feb 21, 2019 at 2:00 PM, \n=?UTF-8?b?Tmd1eeG7hW4gVGjDoWkgTmfhu41j?= Duy wrote:\n> Worktree names are based on $(basename $GIT_WORK_TREE). They aren't\n> significant until 3a3b9d8cde (refs: new ref types to make per-worktree\n> refs visible to all worktrees - 2018-10-21), where worktree name could\n> be part of a refname and must follow refname rules.\n> \n> Update 'worktree add' code to remove special characters to follow\n> these rules. The code could replace chars with '-' more than\n> necessary, but it keeps the code simple. In the future the user will\n> be able to specify the worktree name by themselves if they're not\n> happy with this dumb character substitution.\n> \n> Reported-by: hi-angel@yandex.ru\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> builtin/worktree.c | 47 \n> ++++++++++++++++++++++++++++++++++++++++-\n> t/t2025-worktree-add.sh | 5 +++++\n> 2 files changed, 51 insertions(+), 1 deletion(-)\n> \n> diff --git a/builtin/worktree.c b/builtin/worktree.c\n> index 3f9907fcc9..ff36838a33 100644\n> --- a/builtin/worktree.c\n> +++ b/builtin/worktree.c\n> @@ -262,6 +262,46 @@ static void validate_worktree_add(const char \n> *path, const struct add_opts *opts)\n> \tfree_worktrees(worktrees);\n> }\n> \n> +/*\n> + * worktree name is part of refname and has to pass\n> + * check_refname_component(). Remove unallowed characters to make it\n> + * valid.\n> + */\n> +static void sanitize_worktree_name(struct strbuf *name)\n> +{\n> +\tint i;\n> +\n> +\t/* no ending with .lock */\n> +\tif (ends_with(name->buf, \".lock\"))\n> +\t\tstrbuf_remove(name, name->len - strlen(\".lock\"),\n> +\t\t\t strlen(\".lock\"));\n> +\n> +\t/*\n> +\t * All special chars replaced with dashes. See\n> +\t * check_refname_component() for reference.\n> +\t */\n> +\tfor (i = 0; i < name->len; i++) {\n> +\t\tif (strchr(\":?[]\\\\~ \\t@{}*/.\", name->buf[i]))\n> +\t\t\tname->buf[i] = '-';\n> +\t}\n> +\n> +\t/* remove consecutive dashes, leading or trailing dashes */\n> +\tfor (i = 0; i < name->len; i++) {\n> +\t\twhile (name->buf[i] == '-' &&\n> +\t\t (i == 0 ||\n> +\t\t\ti == name->len - 1 ||\n> +\t\t\t(i < name->len - 1 && name->buf[i + 1] == '-')))\n> +\t\t\tstrbuf_remove(name, i, 1);\n> +\t}\n> +\n> +\t/* last resort, should never ever happen in practice */\n> +\tif (name->len == 0)\n> +\t\tstrbuf_addstr(name, \"worktree\");\n\nI assume this means a user have passed a zero-sized worktree name? But \nzero-sized file/directory names are not possible anyway, would it make \nsense to just return an error in this case?\n\n> +\n> +\tif (check_refname_format(name->buf, REFNAME_ALLOW_ONELEVEL))\n> +\t\tBUG(\"worktree name '%s' is not a valid refname\", name->buf);\n> +}\n> +\n> static int add_worktree(const char *path, const char *refname,\n> \t\t\tconst struct add_opts *opts)\n> {\n> @@ -275,6 +315,7 @@ static int add_worktree(const char *path, const \n> char *refname,\n> \tstruct strbuf symref = STRBUF_INIT;\n> \tstruct commit *commit = NULL;\n> \tint is_branch = 0;\n> +\tstruct strbuf sb_name = STRBUF_INIT;\n> \n> \tvalidate_worktree_add(path, opts);\n> \n> @@ -290,7 +331,10 @@ static int add_worktree(const char *path, const \n> char *refname,\n> \t\tdie(_(\"invalid reference: %s\"), refname);\n> \n> \tname = worktree_basename(path, &len);\n> -\tgit_path_buf(&sb_repo, \"worktrees/%.*s\", (int)(path + len - name), \n> name);\n> +\tstrbuf_add(&sb_name, name, path + len - name);\n> +\tsanitize_worktree_name(&sb_name);\n> +\tname = sb_name.buf;\n> +\tgit_path_buf(&sb_repo, \"worktrees/%s\", name);\n> \tlen = sb_repo.len;\n> \tif (safe_create_leading_directories_const(sb_repo.buf))\n> \t\tdie_errno(_(\"could not create leading directories of '%s'\"),\n> @@ -415,6 +459,7 @@ static int add_worktree(const char *path, const \n> char *refname,\n> \tstrbuf_release(&symref);\n> \tstrbuf_release(&sb_repo);\n> \tstrbuf_release(&sb_git);\n> +\tstrbuf_release(&sb_name);\n> \treturn ret;\n> }\n> \n> diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh\n> index 286bba35d8..0d465adb54 100755\n> --- a/t/t2025-worktree-add.sh\n> +++ b/t/t2025-worktree-add.sh\n> @@ -570,4 +570,9 @@ test_expect_success '\"add\" an existing locked but \n> missing worktree' '\n> \tgit worktree add --force --force --detach gnoo\n> '\n> \n> +test_expect_success 'sanitize generated worktree name' '\n> +\tgit worktree add --detach \". weird*..?.lock\" &&\n> +\ttest -d .git/worktrees/weird\n> +'\n> +\n> test_done\n> --\n> 2.21.0.rc1.337.gdf7f8d0522\n> \n\n\n"},{"id":"369794","messageId":"CACsJy8AERM==LunYTszUf1Fb-uHPZLjkSE5x1T=0Ueqsvq3F_A@mail.gmail.com","threadId":"50533","inReplyTo":"1550748525.30307.1@yandex.ru","subject":"Re: [PATCH] worktree add: sanitize worktree names","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-02-21T11:38:46Z","receivedAt":"2019-02-21T11:39:15Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Feb 21, 2019 at 6:28 PM Konstantin Kharlamov wrote:\n>\n>\n>\n> On Чт, Feb 21, 2019 at 2:00 PM,\n> =?UTF-8?b?Tmd1eeG7hW4gVGjDoWkgTmfhu41j?= Duy wrote:\n> > Worktree names are based on $(basename $GIT_WORK_TREE). They aren't\n> > significant until 3a3b9d8cde (refs: new ref types to make per-worktree\n> > refs visible to all worktrees - 2018-10-21), where worktree name could\n> > be part of a refname and must follow refname rules.\n> >\n> > Update 'worktree add' code to remove special characters to follow\n> > these rules. The code could replace chars with '-' more than\n> > necessary, but it keeps the code simple. In the future the user will\n> > be able to specify the worktree name by themselves if they're not\n> > happy with this dumb character substitution.\n> >\n> > Reported-by: hi-angel@yandex.ru\n> > Signed-off-by: Nguyễn Thái Ngọc Duy \n> > ---\n> > builtin/worktree.c | 47\n> > ++++++++++++++++++++++++++++++++++++++++-\n> > t/t2025-worktree-add.sh | 5 +++++\n> > 2 files changed, 51 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/builtin/worktree.c b/builtin/worktree.c\n> > index 3f9907fcc9..ff36838a33 100644\n> > --- a/builtin/worktree.c\n> > +++ b/builtin/worktree.c\n> > @@ -262,6 +262,46 @@ static void validate_worktree_add(const char\n> > *path, const struct add_opts *opts)\n> > free_worktrees(worktrees);\n> > }\n> >\n> > +/*\n> > + * worktree name is part of refname and has to pass\n> > + * check_refname_component(). Remove unallowed characters to make it\n> > + * valid.\n> > + */\n> > +static void sanitize_worktree_name(struct strbuf *name)\n> > +{\n> > + int i;\n> > +\n> > + /* no ending with .lock */\n> > + if (ends_with(name->buf, \".lock\"))\n> > + strbuf_remove(name, name->len - strlen(\".lock\"),\n> > + strlen(\".lock\"));\n> > +\n> > + /*\n> > + * All special chars replaced with dashes. See\n> > + * check_refname_component() for reference.\n> > + */\n> > + for (i = 0; i < name->len; i++) {\n> > + if (strchr(\":?[]\\\\~ \\t@{}*/.\", name->buf[i]))\n> > + name->buf[i] = '-';\n> > + }\n> > +\n> > + /* remove consecutive dashes, leading or trailing dashes */\n> > + for (i = 0; i < name->len; i++) {\n> > + while (name->buf[i] == '-' &&\n> > + (i == 0 ||\n> > + i == name->len - 1 ||\n> > + (i < name->len - 1 && name->buf[i + 1] == '-')))\n> > + strbuf_remove(name, i, 1);\n> > + }\n> > +\n> > + /* last resort, should never ever happen in practice */\n> > + if (name->len == 0)\n> > + strbuf_addstr(name, \"worktree\");\n>\n> I assume this means a user have passed a zero-sized worktree name? But\n> zero-sized file/directory names are not possible anyway, would it make\n> sense to just return an error in this case?\n\nIt could happen if you do \"git worktree add .lock\". The \".lock\" part\nwill be stripped out, leaving us with an empty string.\n-- \nDuy\n"},{"id":"369795","messageId":"1550749488.30307.2@yandex.ru","threadId":"50533","inReplyTo":"CACsJy8AERM==LunYTszUf1Fb-uHPZLjkSE5x1T=0Ueqsvq3F_A@mail.gmail.com","subject":"Re: [PATCH] worktree add: sanitize worktree names","fromName":"Konstantin Kharlamov","fromEmail":"hi-angel@yandex.ru","sentAt":"2019-02-21T11:44:48Z","receivedAt":"2019-02-21T11:51:57Z","isPatch":true,"sender":{"key":"hi-angel@yandex.ru","avatar":null},"body":"\n\nOn Чт, Feb 21, 2019 at 2:38 PM, Duy Nguyen wrote:\n> On Thu, Feb 21, 2019 at 6:28 PM Konstantin Kharlamov \n> wrote:\n>> \n>> \n>> \n>> On Чт, Feb 21, 2019 at 2:00 PM,\n>> =?UTF-8?b?Tmd1eeG7hW4gVGjDoWkgTmfhu41j?= Duy \n>> wrote:\n>> > Worktree names are based on $(basename $GIT_WORK_TREE). They \n>> aren't\n>> > significant until 3a3b9d8cde (refs: new ref types to make \n>> per-worktree\n>> > refs visible to all worktrees - 2018-10-21), where worktree name \n>> could\n>> > be part of a refname and must follow refname rules.\n>> >\n>> > Update 'worktree add' code to remove special characters to follow\n>> > these rules. The code could replace chars with '-' more than\n>> > necessary, but it keeps the code simple. In the future the user \n>> will\n>> > be able to specify the worktree name by themselves if they're not\n>> > happy with this dumb character substitution.\n>> >\n>> > Reported-by: hi-angel@yandex.ru\n>> > Signed-off-by: Nguyễn Thái Ngọc Duy \n>> > ---\n>> > builtin/worktree.c | 47\n>> > ++++++++++++++++++++++++++++++++++++++++-\n>> > t/t2025-worktree-add.sh | 5 +++++\n>> > 2 files changed, 51 insertions(+), 1 deletion(-)\n>> >\n>> > diff --git a/builtin/worktree.c b/builtin/worktree.c\n>> > index 3f9907fcc9..ff36838a33 100644\n>> > --- a/builtin/worktree.c\n>> > +++ b/builtin/worktree.c\n>> > @@ -262,6 +262,46 @@ static void validate_worktree_add(const char\n>> > *path, const struct add_opts *opts)\n>> > free_worktrees(worktrees);\n>> > }\n>> >\n>> > +/*\n>> > + * worktree name is part of refname and has to pass\n>> > + * check_refname_component(). Remove unallowed characters to \n>> make it\n>> > + * valid.\n>> > + */\n>> > +static void sanitize_worktree_name(struct strbuf *name)\n>> > +{\n>> > + int i;\n>> > +\n>> > + /* no ending with .lock */\n>> > + if (ends_with(name->buf, \".lock\"))\n>> > + strbuf_remove(name, name->len - strlen(\".lock\"),\n>> > + strlen(\".lock\"));\n>> > +\n>> > + /*\n>> > + * All special chars replaced with dashes. See\n>> > + * check_refname_component() for reference.\n>> > + */\n>> > + for (i = 0; i < name->len; i++) {\n>> > + if (strchr(\":?[]\\\\~ \\t@{}*/.\", name->buf[i]))\n>> > + name->buf[i] = '-';\n>> > + }\n>> > +\n>> > + /* remove consecutive dashes, leading or trailing dashes */\n>> > + for (i = 0; i < name->len; i++) {\n>> > + while (name->buf[i] == '-' &&\n>> > + (i == 0 ||\n>> > + i == name->len - 1 ||\n>> > + (i < name->len - 1 && name->buf[i + 1] == \n>> '-')))\n>> > + strbuf_remove(name, i, 1);\n>> > + }\n>> > +\n>> > + /* last resort, should never ever happen in practice */\n>> > + if (name->len == 0)\n>> > + strbuf_addstr(name, \"worktree\");\n>> \n>> I assume this means a user have passed a zero-sized worktree name? \n>> But\n>> zero-sized file/directory names are not possible anyway, would it \n>> make\n>> sense to just return an error in this case?\n> \n> It could happen if you do \"git worktree add .lock\". The \".lock\" part\n> will be stripped out, leaving us with an empty string.\n\nAh, I see. Then, would it maybe make sense to just sanitize the \".lock\" \nout the same way as you did with special symbols, i.e. with dashes?\n\n(I am not a git developer, so not sure if that's a good question, but I \nwould also question why \".lock\" needs to be deleted. I guess git uses \nthe postfix internally, but why can't it be okay with \"name.lock.lock\")\n\n\n"},{"id":"369796","messageId":"CACsJy8CWH-b18uaRvn-bXdsRbn+6QnJ6GNekqG2khGeJUa8S3w@mail.gmail.com","threadId":"50533","inReplyTo":"1550749488.30307.2@yandex.ru","subject":"Re: [PATCH] worktree add: sanitize worktree names","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-02-21T11:52:05Z","receivedAt":"2019-02-21T11:52:34Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Feb 21, 2019 at 6:44 PM Konstantin Kharlamov wrote:\n> >> > +\n> >> > + /* last resort, should never ever happen in practice */\n> >> > + if (name->len == 0)\n> >> > + strbuf_addstr(name, \"worktree\");\n> >>\n> >> I assume this means a user have passed a zero-sized worktree name?\n> >> But\n> >> zero-sized file/directory names are not possible anyway, would it\n> >> make\n> >> sense to just return an error in this case?\n> >\n> > It could happen if you do \"git worktree add .lock\". The \".lock\" part\n> > will be stripped out, leaving us with an empty string.\n>\n> Ah, I see. Then, would it maybe make sense to just sanitize the \".lock\"\n> out the same way as you did with special symbols, i.e. with dashes?\n\nHmm.. I actually did not think of that. Then we could return the error\nif \"name\" is empty.\n\n> (I am not a git developer, so not sure if that's a good question, but I\n> would also question why \".lock\" needs to be deleted. I guess git uses\n\nIt's because \"foo.lock\" is usually a temporary file to prepare things\nbefore we do an atomic update to \"foo\". And the \"refs guys\" were just\nbeing careful when they designed reference names so they reject any\nreference names that end with .lock. You can try to create a branch\nnamed something.lock, it will not go through. This is actually\ndocumented in \"git help check-ref-format\".\n\n> the postfix internally, but why can't it be okay with \"name.lock.lock\")\n\nUh oh I miss this case. I only delete \".lock\" once, \"name.lock\" would\nstill be rejected. Thanks for noticing.\n-- \nDuy\n"},{"id":"369797","messageId":"20190221121943.19778-1-pclouds@gmail.com","threadId":"50533","inReplyTo":"20190221110026.23135-1-pclouds@gmail.com","subject":"[PATCH v2 0/1] worktree add: sanitize worktree names","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2019-02-21T12:19:42Z","receivedAt":"2019-02-21T12:20:32Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"v2 fixes bad \".lock\" handling in v1. I keep the \"name->len == 0\" part\nthough because I found another valid case that could end up there.\n\nNguyễn Thái Ngọc Duy (1):\n worktree add: sanitize worktree names\n\n builtin/worktree.c | 51 ++++++++++++++++++++++++++++++++++++++++-\n t/t2025-worktree-add.sh | 7 ++++++\n 2 files changed, 57 insertions(+), 1 deletion(-)\n\nRange-diff dựa trên v1:\n1: 42a3144874 ! 1: d1b6e1c55b worktree add: sanitize worktree names\n @@ -13,7 +13,7 @@\n be able to specify the worktree name by themselves if they're not\n happy with this dumb character substitution.\n \n - Reported-by: hi-angel@yandex.ru\n + Reported-by: Konstantin Kharlamov \n Signed-off-by: Nguyễn Thái Ngọc Duy \n \n diff --git a/builtin/worktree.c b/builtin/worktree.c\n @@ -30,16 +30,14 @@\n + */\n +static void sanitize_worktree_name(struct strbuf *name)\n +{\n ++\tchar *orig_name = xstrdup(name->buf);\n +\tint i;\n +\n -+\t/* no ending with .lock */\n -+\tif (ends_with(name->buf, \".lock\"))\n -+\t\tstrbuf_remove(name, name->len - strlen(\".lock\"),\n -+\t\t\t strlen(\".lock\"));\n -+\n +\t/*\n +\t * All special chars replaced with dashes. See\n +\t * check_refname_component() for reference.\n ++\t * Note that .lock is also turned to -lock, removing its\n ++\t * special status.\n +\t */\n +\tfor (i = 0; i < name->len; i++) {\n +\t\tif (strchr(\":?[]\\\\~ \\t@{}*/.\", name->buf[i]))\n @@ -55,12 +53,18 @@\n +\t\t\tstrbuf_remove(name, i, 1);\n +\t}\n +\n -+\t/* last resort, should never ever happen in practice */\n ++\t/*\n ++\t * a worktree name of only special chars would be reduced to\n ++\t * an empty string\n ++\t */\n +\tif (name->len == 0)\n +\t\tstrbuf_addstr(name, \"worktree\");\n +\n +\tif (check_refname_format(name->buf, REFNAME_ALLOW_ONELEVEL))\n -+\t\tBUG(\"worktree name '%s' is not a valid refname\", name->buf);\n ++\t\tBUG(\"worktree name '%s' (from '%s') is not a valid refname\",\n ++\t\t name->buf, orig_name);\n ++\n ++\tfree(orig_name);\n +}\n +\n static int add_worktree(const char *path, const char *refname,\n @@ -103,8 +107,10 @@\n '\n \n +test_expect_success 'sanitize generated worktree name' '\n -+\tgit worktree add --detach \". weird*..?.lock\" &&\n -+\ttest -d .git/worktrees/weird\n ++\tgit worktree add --detach \". weird*..?.lock.lock\" &&\n ++\ttest -d .git/worktrees/weird-lock-lock &&\n ++\tgit worktree add --detach .... &&\n ++\ttest -d .git/worktrees/worktree\n +'\n +\n test_done\n-- \n2.21.0.rc1.337.gdf7f8d0522\n\n"},{"id":"369798","messageId":"20190221121943.19778-2-pclouds@gmail.com","threadId":"50533","inReplyTo":"20190221121943.19778-1-pclouds@gmail.com","subject":"[PATCH v2 1/1] worktree add: sanitize worktree names","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2019-02-21T12:19:43Z","receivedAt":"2019-02-21T12:20:37Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Worktree names are based on $(basename $GIT_WORK_TREE). They aren't\nsignificant until 3a3b9d8cde (refs: new ref types to make per-worktree\nrefs visible to all worktrees - 2018-10-21), where worktree name could\nbe part of a refname and must follow refname rules.\n\nUpdate 'worktree add' code to remove special characters to follow\nthese rules. The code could replace chars with '-' more than\nnecessary, but it keeps the code simple. In the future the user will\nbe able to specify the worktree name by themselves if they're not\nhappy with this dumb character substitution.\n\nReported-by: Konstantin Kharlamov \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/worktree.c | 51 ++++++++++++++++++++++++++++++++++++++++-\n t/t2025-worktree-add.sh | 7 ++++++\n 2 files changed, 57 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 3f9907fcc9..53e41db229 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -262,6 +262,50 @@ static void validate_worktree_add(const char *path, const struct add_opts *opts)\n \tfree_worktrees(worktrees);\n }\n \n+/*\n+ * worktree name is part of refname and has to pass\n+ * check_refname_component(). Remove unallowed characters to make it\n+ * valid.\n+ */\n+static void sanitize_worktree_name(struct strbuf *name)\n+{\n+\tchar *orig_name = xstrdup(name->buf);\n+\tint i;\n+\n+\t/*\n+\t * All special chars replaced with dashes. See\n+\t * check_refname_component() for reference.\n+\t * Note that .lock is also turned to -lock, removing its\n+\t * special status.\n+\t */\n+\tfor (i = 0; i < name->len; i++) {\n+\t\tif (strchr(\":?[]\\\\~ \\t@{}*/.\", name->buf[i]))\n+\t\t\tname->buf[i] = '-';\n+\t}\n+\n+\t/* remove consecutive dashes, leading or trailing dashes */\n+\tfor (i = 0; i < name->len; i++) {\n+\t\twhile (name->buf[i] == '-' &&\n+\t\t (i == 0 ||\n+\t\t\ti == name->len - 1 ||\n+\t\t\t(i < name->len - 1 && name->buf[i + 1] == '-')))\n+\t\t\tstrbuf_remove(name, i, 1);\n+\t}\n+\n+\t/*\n+\t * a worktree name of only special chars would be reduced to\n+\t * an empty string\n+\t */\n+\tif (name->len == 0)\n+\t\tstrbuf_addstr(name, \"worktree\");\n+\n+\tif (check_refname_format(name->buf, REFNAME_ALLOW_ONELEVEL))\n+\t\tBUG(\"worktree name '%s' (from '%s') is not a valid refname\",\n+\t\t name->buf, orig_name);\n+\n+\tfree(orig_name);\n+}\n+\n static int add_worktree(const char *path, const char *refname,\n \t\t\tconst struct add_opts *opts)\n {\n@@ -275,6 +319,7 @@ static int add_worktree(const char *path, const char *refname,\n \tstruct strbuf symref = STRBUF_INIT;\n \tstruct commit *commit = NULL;\n \tint is_branch = 0;\n+\tstruct strbuf sb_name = STRBUF_INIT;\n \n \tvalidate_worktree_add(path, opts);\n \n@@ -290,7 +335,10 @@ static int add_worktree(const char *path, const char *refname,\n \t\tdie(_(\"invalid reference: %s\"), refname);\n \n \tname = worktree_basename(path, &len);\n-\tgit_path_buf(&sb_repo, \"worktrees/%.*s\", (int)(path + len - name), name);\n+\tstrbuf_add(&sb_name, name, path + len - name);\n+\tsanitize_worktree_name(&sb_name);\n+\tname = sb_name.buf;\n+\tgit_path_buf(&sb_repo, \"worktrees/%s\", name);\n \tlen = sb_repo.len;\n \tif (safe_create_leading_directories_const(sb_repo.buf))\n \t\tdie_errno(_(\"could not create leading directories of '%s'\"),\n@@ -415,6 +463,7 @@ static int add_worktree(const char *path, const char *refname,\n \tstrbuf_release(&symref);\n \tstrbuf_release(&sb_repo);\n \tstrbuf_release(&sb_git);\n+\tstrbuf_release(&sb_name);\n \treturn ret;\n }\n \ndiff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh\nindex 286bba35d8..71aa6ab9c1 100755\n--- a/t/t2025-worktree-add.sh\n+++ b/t/t2025-worktree-add.sh\n@@ -570,4 +570,11 @@ test_expect_success '\"add\" an existing locked but missing worktree' '\n \tgit worktree add --force --force --detach gnoo\n '\n \n+test_expect_success 'sanitize generated worktree name' '\n+\tgit worktree add --detach \". weird*..?.lock.lock\" &&\n+\ttest -d .git/worktrees/weird-lock-lock &&\n+\tgit worktree add --detach .... &&\n+\ttest -d .git/worktrees/worktree\n+'\n+\n test_done\n-- \n2.21.0.rc1.337.gdf7f8d0522\n\n"},{"id":"369805","messageId":"20190221132210.GC20536@sigill.intra.peff.net","threadId":"50533","inReplyTo":"20190221121943.19778-2-pclouds@gmail.com","subject":"Re: [PATCH v2 1/1] worktree add: sanitize worktree names","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-02-21T13:22:10Z","receivedAt":"2019-02-21T13:22:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 21, 2019 at 07:19:43PM +0700, Nguyễn Thái Ngọc Duy wrote:\n\n> +/*\n> + * worktree name is part of refname and has to pass\n> + * check_refname_component(). Remove unallowed characters to make it\n> + * valid.\n> + */\n> +static void sanitize_worktree_name(struct strbuf *name)\n> +{\n> +\tchar *orig_name = xstrdup(name->buf);\n> +\tint i;\n> +\n> +\t/*\n> +\t * All special chars replaced with dashes. See\n> +\t * check_refname_component() for reference.\n> +\t * Note that .lock is also turned to -lock, removing its\n> +\t * special status.\n> +\t */\n> +\tfor (i = 0; i < name->len; i++) {\n> +\t\tif (strchr(\":?[]\\\\~ \\t@{}*/.\", name->buf[i]))\n> +\t\t\tname->buf[i] = '-';\n> +\t}\n\nThis is reject-known-bad, but I think there are still some other\ncharacters that are not allowed in refnames (e.g., ASCII control\ncharacters). Which would lead to us hitting the BUG() below.\n\nIt might make sense to provide access to refname_disposition() and use\nit here. Alternatively, I think if we did an allow-known-good, it might\nbe OK to have a slightly more restrictive scheme (say, alnum plus\ndashes, plus high-bit chars).\n\n> +\t/* remove consecutive dashes, leading or trailing dashes */\n> +\tfor (i = 0; i < name->len; i++) {\n> +\t\twhile (name->buf[i] == '-' &&\n> +\t\t (i == 0 ||\n> +\t\t\ti == name->len - 1 ||\n> +\t\t\t(i < name->len - 1 && name->buf[i + 1] == '-')))\n> +\t\t\tstrbuf_remove(name, i, 1);\n> +\t}\n\nI think this is correct, though it is possibly to be quadratic in the\nstring length due to the O(n) remove. I think this kind of sanitizing is\nmore readable if done between two strings rather than in-place, like:\n\n for (i = 0; i < name->len; i++) {\n\tif (is_allowed(name->buf[i])) {\n\t\tstrbuf_addch(&dest, name->buf[i]);\n\t\tlast_was_dash = 0;\n\t} else if (!last_was_dash && dest->len)\n\t\tstrbuf_addch(&dest, '-');\n\t\tlast_was_dash = 1;\n\t}\n }\n /* still must handle removal from end of stray \"-\" and \".lock\" */\n strbuf_swap(name, &dest);\n strbuf_release(&dest);\n\nbut that may just be personal preference. I'm OK with it if you prefer\nthe in-place way.\n\n-Peff\n"},{"id":"369806","messageId":"20190221132306.GD20536@sigill.intra.peff.net","threadId":"50533","inReplyTo":"CACsJy8CWH-b18uaRvn-bXdsRbn+6QnJ6GNekqG2khGeJUa8S3w@mail.gmail.com","subject":"Re: [PATCH] worktree add: sanitize worktree names","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-02-21T13:23:06Z","receivedAt":"2019-02-21T13:23:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 21, 2019 at 06:52:05PM +0700, Duy Nguyen wrote:\n\n> > the postfix internally, but why can't it be okay with \"name.lock.lock\")\n> \n> Uh oh I miss this case. I only delete \".lock\" once, \"name.lock\" would\n> still be rejected. Thanks for noticing.\n\nAnother tricky case is \"refs/heads/foo.lock/bar.lock\", which would need\nboth \".lock\"s removed. I think your v2 handles this correctly, though\n(because it disallows \".\" entirely).\n\n-Peff\n"},{"id":"369826","messageId":"6fe399f0-98ad-37e6-f4b1-3a3f6e4bce03@ramsayjones.plus.com","threadId":"50533","inReplyTo":"20190221121943.19778-2-pclouds@gmail.com","subject":"Re: [PATCH v2 1/1] worktree add: sanitize worktree names","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2019-02-21T17:41:53Z","receivedAt":"2019-02-21T17:41:58Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 21/02/2019 12:19, Nguyễn Thái Ngọc Duy wrote:\n> Worktree names are based on $(basename $GIT_WORK_TREE). They aren't\n> significant until 3a3b9d8cde (refs: new ref types to make per-worktree\n> refs visible to all worktrees - 2018-10-21), where worktree name could\n> be part of a refname and must follow refname rules.\n> \n> Update 'worktree add' code to remove special characters to follow\n> these rules. The code could replace chars with '-' more than\n> necessary, but it keeps the code simple. In the future the user will\n> be able to specify the worktree name by themselves if they're not\n> happy with this dumb character substitution.\n> \n> Reported-by: Konstantin Kharlamov \n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> builtin/worktree.c | 51 ++++++++++++++++++++++++++++++++++++++++-\n> t/t2025-worktree-add.sh | 7 ++++++\n> 2 files changed, 57 insertions(+), 1 deletion(-)\n> \n> diff --git a/builtin/worktree.c b/builtin/worktree.c\n> index 3f9907fcc9..53e41db229 100644\n> --- a/builtin/worktree.c\n> +++ b/builtin/worktree.c\n> @@ -262,6 +262,50 @@ static void validate_worktree_add(const char *path, const struct add_opts *opts)\n> \tfree_worktrees(worktrees);\n> }\n> \n> +/*\n> + * worktree name is part of refname and has to pass\n> + * check_refname_component(). Remove unallowed characters to make it\n> + * valid.\n> + */\n> +static void sanitize_worktree_name(struct strbuf *name)\n> +{\n> +\tchar *orig_name = xstrdup(name->buf);\n> +\tint i;\n> +\n> +\t/*\n> +\t * All special chars replaced with dashes. See\n> +\t * check_refname_component() for reference.\n> +\t * Note that .lock is also turned to -lock, removing its\n> +\t * special status.\n> +\t */\n> +\tfor (i = 0; i < name->len; i++) {\n> +\t\tif (strchr(\":?[]\\\\~ \\t@{}*/.\", name->buf[i]))\n> +\t\t\tname->buf[i] = '-';\n> +\t}\n> +\n> +\t/* remove consecutive dashes, leading or trailing dashes */\n\nWhy? So, '[fred]' will be 'sanitized' to 'fred' (rather than '-fred-'),\nwhich would increase the chance of a 'collision' with the 'fred'\nworktree (not very likely, but still). Is that useful? How about\n'x86_64-*-gnu' which now becomes 'x86_64-gnu'?\n \n> +\tfor (i = 0; i < name->len; i++) {\n> +\t\twhile (name->buf[i] == '-' &&\n> +\t\t (i == 0 ||\n> +\t\t\ti == name->len - 1 ||\n> +\t\t\t(i < name->len - 1 && name->buf[i + 1] == '-')))\n> +\t\t\tstrbuf_remove(name, i, 1);\n> +\t}\n> +\n> +\t/*\n> +\t * a worktree name of only special chars would be reduced to\n> +\t * an empty string\n> +\t */> +\tif (name->len == 0)\n> +\t\tstrbuf_addstr(name, \"worktree\");\n\nIf you didn't 'collapse' the name above, you could check for\nan empty name at the top and wouldn't need this (presumably\nan empty name would not be valid).\n\nATB,\nRamsay Jones\n"},{"id":"369892","messageId":"CACsJy8Dw0y7qX7jgNH4j_e4YOg-vv0OVDQv99AzT-koZ=Fq-TQ@mail.gmail.com","threadId":"50533","inReplyTo":"6fe399f0-98ad-37e6-f4b1-3a3f6e4bce03@ramsayjones.plus.com","subject":"Re: [PATCH v2 1/1] worktree add: sanitize worktree names","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-02-22T09:21:16Z","receivedAt":"2019-02-22T09:21:45Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Feb 22, 2019 at 12:42 AM Ramsay Jones\n wrote:\n> > +static void sanitize_worktree_name(struct strbuf *name)\n> > +{\n> > + char *orig_name = xstrdup(name->buf);\n> > + int i;\n> > +\n> > + /*\n> > + * All special chars replaced with dashes. See\n> > + * check_refname_component() for reference.\n> > + * Note that .lock is also turned to -lock, removing its\n> > + * special status.\n> > + */\n> > + for (i = 0; i < name->len; i++) {\n> > + if (strchr(\":?[]\\\\~ \\t@{}*/.\", name->buf[i]))\n> > + name->buf[i] = '-';\n> > + }\n> > +\n> > + /* remove consecutive dashes, leading or trailing dashes */\n>\n> Why? So, '[fred]' will be 'sanitized' to 'fred' (rather than '-fred-'),\n> which would increase the chance of a 'collision' with the 'fred'\n> worktree (not very likely, but still). Is that useful? How about\n> 'x86_64-*-gnu' which now becomes 'x86_64-gnu'?\n\nIt is useful when you want to specify HEAD of [fred] for example.\nWriting worktrees/fred/HEAD is a bit better than\nworktrees/-fred-/HEAD. I haven't done it yet, but these names will be\nshown in \"git worktree list\" too and lots of dashes does not improve\nreadability. Collision is not a problem because if fred is taken, the\nfinal name would be fred1 or fred.\n\nIf you're really bothered with this, you will be able to specify the\nname you want (you can't, yet). You still have to pass the valid\nrefname check, but you have a lot more flexibility.\n\nSo this code only needs to work mostly ok for the common case and I\ncould go either way, clean up consecutive dashes or not. I suppose\nsimpler code would be the tie breaker.\n-- \nDuy\n"},{"id":"370215","messageId":"20190226105851.32273-1-pclouds@gmail.com","threadId":"50533","inReplyTo":"20190221121943.19778-1-pclouds@gmail.com","subject":"[PATCH v3 0/1] worktree add: sanitize worktree names","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2019-02-26T10:58:50Z","receivedAt":"2019-02-26T10:59:03Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"v3 is rewritten to use refname_disposition[] to cover all invalid\nchars.\n\nNguyễn Thái Ngọc Duy (1):\n worktree add: sanitize worktree names\n\n builtin/worktree.c | 37 ++++++++++++++++++++++++++++++++++++-\n refs.c | 6 ++++++\n refs.h | 1 +\n t/t2025-worktree-add.sh | 7 +++++++\n 4 files changed, 50 insertions(+), 1 deletion(-)\n\n-- \n2.21.0.rc1.337.gdf7f8d0522\n\n"},{"id":"370216","messageId":"20190226105851.32273-2-pclouds@gmail.com","threadId":"50533","inReplyTo":"20190226105851.32273-1-pclouds@gmail.com","subject":"[PATCH v3 1/1] worktree add: sanitize worktree names","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2019-02-26T10:58:51Z","receivedAt":"2019-02-26T10:59:08Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Worktree names are based on $(basename $GIT_WORK_TREE). They aren't\nsignificant until 3a3b9d8cde (refs: new ref types to make per-worktree\nrefs visible to all worktrees - 2018-10-21), where worktree name could\nbe part of a refname and must follow refname rules.\n\nUpdate 'worktree add' code to remove special characters to follow\nthese rules. The code could replace chars with '-' more than\nnecessary, but it keeps the code simple. In the future the user will\nbe able to specify the worktree name by themselves if they're not\nhappy with this dumb character substitution.\n\nReported-by: Konstantin Kharlamov \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/worktree.c | 37 ++++++++++++++++++++++++++++++++++++-\n refs.c | 6 ++++++\n refs.h | 1 +\n t/t2025-worktree-add.sh | 7 +++++++\n 4 files changed, 50 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 3f9907fcc9..21469eb52c 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -262,6 +262,36 @@ static void validate_worktree_add(const char *path, const struct add_opts *opts)\n \tfree_worktrees(worktrees);\n }\n \n+static void sanitize_worktree_name(struct strbuf *name)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tint i;\n+\n+\tfor (i = 0; i < name->len; i++) {\n+\t\tint ch = name->buf[i];\n+\n+\t\tif (char_allowed_in_refname(ch))\n+\t\t\tstrbuf_addch(&sb, ch);\n+\t\telse if (sb.len > 0 && sb.buf[sb.len - 1] != '-')\n+\t\t\tstrbuf_addch(&sb, '-');\n+\t}\n+\tif (sb.len > 0 && sb.buf[sb.len - 1] == '-')\n+\t\tstrbuf_setlen(&sb, sb.len - 1);\n+\t/*\n+\t * a worktree name of only special chars would be reduced to\n+\t * an empty string\n+\t */\n+\tif (sb.len == 0)\n+\t\tstrbuf_addstr(&sb, \"worktree\");\n+\n+\tif (check_refname_format(sb.buf, REFNAME_ALLOW_ONELEVEL))\n+\t\tBUG(\"worktree name '%s' (from '%s') is not a valid refname\",\n+\t\t sb.buf, name->buf);\n+\n+\tstrbuf_swap(&sb, name);\n+\tstrbuf_release(&sb);\n+}\n+\n static int add_worktree(const char *path, const char *refname,\n \t\t\tconst struct add_opts *opts)\n {\n@@ -275,6 +305,7 @@ static int add_worktree(const char *path, const char *refname,\n \tstruct strbuf symref = STRBUF_INIT;\n \tstruct commit *commit = NULL;\n \tint is_branch = 0;\n+\tstruct strbuf sb_name = STRBUF_INIT;\n \n \tvalidate_worktree_add(path, opts);\n \n@@ -290,7 +321,10 @@ static int add_worktree(const char *path, const char *refname,\n \t\tdie(_(\"invalid reference: %s\"), refname);\n \n \tname = worktree_basename(path, &len);\n-\tgit_path_buf(&sb_repo, \"worktrees/%.*s\", (int)(path + len - name), name);\n+\tstrbuf_add(&sb_name, name, path + len - name);\n+\tsanitize_worktree_name(&sb_name);\n+\tname = sb_name.buf;\n+\tgit_path_buf(&sb_repo, \"worktrees/%s\", name);\n \tlen = sb_repo.len;\n \tif (safe_create_leading_directories_const(sb_repo.buf))\n \t\tdie_errno(_(\"could not create leading directories of '%s'\"),\n@@ -415,6 +449,7 @@ static int add_worktree(const char *path, const char *refname,\n \tstrbuf_release(&symref);\n \tstrbuf_release(&sb_repo);\n \tstrbuf_release(&sb_git);\n+\tstrbuf_release(&sb_name);\n \treturn ret;\n }\n \ndiff --git a/refs.c b/refs.c\nindex 142888a40a..f23f583db1 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -57,6 +57,12 @@ static unsigned char refname_disposition[256] = {\n \t0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 3, 0, 0, 4, 4\n };\n \n+int char_allowed_in_refname(int ch)\n+{\n+\treturn 0 <= ch && ch < ARRAY_SIZE(refname_disposition) &&\n+\t\trefname_disposition[ch] == 0;\n+}\n+\n /*\n * Try to read one refname component from the front of refname.\n * Return the length of the component found, or -1 if the component is\ndiff --git a/refs.h b/refs.h\nindex 308fa1f03b..61b4073f76 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -459,6 +459,7 @@ int for_each_reflog(each_ref_fn fn, void *cb_data);\n * repeated slashes are accepted.\n */\n int check_refname_format(const char *refname, int flags);\n+int char_allowed_in_refname(int ch);\n \n const char *prettify_refname(const char *refname);\n \ndiff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh\nindex 286bba35d8..ea22207361 100755\n--- a/t/t2025-worktree-add.sh\n+++ b/t/t2025-worktree-add.sh\n@@ -570,4 +570,11 @@ test_expect_success '\"add\" an existing locked but missing worktree' '\n \tgit worktree add --force --force --detach gnoo\n '\n \n+test_expect_success 'sanitize generated worktree name' '\n+\tgit worktree add --detach \". weird*..?.lock.lock.\" &&\n+\ttest -d .git/worktrees/weird-lock-lock &&\n+\tgit worktree add --detach .... &&\n+\ttest -d .git/worktrees/worktree\n+'\n+\n test_done\n-- \n2.21.0.rc1.337.gdf7f8d0522\n\n"},{"id":"370299","messageId":"20190227120859.GB10305@sigill.intra.peff.net","threadId":"50533","inReplyTo":"20190226105851.32273-2-pclouds@gmail.com","subject":"Re: [PATCH v3 1/1] worktree add: sanitize worktree names","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-02-27T12:08:59Z","receivedAt":"2019-02-27T12:09:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 26, 2019 at 05:58:51PM +0700, Nguyễn Thái Ngọc Duy wrote:\n\n> Worktree names are based on $(basename $GIT_WORK_TREE). They aren't\n> significant until 3a3b9d8cde (refs: new ref types to make per-worktree\n> refs visible to all worktrees - 2018-10-21), where worktree name could\n> be part of a refname and must follow refname rules.\n> \n> Update 'worktree add' code to remove special characters to follow\n> these rules. The code could replace chars with '-' more than\n> necessary, but it keeps the code simple. In the future the user will\n> be able to specify the worktree name by themselves if they're not\n> happy with this dumb character substitution.\n\nSo notably this gets around \"..\" and \".lock\" by just disallowing \".\"\nentirely. I think I'm OK with that for worktrees. It does make me a\nlittle nervous to see this new public function, though:\n\n> +int char_allowed_in_refname(int ch)\n> +{\n> +\treturn 0 <= ch && ch < ARRAY_SIZE(refname_disposition) &&\n> +\t\trefname_disposition[ch] == 0;\n> +}\n\nbecause it's not entirely accurate, as you noted above. I wonder if we\ncould name this differently to warn people that the refname rules are\nnot so simple.\n\nIf we just cared about saying \"is this worktree name valid\", I'd suggest\nactually constructing a sample refname with the worktree name embedded\nin it and feeding that to check_refname_format(). But because you want\nto actually sanitize, I don't think there's an easy way to reuse it.\n\nSo this approach is probably the best we can do, though I do still think\nit's worth renaming that function (and/or putting a big warning comment\nin front of it).\n\nOther than that, I didn't see anything objectionable in the patch.\n\n-Peff\n"},{"id":"370308","messageId":"CAPig+cRJZBvwsptPOzx3oPSOnt6+uGLoyOr_JbUnku4kdSwdgA@mail.gmail.com","threadId":"50533","inReplyTo":"20190227120859.GB10305@sigill.intra.peff.net","subject":"Re: [PATCH v3 1/1] worktree add: sanitize worktree names","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-02-27T14:23:33Z","receivedAt":"2019-02-27T14:23:46Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Feb 27, 2019 at 7:09 AM Jeff King wrote:\n> On Tue, Feb 26, 2019 at 05:58:51PM +0700, Nguyễn Thái Ngọc Duy wrote:\n> > Update 'worktree add' code to remove special characters to follow\n> > these rules. The code could replace chars with '-' more than\n> > necessary, but it keeps the code simple. In the future the user will\n> > be able to specify the worktree name by themselves if they're not\n> > happy with this dumb character substitution.\n>\n> So notably this gets around \"..\" and \".lock\" by just disallowing \".\"\n> entirely. I think I'm OK with that for worktrees. It does make me a\n> little nervous to see this new public function, though:\n>\n> > +int char_allowed_in_refname(int ch) [...]\n>\n> because it's not entirely accurate, as you noted above. I wonder if we\n> could name this differently to warn people that the refname rules are\n> not so simple.\n>\n> If we just cared about saying \"is this worktree name valid\", I'd suggest\n> actually constructing a sample refname with the worktree name embedded\n> in it and feeding that to check_refname_format(). But because you want\n> to actually sanitize, I don't think there's an easy way to reuse it.\n>\n> So this approach is probably the best we can do, though I do still think\n> it's worth renaming that function (and/or putting a big warning comment\n> in front of it).\n\nThe above arguments seem to suggest the introduction of a companion to\ncheck_refname_format() for sanitizing, perhaps named\nsanitize_refname_format(), in ref.[hc]. The potential difficulty with\nthat is defining exactly what \"sanitize\" means. Will it be contextual?\n(That is, will git-worktree have differently sanitation needs than\nsome other facility?) If so, perhaps a 'flags' argument could control\nhow sanitization is done.\n"},{"id":"370317","messageId":"20190227160457.GA30817@sigill.intra.peff.net","threadId":"50533","inReplyTo":"CAPig+cRJZBvwsptPOzx3oPSOnt6+uGLoyOr_JbUnku4kdSwdgA@mail.gmail.com","subject":"Re: [PATCH v3 1/1] worktree add: sanitize worktree names","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-02-27T16:04:57Z","receivedAt":"2019-02-27T16:05:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 27, 2019 at 09:23:33AM -0500, Eric Sunshine wrote:\n\n> > If we just cared about saying \"is this worktree name valid\", I'd suggest\n> > actually constructing a sample refname with the worktree name embedded\n> > in it and feeding that to check_refname_format(). But because you want\n> > to actually sanitize, I don't think there's an easy way to reuse it.\n> >\n> > So this approach is probably the best we can do, though I do still think\n> > it's worth renaming that function (and/or putting a big warning comment\n> > in front of it).\n> \n> The above arguments seem to suggest the introduction of a companion to\n> check_refname_format() for sanitizing, perhaps named\n> sanitize_refname_format(), in ref.[hc]. The potential difficulty with\n> that is defining exactly what \"sanitize\" means. Will it be contextual?\n> (That is, will git-worktree have differently sanitation needs than\n> some other facility?) If so, perhaps a 'flags' argument could control\n> how sanitization is done.\n\nI agree that sanitize_refname_format() would be nice, but I'm pretty\nsure it's going to end up having to duplicate many of the rules from\ncheck_refname_format(). Which is ugly if the two ever get out of sync.\n\nBut if we could write it in a way that keeps the actual policy logic in\none factored-out portion, I think it would be worth doing.\n\n-Peff\n"},{"id":"370489","messageId":"xmqqmumc4uri.fsf@gitster-ct.c.googlers.com","threadId":"50533","inReplyTo":"20190227160457.GA30817@sigill.intra.peff.net","subject":"Re: [PATCH v3 1/1] worktree add: sanitize worktree names","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-03T01:22:09Z","receivedAt":"2019-03-03T01:22:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I agree that sanitize_refname_format() would be nice, but I'm pretty\n> sure it's going to end up having to duplicate many of the rules from\n> check_refname_format(). Which is ugly if the two ever get out of sync.\n>\n> But if we could write it in a way that keeps the actual policy logic in\n> one factored-out portion, I think it would be worth doing.\n\nYeah, I do too.\n\nIn the meantime, let's call v3 sufficient improvement from the\ncurrent state for now and queue it.\n"},{"id":"370590","messageId":"CACsJy8D0o6-ihNcpmfhCfQPNo-t2i=NySp65Y8h2e3md2GvXVw@mail.gmail.com","threadId":"50533","inReplyTo":"20190227160457.GA30817@sigill.intra.peff.net","subject":"Re: [PATCH v3 1/1] worktree add: sanitize worktree names","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-03-04T11:19:15Z","receivedAt":"2019-03-04T11:19:44Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Feb 27, 2019 at 11:05 PM Jeff King wrote:\n>\n> On Wed, Feb 27, 2019 at 09:23:33AM -0500, Eric Sunshine wrote:\n>\n> > > If we just cared about saying \"is this worktree name valid\", I'd suggest\n> > > actually constructing a sample refname with the worktree name embedded\n> > > in it and feeding that to check_refname_format(). But because you want\n> > > to actually sanitize, I don't think there's an easy way to reuse it.\n> > >\n> > > So this approach is probably the best we can do, though I do still think\n> > > it's worth renaming that function (and/or putting a big warning comment\n> > > in front of it).\n> >\n> > The above arguments seem to suggest the introduction of a companion to\n> > check_refname_format() for sanitizing, perhaps named\n> > sanitize_refname_format(), in ref.[hc]. The potential difficulty with\n> > that is defining exactly what \"sanitize\" means. Will it be contextual?\n> > (That is, will git-worktree have differently sanitation needs than\n> > some other facility?) If so, perhaps a 'flags' argument could control\n> > how sanitization is done.\n>\n> I agree that sanitize_refname_format() would be nice, but I'm pretty\n> sure it's going to end up having to duplicate many of the rules from\n> check_refname_format(). Which is ugly if the two ever get out of sync.\n>\n> But if we could write it in a way that keeps the actual policy logic in\n> one factored-out portion, I think it would be worth doing.\n\nI think we could make check_refname_format() returns the bad position\nand several different error codes depending on context. Then\nsanitize_.. can just repeatedly call check_refname_format and fix up\nwhatever error it reports. Performance goes straight to hell but I\ndon't think that's a big deal for git-worktree, and it keeps\ncheck_refname_format() simple (relatively speaking).\n\nThe second option is make check_refname_format() call some callback\ninstead of returning error. This allows sanitize_ to fix up in one go\n(inside the callback), but check_refname_format could be a lot uglier,\nand verifying all refs (I think pack-refs does this?) could also be\nslowed down.\n-- \nDuy\n"},{"id":"370591","messageId":"20190304120424.GA7966@ash","threadId":"50533","inReplyTo":"CACsJy8D0o6-ihNcpmfhCfQPNo-t2i=NySp65Y8h2e3md2GvXVw@mail.gmail.com","subject":"Re: [PATCH v3 1/1] worktree add: sanitize worktree names","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-03-04T12:04:24Z","receivedAt":"2019-03-04T12:04:33Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Mar 04, 2019 at 06:19:15PM +0700, Duy Nguyen wrote:\n> On Wed, Feb 27, 2019 at 11:05 PM Jeff King wrote:\n> >\n> > On Wed, Feb 27, 2019 at 09:23:33AM -0500, Eric Sunshine wrote:\n> >\n> > > > If we just cared about saying \"is this worktree name valid\", I'd suggest\n> > > > actually constructing a sample refname with the worktree name embedded\n> > > > in it and feeding that to check_refname_format(). But because you want\n> > > > to actually sanitize, I don't think there's an easy way to reuse it.\n> > > >\n> > > > So this approach is probably the best we can do, though I do still think\n> > > > it's worth renaming that function (and/or putting a big warning comment\n> > > > in front of it).\n> > >\n> > > The above arguments seem to suggest the introduction of a companion to\n> > > check_refname_format() for sanitizing, perhaps named\n> > > sanitize_refname_format(), in ref.[hc]. The potential difficulty with\n> > > that is defining exactly what \"sanitize\" means. Will it be contextual?\n> > > (That is, will git-worktree have differently sanitation needs than\n> > > some other facility?) If so, perhaps a 'flags' argument could control\n> > > how sanitization is done.\n> >\n> > I agree that sanitize_refname_format() would be nice, but I'm pretty\n> > sure it's going to end up having to duplicate many of the rules from\n> > check_refname_format(). Which is ugly if the two ever get out of sync.\n> >\n> > But if we could write it in a way that keeps the actual policy logic in\n> > one factored-out portion, I think it would be worth doing.\n> \n> I think we could make check_refname_format() returns the bad position\n> and several different error codes depending on context. Then\n> sanitize_.. can just repeatedly call check_refname_format and fix up\n> whatever error it reports. Performance goes straight to hell but I\n> don't think that's a big deal for git-worktree, and it keeps\n> check_refname_format() simple (relatively speaking).\n\nThe new refs.c code would look something like this.\ndo_check_refname_component() does not look so bad.\n\n-- 8< --\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 21469eb52c..ca63dd3df6 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -262,36 +262,6 @@ static void validate_worktree_add(const char *path, const struct add_opts *opts)\n \tfree_worktrees(worktrees);\n }\n \n-static void sanitize_worktree_name(struct strbuf *name)\n-{\n-\tstruct strbuf sb = STRBUF_INIT;\n-\tint i;\n-\n-\tfor (i = 0; i < name->len; i++) {\n-\t\tint ch = name->buf[i];\n-\n-\t\tif (char_allowed_in_refname(ch))\n-\t\t\tstrbuf_addch(&sb, ch);\n-\t\telse if (sb.len > 0 && sb.buf[sb.len - 1] != '-')\n-\t\t\tstrbuf_addch(&sb, '-');\n-\t}\n-\tif (sb.len > 0 && sb.buf[sb.len - 1] == '-')\n-\t\tstrbuf_setlen(&sb, sb.len - 1);\n-\t/*\n-\t * a worktree name of only special chars would be reduced to\n-\t * an empty string\n-\t */\n-\tif (sb.len == 0)\n-\t\tstrbuf_addstr(&sb, \"worktree\");\n-\n-\tif (check_refname_format(sb.buf, REFNAME_ALLOW_ONELEVEL))\n-\t\tBUG(\"worktree name '%s' (from '%s') is not a valid refname\",\n-\t\t sb.buf, name->buf);\n-\n-\tstrbuf_swap(&sb, name);\n-\tstrbuf_release(&sb);\n-}\n-\n static int add_worktree(const char *path, const char *refname,\n \t\t\tconst struct add_opts *opts)\n {\n@@ -322,7 +292,7 @@ static int add_worktree(const char *path, const char *refname,\n \n \tname = worktree_basename(path, &len);\n \tstrbuf_add(&sb_name, name, path + len - name);\n-\tsanitize_worktree_name(&sb_name);\n+\tsanitize_worktree_refname(&sb_name);\n \tname = sb_name.buf;\n \tgit_path_buf(&sb_repo, \"worktrees/%s\", name);\n \tlen = sb_repo.len;\ndiff --git a/refs.c b/refs.c\nindex f23f583db1..2d9730e792 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -63,6 +63,17 @@ int char_allowed_in_refname(int ch)\n \t\trefname_disposition[ch] == 0;\n }\n \n+enum check_code {\n+\t refname_ok = 0,\n+\t refname_contains_dotdot,\n+\t refname_contains_atopen,\n+\t refname_has_badchar,\n+\t refname_contains_wildcard,\n+\t refname_starts_with_dot,\n+\t refname_ends_with_dotlock,\n+\t refname_component_has_zero_length\n+};\n+\n /*\n * Try to read one refname component from the front of refname.\n * Return the length of the component found, or -1 if the component is\n@@ -78,10 +89,11 @@ int char_allowed_in_refname(int ch)\n * - it ends with \".lock\", or\n * - it contains a \"@{\" portion\n */\n-static int check_refname_component(const char *refname, int *flags)\n+static enum check_code do_check_refname_component(const char *refname, int *flags, const char **cp_out)\n {\n \tconst char *cp;\n \tchar last = '\\0';\n+\tenum check_code ret = refname_ok;\n \n \tfor (cp = refname; ; cp++) {\n \t\tint ch = *cp & 255;\n@@ -90,18 +102,26 @@ static int check_refname_component(const char *refname, int *flags)\n \t\tcase 1:\n \t\t\tgoto out;\n \t\tcase 2:\n-\t\t\tif (last == '.')\n-\t\t\t\treturn -1; /* Refname contains \"..\". */\n+\t\t\tif (last == '.') {\n+\t\t\t\tret = refname_contains_dotdot;\n+\t\t\t\tgoto done;\n+\t\t\t}\n \t\t\tbreak;\n \t\tcase 3:\n-\t\t\tif (last == '@')\n-\t\t\t\treturn -1; /* Refname contains \"@{\". */\n+\t\t\tif (last == '@') {\n+\t\t\t\tret = refname_contains_atopen; /* @{ */\n+\t\t\t\tgoto done;\n+\t\t\t}\n \t\t\tbreak;\n \t\tcase 4:\n-\t\t\treturn -1;\n+\t\t\tret = refname_has_badchar;\n+\t\t\tgoto done;\n \t\tcase 5:\n-\t\t\tif (!(*flags & REFNAME_REFSPEC_PATTERN))\n-\t\t\t\treturn -1; /* refspec can't be a pattern */\n+\t\t\tif (!(*flags & REFNAME_REFSPEC_PATTERN)) {\n+\t\t\t\t/* refspec can't be a pattern */\n+\t\t\t\tret = refname_contains_wildcard;\n+\t\t\t\tgoto done;\n+\t\t\t}\n \n \t\t\t/*\n \t\t\t * Unset the pattern flag so that we only accept\n@@ -113,16 +133,67 @@ static int check_refname_component(const char *refname, int *flags)\n \t\tlast = ch;\n \t}\n out:\n-\tif (cp == refname)\n-\t\treturn 0; /* Component has zero length. */\n-\tif (refname[0] == '.')\n-\t\treturn -1; /* Component starts with '.'. */\n+\tif (cp == refname) {\n+\t\tret = refname_component_has_zero_length;\n+\t\tgoto done;\n+\t}\n+\tif (refname[0] == '.') {\n+\t\tret = refname_starts_with_dot;\n+\t\tcp = refname;\n+\t\tgoto done;\n+\t}\n \tif (cp - refname >= LOCK_SUFFIX_LEN &&\n-\t !memcmp(cp - LOCK_SUFFIX_LEN, LOCK_SUFFIX, LOCK_SUFFIX_LEN))\n-\t\treturn -1; /* Refname ends with \".lock\". */\n+\t !memcmp(cp - LOCK_SUFFIX_LEN, LOCK_SUFFIX, LOCK_SUFFIX_LEN)) {\n+\t\tcp -= LOCK_SUFFIX_LEN;\n+\t\tret = refname_ends_with_dotlock;\n+\t}\n+done:\n+\t*cp_out = cp;\n+\treturn ret;\n+}\n+\n+static int check_refname_component(const char *refname, int *flags)\n+{\n+\tconst char *cp;\n+\tenum check_code ret;\n+\n+\tret = do_check_refname_component(refname, flags, &cp);\n+\tif (ret)\n+\t\treturn -1;\n \treturn cp - refname;\n }\n \n+void sanitize_worktree_refname(struct strbuf *name)\n+{\n+\tint last_length = -1;\n+\tint flags = 0;\n+\n+\twhile (1) {\n+\t\tconst char *cp;\n+\n+\t\tenum check_code ret = do_check_refname_component(name->buf, &flags, &cp);\n+\t\tif (last_length != -1 && cp - name->buf == last_length)\n+\t\t\tBUG(\"stuck in infinite loop! pos = %d buf = %s\",\n+\t\t\t last_length, name->buf);\n+\t\tlast_length = cp - name->buf;\n+\t\tswitch (ret) {\n+\t\tcase refname_ok:\n+\t\t\treturn;\n+\t\tcase refname_contains_dotdot:\n+\t\tcase refname_contains_atopen:\n+\t\tcase refname_has_badchar:\n+\t\tcase refname_contains_wildcard:\n+\t\tcase refname_ends_with_dotlock:\n+\t\tcase refname_starts_with_dot:\n+\t\t\tname->buf[last_length] = '-';\n+\t\t\tbreak;\n+\t\tcase refname_component_has_zero_length:\n+\t\t\tstrbuf_addstr(name, \"worktree\");\n+\t\t\treturn;\n+\t\t}\n+\t}\n+}\n+\n int check_refname_format(const char *refname, int flags)\n {\n \tint component_len, component_count = 0;\ndiff --git a/refs.h b/refs.h\nindex 61b4073f76..3b65b8d27a 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -459,7 +459,7 @@ int for_each_reflog(each_ref_fn fn, void *cb_data);\n * repeated slashes are accepted.\n */\n int check_refname_format(const char *refname, int flags);\n-int char_allowed_in_refname(int ch);\n+void sanitize_worktree_refname(struct strbuf *name);\n \n const char *prettify_refname(const char *refname);\n \ndiff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh\nindex ea22207361..24c574f365 100755\n--- a/t/t2025-worktree-add.sh\n+++ b/t/t2025-worktree-add.sh\n@@ -571,10 +571,10 @@ test_expect_success '\"add\" an existing locked but missing worktree' '\n '\n \n test_expect_success 'sanitize generated worktree name' '\n-\tgit worktree add --detach \". weird*..?.lock.lock.\" &&\n-\ttest -d .git/worktrees/weird-lock-lock &&\n+\tgit worktree add --detach \". weird*..?.lock.lock\" &&\n+\ttest -d .git/worktrees/---weird-.--.lock-lock &&\n \tgit worktree add --detach .... &&\n-\ttest -d .git/worktrees/worktree\n+\ttest -d .git/worktrees/--.-\n '\n \n test_done\n-- 8< --\n\n--\nDuy\n"},{"id":"370608","messageId":"nycvar.QRO.7.76.6.1903041603320.45@tvgsbejvaqbjf.bet","threadId":"50533","inReplyTo":"20190226105851.32273-2-pclouds@gmail.com","subject":"Re: [PATCH v3 1/1] worktree add: sanitize worktree names","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-03-04T15:06:18Z","receivedAt":"2019-03-04T15:06:29Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Duy,\n\nOn Tue, 26 Feb 2019, Nguyễn Thái Ngọc Duy wrote:\n\n> diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh\n> index 286bba35d8..ea22207361 100755\n> --- a/t/t2025-worktree-add.sh\n> +++ b/t/t2025-worktree-add.sh\n> @@ -570,4 +570,11 @@ test_expect_success '\"add\" an existing locked but missing worktree' '\n> \tgit worktree add --force --force --detach gnoo\n> '\n> \n> +test_expect_success 'sanitize generated worktree name' '\n> +\tgit worktree add --detach \". weird*..?.lock.lock.\" &&\n> +\ttest -d .git/worktrees/weird-lock-lock &&\n> +\tgit worktree add --detach .... &&\n> +\ttest -d .git/worktrees/worktree\n> +'\n> +\n> test_done\n\nYou probably missed that this added test fails on Windows:\n\nhttps://dev.azure.com/gitgitgadget/git/_build/results?buildId=3782&view=logs\n\nThe reason is that you use a \"funny name\" which cannot be represented on\nevery filesystem.\n\nPlease use the FUNNYNAMES prerequisite (or introduce another one, if you\nare uncomfortable with using a test whether tabs are valid in filenames to\nindiciate whether wildcard characters are valid in filenames).\n\nCiao,\nJohannes"},{"id":"370678","messageId":"20190305120834.7284-1-pclouds@gmail.com","threadId":"50533","inReplyTo":"20190226105851.32273-1-pclouds@gmail.com","subject":"[PATCH v4 0/2] worktree add: sanitize worktree names","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2019-03-05T12:08:32Z","receivedAt":"2019-03-05T12:08:46Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"v4 refactors check_refname_component() so that we could do more accurate\nsubstitution (and leave fewer traps).\n\nPerformance of sanitize_worktree_refname() goes back to horrible\nagain. But since it's not really a big deal (no body is going to add\n200 worktrees per second), I don't feel like we should optimize it.\nThat may involve removing the for loop in do_check_refname_component()\nand making things uglier.\n\nThe test is also updated to have FUNNYNAMES prerequisite, which is\nalways unset on Windows. This should fix the breakage there.\n\nNguyễn Thái Ngọc Duy (2):\n refs.c: refactor check_refname_component()\n worktree add: sanitize worktree names\n\n builtin/worktree.c | 7 ++-\n refs.c | 114 ++++++++++++++++++++++++++++++++++------\n refs.h | 1 +\n t/t2025-worktree-add.sh | 5 ++\n 4 files changed, 110 insertions(+), 17 deletions(-)\n\n-- \n2.21.0.rc1.337.gdf7f8d0522\n\n"},{"id":"370679","messageId":"20190305120834.7284-2-pclouds@gmail.com","threadId":"50533","inReplyTo":"20190305120834.7284-1-pclouds@gmail.com","subject":"[PATCH v4 1/2] refs.c: refactor check_refname_component()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2019-03-05T12:08:33Z","receivedAt":"2019-03-05T12:08:53Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"The core logic of this function is factored out to provide more\ninformation when the refname is invalid: what part that is and exact\nwhat is wrong with it. This will be useful for a coming function that\nhas to turn a string into a valid refname component.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n refs.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++------------\n 1 file changed, 62 insertions(+), 16 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 142888a40a..70c55ea1b6 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -57,10 +57,21 @@ static unsigned char refname_disposition[256] = {\n \t0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 3, 0, 0, 4, 4\n };\n \n+enum refname_check_code {\n+\t refname_ok = 0,\n+\t refname_contains_dotdot,\n+\t refname_contains_atopen,\n+\t refname_has_badchar,\n+\t refname_contains_wildcard,\n+\t refname_starts_with_dot,\n+\t refname_ends_with_dotlock,\n+\t refname_component_has_zero_length\n+};\n+\n /*\n * Try to read one refname component from the front of refname.\n- * Return the length of the component found, or -1 if the component is\n- * not legal. It is legal if it is something reasonable to have under\n+ * If the component is legal, return the end of the component in cp_out.\n+ * It is legal if it is something reasonable to have under\n * \".git/refs/\"; We do not like it if:\n *\n * - any path component of it begins with \".\", or\n@@ -71,11 +82,15 @@ static unsigned char refname_disposition[256] = {\n * - it ends with a \"/\", or\n * - it ends with \".lock\", or\n * - it contains a \"@{\" portion\n+ *\n+ * in which case cp_out points to the beginning of the illegal part.\n */\n-static int check_refname_component(const char *refname, int *flags)\n+static enum refname_check_code do_check_refname_component(\n+\tconst char *refname, int *flags, const char **cp_out)\n {\n \tconst char *cp;\n \tchar last = '\\0';\n+\tenum refname_check_code ret = refname_ok;\n \n \tfor (cp = refname; ; cp++) {\n \t\tint ch = *cp & 255;\n@@ -84,18 +99,28 @@ static int check_refname_component(const char *refname, int *flags)\n \t\tcase 1:\n \t\t\tgoto out;\n \t\tcase 2:\n-\t\t\tif (last == '.')\n-\t\t\t\treturn -1; /* Refname contains \"..\". */\n+\t\t\tif (last == '.') {\n+\t\t\t\tcp--;\n+\t\t\t\tret = refname_contains_dotdot;\n+\t\t\t\tgoto done;\n+\t\t\t}\n \t\t\tbreak;\n \t\tcase 3:\n-\t\t\tif (last == '@')\n-\t\t\t\treturn -1; /* Refname contains \"@{\". */\n+\t\t\tif (last == '@') {\n+\t\t\t\tcp--;\n+\t\t\t\tret = refname_contains_atopen; /* @{ */\n+\t\t\t\tgoto done;\n+\t\t\t}\n \t\t\tbreak;\n \t\tcase 4:\n-\t\t\treturn -1;\n+\t\t\tret = refname_has_badchar;\n+\t\t\tgoto done;\n \t\tcase 5:\n-\t\t\tif (!(*flags & REFNAME_REFSPEC_PATTERN))\n-\t\t\t\treturn -1; /* refspec can't be a pattern */\n+\t\t\tif (!(*flags & REFNAME_REFSPEC_PATTERN)) {\n+\t\t\t\t/* refspec can't be a pattern */\n+\t\t\t\tret = refname_contains_wildcard;\n+\t\t\t\tgoto done;\n+\t\t\t}\n \n \t\t\t/*\n \t\t\t * Unset the pattern flag so that we only accept\n@@ -107,13 +132,34 @@ static int check_refname_component(const char *refname, int *flags)\n \t\tlast = ch;\n \t}\n out:\n-\tif (cp == refname)\n-\t\treturn 0; /* Component has zero length. */\n-\tif (refname[0] == '.')\n-\t\treturn -1; /* Component starts with '.'. */\n+\tif (cp == refname) {\n+\t\tret = refname_component_has_zero_length;\n+\t\tgoto done;\n+\t}\n+\tif (refname[0] == '.') {\n+\t\tcp = refname;\n+\t\tret = refname_starts_with_dot;\n+\t\tgoto done;\n+\t}\n \tif (cp - refname >= LOCK_SUFFIX_LEN &&\n-\t !memcmp(cp - LOCK_SUFFIX_LEN, LOCK_SUFFIX, LOCK_SUFFIX_LEN))\n-\t\treturn -1; /* Refname ends with \".lock\". */\n+\t !memcmp(cp - LOCK_SUFFIX_LEN, LOCK_SUFFIX, LOCK_SUFFIX_LEN)) {\n+\t\tcp -= LOCK_SUFFIX_LEN;\n+\t\tret = refname_ends_with_dotlock;\n+\t}\n+done:\n+\t*cp_out = cp;\n+\treturn ret;\n+}\n+\n+/* Return the length of the component if it's legal otherwise -1 */\n+static int check_refname_component(const char *refname, int *flags)\n+{\n+\tconst char *cp;\n+\tenum refname_check_code ret;\n+\n+\tret = do_check_refname_component(refname, flags, &cp);\n+\tif (ret)\n+\t\treturn -1;\n \treturn cp - refname;\n }\n \n-- \n2.21.0.rc1.337.gdf7f8d0522\n\n"},{"id":"370680","messageId":"20190305120834.7284-3-pclouds@gmail.com","threadId":"50533","inReplyTo":"20190305120834.7284-1-pclouds@gmail.com","subject":"[PATCH v4 2/2] worktree add: sanitize worktree names","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2019-03-05T12:08:34Z","receivedAt":"2019-03-05T12:08:59Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Worktree names are based on $(basename $GIT_WORK_TREE). They aren't\nsignificant until 3a3b9d8cde (refs: new ref types to make per-worktree\nrefs visible to all worktrees - 2018-10-21), where worktree name could\nbe part of a refname and must follow refname rules.\n\nUpdate 'worktree add' code to remove special characters to follow these\nrules. In the future the user will be able to specify the worktree name\nby themselves if they're not happy with this dumb character\nsubstitution.\n\nReported-by: Konstantin Kharlamov \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/worktree.c | 7 ++++++-\n refs.c | 36 ++++++++++++++++++++++++++++++++++++\n refs.h | 1 +\n t/t2025-worktree-add.sh | 5 +++++\n 4 files changed, 48 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 3f9907fcc9..ca63dd3df6 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -275,6 +275,7 @@ static int add_worktree(const char *path, const char *refname,\n \tstruct strbuf symref = STRBUF_INIT;\n \tstruct commit *commit = NULL;\n \tint is_branch = 0;\n+\tstruct strbuf sb_name = STRBUF_INIT;\n \n \tvalidate_worktree_add(path, opts);\n \n@@ -290,7 +291,10 @@ static int add_worktree(const char *path, const char *refname,\n \t\tdie(_(\"invalid reference: %s\"), refname);\n \n \tname = worktree_basename(path, &len);\n-\tgit_path_buf(&sb_repo, \"worktrees/%.*s\", (int)(path + len - name), name);\n+\tstrbuf_add(&sb_name, name, path + len - name);\n+\tsanitize_worktree_refname(&sb_name);\n+\tname = sb_name.buf;\n+\tgit_path_buf(&sb_repo, \"worktrees/%s\", name);\n \tlen = sb_repo.len;\n \tif (safe_create_leading_directories_const(sb_repo.buf))\n \t\tdie_errno(_(\"could not create leading directories of '%s'\"),\n@@ -415,6 +419,7 @@ static int add_worktree(const char *path, const char *refname,\n \tstrbuf_release(&symref);\n \tstrbuf_release(&sb_repo);\n \tstrbuf_release(&sb_git);\n+\tstrbuf_release(&sb_name);\n \treturn ret;\n }\n \ndiff --git a/refs.c b/refs.c\nindex 70c55ea1b6..a23a84e431 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -163,6 +163,42 @@ static int check_refname_component(const char *refname, int *flags)\n \treturn cp - refname;\n }\n \n+void sanitize_worktree_refname(struct strbuf *name)\n+{\n+\tint flags = 0, i, max_tries;\n+\tconst char *cp;\n+\tenum refname_check_code ret;\n+\n+\t/*\n+\t * name->len should be enough because we should never need to\n+\t * substitute any position more than once, but let's just add\n+\t * a couple more to be on the safe side.\n+\t */\n+\tmax_tries = name->len + 10;\n+\tfor (i = 0; i < max_tries; i++) {\n+\t\tret = do_check_refname_component(name->buf, &flags, &cp);\n+\t\tswitch (ret) {\n+\t\tcase refname_ok:\n+\t\t\tstrbuf_setlen(name, cp - name->buf);\n+\t\t\treturn;\n+\n+\t\tcase refname_component_has_zero_length:\n+\t\t\tstrbuf_addstr(name, \"worktree\");\n+\t\t\treturn;\n+\n+\t\tcase refname_contains_dotdot:\n+\t\tcase refname_contains_atopen:\n+\t\tcase refname_has_badchar:\n+\t\tcase refname_contains_wildcard:\n+\t\tcase refname_ends_with_dotlock:\n+\t\tcase refname_starts_with_dot:\n+\t\t\t*(char *)cp = '-';\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+\tBUG(\"stuck in infinite loop! buf = %s\", name->buf);\n+}\n+\n int check_refname_format(const char *refname, int flags)\n {\n \tint component_len, component_count = 0;\ndiff --git a/refs.h b/refs.h\nindex 308fa1f03b..3b65b8d27a 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -459,6 +459,7 @@ int for_each_reflog(each_ref_fn fn, void *cb_data);\n * repeated slashes are accepted.\n */\n int check_refname_format(const char *refname, int flags);\n+void sanitize_worktree_refname(struct strbuf *name);\n \n const char *prettify_refname(const char *refname);\n \ndiff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh\nindex 286bba35d8..6e2b90c84f 100755\n--- a/t/t2025-worktree-add.sh\n+++ b/t/t2025-worktree-add.sh\n@@ -570,4 +570,9 @@ test_expect_success '\"add\" an existing locked but missing worktree' '\n \tgit worktree add --force --force --detach gnoo\n '\n \n+test_expect_success FUNNYNAMES 'sanitize generated worktree name' '\n+\tgit worktree add --detach \". weird*..?.lock.lock\" &&\n+\ttest -d .git/worktrees/---weird--.-.lock-lock\n+'\n+\n test_done\n-- \n2.21.0.rc1.337.gdf7f8d0522\n\n"},{"id":"370830","messageId":"20190306214912.GA32630@sigill.intra.peff.net","threadId":"50533","inReplyTo":"20190305120834.7284-2-pclouds@gmail.com","subject":"Re: [PATCH v4 1/2] refs.c: refactor check_refname_component()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-06T21:49:13Z","receivedAt":"2019-03-06T21:49:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 05, 2019 at 07:08:33PM +0700, Nguyễn Thái Ngọc Duy wrote:\n\n> @@ -71,11 +82,15 @@ static unsigned char refname_disposition[256] = {\n> * - it ends with a \"/\", or\n> * - it ends with \".lock\", or\n> * - it contains a \"@{\" portion\n> + *\n> + * in which case cp_out points to the beginning of the illegal part.\n> */\n> -static int check_refname_component(const char *refname, int *flags)\n> +static enum refname_check_code do_check_refname_component(\n> +\tconst char *refname, int *flags, const char **cp_out)\n\nHmm, OK, so we get to know what type of problem, but also the exact\ncharacter where we found it. And then we just keep mutating that char\nuntil we have something that passes.\n\nI can't think of any reason that wouldn't work. As you note, it's\npossibly quadratic, though that might be OK for our purposes.\n\nI had envisioned just sanitizing each character into an output buffer as\nwe did the checks. It does introduce some complexities, though, because\nnow the checking function is doing the replacement (so it has to know\nthe right sanitizing rule for each case).\n\nThe patch below is a rough cut at that, just for discussion. You can\nignore the check-ref-format bits; they were just to make poking at it\neasier, though perhaps we'd want something like that in the long run.\n\nI suspect check_refname_component() could be made a bit more readable by\nreordering a few bits. E.g., why do we check for a leading \".\" at the\n_end_, after having parsed the entire rest of the component for errors?\n\nI dunno. I think I can live with what you've got in your series, but I\nfigured I'd share this for the sake of completeness. If you really love\nit, feel free to adapt it.\n\ndiff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c\nindex bc67d3f0a8..41b5434be2 100644\n--- a/builtin/check-ref-format.c\n+++ b/builtin/check-ref-format.c\n@@ -56,6 +56,7 @@ int cmd_check_ref_format(int argc, const char **argv, const char *prefix)\n \tint i;\n \tint normalize = 0;\n \tint flags = 0;\n+\tint sanitize = 0;\n \tconst char *refname;\n \n \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n@@ -73,13 +74,22 @@ int cmd_check_ref_format(int argc, const char **argv, const char *prefix)\n \t\t\tflags &= ~REFNAME_ALLOW_ONELEVEL;\n \t\telse if (!strcmp(argv[i], \"--refspec-pattern\"))\n \t\t\tflags |= REFNAME_REFSPEC_PATTERN;\n+\t\telse if (!strcmp(argv[i], \"--sanitize\"))\n+\t\t\tsanitize = 1;\n \t\telse\n \t\t\tusage(builtin_check_ref_format_usage);\n \t}\n \tif (! (i == argc - 1))\n \t\tusage(builtin_check_ref_format_usage);\n \n \trefname = argv[i];\n+\tif (sanitize) {\n+\t\tstruct strbuf out = STRBUF_INIT;\n+\t\tsanitize_refname(refname, &out);\n+\t\tprintf(\"%s\\n\", out.buf);\n+\t\tstrbuf_release(&out);\n+\t\treturn 0;\n+\t}\n \tif (normalize)\n \t\trefname = collapse_slashes(refname);\n \tif (check_refname_format(refname, flags))\ndiff --git a/refs.c b/refs.c\nindex 142888a40a..2a0c0c6338 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -72,30 +72,58 @@ static unsigned char refname_disposition[256] = {\n * - it ends with \".lock\", or\n * - it contains a \"@{\" portion\n */\n-static int check_refname_component(const char *refname, int *flags)\n+static int check_refname_component(const char *refname, int *flags,\n+\t\t\t\t struct strbuf *sanitized)\n {\n \tconst char *cp;\n \tchar last = '\\0';\n+\tsize_t component_start;\n+\n+\tif (sanitized)\n+\t\tcomponent_start = sanitized->len;\n \n \tfor (cp = refname; ; cp++) {\n \t\tint ch = *cp & 255;\n \t\tunsigned char disp = refname_disposition[ch];\n+\n+\t\tif (sanitized && disp != 1)\n+\t\t\tstrbuf_addch(sanitized, ch);\n+\n \t\tswitch (disp) {\n \t\tcase 1:\n \t\t\tgoto out;\n \t\tcase 2:\n-\t\t\tif (last == '.')\n-\t\t\t\treturn -1; /* Refname contains \"..\". */\n+\t\t\tif (last == '.') {\n+\t\t\t\t/* Refname contains \"..\". */\n+\t\t\t\tif (sanitized)\n+\t\t\t\t\tsanitized->len--; /* collapse \"..\" to single \".\" */\n+\t\t\t\telse\n+\t\t\t\t\treturn -1;\n+\t\t\t}\n \t\t\tbreak;\n \t\tcase 3:\n-\t\t\tif (last == '@')\n-\t\t\t\treturn -1; /* Refname contains \"@{\". */\n+\t\t\tif (last == '@') {\n+\t\t\t\t/* Refname contains \"@{\". */\n+\t\t\t\tif (sanitized)\n+\t\t\t\t\tsanitized->buf[sanitized->len-1] = '-';\n+\t\t\t\telse\n+\t\t\t\t\treturn -1;\n+\t\t\t}\n \t\t\tbreak;\n \t\tcase 4:\n-\t\t\treturn -1;\n+\t\t\t/* forbidden char */\n+\t\t\tif (sanitized)\n+\t\t\t\tsanitized->buf[sanitized->len-1] = '-';\n+\t\t\telse\n+\t\t\t\treturn -1;\n+\t\t\tbreak;\n \t\tcase 5:\n-\t\t\tif (!(*flags & REFNAME_REFSPEC_PATTERN))\n-\t\t\t\treturn -1; /* refspec can't be a pattern */\n+\t\t\tif (!(*flags & REFNAME_REFSPEC_PATTERN)) {\n+\t\t\t\tif (sanitized)\n+\t\t\t\t\tsanitized->buf[sanitized->len-1] = '-';\n+\t\t\t\telse\n+\t\t\t\t\treturn -1; /* refspec can't be a pattern */\n+\t\t\t}\n \n \t\t\t/*\n \t\t\t * Unset the pattern flag so that we only accept\n@@ -109,26 +137,48 @@ static int check_refname_component(const char *refname, int *flags)\n out:\n \tif (cp == refname)\n \t\treturn 0; /* Component has zero length. */\n-\tif (refname[0] == '.')\n-\t\treturn -1; /* Component starts with '.'. */\n+\n+\tif (refname[0] == '.') {\n+\t\t/* Component starts with '.'. */\n+\t\tif (sanitized)\n+\t\t\tsanitized->buf[component_start] = '-';\n+\t\telse\n+\t\t\treturn -1;\n+\t}\n \tif (cp - refname >= LOCK_SUFFIX_LEN &&\n-\t !memcmp(cp - LOCK_SUFFIX_LEN, LOCK_SUFFIX, LOCK_SUFFIX_LEN))\n-\t\treturn -1; /* Refname ends with \".lock\". */\n+\t !memcmp(cp - LOCK_SUFFIX_LEN, LOCK_SUFFIX, LOCK_SUFFIX_LEN)) {\n+\t\t/* Refname ends with \".lock\". */\n+\t\tif (sanitized)\n+\t\t\tstrbuf_strip_suffix(sanitized, LOCK_SUFFIX);\n+\t\telse\n+\t\t\treturn -1;\n+\t}\n \treturn cp - refname;\n }\n \n-int check_refname_format(const char *refname, int flags)\n+static int check_or_sanitize_refname(const char *refname, int flags,\n+\t\t\t\t struct strbuf *sanitized)\n {\n \tint component_len, component_count = 0;\n \n-\tif (!strcmp(refname, \"@\"))\n+\tif (!strcmp(refname, \"@\")) {\n \t\t/* Refname is a single character '@'. */\n-\t\treturn -1;\n+\t\tif (sanitized)\n+\t\t\tstrbuf_addch(sanitized, '-');\n+\t\telse\n+\t\t\treturn -1;\n+\t}\n \n \twhile (1) {\n+\t\tif (sanitized && sanitized->len)\n+\t\t\tstrbuf_complete(sanitized, '/');\n+\n \t\t/* We are at the start of a path component. */\n-\t\tcomponent_len = check_refname_component(refname, &flags);\n-\t\tif (component_len <= 0)\n+\t\tcomponent_len = check_refname_component(refname, &flags,\n+\t\t\t\t\t\t\tsanitized);\n+\t\tif (sanitized && component_len == 0)\n+\t\t\t; /* OK, omit empty component */\n+\t\telse if (component_len <= 0)\n \t\t\treturn -1;\n \n \t\tcomponent_count++;\n@@ -138,13 +188,29 @@ int check_refname_format(const char *refname, int flags)\n \t\trefname += component_len + 1;\n \t}\n \n-\tif (refname[component_len - 1] == '.')\n-\t\treturn -1; /* Refname ends with '.'. */\n+\tif (refname[component_len - 1] == '.') {\n+\t\t/* Refname ends with '.'. */\n+\t\tif (sanitized)\n+\t\t\t; /* omit ending dot */\n+\t\telse\n+\t\t\treturn -1;\n+\t}\n \tif (!(flags & REFNAME_ALLOW_ONELEVEL) && component_count < 2)\n \t\treturn -1; /* Refname has only one component. */\n \treturn 0;\n }\n \n+int check_refname_format(const char *refname, int flags)\n+{\n+\treturn check_or_sanitize_refname(refname, flags, NULL);\n+}\n+\n+void sanitize_refname(const char *refname, struct strbuf *out)\n+{\n+\tif (check_or_sanitize_refname(refname, 0, out))\n+\t\tBUG(\"sanitizing refname check returned error\");\n+}\n+\n int refname_is_safe(const char *refname)\n {\n \tconst char *rest;\ndiff --git a/refs.h b/refs.h\nindex 308fa1f03b..b99c309dd9 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -460,6 +460,12 @@ int for_each_reflog(each_ref_fn fn, void *cb_data);\n */\n int check_refname_format(const char *refname, int flags);\n \n+/*\n+ * Apply the rules from check_refname_format, but mutate the result until it\n+ * is acceptable, and place the result in \"out\".\n+ */\n+void sanitize_refname(const char *refname, struct strbuf *out);\n+\n const char *prettify_refname(const char *refname);\n \n char *shorten_unambiguous_ref(const char *refname, int strict);\n"},{"id":"370928","messageId":"CAPig+cSra900Zz3zytZTCP=QJpRr0s19gFzng0kJ00T0T8pQ9Q@mail.gmail.com","threadId":"50533","inReplyTo":"20190306214912.GA32630@sigill.intra.peff.net","subject":"Re: [PATCH v4 1/2] refs.c: refactor check_refname_component()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-03-07T23:24:37Z","receivedAt":"2019-03-07T23:24:52Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Mar 6, 2019 at 4:49 PM Jeff King wrote:\n> On Tue, Mar 05, 2019 at 07:08:33PM +0700, Nguyễn Thái Ngọc Duy wrote:\n> I had envisioned just sanitizing each character into an output buffer as\n> we did the checks. It does introduce some complexities, though, because\n> now the checking function is doing the replacement (so it has to know\n> the right sanitizing rule for each case).\n>\n> The patch below is a rough cut at that, just for discussion. You can\n> ignore the check-ref-format bits; they were just to make poking at it\n> easier, though perhaps we'd want something like that in the long run.\n\nThis is more along the lines of what I had envisioned, as well, after\nlooking over the implementation of check_refname_component(). It's a\nbit noisy and loud but easy to follow, and doesn't give rise to\nconcerns about quadratic behavior, etc.\n"},{"id":"370945","messageId":"20190308092834.12549-1-pclouds@gmail.com","threadId":"50533","inReplyTo":"20190305120834.7284-1-pclouds@gmail.com","subject":"[PATCH v5 0/1] worktree add: sanitize worktree names","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2019-03-08T09:28:33Z","receivedAt":"2019-03-08T09:28:46Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"v5 is basically Jeff's version from one of the replies in v4, where\ncheck_refname_component is enhanced to optionally sanitize.\n\nI was reluctant to go this way because it makes check_refname_component\nmore complex (turns out still manageable) and burns worktree rules in\nit. But there may never be the second sanitization user, we deal with\nit when it comes.\n\nAs said, refs.c is pretty much Jeff's except two major changes:\n\n - handle foo.lock.lock correctly by stripping .lock repeatedly\n\n - sanitize refname _components_ instead of full refs. I could construct\n worktrees/ and pass to Jeff's sanitize_refname. But then I need\n to strip worktrees/ after that.\n\nI took credits so that bugs come to me first (then I'll blame him\nanyway while doing some evil laughs)\n\nNguyễn Thái Ngọc Duy (1):\n worktree add: sanitize worktree names\n\n builtin/worktree.c | 10 +++-\n refs.c | 103 ++++++++++++++++++++++++++++++++--------\n refs.h | 6 +++\n t/t2400-worktree-add.sh | 5 ++\n 4 files changed, 104 insertions(+), 20 deletions(-)\n\n-- \n2.21.0.rc1.337.gdf7f8d0522\n\n"},{"id":"370946","messageId":"20190308092834.12549-2-pclouds@gmail.com","threadId":"50533","inReplyTo":"20190308092834.12549-1-pclouds@gmail.com","subject":"[PATCH v5 1/1] worktree add: sanitize worktree names","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2019-03-08T09:28:34Z","receivedAt":"2019-03-08T09:28:51Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Worktree names are based on $(basename $GIT_WORK_TREE). They aren't\nsignificant until 3a3b9d8cde (refs: new ref types to make per-worktree\nrefs visible to all worktrees - 2018-10-21), where worktree name could\nbe part of a refname and must follow refname rules.\n\nUpdate 'worktree add' code to remove special characters to follow\nthese rules. In the future the user will be able to specify the\nworktree name by themselves if they're not happy with this dumb\ncharacter substitution.\n\nReported-by: Konstantin Kharlamov \nHelped-by: Jeff King \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/worktree.c | 10 +++-\n refs.c | 103 ++++++++++++++++++++++++++++++++--------\n refs.h | 6 +++\n t/t2400-worktree-add.sh | 5 ++\n 4 files changed, 104 insertions(+), 20 deletions(-)\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 6cc094a453..756cf3a417 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -275,6 +275,7 @@ static int add_worktree(const char *path, const char *refname,\n \tstruct strbuf symref = STRBUF_INIT;\n \tstruct commit *commit = NULL;\n \tint is_branch = 0;\n+\tstruct strbuf sb_name = STRBUF_INIT;\n \n \tvalidate_worktree_add(path, opts);\n \n@@ -290,7 +291,13 @@ static int add_worktree(const char *path, const char *refname,\n \t\tdie(_(\"invalid reference: %s\"), refname);\n \n \tname = worktree_basename(path, &len);\n-\tgit_path_buf(&sb_repo, \"worktrees/%.*s\", (int)(path + len - name), name);\n+\tstrbuf_add(&sb, name, path + len - name);\n+\tsanitize_refname_component(sb.buf, &sb_name);\n+\tif (!sb_name.len)\n+\t\tBUG(\"How come '%s' becomes empty after sanitization?\", sb.buf);\n+\tstrbuf_reset(&sb);\n+\tname = sb_name.buf;\n+\tgit_path_buf(&sb_repo, \"worktrees/%s\", name);\n \tlen = sb_repo.len;\n \tif (safe_create_leading_directories_const(sb_repo.buf))\n \t\tdie_errno(_(\"could not create leading directories of '%s'\"),\n@@ -416,6 +423,7 @@ static int add_worktree(const char *path, const char *refname,\n \tstrbuf_release(&symref);\n \tstrbuf_release(&sb_repo);\n \tstrbuf_release(&sb_git);\n+\tstrbuf_release(&sb_name);\n \treturn ret;\n }\n \ndiff --git a/refs.c b/refs.c\nindex 142888a40a..e9f83018f0 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -72,30 +72,57 @@ static unsigned char refname_disposition[256] = {\n * - it ends with \".lock\", or\n * - it contains a \"@{\" portion\n */\n-static int check_refname_component(const char *refname, int *flags)\n+static int check_refname_component(const char *refname, int *flags,\n+\t\t\t\t struct strbuf *sanitized)\n {\n \tconst char *cp;\n \tchar last = '\\0';\n+\tsize_t component_start;\n+\n+\tif (sanitized)\n+\t\tcomponent_start = sanitized->len;\n \n \tfor (cp = refname; ; cp++) {\n \t\tint ch = *cp & 255;\n \t\tunsigned char disp = refname_disposition[ch];\n+\n+\t\tif (sanitized && disp != 1)\n+\t\t\tstrbuf_addch(sanitized, ch);\n+\n \t\tswitch (disp) {\n \t\tcase 1:\n \t\t\tgoto out;\n \t\tcase 2:\n-\t\t\tif (last == '.')\n-\t\t\t\treturn -1; /* Refname contains \"..\". */\n+\t\t\tif (last == '.') { /* Refname contains \"..\". */\n+\t\t\t\tif (sanitized)\n+\t\t\t\t\tsanitized->len--; /* collapse \"..\" to single \".\" */\n+\t\t\t\telse\n+\t\t\t\t\treturn -1;\n+\t\t\t}\n \t\t\tbreak;\n \t\tcase 3:\n-\t\t\tif (last == '@')\n-\t\t\t\treturn -1; /* Refname contains \"@{\". */\n+\t\t\tif (last == '@') { /* Refname contains \"@{\". */\n+\t\t\t\tif (sanitized)\n+\t\t\t\t\tsanitized->buf[sanitized->len-1] = '-';\n+\t\t\t\telse\n+\t\t\t\t\treturn -1;\n+\t\t\t}\n \t\t\tbreak;\n \t\tcase 4:\n-\t\t\treturn -1;\n+\t\t\t/* forbidden char */\n+\t\t\tif (sanitized)\n+\t\t\t\tsanitized->buf[sanitized->len-1] = '-';\n+\t\t\telse\n+\t\t\t\treturn -1;\n+\t\t\tbreak;\n \t\tcase 5:\n-\t\t\tif (!(*flags & REFNAME_REFSPEC_PATTERN))\n-\t\t\t\treturn -1; /* refspec can't be a pattern */\n+\t\t\tif (!(*flags & REFNAME_REFSPEC_PATTERN)) {\n+\t\t\t\t/* refspec can't be a pattern */\n+\t\t\t\tif (sanitized)\n+\t\t\t\t\tsanitized->buf[sanitized->len-1] = '-';\n+\t\t\t\telse\n+\t\t\t\t\treturn -1;\n+\t\t\t}\n \n \t\t\t/*\n \t\t\t * Unset the pattern flag so that we only accept\n@@ -109,26 +136,48 @@ static int check_refname_component(const char *refname, int *flags)\n out:\n \tif (cp == refname)\n \t\treturn 0; /* Component has zero length. */\n-\tif (refname[0] == '.')\n-\t\treturn -1; /* Component starts with '.'. */\n+\n+\tif (refname[0] == '.') { /* Component starts with '.'. */\n+\t\tif (sanitized)\n+\t\t\tsanitized->buf[component_start] = '-';\n+\t\telse\n+\t\t\treturn -1;\n+\t}\n \tif (cp - refname >= LOCK_SUFFIX_LEN &&\n-\t !memcmp(cp - LOCK_SUFFIX_LEN, LOCK_SUFFIX, LOCK_SUFFIX_LEN))\n-\t\treturn -1; /* Refname ends with \".lock\". */\n+\t !memcmp(cp - LOCK_SUFFIX_LEN, LOCK_SUFFIX, LOCK_SUFFIX_LEN)) {\n+\t\tif (!sanitized)\n+\t\t\treturn -1;\n+\t\t/* Refname ends with \".lock\". */\n+\t\twhile (strbuf_strip_suffix(sanitized, LOCK_SUFFIX)) {\n+\t\t\t/* try again in case we have .lock.lock */\n+\t\t}\n+\t}\n \treturn cp - refname;\n }\n \n-int check_refname_format(const char *refname, int flags)\n+static int check_or_sanitize_refname(const char *refname, int flags,\n+\t\t\t\t struct strbuf *sanitized)\n {\n \tint component_len, component_count = 0;\n \n-\tif (!strcmp(refname, \"@\"))\n+\tif (!strcmp(refname, \"@\")) {\n \t\t/* Refname is a single character '@'. */\n-\t\treturn -1;\n+\t\tif (sanitized)\n+\t\t\tstrbuf_addch(sanitized, '-');\n+\t\telse\n+\t\t\treturn -1;\n+\t}\n \n \twhile (1) {\n+\t\tif (sanitized && sanitized->len)\n+\t\t\tstrbuf_complete(sanitized, '/');\n+\n \t\t/* We are at the start of a path component. */\n-\t\tcomponent_len = check_refname_component(refname, &flags);\n-\t\tif (component_len <= 0)\n+\t\tcomponent_len = check_refname_component(refname, &flags,\n+\t\t\t\t\t\t\tsanitized);\n+\t\tif (sanitized && component_len == 0)\n+\t\t\t; /* OK, omit empty component */\n+\t\telse if (component_len <= 0)\n \t\t\treturn -1;\n \n \t\tcomponent_count++;\n@@ -138,13 +187,29 @@ int check_refname_format(const char *refname, int flags)\n \t\trefname += component_len + 1;\n \t}\n \n-\tif (refname[component_len - 1] == '.')\n-\t\treturn -1; /* Refname ends with '.'. */\n+\tif (refname[component_len - 1] == '.') {\n+\t\t/* Refname ends with '.'. */\n+\t\tif (sanitized)\n+\t\t\t; /* omit ending dot */\n+\t\telse\n+\t\t\treturn -1;\n+\t}\n \tif (!(flags & REFNAME_ALLOW_ONELEVEL) && component_count < 2)\n \t\treturn -1; /* Refname has only one component. */\n \treturn 0;\n }\n \n+int check_refname_format(const char *refname, int flags)\n+{\n+\treturn check_or_sanitize_refname(refname, flags, NULL);\n+}\n+\n+void sanitize_refname_component(const char *refname, struct strbuf *out)\n+{\n+\tif (check_or_sanitize_refname(refname, REFNAME_ALLOW_ONELEVEL, out))\n+\t\tBUG(\"sanitizing refname '%s' check returned error\", refname);\n+}\n+\n int refname_is_safe(const char *refname)\n {\n \tconst char *rest;\ndiff --git a/refs.h b/refs.h\nindex 308fa1f03b..4d8c5465c3 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -460,6 +460,12 @@ int for_each_reflog(each_ref_fn fn, void *cb_data);\n */\n int check_refname_format(const char *refname, int flags);\n \n+/*\n+ * Apply the rules from check_refname_format, but mutate the result until it\n+ * is acceptable, and place the result in \"out\".\n+ */\n+void sanitize_refname_component(const char *refname, struct strbuf *out);\n+\n const char *prettify_refname(const char *refname);\n \n char *shorten_unambiguous_ref(const char *refname, int strict);\ndiff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh\nindex 286bba35d8..c989dbe321 100755\n--- a/t/t2400-worktree-add.sh\n+++ b/t/t2400-worktree-add.sh\n@@ -570,4 +570,9 @@ test_expect_success '\"add\" an existing locked but missing worktree' '\n \tgit worktree add --force --force --detach gnoo\n '\n \n+test_expect_success FUNNYNAMES 'sanitize generated worktree name' '\n+\tgit worktree add --detach \". weird*..?.lock.lock\" &&\n+\ttest -d .git/worktrees/---weird-.-\n+'\n+\n test_done\n-- \n2.21.0.rc1.337.gdf7f8d0522\n\n"},{"id":"371056","messageId":"CAPig+cQYDuKrRwf9GrGZUTnH=BgSyp8Rmh7ON1p+0qOrHxpe3Q@mail.gmail.com","threadId":"50533","inReplyTo":"20190308092834.12549-2-pclouds@gmail.com","subject":"Re: [PATCH v5 1/1] worktree add: sanitize worktree names","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-03-10T02:02:02Z","receivedAt":"2019-03-10T02:02:15Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Mar 8, 2019 at 4:28 AM Nguyễn Thái Ngọc Duy wrote:\n> Worktree names are based on $(basename $GIT_WORK_TREE). They aren't\n> significant until 3a3b9d8cde (refs: new ref types to make per-worktree\n> refs visible to all worktrees - 2018-10-21), where worktree name could\n> be part of a refname and must follow refname rules.\n>\n> Update 'worktree add' code to remove special characters to follow\n> these rules. In the future the user will be able to specify the\n> worktree name by themselves if they're not happy with this dumb\n> character substitution.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> diff --git a/refs.c b/refs.c\n> @@ -72,30 +72,57 @@ static unsigned char refname_disposition[256] = {\n> +static int check_refname_component(const char *refname, int *flags,\n> + struct strbuf *sanitized)\n> {\n> for (cp = refname; ; cp++) {\n> unsigned char disp = refname_disposition[ch];\n> + if (sanitized && disp != 1)\n> + strbuf_addch(sanitized, ch);\n> +\n> switch (disp) {\n> case 1:\n> goto out;\n> case 2:\n> + if (last == '.') { /* Refname contains \"..\". */\n> + if (sanitized)\n> + sanitized->len--; /* collapse \"..\" to single \".\" */\n\nI think this needs to be:\n\n strbuf_setlen(sanitized, sanitized->len - 1);\n\nto ensure that NUL-terminator ends up in the correct place if this \".\"\nis the very last character in 'refname'. (Otherwise, the NUL will\nremain after the second \".\", thus \"..\" won't be collapsed to \".\" at\nall.)\n\n> + else\n> + return -1;\n> + }\n> break;\n"},{"id":"371121","messageId":"xmqqbm2ikk4q.fsf@gitster-ct.c.googlers.com","threadId":"50533","inReplyTo":"CAPig+cQYDuKrRwf9GrGZUTnH=BgSyp8Rmh7ON1p+0qOrHxpe3Q@mail.gmail.com","subject":"Re: [PATCH v5 1/1] worktree add: sanitize worktree names","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-11T06:20:05Z","receivedAt":"2019-03-11T06:20:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n>> case 2:\n>> + if (last == '.') { /* Refname contains \"..\". */\n>> + if (sanitized)\n>> + sanitized->len--; /* collapse \"..\" to single \".\" */\n>\n> I think this needs to be:\n>\n> strbuf_setlen(sanitized, sanitized->len - 1);\n>\n> to ensure that NUL-terminator ends up in the correct place if this \".\"\n> is the very last character in 'refname'. (Otherwise, the NUL will\n> remain after the second \".\", thus \"..\" won't be collapsed to \".\" at\n> all.)\n\nTrue. Why doesn't it do the similar \"replace with -\" it does for\nother unfortunate characters, though?\n\n"},{"id":"371122","messageId":"xmqqtvg9kjdb.fsf@gitster-ct.c.googlers.com","threadId":"50533","inReplyTo":"20190308092834.12549-2-pclouds@gmail.com","subject":"Re: [PATCH v5 1/1] worktree add: sanitize worktree names","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-11T06:36:32Z","receivedAt":"2019-03-11T06:36:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> Update 'worktree add' code to remove special characters to follow\n> these rules. In the future the user will be able to specify the\n> worktree name by themselves if they're not happy with this dumb\n> character substitution.\n\nThis replaces both of the two patches in v4, and applies to a more\nrecent tip of 'master' (post 7d0c1f4556).\n\n> diff --git a/refs.c b/refs.c\n> index 142888a40a..e9f83018f0 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -72,30 +72,57 @@ static unsigned char refname_disposition[256] = {\n> * - it ends with \".lock\", or\n> * - it contains a \"@{\" portion\n> */\n\nThe comment above needs modernizing (see attached at the end).\n\n> \t\tcase 2:\n> -\t\t\tif (last == '.')\n> -\t\t\t\treturn -1; /* Refname contains \"..\". */\n> +\t\t\tif (last == '.') { /* Refname contains \"..\". */\n> +\t\t\t\tif (sanitized)\n> +\t\t\t\t\tsanitized->len--; /* collapse \"..\" to single \".\" */\n\nAs Eric points out, this needs to be fixed. \n\nI'll use the strbuf_setlen() version suggested by Eric in the\nmeantime, but \"sanitized->buf[sanitized->len-1] = '-'\" as done to\neverything else in the function may be a better idea, especially\nsince they'll be able to name the worktree themselves in the future\nanyway.\n\n> +\n> +\tif (refname[0] == '.') { /* Component starts with '.'. */\n> +\t\tif (sanitized)\n> +\t\t\tsanitized->buf[component_start] = '-';\n\n... and a dot turns into a dash in some cases anyway.\n\n> +\t\telse\n> +\t\t\treturn -1;\n> +\t}\n> \tif (cp - refname >= LOCK_SUFFIX_LEN &&\n> -\t !memcmp(cp - LOCK_SUFFIX_LEN, LOCK_SUFFIX, LOCK_SUFFIX_LEN))\n> -\t\treturn -1; /* Refname ends with \".lock\". */\n> +\t !memcmp(cp - LOCK_SUFFIX_LEN, LOCK_SUFFIX, LOCK_SUFFIX_LEN)) {\n> +\t\tif (!sanitized)\n> +\t\t\treturn -1;\n> +\t\t/* Refname ends with \".lock\". */\n> +\t\twhile (strbuf_strip_suffix(sanitized, LOCK_SUFFIX)) {\n> +\t\t\t/* try again in case we have .lock.lock */\n> +\t\t}\n\nNo need for {}; just have an empty statment\n\n\t\twhile (...) \n\t\t\t; /* try again ... */\n\nThis \"strip all .lock repeatedly\" made me stop and think a bit; this\nwill never make the component empty, as the only way for this loop\nto make it empty is if we have a string that match \"^\\(.lock)\\*$\" in\nit, but the first dot would have already been turned into a dash, so\nwe'll end up with \"-lock\", which is not empty.\n\n> +\t}\n> \treturn cp - refname;\n> }\n\nSee below for a possible further polishment.\n\n * The first hunk is not about this patch but a long-standing issue\n after the comment was given to this function for a single level\n (I do not know or care how it happened--perhaps we had a single\n function that verifies multiple levels which later was split into\n a caller that loops and this function that checks a single level,\n and the comment for the multi-level function was left stale).\n\n * check_refname_component() can now try to sanitize; document it.\n\n * The last hunk is from Eric's comment.\n\n refs.c | 9 +++++++--\n 1 file changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex e9f83018f0..3a1b2a8c3c 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -63,7 +63,7 @@ static unsigned char refname_disposition[256] = {\n * not legal. It is legal if it is something reasonable to have under\n * \".git/refs/\"; We do not like it if:\n *\n- * - any path component of it begins with \".\", or\n+ * - it begins with \".\", or\n * - it has double dots \"..\", or\n * - it has ASCII control characters, or\n * - it has \":\", \"?\", \"[\", \"\\\", \"^\", \"~\", SP, or TAB anywhere, or\n@@ -71,6 +71,10 @@ static unsigned char refname_disposition[256] = {\n * - it ends with a \"/\", or\n * - it ends with \".lock\", or\n * - it contains a \"@{\" portion\n+ *\n+ * When sanitized is not NULL, instead of rejecting the input refname\n+ * as an error, try to come up with a usable replacement for the input\n+ * refname in it.\n */\n static int check_refname_component(const char *refname, int *flags,\n \t\t\t\t struct strbuf *sanitized)\n@@ -95,7 +99,8 @@ static int check_refname_component(const char *refname, int *flags,\n \t\tcase 2:\n \t\t\tif (last == '.') { /* Refname contains \"..\". */\n \t\t\t\tif (sanitized)\n-\t\t\t\t\tsanitized->len--; /* collapse \"..\" to single \".\" */\n+\t\t\t\t\t/* collapse \"..\" to single \".\" */\n+\t\t\t\t\tstrbuf_setlen(sanitized, sanitized->len - 1);\n \t\t\t\telse\n \t\t\t\t\treturn -1;\n \t\t\t}\n"},{"id":"371129","messageId":"CACsJy8CqN=Uu-Fez7T9evazitVopXt2dkQ1rGzKwh94tdiUdvA@mail.gmail.com","threadId":"50533","inReplyTo":"xmqqbm2ikk4q.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v5 1/1] worktree add: sanitize worktree names","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-03-11T09:24:13Z","receivedAt":"2019-03-11T09:24:41Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Mar 11, 2019 at 1:20 PM Junio C Hamano wrote:\n>\n> Eric Sunshine writes:\n>\n> >> case 2:\n> >> + if (last == '.') { /* Refname contains \"..\". */\n> >> + if (sanitized)\n> >> + sanitized->len--; /* collapse \"..\" to single \".\" */\n> >\n> > I think this needs to be:\n> >\n> > strbuf_setlen(sanitized, sanitized->len - 1);\n> >\n> > to ensure that NUL-terminator ends up in the correct place if this \".\"\n> > is the very last character in 'refname'. (Otherwise, the NUL will\n> > remain after the second \".\", thus \"..\" won't be collapsed to \".\" at\n> > all.)\n>\n> True. Why doesn't it do the similar \"replace with -\" it does for\n> other unfortunate characters, though?\n>\n\nI think Jeff saw an opportunity to keep it cleaner (\".\" looks better\nthan \".-\") and took it.\n-- \nDuy\n"},{"id":"371134","messageId":"CACsJy8D2qmu5PpC=--y+_g-dyztzxumo2bKD-ufwd79nm+Pw9g@mail.gmail.com","threadId":"50533","inReplyTo":"xmqqtvg9kjdb.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v5 1/1] worktree add: sanitize worktree names","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-03-11T09:27:26Z","receivedAt":"2019-03-11T09:27:55Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Mar 11, 2019 at 1:36 PM Junio C Hamano wrote:\n> while (...)\n> ; /* try again ... */\n>\n> This \"strip all .lock repeatedly\" made me stop and think a bit; this\n> will never make the component empty, as the only way for this loop\n> to make it empty is if we have a string that match \"^\\(.lock)\\*$\" in\n> it, but the first dot would have already been turned into a dash, so\n> we'll end up with \"-lock\", which is not empty.\n\nYep. I added a BUG() check anyway in worktree.c just in case something\nslips through in the future.\n\n> > + }\n> > return cp - refname;\n> > }\n>\n> See below for a possible further polishment.\n>\n> * The first hunk is not about this patch but a long-standing issue\n> after the comment was given to this function for a single level\n> (I do not know or care how it happened--perhaps we had a single\n> function that verifies multiple levels which later was split into\n> a caller that loops and this function that checks a single level,\n> and the comment for the multi-level function was left stale).\n>\n> * check_refname_component() can now try to sanitize; document it.\n>\n> * The last hunk is from Eric's comment.\n\nThanks.\n-- \nDuy\n"},{"id":"371146","messageId":"nycvar.QRO.7.76.6.1903111401220.41@tvgsbejvaqbjf.bet","threadId":"50533","inReplyTo":"20190308092834.12549-2-pclouds@gmail.com","subject":"Re: [PATCH v5 1/1] worktree add: sanitize worktree names","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-03-11T13:05:32Z","receivedAt":"2019-03-11T13:06:05Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Duy,\n\nOn Fri, 8 Mar 2019, Nguyễn Thái Ngọc Duy wrote:\n\n> diff --git a/refs.c b/refs.c\n> index 142888a40a..e9f83018f0 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -72,30 +72,57 @@ static unsigned char refname_disposition[256] = {\n> * - it ends with \".lock\", or\n> * - it contains a \"@{\" portion\n> */\n> -static int check_refname_component(const char *refname, int *flags)\n> +static int check_refname_component(const char *refname, int *flags,\n> +\t\t\t\t struct strbuf *sanitized)\n> {\n> \tconst char *cp;\n> \tchar last = '\\0';\n> +\tsize_t component_start;\n\nThis variable is uninitialized. It is then...\n\n> +\n> +\tif (sanitized)\n> +\t\tcomponent_start = sanitized->len;\n\n... initialized only when `sanitized` is not `NULL`, and subsequently...\n\n> \n> \tfor (cp = refname; ; cp++) {\n> \t\tint ch = *cp & 255;\n> \t\tunsigned char disp = refname_disposition[ch];\n> +\n> +\t\tif (sanitized && disp != 1)\n> +\t\t\tstrbuf_addch(sanitized, ch);\n> +\n> \t\tswitch (disp) {\n> \t\tcase 1:\n> \t\t\tgoto out;\n> \t\tcase 2:\n> -\t\t\tif (last == '.')\n> -\t\t\t\treturn -1; /* Refname contains \"..\". */\n> +\t\t\tif (last == '.') { /* Refname contains \"..\". */\n> +\t\t\t\tif (sanitized)\n> +\t\t\t\t\tsanitized->len--; /* collapse \"..\" to single \".\" */\n> +\t\t\t\telse\n> +\t\t\t\t\treturn -1;\n> +\t\t\t}\n> \t\t\tbreak;\n> \t\tcase 3:\n> -\t\t\tif (last == '@')\n> -\t\t\t\treturn -1; /* Refname contains \"@{\". */\n> +\t\t\tif (last == '@') { /* Refname contains \"@{\". */\n> +\t\t\t\tif (sanitized)\n> +\t\t\t\t\tsanitized->buf[sanitized->len-1] = '-';\n> +\t\t\t\telse\n> +\t\t\t\t\treturn -1;\n> +\t\t\t}\n> \t\t\tbreak;\n> \t\tcase 4:\n> -\t\t\treturn -1;\n> +\t\t\t/* forbidden char */\n> +\t\t\tif (sanitized)\n> +\t\t\t\tsanitized->buf[sanitized->len-1] = '-';\n> +\t\t\telse\n> +\t\t\t\treturn -1;\n> +\t\t\tbreak;\n> \t\tcase 5:\n> -\t\t\tif (!(*flags & REFNAME_REFSPEC_PATTERN))\n> -\t\t\t\treturn -1; /* refspec can't be a pattern */\n> +\t\t\tif (!(*flags & REFNAME_REFSPEC_PATTERN)) {\n> +\t\t\t\t/* refspec can't be a pattern */\n> +\t\t\t\tif (sanitized)\n> +\t\t\t\t\tsanitized->buf[sanitized->len-1] = '-';\n> +\t\t\t\telse\n> +\t\t\t\t\treturn -1;\n> +\t\t\t}\n> \n> \t\t\t/*\n> \t\t\t * Unset the pattern flag so that we only accept\n> @@ -109,26 +136,48 @@ static int check_refname_component(const char *refname, int *flags)\n> out:\n> \tif (cp == refname)\n> \t\treturn 0; /* Component has zero length. */\n> -\tif (refname[0] == '.')\n> -\t\treturn -1; /* Component starts with '.'. */\n> +\n> +\tif (refname[0] == '.') { /* Component starts with '.'. */\n> +\t\tif (sanitized)\n> +\t\t\tsanitized->buf[component_start] = '-';\n\n... used a loooooooong time after that, also only if `sanitized` is not\n`NULL`.\n\nApparently for some GCC versions, this is too cute, and it complains that\nthis variable might be used uninitialized:\nhttps://dev.azure.com/gitgitgadget/git/_build/results?buildId=4352&view=logs\n\nAnd quite honestly, even for mere humans it is not all *that* clear that\n`sanitized` cannot be changed from `NULL` to non-`NULL` in the code in\nbetween, *in particular* because the changes extend over two hunks, the\ncode between is not shown.\n\nI would strongly advise against trying to be so cute, and just initialize\nthe variable already. Over-optimization in such instances makes the code a\nlot harder to reason about.\n\nCiao,\nJohannes"},{"id":"371202","messageId":"20190311223936.GA24989@sigill.intra.peff.net","threadId":"50533","inReplyTo":"CACsJy8CqN=Uu-Fez7T9evazitVopXt2dkQ1rGzKwh94tdiUdvA@mail.gmail.com","subject":"Re: [PATCH v5 1/1] worktree add: sanitize worktree names","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-11T22:39:36Z","receivedAt":"2019-03-11T22:39:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 11, 2019 at 04:24:13PM +0700, Duy Nguyen wrote:\n\n> > > I think this needs to be:\n> > >\n> > > strbuf_setlen(sanitized, sanitized->len - 1);\n> > >\n> > > to ensure that NUL-terminator ends up in the correct place if this \".\"\n> > > is the very last character in 'refname'. (Otherwise, the NUL will\n> > > remain after the second \".\", thus \"..\" won't be collapsed to \".\" at\n> > > all.)\n> >\n> > True. Why doesn't it do the similar \"replace with -\" it does for\n> > other unfortunate characters, though?\n> \n> I think Jeff saw an opportunity to keep it cleaner (\".\" looks better\n> than \".-\") and took it.\n\nYeah, that was one thing I was going to comment on your patch. The\n\"rules\" I made up were pretty ad-hoc as I was walking through the\nfunction (note it also drops \".lock\" instead of sanitizing it into\n\"-lock\").\n\nBut it may make sense to make things more consistent (even if the result\nisn't entirely reversible).\n\nAnother option _is_ to actually make it reversible. I.e., use \"%2e\"\ninstead of \".\", which would also necessitate replacing \"%\". I don't know\nif that has a huge value for this use-case, but it's a nice property\nthat two sanitized names can't collide (unless they originally\nidentical).\n\n-Peff\n"},{"id":"371221","messageId":"xmqqa7i0iow6.fsf@gitster-ct.c.googlers.com","threadId":"50533","inReplyTo":"20190311223936.GA24989@sigill.intra.peff.net","subject":"Re: [PATCH v5 1/1] worktree add: sanitize worktree names","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-12T06:32:25Z","receivedAt":"2019-03-12T06:32:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Another option _is_ to actually make it reversible. I.e., use \"%2e\"\n> instead of \".\", which would also necessitate replacing \"%\". I don't know\n> if that has a huge value for this use-case, but it's a nice property\n> that two sanitized names can't collide (unless they originally\n> identical).\n\nYeah. That is a good property to have.\n"},{"id":"371222","messageId":"xmqqzhq0h9pk.fsf@gitster-ct.c.googlers.com","threadId":"50533","inReplyTo":"nycvar.QRO.7.76.6.1903111401220.41@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v5 1/1] worktree add: sanitize worktree names","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-12T06:45:43Z","receivedAt":"2019-03-12T06:45:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> +static int check_refname_component(const char *refname, int *flags,\n>> +\t\t\t\t struct strbuf *sanitized)\n>> {\n>> \tconst char *cp;\n>> \tchar last = '\\0';\n>> +\tsize_t component_start;\n>\n> This variable is uninitialized. It is then...\n>\n>> +\n>> +\tif (sanitized)\n>> +\t\tcomponent_start = sanitized->len;\n>\n> ... initialized only when `sanitized` is not `NULL`, and subsequently...\n> ...\n>> +\tif (refname[0] == '.') { /* Component starts with '.'. */\n>> +\t\tif (sanitized)\n>> +\t\t\tsanitized->buf[component_start] = '-';\n> ...\n> ... used a loooooooong time after that, also only if `sanitized` is not\n> `NULL`.\n>\n> Apparently for some GCC versions, this is too cute, and it complains that\n\nIt does require humans (well, at least it did to this one) to be\ncareful when reading the code to know that component_start is valid\nwhen it is used.\n\nThere unfortunately is no good \"default\" value to initialize the\nvariable to. When checking a later component in a series of\ncomponents, it would be looking at non-zero position, so even\ninitializing it to 0 in this function is *not* a more sensible\nfallback default value than any other random garbage value (which\nwould squelch the compiler, but it would mislead the humans\nnevertheless).\n\nAnd that (i.e. the lack of any sensible default value when sanitized\nis NULL) is the reason why the variable is left uninitialized by the\npatch, I think. I do not think the code is trying to be cute at\nall.\n\nI wonder if we make the caller pass a pointer to\n\n\tstruct {\n\t\tstruct strbuf result;\n\t\tsize_t component_start;\n\t} sanitized;\n\n\tsanitized.component_start = sanitized.result.len\n\tcheck_refname_component(refname, flags, &sanitized);\n\nand get rid of the assignment to component_start done by the callee,\nit would appease compilers and makes the code easier to vet. It does\nintroduce one more ad-hoc type, which is a certain downside.\n\nI dunno.\n"}]}