Eric Sunshine[PATCH v2 00/20] rid git-checkout of too-intimate knowledge of new worktree
This is v2 of [1] which rids git-checkout of the need to have specialized knowledge that it's operating in a newly created linked worktree, and decouples git-worktree from git-checkout. Thanks to Duy and Junio for reviews.
A v1 to v2 interdiff is included below.
Changes since v1:
* patch 03/20: relocate from caller to callee a comment explaining why check_linked_checkout() resolves HEAD manually[2]
* patch 06/20 (new): drop "/.git" from "'blorp' already checked out at '/foo/bar/.git'" diagnostic[3]
* patch 07/20 & 08/20 (new): teach checked_linked_checkout() about symbolic-link HEAD[2] (to augment its existing symref-style HEAD knowledge)
* patch 14/20 (new): take advantage of 'struct child_process.env' to make it obvious that assigned environment variables are intended for child process invocations[2]
* patch 16/20: side-step potential problem with future pluggable backends answering ref_exists() incorrectly from cached information after we've invoked git-branch behind their backs[4,5], and add a few in-code comments explaining the logic
* reword several commit messages to avoid stressing "git reset --hard" as endgame, and instead emphasize removal of specialized worktree creation knowledge from git-checkout[2], and resolve conflation of new branch creation, setting of worktree HEAD, and new worktree population.
[1]: http://thread.gmane.org/gmane.comp.version-control.git/273856 [2]: http://article.gmane.org/gmane.comp.version-control.git/273921 [3]: http://thread.gmane.org/gmane.comp.version-control.git/274001 [4]: http://article.gmane.org/gmane.comp.version-control.git/273885 [5]: http://article.gmane.org/gmane.comp.version-control.git/273901
Eric Sunshine (20):
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
checkout: check_linked_checkout: improve "already checked out"
aesthetic
checkout: check_linked_checkout: simplify symref parsing
checkout: teach check_linked_checkout() about symbolic link HEAD
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: elucidate environment variables intended for child processes
worktree: add_worktree: construct worktree-population command locally
worktree: detect branch-name/detached 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 newly created worktreebranch.c | 67 +++++++++++++++++++++++++++ branch.h | 7 +++ builtin/checkout.c | 82 +++------------------------------ builtin/worktree.c | 120 ++++++++++++++++++++++++++++++++---------------- t/t2025-worktree-add.sh | 8 ++++ 5 files changed, 169 insertions(+), 115 deletions(-)
--
2.5.0.rc2.378.g0af52e8
---- 8< ----
diff --git a/branch.c b/branch.c
index 7b8b9a3..dfd7698 100644
--- a/branch.c
+++ b/branch.c
@@ -315,22 +315,28 @@ 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;
+ /*
+ * $GIT_COMMON_DIR/HEAD is practically outside
+ * $GIT_DIR so resolve_ref_unsafe() won't work (it
+ * uses git_path). Parse the ref ourselves.
+ */
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))
+ if (!strbuf_readlink(&sb, path.buf, 0)) {
+ if (!starts_with(sb.buf, "refs/") ||
+ check_refname_format(sb.buf, 0))
+ goto done;
+ } else if (strbuf_read_file(&sb, path.buf, 0) >= 0 &&
+ starts_with(sb.buf, "ref:")) {
+ strbuf_remove(&sb, 0, strlen("ref:"));
+ strbuf_trim(&sb);
+ } else
goto done;
- while (isspace(*start))
- start++;
- end = start;
- while (*end && !isspace(*end))
- end++;
- if (strncmp(start, branch, end - start) || branch[end - start] != '\0')
+ if (strcmp(sb.buf, branch))
goto done;
if (id) {
strbuf_reset(&path);
@@ -341,6 +347,7 @@ static void check_linked_checkout(const char *branch, const char *id)
} else
strbuf_addstr(&gitdir, get_git_common_dir());
skip_prefix(branch, "refs/heads/", &branch);
+ strbuf_strip_suffix(&gitdir, "/.git");
die(_("'%s' is already checked out at '%s'"), branch, gitdir.buf);
done:
strbuf_release(&path);
@@ -354,11 +361,6 @@ void die_if_checked_out(const char *branch)
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());
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 51c57bc..2873064 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -188,6 +188,7 @@ static int add_worktree(const char *path, const char *refname,
const char *name;
struct stat st;
struct child_process cp;
+ struct argv_array child_env = ARGV_ARRAY_INIT;
int counter = 0, len, ret;
struct strbuf symref = STRBUF_INIT;
struct commit *commit = NULL;
@@ -195,11 +196,14 @@ 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)) {
+ /* is 'refname' a branch or commit? */
+ if (opts->force_new_branch) /* definitely a branch */
+ ;
+ else if (!opts->detach && !strbuf_check_branch_ref(&symref, refname) &&
+ ref_exists(symref.buf)) { /* it's a branch */
if (!opts->force)
die_if_checked_out(symref.buf);
- } else {
+ } else { /* must be a commit */
commit = lookup_commit_reference_by_name(refname);
if (!commit)
die(_("invalid reference: %s"), refname);
@@ -262,8 +266,8 @@ static int add_worktree(const char *path, const char *refname,
fprintf_ln(stderr, _("Enter %s (identifier %s)"), path, name);
- setenv(GIT_DIR_ENVIRONMENT, sb_git.buf, 1);
- setenv(GIT_WORK_TREE_ENVIRONMENT, path, 1);
+ argv_array_pushf(&child_env, "%s=%s", GIT_DIR_ENVIRONMENT, sb_git.buf);
+ argv_array_pushf(&child_env, "%s=%s", GIT_WORK_TREE_ENVIRONMENT, path);
memset(&cp, 0, sizeof(cp));
cp.git_cmd = 1;
@@ -273,6 +277,7 @@ static int add_worktree(const char *path, const char *refname,
else
argv_array_pushl(&cp.args, "symbolic-ref", "HEAD",
symref.buf, NULL);
+ cp.env = child_env.argv;
ret = run_command(&cp);
if (ret)
goto done;
@@ -280,6 +285,7 @@ static int add_worktree(const char *path, const char *refname,
cp.argv = NULL;
argv_array_clear(&cp.args);
argv_array_pushl(&cp.args, "reset", "--hard", NULL);
+ cp.env = child_env.argv;
ret = run_command(&cp);
if (!ret) {
is_junk = 0;
@@ -292,6 +298,7 @@ done:
strbuf_reset(&sb);
strbuf_addf(&sb, "%s/locked", sb_repo.buf);
unlink_or_warn(sb.buf);
+ argv_array_clear(&child_env);
strbuf_release(&sb);
strbuf_release(&symref);
strbuf_release(&sb_repo);
diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
index ead8aa2..9e30690 100755
--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh
@@ -83,6 +83,14 @@ test_expect_success 'die the same branch is already checked out' '
)
'
+test_expect_success SYMLINKS 'die the same branch is already checked out (symlink)' '
+ head=$(git -C there rev-parse --git-path HEAD) &&
+ ref=$(git -C there symbolic-ref HEAD) &&
+ rm "$head" &&
+ ln -s "$ref" "$head" &&
+ test_must_fail git -C here checkout newmaster
+'
+
test_expect_success 'not die the same branch is already checked out' '
(
cd here &&
---- 8< ----
