threads / patch / 39833

patch, 16 partsworktree: make setup of new HEAD distinct from worktree population

Subject: [PATCH 13/16] worktree: make setup of new HEAD distinct from worktree population

## tl;dr

31 messages between Jul 11, 2015 and Jul 15, 2015. Diffs are folded; open one to read it.

replies: 30people: 4as markdown or json

Eric Sunshine· Jul 11, 2015, 00:05 UTC · lore

[PATCH 00/16] worktree: use "git reset --hard" to populate worktree

This is a follow-on series to [1], which migrated "git checkout --to" functionality to "git worktree add". That series continued using "git checkout" for the initial population of the new worktree, which required git-checkout to have too intimate knowledge that it was operating in a newly created worktree.

This series eliminates git-checkout from the picture by instead employing "git reset --hard"[2] to populate the new worktree initially.

It is built atop 1eb07d8 (worktree: add: auto-vivify new branch when <branch> is omitted, 2015-07-06), currently in 'next', which is es/worktree-add except for the final patch (which retires --ignore-other-worktrees) since the intention[3] was to drop that patch.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/273415 [2]: http://thread.gmane.org/gmane.comp.version-control.git/273415/focus=273454 [3]: http://thread.gmane.org/gmane.comp.version-control.git/273415/focus=273585

Eric Sunshine (16):
  checkout: avoid resolving HEAD unnecessarily
  checkout: name check_linked_checkouts() more meaningfully
  checkout: improve die_if_checked_out() robustness
  checkout: die_if_checked_out: simplify strbuf management
  checkout: generalize die_if_checked_out() branch name argument
  branch: publish die_if_checked_out()
  worktree: simplify new branch (-b/-B) option checking
  worktree: introduce options container
  worktree: make --detach mutually exclusive with -b/-B
  worktree: make branch creation distinct from worktree population
  worktree: add_worktree: construct worktree-population command locally
  worktree: detect branch symref/detach and error conditions locally
  worktree: make setup of new HEAD distinct from worktree population
  worktree: avoid resolving HEAD unnecessarily
  worktree: populate via "git reset --hard" rather than "git checkout"
  checkout: drop intimate knowledge of new worktree initial population
 branch.c           |  65 +++++++++++++++++++++++++++++++
 branch.h           |   7 ++++
 builtin/checkout.c |  82 +++------------------------------------
 builtin/worktree.c | 110 +++++++++++++++++++++++++++++++++++------------------
 4 files changed, 151 insertions(+), 113 deletions(-)
-- 
2.5.0.rc1.201.ga12d9f8
Eric Sunshine· Jul 11, 2015, 00:05 UTC · re: Eric Sunshine · lore

[PATCH 01/16] checkout: avoid resolving HEAD unnecessarily

When --ignore-other-worktree is specified, we unconditionally skip the check to see if the requested branch is already checked out in a linked worktree. Since we know that we will be skipping that check, there is no need to resolve HEAD in order to detect other conditions under which we may skip the check.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 builtin/checkout.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
Show changes to builtin/checkout.c +3 −3
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 5754554..75f90a9 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1145,13 +1145,13 @@ static int checkout_branch(struct checkout_opts *opts,
 		die(_("Cannot switch branch to a non-commit '%s'"),
 		    new->name);
 
-	if (new->path && !opts->force_detach && !opts->new_branch) {
+	if (new->path && !opts->force_detach && !opts->new_branch &&
+	    !opts->ignore_other_worktrees) {
 		unsigned char sha1[20];
 		int flag;
 		char *head_ref = resolve_refdup("HEAD", 0, sha1, &flag);
 		if (head_ref &&
-		    (!(flag & REF_ISSYMREF) || strcmp(head_ref, new->path)) &&
-		    !opts->ignore_other_worktrees)
+		    (!(flag & REF_ISSYMREF) || strcmp(head_ref, new->path)))
 			check_linked_checkouts(new);
 		free(head_ref);
 	}
-- 
2.5.0.rc1.201.ga12d9f8
Eric Sunshine· Jul 11, 2015, 00:05 UTC · re: Eric Sunshine · lore

[PATCH 02/16] checkout: name check_linked_checkouts() more meaningfully

check_linked_checkouts() doesn't just "check" linked checkouts for "something"; specifically, it aborts the operation if the branch about to be checked out is already checked out elsewhere. Therefore, rename it to die_if_checked_out() to give a better indication of its function. The more meaningful name will be particularly important when this function is later published for use by other callers.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 builtin/checkout.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to builtin/checkout.c +2 −2
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 75f90a9..e75fb5e 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -910,7 +910,7 @@ done:
 	strbuf_release(&gitdir);
 }
 
-static void check_linked_checkouts(struct branch_info *new)
+static void die_if_checked_out(struct branch_info *new)
 {
 	struct strbuf path = STRBUF_INIT;
 	DIR *dir;
@@ -1152,7 +1152,7 @@ static int checkout_branch(struct checkout_opts *opts,
 		char *head_ref = resolve_refdup("HEAD", 0, sha1, &flag);
 		if (head_ref &&
 		    (!(flag & REF_ISSYMREF) || strcmp(head_ref, new->path)))
-			check_linked_checkouts(new);
+			die_if_checked_out(new);
 		free(head_ref);
 	}
 
-- 
2.5.0.rc1.201.ga12d9f8
Eric Sunshine· Jul 11, 2015, 00:05 UTC · re: Eric Sunshine · lore

[PATCH 03/16] checkout: improve die_if_checked_out() robustness

die_if_checked_out() is intended to check if the branch about to be checked out is already checked out either in the main worktree or in a linked worktree. However, if .git/worktrees directory does not exist, then it never bothers checking the main worktree, even though the specified branch might indeed be checked out there, which is fragile behavior.

This hasn't been a problem in practice since the current implementation of "git worktree add" (and, earlier, "git checkout --to") always creates .git/worktrees before die_if_checked_out() is called by the child "git checkout" invocation which populates the new worktree.

However, git-worktree will eventually be retrofitted to populate the new worktree via "git reset --hard" rather than "git checkout", and it will want to use die_if_checked_out() to perform this check as well. But, the reliance upon order of operations (creating .git/worktrees before checking if a branch is already checked out) is fragile, and, as a general function, callers should not be expected to abide by this undocumented and unwarranted restriction. Therefore, make die_if_checked_out() more robust by checking the main worktree whether .git/worktrees exists or not.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 builtin/checkout.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
Show changes to builtin/checkout.c +6 −6
diff --git a/builtin/checkout.c b/builtin/checkout.c
index e75fb5e..fc8bd79 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -916,12 +916,6 @@ static void die_if_checked_out(struct branch_info *new)
 	DIR *dir;
 	struct dirent *d;
 
-	strbuf_addf(&path, "%s/worktrees", get_git_common_dir());
-	if ((dir = opendir(path.buf)) == NULL) {
-		strbuf_release(&path);
-		return;
-	}
-
 	/*
 	 * $GIT_COMMON_DIR/HEAD is practically outside
 	 * $GIT_DIR so resolve_ref_unsafe() won't work (it
@@ -929,6 +923,12 @@ static void die_if_checked_out(struct branch_info *new)
 	 */
 	check_linked_checkout(new, NULL);
 
+	strbuf_addf(&path, "%s/worktrees", get_git_common_dir());
+	if ((dir = opendir(path.buf)) == NULL) {
+		strbuf_release(&path);
+		return;
+	}
+
 	while ((d = readdir(dir)) != NULL) {
 		if (!strcmp(d->d_name, ".") || !strcmp(d->d_name, ".."))
 			continue;
-- 
2.5.0.rc1.201.ga12d9f8
Eric Sunshine· Jul 11, 2015, 00:05 UTC · re: Eric Sunshine · lore

[PATCH 04/16] checkout: die_if_checked_out: simplify strbuf management

There is no reason to keep the strbuf active long after its last use. By releasing it as early as possible, resource management is simplified and there is less worry about future changes resulting in a leak.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 builtin/checkout.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)
Show changes to builtin/checkout.c +3 −4
diff --git a/builtin/checkout.c b/builtin/checkout.c
index fc8bd79..ee33a20 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -924,17 +924,16 @@ static void die_if_checked_out(struct branch_info *new)
 	check_linked_checkout(new, NULL);
 
 	strbuf_addf(&path, "%s/worktrees", get_git_common_dir());
-	if ((dir = opendir(path.buf)) == NULL) {
-		strbuf_release(&path);
+	dir = opendir(path.buf);
+	strbuf_release(&path);
+	if (!dir)
 		return;
-	}
 
 	while ((d = readdir(dir)) != NULL) {
 		if (!strcmp(d->d_name, ".") || !strcmp(d->d_name, ".."))
 			continue;
 		check_linked_checkout(new, d->d_name);
 	}
-	strbuf_release(&path);
 	closedir(dir);
 }
 
-- 
2.5.0.rc1.201.ga12d9f8
Eric Sunshine· Jul 11, 2015, 00:05 UTC · re: Eric Sunshine · lore

[PATCH 05/16] checkout: generalize die_if_checked_out() branch name argument

The plan is to publish die_if_checked_out() so that callers other than git-checkout can take advantage of it, however, those callers won't have access to git-checkout's "struct branch_info". Therefore, change it to accept the full name of the branch as a simple string instead.

