threads / patch / 41828

v2, 5 partsrefs: add new flag RESOLVE_REF_COMMON_DIR to resolve_ref_unsafe

Subject: [PATCH v2 1/5] refs: add new flag RESOLVE_REF_COMMON_DIR to resolve_ref_unsafe

## tl;dr

17 messages between Mar 25, 2016 and Mar 29, 2016. Diffs are folded; open one to read it.

replies: 16people: 3as markdown or json

Kazuki Yamaguchi· Mar 25, 2016, 18:28 UTC · lore

[PATCH v2 0/5] branch: fix branch operations with multiple working trees

[1/5] Adds RESOLVE_REF_COMMON_DIR to resolve_ref_unsafe(). The second - fourth patch depend on this. At the same time, this allows us to remove reimplementation of resolve_ref_unsafe() in worktree.c: parse_ref().

[2/5] Adds REF_COMMON_DIR flag to lock_ref_sha1_basic().

[3/5] Adds create_symref_common_dir(). Same as create_symref() except it doesn't consider $GIT_DIR. create_symref_common_dir("HEAD", some) always updates .git/HEAD. The next patch uses this.

[4/5] Fixes the issue of git branch -m. The behavior when one failed has changed from v1: print an error and continue.

  % git branch -m oldname newname
  error: Unable to create '/path/to/.git/worktrees/wt/HEAD.lock': Permission denied
  error: HEAD of working tree /path/to/wt is not updated.
  error: Unable to create '/path/to/.git/worktrees/wt2/HEAD.lock': Permission denied
  error: HEAD of working tree /path/to/wt2 is not updated.
  fatal: Branch renamed to newname, but HEAD is not updated!

[5/5] Fixes an issue of git branch -d, v1 didn't include this. I noticed git branch -d has same issue and this is for it. This patch is unrelated to the above 4 patches, but the cause is same. This can be applied separately.

Kazuki Yamaguchi (5):
  refs: add new flag RESOLVE_REF_COMMON_DIR to resolve_ref_unsafe
  refs: add REF_COMMON_DIR flag
  refs: add create_symref_common_dir as a variation of create_symref
  branch -m: update all per-worktree HEADs
  branch -d: refuse deleting a branch which is currently checked out
 branch.c             |  32 ++++++++++++++++
 branch.h             |   7 ++++
 builtin/branch.c     |  15 ++++----
 refs.h               |  11 ++++++
 refs/files-backend.c |  34 ++++++++++++++---
 t/t3200-branch.sh    |  29 +++++++++++++-
 worktree.c           | 105 ++++++++++++++++-----------------------------------
 7 files changed, 147 insertions(+), 86 deletions(-)
-- 
2.8.0.rc4.21.g05df949
Kazuki Yamaguchi· Mar 25, 2016, 18:28 UTC · re: Kazuki Yamaguchi · lore

If the new flag RESOLVE_REF_COMMON_DIR is passed to resolve_ref_unsafe, it assumes the refname belongs to $GIT_COMMON_DIR.

resolve_ref_unsafe currently has no way to resolve worktree-specific refs such as HEAD of the main working tree when we are in a linked working tree. worktree.c has a simplified one for this purpose, and this patch allows removing it.

Signed-off-by: Kazuki Yamaguchi <k@rhe.jp>
---
 refs.h               |   4 ++
 refs/files-backend.c |   5 ++-
 worktree.c           | 105 ++++++++++++++++-----------------------------------
 3 files changed, 41 insertions(+), 73 deletions(-)
Show changes to 3 files +41 −73

refs.h, refs/files-backend.c, worktree.c

diff --git a/refs.h b/refs.h
index 2f3decb432cf..5d9ab5c1c5dd 100644
--- a/refs.h
+++ b/refs.h
@@ -25,6 +25,9 @@
  * reference will always be null_sha1 in this case, and the return
  * value is the reference that the symref refers to directly.
  *
