{"thread":{"id":"41767","subject":"[PATCH] branch: update all per-worktree HEADs when renaming a branch","startedAt":"2016-03-21T09:50:16Z","lastAt":"2016-03-25T11:56:14Z","messageCount":5,"participants":["Kazuki Yamaguchi","Eric Sunshine","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"281337","messageId":"1458553816-29091-1-git-send-email-k@rhe.jp","threadId":"41767","inReplyTo":null,"subject":"[PATCH] branch: update all per-worktree HEADs when renaming a branch","fromName":"Kazuki Yamaguchi","fromEmail":"k@rhe.jp","sentAt":"2016-03-21T09:50:16Z","receivedAt":"2016-03-21T09:50:16Z","isPatch":true,"sender":{"key":"k@rhe.jp","avatar":"https://avatars.githubusercontent.com/u/1033740?v=4"},"body":"When renaming a branch, the current code only updates the current\nworking tree's HEAD, but it should update .git/HEAD of all checked out\nworking trees.\n\nThis is the current behavior, /path/to/wt's HEAD is not updated:\n\n % git worktree list\n /path/to 2c3c5f2 [master]\n /path/to/wt 2c3c5f2 [oldname]\n % git branch -m master master2\n % git worktree list\n /path/to 2c3c5f2 [master2]\n /path/to/wt 2c3c5f2 [oldname]\n % git branch -m oldname newname\n % git worktree list\n /path/to 2c3c5f2 [master2]\n /path/to/wt 0000000 [oldname]\n\nThis patch fixes this issue by updating all relevant worktree HEADs\nwhen renaming a branch.\n\nSigned-off-by: Kazuki Yamaguchi \n---\n builtin/branch.c | 4 ++--\n t/t3200-branch.sh | 14 +++++++++++++-\n worktree.c | 38 ++++++++++++++++++++++++++++++++++++++\n worktree.h | 7 +++++++\n 4 files changed, 60 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 7b45b6b..596fb5f 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -20,6 +20,7 @@\n #include \"utf8.h\"\n #include \"wt-status.h\"\n #include \"ref-filter.h\"\n+#include \"worktree.h\"\n \n static const char * const builtin_branch_usage[] = {\n \tN_(\"git branch [] [-r | -a] [--merged | --no-merged]\"),\n@@ -552,8 +553,7 @@ static void rename_branch(const char *oldname, const char *newname, int force)\n \tif (recovery)\n \t\twarning(_(\"Renamed a misnamed branch '%s' away\"), oldref.buf + 11);\n \n-\t/* no need to pass logmsg here as HEAD didn't really move */\n-\tif (!strcmp(oldname, head) && create_symref(\"HEAD\", newref.buf, NULL))\n+\tif (update_worktrees_head_symref(oldref.buf, newref.buf))\n \t\tdie(_(\"Branch renamed to %s, but HEAD is not updated!\"), newname);\n \n \tstrbuf_addf(&oldsection, \"branch.%s\", oldref.buf + 11);\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex a897248..da107d0 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -126,7 +126,19 @@ test_expect_success 'git branch -M foo bar should fail when bar is checked out'\n test_expect_success 'git branch -M baz bam should succeed when baz is checked out' '\n \tgit checkout -b baz &&\n \tgit branch bam &&\n-\tgit branch -M baz bam\n+\tgit branch -M baz bam &&\n+\ttest $(git rev-parse --abbrev-ref HEAD) = bam\n+'\n+\n+test_expect_success 'git branch -M baz bam should succeed when baz is checked out as linked working tree' '\n+\tgit checkout master &&\n+\tgit branch &&\n+\tgit worktree add -b baz bazdir &&\n+\tgit branch -M baz bam &&\n+\t(\n+\t\tcd bazdir &&\n+\t\ttest $(git rev-parse --abbrev-ref HEAD) = bam\n+\t)\n '\n \n test_expect_success 'git branch -M master should work when master is checked out' '\ndiff --git a/worktree.c b/worktree.c\nindex 6181a66..9e7d0f3 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -217,3 +217,41 @@ char *find_shared_symref(const char *symref, const char *target)\n \n \treturn existing;\n }\n+\n+int update_worktrees_head_symref(const char *oldref, const char *newref)\n+{\n+\tint error = 0;\n+\tstruct strbuf path = STRBUF_INIT;\n+\tstruct strbuf origref = STRBUF_INIT;\n+\tint i;\n+\tstruct worktree **worktrees = get_worktrees();\n+\n+\tfor (i = 0; worktrees[i]; i++) {\n+\t\tif (worktrees[i]->is_detached)\n+\t\t\tcontinue;\n+\n+\t\tstrbuf_reset(&path);\n+\t\tstrbuf_reset(&origref);\n+\t\tstrbuf_addf(&path, \"%s/HEAD\", worktrees[i]->git_dir);\n+\n+\t\tif (parse_ref(path.buf, &origref, NULL))\n+\t\t\tcontinue;\n+\n+\t\tif (!strcmp(origref.buf, oldref)) {\n+\t\t\tint prefix_len = strlen(absolute_path(get_git_common_dir())) + 1;\n+\t\t\tconst char *symref = path.buf + prefix_len;\n+\n+\t\t\t/* no need to pass logmsg here as HEAD didn't really move */\n+\t\t\tif (create_symref(symref, newref, NULL)) {\n+\t\t\t\terror = -1;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\t}\n+\n+\tstrbuf_release(&path);\n+\tstrbuf_release(&origref);\n+\tfree_worktrees(worktrees);\n+\n+\treturn error;\n+}\ndiff --git a/worktree.h b/worktree.h\nindex b4b3dda..0d15d11 100644\n--- a/worktree.h\n+++ b/worktree.h\n@@ -35,4 +35,11 @@ extern void free_worktrees(struct worktree **);\n */\n extern char *find_shared_symref(const char *symref, const char *target);\n \n+/*\n+ * Update all per-worktree HEADs pointing the old ref to point the new ref.\n+ * This will be used when renaming a branch. Returns 0 if successful,\n+ * non-zero otherwise.\n+ */\n+extern int update_worktrees_head_symref(const char *, const char *);\n+\n #endif\n-- \n2.8.0.rc3.13.gcd7ec22\n"},{"id":"281350","messageId":"CAPig+cRQj4td82DuqL0tD=3znLAmT4nBrfOjK3phqM7iomQseA@mail.gmail.com","threadId":"41767","inReplyTo":"1458553816-29091-1-git-send-email-k@rhe.jp","subject":"Re: [PATCH] branch: update all per-worktree HEADs when renaming a branch","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-21T17:41:28Z","receivedAt":"2016-03-21T17:41:28Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Mar 21, 2016 at 5:50 AM, Kazuki Yamaguchi wrote:\n> When renaming a branch, the current code only updates the current\n> working tree's HEAD, but it should update .git/HEAD of all checked out\n> working trees.\n>\n> This is the current behavior, /path/to/wt's HEAD is not updated:\n> [...]\n> This patch fixes this issue by updating all relevant worktree HEADs\n> when renaming a branch.\n\nMakes sense; seems like a genuine problem. Some comment below...\n\n> Signed-off-by: Kazuki Yamaguchi \n> ---\n> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\n> @@ -126,7 +126,19 @@ test_expect_success 'git branch -M foo bar should fail when bar is checked out'\n> test_expect_success 'git branch -M baz bam should succeed when baz is checked out' '\n> +test_expect_success 'git branch -M baz bam should succeed when baz is checked out as linked working tree' '\n> + git checkout master &&\n> + git branch &&\n> + git worktree add -b baz bazdir &&\n> + git branch -M baz bam &&\n> + (\n> + cd bazdir &&\n> + test $(git rev-parse --abbrev-ref HEAD) = bam\n> + )\n> '\n\nThis can be done more easily without the subshell:\n\n test $(git -C bazdir rev-parse ...) = bam\n\nCan you also expand the test so that it verifies that the rename works\nas expected when the branch is checked out in multiple worktrees,\nrather than just one. Likewise, it would be nice to check branch\nrename from within a worktree in which the branch is checked out (in\naddition to the test above which does the rename from outside such a\nworktree).\n\nMore below...\n\n> diff --git a/worktree.c b/worktree.c\n> @@ -217,3 +217,41 @@ char *find_shared_symref(const char *symref, const char *target)\n> +int update_worktrees_head_symref(const char *oldref, const char *newref)\n> +{\n> + int error = 0;\n> + struct strbuf path = STRBUF_INIT;\n> + struct strbuf origref = STRBUF_INIT;\n> + int i;\n> + struct worktree **worktrees = get_worktrees();\n> +\n> + for (i = 0; worktrees[i]; i++) {\n> + if (worktrees[i]->is_detached)\n> + continue;\n> +\n> + strbuf_reset(&path);\n> + strbuf_reset(&origref);\n> + strbuf_addf(&path, \"%s/HEAD\", worktrees[i]->git_dir);\n> +\n> + if (parse_ref(path.buf, &origref, NULL))\n> + continue;\n> +\n> + if (!strcmp(origref.buf, oldref)) {\n> + int prefix_len = strlen(absolute_path(get_git_common_dir())) + 1;\n> + const char *symref = path.buf + prefix_len;\n> +\n> + /* no need to pass logmsg here as HEAD didn't really move */\n> + if (create_symref(symref, newref, NULL)) {\n> + error = -1;\n> + break;\n\nIs aborting upon the first error desired behavior? (Genuine question.)\nWould it make more sense to continue attempting the rename for the\nremaining worktrees (and remember that an error was encountered)?\nRelated: Since you're now dealing with multiple worktrees, you can do\na better job of letting the user know in which worktree something went\nwrong rather than merely emitting the relatively generic \"Branch\nrenamed to %s, but HEAD is not updated!\".\n\nMore below...\n\n> + }\n> + }\n> + }\n> +\n> + strbuf_release(&path);\n> + strbuf_release(&origref);\n> + free_worktrees(worktrees);\n> +\n> + return error;\n> +}\n> diff --git a/worktree.h b/worktree.h\n> @@ -35,4 +35,11 @@ extern void free_worktrees(struct worktree **);\n> +/*\n> + * Update all per-worktree HEADs pointing the old ref to point the new ref.\n> + * This will be used when renaming a branch. Returns 0 if successful,\n> + * non-zero otherwise.\n> + */\n> +extern int update_worktrees_head_symref(const char *, const char *);\n\nI guess I can understand the desire to libify this functionality,\nhowever, it feels as if it is a feature of \"branch\" rather than\n\"worktree\", hence perhaps it should reside in top-level branch.[hc]?\n"},{"id":"281402","messageId":"CACsJy8BKBiYTsp5wv9ynr++p2TE-dzS9J+DkOPEKumCL-reLVw@mail.gmail.com","threadId":"41767","inReplyTo":"CAPig+cRQj4td82DuqL0tD=3znLAmT4nBrfOjK3phqM7iomQseA@mail.gmail.com","subject":"Re: [PATCH] branch: update all per-worktree HEADs when renaming a branch","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-03-22T00:49:00Z","receivedAt":"2016-03-22T00:49:00Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Mar 22, 2016 at 12:41 AM, Eric Sunshine wrote:\n>> diff --git a/worktree.c b/worktree.c\n>> @@ -217,3 +217,41 @@ char *find_shared_symref(const char *symref, const char *target)\n>> +int update_worktrees_head_symref(const char *oldref, const char *newref)\n>> +{\n>> + int error = 0;\n>> + struct strbuf path = STRBUF_INIT;\n>> + struct strbuf origref = STRBUF_INIT;\n>> + int i;\n>> + struct worktree **worktrees = get_worktrees();\n>> +\n>> + for (i = 0; worktrees[i]; i++) {\n>> + if (worktrees[i]->is_detached)\n>> + continue;\n>> +\n>> + strbuf_reset(&path);\n>> + strbuf_reset(&origref);\n>> + strbuf_addf(&path, \"%s/HEAD\", worktrees[i]->git_dir);\n>> +\n>> + if (parse_ref(path.buf, &origref, NULL))\n>> + continue;\n>> +\n>> + if (!strcmp(origref.buf, oldref)) {\n>> + int prefix_len = strlen(absolute_path(get_git_common_dir())) + 1;\n>> + const char *symref = path.buf + prefix_len;\n>> +\n>> + /* no need to pass logmsg here as HEAD didn't really move */\n>> + if (create_symref(symref, newref, NULL)) {\n>> + error = -1;\n>> + break;\n>\n> Is aborting upon the first error desired behavior? (Genuine question.)\n> Would it make more sense to continue attempting the rename for the\n> remaining worktrees (and remember that an error was encountered)?\n\nSince all these HEADs stay at the same (or close) location, if one\nfails, I think the rest will fail too. Which leads to a series of\nwarnings if we continue anyway. A more interesting approach is update\nHEADs in a transaction, so we successfully update all or we update\nnone. But I do not know if ref transactions can be used for HEAD,\nespecially worktree HEADs. I'm ok with either abort here or continue\nanyway, though.\n-- \nDuy\n"},{"id":"281800","messageId":"20160325113351.GC24729@chikuwa.rhe.jp","threadId":"41767","inReplyTo":"CAPig+cRQj4td82DuqL0tD=3znLAmT4nBrfOjK3phqM7iomQseA@mail.gmail.com","subject":"Re: [PATCH] branch: update all per-worktree HEADs when renaming a branch","fromName":"Kazuki Yamaguchi","fromEmail":"k@rhe.jp","sentAt":"2016-03-25T11:33:51Z","receivedAt":"2016-03-25T11:33:51Z","isPatch":true,"sender":{"key":"k@rhe.jp","avatar":"https://avatars.githubusercontent.com/u/1033740?v=4"},"body":"Hello,\n\nOn 03/22/2016 03:41 AM, Eric Sunshine wrote:\n>> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\n>> @@ -126,7 +126,19 @@ test_expect_success 'git branch -M foo bar should fail when bar is checked out'\n>> test_expect_success 'git branch -M baz bam should succeed when baz is checked out' '\n>> +test_expect_success 'git branch -M baz bam should succeed when baz is checked out as linked working tree' '\n>> + git checkout master &&\n>> + git branch &&\n>> + git worktree add -b baz bazdir &&\n>> + git branch -M baz bam &&\n>> + (\n>> + cd bazdir &&\n>> + test $(git rev-parse --abbrev-ref HEAD) = bam\n>> + )\n>> '\n> \n> This can be done more easily without the subshell:\n> \n> test $(git -C bazdir rev-parse ...) = bam\n\nThank you for reviewing. And sorry for late response.\nI didn't know -C option, thanks.\n\n> \n> Can you also expand the test so that it verifies that the rename works\n> as expected when the branch is checked out in multiple worktrees,\n> rather than just one. Likewise, it would be nice to check branch\n> rename from within a worktree in which the branch is checked out (in\n> addition to the test above which does the rename from outside such a\n> worktree).\n> \n\nI'll add them.\nAnd I noticed my patch is broken in the latter case (rename in a linked\nworking tree).\nSince create_symref() calls resolve_ref_unsafe() and it uses $GIT_DIR\nfor worktree-specific refs thus my patch fails to update main tree's\nHEAD when we are in a linked working tree.\nI'm thinking about adding new flag to resolve_ref_unsafe(), to force\nusing $GIT_COMMON_DIR. This will at the same time allows to remove\nparse_ref() in worktree.c.\n\n> \n>> diff --git a/worktree.c b/worktree.c\n>> @@ -217,3 +217,41 @@ char *find_shared_symref(const char *symref, const char *target)\n>> +int update_worktrees_head_symref(const char *oldref, const char *newref)\n>> +{\n>> + int error = 0;\n>> + struct strbuf path = STRBUF_INIT;\n>> + struct strbuf origref = STRBUF_INIT;\n>> + int i;\n>> + struct worktree **worktrees = get_worktrees();\n>> +\n>> + for (i = 0; worktrees[i]; i++) {\n>> + if (worktrees[i]->is_detached)\n>> + continue;\n>> +\n>> + strbuf_reset(&path);\n>> + strbuf_reset(&origref);\n>> + strbuf_addf(&path, \"%s/HEAD\", worktrees[i]->git_dir);\n>> +\n>> + if (parse_ref(path.buf, &origref, NULL))\n>> + continue;\n>> +\n>> + if (!strcmp(origref.buf, oldref)) {\n>> + int prefix_len = strlen(absolute_path(get_git_common_dir())) + 1;\n>> + const char *symref = path.buf + prefix_len;\n>> +\n>> + /* no need to pass logmsg here as HEAD didn't really move */\n>> + if (create_symref(symref, newref, NULL)) {\n>> + error = -1;\n>> + break;\n> \n> Is aborting upon the first error desired behavior? (Genuine question.)\n> Would it make more sense to continue attempting the rename for the\n> remaining worktrees (and remember that an error was encountered)?\n> Related: Since you're now dealing with multiple worktrees, you can do\n> a better job of letting the user know in which worktree something went\n> wrong rather than merely emitting the relatively generic \"Branch\n> renamed to %s, but HEAD is not updated!\".\n\nI think both is ok.\nBut continuing shouldn't be harm, so continuing might be better in terms\nof that it can tell the user what files need to be fixed manually.\nI'll try it.\n\n>> +}\n>> diff --git a/worktree.h b/worktree.h\n>> @@ -35,4 +35,11 @@ extern void free_worktrees(struct worktree **);\n>> +/*\n>> + * Update all per-worktree HEADs pointing the old ref to point the new ref.\n>> + * This will be used when renaming a branch. Returns 0 if successful,\n>> + * non-zero otherwise.\n>> + */\n>> +extern int update_worktrees_head_symref(const char *, const char *);\n> \n> I guess I can understand the desire to libify this functionality,\n> however, it feels as if it is a feature of \"branch\" rather than\n> \"worktree\", hence perhaps it should reside in top-level branch.[hc]?\n\nI agree, I'll move it.\nI chose worktree.c just because it has parse_ref().\n"},{"id":"281806","messageId":"20160325115614.GA21341@chikuwa.rhe.jp","threadId":"41767","inReplyTo":"CACsJy8BKBiYTsp5wv9ynr++p2TE-dzS9J+DkOPEKumCL-reLVw@mail.gmail.com","subject":"Re: [PATCH] branch: update all per-worktree HEADs when renaming a branch","fromName":"Kazuki Yamaguchi","fromEmail":"k@rhe.jp","sentAt":"2016-03-25T11:56:14Z","receivedAt":"2016-03-25T11:56:14Z","isPatch":true,"sender":{"key":"k@rhe.jp","avatar":"https://avatars.githubusercontent.com/u/1033740?v=4"},"body":"On Tue, Mar 22, 2016 at 07:49:00AM +0700, Duy Nguyen wrote:\n> On Tue, Mar 22, 2016 at 12:41 AM, Eric Sunshine wrote:\n> >> diff --git a/worktree.c b/worktree.c\n> >> @@ -217,3 +217,41 @@ char *find_shared_symref(const char *symref, const char *target)\n> >> +int update_worktrees_head_symref(const char *oldref, const char *newref)\n> >> +{\n> >> + int error = 0;\n> >> + struct strbuf path = STRBUF_INIT;\n> >> + struct strbuf origref = STRBUF_INIT;\n> >> + int i;\n> >> + struct worktree **worktrees = get_worktrees();\n> >> +\n> >> + for (i = 0; worktrees[i]; i++) {\n> >> + if (worktrees[i]->is_detached)\n> >> + continue;\n> >> +\n> >> + strbuf_reset(&path);\n> >> + strbuf_reset(&origref);\n> >> + strbuf_addf(&path, \"%s/HEAD\", worktrees[i]->git_dir);\n> >> +\n> >> + if (parse_ref(path.buf, &origref, NULL))\n> >> + continue;\n> >> +\n> >> + if (!strcmp(origref.buf, oldref)) {\n> >> + int prefix_len = strlen(absolute_path(get_git_common_dir())) + 1;\n> >> + const char *symref = path.buf + prefix_len;\n> >> +\n> >> + /* no need to pass logmsg here as HEAD didn't really move */\n> >> + if (create_symref(symref, newref, NULL)) {\n> >> + error = -1;\n> >> + break;\n> >\n> > Is aborting upon the first error desired behavior? (Genuine question.)\n> > Would it make more sense to continue attempting the rename for the\n> > remaining worktrees (and remember that an error was encountered)?\n> \n> Since all these HEADs stay at the same (or close) location, if one\n> fails, I think the rest will fail too. Which leads to a series of\n> warnings if we continue anyway. A more interesting approach is update\n> HEADs in a transaction, so we successfully update all or we update\n> none. But I do not know if ref transactions can be used for HEAD,\n> especially worktree HEADs. I'm ok with either abort here or continue\n> anyway, though.\n> -- \n> Duy\n\nThanks for suggestion, but it looks like ref_transaction can be used\nonly for updating non-symbolic references. Extending it only for this\npurpose seems too much...\n"}]}