While here, also give the argument a more meaningful name ("branch" instead of "new").

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 builtin/checkout.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)
Show changes to builtin/checkout.c +8 −7
diff --git a/builtin/checkout.c b/builtin/checkout.c
index ee33a20..673daa0 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -873,7 +873,7 @@ static const char *unique_tracking_name(const char *name, unsigned char *sha1)
 	return NULL;
 }
 
-static void check_linked_checkout(struct branch_info *new, const char *id)
+static void check_linked_checkout(const char *branch, const char *id)
 {
 	struct strbuf sb = STRBUF_INIT;
 	struct strbuf path = STRBUF_INIT;
@@ -893,7 +893,7 @@ static void check_linked_checkout(struct branch_info *new, const char *id)
 	end = start;
 	while (*end && !isspace(*end))
 		end++;
-	if (strncmp(start, new->path, end - start) || new->path[end - start] != '\0')
+	if (strncmp(start, branch, end - start) || branch[end - start] != '\0')
 		goto done;
 	if (id) {
 		strbuf_reset(&path);
@@ -903,14 +903,15 @@ static void check_linked_checkout(struct branch_info *new, const char *id)
 		strbuf_rtrim(&gitdir);
 	} else
 		strbuf_addstr(&gitdir, get_git_common_dir());
-	die(_("'%s' is already checked out at '%s'"), new->name, gitdir.buf);
+	skip_prefix(branch, "refs/heads/", &branch);
+	die(_("'%s' is already checked out at '%s'"), branch, gitdir.buf);
 done:
 	strbuf_release(&path);
 	strbuf_release(&sb);
 	strbuf_release(&gitdir);
 }
 
-static void die_if_checked_out(struct branch_info *new)
+static void die_if_checked_out(const char *branch)
 {
 	struct strbuf path = STRBUF_INIT;
 	DIR *dir;
@@ -921,7 +922,7 @@ static void die_if_checked_out(struct branch_info *new)
 	 * $GIT_DIR so resolve_ref_unsafe() won't work (it
 	 * uses git_path). Parse the ref ourselves.
 	 */
-	check_linked_checkout(new, NULL);
+	check_linked_checkout(branch, NULL);
 
 	strbuf_addf(&path, "%s/worktrees", get_git_common_dir());
 	dir = opendir(path.buf);
@@ -932,7 +933,7 @@ static void die_if_checked_out(struct branch_info *new)
 	while ((d = readdir(dir)) != NULL) {
 		if (!strcmp(d->d_name, ".") || !strcmp(d->d_name, ".."))
 			continue;
-		check_linked_checkout(new, d->d_name);
+		check_linked_checkout(branch, d->d_name);
 	}
 	closedir(dir);
 }
@@ -1151,7 +1152,7 @@ static int checkout_branch(struct checkout_opts *opts,
 		char *head_ref = resolve_refdup("HEAD", 0, sha1, &flag);
 		if (head_ref &&
 		    (!(flag & REF_ISSYMREF) || strcmp(head_ref, new->path)))
-			die_if_checked_out(new);
+			die_if_checked_out(new->path);
 		free(head_ref);
 	}
 
-- 
2.5.0.rc1.201.ga12d9f8
Eric Sunshine· Jul 11, 2015, 00:05 UTC · re: Eric Sunshine · lore

[PATCH 06/16] branch: publish die_if_checked_out()

git-worktree will eventually be retrofitted to populate the new worktree via "git reset --hard" rather than "git checkout", at which time it will need to be able to detect if the branch is already checked out elsewhere, rather than relying upon git-branch to make this determination, so publish die_if_checked_out().

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 branch.c           | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 branch.h           |  7 ++++++
 builtin/checkout.c | 65 ------------------------------------------------------
 3 files changed, 72 insertions(+), 65 deletions(-)
Show changes to 3 files +72 −65

branch.c, branch.h, builtin/checkout.c

diff --git a/branch.c b/branch.c
index 4bab55a..7b8b9a3 100644
--- a/branch.c
+++ b/branch.c
@@ -309,3 +309,68 @@ void remove_branch_state(void)
 	unlink(git_path("MERGE_MODE"));
 	unlink(git_path("SQUASH_MSG"));
 }
+
+static void check_linked_checkout(const char *branch, const char *id)
+{
+	struct strbuf sb = STRBUF_INIT;
+	struct strbuf path = STRBUF_INIT;
+	struct strbuf gitdir = STRBUF_INIT;
+	const char *start, *end;
+
+	if (id)
+		strbuf_addf(&path, "%s/worktrees/%s/HEAD", get_git_common_dir(), id);
+	else
+		strbuf_addf(&path, "%s/HEAD", get_git_common_dir());
+
+	if (strbuf_read_file(&sb, path.buf, 0) < 0 ||
+	    !skip_prefix(sb.buf, "ref:", &start))
+		goto done;
+	while (isspace(*start))
+		start++;
+	end = start;
+	while (*end && !isspace(*end))
+		end++;
+	if (strncmp(start, branch, end - start) || branch[end - start] != '\0')
+		goto done;
+	if (id) {
+		strbuf_reset(&path);
+		strbuf_addf(&path, "%s/worktrees/%s/gitdir", get_git_common_dir(), id);
+		if (strbuf_read_file(&gitdir, path.buf, 0) <= 0)
+			goto done;
+		strbuf_rtrim(&gitdir);
+	} else
+		strbuf_addstr(&gitdir, get_git_common_dir());
+	skip_prefix(branch, "refs/heads/", &branch);
+	die(_("'%s' is already checked out at '%s'"), branch, gitdir.buf);
+done:
+	strbuf_release(&path);
+	strbuf_release(&sb);
+	strbuf_release(&gitdir);
+}
+
+void die_if_checked_out(const char *branch)
+{
+	struct strbuf path = STRBUF_INIT;
+	DIR *dir;
+	struct dirent *d;
+
+	/*
+	 * $GIT_COMMON_DIR/HEAD is practically outside
+	 * $GIT_DIR so resolve_ref_unsafe() won't work (it
+	 * uses git_path). Parse the ref ourselves.
+	 */
+	check_linked_checkout(branch, NULL);
+
+	strbuf_addf(&path, "%s/worktrees", get_git_common_dir());
+	dir = opendir(path.buf);
+	strbuf_release(&path);
+	if (!dir)
+		return;
+
+	while ((d = readdir(dir)) != NULL) {
+		if (!strcmp(d->d_name, ".") || !strcmp(d->d_name, ".."))
+			continue;
+		check_linked_checkout(branch, d->d_name);
+	}
+	closedir(dir);
+}
diff --git a/branch.h b/branch.h
index 64173ab..58aa45f 100644
--- a/branch.h
+++ b/branch.h
@@ -52,4 +52,11 @@ extern void install_branch_config(int flag, const char *local, const char *origi
  */
 extern int read_branch_desc(struct strbuf *, const char *branch_name);
 
+/*
+ * Check if a branch is checked out in the main worktree or any linked
+ * worktree and die (with a message describing its checkout location) if
+ * it is.
+ */
+extern void die_if_checked_out(const char *branch);
+
 #endif
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 673daa0..4ae895c 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -873,71 +873,6 @@ static const char *unique_tracking_name(const char *name, unsigned char *sha1)
 	return NULL;
 }
 
