[PATCH] branch: update all per-worktree HEADs when renaming a branch

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 <redacted>
---
 builtin/branch.c  |  4 ++--
 t/t3200-branch.sh | 14 +++++++++++++-
 worktree.c        | 38 ++++++++++++++++++++++++++++++++++++++
 worktree.h        |  7 +++++++
 4 files changed, 60 insertions(+), 3 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 7b45b6b..596fb5f 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]"),

@@ -552,8 +553,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 a897248..da107d0 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh

@@ -126,7 +126,19 @@ 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 branch &&
+	git worktree add -b baz bazdir &&
+	git branch -M baz bam &&
+	(
+		cd bazdir &&
+		test $(git rev-parse --abbrev-ref HEAD) = bam
+	)
 '
 
 test_expect_success 'git branch -M master should work when master is checked out' '

diff --git a/worktree.c b/worktree.c
index 6181a66..9e7d0f3 100644
--- a/worktree.c
+++ b/worktree.c

@@ -217,3 +217,41 @@ char *find_shared_symref(const char *symref, const char *target)
 
 	return existing;
 }
+
+int update_worktrees_head_symref(const char *oldref, const char *newref)
+{
+	int error = 0;
+	struct strbuf path = STRBUF_INIT;
+	struct strbuf origref = STRBUF_INIT;
+	int i;
+	struct worktree **worktrees = get_worktrees();
+
+	for (i = 0; worktrees[i]; i++) {
+		if (worktrees[i]->is_detached)
+			continue;
+
+		strbuf_reset(&path);
+		strbuf_reset(&origref);
+		strbuf_addf(&path, "%s/HEAD", worktrees[i]->git_dir);
+
+		if (parse_ref(path.buf, &origref, NULL))
+			continue;
+
+		if (!strcmp(origref.buf, oldref)) {
+			int prefix_len = strlen(absolute_path(get_git_common_dir())) + 1;
+			const char *symref = path.buf + prefix_len;
+
+			/* no need to pass logmsg here as HEAD didn't really move */
+			if (create_symref(symref, newref, NULL)) {
+				error = -1;
+				break;
+			}
+		}
+	}
+
+	strbuf_release(&path);
+	strbuf_release(&origref);
+	free_worktrees(worktrees);
+
+	return error;
+}

diff --git a/worktree.h b/worktree.h
index b4b3dda..0d15d11 100644
--- a/worktree.h
+++ b/worktree.h

@@ -35,4 +35,11 @@ extern void free_worktrees(struct worktree **);
  */
 extern char *find_shared_symref(const char *symref, const char *target);
 
