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

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

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

  % git worktree list
  /path/to     2c3c5f2 [master]
  /path/to/wt  2c3c5f2 [oldname]
  % git branch -m master master2
  % git worktree list
  /path/to     2c3c5f2 [master2]
  /path/to/wt  2c3c5f2 [oldname]
  % git branch -m oldname newname
  % git worktree list
  /path/to     2c3c5f2 [master2]
  /path/to/wt  0000000 [oldname]

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

Signed-off-by: Kazuki Yamaguchi <k@rhe.jp>
---
 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

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

[Eric Sunshine]

On Mon, Mar 21, 2016 at 5:50 AM, Kazuki Yamaguchi <k@rhe.jp> 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...

> Signed-off-by: Kazuki Yamaguchi <k@rhe.jp>
> ---
> 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

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

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

More below...

> +                       }
> +               }
> +       }
> +
> +       strbuf_release(&path);
> +       strbuf_release(&origref);
> +       free_worktrees(worktrees);
> +
> +       return error;
> +}
> 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]?

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

[Duy Nguyen]

On Tue, Mar 22, 2016 at 12:41 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> 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

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

[Kazuki Yamaguchi]

Hello,

On 03/22/2016 03:41 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> 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.

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

>> +}
>> 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().

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

[Kazuki Yamaguchi]

On Tue, Mar 22, 2016 at 07:49:00AM +0700, Duy Nguyen wrote:
> On Tue, Mar 22, 2016 at 12:41 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> >> 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...

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

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

-- 
2.8.0.rc4.21.g05df949

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

[Kazuki Yamaguchi]

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

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

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

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

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