-static void check_linked_checkout(const char *branch, const char *id)
-{
-	struct strbuf sb = STRBUF_INIT;
-	struct strbuf path = STRBUF_INIT;
-	struct strbuf gitdir = STRBUF_INIT;
-	const char *start, *end;
-
-	if (id)
-		strbuf_addf(&path, "%s/worktrees/%s/HEAD", get_git_common_dir(), id);
-	else
-		strbuf_addf(&path, "%s/HEAD", get_git_common_dir());
-
-	if (strbuf_read_file(&sb, path.buf, 0) < 0 ||
-	    !skip_prefix(sb.buf, "ref:", &start))
-		goto done;
-	while (isspace(*start))
-		start++;
-	end = start;
-	while (*end && !isspace(*end))
-		end++;
-	if (strncmp(start, branch, end - start) || branch[end - start] != '\0')
-		goto done;
-	if (id) {
-		strbuf_reset(&path);
-		strbuf_addf(&path, "%s/worktrees/%s/gitdir", get_git_common_dir(), id);
-		if (strbuf_read_file(&gitdir, path.buf, 0) <= 0)
-			goto done;
-		strbuf_rtrim(&gitdir);
-	} else
-		strbuf_addstr(&gitdir, get_git_common_dir());
-	skip_prefix(branch, "refs/heads/", &branch);
-	die(_("'%s' is already checked out at '%s'"), branch, gitdir.buf);
-done:
-	strbuf_release(&path);
-	strbuf_release(&sb);
-	strbuf_release(&gitdir);
-}
-
-static void die_if_checked_out(const char *branch)
-{
-	struct strbuf path = STRBUF_INIT;
-	DIR *dir;
-	struct dirent *d;
-
-	/*
-	 * $GIT_COMMON_DIR/HEAD is practically outside
-	 * $GIT_DIR so resolve_ref_unsafe() won't work (it
-	 * uses git_path). Parse the ref ourselves.
-	 */
-	check_linked_checkout(branch, NULL);
-
-	strbuf_addf(&path, "%s/worktrees", get_git_common_dir());
-	dir = opendir(path.buf);
-	strbuf_release(&path);
-	if (!dir)
-		return;
-
-	while ((d = readdir(dir)) != NULL) {
-		if (!strcmp(d->d_name, ".") || !strcmp(d->d_name, ".."))
-			continue;
-		check_linked_checkout(branch, d->d_name);
-	}
-	closedir(dir);
-}
-
 static int parse_branchname_arg(int argc, const char **argv,
 				int dwim_new_local_branch_ok,
 				struct branch_info *new,
-- 
2.5.0.rc1.201.ga12d9f8
Eric Sunshine· Jul 11, 2015, 00:05 UTC · re: Eric Sunshine · lore

[PATCH 07/16] worktree: simplify new branch (-b/-B) option checking

Make 'new_branch' be the name of the new branch for both forced and non-forced cases; and add boolean 'force_new_branch' to indicate forced branch creation. This will simplify logic later on when git-worktree handles branch creation locally rather than delegating it to git-checkout as part of the worktree population phase.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 builtin/worktree.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)
Show changes to builtin/worktree.c +8 −5
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 69248ba..c267410 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -272,7 +272,7 @@ static int add_worktree(const char *path, const char **child_argv)
 
 static int add(int ac, const char **av, const char *prefix)
 {
-	int force = 0, detach = 0;
+	int force = 0, detach = 0, force_new_branch;
 	const char *new_branch = NULL, *new_branch_force = NULL;
 	const char *path, *branch;
 	struct argv_array cmd = ARGV_ARRAY_INIT;
@@ -295,7 +295,11 @@ static int add(int ac, const char **av, const char *prefix)
 	path = prefix ? prefix_filename(prefix, strlen(prefix), av[0]) : av[0];
 	branch = ac < 2 ? "HEAD" : av[1];
 
-	if (ac < 2 && !new_branch && !new_branch_force) {
+	force_new_branch = !!new_branch_force;
+	if (force_new_branch)
+		new_branch = new_branch_force;
+
+	if (ac < 2 && !new_branch) {
 		int n;
 		const char *s = worktree_basename(path, &n);
 		new_branch = xstrndup(s, n);
@@ -305,9 +309,8 @@ static int add(int ac, const char **av, const char *prefix)
 	if (force)
 		argv_array_push(&cmd, "--ignore-other-worktrees");
 	if (new_branch)
-		argv_array_pushl(&cmd, "-b", new_branch, NULL);
-	if (new_branch_force)
-		argv_array_pushl(&cmd, "-B", new_branch_force, NULL);
+		argv_array_pushl(&cmd, force_new_branch ? "-B" : "-b",
+				 new_branch, NULL);
 	if (detach)
 		argv_array_push(&cmd, "--detach");
 	argv_array_push(&cmd, branch);
-- 
2.5.0.rc1.201.ga12d9f8
Eric Sunshine· Jul 11, 2015, 00:05 UTC · re: Eric Sunshine · lore

[PATCH 08/16] worktree: introduce options container

add_worktree() will eventually need to deal with some options itself, so introduce a structure into which options can be conveniently bundled, and pass it along to add_worktree().

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 builtin/worktree.c | 45 +++++++++++++++++++++++++++------------------
 1 file changed, 27 insertions(+), 18 deletions(-)
Show changes to builtin/worktree.c +27 −18
diff --git a/builtin/worktree.c b/builtin/worktree.c
index c267410..253382a 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -12,6 +12,13 @@ static const char * const worktree_usage[] = {
 	NULL
 };
 
+struct add_opts {
+	int force;
+	int detach;
+	const char *new_branch;
+	int force_new_branch;
+};
+
 static int show_only;
 static int verbose;
 static unsigned long expire;
@@ -171,7 +178,8 @@ static const char *worktree_basename(const char *path, int *olen)
 	return name;
 }
 
-static int add_worktree(const char *path, const char **child_argv)
+static int add_worktree(const char *path, const char **child_argv,
+			const struct add_opts *opts)
 {
 	struct strbuf sb_git = STRBUF_INIT, sb_repo = STRBUF_INIT;
 	struct strbuf sb = STRBUF_INIT;
@@ -272,22 +280,23 @@ static int add_worktree(const char *path, const char **child_argv)
 
 static int add(int ac, const char **av, const char *prefix)
 {
-	int force = 0, detach = 0, force_new_branch;
-	const char *new_branch = NULL, *new_branch_force = NULL;
+	struct add_opts opts;
+	const char *new_branch_force = NULL;
 	const char *path, *branch;
 	struct argv_array cmd = ARGV_ARRAY_INIT;
 	struct option options[] = {
-		OPT__FORCE(&force, N_("checkout <branch> even if already checked out in other worktree")),
-		OPT_STRING('b', NULL, &new_branch, N_("branch"),
+		OPT__FORCE(&opts.force, N_("checkout <branch> even if already checked out in other worktree")),
+		OPT_STRING('b', NULL, &opts.new_branch, N_("branch"),
 			   N_("create a new branch")),
 		OPT_STRING('B', NULL, &new_branch_force, N_("branch"),
 			   N_("create or reset a branch")),
-		OPT_BOOL(0, "detach", &detach, N_("detach HEAD at named commit")),
+		OPT_BOOL(0, "detach", &opts.detach, N_("detach HEAD at named commit")),
 		OPT_END()
 	};
 
+	memset(&opts, 0, sizeof(opts));
 	ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
-	if (new_branch && new_branch_force)
+	if (opts.new_branch && new_branch_force)
 		die(_("-b and -B are mutually exclusive"));
 	if (ac < 1 || ac > 2)
 		usage_with_options(worktree_usage, options);
@@ -295,27 +304,27 @@ static int add(int ac, const char **av, const char *prefix)
 	path = prefix ? prefix_filename(prefix, strlen(prefix), av[0]) : av[0];
 	branch = ac < 2 ? "HEAD" : av[1];
 
-	force_new_branch = !!new_branch_force;
-	if (force_new_branch)
-		new_branch = new_branch_force;
+	opts.force_new_branch = !!new_branch_force;
+	if (opts.force_new_branch)
+		opts.new_branch = new_branch_force;
 
-	if (ac < 2 && !new_branch) {
+	if (ac < 2 && !opts.new_branch) {
 		int n;
 		const char *s = worktree_basename(path, &n);
-		new_branch = xstrndup(s, n);
+		opts.new_branch = xstrndup(s, n);
 	}
 
 	argv_array_push(&cmd, "checkout");
-	if (force)
+	if (opts.force)
 		argv_array_push(&cmd, "--ignore-other-worktrees");
-	if (new_branch)
-		argv_array_pushl(&cmd, force_new_branch ? "-B" : "-b",
-				 new_branch, NULL);
-	if (detach)
+	if (opts.new_branch)
+		argv_array_pushl(&cmd, opts.force_new_branch ? "-B" : "-b",
+				 opts.new_branch, NULL);
+	if (opts.detach)
 		argv_array_push(&cmd, "--detach");
 	argv_array_push(&cmd, branch);
 
-	return add_worktree(path, cmd.argv);
+	return add_worktree(path, cmd.argv, &opts);
 }
 
 int cmd_worktree(int ac, const char **av, const char *prefix)
-- 
2.5.0.rc1.201.ga12d9f8
Eric Sunshine· Jul 11, 2015, 00:05 UTC · re: Eric Sunshine · lore

[PATCH 09/16] worktree: make --detach mutually exclusive with -b/-B

Be consistent with git-checkout which disallows this (not particularly meaningful) combination.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 builtin/worktree.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to builtin/worktree.c +2 −2
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 253382a..cd06bf5 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -296,8 +296,8 @@ static int add(int ac, const char **av, const char *prefix)
 
 	memset(&opts, 0, sizeof(opts));
 	ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
-	if (opts.new_branch && new_branch_force)
-		die(_("-b and -B are mutually exclusive"));
+	if (!!opts.detach + !!opts.new_branch + !!new_branch_force > 1)
+		die(_("-b, -B, and --detach are mutually exclusive"));
 	if (ac < 1 || ac > 2)
 		usage_with_options(worktree_usage, options);
 
-- 
2.5.0.rc1.201.ga12d9f8
Eric Sunshine· Jul 11, 2015, 00:05 UTC · re: Eric Sunshine · lore

[PATCH 10/16] worktree: make branch creation distinct from worktree population

The plan is eventually to populate the new worktree via "git reset --hard" rather than "git checkout". Thus, rather than piggybacking on git-checkout's -b/-B ability to create a new branch at checkout time, git-worktree will need to do so itself.

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

I considered calling branch-related API, such as create_branch(), directly, however, git-branch provides extra value which may be useful in the future (such as when --track and --orphan get added to git-worktree), so it seemed wise to re-use git-branch's implementation rather than duplicating the functionality. If this proves the wrong choice, then the series can either be re-rolled or follow-on patches can address the issue.

 builtin/worktree.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)
Show changes to builtin/worktree.c +14 −3
diff --git a/builtin/worktree.c b/builtin/worktree.c
index cd06bf5..9be1c74 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -314,12 +314,23 @@ static int add(int ac, const char **av, const char *prefix)
 		opts.new_branch = xstrndup(s, n);
 	}
 
+	if (opts.new_branch) {
+		struct child_process cp;
+		memset(&cp, 0, sizeof(cp));
+		cp.git_cmd = 1;
+		argv_array_push(&cp.args, "branch");
+		if (opts.force_new_branch)
+			argv_array_push(&cp.args, "--force");
+		argv_array_push(&cp.args, opts.new_branch);
+		argv_array_push(&cp.args, branch);
+		if (run_command(&cp))
+			return -1;
+		branch = opts.new_branch;
+	}
+
 	argv_array_push(&cmd, "checkout");
 	if (opts.force)
 		argv_array_push(&cmd, "--ignore-other-worktrees");
-	if (opts.new_branch)
-		argv_array_pushl(&cmd, opts.force_new_branch ? "-B" : "-b",
-				 opts.new_branch, NULL);
 	if (opts.detach)
 		argv_array_push(&cmd, "--detach");
 	argv_array_push(&cmd, branch);
-- 
2.5.0.rc1.201.ga12d9f8
Duy Nguyen· Jul 12, 2015, 01:20 UTC · re: Eric Sunshine · lore

Re: [PATCH 10/16] worktree: make branch creation distinct from worktree population

On Sat, Jul 11, 2015 at 7:05 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 15 quoted lines
> The plan is eventually to populate the new worktree via "git reset
> --hard" rather than "git checkout". Thus, rather than piggybacking on
> git-checkout's -b/-B ability to create a new branch at checkout time,
> git-worktree will need to do so itself.
>
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> ---
>
> I considered calling branch-related API, such as create_branch(),
> directly, however, git-branch provides extra value which may be useful
> in the future (such as when --track and --orphan get added to
> git-worktree), so it seemed wise to re-use git-branch's implementation
> rather than duplicating the functionality. If this proves the wrong
> choice, then the series can either be re-rolled or follow-on patches can
> address the issue.

I don't know much about ref handling code (especially after the big transaction update), so i may be wrong, but do we need to invalidate some caches in refs.c after this? The same for update-ref in the other patch. We may need to re-read the index after 'reset --hard' too if we ever need to do touch the index after that (unlikely though in the case of 'worktree add')

-- 
Duy
Eric Sunshine· Jul 12, 2015, 02:36 UTC · re: Duy Nguyen · lore

Re: [PATCH 10/16] worktree: make branch creation distinct from worktree population

On Sat, Jul 11, 2015 at 9:20 PM, Duy Nguyen <pclouds@gmail.com> wrote:
Show 23 quoted lines
> On Sat, Jul 11, 2015 at 7:05 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> The plan is eventually to populate the new worktree via "git reset
>> --hard" rather than "git checkout". Thus, rather than piggybacking on
>> git-checkout's -b/-B ability to create a new branch at checkout time,
>> git-worktree will need to do so itself.
>>
>> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
>> ---
>>
>> I considered calling branch-related API, such as create_branch(),
>> directly, however, git-branch provides extra value which may be useful
>> in the future (such as when --track and --orphan get added to
>> git-worktree), so it seemed wise to re-use git-branch's implementation
>> rather than duplicating the functionality. If this proves the wrong
>> choice, then the series can either be re-rolled or follow-on patches can
>> address the issue.
>
> I don't know much about ref handling code (especially after the big
> transaction update), so i may be wrong, but do we need to invalidate
> some caches in refs.c after this? The same for update-ref in the other
> patch. We may need to re-read the index after 'reset --hard' too if we
> ever need to do touch the index after that (unlikely though in the
> case of 'worktree add')

I'm not sure I understand. Are you talking about this patch's implementation or a possible future change which uses the C API rather than git-branch?

If you're talking about this patch, then I don't think we need to do anything more, as the "git branch" and "git reset --hard" invocations are separate process invocations which shouldn't affect the current worktree or the current "git worktree add" process.

If you're talking about a future patch switching over to the C API, and avoiding run_command(), then perhaps (but I haven't investigated that avenue thoroughly enough to answer).

Or, am I totally misunderstanding you?
Duy Nguyen· Jul 12, 2015, 02:48 UTC · re: Eric Sunshine · lore

Re: [PATCH 10/16] worktree: make branch creation distinct from worktree population

On Sun, Jul 12, 2015 at 9:36 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 33 quoted lines
> On Sat, Jul 11, 2015 at 9:20 PM, Duy Nguyen <pclouds@gmail.com> wrote:
>> On Sat, Jul 11, 2015 at 7:05 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> The plan is eventually to populate the new worktree via "git reset
>>> --hard" rather than "git checkout". Thus, rather than piggybacking on
>>> git-checkout's -b/-B ability to create a new branch at checkout time,
>>> git-worktree will need to do so itself.
>>>
>>> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
>>> ---
>>>
>>> I considered calling branch-related API, such as create_branch(),
>>> directly, however, git-branch provides extra value which may be useful
>>> in the future (such as when --track and --orphan get added to
>>> git-worktree), so it seemed wise to re-use git-branch's implementation
>>> rather than duplicating the functionality. If this proves the wrong
>>> choice, then the series can either be re-rolled or follow-on patches can
>>> address the issue.
>>
>> I don't know much about ref handling code (especially after the big
>> transaction update), so i may be wrong, but do we need to invalidate
>> some caches in refs.c after this? The same for update-ref in the other
>> patch. We may need to re-read the index after 'reset --hard' too if we
>> ever need to do touch the index after that (unlikely though in the
>> case of 'worktree add')
>
> I'm not sure I understand. Are you talking about this patch's
> implementation or a possible future change which uses the C API rather
> than git-branch?
>
> If you're talking about this patch, then I don't think we need to do
> anything more, as the "git branch" and "git reset --hard" invocations
> are separate process invocations which shouldn't affect the current
> worktree or the current "git worktree add" process.