+/*
+ * 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

-- 
2.8.0.rc3.13.gcd7ec22

On Mon, Mar 21, 2016 at 5:50 AM, Kazuki Yamaguchi [off-list ref] wrote:

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:
[...]
This patch fixes this issue by updating all relevant worktree HEADs
when renaming a branch.

Makes sense; seems like a genuine problem. Some comment below...

This can be done more easily without the subshell:

    test $(git -C bazdir rev-parse ...) = bam

Can you also expand the test so that it verifies that the rename works
as expected when the branch is checked out in multiple worktrees,
rather than just one. Likewise, it would be nice to check branch
rename from within a worktree in which the branch is checked out (in
addition to the test above which does the rename from outside such a
worktree).

More below...

Is aborting upon the first error desired behavior? (Genuine question.)
Would it make more sense to continue attempting the rename for the
remaining worktrees (and remember that an error was encountered)?
Related: Since you're now dealing with multiple worktrees, you can do
a better job of letting the user know in which worktree something went
wrong rather than merely emitting the relatively generic "Branch
renamed to %s, but HEAD is not updated!".

More below...

I guess I can understand the desire to libify this functionality,
however, it feels as if it is a feature of "branch" rather than
"worktree", hence perhaps it should reside in top-level branch.[hc]?

On Tue, Mar 22, 2016 at 12:41 AM, Eric Sunshine [off-list ref] wrote:

quoted

diff --git a/worktree.c b/worktree.c

@@ -217,3 +217,41 @@ char *find_shared_symref(const char *symref, const char *target)
+int update_worktrees_head_symref(const char *oldref, const char *newref)
+{
+       int error = 0;
+       struct strbuf path = STRBUF_INIT;
+       struct strbuf origref = STRBUF_INIT;
+       int i;
+       struct worktree **worktrees = get_worktrees();
+
+       for (i = 0; worktrees[i]; i++) {
+               if (worktrees[i]->is_detached)
+                       continue;
+
+               strbuf_reset(&path);
+               strbuf_reset(&origref);
+               strbuf_addf(&path, "%s/HEAD", worktrees[i]->git_dir);
+
+               if (parse_ref(path.buf, &origref, NULL))
+                       continue;
+
+               if (!strcmp(origref.buf, oldref)) {
+                       int prefix_len = strlen(absolute_path(get_git_common_dir())) + 1;
+                       const char *symref = path.buf + prefix_len;
+
+                       /* no need to pass logmsg here as HEAD didn't really move */
+                       if (create_symref(symref, newref, NULL)) {
+                               error = -1;
+                               break;

Is aborting upon the first error desired behavior? (Genuine question.)
Would it make more sense to continue attempting the rename for the
remaining worktrees (and remember that an error was encountered)?

Since all these HEADs stay at the same (or close) location, if one
fails, I think the rest will fail too. Which leads to a series of
warnings if we continue anyway. A more interesting approach is update
HEADs in a transaction, so we successfully update all or we update
none. But I do not know if ref transactions can be used for HEAD,
especially worktree HEADs. I'm ok with either abort here or continue
anyway, though.
-- 
Duy

Hello,

On 03/22/2016 03:41 AM, Eric Sunshine [off-list ref] wrote:

quoted

diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh

@@ -126,7 +126,19 @@ 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' '
+test_expect_success 'git branch -M baz bam should succeed when baz is checked out as linked working tree' '
+       git checkout master &&
+       git branch &&
+       git worktree add -b baz bazdir &&
+       git branch -M baz bam &&
+       (
+               cd bazdir &&
+               test $(git rev-parse --abbrev-ref HEAD) = bam
+       )
 '

This can be done more easily without the subshell:

    test $(git -C bazdir rev-parse ...) = bam

Thank you for reviewing. And sorry for late response.
I didn't know -C option, thanks.

Can you also expand the test so that it verifies that the rename works
as expected when the branch is checked out in multiple worktrees,
rather than just one. Likewise, it would be nice to check branch
rename from within a worktree in which the branch is checked out (in
addition to the test above which does the rename from outside such a
worktree).

I'll add them.
And I noticed my patch is broken in the latter case (rename in a linked
working tree).
Since create_symref() calls resolve_ref_unsafe() and it uses $GIT_DIR
for worktree-specific refs thus my patch fails to update main tree's
HEAD when we are in a linked working tree.
I'm thinking about adding new flag to resolve_ref_unsafe(), to force
using $GIT_COMMON_DIR. This will at the same time allows to remove
parse_ref() in worktree.c.

quoted

diff --git a/worktree.c b/worktree.c

@@ -217,3 +217,41 @@ char *find_shared_symref(const char *symref, const char *target)
+int update_worktrees_head_symref(const char *oldref, const char *newref)
+{
+       int error = 0;
+       struct strbuf path = STRBUF_INIT;
+       struct strbuf origref = STRBUF_INIT;
+       int i;
+       struct worktree **worktrees = get_worktrees();
+
+       for (i = 0; worktrees[i]; i++) {
+               if (worktrees[i]->is_detached)
+                       continue;
+
+               strbuf_reset(&path);
+               strbuf_reset(&origref);
+               strbuf_addf(&path, "%s/HEAD", worktrees[i]->git_dir);
+
+               if (parse_ref(path.buf, &origref, NULL))
+                       continue;
+
+               if (!strcmp(origref.buf, oldref)) {
+                       int prefix_len = strlen(absolute_path(get_git_common_dir())) + 1;
+                       const char *symref = path.buf + prefix_len;
+
+                       /* no need to pass logmsg here as HEAD didn't really move */
+                       if (create_symref(symref, newref, NULL)) {
+                               error = -1;
+                               break;

Is aborting upon the first error desired behavior? (Genuine question.)
Would it make more sense to continue attempting the rename for the
remaining worktrees (and remember that an error was encountered)?
Related: Since you're now dealing with multiple worktrees, you can do
a better job of letting the user know in which worktree something went
wrong rather than merely emitting the relatively generic "Branch
renamed to %s, but HEAD is not updated!".

I think both is ok.
But continuing shouldn't be harm, so continuing might be better in terms
of that it can tell the user what files need to be fixed manually.
I'll try it.

quoted

+}

diff --git a/worktree.h b/worktree.h

@@ -35,4 +35,11 @@ extern void free_worktrees(struct worktree **);
+/*
+ * 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 *);

I guess I can understand the desire to libify this functionality,
however, it feels as if it is a feature of "branch" rather than
"worktree", hence perhaps it should reside in top-level branch.[hc]?

I agree, I'll move it.
I chose worktree.c just because it has parse_ref().

On Tue, Mar 22, 2016 at 07:49:00AM +0700, Duy Nguyen wrote:

On Tue, Mar 22, 2016 at 12:41 AM, Eric Sunshine [off-list ref] wrote:

quoted

quoted

diff --git a/worktree.c b/worktree.c

@@ -217,3 +217,41 @@ char *find_shared_symref(const char *symref, const char *target)
+int update_worktrees_head_symref(const char *oldref, const char *newref)
+{
+       int error = 0;
+       struct strbuf path = STRBUF_INIT;
+       struct strbuf origref = STRBUF_INIT;
+       int i;
+       struct worktree **worktrees = get_worktrees();
+
+       for (i = 0; worktrees[i]; i++) {
+               if (worktrees[i]->is_detached)
+                       continue;
+
+               strbuf_reset(&path);
+               strbuf_reset(&origref);
+               strbuf_addf(&path, "%s/HEAD", worktrees[i]->git_dir);
+
+               if (parse_ref(path.buf, &origref, NULL))
+                       continue;
+
+               if (!strcmp(origref.buf, oldref)) {
+                       int prefix_len = strlen(absolute_path(get_git_common_dir())) + 1;
+                       const char *symref = path.buf + prefix_len;
+
+                       /* no need to pass logmsg here as HEAD didn't really move */
+                       if (create_symref(symref, newref, NULL)) {
+                               error = -1;
+                               break;

Is aborting upon the first error desired behavior? (Genuine question.)
Would it make more sense to continue attempting the rename for the
remaining worktrees (and remember that an error was encountered)?

Since all these HEADs stay at the same (or close) location, if one
fails, I think the rest will fail too. Which leads to a series of
warnings if we continue anyway. A more interesting approach is update
HEADs in a transaction, so we successfully update all or we update
none. But I do not know if ref transactions can be used for HEAD,
especially worktree HEADs. I'm ok with either abort here or continue
anyway, though.
-- 
Duy

Thanks for suggestion, but it looks like ref_transaction can be used
only for updating non-symbolic references. Extending it only for this
purpose seems too much...

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 <redacted>
---
 refs.h               |   4 ++
 refs/files-backend.c |   5 ++-
 worktree.c           | 105 ++++++++++++++++-----------------------------------
 3 files changed, 41 insertions(+), 73 deletions(-)

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

[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

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 <redacted>
---
 refs.h               |  3 +++
 refs/files-backend.c | 17 ++++++++++++++---
 2 files changed, 17 insertions(+), 3 deletions(-)

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

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 <redacted>
---
 builtin/branch.c  | 12 +++++++-----
 t/t3200-branch.sh |  6 ++++++
 2 files changed, 13 insertions(+), 5 deletions(-)

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

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 <redacted>
---
 refs.h               |  4 ++++
 refs/files-backend.c | 12 ++++++++++--
 2 files changed, 14 insertions(+), 2 deletions(-)

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

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 <redacted>
---
 branch.c          | 32 ++++++++++++++++++++++++++++++++
 branch.h          |  7 +++++++
 builtin/branch.c  |  3 +--
 t/t3200-branch.sh | 23 ++++++++++++++++++++++-
 4 files changed, 62 insertions(+), 3 deletions(-)

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

Add a new function set_worktree_head_symref, to update HEAD symref for
the specified worktree.

To update HEAD of a linked working tree,
create_symref("worktrees/$work_tree/HEAD", "refs/heads/$branch", msg)
could be used. However when it comes to updating HEAD of the main
working tree, it is unusable because it uses $GIT_DIR for
worktree-specific symrefs (HEAD).

The new function takes git_dir (real directory) as an argument, and
updates HEAD of the working tree. This function will be used when
renaming a branch.

Signed-off-by: Kazuki Yamaguchi <redacted>
---
 refs.h               |  8 ++++++++
 refs/files-backend.c | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+)

diff --git a/refs.h b/refs.h
index 2f3decb432cf..f11154cf5faf 100644
--- a/refs.h
+++ b/refs.h

@@ -306,6 +306,14 @@ extern int rename_ref(const char *oldref, const char *newref, const char *logmsg
 
 extern int create_symref(const char *refname, const char *target, const char *logmsg);
 
+/*
+ * Update HEAD of the specified gitdir.
+ * Similar to create_symref("relative-git-dir/HEAD", target, NULL), but this is
+ * able to update the main working tree's HEAD wherever $GIT_DIR points to.
+ * Return 0 if successful, non-zero otherwise.
+ * */
+extern int set_worktree_head_symref(const char *gitdir, const char *target);
+
 enum action_on_err {
 	UPDATE_REFS_MSG_ON_ERR,
 	UPDATE_REFS_DIE_ON_ERR,

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 81f68f846b69..ec237efec35d 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c

@@ -2894,6 +2894,41 @@ int create_symref(const char *refname, const char *target, const char *logmsg)
 	return ret;
 }
 
+int set_worktree_head_symref(const char *gitdir, const char *target)
+{
+	static struct lock_file head_lock;
+	struct ref_lock *lock;
+	struct strbuf err = STRBUF_INIT;
+	struct strbuf head_path = STRBUF_INIT;
+	const char *head_rel;
+	int ret;
+
+	strbuf_addf(&head_path, "%s/HEAD", absolute_path(gitdir));
+	if (hold_lock_file_for_update(&head_lock, head_path.buf,
+				      LOCK_NO_DEREF) < 0) {
+		error("%s", err.buf);
+		strbuf_release(&err);
+		strbuf_release(&head_path);
+		return -1;
+	}
+
+	/* head_rel will be "HEAD" for the main tree, "worktrees/wt/HEAD" for
+	   linked trees */
+	head_rel = remove_leading_path(head_path.buf,
+				       absolute_path(get_git_common_dir()));
+	/* to make use of create_symref_locked(), initialize ref_lock */
+	lock = xcalloc(1, sizeof(struct ref_lock));
+	lock->lk = &head_lock;
+	lock->ref_name = xstrdup(head_rel);
+	lock->orig_ref_name = xstrdup(head_rel);
+
+	ret = create_symref_locked(lock, head_rel, target, NULL);
+
+	unlock_ref(lock); /* will free lock */
+	strbuf_release(&head_path);
+	return ret;
+}
+
 int reflog_exists(const char *refname)
 {
 	struct stat st;

-- 
2.8.0.rc4.21.g05df949

Changes from v2:
- The flags REF_COMMON_DIR and RESOLVE_REF_COMMON_DIR are removed.
- create_symref_common_dir() is removed and instead adds narrower
  purpose function, set_worktree_head_symref().

[1/2]
Adds a new function set_worktree_head_symref(). This takes git_dir as
the first argument, and updates {git_dir}/HEAD.

The new function uses hold_lock_file_for_update() directly, instead of
through lock_ref_sha1_basic() which the old [v2 3/5] used.

[2/2] (from [v2 4/5])
Uses the new set_worktree_head_symref(), and the
update_worktrees_head_symref() function was renamed to
replace_each_worktree_head_symref(), to avoid confusion with
set_worktree_head_symref() added by [1/2].


Thanks,

Kazuki Yamaguchi (2):
  refs: add a new function set_worktree_head_symref
  branch -m: update all per-worktree HEADs

 branch.c             | 23 +++++++++++++++++++++++
 branch.h             |  7 +++++++
 builtin/branch.c     |  3 +--
 refs.h               |  8 ++++++++
 refs/files-backend.c | 35 +++++++++++++++++++++++++++++++++++
 t/t3200-branch.sh    | 23 ++++++++++++++++++++++-
 6 files changed, 96 insertions(+), 3 deletions(-)

-- 
2.8.0.rc4.21.g05df949

When renaming a branch, currently only the HEAD of current working tree
is updated, but it must update HEADs of all working trees which point at
the old branch.

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 <redacted>
---
 branch.c          | 23 +++++++++++++++++++++++
 branch.h          |  7 +++++++
 builtin/branch.c  |  3 +--
 t/t3200-branch.sh | 23 ++++++++++++++++++++++-
 4 files changed, 53 insertions(+), 3 deletions(-)

diff --git a/branch.c b/branch.c
index c50ea42172ce..416244370783 100644
--- a/branch.c
+++ b/branch.c

@@ -344,3 +344,26 @@ void die_if_checked_out(const char *branch)
 		die(_("'%s' is already checked out at '%s'"), branch, existing);
 	}
 }
+
+int replace_each_worktree_head_symref(const char *oldref, const char *newref)
+{
+	int ret = 0;
+	struct worktree **worktrees = get_worktrees();
+	int i;
+
+	for (i = 0; worktrees[i]; i++) {
+		if (worktrees[i]->is_detached)
+			continue;
+		if (strcmp(oldref, worktrees[i]->head_ref))
+			continue;
+
+		if (set_worktree_head_symref(worktrees[i]->git_dir, newref)) {
+			ret = -1;
+			error(_("HEAD of working tree %s is not updated"),
+			      worktrees[i]->path);
+		}
+	}
+
+	free_worktrees(worktrees);
+	return ret;
+}

diff --git a/branch.h b/branch.h
index 78ad4387cd32..d69163daf793 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 at 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 replace_each_worktree_head_symref(const char *oldref, const char *newref);
+
 #endif

diff --git a/builtin/branch.c b/builtin/branch.c
index 7b45b6bd6b80..b1738313175c 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 (replace_each_worktree_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

On Fri, Mar 25, 2016 at 2:28 PM, Kazuki Yamaguchi [off-list ref] wrote:

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.

On Sun, Mar 27, 2016 at 01:52:18PM -0400, Eric Sunshine wrote:

On Fri, Mar 25, 2016 at 2:28 PM, Kazuki Yamaguchi [off-list ref] wrote:

quoted

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 <redacted>
---

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.

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 <redacted>
---

  % 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(-)

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

On Mon, Mar 28, 2016 at 3:22 AM, Kazuki Yamaguchi [off-list ref] 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).

Signed-off-by: Kazuki Yamaguchi <redacted>
---

  % 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

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?

+                               ret = 1;
+                               continue;
+                       }
+               }
+
                target = resolve_ref_unsafe(name,
                                            RESOLVE_REF_READING
                                            | RESOLVE_REF_NO_RECURSE

On Mon, Mar 28, 2016 at 12:51:21PM -0400, Eric Sunshine wrote:

On Mon, Mar 28, 2016 at 3:22 AM, Kazuki Yamaguchi [off-list ref] wrote:

quoted

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.

quoted

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).

quoted

Signed-off-by: Kazuki Yamaguchi <redacted>
---

  % 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.

quoted

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.

quoted

+                                     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,

quoted

+                               ret = 1;
+                               continue;
+                       }
+               }
+
                target = resolve_ref_unsafe(name,
                                            RESOLVE_REF_READING
                                            | RESOLVE_REF_NO_RECURSE

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 [off-list ref]
Signed-off-by: Kazuki Yamaguchi <redacted>
---
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(-)

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

On Tue, Mar 29, 2016 at 5:28 AM, Kazuki Yamaguchi [off-list ref] wrote:

On Mon, Mar 28, 2016 at 12:51:21PM -0400, Eric Sunshine wrote:

quoted

On Mon, Mar 28, 2016 at 3:22 AM, Kazuki Yamaguchi [off-list ref] wrote:

quoted

+               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.

On Tue, Mar 29, 2016 at 5:38 AM, Kazuki Yamaguchi [off-list ref] 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 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 [off-list ref]
Signed-off-by: Kazuki Yamaguchi <redacted>
---
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 [off-list ref]