branch: update all per-worktree HEADs when renaming a branch

PATCH Superseded Kazuki Yamaguchi 5 participants 30 messages Jun 15, 2016 – Jun 16, 2016

Share & tools

Kazuki Yamaguchi

author

2016-06-15 23:08 UTC #0 lore raw

PATCH 1/1 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 <[email protected]>

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

builtin/branch.c +2 -2

index 7b45b6b..596fb5f 100644
@@ -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);

t/t3200-branch.sh +13 -1

index a897248..da107d0 100755
@@ -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' '

worktree.c +38 -0

index 6181a66..9e7d0f3 100644
@@ -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;
+}

worktree.h +7 -0

index b4b3dda..0d15d11 100644
@@ -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

Signature · 1 line

2.8.0.rc3.13.gcd7ec22

Eric Sunshine

2016-06-15 23:08 UTC #1 lore raw

Quoted 8 lines · Kazuki Yamaguchi

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

Quoted 5 lines

Signed-off-by Kazuki Yamaguchi <[email protected]>

---

t/t3200-branch.sh +0 -0

@@ -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' '

t/t3200-branch.sh test_expect_success 'git branch -M foo bar should fail when bar is checked out' · line 136

Quoted 10 lines

t/t3200-branch.sh +9 -0

+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

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

Quoted 19 lines

worktree.c +17 -0

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

worktree.c char *find_shared_symref(const char *symref, const char *target) · line 243

Quoted 10 lines

worktree.c +10 -0

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

More below...

Quoted 12 lines

worktree.c +10 -0

+                       }
+               }
+       }
+
+       strbuf_release(&path);
+       strbuf_release(&origref);
+       free_worktrees(worktrees);
+
+       return error;
+}

worktree.h extern void free_worktrees(struct worktree **); · line 40

Quoted 6 lines

worktree.h +6 -0

@@ -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]?

Duy Nguyen

2016-06-15 23:08 UTC #2 lore raw

Quoted 25 lines · Eric Sunshine

Quoted 25 lines

worktree.c +23 -0

@@ -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;
+

Quoted 8 lines