The "shouldn't affect" is potentially a problem.If the current 'worktree add' process caches something (in ref handling, for example) that the 'git branch' process changes, then we may need to invalidate cache in 'worktree add' process after run_command(). I guess it's ok in this case since all we do is run_command(), we do not lookup refs or anything else afterwards.

-- 
Duy
Eric Sunshine· Jul 12, 2015, 03:10 UTC · re: Duy Nguyen · lore

Re: [PATCH 10/16] worktree: make branch creation distinct from worktree population

On Sat, Jul 11, 2015 at 10:48 PM, Duy Nguyen <pclouds@gmail.com> wrote:
Show 30 quoted lines
> On Sun, Jul 12, 2015 at 9:36 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Sat, Jul 11, 2015 at 9:20 PM, Duy Nguyen <pclouds@gmail.com> wrote:
>>> On Sat, Jul 11, 2015 at 7:05 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>>> The plan is eventually to populate the new worktree via "git reset
>>>> --hard" rather than "git checkout". Thus, rather than piggybacking on
>>>> git-checkout's -b/-B ability to create a new branch at checkout time,
>>>> git-worktree will need to do so itself.
>>>
>>> I don't know much about ref handling code (especially after the big
>>> transaction update), so i may be wrong, but do we need to invalidate
>>> some caches in refs.c after this? The same for update-ref in the other
>>> patch. We may need to re-read the index after 'reset --hard' too if we
>>> ever need to do touch the index after that (unlikely though in the
>>> case of 'worktree add')
>>
>> I'm not sure I understand. Are you talking about this patch's
>> implementation or a possible future change which uses the C API rather
>> than git-branch?
>>
>> If you're talking about this patch, then I don't think we need to do
>> anything more, as the "git branch" and "git reset --hard" invocations
>> are separate process invocations which shouldn't affect the current
>> worktree or the current "git worktree add" process.
>
> The "shouldn't affect" is potentially a problem.If the current
> 'worktree add' process caches something (in ref handling, for example)
> that the 'git branch' process changes, then we may need to invalidate
> cache in 'worktree add' process after run_command(). I guess it's ok
> in this case since all we do is run_command(), we do not lookup refs
> or anything else afterwards.
With this patch series applied, the code effectively does this:
    branch = ...
    if (create_new_branch) {
        exec "git branch newbranch branch"
        branch = newbranch;
    }
    if (ref_exists(branch) && !detach)
        exec "git symbolic-ref HEAD branch"
    else
        exec "git update-ref HEAD $(git rev-parse branch)"
    exec "git reset --hard"

So, if I understand your concern correctly, then you are worried that, following the git-branch invocation, ref_exists() could return the wrong answer with a pluggable ref-backend since it might be answering based upon stale information. Is that what you mean? If so, I can see how that it could be an issue. (As far as I can tell, the current file-based backend doesn't have a problem with this since it's hitting the filesystem directly to answer the ref_exists() question.)

Eric Sunshine· Jul 12, 2015, 03:14 UTC · re: Eric Sunshine · lore

Re: [PATCH 10/16] worktree: make branch creation distinct from worktree population

On Sat, Jul 11, 2015 at 11:10 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 28 quoted lines
> On Sat, Jul 11, 2015 at 10:48 PM, Duy Nguyen <pclouds@gmail.com> wrote:
>> The "shouldn't affect" is potentially a problem.If the current
>> 'worktree add' process caches something (in ref handling, for example)
>> that the 'git branch' process changes, then we may need to invalidate
>> cache in 'worktree add' process after run_command(). I guess it's ok
>> in this case since all we do is run_command(), we do not lookup refs
>> or anything else afterwards.
>
> With this patch series applied, the code effectively does this:
>
>     branch = ...
>     if (create_new_branch) {
>         exec "git branch newbranch branch"
>         branch = newbranch;
>     }
>     if (ref_exists(branch) && !detach)
>         exec "git symbolic-ref HEAD branch"
>     else
>         exec "git update-ref HEAD $(git rev-parse branch)"
>     exec "git reset --hard"
>
> So, if I understand your concern correctly, then you are worried that,
> following the git-branch invocation, ref_exists() could return the
> wrong answer with a pluggable ref-backend since it might be answering
> based upon stale information. Is that what you mean? If so, I can see
> how that it could be an issue. (As far as I can tell, the current
> file-based backend doesn't have a problem with this since it's hitting
> the filesystem directly to answer the ref_exists() question.)
I meant for this final sentence to end like this:
     ...to answer the ref_exists() question, but it still seems
     fragile since some future change could introduce caching.
