{"thread":{"id":"47678","subject":"[PATCH 0/7] nd/worktree-move reboot","startedAt":"2018-01-24T09:54:16Z","lastAt":"2018-03-05T18:44:06Z","messageCount":49,"participants":["Nguyễn Thái Ngọc Duy","Junio C Hamano","Eric Sunshine","Duy Nguyen","Jeff King","Martin Ågren","Kaartic Sivaraam","SZEDER Gábor"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"337217","messageId":"20180124095357.19645-1-pclouds@gmail.com","threadId":"47678","inReplyTo":null,"subject":"[PATCH 0/7] nd/worktree-move reboot","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-24T09:53:50Z","receivedAt":"2018-01-24T09:54:16Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This series adds two more commands \"git worktree move\" and \"git\nworktree remove\" to do those things. I think I have addressed all\ncomments from the mail threads referenced in \"What's cooking\" mails. I\nalso added the ability to remove a worktree if its worktree area is\nalready deleted.\n\nIt's a big code change (I reorganized remove_worktree() a bit for\nexample to keep the last/new patch clean) so I'm not going to send\ninterdiff.\n\nThere's only one thing left that I should do, mentioned in 6/7, to\nprint detached HEAD before we remove a worktree. But I think if that's\na good idea, it could be done separately on top.\n\nBig thanks to Junio for keeping this on 'pu' all this time. Must be\nhard on you to resolve conflicts over and over.\n\nNguyễn Thái Ngọc Duy (7):\n worktree.c: add validate_worktree()\n worktree.c: add update_worktree_location()\n worktree move: new command\n worktree move: accept destination as directory\n worktree move: refuse to move worktrees with submodules\n worktree remove: new command\n worktree remove: allow it when $GIT_WORK_TREE is already gone\n\n Documentation/git-worktree.txt | 28 +++--\n builtin/worktree.c | 216 +++++++++++++++++++++++++++++++++\n contrib/completion/git-completion.bash | 5 +-\n t/t2028-worktree-move.sh | 65 ++++++++++\n worktree.c | 97 +++++++++++++++\n worktree.h | 18 +++\n 6 files changed, 418 insertions(+), 11 deletions(-)\n\n-- \n2.16.0.47.g3d9b0fac3a\n\n"},{"id":"337218","messageId":"20180124095357.19645-2-pclouds@gmail.com","threadId":"47678","inReplyTo":"20180124095357.19645-1-pclouds@gmail.com","subject":"[PATCH 1/7] worktree.c: add validate_worktree()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-24T09:53:51Z","receivedAt":"2018-01-24T09:54:27Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This function is later used by \"worktree move\" and \"worktree remove\"\nto ensure that we have a good connection between the repository and\nthe worktree. For example, if a worktree is moved manually, the\nworktree location recorded in $GIT_DIR/worktrees/.../gitdir is\nincorrect and we should not move that one.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n worktree.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n worktree.h | 9 ++++++++\n 2 files changed, 81 insertions(+)\n\ndiff --git a/worktree.c b/worktree.c\nindex f5da7d286d..b238d87bf1 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -254,6 +254,78 @@ const char *is_worktree_locked(struct worktree *wt)\n \treturn wt->lock_reason;\n }\n \n+/* convenient wrapper to deal with NULL strbuf */\n+static void strbuf_addf_gently(struct strbuf *buf, const char *fmt, ...)\n+{\n+\tva_list params;\n+\n+\tif (!buf)\n+\t\treturn;\n+\n+\tva_start(params, fmt);\n+\tstrbuf_vaddf(buf, fmt, params);\n+\tva_end(params);\n+}\n+\n+int validate_worktree(const struct worktree *wt, struct strbuf *errmsg)\n+{\n+\tstruct strbuf wt_path = STRBUF_INIT;\n+\tchar *path = NULL;\n+\tint err, ret = -1;\n+\n+\tstrbuf_addf(&wt_path, \"%s/.git\", wt->path);\n+\n+\tif (is_main_worktree(wt)) {\n+\t\tif (is_directory(wt_path.buf)) {\n+\t\t\tret = 0;\n+\t\t\tgoto done;\n+\t\t}\n+\t\t/*\n+\t\t * Main worktree using .git file to point to the\n+\t\t * repository would make it impossible to know where\n+\t\t * the actual worktree is if this function is executed\n+\t\t * from another worktree. No .git file support for now.\n+\t\t */\n+\t\tstrbuf_addf_gently(errmsg,\n+\t\t\t\t _(\"'%s' at main working tree is not the repository directory\"),\n+\t\t\t\t wt_path.buf);\n+\t\tgoto done;\n+\t}\n+\n+\t/*\n+\t * Make sure \"gitdir\" file points to a real .git file and that\n+\t * file points back here.\n+\t */\n+\tif (!is_absolute_path(wt->path)) {\n+\t\tstrbuf_addf_gently(errmsg,\n+\t\t\t\t _(\"'%s' file does not contain absolute path to the working tree location\"),\n+\t\t\t\t git_common_path(\"worktrees/%s/gitdir\", wt->id));\n+\t\tgoto done;\n+\t}\n+\n+\tif (!file_exists(wt_path.buf)) {\n+\t\tstrbuf_addf_gently(errmsg, _(\"'%s' does not exist\"), wt_path.buf);\n+\t\tgoto done;\n+\t}\n+\n+\tpath = xstrdup_or_null(read_gitfile_gently(wt_path.buf, &err));\n+\tif (!path) {\n+\t\tstrbuf_addf_gently(errmsg, _(\"'%s' is not a .git file, error code %d\"),\n+\t\t\t\t wt_path.buf, err);\n+\t\tgoto done;\n+\t}\n+\n+\tret = fspathcmp(path, real_path(git_common_path(\"worktrees/%s\", wt->id)));\n+\n+\tif (ret)\n+\t\tstrbuf_addf_gently(errmsg, _(\"'%s' does not point back to '%s'\"),\n+\t\t\t\t wt->path, git_common_path(\"worktrees/%s\", wt->id));\n+done:\n+\tfree(path);\n+\tstrbuf_release(&wt_path);\n+\treturn ret;\n+}\n+\n int is_worktree_being_rebased(const struct worktree *wt,\n \t\t\t const char *target)\n {\ndiff --git a/worktree.h b/worktree.h\nindex c28a880e18..cb577de8cd 100644\n--- a/worktree.h\n+++ b/worktree.h\n@@ -3,6 +3,8 @@\n \n #include \"refs.h\"\n \n+struct strbuf;\n+\n struct worktree {\n \tchar *path;\n \tchar *id;\n@@ -59,6 +61,13 @@ extern int is_main_worktree(const struct worktree *wt);\n */\n extern const char *is_worktree_locked(struct worktree *wt);\n \n+/*\n+ * Return zero if the worktree is in good condition. Error message is\n+ * returned if \"errmsg\" is not NULL.\n+ */\n+extern int validate_worktree(const struct worktree *wt,\n+\t\t\t struct strbuf *errmsg);\n+\n /*\n * Free up the memory for worktree(s)\n */\n-- \n2.16.0.47.g3d9b0fac3a\n\n"},{"id":"337219","messageId":"20180124095357.19645-3-pclouds@gmail.com","threadId":"47678","inReplyTo":"20180124095357.19645-1-pclouds@gmail.com","subject":"[PATCH 2/7] worktree.c: add update_worktree_location()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-24T09:53:52Z","receivedAt":"2018-01-24T09:54:33Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Signed-off-by: Nguyễn Thái Ngọc Duy \n---\n worktree.c | 18 ++++++++++++++++++\n worktree.h | 6 ++++++\n 2 files changed, 24 insertions(+)\n\ndiff --git a/worktree.c b/worktree.c\nindex b238d87bf1..98e9f32c7f 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -326,6 +326,24 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg)\n \treturn ret;\n }\n \n+void update_worktree_location(struct worktree *wt, const char *path_)\n+{\n+\tstruct strbuf path = STRBUF_INIT;\n+\n+\tif (is_main_worktree(wt))\n+\t\tdie(\"BUG: can't relocate main worktree\");\n+\n+\tstrbuf_add_absolute_path(&path, path_);\n+\tif (fspathcmp(wt->path, path.buf)) {\n+\t\twrite_file(git_common_path(\"worktrees/%s/gitdir\",\n+\t\t\t\t\t wt->id),\n+\t\t\t \"%s/.git\", real_path(path.buf));\n+\t\tfree(wt->path);\n+\t\twt->path = strbuf_detach(&path, NULL);\n+\t}\n+\tstrbuf_release(&path);\n+}\n+\n int is_worktree_being_rebased(const struct worktree *wt,\n \t\t\t const char *target)\n {\ndiff --git a/worktree.h b/worktree.h\nindex cb577de8cd..a913428c3d 100644\n--- a/worktree.h\n+++ b/worktree.h\n@@ -68,6 +68,12 @@ extern const char *is_worktree_locked(struct worktree *wt);\n extern int validate_worktree(const struct worktree *wt,\n \t\t\t struct strbuf *errmsg);\n \n+/*\n+ * Update worktrees/xxx/gitdir with the new path.\n+ */\n+extern void update_worktree_location(struct worktree *wt,\n+\t\t\t\t const char *path_);\n+\n /*\n * Free up the memory for worktree(s)\n */\n-- \n2.16.0.47.g3d9b0fac3a\n\n"},{"id":"337220","messageId":"20180124095357.19645-4-pclouds@gmail.com","threadId":"47678","inReplyTo":"20180124095357.19645-1-pclouds@gmail.com","subject":"[PATCH 3/7] worktree move: new command","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-24T09:53:53Z","receivedAt":"2018-01-24T09:54:36Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This command allows to relocate linked worktrees. Main worktree cannot\n(yet) be moved.\n\nThere are two options to move the main worktree, but both have\ncomplications, so it's not implemented yet. Anyway the options are:\n\n- convert the main worktree to a linked one and move it away, leave\n the git repository where it is. The repo essentially becomes bare\n after this move.\n\n- move the repository with the main worktree. The tricky part is make\n sure all file descriptors to the repository are closed, or it may\n fail on Windows.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n Documentation/git-worktree.txt | 7 ++++-\n builtin/worktree.c | 50 ++++++++++++++++++++++++++++++++++\n contrib/completion/git-completion.bash | 2 +-\n t/t2028-worktree-move.sh | 31 +++++++++++++++++++++\n 4 files changed, 88 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt\nindex 41585f535d..5d115b855a 100644\n--- a/Documentation/git-worktree.txt\n+++ b/Documentation/git-worktree.txt\n@@ -12,6 +12,7 @@ SYNOPSIS\n 'git worktree add' [-f] [--detach] [--checkout] [--lock] [-b ] []\n 'git worktree list' [--porcelain]\n 'git worktree lock' [--reason ] \n+'git worktree move' \n 'git worktree prune' [-n] [-v] [--expire ]\n 'git worktree unlock' \n \n@@ -79,6 +80,11 @@ files from being pruned automatically. This also prevents it from\n being moved or deleted. Optionally, specify a reason for the lock\n with `--reason`.\n \n+move::\n+\n+Move a working tree to a new location. Note that the main working tree\n+cannot be moved.\n+\n prune::\n \n Prune working tree information in $GIT_DIR/worktrees.\n@@ -281,7 +287,6 @@ performed manually, such as:\n \n - `remove` to remove a linked working tree and its administrative files (and\n warn if the working tree is dirty)\n-- `mv` to move or rename a working tree and update its administrative files\n \n GIT\n ---\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 7cef5b120b..2faa95430a 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -17,6 +17,7 @@ static const char * const worktree_usage[] = {\n \tN_(\"git worktree add [] []\"),\n \tN_(\"git worktree list []\"),\n \tN_(\"git worktree lock [] \"),\n+\tN_(\"git worktree move \"),\n \tN_(\"git worktree prune []\"),\n \tN_(\"git worktree unlock \"),\n \tNULL\n@@ -605,6 +606,53 @@ static int unlock_worktree(int ac, const char **av, const char *prefix)\n \treturn ret;\n }\n \n+static int move_worktree(int ac, const char **av, const char *prefix)\n+{\n+\tstruct option options[] = {\n+\t\tOPT_END()\n+\t};\n+\tstruct worktree **worktrees, *wt;\n+\tstruct strbuf dst = STRBUF_INIT;\n+\tstruct strbuf errmsg = STRBUF_INIT;\n+\tconst char *reason;\n+\tchar *path;\n+\n+\tac = parse_options(ac, av, prefix, options, worktree_usage, 0);\n+\tif (ac != 2)\n+\t\tusage_with_options(worktree_usage, options);\n+\n+\tpath = prefix_filename(prefix, av[1]);\n+\tstrbuf_addstr(&dst, path);\n+\tfree(path);\n+\tif (file_exists(dst.buf))\n+\t\tdie(_(\"target '%s' already exists\"), av[1]);\n+\n+\tworktrees = get_worktrees(0);\n+\twt = find_worktree(worktrees, prefix, av[0]);\n+\tif (!wt)\n+\t\tdie(_(\"'%s' is not a working tree\"), av[0]);\n+\tif (is_main_worktree(wt))\n+\t\tdie(_(\"'%s' is a main working tree\"), av[0]);\n+\treason = is_worktree_locked(wt);\n+\tif (reason) {\n+\t\tif (*reason)\n+\t\t\tdie(_(\"cannot move a locked working tree, lock reason: %s\"),\n+\t\t\t reason);\n+\t\tdie(_(\"cannot move a locked working tree\"));\n+\t}\n+\tif (validate_worktree(wt, &errmsg))\n+\t\tdie(_(\"validation failed, cannot move working tree:\\n%s\"),\n+\t\t errmsg.buf);\n+\tstrbuf_release(&errmsg);\n+\n+\tif (rename(wt->path, dst.buf) == -1)\n+\t\tdie_errno(_(\"failed to move '%s' to '%s'\"), wt->path, dst.buf);\n+\n+\tupdate_worktree_location(wt, dst.buf);\n+\n+\treturn 0;\n+}\n+\n int cmd_worktree(int ac, const char **av, const char *prefix)\n {\n \tstruct option options[] = {\n@@ -627,5 +675,7 @@ int cmd_worktree(int ac, const char **av, const char *prefix)\n \t\treturn lock_worktree(ac - 1, av + 1, prefix);\n \tif (!strcmp(av[1], \"unlock\"))\n \t\treturn unlock_worktree(ac - 1, av + 1, prefix);\n+\tif (!strcmp(av[1], \"move\"))\n+\t\treturn move_worktree(ac - 1, av + 1, prefix);\n \tusage_with_options(worktree_usage, options);\n }\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 3683c772c5..b87d387686 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -3087,7 +3087,7 @@ _git_whatchanged ()\n \n _git_worktree ()\n {\n-\tlocal subcommands=\"add list lock prune unlock\"\n+\tlocal subcommands=\"add list lock move prune unlock\"\n \tlocal subcommand=\"$(__git_find_on_cmdline \"$subcommands\")\"\n \tif [ -z \"$subcommand\" ]; then\n \t\t__gitcomp \"$subcommands\"\ndiff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh\nindex 8298aaf97f..bef420a086 100755\n--- a/t/t2028-worktree-move.sh\n+++ b/t/t2028-worktree-move.sh\n@@ -59,4 +59,35 @@ test_expect_success 'unlock worktree twice' '\n \ttest_path_is_missing .git/worktrees/source/locked\n '\n \n+test_expect_success 'move non-worktree' '\n+\tmkdir abc &&\n+\ttest_must_fail git worktree move abc def\n+'\n+\n+test_expect_success 'move locked worktree' '\n+\tgit worktree lock source &&\n+\ttest_must_fail git worktree move source destination &&\n+\tgit worktree unlock source\n+'\n+\n+test_expect_success 'move worktree' '\n+\ttoplevel=\"$(pwd)\" &&\n+\tgit worktree move source destination &&\n+\ttest_path_is_missing source &&\n+\tgit worktree list --porcelain | grep \"^worktree\" >actual &&\n+\tcat <<-EOF >expected &&\n+\tworktree $toplevel\n+\tworktree $toplevel/destination\n+\tworktree $toplevel/elsewhere\n+\tEOF\n+\ttest_cmp expected actual &&\n+\tgit -C destination log --format=%s >actual2 &&\n+\techo init >expected2 &&\n+\ttest_cmp expected2 actual2\n+'\n+\n+test_expect_success 'move main worktree' '\n+\ttest_must_fail git worktree move . def\n+'\n+\n test_done\n-- \n2.16.0.47.g3d9b0fac3a\n\n"},{"id":"337221","messageId":"20180124095357.19645-5-pclouds@gmail.com","threadId":"47678","inReplyTo":"20180124095357.19645-1-pclouds@gmail.com","subject":"[PATCH 4/7] worktree move: accept destination as directory","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-24T09:53:54Z","receivedAt":"2018-01-24T09:54:40Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Similar to \"mv a b/\", which is actually \"mv a b/a\", we extract basename\nof source worktree and create a directory of the same name at\ndestination if dst path is a directory.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/worktree.c | 16 ++++++++++++++--\n 1 file changed, 14 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 2faa95430a..89398e67e4 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -624,8 +624,6 @@ static int move_worktree(int ac, const char **av, const char *prefix)\n \tpath = prefix_filename(prefix, av[1]);\n \tstrbuf_addstr(&dst, path);\n \tfree(path);\n-\tif (file_exists(dst.buf))\n-\t\tdie(_(\"target '%s' already exists\"), av[1]);\n \n \tworktrees = get_worktrees(0);\n \twt = find_worktree(worktrees, prefix, av[0]);\n@@ -633,6 +631,20 @@ static int move_worktree(int ac, const char **av, const char *prefix)\n \t\tdie(_(\"'%s' is not a working tree\"), av[0]);\n \tif (is_main_worktree(wt))\n \t\tdie(_(\"'%s' is a main working tree\"), av[0]);\n+\n+\tif (is_directory(dst.buf)) {\n+\t\tconst char *sep = find_last_dir_sep(wt->path);\n+\n+\t\tif (!sep)\n+\t\t\tdie(_(\"could not figure out destination name from '%s'\"),\n+\t\t\t wt->path);\n+\t\tstrbuf_addstr(&dst, sep);\n+\t\tif (file_exists(dst.buf))\n+\t\t\tdie(_(\"target '%s' already exists\"), dst.buf);\n+\t} else if (file_exists(dst.buf)) {\n+\t\tdie(_(\"target '%s' already exists\"), av[1]);\n+\t}\n+\n \treason = is_worktree_locked(wt);\n \tif (reason) {\n \t\tif (*reason)\n-- \n2.16.0.47.g3d9b0fac3a\n\n"},{"id":"337222","messageId":"20180124095357.19645-6-pclouds@gmail.com","threadId":"47678","inReplyTo":"20180124095357.19645-1-pclouds@gmail.com","subject":"[PATCH 5/7] worktree move: refuse to move worktrees with submodules","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-24T09:53:55Z","receivedAt":"2018-01-24T09:54:44Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Submodules contains .git files with relative paths. After a worktree\nmove, these files need to be updated or they may point to nowhere.\n\nThis is a bandage patch to make sure \"worktree move\" don't break\npeople's worktrees by accident. When .git file update code is in\nplace, this validate_no_submodules() could be removed.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n Documentation/git-worktree.txt | 2 +-\n builtin/worktree.c | 23 +++++++++++++++++++++++\n 2 files changed, 24 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt\nindex 5d115b855a..24bb69de50 100644\n--- a/Documentation/git-worktree.txt\n+++ b/Documentation/git-worktree.txt\n@@ -83,7 +83,7 @@ with `--reason`.\n move::\n \n Move a working tree to a new location. Note that the main working tree\n-cannot be moved.\n+or linked working trees containing submodules cannot be moved.\n \n prune::\n \ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 89398e67e4..15980a0a49 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -606,6 +606,27 @@ static int unlock_worktree(int ac, const char **av, const char *prefix)\n \treturn ret;\n }\n \n+static void validate_no_submodules(const struct worktree *wt)\n+{\n+\tstruct index_state istate = { NULL };\n+\tint i, found_submodules = 0;\n+\n+\tif (read_index_from(&istate, worktree_git_path(wt, \"index\")) > 0) {\n+\t\tfor (i = 0; i < istate.cache_nr; i++) {\n+\t\t\tstruct cache_entry *ce = istate.cache[i];\n+\n+\t\t\tif (S_ISGITLINK(ce->ce_mode)) {\n+\t\t\t\tfound_submodules = 1;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\t}\n+\tdiscard_index(&istate);\n+\n+\tif (found_submodules)\n+\t\tdie(_(\"working trees containing submodules cannot be moved\"));\n+}\n+\n static int move_worktree(int ac, const char **av, const char *prefix)\n {\n \tstruct option options[] = {\n@@ -632,6 +653,8 @@ static int move_worktree(int ac, const char **av, const char *prefix)\n \tif (is_main_worktree(wt))\n \t\tdie(_(\"'%s' is a main working tree\"), av[0]);\n \n+\tvalidate_no_submodules(wt);\n+\n \tif (is_directory(dst.buf)) {\n \t\tconst char *sep = find_last_dir_sep(wt->path);\n \n-- \n2.16.0.47.g3d9b0fac3a\n\n"},{"id":"337223","messageId":"20180124095357.19645-7-pclouds@gmail.com","threadId":"47678","inReplyTo":"20180124095357.19645-1-pclouds@gmail.com","subject":"[PATCH 6/7] worktree remove: new command","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-24T09:53:56Z","receivedAt":"2018-01-24T09:54:54Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This command allows to delete a worktree. Like 'move' you cannot\nremove the main worktree, or one with submodules inside [1].\n\nFor deleting $GIT_WORK_TREE, Untracked files or any staged entries are\nconsidered precious and therefore prevent removal by default. Ignored\nfiles are not precious.\n\nWhen it comes to deleting $GIT_DIR, there's no \"clean\" check because\nthere should not be any valuable data in there, except:\n\n- HEAD reflog. There is nothing we can do about this until somebody\n steps up and implements the ref graveyard.\n\n- Detached HEAD. Technically it can still be recovered. Although it\n may be nice to warn about orphan commits like 'git checkout' does.\n\n[1] We do 'git status' with --ignore-submodules=all for safety\n anyway. But this needs a closer look by submodule people before we\n can allow deletion. For example, if a submodule is totally clean,\n but its repo not absorbed to the main .git dir, then deleting\n worktree also deletes the valuable .submodule repo too.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n Documentation/git-worktree.txt | 21 +++---\n builtin/worktree.c | 131 ++++++++++++++++++++++++++++++++-\n contrib/completion/git-completion.bash | 5 +-\n t/t2028-worktree-move.sh | 26 +++++++\n 4 files changed, 172 insertions(+), 11 deletions(-)\n\ndiff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt\nindex 24bb69de50..6f83723d9a 100644\n--- a/Documentation/git-worktree.txt\n+++ b/Documentation/git-worktree.txt\n@@ -14,6 +14,7 @@ SYNOPSIS\n 'git worktree lock' [--reason ] \n 'git worktree move' \n 'git worktree prune' [-n] [-v] [--expire ]\n+'git worktree remove' [--force] \n 'git worktree unlock' \n \n DESCRIPTION\n@@ -89,6 +90,13 @@ prune::\n \n Prune working tree information in $GIT_DIR/worktrees.\n \n+remove::\n+\n+Remove a working tree. Only clean working trees (no untracked files\n+and no modification in tracked files) can be removed. Unclean working\n+trees or ones with submodules can be removed with `--force`. The main\n+working tree cannot be removed.\n+\n unlock::\n \n Unlock a working tree, allowing it to be pruned, moved or deleted.\n@@ -98,9 +106,10 @@ OPTIONS\n \n -f::\n --force::\n-\tBy default, `add` refuses to create a new working tree when `` is a branch name and\n-\tis already checked out by another working tree. This option overrides\n-\tthat safeguard.\n+\tBy default, `add` refuses to create a new working tree when\n+\t`` is a branch name and is already checked out by\n+\tanother working tree and `remove` refuses to remove an unclean\n+\tworking tree. This option overrides that safeguard.\n \n -b ::\n -B ::\n@@ -282,12 +291,6 @@ Multiple checkout in general is still experimental, and the support\n for submodules is incomplete. It is NOT recommended to make multiple\n checkouts of a superproject.\n \n-git-worktree could provide more automation for tasks currently\n-performed manually, such as:\n-\n-- `remove` to remove a linked working tree and its administrative files (and\n- warn if the working tree is dirty)\n-\n GIT\n ---\n Part of the linkgit:git[1] suite\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 15980a0a49..8b027375d7 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -19,6 +19,7 @@ static const char * const worktree_usage[] = {\n \tN_(\"git worktree lock [] \"),\n \tN_(\"git worktree move \"),\n \tN_(\"git worktree prune []\"),\n+\tN_(\"git worktree remove [] \"),\n \tN_(\"git worktree unlock \"),\n \tNULL\n };\n@@ -624,7 +625,7 @@ static void validate_no_submodules(const struct worktree *wt)\n \tdiscard_index(&istate);\n \n \tif (found_submodules)\n-\t\tdie(_(\"working trees containing submodules cannot be moved\"));\n+\t\tdie(_(\"working trees containing submodules cannot be moved or removed\"));\n }\n \n static int move_worktree(int ac, const char **av, const char *prefix)\n@@ -688,6 +689,132 @@ static int move_worktree(int ac, const char **av, const char *prefix)\n \treturn 0;\n }\n \n+/*\n+ * Note, \"git status --porcelain\" is used to determine if it's safe to\n+ * delete a whole worktree. \"git status\" does not ignore user\n+ * configuration, so if a normal \"git status\" shows \"clean\" for the\n+ * user, then it's ok to remove it.\n+ *\n+ * This assumption may be a bad one. We may want to ignore\n+ * (potentially bad) user settings and only delete a worktree when\n+ * it's absolutely safe to do so from _our_ point of view because we\n+ * know better.\n+ */\n+static void check_clean_worktree(struct worktree *wt,\n+\t\t\t\t const char *original_path)\n+{\n+\tstruct argv_array child_env = ARGV_ARRAY_INIT;\n+\tstruct child_process cp;\n+\tchar buf[1];\n+\tint ret;\n+\n+\tvalidate_no_submodules(wt);\n+\n+\targv_array_pushf(&child_env, \"%s=%s/.git\",\n+\t\t\t GIT_DIR_ENVIRONMENT, wt->path);\n+\targv_array_pushf(&child_env, \"%s=%s\",\n+\t\t\t GIT_WORK_TREE_ENVIRONMENT, wt->path);\n+\tmemset(&cp, 0, sizeof(cp));\n+\targv_array_pushl(&cp.args, \"status\",\n+\t\t\t \"--porcelain\", \"--ignore-submodules=none\",\n+\t\t\t NULL);\n+\tcp.env = child_env.argv;\n+\tcp.git_cmd = 1;\n+\tcp.dir = wt->path;\n+\tcp.out = -1;\n+\tret = start_command(&cp);\n+\tif (ret)\n+\t\tdie_errno(_(\"failed to run git-status on '%s'\"),\n+\t\t\t original_path);\n+\tret = xread(cp.out, buf, sizeof(buf));\n+\tif (ret)\n+\t\tdie(_(\"'%s' is dirty, use --force to delete it\"),\n+\t\t original_path);\n+\tclose(cp.out);\n+\tret = finish_command(&cp);\n+\tif (ret)\n+\t\tdie_errno(_(\"failed to run git-status on '%s', code %d\"),\n+\t\t\t original_path, ret);\n+}\n+\n+static int delete_git_work_tree(struct worktree *wt)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tint ret = 0;\n+\n+\tstrbuf_addstr(&sb, wt->path);\n+\tif (remove_dir_recursively(&sb, 0)) {\n+\t\terror_errno(_(\"failed to delete '%s'\"), sb.buf);\n+\t\tret = -1;\n+\t}\n+\tstrbuf_release(&sb);\n+\n+\treturn ret;\n+}\n+\n+static int delete_git_dir(struct worktree *wt)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tint ret = 0;\n+\n+\tstrbuf_addstr(&sb, git_common_path(\"worktrees/%s\", wt->id));\n+\tif (remove_dir_recursively(&sb, 0)) {\n+\t\terror_errno(_(\"failed to delete '%s'\"), sb.buf);\n+\t\tret = -1;\n+\t}\n+\tstrbuf_release(&sb);\n+\treturn ret;\n+}\n+\n+static int remove_worktree(int ac, const char **av, const char *prefix)\n+{\n+\tint force = 0;\n+\tstruct option options[] = {\n+\t\tOPT_BOOL(0, \"force\", &force,\n+\t\t\t N_(\"force removing even if the worktree is dirty\")),\n+\t\tOPT_END()\n+\t};\n+\tstruct worktree **worktrees, *wt;\n+\tstruct strbuf errmsg = STRBUF_INIT;\n+\tconst char *reason;\n+\tint ret = 0;\n+\n+\tac = parse_options(ac, av, prefix, options, worktree_usage, 0);\n+\tif (ac != 1)\n+\t\tusage_with_options(worktree_usage, options);\n+\n+\tworktrees = get_worktrees(0);\n+\twt = find_worktree(worktrees, prefix, av[0]);\n+\tif (!wt)\n+\t\tdie(_(\"'%s' is not a working tree\"), av[0]);\n+\tif (is_main_worktree(wt))\n+\t\tdie(_(\"'%s' is a main working tree\"), av[0]);\n+\treason = is_worktree_locked(wt);\n+\tif (reason) {\n+\t\tif (*reason)\n+\t\t\tdie(_(\"cannot remove a locked working tree, lock reason: %s\"),\n+\t\t\t reason);\n+\t\tdie(_(\"cannot remove a locked working tree\"));\n+\t}\n+\tif (validate_worktree(wt, &errmsg))\n+\t\tdie(_(\"validation failed, cannot remove working tree:\\n%s\"),\n+\t\t errmsg.buf);\n+\tstrbuf_release(&errmsg);\n+\n+\tif (!force)\n+\t\tcheck_clean_worktree(wt, av[0]);\n+\n+\tret |= delete_git_work_tree(wt);\n+\t/*\n+\t * continue on even if ret is non-zero, there's no going back\n+\t * from here.\n+\t */\n+\tret |= delete_git_dir(wt);\n+\n+\tfree_worktrees(worktrees);\n+\treturn ret;\n+}\n+\n int cmd_worktree(int ac, const char **av, const char *prefix)\n {\n \tstruct option options[] = {\n@@ -712,5 +839,7 @@ int cmd_worktree(int ac, const char **av, const char *prefix)\n \t\treturn unlock_worktree(ac - 1, av + 1, prefix);\n \tif (!strcmp(av[1], \"move\"))\n \t\treturn move_worktree(ac - 1, av + 1, prefix);\n+\tif (!strcmp(av[1], \"remove\"))\n+\t\treturn remove_worktree(ac - 1, av + 1, prefix);\n \tusage_with_options(worktree_usage, options);\n }\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex b87d387686..ff4a39631e 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -3087,7 +3087,7 @@ _git_whatchanged ()\n \n _git_worktree ()\n {\n-\tlocal subcommands=\"add list lock move prune unlock\"\n+\tlocal subcommands=\"add list lock move prune remove unlock\"\n \tlocal subcommand=\"$(__git_find_on_cmdline \"$subcommands\")\"\n \tif [ -z \"$subcommand\" ]; then\n \t\t__gitcomp \"$subcommands\"\n@@ -3105,6 +3105,9 @@ _git_worktree ()\n \t\tprune,--*)\n \t\t\t__gitcomp \"--dry-run --expire --verbose\"\n \t\t\t;;\n+\t\tremove,--*)\n+\t\t\t__gitcomp \"--force\"\n+\t\t\t;;\n \t\t*)\n \t\t\t;;\n \t\tesac\ndiff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh\nindex bef420a086..b3105eaaed 100755\n--- a/t/t2028-worktree-move.sh\n+++ b/t/t2028-worktree-move.sh\n@@ -90,4 +90,30 @@ test_expect_success 'move main worktree' '\n \ttest_must_fail git worktree move . def\n '\n \n+test_expect_success 'remove main worktree' '\n+\ttest_must_fail git worktree remove .\n+'\n+\n+test_expect_success 'remove locked worktree' '\n+\tgit worktree lock destination &&\n+\ttest_must_fail git worktree remove destination &&\n+\tgit worktree unlock destination\n+'\n+\n+test_expect_success 'remove worktree with dirty tracked file' '\n+\techo dirty >>destination/init.t &&\n+\ttest_must_fail git worktree remove destination\n+'\n+\n+test_expect_success 'remove worktree with untracked file' '\n+\tgit -C destination checkout init.t &&\n+\t: >destination/untracked &&\n+\ttest_must_fail git worktree remove destination\n+'\n+\n+test_expect_success 'force remove worktree with untracked file' '\n+\tgit worktree remove --force destination &&\n+\ttest_path_is_missing destination\n+'\n+\n test_done\n-- \n2.16.0.47.g3d9b0fac3a\n\n"},{"id":"337224","messageId":"20180124095357.19645-8-pclouds@gmail.com","threadId":"47678","inReplyTo":"20180124095357.19645-1-pclouds@gmail.com","subject":"[PATCH 7/7] worktree remove: allow it when $GIT_WORK_TREE is already gone","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-24T09:53:57Z","receivedAt":"2018-01-24T09:54:56Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"\"git worktree remove\" basically consists of two things\n\n- delete $GIT_WORK_TREE\n- delete $GIT_DIR (which is $SUPER_GIT_DIR/worktrees/something)\n\nIf $GIT_WORK_TREE is already gone for some reason, we should be able\nto finish the job by deleting $GIT_DIR.\n\nTwo notes:\n\n- $GIT_WORK_TREE _can_ be missing if the worktree is locked. In that\n case we must not delete $GIT_DIR because the real $GIT_WORK_TREE may\n be in a usb stick somewhere. This is already handled because we\n check for lock first.\n\n- validate_worktree() is still called because it may do more checks in\n future (and it already does something else, like checking main\n worktree, but that's irrelevant in this case)\n\nNoticed-by: Kaartic Sivaraam \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/worktree.c | 12 +++++++-----\n t/t2028-worktree-move.sh | 8 ++++++++\n worktree.c | 9 ++++++++-\n worktree.h | 5 ++++-\n 4 files changed, 27 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 8b027375d7..8ce86aef0e 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -676,7 +676,7 @@ static int move_worktree(int ac, const char **av, const char *prefix)\n \t\t\t reason);\n \t\tdie(_(\"cannot move a locked working tree\"));\n \t}\n-\tif (validate_worktree(wt, &errmsg))\n+\tif (validate_worktree(wt, &errmsg, 0))\n \t\tdie(_(\"validation failed, cannot move working tree:\\n%s\"),\n \t\t errmsg.buf);\n \tstrbuf_release(&errmsg);\n@@ -796,15 +796,17 @@ static int remove_worktree(int ac, const char **av, const char *prefix)\n \t\t\t reason);\n \t\tdie(_(\"cannot remove a locked working tree\"));\n \t}\n-\tif (validate_worktree(wt, &errmsg))\n+\tif (validate_worktree(wt, &errmsg, WT_VALIDATE_WORKTREE_MISSING_OK))\n \t\tdie(_(\"validation failed, cannot remove working tree:\\n%s\"),\n \t\t errmsg.buf);\n \tstrbuf_release(&errmsg);\n \n-\tif (!force)\n-\t\tcheck_clean_worktree(wt, av[0]);\n+\tif (file_exists(wt->path)) {\n+\t\tif (!force)\n+\t\t\tcheck_clean_worktree(wt, av[0]);\n \n-\tret |= delete_git_work_tree(wt);\n+\t\tret |= delete_git_work_tree(wt);\n+\t}\n \t/*\n \t * continue on even if ret is non-zero, there's no going back\n \t * from here.\ndiff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh\nindex b3105eaaed..459f676683 100755\n--- a/t/t2028-worktree-move.sh\n+++ b/t/t2028-worktree-move.sh\n@@ -116,4 +116,12 @@ test_expect_success 'force remove worktree with untracked file' '\n \ttest_path_is_missing destination\n '\n \n+test_expect_success 'remove missing worktree' '\n+\tgit worktree add to-be-gone &&\n+\ttest -d .git/worktrees/to-be-gone &&\n+\tmv to-be-gone gone &&\n+\tgit worktree remove to-be-gone &&\n+\ttest_path_is_missing .git/worktrees/to-be-gone\n+'\n+\n test_done\ndiff --git a/worktree.c b/worktree.c\nindex 98e9f32c7f..542196f0ad 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -267,7 +267,8 @@ static void strbuf_addf_gently(struct strbuf *buf, const char *fmt, ...)\n \tva_end(params);\n }\n \n-int validate_worktree(const struct worktree *wt, struct strbuf *errmsg)\n+int validate_worktree(const struct worktree *wt, struct strbuf *errmsg,\n+\t\t unsigned flags)\n {\n \tstruct strbuf wt_path = STRBUF_INIT;\n \tchar *path = NULL;\n@@ -303,6 +304,12 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg)\n \t\tgoto done;\n \t}\n \n+\tif (flags & WT_VALIDATE_WORKTREE_MISSING_OK &&\n+\t !file_exists(wt->path)) {\n+\t\tret = 0;\n+\t\tgoto done;\n+\t}\n+\n \tif (!file_exists(wt_path.buf)) {\n \t\tstrbuf_addf_gently(errmsg, _(\"'%s' does not exist\"), wt_path.buf);\n \t\tgoto done;\ndiff --git a/worktree.h b/worktree.h\nindex a913428c3d..fe38ce10c3 100644\n--- a/worktree.h\n+++ b/worktree.h\n@@ -61,12 +61,15 @@ extern int is_main_worktree(const struct worktree *wt);\n */\n extern const char *is_worktree_locked(struct worktree *wt);\n \n+#define WT_VALIDATE_WORKTREE_MISSING_OK (1 << 0)\n+\n /*\n * Return zero if the worktree is in good condition. Error message is\n * returned if \"errmsg\" is not NULL.\n */\n extern int validate_worktree(const struct worktree *wt,\n-\t\t\t struct strbuf *errmsg);\n+\t\t\t struct strbuf *errmsg,\n+\t\t\t unsigned flags);\n \n /*\n * Update worktrees/xxx/gitdir with the new path.\n-- \n2.16.0.47.g3d9b0fac3a\n\n"},{"id":"337300","messageId":"xmqqshavc7ku.fsf@gitster.mtv.corp.google.com","threadId":"47678","inReplyTo":"20180124095357.19645-1-pclouds@gmail.com","subject":"Re: [PATCH 0/7] nd/worktree-move reboot","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-01-24T20:42:09Z","receivedAt":"2018-01-24T20:42:16Z","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> This series adds two more commands \"git worktree move\" and \"git\n> worktree remove\" to do those things. I think I have addressed all\n> comments from the mail threads referenced in \"What's cooking\" mails. I\n> also added the ability to remove a worktree if its worktree area is\n> already deleted.\n\nThanks for moving this forward.\n\n"},{"id":"338081","messageId":"CAPig+cQzdFhUzwDykO8su=ne0qZr6=51vTxXbMYYnceuH_N=iw@mail.gmail.com","threadId":"47678","inReplyTo":"20180124095357.19645-3-pclouds@gmail.com","subject":"Re: [PATCH 2/7] worktree.c: add update_worktree_location()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-02-02T08:23:45Z","receivedAt":"2018-02-02T08:23:53Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jan 24, 2018 at 4:53 AM, Nguyễn Thái Ngọc Duy wrote:\n> diff --git a/worktree.c b/worktree.c\n> @@ -326,6 +326,24 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg)\n> +void update_worktree_location(struct worktree *wt, const char *path_)\n> +{\n> + struct strbuf path = STRBUF_INIT;\n> +\n> + if (is_main_worktree(wt))\n> + die(\"BUG: can't relocate main worktree\");\n> +\n> + strbuf_add_absolute_path(&path, path_);\n> + if (fspathcmp(wt->path, path.buf)) {\n> + write_file(git_common_path(\"worktrees/%s/gitdir\",\n> + wt->id),\n> + \"%s/.git\", real_path(path.buf));\n\nFor the path stored in 'worktrees//gitdir' (and in wt->path), this\nand other worktree-related code sometimes treats it only as \"absolute\npath\" and sometimes as \"real path\". As a reviewer, I'm having trouble\nunderstanding the logic of why, how, and when this distinction is\nmade. Can you explain a bit to help clarify when \"absolute path\" is\ngood enough and when \"real path\" is needed?\n\n> + free(wt->path);\n> + wt->path = strbuf_detach(&path, NULL);\n> + }\n> + strbuf_release(&path);\n> +}\n"},{"id":"338082","messageId":"CAPig+cQeQRppb2y4YyQnqWWnCO0TXE-PjfTAhxakCJNk45ec1w@mail.gmail.com","threadId":"47678","inReplyTo":"20180124095357.19645-4-pclouds@gmail.com","subject":"Re: [PATCH 3/7] worktree move: new command","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-02-02T09:15:40Z","receivedAt":"2018-02-02T09:15:51Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jan 24, 2018 at 4:53 AM, Nguyễn Thái Ngọc Duy wrote:\n> This command allows to relocate linked worktrees. Main worktree cannot\n> (yet) be moved.\n> [...]\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt\n> @@ -79,6 +80,11 @@ files from being pruned automatically. This also prevents it from\n> +move::\n> +\n> +Move a working tree to a new location. Note that the main working tree\n> +cannot be moved.\n> +\n> @@ -281,7 +287,6 @@ performed manually, such as:\n> - `remove` to remove a linked working tree and its administrative files (and\n> warn if the working tree is dirty)\n> -- `mv` to move or rename a working tree and update its administrative files\n\nA couple other places in this document ought to be updated.\nSpecifically, in DESCRIPTION:\n\n If you move a linked working tree, you need to manually update the\n administrative files so that they do not get pruned automatically.\n See section \"DETAILS\" for more information.\n\nwhich can probably be dropped in its entirety. And, in DETAILS:\n\n If you move a linked working tree, you need to update the 'gitdir'\n file...\n\nshould probably be reworded to\n\n If you _manually_ move a linked working tree, you need to update\n the 'gitdir' file...\n\n> diff --git a/builtin/worktree.c b/builtin/worktree.c\n> @@ -605,6 +606,53 @@ static int unlock_worktree(int ac, const char **av, const char *prefix)\n> +static int move_worktree(int ac, const char **av, const char *prefix)\n> +{\n> + [...]\n> + worktrees = get_worktrees(0);\n> + wt = find_worktree(worktrees, prefix, av[0]);\n> + if (!wt)\n> + die(_(\"'%s' is not a working tree\"), av[0]);\n\nThis is still leaking 'worktrees'[1]. You probably want\nfree_worktrees() immediately after the find_worktree() invocation.\n\n> + if (is_main_worktree(wt))\n> + die(_(\"'%s' is a main working tree\"), av[0]);\n> + reason = is_worktree_locked(wt);\n> + if (reason) {\n> + if (*reason)\n> + die(_(\"cannot move a locked working tree, lock reason: %s\"),\n> + reason);\n> + die(_(\"cannot move a locked working tree\"));\n> + }\n> + if (validate_worktree(wt, &errmsg))\n> + die(_(\"validation failed, cannot move working tree:\\n%s\"),\n> + errmsg.buf);\n\nMinor: All the other error messages are presented on a single line.\nDespite this error message potentially being quite long, it worries me\na bit that presenting it on two lines could complicate scripted\nclients if they need to collect the error message for special\nhandling.\n\n> diff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh\n> @@ -59,4 +59,35 @@ test_expect_success 'unlock worktree twice' '\n> +test_expect_success 'move locked worktree' '\n> + git worktree lock source &&\n> + test_must_fail git worktree move source destination &&\n> + git worktree unlock source\n> +'\n\nAlso from [1], wrapping 'unlock' inside a test_when_finished()\ninvocation seems potentially desirable.\n\n> +test_expect_success 'move worktree' '\n> + toplevel=\"$(pwd)\" &&\n> + git worktree move source destination &&\n> + test_path_is_missing source &&\n> + git worktree list --porcelain | grep \"^worktree\" >actual &&\n> + cat <<-EOF >expected &&\n> + worktree $toplevel\n> + worktree $toplevel/destination\n> + worktree $toplevel/elsewhere\n> + EOF\n> + test_cmp expected actual &&\n\nThis seems somewhat fragile. If someone inserts a test before this one\nwhich adds another worktree, then this test would break. Perhaps the\nfiltering of the --porcelain output could be more strict? (Not\nnecessarily worth a re-roll, though.)\n\n> + git -C destination log --format=%s >actual2 &&\n> + echo init >expected2 &&\n> + test_cmp expected2 actual2\n> +'\n\n[1]: https://public-inbox.org/git/CAPig+cRqgzB4tiTb=fHuFBTqo3QEGM3m378PvOse06KdreKo2Q@mail.gmail.com/\n"},{"id":"338085","messageId":"CAPig+cTAmzAK2vQHuPHJJMT+Q2fiD4P--qv_rSrc2gKbFFhPTg@mail.gmail.com","threadId":"47678","inReplyTo":"20180124095357.19645-5-pclouds@gmail.com","subject":"Re: [PATCH 4/7] worktree move: accept destination as directory","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-02-02T09:35:17Z","receivedAt":"2018-02-02T09:35:25Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jan 24, 2018 at 4:53 AM, Nguyễn Thái Ngọc Duy wrote:\n> Similar to \"mv a b/\", which is actually \"mv a b/a\", we extract basename\n> of source worktree and create a directory of the same name at\n> destination if dst path is a directory.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> diff --git a/builtin/worktree.c b/builtin/worktree.c\n> @@ -624,8 +624,6 @@ static int move_worktree(int ac, const char **av, const char *prefix)\n> path = prefix_filename(prefix, av[1]);\n> strbuf_addstr(&dst, path);\n> free(path);\n> - if (file_exists(dst.buf))\n> - die(_(\"target '%s' already exists\"), av[1]);\n\nNit: The distance between this \"removed\" conditional and the code\ninserted below hampered review a bit since it made it slightly more\nonerous to check for unwanted logic changes. Had patch 3/7 located\nthis conditional below the is_main_worktree() check (where the new\ncode is inserted below), this 'if' would have merely mutated into an\n'else if' but not otherwise moved, thus review would have been\nsimplified. (Not itself worth a re-roll.)\n\n> worktrees = get_worktrees(0);\n> wt = find_worktree(worktrees, prefix, av[0]);\n> @@ -633,6 +631,20 @@ static int move_worktree(int ac, const char **av, const char *prefix)\n> die(_(\"'%s' is not a working tree\"), av[0]);\n> if (is_main_worktree(wt))\n> die(_(\"'%s' is a main working tree\"), av[0]);\n> +\n> + if (is_directory(dst.buf)) {\n> + const char *sep = find_last_dir_sep(wt->path);\n> +\n> + if (!sep)\n> + die(_(\"could not figure out destination name from '%s'\"),\n> + wt->path);\n> + strbuf_addstr(&dst, sep);\n\nDo we know at this point whether 'dst' has a terminating \"/\"? If it\ndoes, then this addstr() could result in \"//\" in the path which might\nbe problematic on Windows.\n\n> + if (file_exists(dst.buf))\n> + die(_(\"target '%s' already exists\"), dst.buf);\n> + } else if (file_exists(dst.buf)) {\n> + die(_(\"target '%s' already exists\"), av[1]);\n> + }\n\nI wonder if it makes sense to collapse the duplicated\nfile_exists()/\"target already exists\" code?\n\n if (is_directory(...)) {\n ...\n strbuf_addstr(&dst, sep);\n }\n if (file_exists(dst.buf))\n die(_(\"target '%s' already exists\"), dst.buf);\n\nIt changes the error message slightly for the non-directory case but\nsimplifies the code a bit.\n\n> reason = is_worktree_locked(wt);\n> if (reason) {\n> if (*reason)\n\nPerhaps add a test to t2028-worktree-move.sh to show that this new\nbehavior works?\n"},{"id":"338086","messageId":"CACsJy8ALCro_4n0u2TBp8Lon_2kX49w9UcAnD8awHmsoriQ7-Q@mail.gmail.com","threadId":"47678","inReplyTo":"CAPig+cQzdFhUzwDykO8su=ne0qZr6=51vTxXbMYYnceuH_N=iw@mail.gmail.com","subject":"Re: [PATCH 2/7] worktree.c: add update_worktree_location()","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-02-02T09:35:35Z","receivedAt":"2018-02-02T09:36:11Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Feb 2, 2018 at 3:23 PM, Eric Sunshine wrote:\n> On Wed, Jan 24, 2018 at 4:53 AM, Nguyễn Thái Ngọc Duy wrote:\n>> diff --git a/worktree.c b/worktree.c\n>> @@ -326,6 +326,24 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg)\n>> +void update_worktree_location(struct worktree *wt, const char *path_)\n>> +{\n>> + struct strbuf path = STRBUF_INIT;\n>> +\n>> + if (is_main_worktree(wt))\n>> + die(\"BUG: can't relocate main worktree\");\n>> +\n>> + strbuf_add_absolute_path(&path, path_);\n>> + if (fspathcmp(wt->path, path.buf)) {\n>> + write_file(git_common_path(\"worktrees/%s/gitdir\",\n>> + wt->id),\n>> + \"%s/.git\", real_path(path.buf));\n>\n> For the path stored in 'worktrees//gitdir' (and in wt->path), this\n> and other worktree-related code sometimes treats it only as \"absolute\n> path\" and sometimes as \"real path\". As a reviewer, I'm having trouble\n> understanding the logic of why, how, and when this distinction is\n> made. Can you explain a bit to help clarify when \"absolute path\" is\n> good enough and when \"real path\" is needed?\n\nOf the top of my head, I think it's \"anything but a relative path\".\nreal_path() is normally used to normalize paths before I compare them.\nWriting a normalized path down is nice to avoid \"../\" but I don't\nthink it's strictly necessary. Well, it's also prettier when you have\nto print the path out. Printing \"/foo/bar/abc/../../xyz\" is not nice.\n\nHmm.. looking back in add_worktree(), we also write \"gitdir\" with\nreal_path(). I think the problem here is that I should have\nreal_path()'d \"path.buf\" _before_ doing fspathcmp(). Maybe that's\nwhere the confusion's from.\n-- \nDuy\n"},{"id":"338091","messageId":"CAPig+cSLMt_9+5-8f_MndJM0hHgp32Q1CUmaosfJcbGEV8j-zg@mail.gmail.com","threadId":"47678","inReplyTo":"20180124095357.19645-6-pclouds@gmail.com","subject":"Re: [PATCH 5/7] worktree move: refuse to move worktrees with submodules","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-02-02T10:06:39Z","receivedAt":"2018-02-02T10:06:45Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jan 24, 2018 at 4:53 AM, Nguyễn Thái Ngọc Duy wrote:\n> Submodules contains .git files with relative paths. After a worktree\n> move, these files need to be updated or they may point to nowhere.\n>\n> This is a bandage patch to make sure \"worktree move\" don't break\n> people's worktrees by accident. When .git file update code is in\n> place, this validate_no_submodules() could be removed.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> diff --git a/builtin/worktree.c b/builtin/worktree.c\n> @@ -606,6 +606,27 @@ static int unlock_worktree(int ac, const char **av, const char *prefix)\n> +static void validate_no_submodules(const struct worktree *wt)\n> +{\n> + struct index_state istate = { NULL };\n> + int i, found_submodules = 0;\n> +\n> + if (read_index_from(&istate, worktree_git_path(wt, \"index\")) > 0) {\n> + for (i = 0; i < istate.cache_nr; i++) {\n> + struct cache_entry *ce = istate.cache[i];\n> +\n> + if (S_ISGITLINK(ce->ce_mode)) {\n> + found_submodules = 1;\n> + break;\n> + }\n> + }\n> + }\n> + discard_index(&istate);\n> +\n> + if (found_submodules)\n> + die(_(\"working trees containing submodules cannot be moved\"));\n\nMinor (not worth a re-roll): This could be simplified slightly by\ndie()ing inside the loop rather than having 'found_submodules' and\nbreaking from the loop. Doing so will leak 'istate', but it's die()ing\nanyhow, so should not be an issue (unless this code is someday\nlibified).\n\n> +}\n"},{"id":"338092","messageId":"CAPig+cRYL6-jEgsQoiQokNSd0zq5zoFkMHBA0Vp9kqa0LvMnsA@mail.gmail.com","threadId":"47678","inReplyTo":"CAPig+cQeQRppb2y4YyQnqWWnCO0TXE-PjfTAhxakCJNk45ec1w@mail.gmail.com","subject":"Re: [PATCH 3/7] worktree move: new command","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-02-02T11:23:00Z","receivedAt":"2018-02-02T11:23:06Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Feb 2, 2018 at 4:15 AM, Eric Sunshine wrote:\n> On Wed, Jan 24, 2018 at 4:53 AM, Nguyễn Thái Ngọc Duy wrote:\n>> +static int move_worktree(int ac, const char **av, const char *prefix)\n>> +{\n>> + [...]\n>> + worktrees = get_worktrees(0);\n>> + wt = find_worktree(worktrees, prefix, av[0]);\n>> + if (!wt)\n>> + die(_(\"'%s' is not a working tree\"), av[0]);\n>\n> This is still leaking 'worktrees'[1]. You probably want\n> free_worktrees() immediately after the find_worktree() invocation.\n\nSorry, free_worktrees() after the last use of 'wt' since you still\nneed to access its fields, which would be the end of the function.\n"},{"id":"338093","messageId":"CAPig+cQvVreeggOj52bVLS6R0ZCar_yfUcyLbN3-nLG81eOQaA@mail.gmail.com","threadId":"47678","inReplyTo":"20180124095357.19645-7-pclouds@gmail.com","subject":"Re: [PATCH 6/7] worktree remove: new command","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-02-02T11:47:13Z","receivedAt":"2018-02-02T11:47:20Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jan 24, 2018 at 4:53 AM, Nguyễn Thái Ngọc Duy wrote:\n> This command allows to delete a worktree. Like 'move' you cannot\n> remove the main worktree, or one with submodules inside [1].\n> [...]\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> diff --git a/builtin/worktree.c b/builtin/worktree.c\n> @@ -688,6 +689,132 @@ static int move_worktree(int ac, const char **av, const char *prefix)\n> +static void check_clean_worktree(struct worktree *wt,\n> + const char *original_path)\n> +{\n> + [...]\n> + validate_no_submodules(wt);\n\nIt's slightly strange seeing worktree validation in a function\nchecking whether the worktree is clean since submodule validation\nisn't an issue of cleanliness. I'd have expected the caller to invoke\nit instead:\n\n int remove_worktree(...) {\n ...\n if (!force) {\n validate_no_submodules(wt);\n check_clean_worktree(wt, av[0]);\n }\n ...\n }\n\nOn the other hand, I could imagine it being called here as appropriate\nif submodule validation eventually also checks submodule cleanliness\nas hinted by the commit message.\n\n> + argv_array_pushf(&child_env, \"%s=%s/.git\",\n> + GIT_DIR_ENVIRONMENT, wt->path);\n> + argv_array_pushf(&child_env, \"%s=%s\",\n> + GIT_WORK_TREE_ENVIRONMENT, wt->path);\n> + memset(&cp, 0, sizeof(cp));\n> + argv_array_pushl(&cp.args, \"status\",\n> + \"--porcelain\", \"--ignore-submodules=none\",\n> + NULL);\n> + cp.env = child_env.argv;\n> + cp.git_cmd = 1;\n> + cp.dir = wt->path;\n> + cp.out = -1;\n> + ret = start_command(&cp);\n> + if (ret)\n> + die_errno(_(\"failed to run git-status on '%s'\"),\n> + original_path);\n\nMinor: I think there was some effort recently to remove \"git-foo\"\nstyle mentions from documentation and error messages. Perhaps this\ncould be \"failed to run 'git status' on '%s'\". Ditto below.\n\n> + ret = xread(cp.out, buf, sizeof(buf));\n> + if (ret)\n> + die(_(\"'%s' is dirty, use --force to delete it\"),\n> + original_path);\n> + close(cp.out);\n> + ret = finish_command(&cp);\n> + if (ret)\n> + die_errno(_(\"failed to run git-status on '%s', code %d\"),\n> + original_path, ret);\n> +}\n> +\n> +static int delete_git_work_tree(struct worktree *wt)\n> +{\n> + struct strbuf sb = STRBUF_INIT;\n> + int ret = 0;\n> +\n> + strbuf_addstr(&sb, wt->path);\n> + if (remove_dir_recursively(&sb, 0)) {\n> + error_errno(_(\"failed to delete '%s'\"), sb.buf);\n> + ret = -1;\n> + }\n> + strbuf_release(&sb);\n> +\n> + return ret;\n> +}\n\nStyle nit: In the very similar delete_git_dir(), just below, there is\nno blank line before 'return'.\n\n> +\n> +static int delete_git_dir(struct worktree *wt)\n> +{\n> + struct strbuf sb = STRBUF_INIT;\n> + int ret = 0;\n> +\n> + strbuf_addstr(&sb, git_common_path(\"worktrees/%s\", wt->id));\n> + if (remove_dir_recursively(&sb, 0)) {\n> + error_errno(_(\"failed to delete '%s'\"), sb.buf);\n> + ret = -1;\n> + }\n> + strbuf_release(&sb);\n> + return ret;\n> +}\n> +\n> +static int remove_worktree(int ac, const char **av, const char *prefix)\n> +{\n> + [...]\n> + if (reason) {\n> + if (*reason)\n> + die(_(\"cannot remove a locked working tree, lock reason: %s\"),\n> + reason);\n> + die(_(\"cannot remove a locked working tree\"));\n> + }\n> + if (validate_worktree(wt, &errmsg))\n> + die(_(\"validation failed, cannot remove working tree:\\n%s\"),\n> + errmsg.buf);\n\nMinor: Same concern as in 3/7 about the multi-line error message\nmaking scripted handling of error message collection more difficult.\n\n> + strbuf_release(&errmsg);\n> +\n> + if (!force)\n> + check_clean_worktree(wt, av[0]);\n> +\n> + ret |= delete_git_work_tree(wt);\n> + /*\n> + * continue on even if ret is non-zero, there's no going back\n> + * from here.\n> + */\n> + ret |= delete_git_dir(wt);\n> +\n> + free_worktrees(worktrees);\n> + return ret;\n> +}\n> diff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh\n> @@ -90,4 +90,30 @@ test_expect_success 'move main worktree' '\n> +test_expect_success 'remove locked worktree' '\n> + git worktree lock destination &&\n> + test_must_fail git worktree remove destination &&\n> + git worktree unlock destination\n> +'\n\nPerhaps place 'unlock' in test_when_finished()[1].\n\n> +test_expect_success 'remove worktree with dirty tracked file' '\n> + echo dirty >>destination/init.t &&\n> + test_must_fail git worktree remove destination\n> +'\n> +\n> +test_expect_success 'remove worktree with untracked file' '\n> + git -C destination checkout init.t &&\n\nReversion of 'init.t' probably belongs in the preceding test which\nmodified it, wrapped in test_when_finished()[1].\n\n> + : >destination/untracked &&\n> + test_must_fail git worktree remove destination\n> +'\n\n[1]: https://public-inbox.org/git/CAPig+cSV9_6j6Nkptma3BewKW8QQcem7gwFCb42VBW4Xe0Vr2w@mail.gmail.com/\n"},{"id":"338094","messageId":"CAPig+cR=kPhRuY=2NPFsPcVOby90bf+zS_hJ_60H+2QTQgjfcQ@mail.gmail.com","threadId":"47678","inReplyTo":"20180124095357.19645-8-pclouds@gmail.com","subject":"Re: [PATCH 7/7] worktree remove: allow it when $GIT_WORK_TREE is already gone","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-02-02T12:59:29Z","receivedAt":"2018-02-02T12:59:36Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jan 24, 2018 at 4:53 AM, Nguyễn Thái Ngọc Duy wrote:\n> [...]\n> - $GIT_WORK_TREE _can_ be missing if the worktree is locked. In that\n> case we must not delete $GIT_DIR because the real $GIT_WORK_TREE may\n> be in a usb stick somewhere. This is already handled because we\n> check for lock first.\n> [...]\n>\n> Noticed-by: Kaartic Sivaraam \n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> diff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh\n> @@ -116,4 +116,12 @@ test_expect_success 'force remove worktree with untracked file' '\n> +test_expect_success 'remove missing worktree' '\n> + git worktree add to-be-gone &&\n> + test -d .git/worktrees/to-be-gone &&\n> + mv to-be-gone gone &&\n> + git worktree remove to-be-gone &&\n> + test_path_is_missing .git/worktrees/to-be-gone\n> +'\n\nPerhaps there could also be a test to verify that a missing but locked\nworktree is _not_ removed?\n"},{"id":"338214","messageId":"CACsJy8A+zMTC2N8Y0Ua-KyLF6Wp2oHL=8mbsSXck6mKZGG=37g@mail.gmail.com","threadId":"47678","inReplyTo":"CAPig+cRYL6-jEgsQoiQokNSd0zq5zoFkMHBA0Vp9kqa0LvMnsA@mail.gmail.com","subject":"Re: [PATCH 3/7] worktree move: new command","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-02-05T13:28:10Z","receivedAt":"2018-02-05T13:28:46Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Feb 2, 2018 at 6:23 PM, Eric Sunshine wrote:\n> On Fri, Feb 2, 2018 at 4:15 AM, Eric Sunshine wrote:\n>> On Wed, Jan 24, 2018 at 4:53 AM, Nguyễn Thái Ngọc Duy wrote:\n>>> +static int move_worktree(int ac, const char **av, const char *prefix)\n>>> +{\n>>> + [...]\n>>> + worktrees = get_worktrees(0);\n>>> + wt = find_worktree(worktrees, prefix, av[0]);\n>>> + if (!wt)\n>>> + die(_(\"'%s' is not a working tree\"), av[0]);\n>>\n>> This is still leaking 'worktrees'[1]. You probably want\n>> free_worktrees() immediately after the find_worktree() invocation.\n>\n> Sorry, free_worktrees() after the last use of 'wt' since you still\n> need to access its fields, which would be the end of the function.\n\nI learned SANITIZE=leak today! It not only catches this but also \"dst\".\n\nJeff is there any ongoing effort to make the test suite pass with\nSANITIZE=leak? My t2038 passed, so I went ahead with the full test\nsuite and saw so many failures. I did see in your original mails that\nyou focused on t0000 and t0001 only..\n-- \nDuy\n"},{"id":"338469","messageId":"20180206021313.GA26323@sigill.intra.peff.net","threadId":"47678","inReplyTo":"CACsJy8A+zMTC2N8Y0Ua-KyLF6Wp2oHL=8mbsSXck6mKZGG=37g@mail.gmail.com","subject":"Re: [PATCH 3/7] worktree move: new command","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-06T02:13:14Z","receivedAt":"2018-02-06T02:13:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 05, 2018 at 08:28:10PM +0700, Duy Nguyen wrote:\n\n> >> This is still leaking 'worktrees'[1]. You probably want\n> >> free_worktrees() immediately after the find_worktree() invocation.\n> >\n> > Sorry, free_worktrees() after the last use of 'wt' since you still\n> > need to access its fields, which would be the end of the function.\n> \n> I learned SANITIZE=leak today! It not only catches this but also \"dst\".\n> \n> Jeff is there any ongoing effort to make the test suite pass with\n> SANITIZE=leak? My t2038 passed, so I went ahead with the full test\n> suite and saw so many failures. I did see in your original mails that\n> you focused on t0000 and t0001 only..\n\nYeah, I did those two scripts to try to prove to myself that the\napproach was good. But I haven't really pushed it any further.\n\nMartin Ågren (cc'd) did some follow-up work, but I think we still have a\nlong way to go. My hope is that people who are interested in\nleak-checking their new code can run some specific script they're\ninterested in, and maybe fix up one or two nearby bits while they're\nthere (either by fixing a leak, or just annotating via UNLEAK). Then we\ncan slowly converge on correctness. :)\n\n-Peff\n"},{"id":"338531","messageId":"CAN0heSofAfuXEUKcfF00ni_a7nAKJYySF_UEnNTL8+nHiBzw-A@mail.gmail.com","threadId":"47678","inReplyTo":"20180206021313.GA26323@sigill.intra.peff.net","subject":"Re: [PATCH 3/7] worktree move: new command","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-02-06T20:05:08Z","receivedAt":"2018-02-06T20:05:19Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 6 February 2018 at 03:13, Jeff King wrote:\n> On Mon, Feb 05, 2018 at 08:28:10PM +0700, Duy Nguyen wrote:\n>> I learned SANITIZE=leak today! It not only catches this but also \"dst\".\n>>\n>> Jeff is there any ongoing effort to make the test suite pass with\n>> SANITIZE=leak? My t2038 passed, so I went ahead with the full test\n>> suite and saw so many failures. I did see in your original mails that\n>> you focused on t0000 and t0001 only..\n>\n> Yeah, I did those two scripts to try to prove to myself that the\n> approach was good. But I haven't really pushed it any further.\n>\n> Martin Ågren (cc'd) did some follow-up work, but I think we still have a\n> long way to go.\n\nAgreed. :-)\n\n> My hope is that people who are interested in\n> leak-checking their new code can run some specific script they're\n> interested in, and maybe fix up one or two nearby bits while they're\n> there (either by fixing a leak, or just annotating via UNLEAK). Then we\n> can slowly converge on correctness. :)\n\nYeah. My experience is that it's easy -- or was for me, anyway -- to\nget carried away trying to fix all \"related\" leaks to the one you\nstarted with, since there are so many dimensions to search in. Two\nobvious aspects are \"leaks nearby\" and \"leaks using the same API\". This\nis not to suggest that the situation is horrible. It's just that if you\npick a leak at random and there is a fair number of \"clusters\" of\nleaks, chances are good you'll find yourself in such a cluster.\n\nAddressing a leak without worrying too much about how it generalizes\nwould still help. ;-)\n\nMartin\n"},{"id":"339063","messageId":"CACsJy8BsdkZFWE2vaWtobMMum85U5vu-iu9MctaWn6vzqsZ3Vg@mail.gmail.com","threadId":"47678","inReplyTo":"CAPig+cQvVreeggOj52bVLS6R0ZCar_yfUcyLbN3-nLG81eOQaA@mail.gmail.com","subject":"Re: [PATCH 6/7] worktree remove: new command","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-02-12T09:26:16Z","receivedAt":"2018-02-12T09:26:53Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Feb 2, 2018 at 6:47 PM, Eric Sunshine wrote:\n> On Wed, Jan 24, 2018 at 4:53 AM, Nguyễn Thái Ngọc Duy wrote:\n>> This command allows to delete a worktree. Like 'move' you cannot\n>> remove the main worktree, or one with submodules inside [1].\n>> [...]\n>> Signed-off-by: Nguyễn Thái Ngọc Duy \n>> ---\n>> diff --git a/builtin/worktree.c b/builtin/worktree.c\n>> @@ -688,6 +689,132 @@ static int move_worktree(int ac, const char **av, const char *prefix)\n>> +static void check_clean_worktree(struct worktree *wt,\n>> + const char *original_path)\n>> +{\n>> + [...]\n>> + validate_no_submodules(wt);\n>\n> It's slightly strange seeing worktree validation in a function\n> checking whether the worktree is clean since submodule validation\n> isn't an issue of cleanliness. I'd have expected the caller to invoke\n> it instead:\n>\n> int remove_worktree(...) {\n> ...\n> if (!force) {\n> validate_no_submodules(wt);\n> check_clean_worktree(wt, av[0]);\n> }\n> ...\n> }\n>\n> On the other hand, I could imagine it being called here as appropriate\n> if submodule validation eventually also checks submodule cleanliness\n> as hinted by the commit message.\n\nYes I basically consider all submodules dirty until somebody sorts\nthem out. I'll add a comment here to clarify this.\n-- \nDuy\n"},{"id":"339064","messageId":"20180212094940.23834-1-pclouds@gmail.com","threadId":"47678","inReplyTo":"20180124095357.19645-1-pclouds@gmail.com","subject":"[PATCH v2 0/7] nd/worktree-move reboot","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-02-12T09:49:33Z","receivedAt":"2018-02-12T09:50:13Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"v2 basically fixes lots of comments from Eric (many thanks!): memory\nleak, typos, document updates, tests, corner case fixes.\n\nInterdiff:\n\ndiff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt\nindex 6f83723d9a..d322acbc67 100644\n--- a/Documentation/git-worktree.txt\n+++ b/Documentation/git-worktree.txt\n@@ -36,10 +36,6 @@ The working tree's administrative files in the repository (see\n `git worktree prune` in the main or any linked working tree to\n clean up any stale administrative files.\n \n-If you move a linked working tree, you need to manually update the\n-administrative files so that they do not get pruned automatically. See\n-section \"DETAILS\" for more information.\n-\n If a linked working tree is stored on a portable device or network share\n which is not always mounted, you can prevent its administrative files from\n being pruned by issuing the `git worktree lock` command, optionally\n@@ -211,7 +207,7 @@ thumb is do not make any assumption about whether a path belongs to\n $GIT_DIR or $GIT_COMMON_DIR when you need to directly access something\n inside $GIT_DIR. Use `git rev-parse --git-path` to get the final path.\n \n-If you move a linked working tree, you need to update the 'gitdir' file\n+If you manually move a linked working tree, you need to update the 'gitdir' file\n in the entry's directory. For example, if a linked working tree is moved\n to `/newpath/test-next` and its `.git` file points to\n `/path/main/.git/worktrees/test-next`, then update\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 8ce86aef0e..f77ef994c4 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -653,21 +653,19 @@ static int move_worktree(int ac, const char **av, const char *prefix)\n \t\tdie(_(\"'%s' is not a working tree\"), av[0]);\n \tif (is_main_worktree(wt))\n \t\tdie(_(\"'%s' is a main working tree\"), av[0]);\n-\n-\tvalidate_no_submodules(wt);\n-\n \tif (is_directory(dst.buf)) {\n \t\tconst char *sep = find_last_dir_sep(wt->path);\n \n \t\tif (!sep)\n \t\t\tdie(_(\"could not figure out destination name from '%s'\"),\n \t\t\t wt->path);\n+\t\tstrbuf_trim_trailing_dir_sep(&dst);\n \t\tstrbuf_addstr(&dst, sep);\n-\t\tif (file_exists(dst.buf))\n-\t\t\tdie(_(\"target '%s' already exists\"), dst.buf);\n-\t} else if (file_exists(dst.buf)) {\n-\t\tdie(_(\"target '%s' already exists\"), av[1]);\n \t}\n+\tif (file_exists(dst.buf))\n+\t\tdie(_(\"target '%s' already exists\"), dst.buf);\n+\n+\tvalidate_no_submodules(wt);\n \n \treason = is_worktree_locked(wt);\n \tif (reason) {\n@@ -677,7 +675,7 @@ static int move_worktree(int ac, const char **av, const char *prefix)\n \t\tdie(_(\"cannot move a locked working tree\"));\n \t}\n \tif (validate_worktree(wt, &errmsg, 0))\n-\t\tdie(_(\"validation failed, cannot move working tree:\\n%s\"),\n+\t\tdie(_(\"validation failed, cannot move working tree: %s\"),\n \t\t errmsg.buf);\n \tstrbuf_release(&errmsg);\n \n@@ -686,6 +684,8 @@ static int move_worktree(int ac, const char **av, const char *prefix)\n \n \tupdate_worktree_location(wt, dst.buf);\n \n+\tstrbuf_release(&dst);\n+\tfree_worktrees(worktrees);\n \treturn 0;\n }\n \n@@ -708,6 +708,10 @@ static void check_clean_worktree(struct worktree *wt,\n \tchar buf[1];\n \tint ret;\n \n+\t/*\n+\t * Until we sort this out, all submodules are \"dirty\" and\n+\t * will abort this function.\n+\t */\n \tvalidate_no_submodules(wt);\n \n \targv_array_pushf(&child_env, \"%s=%s/.git\",\n@@ -724,7 +728,7 @@ static void check_clean_worktree(struct worktree *wt,\n \tcp.out = -1;\n \tret = start_command(&cp);\n \tif (ret)\n-\t\tdie_errno(_(\"failed to run git-status on '%s'\"),\n+\t\tdie_errno(_(\"failed to run 'git status' on '%s'\"),\n \t\t\t original_path);\n \tret = xread(cp.out, buf, sizeof(buf));\n \tif (ret)\n@@ -733,7 +737,7 @@ static void check_clean_worktree(struct worktree *wt,\n \tclose(cp.out);\n \tret = finish_command(&cp);\n \tif (ret)\n-\t\tdie_errno(_(\"failed to run git-status on '%s', code %d\"),\n+\t\tdie_errno(_(\"failed to run 'git status' on '%s', code %d\"),\n \t\t\t original_path, ret);\n }\n \n@@ -748,7 +752,6 @@ static int delete_git_work_tree(struct worktree *wt)\n \t\tret = -1;\n \t}\n \tstrbuf_release(&sb);\n-\n \treturn ret;\n }\n \n@@ -797,7 +800,7 @@ static int remove_worktree(int ac, const char **av, const char *prefix)\n \t\tdie(_(\"cannot remove a locked working tree\"));\n \t}\n \tif (validate_worktree(wt, &errmsg, WT_VALIDATE_WORKTREE_MISSING_OK))\n-\t\tdie(_(\"validation failed, cannot remove working tree:\\n%s\"),\n+\t\tdie(_(\"validation failed, cannot remove working tree: %s\"),\n \t\t errmsg.buf);\n \tstrbuf_release(&errmsg);\n \ndiff --git a/strbuf.c b/strbuf.c\nindex 1df674e919..46930ad027 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -95,6 +95,7 @@ void strbuf_trim(struct strbuf *sb)\n \tstrbuf_rtrim(sb);\n \tstrbuf_ltrim(sb);\n }\n+\n void strbuf_rtrim(struct strbuf *sb)\n {\n \twhile (sb->len > 0 && isspace((unsigned char)sb->buf[sb->len - 1]))\n@@ -102,6 +103,13 @@ void strbuf_rtrim(struct strbuf *sb)\n \tsb->buf[sb->len] = '\\0';\n }\n \n+void strbuf_trim_trailing_dir_sep(struct strbuf *sb)\n+{\n+\twhile (sb->len > 0 && is_dir_sep((unsigned char)sb->buf[sb->len - 1]))\n+\t\tsb->len--;\n+\tsb->buf[sb->len] = '\\0';\n+}\n+\n void strbuf_ltrim(struct strbuf *sb)\n {\n \tchar *b = sb->buf;\ndiff --git a/strbuf.h b/strbuf.h\nindex 14c8c10d66..e6cae5f439 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -179,6 +179,9 @@ extern void strbuf_trim(struct strbuf *);\n extern void strbuf_rtrim(struct strbuf *);\n extern void strbuf_ltrim(struct strbuf *);\n \n+/* Strip trailing directory separators */\n+extern void strbuf_trim_trailing_dir_sep(struct strbuf *);\n+\n /**\n * Replace the contents of the strbuf with a reencoded form. Returns -1\n * on error, 0 on success.\ndiff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh\nindex 459f676683..082368d8c6 100755\n--- a/t/t2028-worktree-move.sh\n+++ b/t/t2028-worktree-move.sh\n@@ -66,21 +66,16 @@ test_expect_success 'move non-worktree' '\n \n test_expect_success 'move locked worktree' '\n \tgit worktree lock source &&\n-\ttest_must_fail git worktree move source destination &&\n-\tgit worktree unlock source\n+\ttest_when_finished \"git worktree unlock source\" &&\n+\ttest_must_fail git worktree move source destination\n '\n \n test_expect_success 'move worktree' '\n \ttoplevel=\"$(pwd)\" &&\n \tgit worktree move source destination &&\n \ttest_path_is_missing source &&\n-\tgit worktree list --porcelain | grep \"^worktree\" >actual &&\n-\tcat <<-EOF >expected &&\n-\tworktree $toplevel\n-\tworktree $toplevel/destination\n-\tworktree $toplevel/elsewhere\n-\tEOF\n-\ttest_cmp expected actual &&\n+\tgit worktree list --porcelain | grep \"^worktree.*/destination\" &&\n+\t! git worktree list --porcelain | grep \"^worktree.*/source\" >empty &&\n \tgit -C destination log --format=%s >actual2 &&\n \techo init >expected2 &&\n \ttest_cmp expected2 actual2\n@@ -90,23 +85,38 @@ test_expect_success 'move main worktree' '\n \ttest_must_fail git worktree move . def\n '\n \n+test_expect_success 'move worktree to another dir' '\n+\ttoplevel=\"$(pwd)\" &&\n+\tmkdir some-dir &&\n+\tgit worktree move destination some-dir &&\n+\ttest_path_is_missing source &&\n+\tgit worktree list --porcelain | grep \"^worktree.*/some-dir/destination\" &&\n+\tgit -C some-dir/destination log --format=%s >actual2 &&\n+\techo init >expected2 &&\n+\ttest_cmp expected2 actual2\n+'\n+\n test_expect_success 'remove main worktree' '\n \ttest_must_fail git worktree remove .\n '\n \n+test_expect_success 'move some-dir/destination back' '\n+\tgit worktree move some-dir/destination destination\n+'\n+\n test_expect_success 'remove locked worktree' '\n \tgit worktree lock destination &&\n-\ttest_must_fail git worktree remove destination &&\n-\tgit worktree unlock destination\n+\ttest_when_finished \"git worktree unlock destination\" &&\n+\ttest_must_fail git worktree remove destination\n '\n \n test_expect_success 'remove worktree with dirty tracked file' '\n \techo dirty >>destination/init.t &&\n+\ttest_when_finished \"git -C destination checkout init.t\" &&\n \ttest_must_fail git worktree remove destination\n '\n \n test_expect_success 'remove worktree with untracked file' '\n-\tgit -C destination checkout init.t &&\n \t: >destination/untracked &&\n \ttest_must_fail git worktree remove destination\n '\n@@ -124,4 +134,13 @@ test_expect_success 'remove missing worktree' '\n \ttest_path_is_missing .git/worktrees/to-be-gone\n '\n \n+test_expect_success 'NOT remove missing-but-locked worktree' '\n+\tgit worktree add gone-but-locked &&\n+\tgit worktree lock gone-but-locked &&\n+\ttest -d .git/worktrees/gone-but-locked &&\n+\tmv gone-but-locked really-gone-now &&\n+\ttest_must_fail git worktree remove gone-but-locked &&\n+\ttest_path_is_dir .git/worktrees/gone-but-locked\n+'\n+\n test_done\ndiff --git a/worktree.c b/worktree.c\nindex 542196f0ad..28989cf06e 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -340,11 +340,10 @@ void update_worktree_location(struct worktree *wt, const char *path_)\n \tif (is_main_worktree(wt))\n \t\tdie(\"BUG: can't relocate main worktree\");\n \n-\tstrbuf_add_absolute_path(&path, path_);\n+\tstrbuf_realpath(&path, path_, 1);\n \tif (fspathcmp(wt->path, path.buf)) {\n-\t\twrite_file(git_common_path(\"worktrees/%s/gitdir\",\n-\t\t\t\t\t wt->id),\n-\t\t\t \"%s/.git\", real_path(path.buf));\n+\t\twrite_file(git_common_path(\"worktrees/%s/gitdir\", wt->id),\n+\t\t\t \"%s/.git\", path.buf);\n \t\tfree(wt->path);\n \t\twt->path = strbuf_detach(&path, NULL);\n \t}\n\n"},{"id":"339065","messageId":"20180212094940.23834-2-pclouds@gmail.com","threadId":"47678","inReplyTo":"20180212094940.23834-1-pclouds@gmail.com","subject":"[PATCH v2 1/7] worktree.c: add validate_worktree()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-02-12T09:49:34Z","receivedAt":"2018-02-12T09:50:17Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This function is later used by \"worktree move\" and \"worktree remove\"\nto ensure that we have a good connection between the repository and\nthe worktree. For example, if a worktree is moved manually, the\nworktree location recorded in $GIT_DIR/worktrees/.../gitdir is\nincorrect and we should not move that one.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n worktree.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++++\n worktree.h | 9 +++++++\n 2 files changed, 81 insertions(+)\n\ndiff --git a/worktree.c b/worktree.c\nindex f5da7d286d..b238d87bf1 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -254,6 +254,78 @@ const char *is_worktree_locked(struct worktree *wt)\n \treturn wt->lock_reason;\n }\n \n+/* convenient wrapper to deal with NULL strbuf */\n+static void strbuf_addf_gently(struct strbuf *buf, const char *fmt, ...)\n+{\n+\tva_list params;\n+\n+\tif (!buf)\n+\t\treturn;\n+\n+\tva_start(params, fmt);\n+\tstrbuf_vaddf(buf, fmt, params);\n+\tva_end(params);\n+}\n+\n+int validate_worktree(const struct worktree *wt, struct strbuf *errmsg)\n+{\n+\tstruct strbuf wt_path = STRBUF_INIT;\n+\tchar *path = NULL;\n+\tint err, ret = -1;\n+\n+\tstrbuf_addf(&wt_path, \"%s/.git\", wt->path);\n+\n+\tif (is_main_worktree(wt)) {\n+\t\tif (is_directory(wt_path.buf)) {\n+\t\t\tret = 0;\n+\t\t\tgoto done;\n+\t\t}\n+\t\t/*\n+\t\t * Main worktree using .git file to point to the\n+\t\t * repository would make it impossible to know where\n+\t\t * the actual worktree is if this function is executed\n+\t\t * from another worktree. No .git file support for now.\n+\t\t */\n+\t\tstrbuf_addf_gently(errmsg,\n+\t\t\t\t _(\"'%s' at main working tree is not the repository directory\"),\n+\t\t\t\t wt_path.buf);\n+\t\tgoto done;\n+\t}\n+\n+\t/*\n+\t * Make sure \"gitdir\" file points to a real .git file and that\n+\t * file points back here.\n+\t */\n+\tif (!is_absolute_path(wt->path)) {\n+\t\tstrbuf_addf_gently(errmsg,\n+\t\t\t\t _(\"'%s' file does not contain absolute path to the working tree location\"),\n+\t\t\t\t git_common_path(\"worktrees/%s/gitdir\", wt->id));\n+\t\tgoto done;\n+\t}\n+\n+\tif (!file_exists(wt_path.buf)) {\n+\t\tstrbuf_addf_gently(errmsg, _(\"'%s' does not exist\"), wt_path.buf);\n+\t\tgoto done;\n+\t}\n+\n+\tpath = xstrdup_or_null(read_gitfile_gently(wt_path.buf, &err));\n+\tif (!path) {\n+\t\tstrbuf_addf_gently(errmsg, _(\"'%s' is not a .git file, error code %d\"),\n+\t\t\t\t wt_path.buf, err);\n+\t\tgoto done;\n+\t}\n+\n+\tret = fspathcmp(path, real_path(git_common_path(\"worktrees/%s\", wt->id)));\n+\n+\tif (ret)\n+\t\tstrbuf_addf_gently(errmsg, _(\"'%s' does not point back to '%s'\"),\n+\t\t\t\t wt->path, git_common_path(\"worktrees/%s\", wt->id));\n+done:\n+\tfree(path);\n+\tstrbuf_release(&wt_path);\n+\treturn ret;\n+}\n+\n int is_worktree_being_rebased(const struct worktree *wt,\n \t\t\t const char *target)\n {\ndiff --git a/worktree.h b/worktree.h\nindex c28a880e18..cb577de8cd 100644\n--- a/worktree.h\n+++ b/worktree.h\n@@ -3,6 +3,8 @@\n \n #include \"refs.h\"\n \n+struct strbuf;\n+\n struct worktree {\n \tchar *path;\n \tchar *id;\n@@ -59,6 +61,13 @@ extern int is_main_worktree(const struct worktree *wt);\n */\n extern const char *is_worktree_locked(struct worktree *wt);\n \n+/*\n+ * Return zero if the worktree is in good condition. Error message is\n+ * returned if \"errmsg\" is not NULL.\n+ */\n+extern int validate_worktree(const struct worktree *wt,\n+\t\t\t struct strbuf *errmsg);\n+\n /*\n * Free up the memory for worktree(s)\n */\n-- \n2.16.1.399.g632f88eed1\n\n"},{"id":"339066","messageId":"20180212094940.23834-3-pclouds@gmail.com","threadId":"47678","inReplyTo":"20180212094940.23834-1-pclouds@gmail.com","subject":"[PATCH v2 2/7] worktree.c: add update_worktree_location()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-02-12T09:49:35Z","receivedAt":"2018-02-12T09:50:21Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Signed-off-by: Nguyễn Thái Ngọc Duy \n---\n worktree.c | 17 +++++++++++++++++\n worktree.h | 6 ++++++\n 2 files changed, 23 insertions(+)\n\ndiff --git a/worktree.c b/worktree.c\nindex b238d87bf1..0373faf0dc 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -326,6 +326,23 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg)\n \treturn ret;\n }\n \n+void update_worktree_location(struct worktree *wt, const char *path_)\n+{\n+\tstruct strbuf path = STRBUF_INIT;\n+\n+\tif (is_main_worktree(wt))\n+\t\tdie(\"BUG: can't relocate main worktree\");\n+\n+\tstrbuf_realpath(&path, path_, 1);\n+\tif (fspathcmp(wt->path, path.buf)) {\n+\t\twrite_file(git_common_path(\"worktrees/%s/gitdir\", wt->id),\n+\t\t\t \"%s/.git\", path.buf);\n+\t\tfree(wt->path);\n+\t\twt->path = strbuf_detach(&path, NULL);\n+\t}\n+\tstrbuf_release(&path);\n+}\n+\n int is_worktree_being_rebased(const struct worktree *wt,\n \t\t\t const char *target)\n {\ndiff --git a/worktree.h b/worktree.h\nindex cb577de8cd..a913428c3d 100644\n--- a/worktree.h\n+++ b/worktree.h\n@@ -68,6 +68,12 @@ extern const char *is_worktree_locked(struct worktree *wt);\n extern int validate_worktree(const struct worktree *wt,\n \t\t\t struct strbuf *errmsg);\n \n+/*\n+ * Update worktrees/xxx/gitdir with the new path.\n+ */\n+extern void update_worktree_location(struct worktree *wt,\n+\t\t\t\t const char *path_);\n+\n /*\n * Free up the memory for worktree(s)\n */\n-- \n2.16.1.399.g632f88eed1\n\n"},{"id":"339067","messageId":"20180212094940.23834-5-pclouds@gmail.com","threadId":"47678","inReplyTo":"20180212094940.23834-1-pclouds@gmail.com","subject":"[PATCH v2 4/7] worktree move: accept destination as directory","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-02-12T09:49:37Z","receivedAt":"2018-02-12T09:50:36Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Similar to \"mv a b/\", which is actually \"mv a b/a\", we extract basename\nof source worktree and create a directory of the same name at\ndestination if dst path is a directory.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/worktree.c | 11 ++++++++++-\n strbuf.c | 8 ++++++++\n strbuf.h | 3 +++\n t/t2028-worktree-move.sh | 11 +++++++++++\n 4 files changed, 32 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 8b9482d1e5..6fe41313c9 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -631,8 +631,17 @@ static int move_worktree(int ac, const char **av, const char *prefix)\n \t\tdie(_(\"'%s' is not a working tree\"), av[0]);\n \tif (is_main_worktree(wt))\n \t\tdie(_(\"'%s' is a main working tree\"), av[0]);\n+\tif (is_directory(dst.buf)) {\n+\t\tconst char *sep = find_last_dir_sep(wt->path);\n+\n+\t\tif (!sep)\n+\t\t\tdie(_(\"could not figure out destination name from '%s'\"),\n+\t\t\t wt->path);\n+\t\tstrbuf_trim_trailing_dir_sep(&dst);\n+\t\tstrbuf_addstr(&dst, sep);\n+\t}\n \tif (file_exists(dst.buf))\n-\t\tdie(_(\"target '%s' already exists\"), av[1]);\n+\t\tdie(_(\"target '%s' already exists\"), dst.buf);\n \n \treason = is_worktree_locked(wt);\n \tif (reason) {\ndiff --git a/strbuf.c b/strbuf.c\nindex 1df674e919..46930ad027 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -95,6 +95,7 @@ void strbuf_trim(struct strbuf *sb)\n \tstrbuf_rtrim(sb);\n \tstrbuf_ltrim(sb);\n }\n+\n void strbuf_rtrim(struct strbuf *sb)\n {\n \twhile (sb->len > 0 && isspace((unsigned char)sb->buf[sb->len - 1]))\n@@ -102,6 +103,13 @@ void strbuf_rtrim(struct strbuf *sb)\n \tsb->buf[sb->len] = '\\0';\n }\n \n+void strbuf_trim_trailing_dir_sep(struct strbuf *sb)\n+{\n+\twhile (sb->len > 0 && is_dir_sep((unsigned char)sb->buf[sb->len - 1]))\n+\t\tsb->len--;\n+\tsb->buf[sb->len] = '\\0';\n+}\n+\n void strbuf_ltrim(struct strbuf *sb)\n {\n \tchar *b = sb->buf;\ndiff --git a/strbuf.h b/strbuf.h\nindex 14c8c10d66..e6cae5f439 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -179,6 +179,9 @@ extern void strbuf_trim(struct strbuf *);\n extern void strbuf_rtrim(struct strbuf *);\n extern void strbuf_ltrim(struct strbuf *);\n \n+/* Strip trailing directory separators */\n+extern void strbuf_trim_trailing_dir_sep(struct strbuf *);\n+\n /**\n * Replace the contents of the strbuf with a reencoded form. Returns -1\n * on error, 0 on success.\ndiff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh\nindex 0f8abc0854..deb486cd8e 100755\n--- a/t/t2028-worktree-move.sh\n+++ b/t/t2028-worktree-move.sh\n@@ -85,4 +85,15 @@ test_expect_success 'move main worktree' '\n \ttest_must_fail git worktree move . def\n '\n \n+test_expect_success 'move worktree to another dir' '\n+\ttoplevel=\"$(pwd)\" &&\n+\tmkdir some-dir &&\n+\tgit worktree move destination some-dir &&\n+\ttest_path_is_missing source &&\n+\tgit worktree list --porcelain | grep \"^worktree.*/some-dir/destination\" &&\n+\tgit -C some-dir/destination log --format=%s >actual2 &&\n+\techo init >expected2 &&\n+\ttest_cmp expected2 actual2\n+'\n+\n test_done\n-- \n2.16.1.399.g632f88eed1\n\n"},{"id":"339068","messageId":"20180212094940.23834-6-pclouds@gmail.com","threadId":"47678","inReplyTo":"20180212094940.23834-1-pclouds@gmail.com","subject":"[PATCH v2 5/7] worktree move: refuse to move worktrees with submodules","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-02-12T09:49:38Z","receivedAt":"2018-02-12T09:50:42Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Submodules contains .git files with relative paths. After a worktree\nmove, these files need to be updated or they may point to nowhere.\n\nThis is a bandage patch to make sure \"worktree move\" don't break\npeople's worktrees by accident. When .git file update code is in\nplace, this validate_no_submodules() could be removed.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n Documentation/git-worktree.txt | 2 +-\n builtin/worktree.c | 23 +++++++++++++++++++++++\n 2 files changed, 24 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt\nindex 4fa1dd3a48..e6764ee8e0 100644\n--- a/Documentation/git-worktree.txt\n+++ b/Documentation/git-worktree.txt\n@@ -79,7 +79,7 @@ with `--reason`.\n move::\n \n Move a working tree to a new location. Note that the main working tree\n-cannot be moved.\n+or linked working trees containing submodules cannot be moved.\n \n prune::\n \ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 6fe41313c9..4789cebe11 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -606,6 +606,27 @@ static int unlock_worktree(int ac, const char **av, const char *prefix)\n \treturn ret;\n }\n \n+static void validate_no_submodules(const struct worktree *wt)\n+{\n+\tstruct index_state istate = { NULL };\n+\tint i, found_submodules = 0;\n+\n+\tif (read_index_from(&istate, worktree_git_path(wt, \"index\")) > 0) {\n+\t\tfor (i = 0; i < istate.cache_nr; i++) {\n+\t\t\tstruct cache_entry *ce = istate.cache[i];\n+\n+\t\t\tif (S_ISGITLINK(ce->ce_mode)) {\n+\t\t\t\tfound_submodules = 1;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\t}\n+\tdiscard_index(&istate);\n+\n+\tif (found_submodules)\n+\t\tdie(_(\"working trees containing submodules cannot be moved\"));\n+}\n+\n static int move_worktree(int ac, const char **av, const char *prefix)\n {\n \tstruct option options[] = {\n@@ -643,6 +664,8 @@ static int move_worktree(int ac, const char **av, const char *prefix)\n \tif (file_exists(dst.buf))\n \t\tdie(_(\"target '%s' already exists\"), dst.buf);\n \n+\tvalidate_no_submodules(wt);\n+\n \treason = is_worktree_locked(wt);\n \tif (reason) {\n \t\tif (*reason)\n-- \n2.16.1.399.g632f88eed1\n\n"},{"id":"339069","messageId":"20180212094940.23834-7-pclouds@gmail.com","threadId":"47678","inReplyTo":"20180212094940.23834-1-pclouds@gmail.com","subject":"[PATCH v2 6/7] worktree remove: new command","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-02-12T09:49:39Z","receivedAt":"2018-02-12T09:50:45Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This command allows to delete a worktree. Like 'move' you cannot\nremove the main worktree, or one with submodules inside [1].\n\nFor deleting $GIT_WORK_TREE, Untracked files or any staged entries are\nconsidered precious and therefore prevent removal by default. Ignored\nfiles are not precious.\n\nWhen it comes to deleting $GIT_DIR, there's no \"clean\" check because\nthere should not be any valuable data in there, except:\n\n- HEAD reflog. There is nothing we can do about this until somebody\n steps up and implements the ref graveyard.\n\n- Detached HEAD. Technically it can still be recovered. Although it\n may be nice to warn about orphan commits like 'git checkout' does.\n\n[1] We do 'git status' with --ignore-submodules=all for safety\n anyway. But this needs a closer look by submodule people before we\n can allow deletion. For example, if a submodule is totally clean,\n but its repo not absorbed to the main .git dir, then deleting\n worktree also deletes the valuable .submodule repo too.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n Documentation/git-worktree.txt | 21 ++--\n builtin/worktree.c | 134 ++++++++++++++++++++++++-\n contrib/completion/git-completion.bash | 5 +-\n t/t2028-worktree-move.sh | 30 ++++++\n 4 files changed, 179 insertions(+), 11 deletions(-)\n\ndiff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt\nindex e6764ee8e0..d322acbc67 100644\n--- a/Documentation/git-worktree.txt\n+++ b/Documentation/git-worktree.txt\n@@ -14,6 +14,7 @@ SYNOPSIS\n 'git worktree lock' [--reason ] \n 'git worktree move' \n 'git worktree prune' [-n] [-v] [--expire ]\n+'git worktree remove' [--force] \n 'git worktree unlock' \n \n DESCRIPTION\n@@ -85,6 +86,13 @@ prune::\n \n Prune working tree information in $GIT_DIR/worktrees.\n \n+remove::\n+\n+Remove a working tree. Only clean working trees (no untracked files\n+and no modification in tracked files) can be removed. Unclean working\n+trees or ones with submodules can be removed with `--force`. The main\n+working tree cannot be removed.\n+\n unlock::\n \n Unlock a working tree, allowing it to be pruned, moved or deleted.\n@@ -94,9 +102,10 @@ OPTIONS\n \n -f::\n --force::\n-\tBy default, `add` refuses to create a new working tree when `` is a branch name and\n-\tis already checked out by another working tree. This option overrides\n-\tthat safeguard.\n+\tBy default, `add` refuses to create a new working tree when\n+\t`` is a branch name and is already checked out by\n+\tanother working tree and `remove` refuses to remove an unclean\n+\tworking tree. This option overrides that safeguard.\n \n -b ::\n -B ::\n@@ -278,12 +287,6 @@ Multiple checkout in general is still experimental, and the support\n for submodules is incomplete. It is NOT recommended to make multiple\n checkouts of a superproject.\n \n-git-worktree could provide more automation for tasks currently\n-performed manually, such as:\n-\n-- `remove` to remove a linked working tree and its administrative files (and\n- warn if the working tree is dirty)\n-\n GIT\n ---\n Part of the linkgit:git[1] suite\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 4789cebe11..990e47b315 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -19,6 +19,7 @@ static const char * const worktree_usage[] = {\n \tN_(\"git worktree lock [] \"),\n \tN_(\"git worktree move \"),\n \tN_(\"git worktree prune []\"),\n+\tN_(\"git worktree remove [] \"),\n \tN_(\"git worktree unlock \"),\n \tNULL\n };\n@@ -624,7 +625,7 @@ static void validate_no_submodules(const struct worktree *wt)\n \tdiscard_index(&istate);\n \n \tif (found_submodules)\n-\t\tdie(_(\"working trees containing submodules cannot be moved\"));\n+\t\tdie(_(\"working trees containing submodules cannot be moved or removed\"));\n }\n \n static int move_worktree(int ac, const char **av, const char *prefix)\n@@ -688,6 +689,135 @@ static int move_worktree(int ac, const char **av, const char *prefix)\n \treturn 0;\n }\n \n+/*\n+ * Note, \"git status --porcelain\" is used to determine if it's safe to\n+ * delete a whole worktree. \"git status\" does not ignore user\n+ * configuration, so if a normal \"git status\" shows \"clean\" for the\n+ * user, then it's ok to remove it.\n+ *\n+ * This assumption may be a bad one. We may want to ignore\n+ * (potentially bad) user settings and only delete a worktree when\n+ * it's absolutely safe to do so from _our_ point of view because we\n+ * know better.\n+ */\n+static void check_clean_worktree(struct worktree *wt,\n+\t\t\t\t const char *original_path)\n+{\n+\tstruct argv_array child_env = ARGV_ARRAY_INIT;\n+\tstruct child_process cp;\n+\tchar buf[1];\n+\tint ret;\n+\n+\t/*\n+\t * Until we sort this out, all submodules are \"dirty\" and\n+\t * will abort this function.\n+\t */\n+\tvalidate_no_submodules(wt);\n+\n+\targv_array_pushf(&child_env, \"%s=%s/.git\",\n+\t\t\t GIT_DIR_ENVIRONMENT, wt->path);\n+\targv_array_pushf(&child_env, \"%s=%s\",\n+\t\t\t GIT_WORK_TREE_ENVIRONMENT, wt->path);\n+\tmemset(&cp, 0, sizeof(cp));\n+\targv_array_pushl(&cp.args, \"status\",\n+\t\t\t \"--porcelain\", \"--ignore-submodules=none\",\n+\t\t\t NULL);\n+\tcp.env = child_env.argv;\n+\tcp.git_cmd = 1;\n+\tcp.dir = wt->path;\n+\tcp.out = -1;\n+\tret = start_command(&cp);\n+\tif (ret)\n+\t\tdie_errno(_(\"failed to run 'git status' on '%s'\"),\n+\t\t\t original_path);\n+\tret = xread(cp.out, buf, sizeof(buf));\n+\tif (ret)\n+\t\tdie(_(\"'%s' is dirty, use --force to delete it\"),\n+\t\t original_path);\n+\tclose(cp.out);\n+\tret = finish_command(&cp);\n+\tif (ret)\n+\t\tdie_errno(_(\"failed to run 'git status' on '%s', code %d\"),\n+\t\t\t original_path, ret);\n+}\n+\n+static int delete_git_work_tree(struct worktree *wt)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tint ret = 0;\n+\n+\tstrbuf_addstr(&sb, wt->path);\n+\tif (remove_dir_recursively(&sb, 0)) {\n+\t\terror_errno(_(\"failed to delete '%s'\"), sb.buf);\n+\t\tret = -1;\n+\t}\n+\tstrbuf_release(&sb);\n+\treturn ret;\n+}\n+\n+static int delete_git_dir(struct worktree *wt)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tint ret = 0;\n+\n+\tstrbuf_addstr(&sb, git_common_path(\"worktrees/%s\", wt->id));\n+\tif (remove_dir_recursively(&sb, 0)) {\n+\t\terror_errno(_(\"failed to delete '%s'\"), sb.buf);\n+\t\tret = -1;\n+\t}\n+\tstrbuf_release(&sb);\n+\treturn ret;\n+}\n+\n+static int remove_worktree(int ac, const char **av, const char *prefix)\n+{\n+\tint force = 0;\n+\tstruct option options[] = {\n+\t\tOPT_BOOL(0, \"force\", &force,\n+\t\t\t N_(\"force removing even if the worktree is dirty\")),\n+\t\tOPT_END()\n+\t};\n+\tstruct worktree **worktrees, *wt;\n+\tstruct strbuf errmsg = STRBUF_INIT;\n+\tconst char *reason;\n+\tint ret = 0;\n+\n+\tac = parse_options(ac, av, prefix, options, worktree_usage, 0);\n+\tif (ac != 1)\n+\t\tusage_with_options(worktree_usage, options);\n+\n+\tworktrees = get_worktrees(0);\n+\twt = find_worktree(worktrees, prefix, av[0]);\n+\tif (!wt)\n+\t\tdie(_(\"'%s' is not a working tree\"), av[0]);\n+\tif (is_main_worktree(wt))\n+\t\tdie(_(\"'%s' is a main working tree\"), av[0]);\n+\treason = is_worktree_locked(wt);\n+\tif (reason) {\n+\t\tif (*reason)\n+\t\t\tdie(_(\"cannot remove a locked working tree, lock reason: %s\"),\n+\t\t\t reason);\n+\t\tdie(_(\"cannot remove a locked working tree\"));\n+\t}\n+\tif (validate_worktree(wt, &errmsg))\n+\t\tdie(_(\"validation failed, cannot remove working tree: %s\"),\n+\t\t errmsg.buf);\n+\tstrbuf_release(&errmsg);\n+\n+\tif (!force)\n+\t\tcheck_clean_worktree(wt, av[0]);\n+\n+\tret |= delete_git_work_tree(wt);\n+\t/*\n+\t * continue on even if ret is non-zero, there's no going back\n+\t * from here.\n+\t */\n+\tret |= delete_git_dir(wt);\n+\n+\tfree_worktrees(worktrees);\n+\treturn ret;\n+}\n+\n int cmd_worktree(int ac, const char **av, const char *prefix)\n {\n \tstruct option options[] = {\n@@ -712,5 +842,7 @@ int cmd_worktree(int ac, const char **av, const char *prefix)\n \t\treturn unlock_worktree(ac - 1, av + 1, prefix);\n \tif (!strcmp(av[1], \"move\"))\n \t\treturn move_worktree(ac - 1, av + 1, prefix);\n+\tif (!strcmp(av[1], \"remove\"))\n+\t\treturn remove_worktree(ac - 1, av + 1, prefix);\n \tusage_with_options(worktree_usage, options);\n }\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex b87d387686..ff4a39631e 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -3087,7 +3087,7 @@ _git_whatchanged ()\n \n _git_worktree ()\n {\n-\tlocal subcommands=\"add list lock move prune unlock\"\n+\tlocal subcommands=\"add list lock move prune remove unlock\"\n \tlocal subcommand=\"$(__git_find_on_cmdline \"$subcommands\")\"\n \tif [ -z \"$subcommand\" ]; then\n \t\t__gitcomp \"$subcommands\"\n@@ -3105,6 +3105,9 @@ _git_worktree ()\n \t\tprune,--*)\n \t\t\t__gitcomp \"--dry-run --expire --verbose\"\n \t\t\t;;\n+\t\tremove,--*)\n+\t\t\t__gitcomp \"--force\"\n+\t\t\t;;\n \t\t*)\n \t\t\t;;\n \t\tesac\ndiff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh\nindex deb486cd8e..4718c4552f 100755\n--- a/t/t2028-worktree-move.sh\n+++ b/t/t2028-worktree-move.sh\n@@ -96,4 +96,34 @@ test_expect_success 'move worktree to another dir' '\n \ttest_cmp expected2 actual2\n '\n \n+test_expect_success 'remove main worktree' '\n+\ttest_must_fail git worktree remove .\n+'\n+\n+test_expect_success 'move some-dir/destination back' '\n+\tgit worktree move some-dir/destination destination\n+'\n+\n+test_expect_success 'remove locked worktree' '\n+\tgit worktree lock destination &&\n+\ttest_when_finished \"git worktree unlock destination\" &&\n+\ttest_must_fail git worktree remove destination\n+'\n+\n+test_expect_success 'remove worktree with dirty tracked file' '\n+\techo dirty >>destination/init.t &&\n+\ttest_when_finished \"git -C destination checkout init.t\" &&\n+\ttest_must_fail git worktree remove destination\n+'\n+\n+test_expect_success 'remove worktree with untracked file' '\n+\t: >destination/untracked &&\n+\ttest_must_fail git worktree remove destination\n+'\n+\n+test_expect_success 'force remove worktree with untracked file' '\n+\tgit worktree remove --force destination &&\n+\ttest_path_is_missing destination\n+'\n+\n test_done\n-- \n2.16.1.399.g632f88eed1\n\n"},{"id":"339070","messageId":"20180212094940.23834-8-pclouds@gmail.com","threadId":"47678","inReplyTo":"20180212094940.23834-1-pclouds@gmail.com","subject":"[PATCH v2 7/7] worktree remove: allow it when $GIT_WORK_TREE is already gone","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-02-12T09:49:40Z","receivedAt":"2018-02-12T09:50:50Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"\"git worktree remove\" basically consists of two things\n\n- delete $GIT_WORK_TREE\n- delete $GIT_DIR (which is $SUPER_GIT_DIR/worktrees/something)\n\nIf $GIT_WORK_TREE is already gone for some reason, we should be able\nto finish the job by deleting $GIT_DIR.\n\nTwo notes:\n\n- $GIT_WORK_TREE _can_ be missing if the worktree is locked. In that\n case we must not delete $GIT_DIR because the real $GIT_WORK_TREE may\n be in a usb stick somewhere. This is already handled because we\n check for lock first.\n\n- validate_worktree() is still called because it may do more checks in\n future (and it already does something else, like checking main\n worktree, but that's irrelevant in this case)\n\nNoticed-by: Kaartic Sivaraam \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/worktree.c | 12 +++++++-----\n t/t2028-worktree-move.sh | 17 +++++++++++++++++\n worktree.c | 9 ++++++++-\n worktree.h | 5 ++++-\n 4 files changed, 36 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 990e47b315..f77ef994c4 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -674,7 +674,7 @@ static int move_worktree(int ac, const char **av, const char *prefix)\n \t\t\t reason);\n \t\tdie(_(\"cannot move a locked working tree\"));\n \t}\n-\tif (validate_worktree(wt, &errmsg))\n+\tif (validate_worktree(wt, &errmsg, 0))\n \t\tdie(_(\"validation failed, cannot move working tree: %s\"),\n \t\t errmsg.buf);\n \tstrbuf_release(&errmsg);\n@@ -799,15 +799,17 @@ static int remove_worktree(int ac, const char **av, const char *prefix)\n \t\t\t reason);\n \t\tdie(_(\"cannot remove a locked working tree\"));\n \t}\n-\tif (validate_worktree(wt, &errmsg))\n+\tif (validate_worktree(wt, &errmsg, WT_VALIDATE_WORKTREE_MISSING_OK))\n \t\tdie(_(\"validation failed, cannot remove working tree: %s\"),\n \t\t errmsg.buf);\n \tstrbuf_release(&errmsg);\n \n-\tif (!force)\n-\t\tcheck_clean_worktree(wt, av[0]);\n+\tif (file_exists(wt->path)) {\n+\t\tif (!force)\n+\t\t\tcheck_clean_worktree(wt, av[0]);\n \n-\tret |= delete_git_work_tree(wt);\n+\t\tret |= delete_git_work_tree(wt);\n+\t}\n \t/*\n \t * continue on even if ret is non-zero, there's no going back\n \t * from here.\ndiff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh\nindex 4718c4552f..082368d8c6 100755\n--- a/t/t2028-worktree-move.sh\n+++ b/t/t2028-worktree-move.sh\n@@ -126,4 +126,21 @@ test_expect_success 'force remove worktree with untracked file' '\n \ttest_path_is_missing destination\n '\n \n+test_expect_success 'remove missing worktree' '\n+\tgit worktree add to-be-gone &&\n+\ttest -d .git/worktrees/to-be-gone &&\n+\tmv to-be-gone gone &&\n+\tgit worktree remove to-be-gone &&\n+\ttest_path_is_missing .git/worktrees/to-be-gone\n+'\n+\n+test_expect_success 'NOT remove missing-but-locked worktree' '\n+\tgit worktree add gone-but-locked &&\n+\tgit worktree lock gone-but-locked &&\n+\ttest -d .git/worktrees/gone-but-locked &&\n+\tmv gone-but-locked really-gone-now &&\n+\ttest_must_fail git worktree remove gone-but-locked &&\n+\ttest_path_is_dir .git/worktrees/gone-but-locked\n+'\n+\n test_done\ndiff --git a/worktree.c b/worktree.c\nindex 0373faf0dc..28989cf06e 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -267,7 +267,8 @@ static void strbuf_addf_gently(struct strbuf *buf, const char *fmt, ...)\n \tva_end(params);\n }\n \n-int validate_worktree(const struct worktree *wt, struct strbuf *errmsg)\n+int validate_worktree(const struct worktree *wt, struct strbuf *errmsg,\n+\t\t unsigned flags)\n {\n \tstruct strbuf wt_path = STRBUF_INIT;\n \tchar *path = NULL;\n@@ -303,6 +304,12 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg)\n \t\tgoto done;\n \t}\n \n+\tif (flags & WT_VALIDATE_WORKTREE_MISSING_OK &&\n+\t !file_exists(wt->path)) {\n+\t\tret = 0;\n+\t\tgoto done;\n+\t}\n+\n \tif (!file_exists(wt_path.buf)) {\n \t\tstrbuf_addf_gently(errmsg, _(\"'%s' does not exist\"), wt_path.buf);\n \t\tgoto done;\ndiff --git a/worktree.h b/worktree.h\nindex a913428c3d..fe38ce10c3 100644\n--- a/worktree.h\n+++ b/worktree.h\n@@ -61,12 +61,15 @@ extern int is_main_worktree(const struct worktree *wt);\n */\n extern const char *is_worktree_locked(struct worktree *wt);\n \n+#define WT_VALIDATE_WORKTREE_MISSING_OK (1 << 0)\n+\n /*\n * Return zero if the worktree is in good condition. Error message is\n * returned if \"errmsg\" is not NULL.\n */\n extern int validate_worktree(const struct worktree *wt,\n-\t\t\t struct strbuf *errmsg);\n+\t\t\t struct strbuf *errmsg,\n+\t\t\t unsigned flags);\n \n /*\n * Update worktrees/xxx/gitdir with the new path.\n-- \n2.16.1.399.g632f88eed1\n\n"},{"id":"339071","messageId":"20180212094940.23834-4-pclouds@gmail.com","threadId":"47678","inReplyTo":"20180212094940.23834-1-pclouds@gmail.com","subject":"[PATCH v2 3/7] worktree move: new command","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-02-12T09:49:36Z","receivedAt":"2018-02-12T09:50:55Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This command allows to relocate linked worktrees. Main worktree cannot\n(yet) be moved.\n\nThere are two options to move the main worktree, but both have\ncomplications, so it's not implemented yet. Anyway the options are:\n\n- convert the main worktree to a linked one and move it away, leave\n the git repository where it is. The repo essentially becomes bare\n after this move.\n\n- move the repository with the main worktree. The tricky part is make\n sure all file descriptors to the repository are closed, or it may\n fail on Windows.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n Documentation/git-worktree.txt | 13 ++++---\n builtin/worktree.c | 53 ++++++++++++++++++++++++++\n contrib/completion/git-completion.bash | 2 +-\n t/t2028-worktree-move.sh | 26 +++++++++++++\n 4 files changed, 87 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt\nindex 41585f535d..4fa1dd3a48 100644\n--- a/Documentation/git-worktree.txt\n+++ b/Documentation/git-worktree.txt\n@@ -12,6 +12,7 @@ SYNOPSIS\n 'git worktree add' [-f] [--detach] [--checkout] [--lock] [-b ] []\n 'git worktree list' [--porcelain]\n 'git worktree lock' [--reason ] \n+'git worktree move' \n 'git worktree prune' [-n] [-v] [--expire ]\n 'git worktree unlock' \n \n@@ -34,10 +35,6 @@ The working tree's administrative files in the repository (see\n `git worktree prune` in the main or any linked working tree to\n clean up any stale administrative files.\n \n-If you move a linked working tree, you need to manually update the\n-administrative files so that they do not get pruned automatically. See\n-section \"DETAILS\" for more information.\n-\n If a linked working tree is stored on a portable device or network share\n which is not always mounted, you can prevent its administrative files from\n being pruned by issuing the `git worktree lock` command, optionally\n@@ -79,6 +76,11 @@ files from being pruned automatically. This also prevents it from\n being moved or deleted. Optionally, specify a reason for the lock\n with `--reason`.\n \n+move::\n+\n+Move a working tree to a new location. Note that the main working tree\n+cannot be moved.\n+\n prune::\n \n Prune working tree information in $GIT_DIR/worktrees.\n@@ -196,7 +198,7 @@ thumb is do not make any assumption about whether a path belongs to\n $GIT_DIR or $GIT_COMMON_DIR when you need to directly access something\n inside $GIT_DIR. Use `git rev-parse --git-path` to get the final path.\n \n-If you move a linked working tree, you need to update the 'gitdir' file\n+If you manually move a linked working tree, you need to update the 'gitdir' file\n in the entry's directory. For example, if a linked working tree is moved\n to `/newpath/test-next` and its `.git` file points to\n `/path/main/.git/worktrees/test-next`, then update\n@@ -281,7 +283,6 @@ performed manually, such as:\n \n - `remove` to remove a linked working tree and its administrative files (and\n warn if the working tree is dirty)\n-- `mv` to move or rename a working tree and update its administrative files\n \n GIT\n ---\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 7cef5b120b..8b9482d1e5 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -17,6 +17,7 @@ static const char * const worktree_usage[] = {\n \tN_(\"git worktree add [] []\"),\n \tN_(\"git worktree list []\"),\n \tN_(\"git worktree lock [] \"),\n+\tN_(\"git worktree move \"),\n \tN_(\"git worktree prune []\"),\n \tN_(\"git worktree unlock \"),\n \tNULL\n@@ -605,6 +606,56 @@ static int unlock_worktree(int ac, const char **av, const char *prefix)\n \treturn ret;\n }\n \n+static int move_worktree(int ac, const char **av, const char *prefix)\n+{\n+\tstruct option options[] = {\n+\t\tOPT_END()\n+\t};\n+\tstruct worktree **worktrees, *wt;\n+\tstruct strbuf dst = STRBUF_INIT;\n+\tstruct strbuf errmsg = STRBUF_INIT;\n+\tconst char *reason;\n+\tchar *path;\n+\n+\tac = parse_options(ac, av, prefix, options, worktree_usage, 0);\n+\tif (ac != 2)\n+\t\tusage_with_options(worktree_usage, options);\n+\n+\tpath = prefix_filename(prefix, av[1]);\n+\tstrbuf_addstr(&dst, path);\n+\tfree(path);\n+\n+\tworktrees = get_worktrees(0);\n+\twt = find_worktree(worktrees, prefix, av[0]);\n+\tif (!wt)\n+\t\tdie(_(\"'%s' is not a working tree\"), av[0]);\n+\tif (is_main_worktree(wt))\n+\t\tdie(_(\"'%s' is a main working tree\"), av[0]);\n+\tif (file_exists(dst.buf))\n+\t\tdie(_(\"target '%s' already exists\"), av[1]);\n+\n+\treason = is_worktree_locked(wt);\n+\tif (reason) {\n+\t\tif (*reason)\n+\t\t\tdie(_(\"cannot move a locked working tree, lock reason: %s\"),\n+\t\t\t reason);\n+\t\tdie(_(\"cannot move a locked working tree\"));\n+\t}\n+\tif (validate_worktree(wt, &errmsg))\n+\t\tdie(_(\"validation failed, cannot move working tree: %s\"),\n+\t\t errmsg.buf);\n+\tstrbuf_release(&errmsg);\n+\n+\tif (rename(wt->path, dst.buf) == -1)\n+\t\tdie_errno(_(\"failed to move '%s' to '%s'\"), wt->path, dst.buf);\n+\n+\tupdate_worktree_location(wt, dst.buf);\n+\n+\tstrbuf_release(&dst);\n+\tfree_worktrees(worktrees);\n+\treturn 0;\n+}\n+\n int cmd_worktree(int ac, const char **av, const char *prefix)\n {\n \tstruct option options[] = {\n@@ -627,5 +678,7 @@ int cmd_worktree(int ac, const char **av, const char *prefix)\n \t\treturn lock_worktree(ac - 1, av + 1, prefix);\n \tif (!strcmp(av[1], \"unlock\"))\n \t\treturn unlock_worktree(ac - 1, av + 1, prefix);\n+\tif (!strcmp(av[1], \"move\"))\n+\t\treturn move_worktree(ac - 1, av + 1, prefix);\n \tusage_with_options(worktree_usage, options);\n }\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 3683c772c5..b87d387686 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -3087,7 +3087,7 @@ _git_whatchanged ()\n \n _git_worktree ()\n {\n-\tlocal subcommands=\"add list lock prune unlock\"\n+\tlocal subcommands=\"add list lock move prune unlock\"\n \tlocal subcommand=\"$(__git_find_on_cmdline \"$subcommands\")\"\n \tif [ -z \"$subcommand\" ]; then\n \t\t__gitcomp \"$subcommands\"\ndiff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh\nindex 8298aaf97f..0f8abc0854 100755\n--- a/t/t2028-worktree-move.sh\n+++ b/t/t2028-worktree-move.sh\n@@ -59,4 +59,30 @@ test_expect_success 'unlock worktree twice' '\n \ttest_path_is_missing .git/worktrees/source/locked\n '\n \n+test_expect_success 'move non-worktree' '\n+\tmkdir abc &&\n+\ttest_must_fail git worktree move abc def\n+'\n+\n+test_expect_success 'move locked worktree' '\n+\tgit worktree lock source &&\n+\ttest_when_finished \"git worktree unlock source\" &&\n+\ttest_must_fail git worktree move source destination\n+'\n+\n+test_expect_success 'move worktree' '\n+\ttoplevel=\"$(pwd)\" &&\n+\tgit worktree move source destination &&\n+\ttest_path_is_missing source &&\n+\tgit worktree list --porcelain | grep \"^worktree.*/destination\" &&\n+\t! git worktree list --porcelain | grep \"^worktree.*/source\" >empty &&\n+\tgit -C destination log --format=%s >actual2 &&\n+\techo init >expected2 &&\n+\ttest_cmp expected2 actual2\n+'\n+\n+test_expect_success 'move main worktree' '\n+\ttest_must_fail git worktree move . def\n+'\n+\n test_done\n-- \n2.16.1.399.g632f88eed1\n\n"},{"id":"339072","messageId":"CACsJy8BjFex==2UQV6-k4_rTnBijJOzBAOmtNmZopc87U19Jmg@mail.gmail.com","threadId":"47678","inReplyTo":"CAN0heSofAfuXEUKcfF00ni_a7nAKJYySF_UEnNTL8+nHiBzw-A@mail.gmail.com","subject":"Re: [PATCH 3/7] worktree move: new command","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-02-12T09:56:33Z","receivedAt":"2018-02-12T09:57:09Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Feb 7, 2018 at 3:05 AM, Martin Ågren wrote:\n> On 6 February 2018 at 03:13, Jeff King wrote:\n>> On Mon, Feb 05, 2018 at 08:28:10PM +0700, Duy Nguyen wrote:\n>>> I learned SANITIZE=leak today! It not only catches this but also \"dst\".\n>>>\n>>> Jeff is there any ongoing effort to make the test suite pass with\n>>> SANITIZE=leak? My t2038 passed, so I went ahead with the full test\n>>> suite and saw so many failures. I did see in your original mails that\n>>> you focused on t0000 and t0001 only..\n>>\n>> Yeah, I did those two scripts to try to prove to myself that the\n>> approach was good. But I haven't really pushed it any further.\n>>\n>> Martin Ågren (cc'd) did some follow-up work, but I think we still have a\n>> long way to go.\n>\n> Agreed. :-)\n\nShould we mark the test files that pass SANITIZE=leak and run as a sub\ntestsuite? This way we can start growing it until it covers everything\n(unlikely) and make sure people don't break what's already passed.\n\nOf course I don't expect everybody to run this new make target with\nSANITIZE=leak. Travis can do that for us and hopefully have some way\nto tell git@vger about new breakages.\n-- \nDuy\n"},{"id":"339123","messageId":"CAN0heSpxcm1a9KN3etWAD5Vc_gkabQmEppa_1P4soZSZOnP8pg@mail.gmail.com","threadId":"47678","inReplyTo":"CACsJy8BjFex==2UQV6-k4_rTnBijJOzBAOmtNmZopc87U19Jmg@mail.gmail.com","subject":"Re: [PATCH 3/7] worktree move: new command","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-02-12T22:15:06Z","receivedAt":"2018-02-12T22:15:13Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 12 February 2018 at 10:56, Duy Nguyen wrote:\n> On Wed, Feb 7, 2018 at 3:05 AM, Martin Ågren wrote:\n>> On 6 February 2018 at 03:13, Jeff King wrote:\n>>> On Mon, Feb 05, 2018 at 08:28:10PM +0700, Duy Nguyen wrote:\n>>>> I learned SANITIZE=leak today! It not only catches this but also \"dst\".\n>>>>\n>>>> Jeff is there any ongoing effort to make the test suite pass with\n>>>> SANITIZE=leak? My t2038 passed, so I went ahead with the full test\n>>>> suite and saw so many failures. I did see in your original mails that\n>>>> you focused on t0000 and t0001 only..\n>>>\n>>> Yeah, I did those two scripts to try to prove to myself that the\n>>> approach was good. But I haven't really pushed it any further.\n>>>\n>>> Martin Ågren (cc'd) did some follow-up work, but I think we still have a\n>>> long way to go.\n>>\n>> Agreed. :-)\n>\n> Should we mark the test files that pass SANITIZE=leak and run as a sub\n> testsuite? This way we can start growing it until it covers everything\n> (unlikely) and make sure people don't break what's already passed.\n>\n> Of course I don't expect everybody to run this new make target with\n> SANITIZE=leak. Travis can do that for us and hopefully have some way\n> to tell git@vger about new breakages.\n\nMakes sense to try to make sure that we don't regress leak-free tests. I\ndon't know what our Travis-budget looks like, but I would volunteer to\nrun something like this periodically using my own cycles.\n\nMy experience with the innards of our Makefiles and test-lib.sh is\nnon-existant, but from a very cursory look it seems like something as\nsimple as loading GIT_SKIP_TESTS from a blacklist-file might do the\ntrick. I could try to look into it in the next couple of days.\n\nMartin\n"},{"id":"339128","messageId":"20180213002758.GA9274@duynguyen.dek-tpc.internal","threadId":"47678","inReplyTo":"CAN0heSpxcm1a9KN3etWAD5Vc_gkabQmEppa_1P4soZSZOnP8pg@mail.gmail.com","subject":"Re: [PATCH 3/7] worktree move: new command","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-02-13T00:27:58Z","receivedAt":"2018-02-13T00:28:14Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Feb 12, 2018 at 11:15:06PM +0100, Martin Ågren wrote:\n> On 12 February 2018 at 10:56, Duy Nguyen wrote:\n> > On Wed, Feb 7, 2018 at 3:05 AM, Martin Ågren wrote:\n> >> On 6 February 2018 at 03:13, Jeff King wrote:\n> >>> On Mon, Feb 05, 2018 at 08:28:10PM +0700, Duy Nguyen wrote:\n> >>>> I learned SANITIZE=leak today! It not only catches this but also \"dst\".\n> >>>>\n> >>>> Jeff is there any ongoing effort to make the test suite pass with\n> >>>> SANITIZE=leak? My t2038 passed, so I went ahead with the full test\n> >>>> suite and saw so many failures. I did see in your original mails that\n> >>>> you focused on t0000 and t0001 only..\n> >>>\n> >>> Yeah, I did those two scripts to try to prove to myself that the\n> >>> approach was good. But I haven't really pushed it any further.\n> >>>\n> >>> Martin Ågren (cc'd) did some follow-up work, but I think we still have a\n> >>> long way to go.\n> >>\n> >> Agreed. :-)\n> >\n> > Should we mark the test files that pass SANITIZE=leak and run as a sub\n> > testsuite? This way we can start growing it until it covers everything\n> > (unlikely) and make sure people don't break what's already passed.\n> >\n> > Of course I don't expect everybody to run this new make target with\n> > SANITIZE=leak. Travis can do that for us and hopefully have some way\n> > to tell git@vger about new breakages.\n> \n> Makes sense to try to make sure that we don't regress leak-free tests. I\n> don't know what our Travis-budget looks like, but I would volunteer to\n> run something like this periodically using my own cycles.\n> \n> My experience with the innards of our Makefiles and test-lib.sh is\n> non-existant, but from a very cursory look it seems like something as\n> simple as loading GIT_SKIP_TESTS from a blacklist-file might do the\n> trick.\n\nYeah my first throught was something along that line too. But maybe\nit's nicer to mark a test file leak-free like this:\n\n-- 8< --\ndiff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh\nindex 459f676683..1446f075b7 100755\n--- a/t/t2028-worktree-move.sh\n+++ b/t/t2028-worktree-move.sh\n@@ -2,6 +2,8 @@\n \n test_description='test git worktree move, remove, lock and unlock'\n \n+test_leak_free=yes\n+\n . ./test-lib.sh\n \n test_expect_success 'setup' '\n-- 8< --\n\nAnd we can run these test files with a new option --leak-check (or\nsomething like that, we already support a similar option --valgrind).\n\nMost of the work will be in test-lib.sh. If we detect --leak-check and\ntest_leak_free is not set, we skip the whole file. In the far far far\nfuture when all files have test_leak_free=yes, we can flip the default\nand delete these lines.\n\nIt would be nice if test-lib.sh can determine if 'git' binary is built\nwith SANITIZE=yes, but I think we can live without it.\n\n> I could try to look into it in the next couple of days.\n\nHave fun :) Let me know if you give up though, I'll give it a shot.\n--\nDuy\n"},{"id":"339258","messageId":"20180214031614.GC25188@sigill.intra.peff.net","threadId":"47678","inReplyTo":"20180213002758.GA9274@duynguyen.dek-tpc.internal","subject":"Re: [PATCH 3/7] worktree move: new command","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-14T03:16:14Z","receivedAt":"2018-02-14T03:16:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 13, 2018 at 07:27:58AM +0700, Duy Nguyen wrote:\n\n> > Makes sense to try to make sure that we don't regress leak-free tests. I\n> > don't know what our Travis-budget looks like, but I would volunteer to\n> > run something like this periodically using my own cycles.\n> > \n> > My experience with the innards of our Makefiles and test-lib.sh is\n> > non-existant, but from a very cursory look it seems like something as\n> > simple as loading GIT_SKIP_TESTS from a blacklist-file might do the\n> > trick.\n> \n> Yeah my first throught was something along that line too. But maybe\n> it's nicer to mark a test file leak-free like this:\n> \n> -- 8< --\n> diff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh\n> index 459f676683..1446f075b7 100755\n> --- a/t/t2028-worktree-move.sh\n> +++ b/t/t2028-worktree-move.sh\n> @@ -2,6 +2,8 @@\n> \n> test_description='test git worktree move, remove, lock and unlock'\n> \n> +test_leak_free=yes\n> +\n> . ./test-lib.sh\n> \n> test_expect_success 'setup' '\n> -- 8< --\n\nHmm. That is not too bad, but somehow it feels funny to me to be\npolluting each test script with these annotations. And to be driving it\nfrom inside the test scripts.\n\nIt seems like:\n\n make SANITIZE=leak test GIT_SKIP_TESTS=\"$(cat known-leaky)\"\n\nwould be sufficient. And updating the list would just be:\n\n # assume we're using prove, which will keep running after failure,\n # and will record the results for us to parse (using \"--state=\").\n # Otherwise use \"make -k\" and grep in t/test-results.\n make SANITIZE=leak test\n (cd t && prove --dry --state=failed) |\n perl -lne '/^(t[0-9]{4})-.*.sh$/ and print $1' |\n sort >known-leaky\n\nThat would update both now-passing and now-failing tests. Presumably\nwe'd keep it checked in, so \"git diff\" would show you the changes.\n\n-Peff\n"},{"id":"339264","messageId":"CACsJy8B9dRshT2RUhnNLbnwTY0VdVdKP9A_YGxwiqqAyMN_OAg@mail.gmail.com","threadId":"47678","inReplyTo":"20180214031614.GC25188@sigill.intra.peff.net","subject":"Re: [PATCH 3/7] worktree move: new command","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-02-14T09:07:12Z","receivedAt":"2018-02-14T09:07:50Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Feb 14, 2018 at 10:16 AM, Jeff King wrote:\n> Hmm. That is not too bad, but somehow it feels funny to me to be\n> polluting each test script with these annotations. And to be driving it\n> from inside the test scripts.\n>\n> It seems like:\n>\n> make SANITIZE=leak test GIT_SKIP_TESTS=\"$(cat known-leaky)\"\n>\n> would be sufficient.\n\nAnd all new test files are considered leak-free by default? I like that!\n\n> And updating the list would just be:\n>\n> # assume we're using prove, which will keep running after failure,\n> # and will record the results for us to parse (using \"--state=\").\n> # Otherwise use \"make -k\" and grep in t/test-results.\n> make SANITIZE=leak test\n> (cd t && prove --dry --state=failed) |\n> perl -lne '/^(t[0-9]{4})-.*.sh$/ and print $1' |\n> sort >known-leaky\n>\n> That would update both now-passing and now-failing tests. Presumably\n> we'd keep it checked in, so \"git diff\" would show you the changes.\n>\n> -Peff\n\n\n\n-- \nDuy\n"},{"id":"339292","messageId":"xmqqo9kro4oq.fsf@gitster-ct.c.googlers.com","threadId":"47678","inReplyTo":"CACsJy8B9dRshT2RUhnNLbnwTY0VdVdKP9A_YGxwiqqAyMN_OAg@mail.gmail.com","subject":"Re: [PATCH 3/7] worktree move: new command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-14T17:35:01Z","receivedAt":"2018-02-14T17:35:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> On Wed, Feb 14, 2018 at 10:16 AM, Jeff King wrote:\n>> Hmm. That is not too bad, but somehow it feels funny to me to be\n>> polluting each test script with these annotations. And to be driving it\n>> from inside the test scripts.\n>>\n>> It seems like:\n>>\n>> make SANITIZE=leak test GIT_SKIP_TESTS=\"$(cat known-leaky)\"\n>>\n>> would be sufficient.\n>\n> And all new test files are considered leak-free by default? I like that!\n\nSounds good ;-)\n\n>\n>> And updating the list would just be:\n>>\n>> # assume we're using prove, which will keep running after failure,\n>> # and will record the results for us to parse (using \"--state=\").\n>> # Otherwise use \"make -k\" and grep in t/test-results.\n>> make SANITIZE=leak test\n>> (cd t && prove --dry --state=failed) |\n>> perl -lne '/^(t[0-9]{4})-.*.sh$/ and print $1' |\n>> sort >known-leaky\n>>\n>> That would update both now-passing and now-failing tests. Presumably\n>> we'd keep it checked in, so \"git diff\" would show you the changes.\n\nSounds good, too.\n"},{"id":"339397","messageId":"20180214215637.6462-1-martin.agren@gmail.com","threadId":"47678","inReplyTo":"xmqqo9kro4oq.fsf@gitster-ct.c.googlers.com","subject":"[PATCH] t/known-leaky: add list of known-leaky test scripts","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-02-14T21:56:37Z","receivedAt":"2018-02-14T21:56:48Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Here's what a list of known leaks might look like. It feels a bit\nawkward to post a known-incomplete list (I don't run all tests). Duy\noffered to pick up the ball if I gave up, maybe you could complete and\npost this as your own? :-? Even if I (or others) can't reproduce the\ncomplete list locally, regressions will be trivial to find, and newly\nleak-free tests fairly easy to notice.\n\nI'm not sure if the shamelessly stolen shell snippets warrant their own\nscripts, or how make targets overriding various variables would be\nreceived. At least they're in the commit message.\n\n-- >8 --\nWe have quite a lot of leaks in the code base. Using SANITIZE=leak, it\nis easy to find them, and every now and then we simply stumble upon one.\nStill, we can expect it to take some time to get to the point where\n`make SANITIZE=leak test` succeeds.\n\nUntil that happens, it would be nice if we could at least try not to\nregress in the sense that a test tXXXX which was at one point leak-free\nturns leaky. Such a regression would indicate that leak-free code\nturned leaky, that a new feature is leaky, or that we simply happen to\ntrigger an existing leak as part of a newly added/modified test.\n\nTo that end, introduce a list of known-leaky tests, i.e., a list of\ntXXXX-values. Now this will be able to find such regressions:\n\n make SANITIZE=leak test GIT_SKIP_TESTS=\"$(cat t/known-leaky)\"\n\nThe list was generated, and can be updated, as follows:\n\n # Assume we're using prove, which will keep running after failure,\n # and will record the results for us to parse (using \"--state=\").\n # Otherwise use \"make -k\" and grep in t/test-results.\n GIT_TEST_OPTS=-i make SANITIZE=leak test\n cd t\n prove --dry --state=failed |\n perl -lne '/^(t[0-9]{4})-.*\\.sh$/ and print $1' |\n sort >known-leaky\n\nThe list added in this commit might be incomplete, since I do not run\nall tests (I'm missing svn, cvs, p4, Windows-only and\nfilesystem-dependent tests, as well as \"writeable /\"). The majority of\nthese do not primarily test our C code, but all of them might trigger\nleaks, in which case they would belong in this list.\n\nSuggested-by: Nguyễn Thái Ngọc Duy \nHelped-by: Jeff King \nSigned-off-by: Martin Ågren \n---\n t/known-leaky | 539 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 539 insertions(+)\n create mode 100644 t/known-leaky\n\ndiff --git a/t/known-leaky b/t/known-leaky\nnew file mode 100644\nindex 0000000000..80a0af4d09\n--- /dev/null\n+++ b/t/known-leaky\n@@ -0,0 +1,539 @@\n+t0002\n+t0003\n+t0006\n+t0008\n+t0009\n+t0012\n+t0020\n+t0021\n+t0023\n+t0040\n+t0050\n+t0056\n+t0060\n+t0062\n+t0064\n+t0070\n+t0090\n+t0100\n+t0101\n+t0110\n+t0203\n+t0300\n+t0301\n+t0302\n+t1001\n+t1002\n+t1004\n+t1005\n+t1006\n+t1007\n+t1008\n+t1011\n+t1012\n+t1013\n+t1020\n+t1021\n+t1050\n+t1051\n+t1090\n+t1301\n+t1302\n+t1304\n+t1306\n+t1308\n+t1350\n+t1400\n+t1401\n+t1402\n+t1403\n+t1404\n+t1405\n+t1406\n+t1407\n+t1408\n+t1409\n+t1410\n+t1411\n+t1412\n+t1413\n+t1414\n+t1430\n+t1450\n+t1500\n+t1502\n+t1505\n+t1507\n+t1508\n+t1510\n+t1511\n+t1512\n+t1514\n+t1700\n+t2007\n+t2008\n+t2009\n+t2010\n+t2011\n+t2012\n+t2013\n+t2014\n+t2015\n+t2016\n+t2017\n+t2018\n+t2019\n+t2020\n+t2021\n+t2022\n+t2023\n+t2024\n+t2025\n+t2026\n+t2027\n+t2030\n+t2103\n+t2106\n+t2200\n+t2203\n+t2204\n+t3001\n+t3004\n+t3005\n+t3007\n+t3010\n+t3020\n+t3030\n+t3031\n+t3032\n+t3033\n+t3034\n+t3040\n+t3050\n+t3060\n+t3200\n+t3201\n+t3202\n+t3203\n+t3204\n+t3205\n+t3210\n+t3301\n+t3302\n+t3303\n+t3304\n+t3306\n+t3307\n+t3308\n+t3309\n+t3310\n+t3311\n+t3400\n+t3402\n+t3403\n+t3404\n+t3405\n+t3406\n+t3407\n+t3408\n+t3409\n+t3410\n+t3411\n+t3412\n+t3413\n+t3414\n+t3415\n+t3416\n+t3417\n+t3418\n+t3419\n+t3420\n+t3421\n+t3425\n+t3426\n+t3427\n+t3428\n+t3429\n+t3500\n+t3501\n+t3502\n+t3503\n+t3504\n+t3505\n+t3506\n+t3507\n+t3508\n+t3509\n+t3510\n+t3511\n+t3512\n+t3513\n+t3600\n+t3700\n+t3701\n+t3800\n+t3900\n+t3901\n+t3903\n+t3904\n+t3905\n+t3906\n+t4001\n+t4008\n+t4010\n+t4013\n+t4014\n+t4015\n+t4016\n+t4017\n+t4018\n+t4020\n+t4021\n+t4022\n+t4023\n+t4026\n+t4027\n+t4028\n+t4030\n+t4031\n+t4036\n+t4038\n+t4039\n+t4041\n+t4042\n+t4043\n+t4044\n+t4045\n+t4047\n+t4048\n+t4049\n+t4051\n+t4052\n+t4053\n+t4055\n+t4056\n+t4057\n+t4059\n+t4060\n+t4061\n+t4064\n+t4065\n+t4103\n+t4107\n+t4108\n+t4111\n+t4114\n+t4115\n+t4117\n+t4118\n+t4120\n+t4121\n+t4122\n+t4124\n+t4125\n+t4127\n+t4131\n+t4135\n+t4137\n+t4138\n+t4150\n+t4151\n+t4152\n+t4153\n+t4200\n+t4201\n+t4202\n+t4203\n+t4204\n+t4205\n+t4206\n+t4207\n+t4208\n+t4209\n+t4210\n+t4211\n+t4212\n+t4213\n+t4252\n+t4253\n+t4254\n+t4255\n+t4300\n+t5000\n+t5001\n+t5002\n+t5003\n+t5004\n+t5100\n+t5150\n+t5300\n+t5301\n+t5302\n+t5303\n+t5304\n+t5305\n+t5306\n+t5310\n+t5311\n+t5312\n+t5313\n+t5314\n+t5315\n+t5316\n+t5317\n+t5400\n+t5401\n+t5402\n+t5403\n+t5404\n+t5405\n+t5406\n+t5407\n+t5408\n+t5500\n+t5501\n+t5502\n+t5503\n+t5504\n+t5505\n+t5506\n+t5509\n+t5510\n+t5512\n+t5513\n+t5514\n+t5515\n+t5516\n+t5517\n+t5518\n+t5519\n+t5520\n+t5521\n+t5522\n+t5523\n+t5524\n+t5525\n+t5526\n+t5527\n+t5528\n+t5531\n+t5532\n+t5533\n+t5534\n+t5535\n+t5536\n+t5537\n+t5538\n+t5539\n+t5540\n+t5541\n+t5542\n+t5543\n+t5544\n+t5545\n+t5546\n+t5547\n+t5550\n+t5551\n+t5560\n+t5561\n+t5570\n+t5571\n+t5572\n+t5573\n+t5600\n+t5601\n+t5603\n+t5604\n+t5605\n+t5606\n+t5607\n+t5609\n+t5610\n+t5611\n+t5612\n+t5613\n+t5614\n+t5700\n+t5801\n+t5802\n+t5810\n+t5811\n+t5812\n+t5813\n+t5814\n+t5815\n+t5900\n+t6000\n+t6002\n+t6003\n+t6004\n+t6006\n+t6007\n+t6008\n+t6009\n+t6010\n+t6011\n+t6012\n+t6013\n+t6014\n+t6015\n+t6016\n+t6017\n+t6018\n+t6019\n+t6020\n+t6021\n+t6022\n+t6024\n+t6025\n+t6026\n+t6027\n+t6028\n+t6029\n+t6030\n+t6031\n+t6032\n+t6033\n+t6034\n+t6035\n+t6036\n+t6037\n+t6038\n+t6040\n+t6041\n+t6042\n+t6044\n+t6045\n+t6050\n+t6060\n+t6100\n+t6101\n+t6110\n+t6111\n+t6112\n+t6120\n+t6130\n+t6131\n+t6132\n+t6133\n+t6134\n+t6200\n+t6300\n+t6301\n+t6302\n+t6500\n+t6501\n+t7001\n+t7003\n+t7004\n+t7005\n+t7006\n+t7007\n+t7008\n+t7009\n+t7010\n+t7011\n+t7012\n+t7030\n+t7060\n+t7061\n+t7062\n+t7063\n+t7064\n+t7102\n+t7103\n+t7104\n+t7105\n+t7106\n+t7110\n+t7111\n+t7112\n+t7201\n+t7300\n+t7301\n+t7400\n+t7401\n+t7402\n+t7403\n+t7405\n+t7406\n+t7407\n+t7408\n+t7409\n+t7410\n+t7411\n+t7412\n+t7413\n+t7414\n+t7500\n+t7501\n+t7502\n+t7503\n+t7504\n+t7505\n+t7506\n+t7507\n+t7508\n+t7509\n+t7510\n+t7511\n+t7512\n+t7513\n+t7514\n+t7515\n+t7516\n+t7517\n+t7519\n+t7520\n+t7521\n+t7600\n+t7601\n+t7602\n+t7603\n+t7604\n+t7605\n+t7606\n+t7607\n+t7608\n+t7609\n+t7610\n+t7611\n+t7612\n+t7613\n+t7614\n+t7700\n+t7701\n+t7702\n+t7800\n+t7810\n+t7811\n+t7814\n+t8001\n+t8002\n+t8003\n+t8004\n+t8005\n+t8006\n+t8007\n+t8008\n+t8009\n+t8010\n+t8011\n+t9001\n+t9002\n+t9003\n+t9004\n+t9010\n+t9020\n+t9300\n+t9301\n+t9302\n+t9303\n+t9350\n+t9351\n+t9500\n+t9502\n+t9700\n+t9902\n+t9903\n-- \n2.16.1.72.g5be1f00a9a\n\n"},{"id":"339641","messageId":"20180219212937.GB9748@sigill.intra.peff.net","threadId":"47678","inReplyTo":"20180214215637.6462-1-martin.agren@gmail.com","subject":"Re: [PATCH] t/known-leaky: add list of known-leaky test scripts","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-19T21:29:37Z","receivedAt":"2018-02-19T21:29:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 14, 2018 at 10:56:37PM +0100, Martin Ågren wrote:\n\n> Here's what a list of known leaks might look like. It feels a bit\n> awkward to post a known-incomplete list (I don't run all tests). Duy\n> offered to pick up the ball if I gave up, maybe you could complete and\n> post this as your own? :-? Even if I (or others) can't reproduce the\n> complete list locally, regressions will be trivial to find, and newly\n> leak-free tests fairly easy to notice.\n\nI didn't think about that when I posted my scripts. In general, it's OK\nto me if you miss a script when you generate the \"leaky\" list. But if\nyou skip it, you cannot say whether it is leaky or not, and should\nprobably neither add nor remove it from the known-leaky list. So I think\nthe second shell snippet needs to become a little more clever about\nskipped test scripts.\n\nEven that isn't 100% fool-proof, as some individual tests may be skipped\nor not skipped on various platforms. But it may be enough in practice\n(and eventually we'd have no known-leaky tests, of course ;) ).\n\nOr alternatively, we could just not bother with checking this into the\nrepository, and it becomes a local thing for people interested in\nleak-testing. What's the value in having a shared known-leaky list,\nespecially if we don't expect most people to run it.\n\nI guess it lets us add a Travis job to do the leak-checking, which might\nget more coverage. So maybe if we do have an in-repo known-leaky, it\nshould match some canonical Travis environment. That won't give us\ncomplete coverage, but at this point we're just trying to notice\nlow-hanging fruit.\n\n-Peff\n"},{"id":"339740","messageId":"CAN0heSoDJdA3tTVZZ=imj8+z5FeqKy00dA2kWP4CAC4=+7VBiw@mail.gmail.com","threadId":"47678","inReplyTo":"20180219212937.GB9748@sigill.intra.peff.net","subject":"Re: [PATCH] t/known-leaky: add list of known-leaky test scripts","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-02-20T20:44:51Z","receivedAt":"2018-02-20T20:44:58Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 19 February 2018 at 22:29, Jeff King wrote:\n> On Wed, Feb 14, 2018 at 10:56:37PM +0100, Martin Ågren wrote:\n>\n>> Here's what a list of known leaks might look like. It feels a bit\n>> awkward to post a known-incomplete list (I don't run all tests). Duy\n>> offered to pick up the ball if I gave up, maybe you could complete and\n>> post this as your own? :-? Even if I (or others) can't reproduce the\n>> complete list locally, regressions will be trivial to find, and newly\n>> leak-free tests fairly easy to notice.\n>\n> I didn't think about that when I posted my scripts. In general, it's OK\n> to me if you miss a script when you generate the \"leaky\" list. But if\n> you skip it, you cannot say whether it is leaky or not, and should\n> probably neither add nor remove it from the known-leaky list. So I think\n> the second shell snippet needs to become a little more clever about\n> skipped test scripts.\n>\n> Even that isn't 100% fool-proof, as some individual tests may be skipped\n> or not skipped on various platforms. But it may be enough in practice\n> (and eventually we'd have no known-leaky tests, of course ;) ).\n\nAll good points.\n\n> Or alternatively, we could just not bother with checking this into the\n> repository, and it becomes a local thing for people interested in\n> leak-testing. What's the value in having a shared known-leaky list,\n> especially if we don't expect most people to run it.\n\nThis sums up my feeling about this.\n\n> I guess it lets us add a Travis job to do the leak-checking, which might\n> get more coverage. So maybe if we do have an in-repo known-leaky, it\n> should match some canonical Travis environment. That won't give us\n> complete coverage, but at this point we're just trying to notice\n> low-hanging fruit.\n\nRight. A Travis job to get non-complete but consistent data would be\n\"nice\" (TM), but at this point it would perhaps just result in\nblacklist-churning. At some point, adding/removing entries in a\nblacklist will have some signal-value and psychology to it, but right\nnow it might just be noise. (My patch adds an incomplete blacklist of\n539 scripts...)\n\nFor finding this sort of low-hanging fruit (windfall?) any local\nscripting and greping will do. I sort of hesitate wasting community CPU\ncycles on something which might be appreciated, in theory, but that\ndoesn't have enough attention to move beyond \"every now and then,\nsomething is fixed, but just as often, tests gain leaks and the\nblacklist gains entries\".\n\nTo sum up: I probably won't be looking into Travis-ing such a blacklist\nin the near future.\n\nMartin\n"},{"id":"339744","messageId":"20180220210822.GA15010@sigill.intra.peff.net","threadId":"47678","inReplyTo":"CAN0heSoDJdA3tTVZZ=imj8+z5FeqKy00dA2kWP4CAC4=+7VBiw@mail.gmail.com","subject":"Re: [PATCH] t/known-leaky: add list of known-leaky test scripts","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-20T21:08:22Z","receivedAt":"2018-02-20T21:08:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 20, 2018 at 09:44:51PM +0100, Martin Ågren wrote:\n\n> For finding this sort of low-hanging fruit (windfall?) any local\n> scripting and greping will do. I sort of hesitate wasting community CPU\n> cycles on something which might be appreciated, in theory, but that\n> doesn't have enough attention to move beyond \"every now and then,\n> something is fixed, but just as often, tests gain leaks and the\n> blacklist gains entries\".\n> \n> To sum up: I probably won't be looking into Travis-ing such a blacklist\n> in the near future.\n\nYeah, I think that's the right path for now. Hopefully we can get close\nto zero leaks at some point, and then think about adding this kind of\ninfrastructure. Thanks for talking it through.\n\n-Peff\n"},{"id":"339809","messageId":"xmqqa7w21dz7.fsf@gitster-ct.c.googlers.com","threadId":"47678","inReplyTo":"CAN0heSoDJdA3tTVZZ=imj8+z5FeqKy00dA2kWP4CAC4=+7VBiw@mail.gmail.com","subject":"Re: [PATCH] t/known-leaky: add list of known-leaky test scripts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-21T16:53:16Z","receivedAt":"2018-02-21T16:53:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren writes:\n\n> On 19 February 2018 at 22:29, Jeff King wrote:\n> ...\n>> Or alternatively, we could just not bother with checking this into the\n>> repository, and it becomes a local thing for people interested in\n>> leak-testing. What's the value in having a shared known-leaky list,\n>> especially if we don't expect most people to run it.\n>\n> This sums up my feeling about this.\n\nEven though keeping track of list of known-leaky tests may not be so\ninteresting, we can still salvage useful pieces from the discussion\nand make them available to developers, e.g. something like\n\n prove --dry --state=failed |\n perl -lne '/^(t[0-9]{4})-.*\\.sh$/ and print $1' | sort >$@+\n if cmp >/dev/null $@ $@+; then rm $@+; else mv $@+ $@; fi\n\ncould be made into a target to stash away the list of failing tests\nafter a test run?\n\n"},{"id":"339815","messageId":"20180221182550.GB4333@sigill.intra.peff.net","threadId":"47678","inReplyTo":"xmqqa7w21dz7.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] t/known-leaky: add list of known-leaky test scripts","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-21T18:25:50Z","receivedAt":"2018-02-21T18:25:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 21, 2018 at 08:53:16AM -0800, Junio C Hamano wrote:\n\n> Even though keeping track of list of known-leaky tests may not be so\n> interesting, we can still salvage useful pieces from the discussion\n> and make them available to developers, e.g. something like\n> \n> prove --dry --state=failed |\n> perl -lne '/^(t[0-9]{4})-.*\\.sh$/ and print $1' | sort >$@+\n> if cmp >/dev/null $@ $@+; then rm $@+; else mv $@+ $@; fi\n> \n> could be made into a target to stash away the list of failing tests\n> after a test run?\n\nUnfortunately there are some caveats in that snippet:\n\n 1. You are using prove.\n\n 2. You are using --state=save in the initial run.\n\nI think we might be better off having the test scripts write to\ntest-results/*.counts even when run under a TAP harness, and then we can\nhave a consistent way to get the list of failed tests (we already have a\n\"make failed\" that works this way).\n\n-Peff\n"},{"id":"340194","messageId":"a86793c8-2051-4d92-967b-1d24dbf4a2ff@gmail.com","threadId":"47678","inReplyTo":"CAN0heSoDJdA3tTVZZ=imj8+z5FeqKy00dA2kWP4CAC4=+7VBiw@mail.gmail.com","subject":"Re: [PATCH] t/known-leaky: add list of known-leaky test scripts","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2018-02-25T03:48:04Z","receivedAt":"2018-02-25T03:48:16Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"On Wednesday 21 February 2018 02:14 AM, Martin Ågren wrote:\n> To sum up: I probably won't be looking into Travis-ing such a blacklist\n> in the near future.\n> \n\nJust thinking out loud, how about having a white-list instead of a\nblack-list and using it to run only those tests in the white list.\nSomething like,\n\nt/white_list\n------------\nt0000\nt0001\n\nTo run\n------\n\ncd t/\nfor test in $(cat white_list)\ndo\n white_list_tests=\"$white_list_tests $test*\"\ndone\nmake SANITIZE=leak $white_list_tests\n\n\n\n-- \nKaartic\n\nQUOTE\n---\n\n“The most valuable person on any team is the person who makes everyone\nelse on the team more valuable, not the person who knows the most.”\n\n - Joel Spolsky\n\n"},{"id":"340329","messageId":"CAN0heSq0aWXkjEN7zjQs7yV8V0JSupNwy30yvqcM_V8M-rw-9A@mail.gmail.com","threadId":"47678","inReplyTo":"a86793c8-2051-4d92-967b-1d24dbf4a2ff@gmail.com","subject":"Re: [PATCH] t/known-leaky: add list of known-leaky test scripts","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-02-26T21:22:31Z","receivedAt":"2018-02-26T21:22:37Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 25 February 2018 at 04:48, Kaartic Sivaraam\n wrote:\n> On Wednesday 21 February 2018 02:14 AM, Martin Ågren wrote:\n>> To sum up: I probably won't be looking into Travis-ing such a blacklist\n>> in the near future.\n>>\n>\n> Just thinking out loud, how about having a white-list instead of a\n> black-list and using it to run only those tests in the white list.\n> Something like,\n>\n> t/white_list\n> ------------\n> t0000\n> t0001\n>\n> To run\n> ------\n>\n> cd t/\n> for test in $(cat white_list)\n> do\n> white_list_tests=\"$white_list_tests $test*\"\n> done\n> make SANITIZE=leak $white_list_tests\n\nYeah, that would also work. An incomplete whitelist can't cause errors\nfor those running other tests, as an incomplete blacklist could. So\nthat's good. At some point, the whitelist would need to be turned into a\nblacklist. At the very latest when the whitelist is the full set of\ntests, in order to flip the default of new tests. ;-) Right now, I think\nwhitelists and blacklists are about equally useful.\n\nLet's hope we're heading for a future where a blacklist gets more and\nmore feasible, whereas a whitelist would get longer and longer. ;-)\n\nMartin\n"},{"id":"340974","messageId":"20180304052647.26614-1-sunshine@sunshineco.com","threadId":"47678","inReplyTo":"20180212094940.23834-1-pclouds@gmail.com","subject":"[PATCH] t2028: fix minor error and issues in newly-added \"worktree move\" tests","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-03-04T05:26:47Z","receivedAt":"2018-03-04T05:33:30Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"Recently-added \"git worktree move\" tests include a minor error and a few\nsmall issues. Specifically:\n\n* checking non-existence of wrong file (\"source\" instead of\n \"destination\")\n\n* unneeded redirect (\">empty\")\n\n* unused variable (\"toplevel\")\n\n* restoring a worktree location by means of a separate test somewhat\n distant from the test which moved it rather than using\n test_when_finished() to restore it in a self-contained fashion\n\nSigned-off-by: Eric Sunshine \n---\n\nThis patch is built atop nd/worktree-move-reboot in 'next'.\n\nI didn't get around to doing a proper review of nd/worktree-move-reboot\nv2 [1] until after it had graduated to 'next'. Although v2 fixed all the\nissues identified in my review of v1 [2], it introduced a few minor\nissues of its own. This patch addresses those issues.\n\n[1]: https://public-inbox.org/git/20180212094940.23834-1-pclouds@gmail.com/\n[2]: https://public-inbox.org/git/20180124095357.19645-1-pclouds@gmail.com/\n\n t/t2028-worktree-move.sh | 10 +++-------\n 1 file changed, 3 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh\nindex 082368d8c6..d70d13dabe 100755\n--- a/t/t2028-worktree-move.sh\n+++ b/t/t2028-worktree-move.sh\n@@ -75,7 +75,7 @@ test_expect_success 'move worktree' '\n \tgit worktree move source destination &&\n \ttest_path_is_missing source &&\n \tgit worktree list --porcelain | grep \"^worktree.*/destination\" &&\n-\t! git worktree list --porcelain | grep \"^worktree.*/source\" >empty &&\n+\t! git worktree list --porcelain | grep \"^worktree.*/source\" &&\n \tgit -C destination log --format=%s >actual2 &&\n \techo init >expected2 &&\n \ttest_cmp expected2 actual2\n@@ -86,10 +86,10 @@ test_expect_success 'move main worktree' '\n '\n \n test_expect_success 'move worktree to another dir' '\n-\ttoplevel=\"$(pwd)\" &&\n \tmkdir some-dir &&\n \tgit worktree move destination some-dir &&\n-\ttest_path_is_missing source &&\n+\ttest_when_finished \"git worktree move some-dir/destination destination\" &&\n+\ttest_path_is_missing destination &&\n \tgit worktree list --porcelain | grep \"^worktree.*/some-dir/destination\" &&\n \tgit -C some-dir/destination log --format=%s >actual2 &&\n \techo init >expected2 &&\n@@ -100,10 +100,6 @@ test_expect_success 'remove main worktree' '\n \ttest_must_fail git worktree remove .\n '\n \n-test_expect_success 'move some-dir/destination back' '\n-\tgit worktree move some-dir/destination destination\n-'\n-\n test_expect_success 'remove locked worktree' '\n \tgit worktree lock destination &&\n \ttest_when_finished \"git worktree unlock destination\" &&\n-- \n2.16.2.660.g709887971b\n\n"},{"id":"340975","messageId":"CAPig+cT88ZsD4=U0NUPM2=sv43gxFEf7ivxu1zMHr0gsMGdTmg@mail.gmail.com","threadId":"47678","inReplyTo":"20180212094940.23834-1-pclouds@gmail.com","subject":"Re: [PATCH v2 0/7] nd/worktree-move reboot","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-03-04T05:36:47Z","receivedAt":"2018-03-04T05:36:53Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Feb 12, 2018 at 4:49 AM, Nguyễn Thái Ngọc Duy wrote:\n> v2 basically fixes lots of comments from Eric (many thanks!): memory\n> leak, typos, document updates, tests, corner case fixes.\n> Interdiff:\n\nThanks, I finally got around to doing a full re-read of the entire\nseries. v2 appears to address all issues raised in my review[1] of v1.\n\nI did find a few minor issues in tests newly added and revised in v2.\nHowever, since v2 already migrated to 'next', rather than pointing out\nthe issues as review comments, I instead sent a patch[2] (built atop\nnd/worktree-move-reboot) to address them.\n\n[1]: https://public-inbox.org/git/20180124095357.19645-1-pclouds@gmail.com/\n[2]: https://public-inbox.org/git/20180304052647.26614-1-sunshine@sunshineco.com/\n"},{"id":"341001","messageId":"CACsJy8Ba+CbexdCTBeBNHr0URJfJM8PbAjeVEkYxbpCmEo3shg@mail.gmail.com","threadId":"47678","inReplyTo":"20180304052647.26614-1-sunshine@sunshineco.com","subject":"Re: [PATCH] t2028: fix minor error and issues in newly-added \"worktree move\" tests","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-03-05T09:32:08Z","receivedAt":"2018-03-05T09:32:49Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Mar 4, 2018 at 12:26 PM, Eric Sunshine wrote:\n> Recently-added \"git worktree move\" tests include a minor error and a few\n> small issues. Specifically:\n>\n> * checking non-existence of wrong file (\"source\" instead of\n> \"destination\")\n>\n> * unneeded redirect (\">empty\")\n>\n> * unused variable (\"toplevel\")\n>\n> * restoring a worktree location by means of a separate test somewhat\n> distant from the test which moved it rather than using\n> test_when_finished() to restore it in a self-contained fashion\n\nArgh... You're right again :) This looks good.\n\n>\n> Signed-off-by: Eric Sunshine \n> ---\n>\n> This patch is built atop nd/worktree-move-reboot in 'next'.\n>\n> I didn't get around to doing a proper review of nd/worktree-move-reboot\n> v2 [1] until after it had graduated to 'next'. Although v2 fixed all the\n> issues identified in my review of v1 [2], it introduced a few minor\n> issues of its own. This patch addresses those issues.\n>\n> [1]: https://public-inbox.org/git/20180212094940.23834-1-pclouds@gmail.com/\n> [2]: https://public-inbox.org/git/20180124095357.19645-1-pclouds@gmail.com/\n>\n> t/t2028-worktree-move.sh | 10 +++-------\n> 1 file changed, 3 insertions(+), 7 deletions(-)\n>\n> diff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh\n> index 082368d8c6..d70d13dabe 100755\n> --- a/t/t2028-worktree-move.sh\n> +++ b/t/t2028-worktree-move.sh\n> @@ -75,7 +75,7 @@ test_expect_success 'move worktree' '\n> git worktree move source destination &&\n> test_path_is_missing source &&\n> git worktree list --porcelain | grep \"^worktree.*/destination\" &&\n> - ! git worktree list --porcelain | grep \"^worktree.*/source\" >empty &&\n> + ! git worktree list --porcelain | grep \"^worktree.*/source\" &&\n> git -C destination log --format=%s >actual2 &&\n> echo init >expected2 &&\n> test_cmp expected2 actual2\n> @@ -86,10 +86,10 @@ test_expect_success 'move main worktree' '\n> '\n>\n> test_expect_success 'move worktree to another dir' '\n> - toplevel=\"$(pwd)\" &&\n> mkdir some-dir &&\n> git worktree move destination some-dir &&\n> - test_path_is_missing source &&\n> + test_when_finished \"git worktree move some-dir/destination destination\" &&\n> + test_path_is_missing destination &&\n> git worktree list --porcelain | grep \"^worktree.*/some-dir/destination\" &&\n> git -C some-dir/destination log --format=%s >actual2 &&\n> echo init >expected2 &&\n> @@ -100,10 +100,6 @@ test_expect_success 'remove main worktree' '\n> test_must_fail git worktree remove .\n> '\n>\n> -test_expect_success 'move some-dir/destination back' '\n> - git worktree move some-dir/destination destination\n> -'\n> -\n> test_expect_success 'remove locked worktree' '\n> git worktree lock destination &&\n> test_when_finished \"git worktree unlock destination\" &&\n> --\n> 2.16.2.660.g709887971b\n>\n-- \nDuy\n"},{"id":"341017","messageId":"20180305124848.7272-1-szeder.dev@gmail.com","threadId":"47678","inReplyTo":"20180304052647.26614-1-sunshine@sunshineco.com","subject":"Re: [PATCH] t2028: fix minor error and issues in newly-added \"worktree move\" tests","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-03-05T12:48:48Z","receivedAt":"2018-03-05T12:49:17Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"> Recently-added \"git worktree move\" tests include a minor error and a few\n> small issues. Specifically:\n> \n> * checking non-existence of wrong file (\"source\" instead of\n> \"destination\")\n> \n> * unneeded redirect (\">empty\")\n> \n> * unused variable (\"toplevel\")\n> \n> * restoring a worktree location by means of a separate test somewhat\n> distant from the test which moved it rather than using\n> test_when_finished() to restore it in a self-contained fashion\n\nThere is one more issue in these tests.\n \n\n> diff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh\n> index 082368d8c6..d70d13dabe 100755\n> --- a/t/t2028-worktree-move.sh\n> +++ b/t/t2028-worktree-move.sh\n> @@ -75,7 +75,7 @@ test_expect_success 'move worktree' '\n> \tgit worktree move source destination &&\n> \ttest_path_is_missing source &&\n> \tgit worktree list --porcelain | grep \"^worktree.*/destination\" &&\n> -\t! git worktree list --porcelain | grep \"^worktree.*/source\" >empty &&\n> +\t! git worktree list --porcelain | grep \"^worktree.*/source\" &&\n\nThe main purpose of this test script is to test the 'git worktree'\ncommand, but these pipes hide its exit code.\nCould you please save 'git worktree's output into an intermediate\nfile, and run 'grep' on the file's contents?\n\nThis also applies to two other tests in this test script.\n\n> \tgit -C destination log --format=%s >actual2 &&\n> \techo init >expected2 &&\n> \ttest_cmp expected2 actual2\n> @@ -86,10 +86,10 @@ test_expect_success 'move main worktree' '\n> '\n> \n> test_expect_success 'move worktree to another dir' '\n> -\ttoplevel=\"$(pwd)\" &&\n> \tmkdir some-dir &&\n> \tgit worktree move destination some-dir &&\n> -\ttest_path_is_missing source &&\n> +\ttest_when_finished \"git worktree move some-dir/destination destination\" &&\n> +\ttest_path_is_missing destination &&\n> \tgit worktree list --porcelain | grep \"^worktree.*/some-dir/destination\" &&\n> \tgit -C some-dir/destination log --format=%s >actual2 &&\n> \techo init >expected2 &&\n> @@ -100,10 +100,6 @@ test_expect_success 'remove main worktree' '\n> \ttest_must_fail git worktree remove .\n> '\n> \n> -test_expect_success 'move some-dir/destination back' '\n> -\tgit worktree move some-dir/destination destination\n> -'\n> -\n> test_expect_success 'remove locked worktree' '\n> \tgit worktree lock destination &&\n> \ttest_when_finished \"git worktree unlock destination\" &&\n> -- \n> 2.16.2.660.g709887971b\n> \n> \n"},{"id":"341045","messageId":"xmqqh8pu9xmq.fsf@gitster-ct.c.googlers.com","threadId":"47678","inReplyTo":"20180305124848.7272-1-szeder.dev@gmail.com","subject":"Re: [PATCH] t2028: fix minor error and issues in newly-added \"worktree move\" tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-05T18:37:49Z","receivedAt":"2018-03-05T18:37:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> There is one more issue in these tests.\n> ...\n> The main purpose of this test script is to test the 'git worktree'\n> command, but these pipes hide its exit code.\n> Could you please save 'git worktree's output into an intermediate\n> file, and run 'grep' on the file's contents?\n\nHere is what I tentatively came up with, while deciding what should\nbe queued based on Eric's patch, as a possible squash/fixup.\n\n t/t2028-worktree-move.sh | 12 ++++++++----\n 1 file changed, 8 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh\nindex d70d13dabe..1c391f370e 100755\n--- a/t/t2028-worktree-move.sh\n+++ b/t/t2028-worktree-move.sh\n@@ -7,7 +7,8 @@ test_description='test git worktree move, remove, lock and unlock'\n test_expect_success 'setup' '\n \ttest_commit init &&\n \tgit worktree add source &&\n-\tgit worktree list --porcelain | grep \"^worktree\" >actual &&\n+\tgit worktree list --porcelain >out &&\n+\tgrep \"^worktree\" out >actual &&\n \tcat <<-EOF >expected &&\n \tworktree $(pwd)\n \tworktree $(pwd)/source\n@@ -74,8 +75,10 @@ test_expect_success 'move worktree' '\n \ttoplevel=\"$(pwd)\" &&\n \tgit worktree move source destination &&\n \ttest_path_is_missing source &&\n-\tgit worktree list --porcelain | grep \"^worktree.*/destination\" &&\n-\t! git worktree list --porcelain | grep \"^worktree.*/source\" &&\n+\tgit worktree list --porcelain >out &&\n+\tgrep \"^worktree.*/destination\" out &&\n+\tgit worktree list --porcelain >out &&\n+\t! grep \"^worktree.*/source\" out &&\n \tgit -C destination log --format=%s >actual2 &&\n \techo init >expected2 &&\n \ttest_cmp expected2 actual2\n@@ -90,7 +93,8 @@ test_expect_success 'move worktree to another dir' '\n \tgit worktree move destination some-dir &&\n \ttest_when_finished \"git worktree move some-dir/destination destination\" &&\n \ttest_path_is_missing destination &&\n-\tgit worktree list --porcelain | grep \"^worktree.*/some-dir/destination\" &&\n+\tgit worktree list --porcelain >out &&\n+\tgrep \"^worktree.*/some-dir/destination\" out &&\n \tgit -C some-dir/destination log --format=%s >actual2 &&\n \techo init >expected2 &&\n \ttest_cmp expected2 actual2\n"},{"id":"341047","messageId":"CAPig+cQLmYQeRhPxvZHmY7gApnbE25H_KoSWs-ZjuBo4BruimQ@mail.gmail.com","threadId":"47678","inReplyTo":"xmqqh8pu9xmq.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] t2028: fix minor error and issues in newly-added \"worktree move\" tests","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-03-05T18:44:00Z","receivedAt":"2018-03-05T18:44:06Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Mar 5, 2018 at 1:37 PM, Junio C Hamano wrote:\n> SZEDER Gábor writes:\n>\n>> Could you please save 'git worktree's output into an intermediate\n>> file, and run 'grep' on the file's contents?\n>\n> Here is what I tentatively came up with, while deciding what should\n> be queued based on Eric's patch, as a possible squash/fixup.\n\nThanks for saving a round-trip. One comment below...\n\n> diff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh\n> @@ -74,8 +75,10 @@ test_expect_success 'move worktree' '\n> toplevel=\"$(pwd)\" &&\n> git worktree move source destination &&\n> test_path_is_missing source &&\n> - git worktree list --porcelain | grep \"^worktree.*/destination\" &&\n> - ! git worktree list --porcelain | grep \"^worktree.*/source\" &&\n> + git worktree list --porcelain >out &&\n> + grep \"^worktree.*/destination\" out &&\n> + git worktree list --porcelain >out &&\n> + ! grep \"^worktree.*/source\" out &&\n\nThe second \"git worktree list --porcelain >out\" can be dropped,\nleaving the two grep's back-to-back since it they can test the same\n'out' file\n\n> git -C destination log --format=%s >actual2 &&\n> echo init >expected2 &&\n> test_cmp expected2 actual2\n"}]}