+ * If the RESOLVE_REF_COMMON_DIR flag is passed, assumes the refname is
+ * always directly under $GIT_COMMON_DIR.
+ *
  * If flags is non-NULL, set the value that it points to the
  * combination of REF_ISPACKED (if the reference was found among the
  * packed references), REF_ISSYMREF (if the initial reference was a
@@ -51,6 +54,7 @@
 #define RESOLVE_REF_READING 0x01
 #define RESOLVE_REF_NO_RECURSE 0x02
 #define RESOLVE_REF_ALLOW_BAD_NAME 0x04
+#define RESOLVE_REF_COMMON_DIR 0x08
 
 extern const char *resolve_ref_unsafe(const char *refname, int resolve_flags,
 				      unsigned char *sha1, int *flags);
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 81f68f846b69..a534f1a1e078 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1448,7 +1448,10 @@ static const char *resolve_ref_1(const char *refname,
 		}
 
 		strbuf_reset(sb_path);
-		strbuf_git_path(sb_path, "%s", refname);
+		if (resolve_flags & RESOLVE_REF_COMMON_DIR)
+			strbuf_addf(sb_path, "%s/%s", get_git_common_dir(), refname);
+		else
+			strbuf_git_path(sb_path, "%s", refname);
 		path = sb_path->buf;
 
 		/*
diff --git a/worktree.c b/worktree.c
index 6181a66f1ee2..e9e945ea1373 100644
--- a/worktree.c
+++ b/worktree.c
@@ -16,53 +16,19 @@ void free_worktrees(struct worktree **worktrees)
 	free (worktrees);
 }
 
-/*
- * read 'path_to_ref' into 'ref'.  Also if is_detached is not NULL,
- * set is_detached to 1 (0) if the ref is detatched (is not detached).
- *
- * $GIT_COMMON_DIR/$symref (e.g. HEAD) is practically outside $GIT_DIR so
- * for linked worktrees, `resolve_ref_unsafe()` won't work (it uses
- * git_path). Parse the ref ourselves.
- *
- * return -1 if the ref is not a proper ref, 0 otherwise (success)
- */
-static int parse_ref(char *path_to_ref, struct strbuf *ref, int *is_detached)
-{
-	if (is_detached)
-		*is_detached = 0;
-	if (!strbuf_readlink(ref, path_to_ref, 0)) {
-		/* HEAD is symbolic link */
-		if (!starts_with(ref->buf, "refs/") ||
-				check_refname_format(ref->buf, 0))
-			return -1;
-	} else if (strbuf_read_file(ref, path_to_ref, 0) >= 0) {
-		/* textual symref or detached */
-		if (!starts_with(ref->buf, "ref:")) {
-			if (is_detached)
-				*is_detached = 1;
-		} else {
-			strbuf_remove(ref, 0, strlen("ref:"));
-			strbuf_trim(ref);
-			if (check_refname_format(ref->buf, 0))
-				return -1;
-		}
-	} else
-		return -1;
-	return 0;
-}
-
 /**
- * Add the head_sha1 and head_ref (if not detached) to the given worktree
+ * Add the is_detached, head_sha1 and head_ref (if not detached) to the given worktree
  */
-static void add_head_info(struct strbuf *head_ref, struct worktree *worktree)
+static void add_head_info(const char *head_ref, const unsigned char *sha1,
+			  struct worktree *worktree)
 {
-	if (head_ref->len) {
-		if (worktree->is_detached) {
-			get_sha1_hex(head_ref->buf, worktree->head_sha1);
-		} else {
-			resolve_ref_unsafe(head_ref->buf, 0, worktree->head_sha1, NULL);
-			worktree->head_ref = strbuf_detach(head_ref, NULL);
-		}
+	worktree->is_detached = !is_null_sha1(sha1);
+	if (worktree->is_detached) {
+		hashcpy(worktree->head_sha1, sha1);
+		worktree->head_ref = NULL;
+	} else {
+		resolve_ref_unsafe(head_ref, 0, worktree->head_sha1, NULL);
+		worktree->head_ref = xstrdup(head_ref);
 	}
 }
 
@@ -72,12 +38,11 @@ static void add_head_info(struct strbuf *head_ref, struct worktree *worktree)
 static struct worktree *get_main_worktree(void)
 {
 	struct worktree *worktree = NULL;
-	struct strbuf path = STRBUF_INIT;
 	struct strbuf worktree_path = STRBUF_INIT;
 	struct strbuf gitdir = STRBUF_INIT;
-	struct strbuf head_ref = STRBUF_INIT;
+	const char *head_ref;
+	unsigned char sha1[20];
 	int is_bare = 0;
-	int is_detached = 0;
 
 	strbuf_addf(&gitdir, "%s", absolute_path(get_git_common_dir()));
 	strbuf_addbuf(&worktree_path, &gitdir);
@@ -85,24 +50,21 @@ static struct worktree *get_main_worktree(void)
 	if (is_bare)
 		strbuf_strip_suffix(&worktree_path, "/.");
 
-	strbuf_addf(&path, "%s/HEAD", get_git_common_dir());
-
-	if (parse_ref(path.buf, &head_ref, &is_detached) < 0)
+	head_ref = resolve_ref_unsafe("HEAD",
+			RESOLVE_REF_NO_RECURSE | RESOLVE_REF_COMMON_DIR,
+			sha1, NULL);
+	if (!head_ref)
 		goto done;
 
 	worktree = xmalloc(sizeof(struct worktree));
 	worktree->path = strbuf_detach(&worktree_path, NULL);
 	worktree->git_dir = strbuf_detach(&gitdir, NULL);
 	worktree->is_bare = is_bare;
-	worktree->head_ref = NULL;
-	worktree->is_detached = is_detached;
-	add_head_info(&head_ref, worktree);
+	add_head_info(head_ref, sha1, worktree);
 
 done:
-	strbuf_release(&path);
 	strbuf_release(&gitdir);
 	strbuf_release(&worktree_path);
-	strbuf_release(&head_ref);
 	return worktree;
 }
 
@@ -112,8 +74,8 @@ static struct worktree *get_linked_worktree(const char *id)
 	struct strbuf path = STRBUF_INIT;
 	struct strbuf worktree_path = STRBUF_INIT;
 	struct strbuf gitdir = STRBUF_INIT;
-	struct strbuf head_ref = STRBUF_INIT;
-	int is_detached = 0;
+	const char *head_ref;
+	unsigned char sha1[20];
 
 	if (!id)
 		die("Missing linked worktree name");
@@ -133,24 +95,23 @@ static struct worktree *get_linked_worktree(const char *id)
 	}
 
 	strbuf_reset(&path);
-	strbuf_addf(&path, "%s/worktrees/%s/HEAD", get_git_common_dir(), id);
-
-	if (parse_ref(path.buf, &head_ref, &is_detached) < 0)
+	strbuf_addf(&path, "worktrees/%s/HEAD", id);
+	head_ref = resolve_ref_unsafe(path.buf,
+			RESOLVE_REF_NO_RECURSE | RESOLVE_REF_COMMON_DIR,
+			sha1, NULL);
+	if (!head_ref)
 		goto done;
 
 	worktree = xmalloc(sizeof(struct worktree));
 	worktree->path = strbuf_detach(&worktree_path, NULL);
 	worktree->git_dir = strbuf_detach(&gitdir, NULL);
 	worktree->is_bare = 0;
-	worktree->head_ref = NULL;
-	worktree->is_detached = is_detached;
-	add_head_info(&head_ref, worktree);
+	add_head_info(head_ref, sha1, worktree);
 
 done:
 	strbuf_release(&path);
 	strbuf_release(&gitdir);
 	strbuf_release(&worktree_path);
-	strbuf_release(&head_ref);
 	return worktree;
 }
 