[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 <k@rhe.jp>
---
 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

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

[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 <k@rhe.jp>
---
 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

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

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

  % git worktree list
  /path/to     2c3c5f2 [master]
  /path/to/wt  2c3c5f2 [oldname]
  % git branch -m master master2
  % git worktree list
  /path/to     2c3c5f2 [master2]
  /path/to/wt  2c3c5f2 [oldname]
  % git branch -m oldname newname
  % git worktree list
  /path/to     2c3c5f2 [master2]
  /path/to/wt  0000000 [oldname]

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

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

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

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

[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 <k@rhe.jp>
---
 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

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

[Junio C Hamano]

Kazuki Yamaguchi <k@rhe.jp> writes:

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

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

Thanks.

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

[Junio C Hamano]

Kazuki Yamaguchi <k@rhe.jp> writes:

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

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

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

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

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

[Kazuki Yamaguchi]

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

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

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

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

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

Thanks, 

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

[PATCH v3 0/2] update all per-worktree HEADs when renaming a branch

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

-- 
2.8.0.rc4.21.g05df949

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

[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 <k@rhe.jp>
---
 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

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

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

  % git worktree list
  /path/to     2c3c5f2 [master]
  /path/to/wt  2c3c5f2 [oldname]
  % git branch -m master master2
  % git worktree list
  /path/to     2c3c5f2 [master2]
  /path/to/wt  2c3c5f2 [oldname]
  % git branch -m oldname newname
  % git worktree list
  /path/to     2c3c5f2 [master2]
  /path/to/wt  0000000 [oldname]

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

Signed-off-by: Kazuki Yamaguchi <k@rhe.jp>
---
 branch.c          | 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

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

[Eric Sunshine]

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

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

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

[Kazuki Yamaguchi]

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

Yes, indeed. I'll do it. Thanks.

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

[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 <k@rhe.jp>
---

  % git worktree list
  /path/to      2c3c5f2 [master]
  /path/to/wt   2c3c5f2 [branch-a]
  % git branch -d branch-a
  error: Cannot delete the branch 'branch-a' which is currently checked out at '/path/to/wt'

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

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

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

[Eric Sunshine]

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

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

    s/deleting is allowed/deletion incorrectly succeeds/

would make it more clear.

> Use find_shared_symref() to check if the branch is in use, not just
> comparing with the current working tree's HEAD.

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

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

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

[1]: http://thread.gmane.org/gmane.comp.version-control.git/289413/focus=289932

> diff --git a/builtin/branch.c b/builtin/branch.c
> @@ -215,16 +216,21 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
>                 int flags = 0;
>
>                 strbuf_branchname(&bname, argv[i]);
> -               if (kinds == FILTER_REFS_BRANCHES && !strcmp(head, bname.buf)) {
> -                       error(_("Cannot delete the branch '%s' "
> -                             "which you are currently on."), bname.buf);
> -                       ret = 1;
> -                       continue;
> -               }
> -
>                 free(name);
> -
>                 name = mkpathdup(fmt, bname.buf);
> +
> +               if (kinds == FILTER_REFS_BRANCHES) {
> +                       char *worktree = find_shared_symref("HEAD", name);
> +                       if (worktree) {
> +                               error(_("Cannot delete the branch '%s' "
> +                                       "which is currently checked out at '%s'"),

This could be stated more concisely as:

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

> +                                     bname.buf, worktree);
> +                               free(worktree);

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

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

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

[Junio C Hamano]

Kazuki Yamaguchi <k@rhe.jp> writes:

> 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 <k@rhe.jp>
> ---

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?

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

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

[David Turner]

On Mon, 2016-03-28 at 10:48 -0700, Junio C Hamano wrote:
> Kazuki Yamaguchi <k@rhe.jp> writes:
> 
> > 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 <k@rhe.jp>
> > ---
> 
> 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.

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

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

[Kazuki Yamaguchi]

On Mon, Mar 28, 2016 at 12:51:21PM -0400, Eric Sunshine wrote:
> On Mon, Mar 28, 2016 at 3:22 AM, Kazuki Yamaguchi <k@rhe.jp> wrote:
> > 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.

> 
> > Use find_shared_symref() to check if the branch is in use, not just
> > comparing with the current working tree's HEAD.
> 
> This version of the patch is nicer. Thanks. See a couple minor
> comments below which may or may not be worth a re-roll (you decide).
> 
> > Signed-off-by: Kazuki Yamaguchi <k@rhe.jp>
> > ---
> >
> >   % git worktree list
> >   /path/to      2c3c5f2 [master]
> >   /path/to/wt   2c3c5f2 [branch-a]
> >   % git branch -d branch-a
> >   error: Cannot delete the branch 'branch-a' which is currently checked out at '/path/to/wt'
> 
> Thanks for an example of the new behavior. It's also helpful to
> reviewers if you use this space to explain what changed since the
> previous version, and to provide a link to the previous attempt, like
> this[1].
> 
> [1]: http://thread.gmane.org/gmane.comp.version-control.git/289413/focus=289932

I'll do from next time.

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

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

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

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

[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 <sunshine@sunshineco.com>
Signed-off-by: Kazuki Yamaguchi <k@rhe.jp>
---
Changes from v2:
- Amended commit message
- Dropped "which is" from error message

The previous versions of the patch are:
- [v1] http://thread.gmane.org/gmane.comp.version-control.git/289413/focus=289932
- [v2] http://thread.gmane.org/gmane.comp.version-control.git/289413/focus=290027

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

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

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

[Eric Sunshine]

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

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

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

[Eric Sunshine]

On Tue, Mar 29, 2016 at 5:38 AM, Kazuki Yamaguchi <k@rhe.jp> wrote:
> When a branch is checked out by current working tree, deleting the
> branch is forbidden. However when the branch is checked out only by
> other working trees, deleting incorrectly succeeds.
> Use find_shared_symref() to check if the branch is in use, not just
> comparing with the current working tree's HEAD.
>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Kazuki Yamaguchi <k@rhe.jp>
> ---
> Changes from v2:
> - Amended commit message
> - Dropped "which is" from error message

Thanks, this version addresses my previous review comments and is:

    Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>

> 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

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

[Eric Sunshine]

On Sun, Mar 27, 2016 at 10:37 AM, Kazuki Yamaguchi <k@rhe.jp> wrote:
> 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 <k@rhe.jp>
> ---
> diff --git 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)
> +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.

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

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

[Kazuki Yamaguchi]

On Thu, Apr 07, 2016 at 05:20:10PM -0400, Eric Sunshine wrote:
> On Sun, Mar 27, 2016 at 10:37 AM, Kazuki Yamaguchi <k@rhe.jp> wrote:
> > 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 <k@rhe.jp>
> > ---
> > diff --git 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)
> > +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.

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

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

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

[Eric Sunshine]

On Fri, Apr 8, 2016 at 2:37 AM, Kazuki Yamaguchi <k@rhe.jp> wrote:
> On Thu, Apr 07, 2016 at 05:20:10PM -0400, Eric Sunshine wrote:
>> On Sun, Mar 27, 2016 at 10:37 AM, Kazuki Yamaguchi <k@rhe.jp> wrote:
>> > +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.
>
> 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.