[v2,3/5] fetch: use new branch_checked_out() and add tests

Commit Message

Derrick Stolee June 14, 2022, 7:27 p.m. UTC

From: Derrick Stolee <[e-mail]>

When fetching refs from a remote, it is possible that the refspec will
cause use to overwrite a ref that is checked out in a worktree. The
existing logic in builtin/fetch.c uses a possibly-slow mechanism. Update
those sections to use the new, more efficient branch_checked_out()
helper.

These uses were not previously tested, so add a test case that can be
used for these kinds of collisions. There is only one test now, but more
tests will be added as other consumers of branch_checked_out() are
added.

Note that there are two uses in builtin/fetch.c, but only one of the
messages is tested. This is because the tested check is run before
completing the fetch, and the untested check is not reachable without
concurrent updates to the filesystem. Thus, it is beneficial to keep
that extra check for the sake of defense-in-depth. However, we should
not attempt to test the check, as the effort required is too
complicated to be worth the effort. This use in update_local_ref()
also requires a change in the error message because we no longer have
access to the worktree struct, only the path of the worktree. This error
is so rare that making a distinction between the two is not critical.

Signed-off-by: Derrick Stolee <[e-mail]>
---
 builtin/fetch.c           | 22 +++++++-----------
 t/t2407-worktree-heads.sh | 47 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+), 14 deletions(-)

Patch

diff --git a/builtin/fetch.c b/builtin/fetch.c
index ac29c2b1ae3..7fdbfee5c93 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -885,7 +885,6 @@  static int update_local_ref(struct ref *ref,
 			    struct worktree **worktrees)
 {
 	struct commit *current = NULL, *updated;
-	const struct worktree *wt;
 	const char *pretty_ref = prettify_refname(ref->name);
 	int fast_forward = 0;
 
@@ -900,16 +899,14 @@  static int update_local_ref(struct ref *ref,
 	}
 
 	if (!update_head_ok &&
-	    (wt = find_shared_symref(worktrees, "HEAD", ref->name)) &&
-	    !wt->is_bare && !is_null_oid(&ref->old_oid)) {
+	    !is_null_oid(&ref->old_oid) &&
+	    branch_checked_out(ref->name)) {
 		/*
 		 * If this is the head, and it's not okay to update
 		 * the head, and the old value of the head isn't empty...
 		 */
 		format_display(display, '!', _("[rejected]"),
-			       wt->is_current ?
-				       _("can't fetch in current branch") :
-				       _("checked out in another worktree"),
+			       _("can't fetch into checked-out branch"),
 			       remote, pretty_ref, summary_width);
 		return 1;
 	}
@@ -1434,19 +1431,16 @@  cleanup:
 	return result;
 }
 
-static void check_not_current_branch(struct ref *ref_map,
-				     struct worktree **worktrees)
+static void check_not_current_branch(struct ref *ref_map)
 {
-	const struct worktree *wt;
+	const char *path;
 	for (; ref_map; ref_map = ref_map->next)
 		if (ref_map->peer_ref &&
 		    starts_with(ref_map->peer_ref->name, "refs/heads/") &&
-		    (wt = find_shared_symref(worktrees, "HEAD",
-					     ref_map->peer_ref->name)) &&
-		    !wt->is_bare)
+		    (path = branch_checked_out(ref_map->peer_ref->name)))
 			die(_("refusing to fetch into branch '%s' "
 			      "checked out at '%s'"),
-			    ref_map->peer_ref->name, wt->path);
+			    ref_map->peer_ref->name, path);
 }
 
 static int truncate_fetch_head(void)
@@ -1650,7 +1644,7 @@  static int do_fetch(struct transport *transport,
 	ref_map = get_ref_map(transport->remote, remote_refs, rs,
 			      tags, &autotags);
 	if (!update_head_ok)
-		check_not_current_branch(ref_map, worktrees);
+		check_not_current_branch(ref_map);
 
 	retcode = open_fetch_head(&fetch_head);
 	if (retcode)
diff --git a/t/t2407-worktree-heads.sh b/t/t2407-worktree-heads.sh
index a838f2be474..1fbde05fe2b 100755
--- a/t/t2407-worktree-heads.sh
+++ b/t/t2407-worktree-heads.sh
@@ -15,6 +15,21 @@  test_expect_success 'setup' '
 		test_commit $i &&
 		git branch wt-$i &&
 		git worktree add wt-$i wt-$i || return 1
+	done &&
+
+	# Create a server that updates each branch by one commit
+	git init server &&
+	test_commit -C server initial &&
+	git remote add server ./server &&
+	for i in 1 2 3 4
+	do
+		git -C server checkout -b wt-$i &&
+		test_commit -C server A-$i || return 1
+	done &&
+	for i in 1 2
+	do
+		git -C server checkout -b fake-$i &&
+		test_commit -C server f-$i || return 1
 	done
 '
 
@@ -48,4 +63,36 @@  test_expect_success 'refuse to overwrite: worktree in rebase' '
 	grep "cannot force update the branch '\''fake-1'\'' checked out at.*wt-3" err
 '
 
+test_expect_success !SANITIZE_LEAK 'refuse to fetch over ref: checked out' '
+	test_must_fail git fetch server +refs/heads/wt-3:refs/heads/wt-3 2>err &&
+	grep "refusing to fetch into branch '\''refs/heads/wt-3'\''" err &&
+
+	# General fetch into refs/heads/ will fail on first ref,
+	# so use a generic error message check.
+	test_must_fail git fetch server +refs/heads/*:refs/heads/* 2>err &&
+	grep "refusing to fetch into branch" err
+'
+
+test_expect_success !SANITIZE_LEAK 'refuse to fetch over ref: worktree in bisect' '
+	test_when_finished rm -rf .git/worktrees/wt-*/BISECT_* &&
+
+	touch .git/worktrees/wt-4/BISECT_LOG &&
+	echo refs/heads/fake-2 >.git/worktrees/wt-4/BISECT_START &&
+
+	test_must_fail git fetch server +refs/heads/fake-2:refs/heads/fake-2 2>err &&
+	grep "refusing to fetch into branch" err
+'
+
+test_expect_success !SANITIZE_LEAK 'refuse to fetch over ref: worktree in rebase' '
+	test_when_finished rm -rf .git/worktrees/wt-*/rebase-merge &&
+
+	mkdir -p .git/worktrees/wt-4/rebase-merge &&
+	touch .git/worktrees/wt-4/rebase-merge/interactive &&
+	echo refs/heads/fake-1 >.git/worktrees/wt-4/rebase-merge/head-name &&
+	echo refs/heads/fake-2 >.git/worktrees/wt-4/rebase-merge/onto &&
+
+	test_must_fail git fetch server +refs/heads/fake-1:refs/heads/fake-1 2>err &&
+	grep "refusing to fetch into branch" err
+'
+
 test_done