worktree.c char *find_shared_symref(const char *symref, const char *target) · line 247
Quoted 4 lines
worktree.c +4 -0
+                       /* 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.

Signature · 1 line

Duy

Kazuki Yamaguchi

author

2016-06-15 23:08 UTC #3 lore raw

Quoted 35 lines · Duy Nguyen

Quoted 25 lines · Eric Sunshine

Quoted 25 lines

worktree.c +23 -0

@@ -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;
+

Quoted 8 lines

worktree.c char *find_shared_symref(const char *symref, const char *target) · line 247
Quoted 4 lines
worktree.c +4 -0
+                       /* 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)?

Quoted 9 lines

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.
Signature · 1 line
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...

Kazuki Yamaguchi

author

2016-06-15 23:08 UTC #4 lore raw

In reply to Eric Sunshine

Hello,

Quoted 16 lines · Eric Sunshine

t/t3200-branch.sh test_expect_success 'git branch -M foo bar should fail when bar is checked out' · line 136

Quoted 13 lines

t/t3200-branch.sh +9 -0

@@ -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:

Quoted 1 line

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

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

Quoted 8 lines

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 31 lines

Quoted 20 lines

worktree.c +17 -0

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

worktree.c char *find_shared_symref(const char *symref, const char *target) · line 243

Quoted 10 lines

worktree.c +10 -0

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

Quoted 7 lines

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 5 lines

worktree.c +1 -0

+}

worktree.h +2 -0

@@ -35,4 +35,11 @@ extern void free_worktrees(struct worktree **);
+/*
+ * Update all per-worktree HEADs pointing the old ref to point the new ref.

Quoted 8 lines

worktree.h extern void free_worktrees(struct worktree **); · line 43
Quoted 4 lines
worktree.h +4 -0
+ * 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().

Kazuki Yamaguchi

author

2016-06-15 23:08 UTC #5 lore raw

v2 cover branch: fix branch operations with multiple working trees

In reply to Kazuki Yamaguchi

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

Signature · 1 line

2.8.0.rc4.21.g05df949

Kazuki Yamaguchi

author

2016-06-15 23:08 UTC #6 lore raw

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

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 <[email protected]>

---
 refs.h               |   4 ++
 refs/files-backend.c |   5 ++-
 worktree.c           | 105 ++++++++++++++++-----------------------------------
 3 files changed, 41 insertions(+), 73 deletions(-)

refs.h +4 -0

index 2f3decb432cf..5d9ab5c1c5dd 100644
@@ -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);

refs/files-backend.c +4 -1

index 81f68f846b69..a534f1a1e078 100644
@@ -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;
 
 		/*

worktree.c +33 ~~-72~~

181 lines not on this page · raw message

Signature · 1 line

2.8.0.rc4.21.g05df949

Kazuki Yamaguchi

author

2016-06-15 23:08 UTC #7 lore raw

PATCH v2 2/5 refs: add REF_COMMON_DIR flag

In reply to Kazuki Yamaguchi

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 <[email protected]>

---
 refs.h               |  4 ++++
 refs/files-backend.c | 12 ++++++++++--
 2 files changed, 14 insertions(+), 2 deletions(-)

refs.h +4 -0

index 5d9ab5c1c5dd..dc4782241e49 100644
@@ -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.

refs/files-backend.c +10 -2

index a534f1a1e078..2a808d520213 100644
@@ -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)) {

Signature · 1 line

2.8.0.rc4.21.g05df949

Kazuki Yamaguchi

author

2016-06-15 23:08 UTC #8 lore raw

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

In reply to Kazuki Yamaguchi

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 <[email protected]>

---
 refs.h               |  3 +++
 refs/files-backend.c | 17 ++++++++++++++---
 2 files changed, 17 insertions(+), 3 deletions(-)

refs.h +3 -0

index dc4782241e49..ff8dbbe1a0e5 100644
@@ -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,

refs/files-backend.c +14 -3

index 2a808d520213..1fe4d4e75188 100644
@@ -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;

Signature · 1 line

2.8.0.rc4.21.g05df949

Kazuki Yamaguchi

author

2016-06-15 23:08 UTC #9 lore raw

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

In reply to Kazuki Yamaguchi

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.

Signed-off-by Kazuki Yamaguchi <[email protected]>

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

branch.c +32 -0

index c50ea42172ce..df0928e32a70 100644
@@ -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;
+}

branch.h +7 -0

index 78ad4387cd32..3f5ae4b8866e 100644
@@ -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

builtin/branch.c +1 -2

index 7b45b6bd6b80..31eb473d3e6a 100644
@@ -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);

t/t3200-branch.sh +22 -1

index a89724849065..f7d438bd7d1d 100755
@@ -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' '

Signature · 1 line

2.8.0.rc4.21.g05df949

Kazuki Yamaguchi

author

2016-06-15 23:08 UTC #10 lore raw

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

In reply to Kazuki Yamaguchi

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 <[email protected]>

---
 builtin/branch.c  | 12 +++++++-----
 t/t3200-branch.sh |  6 ++++++
 2 files changed, 13 insertions(+), 5 deletions(-)

builtin/branch.c +7 -5

index 31eb473d3e6a..e64aa68cf722 100644
@@ -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

t/t3200-branch.sh +6 -0

index f7d438bd7d1d..f3e3b6cf2eab 100755
@@ -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)" &&

Signature · 1 line

2.8.0.rc4.21.g05df949

Junio C Hamano

2016-06-15 23:08 UTC #11 lore raw

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

Quoted 8 lines · Kazuki Yamaguchi

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 <[email protected]>
---

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.

Kazuki Yamaguchi

author

2016-06-15 23:09 UTC #12 lore raw

PATCH v2 1/1 branch -d: refuse deleting a branch which is currently checked out

In reply to Kazuki Yamaguchi

Signed-off-by Kazuki Yamaguchi <[email protected]>

---

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

builtin/branch.c +14 -8

index 7b45b6bd6b80..de1641306c9e 100644
@@ -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

t/t3200-branch.sh +6 -0

index a89724849065..508007fd3772 100755
@@ -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)" &&

Signature · 1 line

2.8.0.rc4.17.gdb328b3

Eric Sunshine

2016-06-15 23:09 UTC #13 lore raw

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

Quoted 3 lines · Kazuki Yamaguchi

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.

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

Quoted 8 lines

Signed-off-by Kazuki Yamaguchi <[email protected]>
---
% 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

Quoted 11 lines

builtin/branch.c +0 -6

@@ -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;
-               }

builtin/branch.c static int delete_branches(int argc, const char **argv, int force, int kinds, · line 226

Quoted 10 lines

builtin/branch.c +6 -2

-
                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'"

builtin/branch.c static int delete_branches(int argc, const char **argv, int force, int kinds, · line 228

Quoted 2 lines

builtin/branch.c +2 -0

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

Quoted 8 lines

builtin/branch.c +5 -0

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

Kazuki Yamaguchi

author

2016-06-15 23:09 UTC #14 lore raw

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

Quoted 11 lines · Eric Sunshine

Quoted 3 lines · Kazuki Yamaguchi
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 16 lines

Quoted 3 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).
Quoted 8 lines
Signed-off-by Kazuki Yamaguchi <[email protected]>
---
% 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'

Quoted 6 lines

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 25 lines

Quoted 12 lines

builtin/branch.c +0 -6

@@ -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;
-               }

builtin/branch.c static int delete_branches(int argc, const char **argv, int force, int kinds, · line 226

Quoted 10 lines

builtin/branch.c +6 -2

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

Quoted 1 line

"Cannot delete branch '%s' checked out at '%s'"

I'll use it. Thanks.

Quoted 7 lines

builtin/branch.c static int delete_branches(int argc, const char **argv, int force, int kinds, · line 228
Quoted 3 lines
builtin/branch.c +2 -0
+                                     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 9 lines

builtin/branch.c +5 -0

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

Eric Sunshine

2016-06-15 23:09 UTC #15 lore raw

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

Quoted 14 lines · Kazuki Yamaguchi

Quoted 9 lines · Eric Sunshine
builtin/branch.c static int delete_branches(int argc, const char **argv, int force, int kinds, · line 228
Quoted 7 lines · Kazuki Yamaguchi
builtin/branch.c +7 -0
+               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);
Quoted 3 lines
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?

Quoted 5 lines

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.

Eric Sunshine

2016-06-15 23:09 UTC #16 lore raw

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

In reply to Kazuki Yamaguchi

Quoted 12 lines · Kazuki Yamaguchi

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 <[email protected]>
---
builtin/branch.c +0 -0
@@ -215,16 +216,17 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
                int flags = 0;
 

builtin/branch.c static int delete_branches(int argc, const char **argv, int force, int kinds, · line 225

Quoted 10 lines

builtin/branch.c +6 -2

                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

author

2016-06-15 23:09 UTC #17 lore raw

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

Quoted 22 lines · Eric Sunshine

Quoted 21 lines · Kazuki Yamaguchi

Signed-off-by Kazuki Yamaguchi <[email protected]>

---

builtin/branch.c +5 -2

@@ -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);

Quoted 8 lines

builtin/branch.c static int delete_branches(int argc, const char **argv, int force, int kinds, · line 225
Quoted 1 line
builtin/branch.c +1 -0
+                             "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

author

2016-06-15 23:09 UTC #18 lore raw

PATCH v3 1/1 branch -d: refuse deleting a branch which is currently checked out

In reply to Kazuki Yamaguchi

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 <[email protected]>

Signed-off-by Kazuki Yamaguchi <[email protected]>

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

builtin/branch.c +14 -8

index 7b45b6bd6b80..8885d9f8e2cd 100644
@@ -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

t/t3200-branch.sh +6 -0

index a89724849065..508007fd3772 100755
@@ -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)" &&

Signature · 1 line

2.8.0.rc4.17.g02aa164.dirty

Eric Sunshine

Reviewed-by on v3 1/1

2016-06-15 23:09 UTC #19 lore raw

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

Quoted 6 lines · Kazuki Yamaguchi

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.

Quoted 6 lines

Helped-by Eric Sunshine <[email protected]>
Signed-off-by Kazuki Yamaguchi <[email protected]>
---
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 <[email protected]>

Quoted 69 lines

 builtin/branch.c  | 22 ++++++++++++++--------
 t/t3200-branch.sh |  6 ++++++
 2 files changed, 20 insertions(+), 8 deletions(-)

builtin/branch.c +14 -8

index 7b45b6bd6b80..8885d9f8e2cd 100644
@@ -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

t/t3200-branch.sh +6 -1

index a89724849065..508007fd3772 100755
@@ -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

2016-06-15 23:08 UTC #20 lore raw

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

In reply to Kazuki Yamaguchi

Quoted 7 lines · Kazuki Yamaguchi

[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?

Quoted 4 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?

Quoted 11 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

author

2016-06-15 23:08 UTC #21 lore raw

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

Quoted 7 lines · Junio C Hamano

Quoted 5 lines · Kazuki Yamaguchi
[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().

Quoted 9 lines

Quoted 2 lines
[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.

Quoted 5 lines

Quoted 4 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.

Quoted 7 lines

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,

Quoted 13 lines

Quoted 11 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

author

2016-06-15 23:09 UTC #22 lore raw

v3 cover update all per-worktree HEADs when renaming a branch

In reply to Kazuki Yamaguchi

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

Signature · 1 line

2.8.0.rc4.21.g05df949

Kazuki Yamaguchi

author

2016-06-15 23:09 UTC #23 lore raw

PATCH v3 1/2 refs: add a new function set_worktree_head_symref

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 <[email protected]>

---
 refs.h               |  8 ++++++++
 refs/files-backend.c | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+)

refs.h +8 -0

index 2f3decb432cf..f11154cf5faf 100644
@@ -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,

refs/files-backend.c +35 -0

index 81f68f846b69..ec237efec35d 100644
@@ -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;

Signature · 1 line

2.8.0.rc4.21.g05df949

Junio C Hamano

2016-06-15 23:09 UTC #24 lore raw

Re: [PATCH v3 1/2] refs: add a new function set_worktree_head_symref

Quoted 9 lines · Kazuki Yamaguchi

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

Quoted 6 lines

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 <[email protected]>
---

I suspect that this helper function should be in the common layer (refs.c) to be redirected to specific backend(s).

David & Michael, what do you think?

Quoted 69 lines

 refs.h               |  8 ++++++++
 refs/files-backend.c | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+)

refs.h +8 -0

index 2f3decb432cf..f11154cf5faf 100644
@@ -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,

refs/files-backend.c +35 -0

index 81f68f846b69..ec237efec35d 100644
@@ -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;

David Turner

2016-06-15 23:09 UTC #25 lore raw

Re: [PATCH v3 1/2] refs: add a new function set_worktree_head_symref

Quoted 17 lines · Junio C Hamano

Quoted 15 lines · Kazuki Yamaguchi
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.

Quoted 7 lines

Quoted 2 lines
Signed-off-by Kazuki Yamaguchi <[email protected]>
---
I suspect that this helper function should be in the common layer (refs.c) to be redirected to specific backend(s).
David & Michael, what do you think?

HEAD is a per-worktree ref, so it's always handled by the files backend. So this looks fine to me.

Quoted 6 lines

Quoted 6 lines
+/*
+ * 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.

nit: "able to update the main working tree's HEAD regardless of where GIT_DIR points to" would be clearer.

Eric Sunshine

2016-06-16 02:18 UTC #26 lore raw

Re: [PATCH v3 1/2] refs: add a new function set_worktree_head_symref

In reply to Kazuki Yamaguchi

Quoted 20 lines · Kazuki Yamaguchi

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 <[email protected]>
---
refs/files-backend.c +3 -0
@@ -2894,6 +2894,41 @@ int create_symref(const char *refname, const char *target, const char *logmsg)
+int set_worktree_head_symref(const char *gitdir, const char *target)
+{
+       static struct lock_file head_lock;

refs/files-backend.c int create_symref(const char *refname, const char *target, const char *logmsg) · line 2906

Quoted 10 lines

refs/files-backend.c +10 -0

+       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);

'err' has not been populated at this point, so I suspect that this error message is likely to be rather uninformative.

Quoted 22 lines

refs/files-backend.c +22 -0

+               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;
+}
+

Kazuki Yamaguchi

author

2016-06-16 02:18 UTC #27 lore raw

Re: [PATCH v3 1/2] refs: add a new function set_worktree_head_symref

Quoted 26 lines · Eric Sunshine

Quoted 25 lines · Kazuki Yamaguchi
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 <[email protected]>
---
refs/files-backend.c +8 -0
@@ -2894,6 +2894,41 @@ int create_symref(const char *refname, const char *target, const char *logmsg)
+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;

Quoted 8 lines

refs/files-backend.c int create_symref(const char *refname, const char *target, const char *logmsg) · line 2909
Quoted 5 lines
refs/files-backend.c +5 -0
+
+       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);
'err' has not been populated at this point, so I suspect that this error message is likely to be rather uninformative.

Yes, unable_to_lock_message() is missing. Thank you for pointing it out.

Quoted 23 lines

refs/files-backend.c +22 -0

+               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;
+}
+

Eric Sunshine

2016-06-16 02:18 UTC #28 lore raw

Re: [PATCH v3 1/2] refs: add a new function set_worktree_head_symref

Quoted 19 lines · Kazuki Yamaguchi

Quoted 17 lines · Eric Sunshine

refs/files-backend.c int create_symref(const char *refname, const char *target, const char *logmsg) · line 2909

Quoted 13 lines · Kazuki Yamaguchi

refs/files-backend.c +13 -0

+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);

'err' has not been populated at this point, so I suspect that this error message is likely to be rather uninformative.

Quoted 1 line

Yes, unable_to_lock_message() is missing. Thank you for pointing it out.

You're welcome. As this patch is already in Junio's "next" branch, if you post a fix, it should be incremental atop "ky/branch-m-worktree", rather than as a re-roll of this series.

Kazuki Yamaguchi

author

2016-06-15 23:09 UTC #29 lore raw

PATCH v3 2/2 branch -m: update all per-worktree HEADs

In reply to Kazuki Yamaguchi

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:

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

Signed-off-by Kazuki Yamaguchi <[email protected]>

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

branch.c +23 -0

index c50ea42172ce..416244370783 100644
@@ -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;
+}

branch.h +7 -0

index 78ad4387cd32..d69163daf793 100644
@@ -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

builtin/branch.c +1 -2

index 7b45b6bd6b80..b1738313175c 100644
@@ -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);

t/t3200-branch.sh +22 -1

index a89724849065..f7d438bd7d1d 100755
@@ -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' '

Signature · 1 line

2.8.0.rc4.21.g05df949