@@ -192,27 +153,27 @@ char *find_shared_symref(const char *symref, const char *target)
 {
 	char *existing = NULL;
 	struct strbuf path = STRBUF_INIT;
-	struct strbuf sb = STRBUF_INIT;
 	struct worktree **worktrees = get_worktrees();
+	const char *resolved;
 	int i = 0;
+	unsigned char sha1[20];
+	int common_prefix_len = strlen(absolute_path(get_git_common_dir())) + 1;
 
 	for (i = 0; worktrees[i]; i++) {
 		strbuf_reset(&path);
-		strbuf_reset(&sb);
 		strbuf_addf(&path, "%s/%s", worktrees[i]->git_dir, symref);
+		strbuf_remove(&path, 0, common_prefix_len);
 
-		if (parse_ref(path.buf, &sb, NULL)) {
-			continue;
-		}
-
-		if (!strcmp(sb.buf, target)) {
+		resolved = resolve_ref_unsafe(path.buf,
+				RESOLVE_REF_NO_RECURSE | RESOLVE_REF_COMMON_DIR,
+				sha1, NULL);
+		if (resolved && !strcmp(resolved, target)) {
 			existing = xstrdup(worktrees[i]->path);
 			break;
 		}
 	}
 
 	strbuf_release(&path);
-	strbuf_release(&sb);
 	free_worktrees(worktrees);
 
 	return existing;
-- 
2.8.0.rc4.21.g05df949
Kazuki Yamaguchi· Mar 25, 2016, 18:28 UTC · re: Kazuki Yamaguchi · lore

[PATCH v2 2/5] refs: add REF_COMMON_DIR flag

Add a flag to force using $GIT_COMMON_DIR, instead of selecting $GIT_DIR or $GIT_COMMON_DIR by refname.

This allows updating worktree-specific refs of the main working tree from a linked working tree. We will use this later.

Signed-off-by: Kazuki Yamaguchi <k@rhe.jp>
---
 refs.h               |  4 ++++
 refs/files-backend.c | 12 ++++++++++--
 2 files changed, 14 insertions(+), 2 deletions(-)
Show changes to 2 files +14 −2

refs.h, refs/files-backend.c

diff --git a/refs.h b/refs.h
index 5d9ab5c1c5dd..dc4782241e49 100644
--- a/refs.h
+++ b/refs.h
@@ -238,10 +238,14 @@ int pack_refs(unsigned int flags);
  * REF_NODEREF: act on the ref directly, instead of dereferencing
  *              symbolic references.
  *
+ * REF_COMMON_DIR: use $GIT_COMMON_DIR always. If not specified, $GIT_DIR or
+ *                 $GIT_COMMON_DIR is used depending on refname.
+ *
  * Other flags are reserved for internal use.
  */
 #define REF_NODEREF	0x01
 #define REF_FORCE_CREATE_REFLOG 0x40
+#define REF_COMMON_DIR 0x80
 
 /*
  * Setup reflog before using. Fill in err and return -1 on failure.
diff --git a/refs/files-backend.c b/refs/files-backend.c
index a534f1a1e078..2a808d520213 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1918,6 +1918,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 		resolve_flags |= RESOLVE_REF_NO_RECURSE;
 		lflags |= LOCK_NO_DEREF;
 	}
+	if (flags & REF_COMMON_DIR)
+		resolve_flags |= RESOLVE_REF_COMMON_DIR;
 
 	refname = resolve_ref_unsafe(refname, resolve_flags,
 				     lock->old_oid.hash, &type);
@@ -1928,7 +1930,10 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 		 * it is normal for the empty directory 'foo'
 		 * to remain.
 		 */
-		strbuf_git_path(&orig_ref_file, "%s", orig_refname);
+		if (flags & REF_COMMON_DIR)
+			strbuf_addf(&orig_ref_file, "%s/%s", get_git_common_dir(), orig_refname);
+		else
+			strbuf_git_path(&orig_ref_file, "%s", orig_refname);
 		if (remove_empty_directories(&orig_ref_file)) {
 			last_errno = errno;
 			if (!verify_refname_available_dir(orig_refname, extras, skip,
@@ -1973,7 +1978,10 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 
 	lock->ref_name = xstrdup(refname);
 	lock->orig_ref_name = xstrdup(orig_refname);
-	strbuf_git_path(&ref_file, "%s", refname);
+	if (flags & REF_COMMON_DIR)
+		strbuf_addf(&ref_file, "%s/%s", get_git_common_dir(), refname);
+	else
+		strbuf_git_path(&ref_file, "%s", refname);
 
  retry:
 	switch (safe_create_leading_directories_const(ref_file.buf)) {
-- 
2.8.0.rc4.21.g05df949
Kazuki Yamaguchi· Mar 25, 2016, 18:28 UTC · re: Kazuki Yamaguchi · lore

[PATCH v2 3/5] refs: add create_symref_common_dir as a variation of create_symref

Add a new function create_symref_common_dir. This function passes REF_COMMON_DIR to lock_ref_sha1_basic, unlike create_symref, so to make it possible to update main working tree's per-worktree symbolic refs (HEAD) when we are in a linked working tree.

Assume we have a linked working tree and we are in it. If we call create_symref("HEAD", "refs/heads/branch-a", NULL), this updates the working tree's HEAD, located at .git/worktrees/tree-a/HEAD, rather than the main working tree's HEAD, .git/HEAD. The new function create_symref_common_dir always updates the main working tree's HEAD regardless of where we are.

This will be needed when renaming a branch.
Signed-off-by: Kazuki Yamaguchi <k@rhe.jp>
---
 refs.h               |  3 +++
 refs/files-backend.c | 17 ++++++++++++++---
 2 files changed, 17 insertions(+), 3 deletions(-)
Show changes to 2 files +17 −3

refs.h, refs/files-backend.c

diff --git a/refs.h b/refs.h
index dc4782241e49..ff8dbbe1a0e5 100644
--- a/refs.h
+++ b/refs.h
@@ -312,7 +312,10 @@ extern char *shorten_unambiguous_ref(const char *refname, int strict);
 /** rename ref, return 0 on success **/
 extern int rename_ref(const char *oldref, const char *newref, const char *logmsg);
 
+/* create or update a symref */
 extern int create_symref(const char *refname, const char *target, const char *logmsg);
+/* same as create_symref, but refname is always $GIT_COMMON_DIR/refname */
+extern int create_symref_common_dir(const char *refname, const char *target, const char *logmsg);
 
 enum action_on_err {
 	UPDATE_REFS_MSG_ON_ERR,
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 2a808d520213..1fe4d4e75188 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2886,14 +2886,15 @@ static int create_symref_locked(struct ref_lock *lock, const char *refname,
 	return 0;
 }
 
-int create_symref(const char *refname, const char *target, const char *logmsg)
+static int create_symref_internal(const char *refname, const char *target,
+				  const char *logmsg, unsigned int flags)
 {
 	struct strbuf err = STRBUF_INIT;
 	struct ref_lock *lock;
 	int ret;
 
-	lock = lock_ref_sha1_basic(refname, NULL, NULL, NULL, REF_NODEREF, NULL,
-				   &err);
+	lock = lock_ref_sha1_basic(refname, NULL, NULL, NULL, REF_NODEREF | flags,
+				   NULL, &err);
 	if (!lock) {
 		error("%s", err.buf);
 		strbuf_release(&err);
@@ -2905,6 +2906,16 @@ int create_symref(const char *refname, const char *target, const char *logmsg)
 	return ret;
 }
 
+int create_symref(const char *refname, const char *target, const char *logmsg)
+{
+	return create_symref_internal(refname, target, logmsg, 0);
+}
+
+int create_symref_common_dir(const char *refname, const char *target, const char *logmsg)
+{
+	return create_symref_internal(refname, target, logmsg, REF_COMMON_DIR);
+}
+
 int reflog_exists(const char *refname)
 {
 	struct stat st;
-- 
2.8.0.rc4.21.g05df949
Kazuki Yamaguchi· Mar 25, 2016, 18:28 UTC · re: Kazuki Yamaguchi · lore

[PATCH v2 4/5] branch -m: update all per-worktree HEADs

When renaming a branch, the current code only updates the current working tree's HEAD, but it should update .git/HEAD of all checked out working trees.

This is the current behavior, /path/to/wt's HEAD is not updated:
  % git worktree list
  /path/to     2c3c5f2 [master]
  /path/to/wt  2c3c5f2 [oldname]
  % git branch -m master master2
  % git worktree list
  /path/to     2c3c5f2 [master2]
  /path/to/wt  2c3c5f2 [oldname]
  % git branch -m oldname newname
  % git worktree list
  /path/to     2c3c5f2 [master2]
  /path/to/wt  0000000 [oldname]

This patch fixes this issue by updating all relevant worktree HEADs when renaming a branch.

Signed-off-by: Kazuki Yamaguchi <k@rhe.jp>
---
 branch.c          | 32 ++++++++++++++++++++++++++++++++
 branch.h          |  7 +++++++
 builtin/branch.c  |  3 +--
 t/t3200-branch.sh | 23 ++++++++++++++++++++++-
 4 files changed, 62 insertions(+), 3 deletions(-)
Show changes to 4 files +62 −3

branch.c, branch.h, builtin/branch.c, t/t3200-branch.sh

diff --git a/branch.c b/branch.c
index c50ea42172ce..df0928e32a70 100644
--- a/branch.c
+++ b/branch.c
@@ -344,3 +344,35 @@ void die_if_checked_out(const char *branch)
 		die(_("'%s' is already checked out at '%s'"), branch, existing);
 	}
 }
+
+int update_worktrees_head_symref(const char *oldref, const char *newref)
+{
+	int ret = 0;
+	struct strbuf symref = STRBUF_INIT;
+	struct worktree **worktrees = get_worktrees();
+	int i;
+	int common_prefix_len = strlen(absolute_path(get_git_common_dir())) + 1;
+
+	for (i = 0; worktrees[i]; i++) {
+		if (worktrees[i]->is_detached)
+			continue;
+
+		if (strcmp(oldref, worktrees[i]->head_ref))
+			continue;
+
+		strbuf_reset(&symref);
+		strbuf_addf(&symref, "%s/HEAD", worktrees[i]->git_dir);
+		strbuf_remove(&symref, 0, common_prefix_len);
+
+		if (create_symref_common_dir(symref.buf, newref, NULL)) {
+			ret = -1;
+			error(_("HEAD of working tree %s is not updated."),
+			      worktrees[i]->path);
+		}
+	}
+
+	strbuf_release(&symref);
+	free_worktrees(worktrees);
+
+	return ret;
+}
diff --git a/branch.h b/branch.h
index 78ad4387cd32..3f5ae4b8866e 100644
--- a/branch.h
+++ b/branch.h
@@ -60,4 +60,11 @@ extern int read_branch_desc(struct strbuf *, const char *branch_name);
  */
 extern void die_if_checked_out(const char *branch);
 
+/*
+ * Update all per-worktree HEADs pointing the old ref to point the new ref.
+ * This will be used when renaming a branch. Returns 0 if successful,
+ * non-zero otherwise.
+ */
+extern int update_worktrees_head_symref(const char *, const char *);
+
 #endif
diff --git a/builtin/branch.c b/builtin/branch.c
index 7b45b6bd6b80..31eb473d3e6a 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -552,8 +552,7 @@ static void rename_branch(const char *oldname, const char *newname, int force)
 	if (recovery)
 		warning(_("Renamed a misnamed branch '%s' away"), oldref.buf + 11);
 
-	/* no need to pass logmsg here as HEAD didn't really move */
-	if (!strcmp(oldname, head) && create_symref("HEAD", newref.buf, NULL))
+	if (update_worktrees_head_symref(oldref.buf, newref.buf))
 		die(_("Branch renamed to %s, but HEAD is not updated!"), newname);
 
 	strbuf_addf(&oldsection, "branch.%s", oldref.buf + 11);
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index a89724849065..f7d438bd7d1d 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -126,7 +126,28 @@ test_expect_success 'git branch -M foo bar should fail when bar is checked out'
 test_expect_success 'git branch -M baz bam should succeed when baz is checked out' '
 	git checkout -b baz &&
 	git branch bam &&
-	git branch -M baz bam
+	git branch -M baz bam &&
+	test $(git rev-parse --abbrev-ref HEAD) = bam
+'
+
+test_expect_success 'git branch -M baz bam should succeed when baz is checked out as linked working tree' '
+	git checkout master &&
+	git worktree add -b baz bazdir &&
+	git worktree add -f bazdir2 baz &&
+	git branch -M baz bam &&
+	test $(git -C bazdir rev-parse --abbrev-ref HEAD) = bam &&
+	test $(git -C bazdir2 rev-parse --abbrev-ref HEAD) = bam
+'
+
+test_expect_success 'git branch -M baz bam should succeed within a worktree in which baz is checked out' '
+	git checkout -b baz &&
+	git worktree add -f bazdir3 baz &&
+	(
+		cd bazdir3 &&
+		git branch -M baz bam &&
+		test $(git rev-parse --abbrev-ref HEAD) = bam
+	) &&
+	test $(git rev-parse --abbrev-ref HEAD) = bam
 '
 
 test_expect_success 'git branch -M master should work when master is checked out' '
-- 
2.8.0.rc4.21.g05df949
Kazuki Yamaguchi· Mar 25, 2016, 18:28 UTC · re: Kazuki Yamaguchi · lore

[PATCH v2 5/5] branch -d: refuse deleting a branch which is currently checked out

When a branch is checked out by current working tree, deleting the branch is forbidden. However when the branch is checked out only by other working trees, deleting is allowed. Use find_shared_symref() to check if the branch is in use, not just comparing with the current working tree's HEAD.

Signed-off-by: Kazuki Yamaguchi <k@rhe.jp>
---
 builtin/branch.c  | 12 +++++++-----
 t/t3200-branch.sh |  6 ++++++
 2 files changed, 13 insertions(+), 5 deletions(-)
Show changes to 2 files +13 −5

builtin/branch.c, t/t3200-branch.sh

diff --git a/builtin/branch.c b/builtin/branch.c
index 31eb473d3e6a..e64aa68cf722 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -20,6 +20,7 @@
 #include "utf8.h"
 #include "wt-status.h"
 #include "ref-filter.h"
+#include "worktree.h"
 
 static const char * const builtin_branch_usage[] = {
 	N_("git branch [<options>] [-r | -a] [--merged | --no-merged]"),
@@ -215,16 +216,17 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 		int flags = 0;
 
 		strbuf_branchname(&bname, argv[i]);
-		if (kinds == FILTER_REFS_BRANCHES && !strcmp(head, bname.buf)) {
+		free(name);
+		name = mkpathdup(fmt, bname.buf);
+
+		if (kinds == FILTER_REFS_BRANCHES &&
+		    find_shared_symref("HEAD", name)) {
 			error(_("Cannot delete the branch '%s' "
-			      "which you are currently on."), bname.buf);
+			      "which is currently checked out."), bname.buf);
 			ret = 1;
 			continue;
 		}
 
-		free(name);
-
-		name = mkpathdup(fmt, bname.buf);
 		target = resolve_ref_unsafe(name,
 					    RESOLVE_REF_READING
 					    | RESOLVE_REF_NO_RECURSE
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index f7d438bd7d1d..f3e3b6cf2eab 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -424,6 +424,12 @@ test_expect_success 'test deleting branch without config' '
 	test_i18ncmp expect actual
 '
 
+test_expect_success 'deleting currently checked out branch fails' '
+	git worktree add -b my7 my7 &&
+	test_must_fail git -C my7 branch -d my7 &&
+	test_must_fail git branch -d my7
+'
+
 test_expect_success 'test --track without .fetch entries' '
 	git branch --track my8 &&
 	test "$(git config branch.my8.remote)" &&
-- 
2.8.0.rc4.21.g05df949
Junio C Hamano· Mar 25, 2016, 21:00 UTC · re: Kazuki Yamaguchi · lore

Re: [PATCH v2 5/5] branch -d: refuse deleting a branch which is currently checked out

Kazuki Yamaguchi <k@rhe.jp> writes:
Show 8 quoted lines
> When a branch is checked out by current working tree, deleting the
> branch is forbidden. However when the branch is checked out only by
> other working trees, deleting is allowed.
> Use find_shared_symref() to check if the branch is in use, not just
> comparing with the current working tree's HEAD.
>
> Signed-off-by: Kazuki Yamaguchi <k@rhe.jp>
> ---

It seems that applying this directly on top of 'maint' does fix the issue (and applying only part of the patch for t/ illustrates the existing breakage), so let's make it a separate topic as you hinted in your cover letter.

Thanks.
Eric Sunshine· Mar 27, 2016, 17:52 UTC · re: Kazuki Yamaguchi · lore

Re: [PATCH v2 5/5] branch -d: refuse deleting a branch which is currently checked out

On Fri, Mar 25, 2016 at 2:28 PM, Kazuki Yamaguchi <k@rhe.jp> wrote:
Show 22 quoted lines
> When a branch is checked out by current working tree, deleting the
> branch is forbidden. However when the branch is checked out only by
> other working trees, deleting is allowed.
> Use find_shared_symref() to check if the branch is in use, not just
> comparing with the current working tree's HEAD.
>
> Signed-off-by: Kazuki Yamaguchi <k@rhe.jp>
> ---
> diff --git a/builtin/branch.c b/builtin/branch.c
> @@ -215,16 +216,17 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
>                 int flags = 0;
>
>                 strbuf_branchname(&bname, argv[i]);
> -               if (kinds == FILTER_REFS_BRANCHES && !strcmp(head, bname.buf)) {
> +               free(name);
> +               name = mkpathdup(fmt, bname.buf);
> +
> +               if (kinds == FILTER_REFS_BRANCHES &&
> +                   find_shared_symref("HEAD", name)) {
>                         error(_("Cannot delete the branch '%s' "
> -                             "which you are currently on."), bname.buf);
> +                             "which is currently checked out."), bname.buf);

Would it be possible to do a better job of letting the user know what went wrong by stating in which worktree(s) the branch is checked out? My concern is that someone seeing this message might respond "huh? I have 'master' checked out, so why is this telling me that 'foo' is checked out", and not realize that 'foo' is in fact checked out in a different worktree.

Kazuki Yamaguchi· Mar 28, 2016, 07:16 UTC · re: Eric Sunshine · lore

Re: [PATCH v2 5/5] branch -d: refuse deleting a branch which is currently checked out

On Sun, Mar 27, 2016 at 01:52:18PM -0400, Eric Sunshine wrote:
Show 30 quoted lines
> On Fri, Mar 25, 2016 at 2:28 PM, Kazuki Yamaguchi <k@rhe.jp> wrote:
> > When a branch is checked out by current working tree, deleting the
> > branch is forbidden. However when the branch is checked out only by
> > other working trees, deleting is allowed.
> > Use find_shared_symref() to check if the branch is in use, not just
> > comparing with the current working tree's HEAD.
> >
> > Signed-off-by: Kazuki Yamaguchi <k@rhe.jp>
> > ---
> > diff --git a/builtin/branch.c b/builtin/branch.c
> > @@ -215,16 +216,17 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
> >                 int flags = 0;
> >
> >                 strbuf_branchname(&bname, argv[i]);
> > -               if (kinds == FILTER_REFS_BRANCHES && !strcmp(head, bname.buf)) {
> > +               free(name);
> > +               name = mkpathdup(fmt, bname.buf);
> > +
> > +               if (kinds == FILTER_REFS_BRANCHES &&
> > +                   find_shared_symref("HEAD", name)) {
> >                         error(_("Cannot delete the branch '%s' "
> > -                             "which you are currently on."), bname.buf);
> > +                             "which is currently checked out."), bname.buf);
> 
> Would it be possible to do a better job of letting the user know what
> went wrong by stating in which worktree(s) the branch is checked out?
> My concern is that someone seeing this message might respond "huh? I
> have 'master' checked out, so why is this telling me that 'foo' is
> checked out", and not realize that 'foo' is in fact checked out in a
> different worktree.
Yes, indeed. I'll do it. Thanks.
Kazuki Yamaguchi· Mar 28, 2016, 07:22 UTC · re: Kazuki Yamaguchi · lore

[PATCH v2] branch -d: refuse deleting a branch which is currently checked out

When a branch is checked out by current working tree, deleting the branch is forbidden. However when the branch is checked out only by other working trees, deleting is allowed. Use find_shared_symref() to check if the branch is in use, not just comparing with the current working tree's HEAD.

Signed-off-by: Kazuki Yamaguchi <k@rhe.jp>
---
  % git worktree list
  /path/to      2c3c5f2 [master]
  /path/to/wt   2c3c5f2 [branch-a]
  % git branch -d branch-a
  error: Cannot delete the branch 'branch-a' which is currently checked out at '/path/to/wt'
 builtin/branch.c  | 22 ++++++++++++++--------
 t/t3200-branch.sh |  6 ++++++
 2 files changed, 20 insertions(+), 8 deletions(-)
Show changes to 2 files +20 −8

builtin/branch.c, t/t3200-branch.sh

diff --git a/builtin/branch.c b/builtin/branch.c
index 7b45b6bd6b80..de1641306c9e 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -20,6 +20,7 @@
 #include "utf8.h"
 #include "wt-status.h"
 #include "ref-filter.h"
+#include "worktree.h"
 
 static const char * const builtin_branch_usage[] = {
 	N_("git branch [<options>] [-r | -a] [--merged | --no-merged]"),
@@ -215,16 +216,21 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 		int flags = 0;
 
 		strbuf_branchname(&bname, argv[i]);
-		if (kinds == FILTER_REFS_BRANCHES && !strcmp(head, bname.buf)) {
-			error(_("Cannot delete the branch '%s' "
-			      "which you are currently on."), bname.buf);
-			ret = 1;
-			continue;
-		}
-
 		free(name);
-
 		name = mkpathdup(fmt, bname.buf);
+
+		if (kinds == FILTER_REFS_BRANCHES) {
+			char *worktree = find_shared_symref("HEAD", name);
+			if (worktree) {
+				error(_("Cannot delete the branch '%s' "
+					"which is currently checked out at '%s'"),
+				      bname.buf, worktree);
+				free(worktree);
+				ret = 1;
+				continue;
+			}
+		}
+
 		target = resolve_ref_unsafe(name,
 					    RESOLVE_REF_READING
 					    | RESOLVE_REF_NO_RECURSE
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index a89724849065..508007fd3772 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -403,6 +403,12 @@ test_expect_success 'test deleting branch without config' '
 	test_i18ncmp expect actual
 '
 
+test_expect_success 'deleting currently checked out branch fails' '
+	git worktree add -b my7 my7 &&
+	test_must_fail git -C my7 branch -d my7 &&
+	test_must_fail git branch -d my7
+'
+
 test_expect_success 'test --track without .fetch entries' '
 	git branch --track my8 &&
 	test "$(git config branch.my8.remote)" &&
-- 
2.8.0.rc4.17.gdb328b3
Eric Sunshine· Mar 28, 2016, 16:51 UTC · re: Kazuki Yamaguchi · lore

Re: [PATCH v2] branch -d: refuse deleting a branch which is currently checked out

On Mon, Mar 28, 2016 at 3:22 AM, Kazuki Yamaguchi <k@rhe.jp> wrote:
> When a branch is checked out by current working tree, deleting the
> branch is forbidden. However when the branch is checked out only by
> other working trees, deleting is allowed.

It's not quite clear from this description that it is bad for deletion to succeed in the second case. Perhaps:

    s/deleting is allowed/deletion incorrectly succeeds/
would make it more clear.
> Use find_shared_symref() to check if the branch is in use, not just
> comparing with the current working tree's HEAD.

This version of the patch is nicer. Thanks. See a couple minor comments below which may or may not be worth a re-roll (you decide).

Show 8 quoted lines
> Signed-off-by: Kazuki Yamaguchi <k@rhe.jp>
> ---
>
>   % git worktree list
>   /path/to      2c3c5f2 [master]
>   /path/to/wt   2c3c5f2 [branch-a]
>   % git branch -d branch-a
>   error: Cannot delete the branch 'branch-a' which is currently checked out at '/path/to/wt'

Thanks for an example of the new behavior. It's also helpful to reviewers if you use this space to explain what changed since the previous version, and to provide a link to the previous attempt, like this[1].

[1]: http://thread.gmane.org/gmane.comp.version-control.git/289413/focus=289932
Show 21 quoted lines
> diff --git a/builtin/branch.c b/builtin/branch.c
> @@ -215,16 +216,21 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
>                 int flags = 0;
>
>                 strbuf_branchname(&bname, argv[i]);
> -               if (kinds == FILTER_REFS_BRANCHES && !strcmp(head, bname.buf)) {
> -                       error(_("Cannot delete the branch '%s' "
> -                             "which you are currently on."), bname.buf);
> -                       ret = 1;
> -                       continue;
> -               }
> -
>                 free(name);
> -
>                 name = mkpathdup(fmt, bname.buf);
> +
> +               if (kinds == FILTER_REFS_BRANCHES) {
> +                       char *worktree = find_shared_symref("HEAD", name);
> +                       if (worktree) {
> +                               error(_("Cannot delete the branch '%s' "
> +                                       "which is currently checked out at '%s'"),
This could be stated more concisely as:
    "Cannot delete branch '%s' checked out at '%s'"
> +                                     bname.buf, worktree);
> +                               free(worktree);

Would it make sense to show all worktrees at which this branch is checked out, rather than only one, or is that not worth the effort and extra code ugliness?

Show 8 quoted lines
> +                               ret = 1;
> +                               continue;
> +                       }
> +               }
> +
>                 target = resolve_ref_unsafe(name,
>                                             RESOLVE_REF_READING
>                                             | RESOLVE_REF_NO_RECURSE
Kazuki Yamaguchi· Mar 29, 2016, 09:28 UTC · re: Eric Sunshine · lore

Re: [PATCH v2] branch -d: refuse deleting a branch which is currently checked out

On Mon, Mar 28, 2016 at 12:51:21PM -0400, Eric Sunshine wrote:
Show 11 quoted lines
> On Mon, Mar 28, 2016 at 3:22 AM, Kazuki Yamaguchi <k@rhe.jp> wrote:
> > When a branch is checked out by current working tree, deleting the
> > branch is forbidden. However when the branch is checked out only by
> > other working trees, deleting is allowed.
> 
> It's not quite clear from this description that it is bad for deletion
> to succeed in the second case. Perhaps:
> 
>     s/deleting is allowed/deletion incorrectly succeeds/
> 
> would make it more clear.
Thanks.
Show 22 quoted lines
> 
> > Use find_shared_symref() to check if the branch is in use, not just
> > comparing with the current working tree's HEAD.
> 
> This version of the patch is nicer. Thanks. See a couple minor
> comments below which may or may not be worth a re-roll (you decide).
> 
> > Signed-off-by: Kazuki Yamaguchi <k@rhe.jp>
> > ---
> >
> >   % git worktree list
> >   /path/to      2c3c5f2 [master]
> >   /path/to/wt   2c3c5f2 [branch-a]
> >   % git branch -d branch-a
> >   error: Cannot delete the branch 'branch-a' which is currently checked out at '/path/to/wt'
> 
> Thanks for an example of the new behavior. It's also helpful to
> reviewers if you use this space to explain what changed since the
> previous version, and to provide a link to the previous attempt, like
> this[1].
> 
> [1]: http://thread.gmane.org/gmane.comp.version-control.git/289413/focus=289932
I'll do from next time.
Show 26 quoted lines
> 
> > diff --git a/builtin/branch.c b/builtin/branch.c
> > @@ -215,16 +216,21 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
> >                 int flags = 0;
> >
> >                 strbuf_branchname(&bname, argv[i]);
> > -               if (kinds == FILTER_REFS_BRANCHES && !strcmp(head, bname.buf)) {
> > -                       error(_("Cannot delete the branch '%s' "
> > -                             "which you are currently on."), bname.buf);
> > -                       ret = 1;
> > -                       continue;
> > -               }
> > -
> >                 free(name);
> > -
> >                 name = mkpathdup(fmt, bname.buf);
> > +
> > +               if (kinds == FILTER_REFS_BRANCHES) {
> > +                       char *worktree = find_shared_symref("HEAD", name);
> > +                       if (worktree) {
> > +                               error(_("Cannot delete the branch '%s' "
> > +                                       "which is currently checked out at '%s'"),
> 
> This could be stated more concisely as:
> 
>     "Cannot delete branch '%s' checked out at '%s'"
I'll use it. Thanks.
Show 7 quoted lines
> 
> > +                                     bname.buf, worktree);
> > +                               free(worktree);
> 
> Would it make sense to show all worktrees at which this branch is
> checked out, rather than only one, or is that not worth the effort and
> extra code ugliness?

I thought one is enough. I think the worktrees usually won't be more than one, considering "git worktree add" requires additional option to check out an already checked out branch. Also, since the branch is not actually deleted at that time, the user can safely retry after checking "git worktree list".

Thanks,
Show 9 quoted lines
> 
> > +                               ret = 1;
> > +                               continue;
> > +                       }
> > +               }
> > +
> >                 target = resolve_ref_unsafe(name,
> >                                             RESOLVE_REF_READING
> >                                             | RESOLVE_REF_NO_RECURSE
Eric Sunshine· Mar 29, 2016, 18:47 UTC · re: Kazuki Yamaguchi · lore

Re: [PATCH v2] branch -d: refuse deleting a branch which is currently checked out

On Tue, Mar 29, 2016 at 5:28 AM, Kazuki Yamaguchi <k@rhe.jp> wrote:
Show 19 quoted lines
> On Mon, Mar 28, 2016 at 12:51:21PM -0400, Eric Sunshine wrote:
>> On Mon, Mar 28, 2016 at 3:22 AM, Kazuki Yamaguchi <k@rhe.jp> wrote:
>> > +               if (kinds == FILTER_REFS_BRANCHES) {
>> > +                       char *worktree = find_shared_symref("HEAD", name);
>> > +                       if (worktree) {
>> > +                               error(_("Cannot delete the branch '%s' "
>> > +                                       "which is currently checked out at '%s'"),
>> > +                                     bname.buf, worktree);
>> > +                               free(worktree);
>>
>> Would it make sense to show all worktrees at which this branch is
>> checked out, rather than only one, or is that not worth the effort and
>> extra code ugliness?
>
> I thought one is enough.
> I think the worktrees usually won't be more than one, considering
> "git worktree add" requires additional option to check out an already
> checked out branch. Also, since the branch is not actually deleted at
> that time, the user can safely retry after checking "git worktree list".

Fair enough. A more thorough error message can be done a future enhancement if there is a need for it.

Kazuki Yamaguchi· Mar 29, 2016, 09:38 UTC · re: Kazuki Yamaguchi · lore

[PATCH v3] branch -d: refuse deleting a branch which is currently checked out

When a branch is checked out by current working tree, deleting the branch is forbidden. However when the branch is checked out only by other working trees, deleting incorrectly succeeds. Use find_shared_symref() to check if the branch is in use, not just comparing with the current working tree's HEAD.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Kazuki Yamaguchi <k@rhe.jp>
---
Changes from v2:
- Amended commit message
- Dropped "which is" from error message
The previous versions of the patch are:
- [v1] http://thread.gmane.org/gmane.comp.version-control.git/289413/focus=289932
- [v2] http://thread.gmane.org/gmane.comp.version-control.git/289413/focus=290027
 builtin/branch.c  | 22 ++++++++++++++--------
 t/t3200-branch.sh |  6 ++++++
 2 files changed, 20 insertions(+), 8 deletions(-)
Show changes to 2 files +20 −8

builtin/branch.c, t/t3200-branch.sh

diff --git a/builtin/branch.c b/builtin/branch.c
index 7b45b6bd6b80..8885d9f8e2cd 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -20,6 +20,7 @@
 #include "utf8.h"
 #include "wt-status.h"
 #include "ref-filter.h"
+#include "worktree.h"
 
 static const char * const builtin_branch_usage[] = {
 	N_("git branch [<options>] [-r | -a] [--merged | --no-merged]"),
@@ -215,16 +216,21 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 		int flags = 0;
 
 		strbuf_branchname(&bname, argv[i]);
-		if (kinds == FILTER_REFS_BRANCHES && !strcmp(head, bname.buf)) {
-			error(_("Cannot delete the branch '%s' "
-			      "which you are currently on."), bname.buf);
-			ret = 1;
-			continue;
-		}
-
 		free(name);
-
 		name = mkpathdup(fmt, bname.buf);
+
+		if (kinds == FILTER_REFS_BRANCHES) {
+			char *worktree = find_shared_symref("HEAD", name);
+			if (worktree) {
+				error(_("Cannot delete branch '%s' "
+					"checked out at '%s'"),
+				      bname.buf, worktree);
+				free(worktree);
+				ret = 1;
+				continue;
+			}
+		}
+
 		target = resolve_ref_unsafe(name,
 					    RESOLVE_REF_READING
 					    | RESOLVE_REF_NO_RECURSE
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index a89724849065..508007fd3772 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -403,6 +403,12 @@ test_expect_success 'test deleting branch without config' '
 	test_i18ncmp expect actual
 '
 
+test_expect_success 'deleting currently checked out branch fails' '
+	git worktree add -b my7 my7 &&
+	test_must_fail git -C my7 branch -d my7 &&
+	test_must_fail git branch -d my7
+'
+
 test_expect_success 'test --track without .fetch entries' '
 	git branch --track my8 &&
 	test "$(git config branch.my8.remote)" &&
-- 
2.8.0.rc4.17.g02aa164.dirty
Eric Sunshine· Mar 29, 2016, 18:57 UTC · re: Kazuki Yamaguchi · lore

Re: [PATCH v3] branch -d: refuse deleting a branch which is currently checked out

On Tue, Mar 29, 2016 at 5:38 AM, Kazuki Yamaguchi <k@rhe.jp> wrote:
Show 12 quoted lines
> When a branch is checked out by current working tree, deleting the
> branch is forbidden. However when the branch is checked out only by
> other working trees, deleting incorrectly succeeds.
> Use find_shared_symref() to check if the branch is in use, not just
> comparing with the current working tree's HEAD.
>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Kazuki Yamaguchi <k@rhe.jp>
> ---
> Changes from v2:
> - Amended commit message
> - Dropped "which is" from error message
Thanks, this version addresses my previous review comments and is:
    Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>
Show 69 quoted lines
> The previous versions of the patch are:
> - [v1] http://thread.gmane.org/gmane.comp.version-control.git/289413/focus=289932
> - [v2] http://thread.gmane.org/gmane.comp.version-control.git/289413/focus=290027
>
>  builtin/branch.c  | 22 ++++++++++++++--------
>  t/t3200-branch.sh |  6 ++++++
>  2 files changed, 20 insertions(+), 8 deletions(-)
>
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 7b45b6bd6b80..8885d9f8e2cd 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -20,6 +20,7 @@
>  #include "utf8.h"
>  #include "wt-status.h"
>  #include "ref-filter.h"
> +#include "worktree.h"
>
>  static const char * const builtin_branch_usage[] = {
>         N_("git branch [<options>] [-r | -a] [--merged | --no-merged]"),
> @@ -215,16 +216,21 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
>                 int flags = 0;
>
>                 strbuf_branchname(&bname, argv[i]);
> -               if (kinds == FILTER_REFS_BRANCHES && !strcmp(head, bname.buf)) {
> -                       error(_("Cannot delete the branch '%s' "
> -                             "which you are currently on."), bname.buf);
> -                       ret = 1;
> -                       continue;
> -               }
> -
>                 free(name);
> -
>                 name = mkpathdup(fmt, bname.buf);
> +
> +               if (kinds == FILTER_REFS_BRANCHES) {
> +                       char *worktree = find_shared_symref("HEAD", name);
> +                       if (worktree) {
> +                               error(_("Cannot delete branch '%s' "
> +                                       "checked out at '%s'"),
> +                                     bname.buf, worktree);
> +                               free(worktree);
> +                               ret = 1;
> +                               continue;
> +                       }
> +               }
> +
>                 target = resolve_ref_unsafe(name,
>                                             RESOLVE_REF_READING
>                                             | RESOLVE_REF_NO_RECURSE
> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> index a89724849065..508007fd3772 100755
> --- a/t/t3200-branch.sh
> +++ b/t/t3200-branch.sh
> @@ -403,6 +403,12 @@ test_expect_success 'test deleting branch without config' '
>         test_i18ncmp expect actual
>  '
>
> +test_expect_success 'deleting currently checked out branch fails' '
> +       git worktree add -b my7 my7 &&
> +       test_must_fail git -C my7 branch -d my7 &&
> +       test_must_fail git branch -d my7
> +'
> +
>  test_expect_success 'test --track without .fetch entries' '
>         git branch --track my8 &&
>         test "$(git config branch.my8.remote)" &&
> --
> 2.8.0.rc4.17.g02aa164.dirty
Junio C Hamano· Mar 25, 2016, 21:13 UTC · re: Kazuki Yamaguchi · lore

Re: [PATCH v2 0/5] branch: fix branch operations with multiple working trees

Kazuki Yamaguchi <k@rhe.jp> writes:
Show 7 quoted lines
> [1/5]
> Adds RESOLVE_REF_COMMON_DIR to resolve_ref_unsafe(). The second - fourth
> patch depend on this. At the same time, this allows us to remove
> reimplementation of resolve_ref_unsafe() in worktree.c: parse_ref().
>
> [2/5]
> Adds REF_COMMON_DIR flag to lock_ref_sha1_basic().

While the code reduction in 1/5 is a very good and welcome change, these new flags make me wonder if they can be easily misused in a way that contradicts with what other parts of the system (e.g. path.c::common_list[]) tell us. Am I worried too much without a good reason?

> [3/5]
> Adds create_symref_common_dir(). Same as create_symref() except it
> doesn't consider $GIT_DIR. create_symref_common_dir("HEAD", some) always
> updates .git/HEAD. The next patch uses this.

Similarly, would this allow updating "refs/remotes/origin/HEAD" (which is also a symbolic ref) to different values for different worktrees? In other words, I am wondering if this new function should be narrower in scope--e.g. used only to update "HEAD" and no other symbolic refs. Or will the additional flexibility be useful?

Show 11 quoted lines
> [4/5]
> Fixes the issue of git branch -m.
> The behavior when one failed has changed from v1: print an error and
> continue.
>
>   % git branch -m oldname newname
>   error: Unable to create '/path/to/.git/worktrees/wt/HEAD.lock': Permission denied
>   error: HEAD of working tree /path/to/wt is not updated.
>   error: Unable to create '/path/to/.git/worktrees/wt2/HEAD.lock': Permission denied
>   error: HEAD of working tree /path/to/wt2 is not updated.
>   fatal: Branch renamed to newname, but HEAD is not updated!
Sounds like a good goal.
Kazuki Yamaguchi· Mar 27, 2016, 07:29 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 0/5] branch: fix branch operations with multiple working trees

On Fri, Mar 25, 2016 at 02:13:07PM -0700, Junio C Hamano wrote:
Show 16 quoted lines
> Kazuki Yamaguchi <k@rhe.jp> writes:
> 
> > [1/5]
> > Adds RESOLVE_REF_COMMON_DIR to resolve_ref_unsafe(). The second - fourth
> > patch depend on this. At the same time, this allows us to remove
> > reimplementation of resolve_ref_unsafe() in worktree.c: parse_ref().
> >
> > [2/5]
> > Adds REF_COMMON_DIR flag to lock_ref_sha1_basic().
> 
> While the code reduction in 1/5 is a very good and welcome change,
> these new flags make me wonder if they can be easily misused in a
> way that contradicts with what other parts of the system (e.g.
> path.c::common_list[]) tell us.  Am I worried too much without a
> good reason?
> 

Umm... Certainly I can't come up with other use cases of there flags, especially the latter one. I'll reconsider.

Show 12 quoted lines
> > [3/5]
> > Adds create_symref_common_dir(). Same as create_symref() except it
> > doesn't consider $GIT_DIR. create_symref_common_dir("HEAD", some) always
> > updates .git/HEAD. The next patch uses this.
> 
> Similarly, would this allow updating "refs/remotes/origin/HEAD"
> (which is also a symbolic ref) to different values for different
> worktrees?  In other words, I am wondering if this new function
> should be narrower in scope--e.g. used only to update "HEAD" and
> no other symbolic refs.  Or will the additional flexibility be
> useful?
> 

For non-per-worktree ref names, create_symref_common_dir() and create_symref() should work in the same way. But yes, as far as I know, there is no other chance to update other worktree's per-worktree symrefs.

How about like this (will update per-worktree-gitdir/HEAD): set_worktree_head_symref("per-worktree-gitdir", "refs/heads/master")

Thanks, 
Show 13 quoted lines
> > [4/5]
> > Fixes the issue of git branch -m.
> > The behavior when one failed has changed from v1: print an error and
> > continue.
> >
> >   % git branch -m oldname newname
> >   error: Unable to create '/path/to/.git/worktrees/wt/HEAD.lock': Permission denied
> >   error: HEAD of working tree /path/to/wt is not updated.
> >   error: Unable to create '/path/to/.git/worktrees/wt2/HEAD.lock': Permission denied
> >   error: HEAD of working tree /path/to/wt2 is not updated.
> >   fatal: Branch renamed to newname, but HEAD is not updated!
> 
> Sounds like a good goal.

← back to recent threads