git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Eric Sunshine <sunshine@sunshineco.com>
To: git@vger.kernel.org
Cc: Junio C Hamano <gitster@pobox.com>,
	Duy Nguyen <pclouds@gmail.com>,
	Michael J Gruber <git@drmicha.warpmail.net>,
	Eric Sunshine <sunshine@sunshineco.com>
Subject: [PATCH v3 03/22] checkout: improve die_if_checked_out() robustness
Date: Fri, 17 Jul 2015 18:59:58 -0400	[thread overview]
Message-ID: <1437174017-81687-4-git-send-email-sunshine@sunshineco.com> (raw)
In-Reply-To: <1437174017-81687-1-git-send-email-sunshine@sunshineco.com>

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 want to call die_if_checked_out()
itself rather than only doing so indirectly as a side-effect of invoking
git-checkout, and reliance upon order of operations (creating
.git/worktrees before checking if a branch is already checked out) is
fragile. 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.

While here, also move a comment explaining why die_if_checked_out()'s
helper parses HEAD manually. Such information resides more naturally
with the helper itself rather than at its first point of call.

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

No changes since v2.

 builtin/checkout.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index e75fb5e..1992c41 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -880,6 +880,11 @@ static void check_linked_checkout(struct branch_info *new, const char *id)
 	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
@@ -916,19 +921,14 @@ static void die_if_checked_out(struct branch_info *new)
 	DIR *dir;
 	struct dirent *d;
 
+	check_linked_checkout(new, NULL);
+
 	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
-	 * uses git_path). Parse the ref ourselves.
-	 */
-	check_linked_checkout(new, NULL);
-
 	while ((d = readdir(dir)) != NULL) {
 		if (!strcmp(d->d_name, ".") || !strcmp(d->d_name, ".."))
 			continue;
-- 
2.5.0.rc2.378.g0af52e8

  parent reply	other threads:[~2015-07-17 23:02 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-07-17 22:59 [PATCH v3 00/22] rid git-checkout of too-intimate knowledge of new worktree Eric Sunshine
2015-07-17 22:59 ` [PATCH v3 01/22] checkout: avoid resolving HEAD unnecessarily Eric Sunshine
2015-07-17 22:59 ` [PATCH v3 02/22] checkout: name check_linked_checkouts() more meaningfully Eric Sunshine
2015-07-17 22:59 ` Eric Sunshine [this message]
2015-07-17 22:59 ` [PATCH v3 04/22] checkout: die_if_checked_out: simplify strbuf management Eric Sunshine
2015-07-17 23:00 ` [PATCH v3 05/22] checkout: generalize die_if_checked_out() branch name argument Eric Sunshine
2015-07-17 23:00 ` [PATCH v3 06/22] checkout: check_linked_checkout: improve "already checked out" aesthetic Eric Sunshine
2015-07-17 23:00 ` [PATCH v3 07/22] checkout: check_linked_checkout: simplify symref parsing Eric Sunshine
2015-07-17 23:00 ` [PATCH v3 08/22] checkout: teach check_linked_checkout() about symbolic link HEAD Eric Sunshine
2015-07-17 23:00 ` [PATCH v3 09/22] branch: publish die_if_checked_out() Eric Sunshine
2015-07-17 23:00 ` [PATCH v3 10/22] worktree: improve worktree setup message Eric Sunshine
2015-07-17 23:00 ` [PATCH v3 11/22] worktree: simplify new branch (-b/-B) option checking Eric Sunshine
2015-07-17 23:00 ` [PATCH v3 12/22] worktree: introduce options container Eric Sunshine
2015-07-17 23:00 ` [PATCH v3 13/22] worktree: make --detach mutually exclusive with -b/-B Eric Sunshine
2015-07-17 23:00 ` [PATCH v3 14/22] worktree: add: suppress auto-vivication with --detach and no <branch> Eric Sunshine
2015-07-17 23:20   ` Eric Sunshine
2015-07-17 23:00 ` [PATCH v3 15/22] worktree: make branch creation distinct from worktree population Eric Sunshine
2015-07-17 23:00 ` [PATCH v3 16/22] worktree: elucidate environment variables intended for child processes Eric Sunshine
2015-07-17 23:00 ` [PATCH v3 17/22] worktree: add_worktree: construct worktree-population command locally Eric Sunshine
2015-07-17 23:00 ` [PATCH v3 18/22] worktree: detect branch-name/detached and error conditions locally Eric Sunshine
2015-07-17 23:00 ` [PATCH v3 19/22] worktree: make setup of new HEAD distinct from worktree population Eric Sunshine
2015-07-17 23:00 ` [PATCH v3 20/22] worktree: avoid resolving HEAD unnecessarily Eric Sunshine
2015-07-17 23:00 ` [PATCH v3 21/22] worktree: populate via "git reset --hard" rather than "git checkout" Eric Sunshine
2015-07-17 23:00 ` [PATCH v3 22/22] checkout: drop intimate knowledge of newly created worktree Eric Sunshine

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1437174017-81687-4-git-send-email-sunshine@sunshineco.com \
    --to=sunshine@sunshineco.com \
    --cc=git@drmicha.warpmail.net \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=pclouds@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).