{"thread":{"id":"41828","subject":"[PATCH v2 1/5] refs: add new flag RESOLVE_REF_COMMON_DIR to resolve_ref_unsafe","startedAt":"2016-03-25T18:28:18Z","lastAt":"2016-03-29T18:57:14Z","messageCount":17,"participants":["Kazuki Yamaguchi","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":2,"patchTotal":5},"messages":[{"id":"281854","messageId":"cover.1458927521.git.k@rhe.jp","threadId":"41828","inReplyTo":"1458553816-29091-1-git-send-email-k@rhe.jp","subject":"[PATCH v2 0/5] branch: fix branch operations with multiple working trees","fromName":"Kazuki Yamaguchi","fromEmail":"k@rhe.jp","sentAt":"2016-03-25T18:28:18Z","receivedAt":"2016-03-25T18:28:18Z","isPatch":true,"sender":{"key":"k@rhe.jp","avatar":"https://avatars.githubusercontent.com/u/1033740?v=4"},"body":"[1/5]\nAdds RESOLVE_REF_COMMON_DIR to resolve_ref_unsafe(). The second - fourth\npatch depend on this. At the same time, this allows us to remove\nreimplementation of resolve_ref_unsafe() in worktree.c: parse_ref().\n\n[2/5]\nAdds REF_COMMON_DIR flag to lock_ref_sha1_basic().\n\n[3/5]\nAdds create_symref_common_dir(). Same as create_symref() except it\ndoesn't consider $GIT_DIR. create_symref_common_dir(\"HEAD\", some) always\nupdates .git/HEAD. The next patch uses this.\n\n[4/5]\nFixes the issue of git branch -m.\nThe behavior when one failed has changed from v1: print an error and\ncontinue.\n\n % git branch -m oldname newname\n error: Unable to create '/path/to/.git/worktrees/wt/HEAD.lock': Permission denied\n error: HEAD of working tree /path/to/wt is not updated.\n error: Unable to create '/path/to/.git/worktrees/wt2/HEAD.lock': Permission denied\n error: HEAD of working tree /path/to/wt2 is not updated.\n fatal: Branch renamed to newname, but HEAD is not updated!\n\n[5/5]\nFixes an issue of git branch -d, v1 didn't include this.\nI noticed git branch -d has same issue and this is for it.\nThis patch is unrelated to the above 4 patches, but the cause is same.\nThis can be applied separately.\n\n\nKazuki Yamaguchi (5):\n refs: add new flag RESOLVE_REF_COMMON_DIR to resolve_ref_unsafe\n refs: add REF_COMMON_DIR flag\n refs: add create_symref_common_dir as a variation of create_symref\n branch -m: update all per-worktree HEADs\n branch -d: refuse deleting a branch which is currently checked out\n\n branch.c | 32 ++++++++++++++++\n branch.h | 7 ++++\n builtin/branch.c | 15 ++++----\n refs.h | 11 ++++++\n refs/files-backend.c | 34 ++++++++++++++---\n t/t3200-branch.sh | 29 +++++++++++++-\n worktree.c | 105 ++++++++++++++++-----------------------------------\n 7 files changed, 147 insertions(+), 86 deletions(-)\n\n-- \n2.8.0.rc4.21.g05df949\n"},{"id":"281853","messageId":"7a5fbdea60e3eb8513e686eee4447526450b5131.1458927521.git.k@rhe.jp","threadId":"41828","inReplyTo":"cover.1458927521.git.k@rhe.jp","subject":"[PATCH v2 1/5] refs: add new flag RESOLVE_REF_COMMON_DIR to resolve_ref_unsafe","fromName":"Kazuki Yamaguchi","fromEmail":"k@rhe.jp","sentAt":"2016-03-25T18:28:19Z","receivedAt":"2016-03-25T18:28:19Z","isPatch":true,"sender":{"key":"k@rhe.jp","avatar":"https://avatars.githubusercontent.com/u/1033740?v=4"},"body":"If the new flag RESOLVE_REF_COMMON_DIR is passed to resolve_ref_unsafe,\nit assumes the refname belongs to $GIT_COMMON_DIR.\n\nresolve_ref_unsafe currently has no way to resolve worktree-specific\nrefs such as HEAD of the main working tree when we are in a linked\nworking tree.\nworktree.c has a simplified one for this purpose, and this patch allows\nremoving it.\n\nSigned-off-by: Kazuki Yamaguchi \n---\n refs.h | 4 ++\n refs/files-backend.c | 5 ++-\n worktree.c | 105 ++++++++++++++++-----------------------------------\n 3 files changed, 41 insertions(+), 73 deletions(-)\n\ndiff --git a/refs.h b/refs.h\nindex 2f3decb432cf..5d9ab5c1c5dd 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -25,6 +25,9 @@\n * reference will always be null_sha1 in this case, and the return\n * value is the reference that the symref refers to directly.\n *\n+ * If the RESOLVE_REF_COMMON_DIR flag is passed, assumes the refname is\n+ * always directly under $GIT_COMMON_DIR.\n+ *\n * If flags is non-NULL, set the value that it points to the\n * combination of REF_ISPACKED (if the reference was found among the\n * packed references), REF_ISSYMREF (if the initial reference was a\n@@ -51,6 +54,7 @@\n #define RESOLVE_REF_READING 0x01\n #define RESOLVE_REF_NO_RECURSE 0x02\n #define RESOLVE_REF_ALLOW_BAD_NAME 0x04\n+#define RESOLVE_REF_COMMON_DIR 0x08\n \n extern const char *resolve_ref_unsafe(const char *refname, int resolve_flags,\n \t\t\t\t unsigned char *sha1, int *flags);\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 81f68f846b69..a534f1a1e078 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1448,7 +1448,10 @@ static const char *resolve_ref_1(const char *refname,\n \t\t}\n \n \t\tstrbuf_reset(sb_path);\n-\t\tstrbuf_git_path(sb_path, \"%s\", refname);\n+\t\tif (resolve_flags & RESOLVE_REF_COMMON_DIR)\n+\t\t\tstrbuf_addf(sb_path, \"%s/%s\", get_git_common_dir(), refname);\n+\t\telse\n+\t\t\tstrbuf_git_path(sb_path, \"%s\", refname);\n \t\tpath = sb_path->buf;\n \n \t\t/*\ndiff --git a/worktree.c b/worktree.c\nindex 6181a66f1ee2..e9e945ea1373 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -16,53 +16,19 @@ void free_worktrees(struct worktree **worktrees)\n \tfree (worktrees);\n }\n \n-/*\n- * read 'path_to_ref' into 'ref'. Also if is_detached is not NULL,\n- * set is_detached to 1 (0) if the ref is detatched (is not detached).\n- *\n- * $GIT_COMMON_DIR/$symref (e.g. HEAD) is practically outside $GIT_DIR so\n- * for linked worktrees, `resolve_ref_unsafe()` won't work (it uses\n- * git_path). Parse the ref ourselves.\n- *\n- * return -1 if the ref is not a proper ref, 0 otherwise (success)\n- */\n-static int parse_ref(char *path_to_ref, struct strbuf *ref, int *is_detached)\n-{\n-\tif (is_detached)\n-\t\t*is_detached = 0;\n-\tif (!strbuf_readlink(ref, path_to_ref, 0)) {\n-\t\t/* HEAD is symbolic link */\n-\t\tif (!starts_with(ref->buf, \"refs/\") ||\n-\t\t\t\tcheck_refname_format(ref->buf, 0))\n-\t\t\treturn -1;\n-\t} else if (strbuf_read_file(ref, path_to_ref, 0) >= 0) {\n-\t\t/* textual symref or detached */\n-\t\tif (!starts_with(ref->buf, \"ref:\")) {\n-\t\t\tif (is_detached)\n-\t\t\t\t*is_detached = 1;\n-\t\t} else {\n-\t\t\tstrbuf_remove(ref, 0, strlen(\"ref:\"));\n-\t\t\tstrbuf_trim(ref);\n-\t\t\tif (check_refname_format(ref->buf, 0))\n-\t\t\t\treturn -1;\n-\t\t}\n-\t} else\n-\t\treturn -1;\n-\treturn 0;\n-}\n-\n /**\n- * Add the head_sha1 and head_ref (if not detached) to the given worktree\n+ * Add the is_detached, head_sha1 and head_ref (if not detached) to the given worktree\n */\n-static void add_head_info(struct strbuf *head_ref, struct worktree *worktree)\n+static void add_head_info(const char *head_ref, const unsigned char *sha1,\n+\t\t\t struct worktree *worktree)\n {\n-\tif (head_ref->len) {\n-\t\tif (worktree->is_detached) {\n-\t\t\tget_sha1_hex(head_ref->buf, worktree->head_sha1);\n-\t\t} else {\n-\t\t\tresolve_ref_unsafe(head_ref->buf, 0, worktree->head_sha1, NULL);\n-\t\t\tworktree->head_ref = strbuf_detach(head_ref, NULL);\n-\t\t}\n+\tworktree->is_detached = !is_null_sha1(sha1);\n+\tif (worktree->is_detached) {\n+\t\thashcpy(worktree->head_sha1, sha1);\n+\t\tworktree->head_ref = NULL;\n+\t} else {\n+\t\tresolve_ref_unsafe(head_ref, 0, worktree->head_sha1, NULL);\n+\t\tworktree->head_ref = xstrdup(head_ref);\n \t}\n }\n \n@@ -72,12 +38,11 @@ static void add_head_info(struct strbuf *head_ref, struct worktree *worktree)\n static struct worktree *get_main_worktree(void)\n {\n \tstruct worktree *worktree = NULL;\n-\tstruct strbuf path = STRBUF_INIT;\n \tstruct strbuf worktree_path = STRBUF_INIT;\n \tstruct strbuf gitdir = STRBUF_INIT;\n-\tstruct strbuf head_ref = STRBUF_INIT;\n+\tconst char *head_ref;\n+\tunsigned char sha1[20];\n \tint is_bare = 0;\n-\tint is_detached = 0;\n \n \tstrbuf_addf(&gitdir, \"%s\", absolute_path(get_git_common_dir()));\n \tstrbuf_addbuf(&worktree_path, &gitdir);\n@@ -85,24 +50,21 @@ static struct worktree *get_main_worktree(void)\n \tif (is_bare)\n \t\tstrbuf_strip_suffix(&worktree_path, \"/.\");\n \n-\tstrbuf_addf(&path, \"%s/HEAD\", get_git_common_dir());\n-\n-\tif (parse_ref(path.buf, &head_ref, &is_detached) < 0)\n+\thead_ref = resolve_ref_unsafe(\"HEAD\",\n+\t\t\tRESOLVE_REF_NO_RECURSE | RESOLVE_REF_COMMON_DIR,\n+\t\t\tsha1, NULL);\n+\tif (!head_ref)\n \t\tgoto done;\n \n \tworktree = xmalloc(sizeof(struct worktree));\n \tworktree->path = strbuf_detach(&worktree_path, NULL);\n \tworktree->git_dir = strbuf_detach(&gitdir, NULL);\n \tworktree->is_bare = is_bare;\n-\tworktree->head_ref = NULL;\n-\tworktree->is_detached = is_detached;\n-\tadd_head_info(&head_ref, worktree);\n+\tadd_head_info(head_ref, sha1, worktree);\n \n done:\n-\tstrbuf_release(&path);\n \tstrbuf_release(&gitdir);\n \tstrbuf_release(&worktree_path);\n-\tstrbuf_release(&head_ref);\n \treturn worktree;\n }\n \n@@ -112,8 +74,8 @@ static struct worktree *get_linked_worktree(const char *id)\n \tstruct strbuf path = STRBUF_INIT;\n \tstruct strbuf worktree_path = STRBUF_INIT;\n \tstruct strbuf gitdir = STRBUF_INIT;\n-\tstruct strbuf head_ref = STRBUF_INIT;\n-\tint is_detached = 0;\n+\tconst char *head_ref;\n+\tunsigned char sha1[20];\n \n \tif (!id)\n \t\tdie(\"Missing linked worktree name\");\n@@ -133,24 +95,23 @@ static struct worktree *get_linked_worktree(const char *id)\n \t}\n \n \tstrbuf_reset(&path);\n-\tstrbuf_addf(&path, \"%s/worktrees/%s/HEAD\", get_git_common_dir(), id);\n-\n-\tif (parse_ref(path.buf, &head_ref, &is_detached) < 0)\n+\tstrbuf_addf(&path, \"worktrees/%s/HEAD\", id);\n+\thead_ref = resolve_ref_unsafe(path.buf,\n+\t\t\tRESOLVE_REF_NO_RECURSE | RESOLVE_REF_COMMON_DIR,\n+\t\t\tsha1, NULL);\n+\tif (!head_ref)\n \t\tgoto done;\n \n \tworktree = xmalloc(sizeof(struct worktree));\n \tworktree->path = strbuf_detach(&worktree_path, NULL);\n \tworktree->git_dir = strbuf_detach(&gitdir, NULL);\n \tworktree->is_bare = 0;\n-\tworktree->head_ref = NULL;\n-\tworktree->is_detached = is_detached;\n-\tadd_head_info(&head_ref, worktree);\n+\tadd_head_info(head_ref, sha1, worktree);\n \n done:\n \tstrbuf_release(&path);\n \tstrbuf_release(&gitdir);\n \tstrbuf_release(&worktree_path);\n-\tstrbuf_release(&head_ref);\n \treturn worktree;\n }\n \n@@ -192,27 +153,27 @@ char *find_shared_symref(const char *symref, const char *target)\n {\n \tchar *existing = NULL;\n \tstruct strbuf path = STRBUF_INIT;\n-\tstruct strbuf sb = STRBUF_INIT;\n \tstruct worktree **worktrees = get_worktrees();\n+\tconst char *resolved;\n \tint i = 0;\n+\tunsigned char sha1[20];\n+\tint common_prefix_len = strlen(absolute_path(get_git_common_dir())) + 1;\n \n \tfor (i = 0; worktrees[i]; i++) {\n \t\tstrbuf_reset(&path);\n-\t\tstrbuf_reset(&sb);\n \t\tstrbuf_addf(&path, \"%s/%s\", worktrees[i]->git_dir, symref);\n+\t\tstrbuf_remove(&path, 0, common_prefix_len);\n \n-\t\tif (parse_ref(path.buf, &sb, NULL)) {\n-\t\t\tcontinue;\n-\t\t}\n-\n-\t\tif (!strcmp(sb.buf, target)) {\n+\t\tresolved = resolve_ref_unsafe(path.buf,\n+\t\t\t\tRESOLVE_REF_NO_RECURSE | RESOLVE_REF_COMMON_DIR,\n+\t\t\t\tsha1, NULL);\n+\t\tif (resolved && !strcmp(resolved, target)) {\n \t\t\texisting = xstrdup(worktrees[i]->path);\n \t\t\tbreak;\n \t\t}\n \t}\n \n \tstrbuf_release(&path);\n-\tstrbuf_release(&sb);\n \tfree_worktrees(worktrees);\n \n \treturn existing;\n-- \n2.8.0.rc4.21.g05df949\n"},{"id":"281857","messageId":"baa0ab35c01c536fc25dbc83402c116f8515d3e7.1458927521.git.k@rhe.jp","threadId":"41828","inReplyTo":"cover.1458927521.git.k@rhe.jp","subject":"[PATCH v2 2/5] refs: add REF_COMMON_DIR flag","fromName":"Kazuki Yamaguchi","fromEmail":"k@rhe.jp","sentAt":"2016-03-25T18:28:20Z","receivedAt":"2016-03-25T18:28:20Z","isPatch":true,"sender":{"key":"k@rhe.jp","avatar":"https://avatars.githubusercontent.com/u/1033740?v=4"},"body":"Add a flag to force using $GIT_COMMON_DIR, instead of selecting $GIT_DIR\nor $GIT_COMMON_DIR by refname.\n\nThis allows updating worktree-specific refs of the main working tree\nfrom a linked working tree. We will use this later.\n\nSigned-off-by: Kazuki Yamaguchi \n---\n refs.h | 4 ++++\n refs/files-backend.c | 12 ++++++++++--\n 2 files changed, 14 insertions(+), 2 deletions(-)\n\ndiff --git a/refs.h b/refs.h\nindex 5d9ab5c1c5dd..dc4782241e49 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -238,10 +238,14 @@ int pack_refs(unsigned int flags);\n * REF_NODEREF: act on the ref directly, instead of dereferencing\n * symbolic references.\n *\n+ * REF_COMMON_DIR: use $GIT_COMMON_DIR always. If not specified, $GIT_DIR or\n+ * $GIT_COMMON_DIR is used depending on refname.\n+ *\n * Other flags are reserved for internal use.\n */\n #define REF_NODEREF\t0x01\n #define REF_FORCE_CREATE_REFLOG 0x40\n+#define REF_COMMON_DIR 0x80\n \n /*\n * Setup reflog before using. Fill in err and return -1 on failure.\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex a534f1a1e078..2a808d520213 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1918,6 +1918,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t\tresolve_flags |= RESOLVE_REF_NO_RECURSE;\n \t\tlflags |= LOCK_NO_DEREF;\n \t}\n+\tif (flags & REF_COMMON_DIR)\n+\t\tresolve_flags |= RESOLVE_REF_COMMON_DIR;\n \n \trefname = resolve_ref_unsafe(refname, resolve_flags,\n \t\t\t\t lock->old_oid.hash, &type);\n@@ -1928,7 +1930,10 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t\t * it is normal for the empty directory 'foo'\n \t\t * to remain.\n \t\t */\n-\t\tstrbuf_git_path(&orig_ref_file, \"%s\", orig_refname);\n+\t\tif (flags & REF_COMMON_DIR)\n+\t\t\tstrbuf_addf(&orig_ref_file, \"%s/%s\", get_git_common_dir(), orig_refname);\n+\t\telse\n+\t\t\tstrbuf_git_path(&orig_ref_file, \"%s\", orig_refname);\n \t\tif (remove_empty_directories(&orig_ref_file)) {\n \t\t\tlast_errno = errno;\n \t\t\tif (!verify_refname_available_dir(orig_refname, extras, skip,\n@@ -1973,7 +1978,10 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \n \tlock->ref_name = xstrdup(refname);\n \tlock->orig_ref_name = xstrdup(orig_refname);\n-\tstrbuf_git_path(&ref_file, \"%s\", refname);\n+\tif (flags & REF_COMMON_DIR)\n+\t\tstrbuf_addf(&ref_file, \"%s/%s\", get_git_common_dir(), refname);\n+\telse\n+\t\tstrbuf_git_path(&ref_file, \"%s\", refname);\n \n retry:\n \tswitch (safe_create_leading_directories_const(ref_file.buf)) {\n-- \n2.8.0.rc4.21.g05df949\n"},{"id":"281855","messageId":"1d91378d1f1e994ccc69d5c6ef4f0d5332d37eec.1458927521.git.k@rhe.jp","threadId":"41828","inReplyTo":"cover.1458927521.git.k@rhe.jp","subject":"[PATCH v2 3/5] refs: add create_symref_common_dir as a variation of create_symref","fromName":"Kazuki Yamaguchi","fromEmail":"k@rhe.jp","sentAt":"2016-03-25T18:28:21Z","receivedAt":"2016-03-25T18:28:21Z","isPatch":true,"sender":{"key":"k@rhe.jp","avatar":"https://avatars.githubusercontent.com/u/1033740?v=4"},"body":"Add a new function create_symref_common_dir. This function passes\nREF_COMMON_DIR to lock_ref_sha1_basic, unlike create_symref, so to make\nit possible to update main working tree's per-worktree symbolic refs\n(HEAD) when we are in a linked working tree.\n\nAssume we have a linked working tree and we are in it. If we call\ncreate_symref(\"HEAD\", \"refs/heads/branch-a\", NULL), this updates the\nworking tree's HEAD, located at .git/worktrees/tree-a/HEAD, rather than\nthe main working tree's HEAD, .git/HEAD.\nThe new function create_symref_common_dir always updates the main\nworking tree's HEAD regardless of where we are.\n\nThis will be needed when renaming a branch.\n\nSigned-off-by: Kazuki Yamaguchi \n---\n refs.h | 3 +++\n refs/files-backend.c | 17 ++++++++++++++---\n 2 files changed, 17 insertions(+), 3 deletions(-)\n\ndiff --git a/refs.h b/refs.h\nindex dc4782241e49..ff8dbbe1a0e5 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -312,7 +312,10 @@ extern char *shorten_unambiguous_ref(const char *refname, int strict);\n /** rename ref, return 0 on success **/\n extern int rename_ref(const char *oldref, const char *newref, const char *logmsg);\n \n+/* create or update a symref */\n extern int create_symref(const char *refname, const char *target, const char *logmsg);\n+/* same as create_symref, but refname is always $GIT_COMMON_DIR/refname */\n+extern int create_symref_common_dir(const char *refname, const char *target, const char *logmsg);\n \n enum action_on_err {\n \tUPDATE_REFS_MSG_ON_ERR,\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 2a808d520213..1fe4d4e75188 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2886,14 +2886,15 @@ static int create_symref_locked(struct ref_lock *lock, const char *refname,\n \treturn 0;\n }\n \n-int create_symref(const char *refname, const char *target, const char *logmsg)\n+static int create_symref_internal(const char *refname, const char *target,\n+\t\t\t\t const char *logmsg, unsigned int flags)\n {\n \tstruct strbuf err = STRBUF_INIT;\n \tstruct ref_lock *lock;\n \tint ret;\n \n-\tlock = lock_ref_sha1_basic(refname, NULL, NULL, NULL, REF_NODEREF, NULL,\n-\t\t\t\t &err);\n+\tlock = lock_ref_sha1_basic(refname, NULL, NULL, NULL, REF_NODEREF | flags,\n+\t\t\t\t NULL, &err);\n \tif (!lock) {\n \t\terror(\"%s\", err.buf);\n \t\tstrbuf_release(&err);\n@@ -2905,6 +2906,16 @@ int create_symref(const char *refname, const char *target, const char *logmsg)\n \treturn ret;\n }\n \n+int create_symref(const char *refname, const char *target, const char *logmsg)\n+{\n+\treturn create_symref_internal(refname, target, logmsg, 0);\n+}\n+\n+int create_symref_common_dir(const char *refname, const char *target, const char *logmsg)\n+{\n+\treturn create_symref_internal(refname, target, logmsg, REF_COMMON_DIR);\n+}\n+\n int reflog_exists(const char *refname)\n {\n \tstruct stat st;\n-- \n2.8.0.rc4.21.g05df949\n"},{"id":"281858","messageId":"30620c02662c2eac34e50027326f190940b51fb8.1458927521.git.k@rhe.jp","threadId":"41828","inReplyTo":"cover.1458927521.git.k@rhe.jp","subject":"[PATCH v2 4/5] branch -m: update all per-worktree HEADs","fromName":"Kazuki Yamaguchi","fromEmail":"k@rhe.jp","sentAt":"2016-03-25T18:28:22Z","receivedAt":"2016-03-25T18:28:22Z","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 branch.c | 32 ++++++++++++++++++++++++++++++++\n branch.h | 7 +++++++\n builtin/branch.c | 3 +--\n t/t3200-branch.sh | 23 ++++++++++++++++++++++-\n 4 files changed, 62 insertions(+), 3 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex c50ea42172ce..df0928e32a70 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -344,3 +344,35 @@ void die_if_checked_out(const char *branch)\n \t\tdie(_(\"'%s' is already checked out at '%s'\"), branch, existing);\n \t}\n }\n+\n+int update_worktrees_head_symref(const char *oldref, const char *newref)\n+{\n+\tint ret = 0;\n+\tstruct strbuf symref = STRBUF_INIT;\n+\tstruct worktree **worktrees = get_worktrees();\n+\tint i;\n+\tint common_prefix_len = strlen(absolute_path(get_git_common_dir())) + 1;\n+\n+\tfor (i = 0; worktrees[i]; i++) {\n+\t\tif (worktrees[i]->is_detached)\n+\t\t\tcontinue;\n+\n+\t\tif (strcmp(oldref, worktrees[i]->head_ref))\n+\t\t\tcontinue;\n+\n+\t\tstrbuf_reset(&symref);\n+\t\tstrbuf_addf(&symref, \"%s/HEAD\", worktrees[i]->git_dir);\n+\t\tstrbuf_remove(&symref, 0, common_prefix_len);\n+\n+\t\tif (create_symref_common_dir(symref.buf, newref, NULL)) {\n+\t\t\tret = -1;\n+\t\t\terror(_(\"HEAD of working tree %s is not updated.\"),\n+\t\t\t worktrees[i]->path);\n+\t\t}\n+\t}\n+\n+\tstrbuf_release(&symref);\n+\tfree_worktrees(worktrees);\n+\n+\treturn ret;\n+}\ndiff --git a/branch.h b/branch.h\nindex 78ad4387cd32..3f5ae4b8866e 100644\n--- a/branch.h\n+++ b/branch.h\n@@ -60,4 +60,11 @@ extern int read_branch_desc(struct strbuf *, const char *branch_name);\n */\n extern void die_if_checked_out(const char *branch);\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\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 7b45b6bd6b80..31eb473d3e6a 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -552,8 +552,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 a89724849065..f7d438bd7d1d 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -126,7 +126,28 @@ 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 worktree add -b baz bazdir &&\n+\tgit worktree add -f bazdir2 baz &&\n+\tgit branch -M baz bam &&\n+\ttest $(git -C bazdir rev-parse --abbrev-ref HEAD) = bam &&\n+\ttest $(git -C bazdir2 rev-parse --abbrev-ref HEAD) = bam\n+'\n+\n+test_expect_success 'git branch -M baz bam should succeed within a worktree in which baz is checked out' '\n+\tgit checkout -b baz &&\n+\tgit worktree add -f bazdir3 baz &&\n+\t(\n+\t\tcd bazdir3 &&\n+\t\tgit branch -M baz bam &&\n+\t\ttest $(git rev-parse --abbrev-ref HEAD) = bam\n+\t) &&\n+\ttest $(git rev-parse --abbrev-ref HEAD) = bam\n '\n \n test_expect_success 'git branch -M master should work when master is checked out' '\n-- \n2.8.0.rc4.21.g05df949\n"},{"id":"281856","messageId":"cbc5116e5069f20545d66e12e082e0e17f4ecced.1458927521.git.k@rhe.jp","threadId":"41828","inReplyTo":"cover.1458927521.git.k@rhe.jp","subject":"[PATCH v2 5/5] branch -d: refuse deleting a branch which is currently checked out","fromName":"Kazuki Yamaguchi","fromEmail":"k@rhe.jp","sentAt":"2016-03-25T18:28:23Z","receivedAt":"2016-03-25T18:28:23Z","isPatch":true,"sender":{"key":"k@rhe.jp","avatar":"https://avatars.githubusercontent.com/u/1033740?v=4"},"body":"When a branch is checked out by current working tree, deleting the\nbranch is forbidden. However when the branch is checked out only by\nother working trees, deleting is allowed.\nUse find_shared_symref() to check if the branch is in use, not just\ncomparing with the current working tree's HEAD.\n\nSigned-off-by: Kazuki Yamaguchi \n---\n builtin/branch.c | 12 +++++++-----\n t/t3200-branch.sh | 6 ++++++\n 2 files changed, 13 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 31eb473d3e6a..e64aa68cf722 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@@ -215,16 +216,17 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \t\tint flags = 0;\n \n \t\tstrbuf_branchname(&bname, argv[i]);\n-\t\tif (kinds == FILTER_REFS_BRANCHES && !strcmp(head, bname.buf)) {\n+\t\tfree(name);\n+\t\tname = mkpathdup(fmt, bname.buf);\n+\n+\t\tif (kinds == FILTER_REFS_BRANCHES &&\n+\t\t find_shared_symref(\"HEAD\", name)) {\n \t\t\terror(_(\"Cannot delete the branch '%s' \"\n-\t\t\t \"which you are currently on.\"), bname.buf);\n+\t\t\t \"which is currently checked out.\"), bname.buf);\n \t\t\tret = 1;\n \t\t\tcontinue;\n \t\t}\n \n-\t\tfree(name);\n-\n-\t\tname = mkpathdup(fmt, bname.buf);\n \t\ttarget = resolve_ref_unsafe(name,\n \t\t\t\t\t RESOLVE_REF_READING\n \t\t\t\t\t | RESOLVE_REF_NO_RECURSE\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex f7d438bd7d1d..f3e3b6cf2eab 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -424,6 +424,12 @@ test_expect_success 'test deleting branch without config' '\n \ttest_i18ncmp expect actual\n '\n \n+test_expect_success 'deleting currently checked out branch fails' '\n+\tgit worktree add -b my7 my7 &&\n+\ttest_must_fail git -C my7 branch -d my7 &&\n+\ttest_must_fail git branch -d my7\n+'\n+\n test_expect_success 'test --track without .fetch entries' '\n \tgit branch --track my8 &&\n \ttest \"$(git config branch.my8.remote)\" &&\n-- \n2.8.0.rc4.21.g05df949\n"},{"id":"281876","messageId":"xmqq4mbu2ry5.fsf@gitster.mtv.corp.google.com","threadId":"41828","inReplyTo":"cbc5116e5069f20545d66e12e082e0e17f4ecced.1458927521.git.k@rhe.jp","subject":"Re: [PATCH v2 5/5] branch -d: refuse deleting a branch which is currently checked out","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-25T21:00:50Z","receivedAt":"2016-03-25T21:00:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kazuki Yamaguchi writes:\n\n> When a branch is checked out by current working tree, deleting the\n> branch is forbidden. However when the branch is checked out only by\n> other working trees, deleting is allowed.\n> Use find_shared_symref() to check if the branch is in use, not just\n> comparing with the current working tree's HEAD.\n>\n> Signed-off-by: Kazuki Yamaguchi \n> ---\n\nIt seems that applying this directly on top of 'maint' does fix the\nissue (and applying only part of the patch for t/ illustrates the\nexisting breakage), so let's make it a separate topic as you hinted\nin your cover letter.\n\nThanks.\n"},{"id":"281877","messageId":"xmqqzitm1ct8.fsf@gitster.mtv.corp.google.com","threadId":"41828","inReplyTo":"cover.1458927521.git.k@rhe.jp","subject":"Re: [PATCH v2 0/5] branch: fix branch operations with multiple working trees","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-25T21:13:07Z","receivedAt":"2016-03-25T21:13:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kazuki Yamaguchi writes:\n\n> [1/5]\n> Adds RESOLVE_REF_COMMON_DIR to resolve_ref_unsafe(). The second - fourth\n> patch depend on this. At the same time, this allows us to remove\n> reimplementation of resolve_ref_unsafe() in worktree.c: parse_ref().\n>\n> [2/5]\n> Adds REF_COMMON_DIR flag to lock_ref_sha1_basic().\n\nWhile the code reduction in 1/5 is a very good and welcome change,\nthese new flags make me wonder if they can be easily misused in a\nway that contradicts with what other parts of the system (e.g.\npath.c::common_list[]) tell us. Am I worried too much without a\ngood reason?\n\n> [3/5]\n> Adds create_symref_common_dir(). Same as create_symref() except it\n> doesn't consider $GIT_DIR. create_symref_common_dir(\"HEAD\", some) always\n> updates .git/HEAD. The next patch uses this.\n\nSimilarly, would this allow updating \"refs/remotes/origin/HEAD\"\n(which is also a symbolic ref) to different values for different\nworktrees? In other words, I am wondering if this new function\nshould be narrower in scope--e.g. used only to update \"HEAD\" and\nno other symbolic refs. Or will the additional flexibility be\nuseful?\n\n> [4/5]\n> Fixes the issue of git branch -m.\n> The behavior when one failed has changed from v1: print an error and\n> continue.\n>\n> % git branch -m oldname newname\n> error: Unable to create '/path/to/.git/worktrees/wt/HEAD.lock': Permission denied\n> error: HEAD of working tree /path/to/wt is not updated.\n> error: Unable to create '/path/to/.git/worktrees/wt2/HEAD.lock': Permission denied\n> error: HEAD of working tree /path/to/wt2 is not updated.\n> fatal: Branch renamed to newname, but HEAD is not updated!\n\nSounds like a good goal.\n"},{"id":"281922","messageId":"20160327072942.GA13750@chikuwa.rhe.jp","threadId":"41828","inReplyTo":"xmqqzitm1ct8.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 0/5] branch: fix branch operations with multiple working trees","fromName":"Kazuki Yamaguchi","fromEmail":"k@rhe.jp","sentAt":"2016-03-27T07:29:42Z","receivedAt":"2016-03-27T07:29:42Z","isPatch":true,"sender":{"key":"k@rhe.jp","avatar":"https://avatars.githubusercontent.com/u/1033740?v=4"},"body":"On Fri, Mar 25, 2016 at 02:13:07PM -0700, Junio C Hamano wrote:\n> Kazuki Yamaguchi writes:\n> \n> > [1/5]\n> > Adds RESOLVE_REF_COMMON_DIR to resolve_ref_unsafe(). The second - fourth\n> > patch depend on this. At the same time, this allows us to remove\n> > reimplementation of resolve_ref_unsafe() in worktree.c: parse_ref().\n> >\n> > [2/5]\n> > Adds REF_COMMON_DIR flag to lock_ref_sha1_basic().\n> \n> While the code reduction in 1/5 is a very good and welcome change,\n> these new flags make me wonder if they can be easily misused in a\n> way that contradicts with what other parts of the system (e.g.\n> path.c::common_list[]) tell us. Am I worried too much without a\n> good reason?\n> \n\nUmm...\nCertainly I can't come up with other use cases of there flags,\nespecially the latter one.\nI'll reconsider.\n\n> > [3/5]\n> > Adds create_symref_common_dir(). Same as create_symref() except it\n> > doesn't consider $GIT_DIR. create_symref_common_dir(\"HEAD\", some) always\n> > updates .git/HEAD. The next patch uses this.\n> \n> Similarly, would this allow updating \"refs/remotes/origin/HEAD\"\n> (which is also a symbolic ref) to different values for different\n> worktrees? In other words, I am wondering if this new function\n> should be narrower in scope--e.g. used only to update \"HEAD\" and\n> no other symbolic refs. Or will the additional flexibility be\n> useful?\n> \n\nFor non-per-worktree ref names, create_symref_common_dir() and\ncreate_symref() should work in the same way.\nBut yes, as far as I know, there is no other chance to update other\nworktree's per-worktree symrefs.\n\nHow about like this (will update per-worktree-gitdir/HEAD):\nset_worktree_head_symref(\"per-worktree-gitdir\", \"refs/heads/master\")\n\nThanks, \n\n> > [4/5]\n> > Fixes the issue of git branch -m.\n> > The behavior when one failed has changed from v1: print an error and\n> > continue.\n> >\n> > % git branch -m oldname newname\n> > error: Unable to create '/path/to/.git/worktrees/wt/HEAD.lock': Permission denied\n> > error: HEAD of working tree /path/to/wt is not updated.\n> > error: Unable to create '/path/to/.git/worktrees/wt2/HEAD.lock': Permission denied\n> > error: HEAD of working tree /path/to/wt2 is not updated.\n> > fatal: Branch renamed to newname, but HEAD is not updated!\n> \n> Sounds like a good goal.\n"},{"id":"281942","messageId":"CAPig+cSCC+OzotkTx89iS+t4DRd3F+QoHP4n-v_+rxXU2R+2LA@mail.gmail.com","threadId":"41828","inReplyTo":"cbc5116e5069f20545d66e12e082e0e17f4ecced.1458927521.git.k@rhe.jp","subject":"Re: [PATCH v2 5/5] branch -d: refuse deleting a branch which is currently checked out","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-27T17:52:18Z","receivedAt":"2016-03-27T17:52:18Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Mar 25, 2016 at 2:28 PM, Kazuki Yamaguchi wrote:\n> When a branch is checked out by current working tree, deleting the\n> branch is forbidden. However when the branch is checked out only by\n> other working trees, deleting is allowed.\n> Use find_shared_symref() to check if the branch is in use, not just\n> comparing with the current working tree's HEAD.\n>\n> Signed-off-by: Kazuki Yamaguchi \n> ---\n> diff --git a/builtin/branch.c b/builtin/branch.c\n> @@ -215,16 +216,17 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n> int flags = 0;\n>\n> strbuf_branchname(&bname, argv[i]);\n> - if (kinds == FILTER_REFS_BRANCHES && !strcmp(head, bname.buf)) {\n> + free(name);\n> + name = mkpathdup(fmt, bname.buf);\n> +\n> + if (kinds == FILTER_REFS_BRANCHES &&\n> + find_shared_symref(\"HEAD\", name)) {\n> error(_(\"Cannot delete the branch '%s' \"\n> - \"which you are currently on.\"), bname.buf);\n> + \"which is currently checked out.\"), bname.buf);\n\nWould it be possible to do a better job of letting the user know what\nwent wrong by stating in which worktree(s) the branch is checked out?\nMy concern is that someone seeing this message might respond \"huh? I\nhave 'master' checked out, so why is this telling me that 'foo' is\nchecked out\", and not realize that 'foo' is in fact checked out in a\ndifferent worktree.\n"},{"id":"281949","messageId":"20160328071614.GA27034@chikuwa.rhe.jp","threadId":"41828","inReplyTo":"CAPig+cSCC+OzotkTx89iS+t4DRd3F+QoHP4n-v_+rxXU2R+2LA@mail.gmail.com","subject":"Re: [PATCH v2 5/5] branch -d: refuse deleting a branch which is currently checked out","fromName":"Kazuki Yamaguchi","fromEmail":"k@rhe.jp","sentAt":"2016-03-28T07:16:14Z","receivedAt":"2016-03-28T07:16:14Z","isPatch":true,"sender":{"key":"k@rhe.jp","avatar":"https://avatars.githubusercontent.com/u/1033740?v=4"},"body":"On Sun, Mar 27, 2016 at 01:52:18PM -0400, Eric Sunshine wrote:\n> On Fri, Mar 25, 2016 at 2:28 PM, Kazuki Yamaguchi wrote:\n> > When a branch is checked out by current working tree, deleting the\n> > branch is forbidden. However when the branch is checked out only by\n> > other working trees, deleting is allowed.\n> > Use find_shared_symref() to check if the branch is in use, not just\n> > comparing with the current working tree's HEAD.\n> >\n> > Signed-off-by: Kazuki Yamaguchi \n> > ---\n> > diff --git a/builtin/branch.c b/builtin/branch.c\n> > @@ -215,16 +216,17 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n> > int flags = 0;\n> >\n> > strbuf_branchname(&bname, argv[i]);\n> > - if (kinds == FILTER_REFS_BRANCHES && !strcmp(head, bname.buf)) {\n> > + free(name);\n> > + name = mkpathdup(fmt, bname.buf);\n> > +\n> > + if (kinds == FILTER_REFS_BRANCHES &&\n> > + find_shared_symref(\"HEAD\", name)) {\n> > error(_(\"Cannot delete the branch '%s' \"\n> > - \"which you are currently on.\"), bname.buf);\n> > + \"which is currently checked out.\"), bname.buf);\n> \n> Would it be possible to do a better job of letting the user know what\n> went wrong by stating in which worktree(s) the branch is checked out?\n> My concern is that someone seeing this message might respond \"huh? I\n> have 'master' checked out, so why is this telling me that 'foo' is\n> checked out\", and not realize that 'foo' is in fact checked out in a\n> different worktree.\n\nYes, indeed. I'll do it. Thanks.\n"},{"id":"281950","messageId":"1459149771-14790-1-git-send-email-k@rhe.jp","threadId":"41828","inReplyTo":"cbc5116e5069f20545d66e12e082e0e17f4ecced.1458927521.git.k@rhe.jp","subject":"[PATCH v2] branch -d: refuse deleting a branch which is currently checked out","fromName":"Kazuki Yamaguchi","fromEmail":"k@rhe.jp","sentAt":"2016-03-28T07:22:51Z","receivedAt":"2016-03-28T07:22:51Z","isPatch":true,"sender":{"key":"k@rhe.jp","avatar":"https://avatars.githubusercontent.com/u/1033740?v=4"},"body":"When a branch is checked out by current working tree, deleting the\nbranch is forbidden. However when the branch is checked out only by\nother working trees, deleting is allowed.\nUse find_shared_symref() to check if the branch is in use, not just\ncomparing with the current working tree's HEAD.\n\nSigned-off-by: Kazuki Yamaguchi \n---\n\n % git worktree list\n /path/to 2c3c5f2 [master]\n /path/to/wt 2c3c5f2 [branch-a]\n % git branch -d branch-a\n error: Cannot delete the branch 'branch-a' which is currently checked out at '/path/to/wt'\n\n builtin/branch.c | 22 ++++++++++++++--------\n t/t3200-branch.sh | 6 ++++++\n 2 files changed, 20 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 7b45b6bd6b80..de1641306c9e 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@@ -215,16 +216,21 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \t\tint flags = 0;\n \n \t\tstrbuf_branchname(&bname, argv[i]);\n-\t\tif (kinds == FILTER_REFS_BRANCHES && !strcmp(head, bname.buf)) {\n-\t\t\terror(_(\"Cannot delete the branch '%s' \"\n-\t\t\t \"which you are currently on.\"), bname.buf);\n-\t\t\tret = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\n \t\tfree(name);\n-\n \t\tname = mkpathdup(fmt, bname.buf);\n+\n+\t\tif (kinds == FILTER_REFS_BRANCHES) {\n+\t\t\tchar *worktree = find_shared_symref(\"HEAD\", name);\n+\t\t\tif (worktree) {\n+\t\t\t\terror(_(\"Cannot delete the branch '%s' \"\n+\t\t\t\t\t\"which is currently checked out at '%s'\"),\n+\t\t\t\t bname.buf, worktree);\n+\t\t\t\tfree(worktree);\n+\t\t\t\tret = 1;\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\t\t}\n+\n \t\ttarget = resolve_ref_unsafe(name,\n \t\t\t\t\t RESOLVE_REF_READING\n \t\t\t\t\t | RESOLVE_REF_NO_RECURSE\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex a89724849065..508007fd3772 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -403,6 +403,12 @@ test_expect_success 'test deleting branch without config' '\n \ttest_i18ncmp expect actual\n '\n \n+test_expect_success 'deleting currently checked out branch fails' '\n+\tgit worktree add -b my7 my7 &&\n+\ttest_must_fail git -C my7 branch -d my7 &&\n+\ttest_must_fail git branch -d my7\n+'\n+\n test_expect_success 'test --track without .fetch entries' '\n \tgit branch --track my8 &&\n \ttest \"$(git config branch.my8.remote)\" &&\n-- \n2.8.0.rc4.17.gdb328b3\n"},{"id":"281970","messageId":"CAPig+cSzTwup6ojboVkP8nMR91-ZUU9FCbAK5NcrcohfFh2taQ@mail.gmail.com","threadId":"41828","inReplyTo":"1459149771-14790-1-git-send-email-k@rhe.jp","subject":"Re: [PATCH v2] branch -d: refuse deleting a branch which is currently checked out","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-28T16:51:21Z","receivedAt":"2016-03-28T16:51:21Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Mar 28, 2016 at 3:22 AM, Kazuki Yamaguchi wrote:\n> When a branch is checked out by current working tree, deleting the\n> branch is forbidden. However when the branch is checked out only by\n> other working trees, deleting is allowed.\n\nIt's not quite clear from this description that it is bad for deletion\nto succeed in the second case. Perhaps:\n\n s/deleting is allowed/deletion incorrectly succeeds/\n\nwould make it more clear.\n\n> Use find_shared_symref() to check if the branch is in use, not just\n> comparing with the current working tree's HEAD.\n\nThis version of the patch is nicer. Thanks. See a couple minor\ncomments below which may or may not be worth a re-roll (you decide).\n\n> Signed-off-by: Kazuki Yamaguchi \n> ---\n>\n> % git worktree list\n> /path/to 2c3c5f2 [master]\n> /path/to/wt 2c3c5f2 [branch-a]\n> % git branch -d branch-a\n> error: Cannot delete the branch 'branch-a' which is currently checked out at '/path/to/wt'\n\nThanks for an example of the new behavior. It's also helpful to\nreviewers if you use this space to explain what changed since the\nprevious version, and to provide a link to the previous attempt, like\nthis[1].\n\n[1]: http://thread.gmane.org/gmane.comp.version-control.git/289413/focus=289932\n\n> diff --git a/builtin/branch.c b/builtin/branch.c\n> @@ -215,16 +216,21 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n> int flags = 0;\n>\n> strbuf_branchname(&bname, argv[i]);\n> - if (kinds == FILTER_REFS_BRANCHES && !strcmp(head, bname.buf)) {\n> - error(_(\"Cannot delete the branch '%s' \"\n> - \"which you are currently on.\"), bname.buf);\n> - ret = 1;\n> - continue;\n> - }\n> -\n> free(name);\n> -\n> name = mkpathdup(fmt, bname.buf);\n> +\n> + if (kinds == FILTER_REFS_BRANCHES) {\n> + char *worktree = find_shared_symref(\"HEAD\", name);\n> + if (worktree) {\n> + error(_(\"Cannot delete the branch '%s' \"\n> + \"which is currently checked out at '%s'\"),\n\nThis could be stated more concisely as:\n\n \"Cannot delete branch '%s' checked out at '%s'\"\n\n> + bname.buf, worktree);\n> + free(worktree);\n\nWould it make sense to show all worktrees at which this branch is\nchecked out, rather than only one, or is that not worth the effort and\nextra code ugliness?\n\n> + ret = 1;\n> + continue;\n> + }\n> + }\n> +\n> target = resolve_ref_unsafe(name,\n> RESOLVE_REF_READING\n> | RESOLVE_REF_NO_RECURSE\n"},{"id":"282029","messageId":"20160329092802.GA17915@chikuwa.rhe.jp","threadId":"41828","inReplyTo":"CAPig+cSzTwup6ojboVkP8nMR91-ZUU9FCbAK5NcrcohfFh2taQ@mail.gmail.com","subject":"Re: [PATCH v2] branch -d: refuse deleting a branch which is currently checked out","fromName":"Kazuki Yamaguchi","fromEmail":"k@rhe.jp","sentAt":"2016-03-29T09:28:02Z","receivedAt":"2016-03-29T09:28:02Z","isPatch":true,"sender":{"key":"k@rhe.jp","avatar":"https://avatars.githubusercontent.com/u/1033740?v=4"},"body":"On Mon, Mar 28, 2016 at 12:51:21PM -0400, Eric Sunshine wrote:\n> On Mon, Mar 28, 2016 at 3:22 AM, Kazuki Yamaguchi wrote:\n> > When a branch is checked out by current working tree, deleting the\n> > branch is forbidden. However when the branch is checked out only by\n> > other working trees, deleting is allowed.\n> \n> It's not quite clear from this description that it is bad for deletion\n> to succeed in the second case. Perhaps:\n> \n> s/deleting is allowed/deletion incorrectly succeeds/\n> \n> would make it more clear.\n\nThanks.\n\n> \n> > Use find_shared_symref() to check if the branch is in use, not just\n> > comparing with the current working tree's HEAD.\n> \n> This version of the patch is nicer. Thanks. See a couple minor\n> comments below which may or may not be worth a re-roll (you decide).\n> \n> > Signed-off-by: Kazuki Yamaguchi \n> > ---\n> >\n> > % git worktree list\n> > /path/to 2c3c5f2 [master]\n> > /path/to/wt 2c3c5f2 [branch-a]\n> > % git branch -d branch-a\n> > error: Cannot delete the branch 'branch-a' which is currently checked out at '/path/to/wt'\n> \n> Thanks for an example of the new behavior. It's also helpful to\n> reviewers if you use this space to explain what changed since the\n> previous version, and to provide a link to the previous attempt, like\n> this[1].\n> \n> [1]: http://thread.gmane.org/gmane.comp.version-control.git/289413/focus=289932\n\nI'll do from next time.\n\n> \n> > diff --git a/builtin/branch.c b/builtin/branch.c\n> > @@ -215,16 +216,21 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n> > int flags = 0;\n> >\n> > strbuf_branchname(&bname, argv[i]);\n> > - if (kinds == FILTER_REFS_BRANCHES && !strcmp(head, bname.buf)) {\n> > - error(_(\"Cannot delete the branch '%s' \"\n> > - \"which you are currently on.\"), bname.buf);\n> > - ret = 1;\n> > - continue;\n> > - }\n> > -\n> > free(name);\n> > -\n> > name = mkpathdup(fmt, bname.buf);\n> > +\n> > + if (kinds == FILTER_REFS_BRANCHES) {\n> > + char *worktree = find_shared_symref(\"HEAD\", name);\n> > + if (worktree) {\n> > + error(_(\"Cannot delete the branch '%s' \"\n> > + \"which is currently checked out at '%s'\"),\n> \n> This could be stated more concisely as:\n> \n> \"Cannot delete branch '%s' checked out at '%s'\"\n\nI'll use it. Thanks.\n\n> \n> > + bname.buf, worktree);\n> > + free(worktree);\n> \n> Would it make sense to show all worktrees at which this branch is\n> checked out, rather than only one, or is that not worth the effort and\n> extra code ugliness?\n\nI thought one is enough.\nI think the worktrees usually won't be more than one, considering\n\"git worktree add\" requires additional option to check out an already\nchecked out branch. Also, since the branch is not actually deleted at\nthat time, the user can safely retry after checking \"git worktree list\".\n\n\nThanks,\n\n> \n> > + ret = 1;\n> > + continue;\n> > + }\n> > + }\n> > +\n> > target = resolve_ref_unsafe(name,\n> > RESOLVE_REF_READING\n> > | RESOLVE_REF_NO_RECURSE\n"},{"id":"282033","messageId":"1459244319-21241-1-git-send-email-k@rhe.jp","threadId":"41828","inReplyTo":"cbc5116e5069f20545d66e12e082e0e17f4ecced.1458927521.git.k@rhe.jp","subject":"[PATCH v3] branch -d: refuse deleting a branch which is currently checked out","fromName":"Kazuki Yamaguchi","fromEmail":"k@rhe.jp","sentAt":"2016-03-29T09:38:39Z","receivedAt":"2016-03-29T09:38:39Z","isPatch":true,"sender":{"key":"k@rhe.jp","avatar":"https://avatars.githubusercontent.com/u/1033740?v=4"},"body":"When a branch is checked out by current working tree, deleting the\nbranch is forbidden. However when the branch is checked out only by\nother working trees, deleting incorrectly succeeds.\nUse find_shared_symref() to check if the branch is in use, not just\ncomparing with the current working tree's HEAD.\n\nHelped-by: Eric Sunshine \nSigned-off-by: Kazuki Yamaguchi \n---\nChanges from v2:\n- Amended commit message\n- Dropped \"which is\" from error message\n\nThe previous versions of the patch are:\n- [v1] http://thread.gmane.org/gmane.comp.version-control.git/289413/focus=289932\n- [v2] http://thread.gmane.org/gmane.comp.version-control.git/289413/focus=290027\n\n builtin/branch.c | 22 ++++++++++++++--------\n t/t3200-branch.sh | 6 ++++++\n 2 files changed, 20 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 7b45b6bd6b80..8885d9f8e2cd 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@@ -215,16 +216,21 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \t\tint flags = 0;\n \n \t\tstrbuf_branchname(&bname, argv[i]);\n-\t\tif (kinds == FILTER_REFS_BRANCHES && !strcmp(head, bname.buf)) {\n-\t\t\terror(_(\"Cannot delete the branch '%s' \"\n-\t\t\t \"which you are currently on.\"), bname.buf);\n-\t\t\tret = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\n \t\tfree(name);\n-\n \t\tname = mkpathdup(fmt, bname.buf);\n+\n+\t\tif (kinds == FILTER_REFS_BRANCHES) {\n+\t\t\tchar *worktree = find_shared_symref(\"HEAD\", name);\n+\t\t\tif (worktree) {\n+\t\t\t\terror(_(\"Cannot delete branch '%s' \"\n+\t\t\t\t\t\"checked out at '%s'\"),\n+\t\t\t\t bname.buf, worktree);\n+\t\t\t\tfree(worktree);\n+\t\t\t\tret = 1;\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\t\t}\n+\n \t\ttarget = resolve_ref_unsafe(name,\n \t\t\t\t\t RESOLVE_REF_READING\n \t\t\t\t\t | RESOLVE_REF_NO_RECURSE\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex a89724849065..508007fd3772 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -403,6 +403,12 @@ test_expect_success 'test deleting branch without config' '\n \ttest_i18ncmp expect actual\n '\n \n+test_expect_success 'deleting currently checked out branch fails' '\n+\tgit worktree add -b my7 my7 &&\n+\ttest_must_fail git -C my7 branch -d my7 &&\n+\ttest_must_fail git branch -d my7\n+'\n+\n test_expect_success 'test --track without .fetch entries' '\n \tgit branch --track my8 &&\n \ttest \"$(git config branch.my8.remote)\" &&\n-- \n2.8.0.rc4.17.g02aa164.dirty\n"},{"id":"282082","messageId":"CAPig+cRd8DzW+SaX6vwa7cv8pgMZKQL_dzWy8mDOYn7QkCtM8g@mail.gmail.com","threadId":"41828","inReplyTo":"20160329092802.GA17915@chikuwa.rhe.jp","subject":"Re: [PATCH v2] branch -d: refuse deleting a branch which is currently checked out","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-29T18:47:10Z","receivedAt":"2016-03-29T18:47:10Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Mar 29, 2016 at 5:28 AM, Kazuki Yamaguchi wrote:\n> On Mon, Mar 28, 2016 at 12:51:21PM -0400, Eric Sunshine wrote:\n>> On Mon, Mar 28, 2016 at 3:22 AM, Kazuki Yamaguchi wrote:\n>> > + if (kinds == FILTER_REFS_BRANCHES) {\n>> > + char *worktree = find_shared_symref(\"HEAD\", name);\n>> > + if (worktree) {\n>> > + error(_(\"Cannot delete the branch '%s' \"\n>> > + \"which is currently checked out at '%s'\"),\n>> > + bname.buf, worktree);\n>> > + free(worktree);\n>>\n>> Would it make sense to show all worktrees at which this branch is\n>> checked out, rather than only one, or is that not worth the effort and\n>> extra code ugliness?\n>\n> I thought one is enough.\n> I think the worktrees usually won't be more than one, considering\n> \"git worktree add\" requires additional option to check out an already\n> checked out branch. Also, since the branch is not actually deleted at\n> that time, the user can safely retry after checking \"git worktree list\".\n\nFair enough. A more thorough error message can be done a future\nenhancement if there is a need for it.\n"},{"id":"282083","messageId":"CAPig+cQPFzgEgdNUJSa9baUvc4BuJp8BHOLA88QJqW8N3hE8AQ@mail.gmail.com","threadId":"41828","inReplyTo":"1459244319-21241-1-git-send-email-k@rhe.jp","subject":"Re: [PATCH v3] branch -d: refuse deleting a branch which is currently checked out","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-29T18:57:14Z","receivedAt":"2016-03-29T18:57:14Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Mar 29, 2016 at 5:38 AM, Kazuki Yamaguchi wrote:\n> When a branch is checked out by current working tree, deleting the\n> branch is forbidden. However when the branch is checked out only by\n> other working trees, deleting incorrectly succeeds.\n> Use find_shared_symref() to check if the branch is in use, not just\n> comparing with the current working tree's HEAD.\n>\n> Helped-by: Eric Sunshine \n> Signed-off-by: Kazuki Yamaguchi \n> ---\n> Changes from v2:\n> - Amended commit message\n> - Dropped \"which is\" from error message\n\nThanks, this version addresses my previous review comments and is:\n\n Reviewed-by: Eric Sunshine \n\n> The previous versions of the patch are:\n> - [v1] http://thread.gmane.org/gmane.comp.version-control.git/289413/focus=289932\n> - [v2] http://thread.gmane.org/gmane.comp.version-control.git/289413/focus=290027\n>\n> builtin/branch.c | 22 ++++++++++++++--------\n> t/t3200-branch.sh | 6 ++++++\n> 2 files changed, 20 insertions(+), 8 deletions(-)\n>\n> diff --git a/builtin/branch.c b/builtin/branch.c\n> index 7b45b6bd6b80..8885d9f8e2cd 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> N_(\"git branch [] [-r | -a] [--merged | --no-merged]\"),\n> @@ -215,16 +216,21 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n> int flags = 0;\n>\n> strbuf_branchname(&bname, argv[i]);\n> - if (kinds == FILTER_REFS_BRANCHES && !strcmp(head, bname.buf)) {\n> - error(_(\"Cannot delete the branch '%s' \"\n> - \"which you are currently on.\"), bname.buf);\n> - ret = 1;\n> - continue;\n> - }\n> -\n> free(name);\n> -\n> name = mkpathdup(fmt, bname.buf);\n> +\n> + if (kinds == FILTER_REFS_BRANCHES) {\n> + char *worktree = find_shared_symref(\"HEAD\", name);\n> + if (worktree) {\n> + error(_(\"Cannot delete branch '%s' \"\n> + \"checked out at '%s'\"),\n> + bname.buf, worktree);\n> + free(worktree);\n> + ret = 1;\n> + continue;\n> + }\n> + }\n> +\n> target = resolve_ref_unsafe(name,\n> RESOLVE_REF_READING\n> | RESOLVE_REF_NO_RECURSE\n> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\n> index a89724849065..508007fd3772 100755\n> --- a/t/t3200-branch.sh\n> +++ b/t/t3200-branch.sh\n> @@ -403,6 +403,12 @@ test_expect_success 'test deleting branch without config' '\n> test_i18ncmp expect actual\n> '\n>\n> +test_expect_success 'deleting currently checked out branch fails' '\n> + git worktree add -b my7 my7 &&\n> + test_must_fail git -C my7 branch -d my7 &&\n> + test_must_fail git branch -d my7\n> +'\n> +\n> test_expect_success 'test --track without .fetch entries' '\n> git branch --track my8 &&\n> test \"$(git config branch.my8.remote)\" &&\n> --\n> 2.8.0.rc4.17.g02aa164.dirty\n"}]}