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

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

[Eric Sunshine]

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

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

[Eric Sunshine]

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

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

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

[Eric Sunshine]

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

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

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

[Eric Sunshine]

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

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

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

[Eric Sunshine]

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

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

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

[Eric Sunshine]

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

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

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

[Eric Sunshine]

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

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

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

[Eric Sunshine]

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

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

[PATCH 08/16] worktree: introduce options container

[Eric Sunshine]

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

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

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

[Eric Sunshine]

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

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

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

[Eric Sunshine]

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

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

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

[Eric Sunshine]

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

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

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

[Eric Sunshine]

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

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

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

[Eric Sunshine]

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

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

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

[Eric Sunshine]

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

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

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

[Eric Sunshine]

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

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

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

[Eric Sunshine]

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

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

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

[Duy Nguyen]

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')
-- 
Duy

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

[Eric Sunshine]

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.

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?

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

[Duy Nguyen]

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

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

[Eric Sunshine]

On Sat, Jul 11, 2015 at 10:48 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> 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.)

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

[Eric Sunshine]

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.

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

[Eric Sunshine]

On Sat, Jul 11, 2015 at 11:14 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> 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"

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

[Duy Nguyen]

(resend, +everybody)

On Sun, Jul 12, 2015 at 10:27 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> 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

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

[Eric Sunshine]

On Sun, Jul 12, 2015 at 04:54:02PM +0700, Duy Nguyen wrote:
> 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(-)

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

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

[Junio C Hamano]

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.

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

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 13.07.2015 20:36:
> 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?

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

[Duy Nguyen]

On Tue, Jul 14, 2015 at 4:53 PM, Michael J Gruber
<git@drmicha.warpmail.net> wrote:
> 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

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

[Eric Sunshine]

On Tue, Jul 14, 2015 at 5:53 AM, Michael J Gruber
<git@drmicha.warpmail.net> wrote:
>> 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

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

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

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

[Eric Sunshine]

On Mon, Jul 13, 2015 at 2:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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?

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

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

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

[Duy Nguyen]

On Wed, Jul 15, 2015 at 1:48 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>  - 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.

> 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