{"thread":{"id":"45756","subject":"[PATCH 0/6] nd/worktree-move update","startedAt":"2017-04-20T10:11:21Z","lastAt":"2017-04-24T11:15:37Z","messageCount":15,"participants":["Nguyễn Thái Ngọc Duy","Junio C Hamano","Jeff King","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"317317","messageId":"20170420101024.7593-1-pclouds@gmail.com","threadId":"45756","inReplyTo":null,"subject":"[PATCH 0/6] nd/worktree-move update","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-20T10:10:18Z","receivedAt":"2017-04-20T10:11:21Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This\n\n - squashes in Johannes' fix (that's already on jch/nd/worktree-move)\n - fixes the compile problem on latest master (because prefix_filename\n   takes one argument less)\n - fixes the test failure because real_path() is called twice (the\n   first one hidden in read_gitfile_gently) but the output is not\n   duplicated.\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 74fc8578fc..b5afba1646 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -561,9 +561,7 @@ static int move_worktree(int ac, const char **av, const char *prefix)\n \tif (ac != 2)\n \t\tusage_with_options(worktree_usage, options);\n \n-\tstrbuf_addstr(&dst, prefix_filename(prefix,\n-\t\t\t\t\t    strlen(prefix),\n-\t\t\t\t\t    av[1]));\n+\tstrbuf_addstr(&dst, prefix_filename(prefix, av[1]));\n \tif (is_directory(dst.buf))\n \t\t/*\n \t\t * keep going, dst will be appended after we get the\ndiff --git a/worktree.c b/worktree.c\nindex 85bf481cec..c695dcf982 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -311,8 +311,8 @@ static int report(int quiet, const char *fmt, ...)\n int validate_worktree(const struct worktree *wt, int quiet)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n-\tconst char *path;\n-\tint err;\n+\tchar *path;\n+\tint err, ret;\n \n \tif (is_main_worktree(wt)) {\n \t\t/*\n@@ -344,14 +344,17 @@ int validate_worktree(const struct worktree *wt, int quiet)\n \t\treturn report(quiet, _(\"'%s/.git' does not exist\"), wt->path);\n \t}\n \n-\tpath = read_gitfile_gently(sb.buf, &err);\n+\tpath = xstrdup_or_null(read_gitfile_gently(sb.buf, &err));\n \tstrbuf_release(&sb);\n \tif (!path)\n \t\treturn report(quiet, _(\"'%s/.git' is not a .git file, error code %d\"),\n \t\t\t      wt->path, err);\n \n-\tif (fspathcmp(path, real_path(git_common_path(\"worktrees/%s\", wt->id))))\n-\t\treturn report(quiet, _(\"'%s' does not point back to\"),\n+\tret = fspathcmp(path, real_path(git_common_path(\"worktrees/%s\", wt->id)));\n+\tfree(path);\n+\n+\tif (ret)\n+\t\treturn report(quiet, _(\"'%s' does not point back to '%s'\"),\n \t\t\t      wt->path, git_common_path(\"worktrees/%s\", wt->id));\n \n \treturn 0;\n\nNguyễn Thái Ngọc Duy (6):\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\n Documentation/git-worktree.txt         |  28 +++---\n builtin/worktree.c                     | 160 +++++++++++++++++++++++++++++++++\n contrib/completion/git-completion.bash |   5 +-\n t/t2028-worktree-move.sh               |  57 ++++++++++++\n worktree.c                             |  87 ++++++++++++++++++\n worktree.h                             |  11 +++\n 6 files changed, 337 insertions(+), 11 deletions(-)\n\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"317318","messageId":"20170420101024.7593-2-pclouds@gmail.com","threadId":"45756","inReplyTo":"20170420101024.7593-1-pclouds@gmail.com","subject":"[PATCH 1/6] worktree.c: add validate_worktree()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-20T10:10:19Z","receivedAt":"2017-04-20T10:11:23Z","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 <pclouds@gmail.com>\n---\n worktree.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n worktree.h |  5 +++++\n 2 files changed, 71 insertions(+)\n\ndiff --git a/worktree.c b/worktree.c\nindex bae787cf8d..40cc031ac9 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -294,6 +294,72 @@ const char *is_worktree_locked(struct worktree *wt)\n \treturn wt->lock_reason;\n }\n \n+static int report(int quiet, const char *fmt, ...)\n+{\n+\tva_list params;\n+\n+\tif (quiet)\n+\t\treturn -1;\n+\n+\tva_start(params, fmt);\n+\tvfprintf(stderr, fmt, params);\n+\tfputc('\\n', stderr);\n+\tva_end(params);\n+\treturn -1;\n+}\n+\n+int validate_worktree(const struct worktree *wt, int quiet)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tchar *path;\n+\tint err, ret;\n+\n+\tif (is_main_worktree(wt)) {\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(&sb, \"%s/.git\", wt->path);\n+\t\tif (!is_directory(sb.buf)) {\n+\t\t\tstrbuf_release(&sb);\n+\t\t\treturn report(quiet, _(\"'%s/.git' at main worktree is not the repository directory\"),\n+\t\t\t\t      wt->path);\n+\t\t}\n+\t\treturn 0;\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\treturn report(quiet, _(\"'%s' file does not contain absolute path to the worktree location\"),\n+\t\t\t      git_common_path(\"worktrees/%s/gitdir\", wt->id));\n+\n+\tstrbuf_addf(&sb, \"%s/.git\", wt->path);\n+\tif (!file_exists(sb.buf)) {\n+\t\tstrbuf_release(&sb);\n+\t\treturn report(quiet, _(\"'%s/.git' does not exist\"), wt->path);\n+\t}\n+\n+\tpath = xstrdup_or_null(read_gitfile_gently(sb.buf, &err));\n+\tstrbuf_release(&sb);\n+\tif (!path)\n+\t\treturn report(quiet, _(\"'%s/.git' is not a .git file, error code %d\"),\n+\t\t\t      wt->path, err);\n+\n+\tret = fspathcmp(path, real_path(git_common_path(\"worktrees/%s\", wt->id)));\n+\tfree(path);\n+\n+\tif (ret)\n+\t\treturn report(quiet, _(\"'%s' does not point back to '%s'\"),\n+\t\t\t      wt->path, git_common_path(\"worktrees/%s\", wt->id));\n+\n+\treturn 0;\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 6bfb985203..33f7405e33 100644\n--- a/worktree.h\n+++ b/worktree.h\n@@ -58,6 +58,11 @@ extern int is_main_worktree(const struct worktree *wt);\n extern const char *is_worktree_locked(struct worktree *wt);\n \n /*\n+ * Return zero if the worktree is in good condition.\n+ */\n+extern int validate_worktree(const struct worktree *wt, int quiet);\n+\n+/*\n  * Free up the memory for worktree(s)\n  */\n extern void free_worktrees(struct worktree **);\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"317319","messageId":"20170420101024.7593-3-pclouds@gmail.com","threadId":"45756","inReplyTo":"20170420101024.7593-1-pclouds@gmail.com","subject":"[PATCH 2/6] worktree.c: add update_worktree_location()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-20T10:10:20Z","receivedAt":"2017-04-20T10:11: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 <pclouds@gmail.com>\n---\n worktree.c | 21 +++++++++++++++++++++\n worktree.h |  6 ++++++\n 2 files changed, 27 insertions(+)\n\ndiff --git a/worktree.c b/worktree.c\nindex 40cc031ac9..c695dcf982 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -360,6 +360,27 @@ int validate_worktree(const struct worktree *wt, int quiet)\n \treturn 0;\n }\n \n+int update_worktree_location(struct worktree *wt, const char *path_)\n+{\n+\tstruct strbuf path = STRBUF_INIT;\n+\tint ret = 0;\n+\n+\tif (is_main_worktree(wt))\n+\t\treturn 0;\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\tret = 0;\n+\t}\n+\tstrbuf_release(&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 33f7405e33..b896bdec55 100644\n--- a/worktree.h\n+++ b/worktree.h\n@@ -63,6 +63,12 @@ extern const char *is_worktree_locked(struct worktree *wt);\n extern int validate_worktree(const struct worktree *wt, int quiet);\n \n /*\n+ * Update worktrees/xxx/gitdir with the new path.\n+ */\n+extern int 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 extern void free_worktrees(struct worktree **);\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"317320","messageId":"20170420101024.7593-4-pclouds@gmail.com","threadId":"45756","inReplyTo":"20170420101024.7593-1-pclouds@gmail.com","subject":"[PATCH 3/6] worktree move: new command","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-20T10:10:21Z","receivedAt":"2017-04-20T10:11:36Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"There 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 the\n   git repository where it is. The repo essentially becomes bare after\n   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 <pclouds@gmail.com>\n---\n Documentation/git-worktree.txt         |  7 +++++-\n builtin/worktree.c                     | 41 ++++++++++++++++++++++++++++++++++\n contrib/completion/git-completion.bash |  2 +-\n t/t2028-worktree-move.sh               | 31 +++++++++++++++++++++++++\n 4 files changed, 79 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt\nindex 553cf8413f..b47a3247bb 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] [-b <new-branch>] <path> [<branch>]\n 'git worktree list' [--porcelain]\n 'git worktree lock' [--reason <string>] <worktree>\n+'git worktree move' <worktree> <new-path>\n 'git worktree prune' [-n] [-v] [--expire <expire>]\n 'git worktree unlock' <worktree>\n \n@@ -71,6 +72,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 yet.\n+\n prune::\n \n Prune working tree information in $GIT_DIR/worktrees.\n@@ -252,7 +258,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 9993ded41a..e3fbfe2a71 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -15,6 +15,7 @@ static const char * const worktree_usage[] = {\n \tN_(\"git worktree add [<options>] <path> [<branch>]\"),\n \tN_(\"git worktree list [<options>]\"),\n \tN_(\"git worktree lock [<options>] <path>\"),\n+\tN_(\"git worktree move <worktree> <new-path>\"),\n \tN_(\"git worktree prune [<options>]\"),\n \tN_(\"git worktree unlock <path>\"),\n \tNULL\n@@ -525,6 +526,44 @@ 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+\tconst char *reason;\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+\tstrbuf_addstr(&dst, prefix_filename(prefix, av[1]));\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 directory\"), av[0]);\n+\tif (is_main_worktree(wt))\n+\t\tdie(_(\"'%s' is a main working directory\"), av[0]);\n+\treason = is_worktree_locked(wt);\n+\tif (reason) {\n+\t\tif (*reason)\n+\t\t\tdie(_(\"already locked, reason: %s\"), reason);\n+\t\tdie(_(\"already locked, no reason\"));\n+\t}\n+\tif (validate_worktree(wt, 0))\n+\t\treturn -1;\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+\treturn update_worktree_location(wt, dst.buf);\n+}\n+\n int cmd_worktree(int ac, const char **av, const char *prefix)\n {\n \tstruct option options[] = {\n@@ -547,5 +586,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 1150164d5c..651809c52f 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -3036,7 +3036,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.11.0.157.gd943d85\n\n"},{"id":"317321","messageId":"20170420101024.7593-5-pclouds@gmail.com","threadId":"45756","inReplyTo":"20170420101024.7593-1-pclouds@gmail.com","subject":"[PATCH 4/6] worktree move: accept destination as directory","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-20T10:10:22Z","receivedAt":"2017-04-20T10:11:38Z","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 <pclouds@gmail.com>\n---\n builtin/worktree.c | 19 ++++++++++++++++++-\n 1 file changed, 18 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex e3fbfe2a71..116507e47e 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -540,7 +540,13 @@ static int move_worktree(int ac, const char **av, const char *prefix)\n \t\tusage_with_options(worktree_usage, options);\n \n \tstrbuf_addstr(&dst, prefix_filename(prefix, av[1]));\n-\tif (file_exists(dst.buf))\n+\tif (is_directory(dst.buf))\n+\t\t/*\n+\t\t * keep going, dst will be appended after we get the\n+\t\t * source's absolute path\n+\t\t */\n+\t\t;\n+\telse if (file_exists(dst.buf))\n \t\tdie(_(\"target '%s' already exists\"), av[1]);\n \n \tworktrees = get_worktrees(0);\n@@ -558,6 +564,17 @@ static int move_worktree(int ac, const char **av, const char *prefix)\n \tif (validate_worktree(wt, 0))\n \t\treturn -1;\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}\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-- \n2.11.0.157.gd943d85\n\n"},{"id":"317322","messageId":"20170420101024.7593-6-pclouds@gmail.com","threadId":"45756","inReplyTo":"20170420101024.7593-1-pclouds@gmail.com","subject":"[PATCH 5/6] worktree move: refuse to move worktrees with submodules","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-20T10:10:23Z","receivedAt":"2017-04-20T10:11:45Z","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 <pclouds@gmail.com>\n---\n builtin/worktree.c | 23 +++++++++++++++++++++++\n 1 file changed, 23 insertions(+)\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 116507e47e..825b3e9d9a 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -526,6 +526,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 = {0};\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(_(\"This working tree contains submodules and cannot be moved yet\"));\n+}\n+\n static int move_worktree(int ac, const char **av, const char *prefix)\n {\n \tstruct option options[] = {\n@@ -564,6 +585,8 @@ static int move_worktree(int ac, const char **av, const char *prefix)\n \tif (validate_worktree(wt, 0))\n \t\treturn -1;\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.11.0.157.gd943d85\n\n"},{"id":"317323","messageId":"20170420101024.7593-7-pclouds@gmail.com","threadId":"45756","inReplyTo":"20170420101024.7593-1-pclouds@gmail.com","subject":"[PATCH 6/6] worktree remove: new command","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-20T10:10:24Z","receivedAt":"2017-04-20T10:11:51Z","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 <pclouds@gmail.com>\n---\n Documentation/git-worktree.txt         | 21 +++++----\n builtin/worktree.c                     | 79 ++++++++++++++++++++++++++++++++++\n contrib/completion/git-completion.bash |  5 ++-\n t/t2028-worktree-move.sh               | 26 +++++++++++\n 4 files changed, 121 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt\nindex b47a3247bb..020eeb157f 100644\n--- a/Documentation/git-worktree.txt\n+++ b/Documentation/git-worktree.txt\n@@ -14,6 +14,7 @@ SYNOPSIS\n 'git worktree lock' [--reason <string>] <worktree>\n 'git worktree move' <worktree> <new-path>\n 'git worktree prune' [-n] [-v] [--expire <expire>]\n+'git worktree remove' [--force] <worktree>\n 'git worktree unlock' <worktree>\n \n DESCRIPTION\n@@ -81,6 +82,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 can be removed with `--force`. The main working tree cannot be\n+removed.\n+\n unlock::\n \n Unlock a working tree, allowing it to be pruned, moved or deleted.\n@@ -90,9 +98,10 @@ OPTIONS\n \n -f::\n --force::\n-\tBy default, `add` refuses to create a new working tree when `<branch>`\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`<branch>` is already checked out by another working tree and\n+\t`remove` refuses to remove an unclean working tree. This option\n+\toverrides that safeguard.\n \n -b <new-branch>::\n -B <new-branch>::\n@@ -253,12 +262,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 825b3e9d9a..b5afba1646 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 lock [<options>] <path>\"),\n \tN_(\"git worktree move <worktree> <new-path>\"),\n \tN_(\"git worktree prune [<options>]\"),\n+\tN_(\"git worktree remove [<options>] <worktree>\"),\n \tN_(\"git worktree unlock <path>\"),\n \tNULL\n };\n@@ -604,6 +605,82 @@ static int move_worktree(int ac, const char **av, const char *prefix)\n \treturn update_worktree_location(wt, dst.buf);\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 sb = 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 directory\"), av[0]);\n+\tif (is_main_worktree(wt))\n+\t\tdie(_(\"'%s' is a main working directory\"), av[0]);\n+\treason = is_worktree_locked(wt);\n+\tif (reason) {\n+\t\tif (*reason)\n+\t\t\tdie(_(\"already locked, reason: %s\"), reason);\n+\t\tdie(_(\"already locked, no reason\"));\n+\t}\n+\tif (validate_worktree(wt, 0))\n+\t\treturn -1;\n+\n+\tif (!force) {\n+\t\tstruct argv_array child_env = ARGV_ARRAY_INIT;\n+\t\tstruct child_process cp;\n+\t\tchar buf[1];\n+\n+\t\targv_array_pushf(&child_env, \"%s=%s/.git\",\n+\t\t\t\t GIT_DIR_ENVIRONMENT, wt->path);\n+\t\targv_array_pushf(&child_env, \"%s=%s\",\n+\t\t\t\t GIT_WORK_TREE_ENVIRONMENT, wt->path);\n+\t\tmemset(&cp, 0, sizeof(cp));\n+\t\targv_array_pushl(&cp.args, \"status\", \"--porcelain\", NULL);\n+\t\tcp.env = child_env.argv;\n+\t\tcp.git_cmd = 1;\n+\t\tcp.dir = wt->path;\n+\t\tcp.out = -1;\n+\t\tret = start_command(&cp);\n+\t\tif (ret)\n+\t\t\tdie_errno(_(\"failed to run git-status on '%s', code %d\"),\n+\t\t\t\t  av[0], ret);\n+\t\tret = xread(cp.out, buf, sizeof(buf));\n+\t\tif (ret)\n+\t\t\tdie(_(\"'%s' is dirty, use --force to delete it\"), av[0]);\n+\t\tclose(cp.out);\n+\t\tret = finish_command(&cp);\n+\t\tif (ret)\n+\t\t\tdie_errno(_(\"failed to run git-status on '%s', code %d\"),\n+\t\t\t\t  av[0], ret);\n+\t}\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_reset(&sb);\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+\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@@ -628,5 +705,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 651809c52f..eb8cb7ecee 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -3036,7 +3036,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@@ -3054,6 +3054,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.11.0.157.gd943d85\n\n"},{"id":"317446","messageId":"xmqqh91i7bdy.fsf@gitster.mtv.corp.google.com","threadId":"45756","inReplyTo":"20170420101024.7593-2-pclouds@gmail.com","subject":"Re: [PATCH 1/6] worktree.c: add validate_worktree()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-21T02:16:57Z","receivedAt":"2017-04-21T02:17:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n\n> This function is later used by \"worktree move\" and \"worktree remove\"\n> to ensure that we have a good connection between the repository and\n> the worktree. For example, if a worktree is moved manually, the\n> worktree location recorded in $GIT_DIR/worktrees/.../gitdir is\n> incorrect and we should not move that one.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n>  worktree.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n>  worktree.h |  5 +++++\n>  2 files changed, 71 insertions(+)\n>\n> diff --git a/worktree.c b/worktree.c\n> index bae787cf8d..40cc031ac9 100644\n> --- a/worktree.c\n> +++ b/worktree.c\n> @@ -294,6 +294,72 @@ const char *is_worktree_locked(struct worktree *wt)\n>  \treturn wt->lock_reason;\n>  }\n>  \n> +static int report(int quiet, const char *fmt, ...)\n> +{\n> +\tva_list params;\n> +\n> +\tif (quiet)\n> +\t\treturn -1;\n> +\n> +\tva_start(params, fmt);\n> +\tvfprintf(stderr, fmt, params);\n> +\tfputc('\\n', stderr);\n> +\tva_end(params);\n> +\treturn -1;\n> +}\n> +\n> +int validate_worktree(const struct worktree *wt, int quiet)\n> +{\n> +\tstruct strbuf sb = STRBUF_INIT;\n> +\tchar *path;\n> +\tint err, ret;\n> +\n> +\tif (is_main_worktree(wt)) {\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(&sb, \"%s/.git\", wt->path);\n> +\t\tif (!is_directory(sb.buf)) {\n> +\t\t\tstrbuf_release(&sb);\n> +\t\t\treturn report(quiet, _(\"'%s/.git' at main worktree is not the repository directory\"),\n> +\t\t\t\t      wt->path);\n\nThese messages come without telling what they are.  Should they say\nthat these are errors?  Or does the severity depend on the caller,\ni.e. why they want to know if a particular worktree is valid, and\nsometimes these are errors and other times they are mere warnings?\n\n> +\t\t}\n> +\t\treturn 0;\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\treturn report(quiet, _(\"'%s' file does not contain absolute path to the worktree location\"),\n> +\t\t\t      git_common_path(\"worktrees/%s/gitdir\", wt->id));\n\nIt makes me wonder if this kind of error reporting belongs to the\nplace where these are read (and a new wt is written out to the\nfilesystem, perhaps).  The programmer who wrote this code may have\nknown that wt->path is prepared by reading \"worktrees/%s/gitdir\" and\nwithout doing real_path() or absolute_path() on the result when this\ncode was written, but nothing guarantees that to stay true over time\nas the code evolves.\n\n> +\tstrbuf_addf(&sb, \"%s/.git\", wt->path);\n> +\tif (!file_exists(sb.buf)) {\n> +\t\tstrbuf_release(&sb);\n> +\t\treturn report(quiet, _(\"'%s/.git' does not exist\"), wt->path);\n> +\t}\n> +\n> +\tpath = xstrdup_or_null(read_gitfile_gently(sb.buf, &err));\n> +\tstrbuf_release(&sb);\n> +\tif (!path)\n> +\t\treturn report(quiet, _(\"'%s/.git' is not a .git file, error code %d\"),\n> +\t\t\t      wt->path, err);\n\nThe above should do, at least for now.  It is unfortunate that no\nexisting code needs to turn READ_GITFILE_ERR_* into human readble\nerror message.\n\n> +\tret = fspathcmp(path, real_path(git_common_path(\"worktrees/%s\", wt->id)));\n> +\tfree(path);\n> +\n> +\tif (ret)\n> +\t\treturn report(quiet, _(\"'%s' does not point back to '%s'\"),\n> +\t\t\t      wt->path, git_common_path(\"worktrees/%s\", wt->id));\n> +\n> +\treturn 0;\n> +}\n> +\n>  int is_worktree_being_rebased(const struct worktree *wt,\n>  \t\t\t      const char *target)\n>  {\n> diff --git a/worktree.h b/worktree.h\n> index 6bfb985203..33f7405e33 100644\n> --- a/worktree.h\n> +++ b/worktree.h\n> @@ -58,6 +58,11 @@ extern int is_main_worktree(const struct worktree *wt);\n>  extern const char *is_worktree_locked(struct worktree *wt);\n>  \n>  /*\n> + * Return zero if the worktree is in good condition.\n> + */\n> +extern int validate_worktree(const struct worktree *wt, int quiet);\n> +\n> +/*\n>   * Free up the memory for worktree(s)\n>   */\n>  extern void free_worktrees(struct worktree **);\n"},{"id":"317447","messageId":"xmqqd1c67b4c.fsf@gitster.mtv.corp.google.com","threadId":"45756","inReplyTo":"20170420101024.7593-3-pclouds@gmail.com","subject":"Re: [PATCH 2/6] worktree.c: add update_worktree_location()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-21T02:22:43Z","receivedAt":"2017-04-21T02:23:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n\n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n>  worktree.c | 21 +++++++++++++++++++++\n>  worktree.h |  6 ++++++\n>  2 files changed, 27 insertions(+)\n>\n> diff --git a/worktree.c b/worktree.c\n> index 40cc031ac9..c695dcf982 100644\n> --- a/worktree.c\n> +++ b/worktree.c\n> @@ -360,6 +360,27 @@ int validate_worktree(const struct worktree *wt, int quiet)\n>  \treturn 0;\n>  }\n>  \n> +int update_worktree_location(struct worktree *wt, const char *path_)\n> +{\n> +\tstruct strbuf path = STRBUF_INIT;\n> +\tint ret = 0;\n> +\n> +\tif (is_main_worktree(wt))\n> +\t\treturn 0;\n\nThere is no \"refusing to move main worktree location\" error message\nissued?  Perhaps it is done elsewhere and this is merely for an\nadded safety (in which case it is OK)?\n\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\tret = 0;\n> +\t}\n> +\tstrbuf_release(&path);\n> +\treturn ret;\n> +}\n\nDoes anybody set \"ret\" to anything but 0 in this function?  It is\nunclear what the return value from this function means in the first\nplace, but I am assuming that this is meant to follow the usual\nconvention of 0 for success and negative for error, in which case\nthe \"return early if this is the primary one\" would want to return\n-1, I guess.\n\n>  int is_worktree_being_rebased(const struct worktree *wt,\n>  \t\t\t      const char *target)\n>  {\n> diff --git a/worktree.h b/worktree.h\n> index 33f7405e33..b896bdec55 100644\n> --- a/worktree.h\n> +++ b/worktree.h\n> @@ -63,6 +63,12 @@ extern const char *is_worktree_locked(struct worktree *wt);\n>  extern int validate_worktree(const struct worktree *wt, int quiet);\n>  \n>  /*\n> + * Update worktrees/xxx/gitdir with the new path.\n> + */\n> +extern int 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>  extern void free_worktrees(struct worktree **);\n"},{"id":"317448","messageId":"xmqq4lxi7acb.fsf@gitster.mtv.corp.google.com","threadId":"45756","inReplyTo":"20170420101024.7593-4-pclouds@gmail.com","subject":"Re: [PATCH 3/6] worktree move: new command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-21T02:39:32Z","receivedAt":"2017-04-21T02:39:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n\n> There are two options to move the main worktree, but both have\n> complications, 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 the\n>    git repository where it is. The repo essentially becomes bare after\n>    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>\n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n>  Documentation/git-worktree.txt         |  7 +++++-\n>  builtin/worktree.c                     | 41 ++++++++++++++++++++++++++++++++++\n>  contrib/completion/git-completion.bash |  2 +-\n>  t/t2028-worktree-move.sh               | 31 +++++++++++++++++++++++++\n>  4 files changed, 79 insertions(+), 2 deletions(-)\n>\n> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt\n> index 553cf8413f..b47a3247bb 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] [-b <new-branch>] <path> [<branch>]\n>  'git worktree list' [--porcelain]\n>  'git worktree lock' [--reason <string>] <worktree>\n> +'git worktree move' <worktree> <new-path>\n>  'git worktree prune' [-n] [-v] [--expire <expire>]\n>  'git worktree unlock' <worktree>\n>  \n> @@ -71,6 +72,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 yet.\n> +\n\nYou do not need to say \"yet\" here.  It may come, or it may never\ncome, and it does not matter to the readers an iota when they read\nthis.  The only thing that matters to them that they need to know is\nthat they cannot move the primary one.\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> +\tconst char *reason;\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> +\tstrbuf_addstr(&dst, prefix_filename(prefix, av[1]));\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 directory\"), av[0]);\n> +\tif (is_main_worktree(wt))\n> +\t\tdie(_(\"'%s' is a main working directory\"), av[0]);\n\ns/directory/tree/ perhaps, as Documentation/git-worktree.txt\nadvertises these as \"working trees\"?\n\nThe user _may_ be well aware that av[0] is the primary one, and this\nmessage would solicit a \"Huh--so what?\" from such a user, unless it\nsays that moving the primary one is not supported.\n\n> +\treason = is_worktree_locked(wt);\n> +\tif (reason) {\n> +\t\tif (*reason)\n> +\t\t\tdie(_(\"already locked, reason: %s\"), reason);\n\nGood.\n\n> +\t\tdie(_(\"already locked, no reason\"));\n\nI would suggest s/, no reason// here.  To somebody who reads these\ntwo lines of calls to die(), it is clear what you wanted to mean by\nthat (i.e. we would have given the reason string if it were\navalable, but there isn't, so we are stressing the fact that we got\nnothing), but to an end user who only sees the latter, without\nnecessarily knowing that some other times the former message may\nhave been given, it is confusing.\n\nFor that matter, I am not sure \"already\" is a good phrase to use\nhere.  It's not like the end-user is asking to lock the worktree.\nIf we refuse to move a locked worktree, perhaps we should say so.\ni.e. \"cannot move a locked working tree (reason for locking: %s)\"\nor something.\n\n"},{"id":"317449","messageId":"xmqqzifa5viu.fsf@gitster.mtv.corp.google.com","threadId":"45756","inReplyTo":"20170420101024.7593-5-pclouds@gmail.com","subject":"Re: [PATCH 4/6] worktree move: accept destination as directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-21T02:44:57Z","receivedAt":"2017-04-21T02:45:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n\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 <pclouds@gmail.com>\n> ---\n>  builtin/worktree.c | 19 ++++++++++++++++++-\n>  1 file changed, 18 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/worktree.c b/builtin/worktree.c\n> index e3fbfe2a71..116507e47e 100644\n> --- a/builtin/worktree.c\n> +++ b/builtin/worktree.c\n> @@ -540,7 +540,13 @@ static int move_worktree(int ac, const char **av, const char *prefix)\n>  \t\tusage_with_options(worktree_usage, options);\n>  \n>  \tstrbuf_addstr(&dst, prefix_filename(prefix, av[1]));\n> -\tif (file_exists(dst.buf))\n> +\tif (is_directory(dst.buf))\n> +\t\t/*\n> +\t\t * keep going, dst will be appended after we get the\n> +\t\t * source's absolute path\n> +\t\t */\n> +\t\t;\n\nUgly.  Why not do that \"infer the real destination b/a when existing\ndirectory b/ was given\" here, without \"else if\" so that we can catch\n\"hey, b/a exists already\" with the existing code?  That way you do\nnot have to do is_directory() twice.\n\nFor that matter, perhaps we should go back to 3/6 and move the \"if\ndst.buf exists error out\" to after wt is validated.  That would make\nit stand out why having these is_directory() on the same thing twice\nis ugly, I would think.\n\n> +\telse if (file_exists(dst.buf))\n>  \t\tdie(_(\"target '%s' already exists\"), av[1]);\n>  \n>  \tworktrees = get_worktrees(0);\n> @@ -558,6 +564,17 @@ static int move_worktree(int ac, const char **av, const char *prefix)\n>  \tif (validate_worktree(wt, 0))\n>  \t\treturn -1;\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}\n> +\n>  \tif (rename(wt->path, dst.buf) == -1)\n>  \t\tdie_errno(_(\"failed to move '%s' to '%s'\"), wt->path, dst.buf);\n"},{"id":"317450","messageId":"xmqqvapy5vdy.fsf@gitster.mtv.corp.google.com","threadId":"45756","inReplyTo":"20170420101024.7593-6-pclouds@gmail.com","subject":"Re: [PATCH 5/6] worktree move: refuse to move worktrees with submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-21T02:47:53Z","receivedAt":"2017-04-21T02:48:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n\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\nRight.  I am OK with \"small steps first\" approach like this.  We'd\nneed to document the limitation, though, i.e. \"Note that you cannot\nmove the primary working tree.  Also you cannot move a working tree\nthat has a submodule.\"\n\nAgain drop \"yet\" from the error message.  Our aspiration does not\nease the pain the end users get when they are given the error\nmessage.\n"},{"id":"317451","messageId":"xmqqo9vq5tag.fsf@gitster.mtv.corp.google.com","threadId":"45756","inReplyTo":"20170420101024.7593-7-pclouds@gmail.com","subject":"Re: [PATCH 6/6] worktree remove: new command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-21T03:33:11Z","receivedAt":"2017-04-21T03:33:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\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 directory\"), av[0]);\n> +\tif (is_main_worktree(wt))\n> +\t\tdie(_(\"'%s' is a main working directory\"), av[0]);\n\nThe same comment as 3/6 applies here.\n\n> +\treason = is_worktree_locked(wt);\n> +\tif (reason) {\n> +\t\tif (*reason)\n> +\t\t\tdie(_(\"already locked, reason: %s\"), reason);\n> +\t\tdie(_(\"already locked, no reason\"));\n> +\t}\n\nThe same comment as 3/6 applies here, too.\n\nThis is shared with 3/6 but I wonder if \"--force\" should be usable\nas a way to bust this refusal.  There is an \"unlock\" operation, so\nprobably such a short-cut is not necessary---if you want to repair\nyour repository by moving or removing a working tree and if you\ncannot do so due to an outstanding lock, you can do a two-step dance\n\"unlock followed by move or remove\".  So I am OK with \"--force\" that\ndoes not bust the lock.\n\n> +\tif (validate_worktree(wt, 0))\n> +\t\treturn -1;\n> +\n> +\tif (!force) {\n> +\t\tstruct argv_array child_env = ARGV_ARRAY_INIT;\n> +\t\tstruct child_process cp;\n> +\t\tchar buf[1];\n> +\n> +\t\targv_array_pushf(&child_env, \"%s=%s/.git\",\n> +\t\t\t\t GIT_DIR_ENVIRONMENT, wt->path);\n> +\t\targv_array_pushf(&child_env, \"%s=%s\",\n> +\t\t\t\t GIT_WORK_TREE_ENVIRONMENT, wt->path);\n> +\t\tmemset(&cp, 0, sizeof(cp));\n> +\t\targv_array_pushl(&cp.args, \"status\", \"--porcelain\", NULL);\n> +\t\tcp.env = child_env.argv;\n> +\t\tcp.git_cmd = 1;\n> +\t\tcp.dir = wt->path;\n> +\t\tcp.out = -1;\n> +\t\tret = start_command(&cp);\n> +\t\tif (ret)\n> +\t\t\tdie_errno(_(\"failed to run git-status on '%s', code %d\"),\n> +\t\t\t\t  av[0], ret);\n\nDo we return \"code\" from start_command() that is usable like this?\n\nIs this \"git status --porcelain\" call affected by settings like\n\"submodule.*.ignore\"?  If so, is that a good thing?\n\nOh, submodules.  Unlike \"move\" that may make their .git files\npointing at strange places after the operation finishes, \"remove\"\ndoes not have to worry about them, because they are going to\ndisappear--I think that is OK, but I could be missing some cases\nwhere a working tree that is not dirty may still want to be kept.\nI dunno.\n\n> +\t\tret = xread(cp.out, buf, sizeof(buf));\n> +\t\tif (ret)\n> +\t\t\tdie(_(\"'%s' is dirty, use --force to delete it\"), av[0]);\n> +\t\tclose(cp.out);\n> +\t\tret = finish_command(&cp);\n> +\t\tif (ret)\n> +\t\t\tdie_errno(_(\"failed to run git-status on '%s', code %d\"),\n> +\t\t\t\t  av[0], ret);\n> +\t}\n> +\tstrbuf_addstr(&sb, wt->path);\n> +\tif (remove_dir_recursively(&sb, 0)) {\n\nOh, submodules.  If this working tree has submodules that are not\nyet absorbed, wouldn't this go into their \".git\" recursively and\nend up losing everything?\n\n> +\t\terror_errno(_(\"failed to delete '%s'\"), sb.buf);\n> +\t\tret = -1;\n> +\t}\n> +\tstrbuf_reset(&sb);\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"},{"id":"317491","messageId":"20170421145916.mknekgqzhxffu7di@sigill.intra.peff.net","threadId":"45756","inReplyTo":"20170420101024.7593-1-pclouds@gmail.com","subject":"Re: [PATCH 0/6] nd/worktree-move update","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-21T14:59:16Z","receivedAt":"2017-04-21T15:56:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 20, 2017 at 05:10:18PM +0700, Nguyễn Thái Ngọc Duy wrote:\n\n>  - fixes the compile problem on latest master (because prefix_filename\n>    takes one argument less)\n\nIt also now returns an allocated buffer.\n\nSo:\n\n> --- a/builtin/worktree.c\n> +++ b/builtin/worktree.c\n> @@ -561,9 +561,7 @@ static int move_worktree(int ac, const char **av, const char *prefix)\n>  \tif (ac != 2)\n>  \t\tusage_with_options(worktree_usage, options);\n>  \n> -\tstrbuf_addstr(&dst, prefix_filename(prefix,\n> -\t\t\t\t\t    strlen(prefix),\n> -\t\t\t\t\t    av[1]));\n> +\tstrbuf_addstr(&dst, prefix_filename(prefix, av[1]));\n\n...this is now a leak. Probably:\n\n  const char *filename = prefix_filename(prefix, av[1]);\n  strbuf_attach(&dst, filename, strlen(filename), strlen(filename));\n\nis what you want. That would be less awkward if we had a\nstrbuf_attach_str().\n\nOr if we had a strbuf variant of prefix_filename(), you could do:\n\n  prefix_filename_buf(&dst, prefix, av[1]);\n\nI almost added that when I did the prefix_filename() work, since it uses\na strbuf internally. But there were no callers that would have used it.\nMaybe it's worth doing now.\n\n-- >8 --\nFrom d0b933fc023023a017df9268360aa327c28b90f0 Mon Sep 17 00:00:00 2001\nFrom: Jeff King <peff@peff.net>\nDate: Fri, 21 Apr 2017 10:53:07 -0400\nSubject: [PATCH] prefix_filename: add strbuf variant\n\nNow that prefix_filename() always allocates, it's awkward to\nput its value directly into a strbuf. You have to either\nfree:\n\n  const char *filename = prefix_filename(prefix, arg);\n  strbuf_addstr(&buf, filename);\n  free(filename);\n\nor you have to attach:\n\n  const char *filename = prefix_filename(prefix, arg);\n  strbuf_attach(&buf, filename, strlen(filename), strlen(filename));\n\nSince we're already using a strbuf internally, it's easy to\nprovide a variant that lets you write directly into one:\n\n  prefix_filename_buf(&buf, prefix, arg);\n\nFor consistency with git_path_buf(), the function overwrites\nthe strbuf rather than appending.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n abspath.c | 17 ++++++++++++-----\n cache.h   |  6 ++++++\n 2 files changed, 18 insertions(+), 5 deletions(-)\n\ndiff --git a/abspath.c b/abspath.c\nindex 7f1cfe979..7594f62a5 100644\n--- a/abspath.c\n+++ b/abspath.c\n@@ -246,21 +246,28 @@ char *absolute_pathdup(const char *path)\n \treturn strbuf_detach(&sb, NULL);\n }\n \n-char *prefix_filename(const char *pfx, const char *arg)\n+void prefix_filename_buf(struct strbuf *path, const char *pfx, const char *arg)\n {\n-\tstruct strbuf path = STRBUF_INIT;\n \tsize_t pfx_len = pfx ? strlen(pfx) : 0;\n \n+\tstrbuf_reset(path);\n+\n \tif (!pfx_len)\n \t\t; /* nothing to prefix */\n \telse if (is_absolute_path(arg))\n \t\tpfx_len = 0;\n \telse\n-\t\tstrbuf_add(&path, pfx, pfx_len);\n+\t\tstrbuf_add(path, pfx, pfx_len);\n \n-\tstrbuf_addstr(&path, arg);\n+\tstrbuf_addstr(path, arg);\n #ifdef GIT_WINDOWS_NATIVE\n-\tconvert_slashes(path.buf + pfx_len);\n+\tconvert_slashes(path->buf + pfx_len);\n #endif\n+}\n+\n+char *prefix_filename(const char *pfx, const char *arg)\n+{\n+\tstruct strbuf path = STRBUF_INIT;\n+\tprefix_filename_buf(&path, pfx, arg);\n \treturn strbuf_detach(&path, NULL);\n }\ndiff --git a/cache.h b/cache.h\nindex ba27595d5..209039a98 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -548,6 +548,12 @@ extern char *prefix_path_gently(const char *prefix, int len, int *remaining, con\n  */\n extern char *prefix_filename(const char *prefix, const char *path);\n \n+/*\n+ * Like prefix_filename, but write into \"buf\", overwriting any\n+ * previous contents of the strbuf.\n+ */\n+extern void prefix_filename_buf(struct strbuf *out, const char *prefix, const char *path);\n+\n extern int check_filename(const char *prefix, const char *name);\n extern void verify_filename(const char *prefix,\n \t\t\t    const char *name,\n-- \n2.13.0.rc0.364.g36b4d8031\n\n"},{"id":"317667","messageId":"CACsJy8CCsZWAQJwRQQAYerGLgiaGMZesGZPi4NJvkOtEB0m7MA@mail.gmail.com","threadId":"45756","inReplyTo":"xmqqh91i7bdy.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/6] worktree.c: add validate_worktree()","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-24T11:13:58Z","receivedAt":"2017-04-24T11:15:37Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Apr 21, 2017 at 9:16 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>> +int validate_worktree(const struct worktree *wt, int quiet)\n>> +{\n>> +     struct strbuf sb = STRBUF_INIT;\n>> +     char *path;\n>> +     int err, ret;\n>> +\n>> +     if (is_main_worktree(wt)) {\n>> +             /*\n>> +              * Main worktree using .git file to point to the\n>> +              * repository would make it impossible to know where\n>> +              * the actual worktree is if this function is executed\n>> +              * from another worktree. No .git file support for now.\n>> +              */\n>> +             strbuf_addf(&sb, \"%s/.git\", wt->path);\n>> +             if (!is_directory(sb.buf)) {\n>> +                     strbuf_release(&sb);\n>> +                     return report(quiet, _(\"'%s/.git' at main worktree is not the repository directory\"),\n>> +                                   wt->path);\n>\n> These messages come without telling what they are.  Should they say\n> that these are errors?  Or does the severity depend on the caller,\n> i.e. why they want to know if a particular worktree is valid, and\n> sometimes these are errors and other times they are mere warnings?\n\nI'll save the error in a strbuf and let the caller decide how to\npresent them, which gives better context (e.g. \"unable to move\nworktree because ...\")\n\n>> +             }\n>> +             return 0;\n>> +     }\n>> +\n>> +     /*\n>> +      * Make sure \"gitdir\" file points to a real .git file and that\n>> +      * file points back here.\n>> +      */\n>> +     if (!is_absolute_path(wt->path))\n>> +             return report(quiet, _(\"'%s' file does not contain absolute path to the worktree location\"),\n>> +                           git_common_path(\"worktrees/%s/gitdir\", wt->id));\n>\n> It makes me wonder if this kind of error reporting belongs to the\n> place where these are read (and a new wt is written out to the\n> filesystem, perhaps).  The programmer who wrote this code may have\n> known that wt->path is prepared by reading \"worktrees/%s/gitdir\" and\n> without doing real_path() or absolute_path() on the result when this\n> code was written, but nothing guarantees that to stay true over time\n> as the code evolves.\n\nThis is almost like fsck for worktrees and for now only be checked\nbefore we do destructive things to worktrees (moving, removing..).\n\nYeah we probably should do this at read time too (after checking if a\nworktree is locked, and skip the next check because wt->path may not\nexist). But we probably want to either make this function cheaper, or\nwe cache the worktree list. Probably the latter. It's on my todo list.\n-- \nDuy\n"}]}