Eric Sunshine· Jul 12, 2015, 03:27 UTC · re: Eric Sunshine · lore

Re: [PATCH 10/16] worktree: make branch creation distinct from worktree population

On Sat, Jul 11, 2015 at 11:14 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 34 quoted lines
> On Sat, Jul 11, 2015 at 11:10 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Sat, Jul 11, 2015 at 10:48 PM, Duy Nguyen <pclouds@gmail.com> wrote:
>>> The "shouldn't affect" is potentially a problem.If the current
>>> 'worktree add' process caches something (in ref handling, for example)
>>> that the 'git branch' process changes, then we may need to invalidate
>>> cache in 'worktree add' process after run_command(). I guess it's ok
>>> in this case since all we do is run_command(), we do not lookup refs
>>> or anything else afterwards.
>>
>> With this patch series applied, the code effectively does this:
>>
>>     branch = ...
>>     if (create_new_branch) {
>>         exec "git branch newbranch branch"
>>         branch = newbranch;
>>     }
>>     if (ref_exists(branch) && !detach)
>>         exec "git symbolic-ref HEAD branch"
>>     else
>>         exec "git update-ref HEAD $(git rev-parse branch)"
>>     exec "git reset --hard"
>>
>> So, if I understand your concern correctly, then you are worried that,
>> following the git-branch invocation, ref_exists() could return the
>> wrong answer with a pluggable ref-backend since it might be answering
>> based upon stale information. Is that what you mean? If so, I can see
>> how that it could be an issue. (As far as I can tell, the current
>> file-based backend doesn't have a problem with this since it's hitting
>> the filesystem directly to answer the ref_exists() question.)
>
> I meant for this final sentence to end like this:
>
>      ...to answer the ref_exists() question, but it still seems
>      fragile since some future change could introduce caching.

In this case, it's easy enough to side-step the issue since there's no need to call ref_exists() if the new branch was created successfully (since we know it exists). The logic would effectively become this:

    branch = ...
    if (create_new_branch) {
        exec "git branch newbranch branch"
        exec "git symbolic-ref HEAD newbranch"
    } else if (ref_exists(branch) && !detach)
        exec "git symbolic-ref HEAD branch"
    else
        exec "git update-ref HEAD $(git rev-parse branch)"
    exec "git reset --hard"
Duy Nguyen· Jul 12, 2015, 10:03 UTC · re: Eric Sunshine · lore

Re: [PATCH 10/16] worktree: make branch creation distinct from worktree population

(resend, +everybody)
On Sun, Jul 12, 2015 at 10:27 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 26 quoted lines
>>> So, if I understand your concern correctly, then you are worried that,
>>> following the git-branch invocation, ref_exists() could return the
>>> wrong answer with a pluggable ref-backend since it might be answering
>>> based upon stale information. Is that what you mean? If so, I can see
>>> how that it could be an issue. (As far as I can tell, the current
>>> file-based backend doesn't have a problem with this since it's hitting
>>> the filesystem directly to answer the ref_exists() question.)
>>
>> I meant for this final sentence to end like this:
>>
>>      ...to answer the ref_exists() question, but it still seems
>>      fragile since some future change could introduce caching.
>
> In this case, it's easy enough to side-step the issue since there's no
> need to call ref_exists() if the new branch was created successfully
> (since we know it exists). The logic would effectively become this:
>
>     branch = ...
>     if (create_new_branch) {
>         exec "git branch newbranch branch"
>         exec "git symbolic-ref HEAD newbranch"
>     } else if (ref_exists(branch) && !detach)
>         exec "git symbolic-ref HEAD branch"
>     else
>         exec "git update-ref HEAD $(git rev-parse branch)"
>     exec "git reset --hard"

Yeah.. Another option we can take is deal with this at run-command.c level (and outside this series) because this could affect everywhere: by default, invalidate all cache after running any git commands. The caller can pass options to keep some cache intact if they know the command won't touch it.

If ref_exists() is the only thing we use, right now it does not use cache so we should be safe. If the new ref backend introduces a cache, they would need to examine all callers anyway, including this one. The cache in refs.c seems to be for for_each_ref.. only, which we don't call here.

-- 
Duy
Eric Sunshine· Jul 11, 2015, 00:05 UTC · re: Eric Sunshine · lore

[PATCH 11/16] worktree: add_worktree: construct worktree-population command locally

The caller of add_worktree() provides it with a command to invoke to populate the new worktree. This was a useful abstraction during the conversion of "git checkout --to" functionality to "git worktree add" since git-checkout and git-worktree constructed the population command differently. However, now that "git checkout --to" has been retired, and add_worktree() has access to the options given to "worktree add", this extra indirection is no longer useful, and makes the code a bit more convoluted than it needs to be.

Moreover, the eventual goal is for git-worktree to populate the new worktree via "git reset --hard" rather than "git checkout", and to do so, it may need to invoke other commands in addition to the worktree-population command, so it will be doing command construction itself anyhow. Therefore, relocate construction of the worktree-population command from add() to add_worktree().

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 builtin/worktree.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)
Show changes to builtin/worktree.c +8 −11
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 9be1c74..e04a6d1 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -178,7 +178,7 @@ static const char *worktree_basename(const char *path, int *olen)
 	return name;
 }
 
-static int add_worktree(const char *path, const char **child_argv,
+static int add_worktree(const char *path, const char *refname,
 			const struct add_opts *opts)
 {
 	struct strbuf sb_git = STRBUF_INIT, sb_repo = STRBUF_INIT;
@@ -260,7 +260,12 @@ static int add_worktree(const char *path, const char **child_argv,
 	setenv(GIT_WORK_TREE_ENVIRONMENT, path, 1);
 	memset(&cp, 0, sizeof(cp));
 	cp.git_cmd = 1;
-	cp.argv = child_argv;
+	argv_array_push(&cp.args, "checkout");
+	if (opts->force)
+		argv_array_push(&cp.args, "--ignore-other-worktrees");
+	if (opts->detach)
+		argv_array_push(&cp.args, "--detach");
+	argv_array_push(&cp.args, refname);
 	ret = run_command(&cp);
 	if (!ret) {
 		is_junk = 0;
@@ -283,7 +288,6 @@ static int add(int ac, const char **av, const char *prefix)
 	struct add_opts opts;
 	const char *new_branch_force = NULL;
 	const char *path, *branch;
-	struct argv_array cmd = ARGV_ARRAY_INIT;
 	struct option options[] = {
 		OPT__FORCE(&opts.force, N_("checkout <branch> even if already checked out in other worktree")),
 		OPT_STRING('b', NULL, &opts.new_branch, N_("branch"),
@@ -328,14 +332,7 @@ static int add(int ac, const char **av, const char *prefix)
 		branch = opts.new_branch;
 	}
 
-	argv_array_push(&cmd, "checkout");
-	if (opts.force)
-		argv_array_push(&cmd, "--ignore-other-worktrees");
-	if (opts.detach)
-		argv_array_push(&cmd, "--detach");
-	argv_array_push(&cmd, branch);
-
-	return add_worktree(path, cmd.argv, &opts);
+	return add_worktree(path, branch, &opts);
 }
 
 int cmd_worktree(int ac, const char **av, const char *prefix)
-- 
2.5.0.rc1.201.ga12d9f8
Eric Sunshine· Jul 11, 2015, 00:05 UTC · re: Eric Sunshine · lore

[PATCH 12/16] worktree: detect branch symref/detach and error conditions locally

The eventual goal is for git-worktree to populate the new worktree via "git reset --hard" rather than "git checkout". As a consequence, git-worktree will no longer be able to rely upon git-branch to determine the state of the worktree (detached or branch symref), or to check for error conditions, such as the requested branch already checked out elsewhere, or an invalid reference. Therefore, imbue git-worktree with the intelligence to determine a branch symref or detached state locally, and to perform error checking on its own.

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

This patch is somewhat RFC since I spent a lot of time browsing the branch- and ref-related APIs figuring out how to determine if the provided refname was a branch name or some other (detached) reference. I'm not at all sure that my use of strbuf_check_branch_ref(), ref_exists(), and lookup_commit_reference_by_name() is the best approach, or even appropriate, although it seems to work as desired.

 builtin/worktree.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)
Show changes to builtin/worktree.c +15 −0
diff --git a/builtin/worktree.c b/builtin/worktree.c
index e04a6d1..babdef1 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -3,6 +3,8 @@
 #include "dir.h"
 #include "parse-options.h"
 #include "argv-array.h"
+#include "branch.h"
+#include "refs.h"
 #include "run-command.h"
 #include "sigchain.h"
 
@@ -187,11 +189,23 @@ static int add_worktree(const char *path, const char *refname,
 	struct stat st;
 	struct child_process cp;
 	int counter = 0, len, ret;
+	struct strbuf symref = STRBUF_INIT;
+	struct commit *commit = NULL;
 	unsigned char rev[20];
 
 	if (file_exists(path) && !is_empty_dir(path))
 		die(_("'%s' already exists"), path);
 
+	if (!opts->detach && !strbuf_check_branch_ref(&symref, refname) &&
+	    ref_exists(symref.buf)) {
+		if (!opts->force)
+			die_if_checked_out(symref.buf);
+	} else {
+		commit = lookup_commit_reference_by_name(refname);
+		if (!commit)
+			die(_("invalid reference: %s"), refname);
+	}
+
 	name = worktree_basename(path, &len);
 	strbuf_addstr(&sb_repo,
 		      git_path("worktrees/%.*s", (int)(path + len - name), name));
@@ -278,6 +292,7 @@ static int add_worktree(const char *path, const char *refname,
 	strbuf_addf(&sb, "%s/locked", sb_repo.buf);
 	unlink_or_warn(sb.buf);
 	strbuf_release(&sb);
+	strbuf_release(&symref);
 	strbuf_release(&sb_repo);
 	strbuf_release(&sb_git);
 	return ret;
-- 
2.5.0.rc1.201.ga12d9f8
Eric Sunshine· Jul 11, 2015, 00:05 UTC · re: Eric Sunshine · lore

The eventual goal is for git-worktree to populate the new worktree via "git reset --hard" rather than "git checkout". As a consequence, git-worktree will no longer be able to delegate to git-branch the setting of the new worktree's HEAD to the desired branch (or commit, if detached). Therefore, make git-worktree responsible for setting up HEAD as either a symbolic reference if associated with a branch, or detached if not.

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

As with the patch which separates branch creation from worktree population (by using git-branch instead of "git checkout -b"), this patch invokes git-symbolic-ref and git-update-ref rather than using the lower-level C API, due to the added value of those commands. If this proves the wrong approach, then the series can be re-rolled or follow-on patches can instead use the C API.

 builtin/worktree.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)
Show changes to builtin/worktree.c +14 −5
diff --git a/builtin/worktree.c b/builtin/worktree.c
index babdef1..94c1701 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -274,12 +274,20 @@ static int add_worktree(const char *path, const char *refname,
 	setenv(GIT_WORK_TREE_ENVIRONMENT, path, 1);
 	memset(&cp, 0, sizeof(cp));
 	cp.git_cmd = 1;
+
+	if (commit)
+		argv_array_pushl(&cp.args, "update-ref", "HEAD",
+				 sha1_to_hex(commit->object.sha1), NULL);
+	else
+		argv_array_pushl(&cp.args, "symbolic-ref", "HEAD",
+				 symref.buf, NULL);
+	ret = run_command(&cp);
+	if (ret)
+		goto done;
+
+	cp.argv = NULL;
+	argv_array_clear(&cp.args);
 	argv_array_push(&cp.args, "checkout");
-	if (opts->force)
-		argv_array_push(&cp.args, "--ignore-other-worktrees");
-	if (opts->detach)
-		argv_array_push(&cp.args, "--detach");
-	argv_array_push(&cp.args, refname);
 	ret = run_command(&cp);
 	if (!ret) {
 		is_junk = 0;
@@ -288,6 +296,7 @@ static int add_worktree(const char *path, const char *refname,
 		junk_work_tree = NULL;
 		junk_git_dir = NULL;
 	}
+done:
 	strbuf_reset(&sb);
 	strbuf_addf(&sb, "%s/locked", sb_repo.buf);
 	unlink_or_warn(sb.buf);
-- 
2.5.0.rc1.201.ga12d9f8
Eric Sunshine· Jul 11, 2015, 00:05 UTC · re: Eric Sunshine · lore

[PATCH 14/16] worktree: avoid resolving HEAD unnecessarily

Now that git-worktree sets HEAD explicitly to its final value via either git-symbolic-ref or git-update-ref, rather than relying upon git-checkout to do so, the "hack" for pacifying is_git_directory() with a temporary HEAD, though still necessary, can be simplified.

Since the real HEAD is now populated with its proper final value, the value of the temporary HEAD truly no longer matters, and any value which looks like an object ID is good enough to satisfy is_git_directory(). Therefore, just set the temporary HEAD to a literal value rather than going through the effort of resolving the current branch's HEAD.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 builtin/worktree.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)
Show changes to builtin/worktree.c +5 −12
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 94c1701..9101a3c 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -191,7 +191,6 @@ static int add_worktree(const char *path, const char *refname,
 	int counter = 0, len, ret;
 	struct strbuf symref = STRBUF_INIT;
 	struct commit *commit = NULL;
-	unsigned char rev[20];
 
 	if (file_exists(path) && !is_empty_dir(path))
 		die(_("'%s' already exists"), path);
@@ -249,20 +248,14 @@ static int add_worktree(const char *path, const char *refname,
 		   real_path(get_git_common_dir()), name);
 	/*
 	 * This is to keep resolve_ref() happy. We need a valid HEAD
-	 * or is_git_directory() will reject the directory. Moreover, HEAD
-	 * in the new worktree must resolve to the same value as HEAD in
-	 * the current tree since the command invoked to populate the new
-	 * worktree will be handed the branch/ref specified by the user.
-	 * For instance, if the user asks for the new worktree to be based
-	 * at HEAD~5, then the resolved HEAD~5 in the new worktree must
-	 * match the resolved HEAD~5 in the current tree in order to match
-	 * the user's expectation.
+	 * or is_git_directory() will reject the directory. Any value which
+	 * looks like an object ID will do since it will be immediately
+	 * replaced by the symbolic-ref or update-ref invocation in the new
+	 * worktree.
 	 */
-	if (!resolve_ref_unsafe("HEAD", 0, rev, NULL))
-		die(_("unable to resolve HEAD"));
 	strbuf_reset(&sb);
 	strbuf_addf(&sb, "%s/HEAD", sb_repo.buf);
-	write_file(sb.buf, 1, "%s\n", sha1_to_hex(rev));
+	write_file(sb.buf, 1, "0000000000000000000000000000000000000000\n");
 	strbuf_reset(&sb);
 	strbuf_addf(&sb, "%s/commondir", sb_repo.buf);
 	write_file(sb.buf, 1, "../..\n");
-- 
2.5.0.rc1.201.ga12d9f8
Eric Sunshine· Jul 11, 2015, 00:05 UTC · re: Eric Sunshine · lore

[PATCH 15/16] worktree: populate via "git reset --hard" rather than "git checkout"

Now that git-worktree handles all functionality (--force, --detach, -b/-B) previously delegated to git-checkout, actual population of the new worktree can be accomplished more directly and more lightweight with "git reset --hard" rather than "git checkout".

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 builtin/worktree.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
Show changes to builtin/worktree.c +1 −2
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 9101a3c..51c57bc 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -262,7 +262,6 @@ static int add_worktree(const char *path, const char *refname,
 
 	fprintf_ln(stderr, _("Enter %s (identifier %s)"), path, name);
 
-	setenv("GIT_CHECKOUT_NEW_WORKTREE", "1", 1);
 	setenv(GIT_DIR_ENVIRONMENT, sb_git.buf, 1);
 	setenv(GIT_WORK_TREE_ENVIRONMENT, path, 1);
 	memset(&cp, 0, sizeof(cp));
@@ -280,7 +279,7 @@ static int add_worktree(const char *path, const char *refname,
 
 	cp.argv = NULL;
 	argv_array_clear(&cp.args);
-	argv_array_push(&cp.args, "checkout");
+	argv_array_pushl(&cp.args, "reset", "--hard", NULL);
 	ret = run_command(&cp);
 	if (!ret) {
 		is_junk = 0;
-- 
2.5.0.rc1.201.ga12d9f8
Eric Sunshine· Jul 11, 2015, 00:05 UTC · re: Eric Sunshine · lore

[PATCH 16/16] checkout: drop intimate knowledge of new worktree initial population

Now that git-worktree no longer relies upon git-checkout to perform initial population of the new worktree, git-checkout no longer needs intimate knowledge that it may be working on a newly created worktree. Therefore, drop 'new_worktree_mode' and the private GIT_CHECKOUT_NEW_WORKTREE environment variable by which git-worktree communicated to git-checkout that it was being invoked to populate a new worktree.

This reverts the remaining changes to checkout.c by 529fef2 (checkout: support checking out into a new working directory, 2014-11-30).

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 builtin/checkout.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)
Show changes to builtin/checkout.c +2 −7
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 4ae895c..02d78ba 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -48,8 +48,6 @@ struct checkout_opts {
 	const char *prefix;
 	struct pathspec pathspec;
 	struct tree *source_tree;
-
-	int new_worktree_mode;
 };
 
 static int post_checkout_hook(struct commit *old, struct commit *new,
@@ -491,7 +489,7 @@ static int merge_working_tree(const struct checkout_opts *opts,
 			topts.dir->flags |= DIR_SHOW_IGNORED;
 			setup_standard_excludes(topts.dir);
 		}
-		tree = parse_tree_indirect(old->commit && !opts->new_worktree_mode ?
+		tree = parse_tree_indirect(old->commit ?
 					   old->commit->object.sha1 :
 					   EMPTY_TREE_SHA1_BIN);
 		init_tree_desc(&trees[0], tree->buffer, tree->size);
@@ -807,8 +805,7 @@ static int switch_branches(const struct checkout_opts *opts,
 		return ret;
 	}
 
-	if (!opts->quiet && !old.path && old.commit &&
-	    new->commit != old.commit && !opts->new_worktree_mode)
+	if (!opts->quiet && !old.path && old.commit && new->commit != old.commit)
 		orphaned_commit_warning(old.commit, new->commit);
 
 	update_refs_for_switch(opts, &old, new);
@@ -1151,8 +1148,6 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, options, checkout_usage,
 			     PARSE_OPT_KEEP_DASHDASH);
 
-	opts.new_worktree_mode = getenv("GIT_CHECKOUT_NEW_WORKTREE") != NULL;
-
 	if (conflict_style) {
 		opts.merge = 1; /* implied */
 		git_xmerge_config("merge.conflictstyle", conflict_style, NULL);
-- 
2.5.0.rc1.201.ga12d9f8
Junio C Hamano· Jul 13, 2015, 18:36 UTC · re: Eric Sunshine · lore

Re: [PATCH 00/16] worktree: use "git reset --hard" to populate worktree

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 13 quoted lines
> This is a follow-on series to [1], which migrated "git checkout --to"
> functionality to "git worktree add". That series continued using "git
> checkout" for the initial population of the new worktree, which required
> git-checkout to have too intimate knowledge that it was operating in a
> newly created worktree.
>
> This series eliminates git-checkout from the picture by instead
> employing "git reset --hard"[2] to populate the new worktree initially.
>
> It is built atop 1eb07d8 (worktree: add: auto-vivify new branch when
> <branch> is omitted, 2015-07-06), currently in 'next', which is
> es/worktree-add except for the final patch (which retires
> --ignore-other-worktrees) since the intention[3] was to drop that patch.

A few comments on things I noticed while reading (mostly coming from the original before this patch series):

 - What does this comment apply to?
        /*
         * $GIT_COMMON_DIR/HEAD is practically outside
         * $GIT_DIR so resolve_ref_unsafe() won't work (it
         * uses git_path). Parse the ref ourselves.
         */
   It appears in front of a call to check-linked-checkout, but I
   think the comment attempts to explain why it manually decides
   what the path should be in that function, so perhaps move it to
   the callee from the caller?
 - check_linked_checkout() when trying to decide what branch is
   checked out assumes HEAD is always a regular file, but I do not
   think we have dropped the support of SYMLINK_HEAD yet.  It needs
   to check st_mode and readlink(2), like resolve_ref_unsafe() does.
 - After a new skelton worktree is set up, the code runs a few
   commands to finish populating it, under a different pair of
   GIT_DIR/GIT_WORK_TREE, but the function does so with setenv(); it
   may be cleaner to use cp.env[] for it, as the process we care
   about using the updated environment is not "worktree add" command
   we are running ourselves, but "update-ref/symbolic-ref" and
   "reset" commands that run in the new worktree.
Other than that, looks nicely done.

I however have to wonder if the stress on "reset --hard" on log messages of various commits (and in the endgame) is somewhat misplaced.

The primary thing we wanted to see, which this series nicely brings us, is to remove "new-worktree-mode" hack from "checkout" (in other words, instead of "reset --hard", "checkout -f" would also have been a satisfactory endgame).

Thanks.
Michael J Gruber· Jul 14, 2015, 09:53 UTC · re: Junio C Hamano · lore

Re: [PATCH 00/16] worktree: use "git reset --hard" to populate worktree

Junio C Hamano venit, vidit, dixit 13.07.2015 20:36:
Show 58 quoted lines
> Eric Sunshine <sunshine@sunshineco.com> writes:
> 
>> This is a follow-on series to [1], which migrated "git checkout --to"
>> functionality to "git worktree add". That series continued using "git
>> checkout" for the initial population of the new worktree, which required
>> git-checkout to have too intimate knowledge that it was operating in a
>> newly created worktree.
>>
>> This series eliminates git-checkout from the picture by instead
>> employing "git reset --hard"[2] to populate the new worktree initially.
>>
>> It is built atop 1eb07d8 (worktree: add: auto-vivify new branch when
>> <branch> is omitted, 2015-07-06), currently in 'next', which is
>> es/worktree-add except for the final patch (which retires
>> --ignore-other-worktrees) since the intention[3] was to drop that patch.
> 
> A few comments on things I noticed while reading (mostly coming from
> the original before this patch series):
> 
>  - What does this comment apply to?
> 
>         /*
>          * $GIT_COMMON_DIR/HEAD is practically outside
>          * $GIT_DIR so resolve_ref_unsafe() won't work (it
>          * uses git_path). Parse the ref ourselves.
>          */
> 
>    It appears in front of a call to check-linked-checkout, but I
>    think the comment attempts to explain why it manually decides
>    what the path should be in that function, so perhaps move it to
>    the callee from the caller?
> 
>  - check_linked_checkout() when trying to decide what branch is
>    checked out assumes HEAD is always a regular file, but I do not
>    think we have dropped the support of SYMLINK_HEAD yet.  It needs
>    to check st_mode and readlink(2), like resolve_ref_unsafe() does.
> 
>  - After a new skelton worktree is set up, the code runs a few
>    commands to finish populating it, under a different pair of
>    GIT_DIR/GIT_WORK_TREE, but the function does so with setenv(); it
>    may be cleaner to use cp.env[] for it, as the process we care
>    about using the updated environment is not "worktree add" command
>    we are running ourselves, but "update-ref/symbolic-ref" and
>    "reset" commands that run in the new worktree.
> 
> Other than that, looks nicely done.
> 
> I however have to wonder if the stress on "reset --hard" on log
> messages of various commits (and in the endgame) is somewhat
> misplaced.
> 
> The primary thing we wanted to see, which this series nicely brings
> us, is to remove "new-worktree-mode" hack from "checkout" (in other
> words, instead of "reset --hard", "checkout -f" would also have been
> a satisfactory endgame).
> 
> Thanks.
> 

Related to that, I'm interested in "worktree list", and I'm wondering how many more worktree commands we foresee, and therefore how much refactoring should be done: Currently, the parsing of the contents of .../worktrees/ into worktree information is done right in prune-spcefic functions. They will have to be refactored. The following questions come to my mind:

- Is a simple funtion refactoring enough, or should we introduce a
worktree struct (and a list of such)?
- Should each command do its own looping, or do we want
for_each_worktree() with a callback?
- Is a fixed output format for "list"[1] enough, or do we want something
like for-each-ref or log formats (GSOC project...)?
- Finally: Who will be stepping on whose toes doing this?
Michael
[1] Something like:
* fooworktree (master)
  barworktree (HEAD detached from deadbeef)

with "*" denoting the worktree you're in (if any) and (optionally?) adding the "on" info from "git branch" in parentheses after each worktree (checked out branch name, or detached info). Maybe the path, too?

Duy Nguyen· Jul 14, 2015, 10:09 UTC · re: Michael J Gruber · lore

Re: [PATCH 00/16] worktree: use "git reset --hard" to populate worktree

On Tue, Jul 14, 2015 at 4:53 PM, Michael J Gruber <git@drmicha.warpmail.net> wrote:

Show 11 quoted lines
> Related to that, I'm interested in "worktree list", and I'm wondering
> how many more worktree commands we foresee, and therefore how much
> refactoring should be done: Currently, the parsing of the contents of
> .../worktrees/ into worktree information is done right in prune-spcefic
> functions. They will have to be refactored. The following questions come
> to my mind:
>
> - Is a simple funtion refactoring enough, or should we introduce a
> worktree struct (and a list of such)?
> - Should each command do its own looping, or do we want
> for_each_worktree() with a callback?
fhe for_each_xxx sounds like a good pattern to reuse.
> - Is a fixed output format for "list"[1] enough, or do we want something
> like for-each-ref or log formats (GSOC project...)?

We could stick to a fixed format for "worktree list" then introduce --format later if one output does not please everybody.

> - Finally: Who will be stepping on whose toes doing this?

I'm happy to step aside and let people implement it. I still have plenty of hanging topics to finish up :(

-- 
Duy
Eric Sunshine· Jul 14, 2015, 16:40 UTC · re: Michael J Gruber · lore

Re: [PATCH 00/16] worktree: use "git reset --hard" to populate worktree

On Tue, Jul 14, 2015 at 5:53 AM, Michael J Gruber <git@drmicha.warpmail.net> wrote:

Show 8 quoted lines
>> Eric Sunshine <sunshine@sunshineco.com> writes:
>>> This is a follow-on series to [1], which migrated "git checkout --to"
>>> functionality to "git worktree add". That series continued using "git
>>> checkout" for the initial population of the new worktree, which required
>>> git-checkout to have too intimate knowledge that it was operating in a
>>> newly created worktree.
> Related to that, I'm interested in "worktree list", and I'm wondering
> how many more worktree commands we foresee[...]

The ones I and others came up with (beyond 'add' and 'prune') are 'list', 'remove', 'mv', and 'lock' (for locking and unlocking). I specifically added a BUGS section to the documentation[1] as a reminder.

[1]: http://article.gmane.org/gmane.comp.version-control.git/273431
Show 10 quoted lines
> , and therefore how much
> refactoring should be done: Currently, the parsing of the contents of
> .../worktrees/ into worktree information is done right in prune-spcefic
> functions. They will have to be refactored. The following questions come
> to my mind:
>
> - Is a simple funtion refactoring enough, or should we introduce a
> worktree struct (and a list of such)?
> - Should each command do its own looping, or do we want
> for_each_worktree() with a callback?

for_each_worktree() might be overkill at this time, as I think only 'prune' and 'list' would benefit directly. 'remove', 'lock', and 'mv' probably just want to lookup a particular worktree (with 'mv', when renaming, also possibly looking up the destination worktree to check if it already exist).

> - Is a fixed output format for "list"[1] enough, or do we want something
> like for-each-ref or log formats (GSOC project...)?
> - Finally: Who will be stepping on whose toes doing this?

I had considered working on some of the commands as time permits, but don't currently have concrete plans to do so. You're welcome to jump in and tackle these ideas (but perhaps let us know, so toes don't get trod upon).

Show 6 quoted lines
> [1] Something like:
>
> * fooworktree (master)
>   barworktree (HEAD detached from deadbeef)
>
> with "*" denoting the worktree you're in (if any) and (optionally?)

Considering that "git worktree list" can be invoked from the main worktree or any linked worktree, it would be a good idea to include the main worktree in the output as well.

> adding the "on" info from "git branch" in parentheses after each
> worktree (checked out branch name, or detached info). Maybe the path, too?

I also had something very much like this in mind, possibly extending the output either with --verbose or custom format capability (keeping the GSoC project in mind), but otherwise hadn't put much thought into it.

Showing the path seems quite important, so I'd say yes to that question, probably by default.

Eric Sunshine· Jul 15, 2015, 06:48 UTC · re: Junio C Hamano · lore

Re: [PATCH 00/16] worktree: use "git reset --hard" to populate worktree

On Mon, Jul 13, 2015 at 2:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 19 quoted lines
> Eric Sunshine <sunshine@sunshineco.com> writes:
>> This series eliminates git-checkout from the picture by instead
>> employing "git reset --hard"[2] to populate the new worktree initially.
>
> A few comments on things I noticed while reading (mostly coming from
> the original before this patch series):
>
>  - What does this comment apply to?
>
>         /*
>          * $GIT_COMMON_DIR/HEAD is practically outside
>          * $GIT_DIR so resolve_ref_unsafe() won't work (it
>          * uses git_path). Parse the ref ourselves.
>          */
>
>    It appears in front of a call to check-linked-checkout, but I
>    think the comment attempts to explain why it manually decides
>    what the path should be in that function, so perhaps move it to
>    the callee from the caller?

The placement of the comment in the original code wasn't bad, but after patch 3/16 moves code around, the comment does become somewhat confusing, so moving it to the callee seems a reasonable idea.

>  - check_linked_checkout() when trying to decide what branch is
>    checked out assumes HEAD is always a regular file, but I do not
>    think we have dropped the support of SYMLINK_HEAD yet.  It needs
>    to check st_mode and readlink(2), like resolve_ref_unsafe() does.

Hmm, I wasn't aware of SYMLINK_HEAD (and don't know if Duy was). The related code in resolve_ref_unsafe() is fairly involved, worrying about race conditions and such, however, I guess check_linked_checkout()'s implementation can perhaps be simpler, as it's probably far less catastrophic for it to give the wrong answer (or just die) under such a race?

Show 7 quoted lines
>  - After a new skelton worktree is set up, the code runs a few
>    commands to finish populating it, under a different pair of
>    GIT_DIR/GIT_WORK_TREE, but the function does so with setenv(); it
>    may be cleaner to use cp.env[] for it, as the process we care
>    about using the updated environment is not "worktree add" command
>    we are running ourselves, but "update-ref/symbolic-ref" and
>    "reset" commands that run in the new worktree.

After sending the series, I was realized that this could be done more cleanly with -C, but that would have to be repeated for each command, so cp.env[] might indeed be a better choice.

Show 10 quoted lines
> Other than that, looks nicely done.
>
> I however have to wonder if the stress on "reset --hard" on log
> messages of various commits (and in the endgame) is somewhat
> misplaced.
>
> The primary thing we wanted to see, which this series nicely brings
> us, is to remove "new-worktree-mode" hack from "checkout" (in other
> words, instead of "reset --hard", "checkout -f" would also have been
> a satisfactory endgame).

I'll see if the commit messages can be reworded a bit without becoming too wordy. ("git reset --hard" has a nice conciseness.)

Duy Nguyen· Jul 15, 2015, 09:59 UTC · re: Eric Sunshine · lore

Re: [PATCH 00/16] worktree: use "git reset --hard" to populate worktree

On Wed, Jul 15, 2015 at 1:48 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 6 quoted lines
>>  - check_linked_checkout() when trying to decide what branch is
>>    checked out assumes HEAD is always a regular file, but I do not
>>    think we have dropped the support of SYMLINK_HEAD yet.  It needs
>>    to check st_mode and readlink(2), like resolve_ref_unsafe() does.
>
> Hmm, I wasn't aware of SYMLINK_HEAD (and don't know if Duy was). The
I'm aware of it. I just didn't remember it when I wrote this code.
Show 5 quoted lines
> related code in resolve_ref_unsafe() is fairly involved, worrying
> about race conditions and such, however, I guess
> check_linked_checkout()'s implementation can perhaps be simpler, as
> it's probably far less catastrophic for it to give the wrong answer
> (or just die) under such a race?

And if I remember correctly Mike Haggerty had a series to refactor ref parsing code and reuse in this place (it was my promise to do it, but he took over). I think the series was halted because refs.c was going through a major update at that time. I think we could leave it as is for now and completely replace it at some point in future.

-- 
Duy
Eric Sunshine· Jul 12, 2015, 19:20 UTC · lore

Re: [PATCH 10/16] worktree: make branch creation distinct from worktree population

On Sun, Jul 12, 2015 at 04:54:02PM +0700, Duy Nguyen wrote:
Show 25 quoted lines
> On Sun, Jul 12, 2015 at 10:27 AM, Eric Sunshine <sunshine@sunshineco.com> wrot> > In this case, it's easy enough to side-step the issue since there's no
> > need to call ref_exists() if the new branch was created successfully
> > (since we know it exists). The logic would effectively become this:
> >
> >     branch = ...
> >     if (create_new_branch) {
> >         exec "git branch newbranch branch"
> >         exec "git symbolic-ref HEAD newbranch"
> >     } else if (ref_exists(branch) && !detach)
> >         exec "git symbolic-ref HEAD branch"
> >     else
> >         exec "git update-ref HEAD $(git rev-parse branch)"
> >     exec "git reset --hard"
> 
> Yeah.. Another option we can take is deal with this at run-command.c
> level (and outside this series) because this could affect everywhere:
> by default, invalidate all cache after running any git commands. The
> caller can pass options to keep some cache intact if they know the
> command won't touch it.
> 
> If ref_exists() is the only thing we use, right now it does not use
> cache so we should be safe. If the new ref backend introduces a cache,
> they would need to examine all callers anyway, including this one. The
> cache in refs.c seems to be for for_each_ref.. only, which we don't
> call here.

In case I don't need to re-roll the series for some other reason, here's a squash-in making the above adjustment to patch 12/16, which Junio can pick up if he wants to:

---- 8< ----
From: Eric Sunshine <sunshine@sunshineco.com>
Subject: [PATCH] fixup! worktree: detect branch symref/detach and error conditions locally
Current logic is effectively:
    branch = ...
    if (create_new_branch) {
        run "git branch newbranch branch"
        branch = newbranch
    }
    if (!detach && ref_exists(branch)) {
        if (!force)
            die_if_already_checked_out(branch)
        run "git symbolic-ref HEAD branch"
    } else
        run "git update-ref HEAD $(git rev-parse branch)"
    run "git reset --hard"

The ref_exists() call following 'run "git branch newbranch branch"' works fine since ref_exists() hits the filesystem directly to answer the question rather than relying upon potentially stale cached information. However, this is potentially fragile if someone some day implements caching. Moreover, with pluggable backends on the horizon, we may not be able to rely upon ref_exists() in this process accurately reflecting changes made by a subprocess.

If new branch creation was successful, then we know it's a branch, and don't need to ask ref_exists() about it, thus we don't need to worry about ref_exists() possibly returning an answer based upon stale information, nor do we have to check if the new branch is already checked out elsewhere. Therefore, rework the logic slightly to become effectively:

    branch = ...
    if (create_new_branch) {
        run "git branch newbranch branch"
        run "git symbolic-ref HEAD newbranch"
    } else if (!detach && ref_exists(branch)) {
        if (!force)
            die_if_already_checked_out(branch)
        run "git symbolic-ref HEAD branch"
    } else
        run "git update-ref HEAD $(git rev-parse branch)"
    run "git reset --hard"
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 builtin/worktree.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
Show changes to builtin/worktree.c +4 −2
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 51c57bc..39f87a4 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -195,8 +195,10 @@ static int add_worktree(const char *path, const char *refname,
 	if (file_exists(path) && !is_empty_dir(path))
 		die(_("'%s' already exists"), path);
 
-	if (!opts->detach && !strbuf_check_branch_ref(&symref, refname) &&
-	    ref_exists(symref.buf)) {
+	if (opts->force_new_branch)
+		;
+	else if (!opts->detach && !strbuf_check_branch_ref(&symref, refname) &&
+		 ref_exists(symref.buf)) {
 		if (!opts->force)
 			die_if_checked_out(symref.buf);
 	} else {
-- 
2.5.0.rc1.387.g8463c8d

← back to recent threads