fetch: use new branch_checked_out() and add tests

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 <derrickstolee@github.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Derrick Stolee committed Jun 14, 2022 at 19:27 UTC 12d47e3b1fbdc3891ec9d2106a43809ce7fa1363
2 files changed +55 -14
builtin/fetch.c
+8 -14
@@ -885,7 +885,6 @@ static int update_local_ref(struct ref *ref,
885 struct worktree **worktrees)
886 {
887 struct commit *current = NULL, *updated;
888 - const struct worktree *wt;
888 const char *pretty_ref = prettify_refname(ref->name);
889 int fast_forward = 0;
890
@@ -900,16 +899,14 @@ static int update_local_ref(struct ref *ref,
899 }
900
901 if (!update_head_ok &&
903 - (wt = find_shared_symref(worktrees, "HEAD", ref->name)) &&
904 - !wt->is_bare && !is_null_oid(&ref->old_oid)) {
902 + !is_null_oid(&ref->old_oid) &&
903 + branch_checked_out(ref->name)) {
904 /*
905 * If this is the head, and it's not okay to update
906 * the head, and the old value of the head isn't empty...
907 */
908 format_display(display, '!', _("[rejected]"),
910 - wt->is_current ?
911 - _("can't fetch in current branch") :
912 - _("checked out in another worktree"),
909 + _("can't fetch into checked-out branch"),
910 remote, pretty_ref, summary_width);
911 return 1;
912 }
@@ -1434,19 +1431,16 @@ cleanup:
1431 return result;
1432 }
1433
1437 -static void check_not_current_branch(struct ref *ref_map,
1438 - struct worktree **worktrees)
1434 +static void check_not_current_branch(struct ref *ref_map)
1435 {
1440 - const struct worktree *wt;
1436 + const char *path;
1437 for (; ref_map; ref_map = ref_map->next)
1438 if (ref_map->peer_ref &&
1439 starts_with(ref_map->peer_ref->name, "refs/heads/") &&
1444 - (wt = find_shared_symref(worktrees, "HEAD",
1445 - ref_map->peer_ref->name)) &&
1446 - !wt->is_bare)
1440 + (path = branch_checked_out(ref_map->peer_ref->name)))
1441 die(_("refusing to fetch into branch '%s' "
1442 "checked out at '%s'"),
1449 - ref_map->peer_ref->name, wt->path);
1443 + ref_map->peer_ref->name, path);
1444 }
1445
1446 static int truncate_fetch_head(void)
@@ -1650,7 +1644,7 @@ static int do_fetch(struct transport *transport,
1644 ref_map = get_ref_map(transport->remote, remote_refs, rs,
1645 tags, &autotags);
1646 if (!update_head_ok)
1653 - check_not_current_branch(ref_map, worktrees);
1647 + check_not_current_branch(ref_map);
1648
1649 retcode = open_fetch_head(&fetch_head);
1650 if (retcode)
t/t2407-worktree-heads.sh
+47
@@ -15,6 +15,21 @@ test_expect_success 'setup' '
15 test_commit $i &&
16 git branch wt-$i &&
17 git worktree add wt-$i wt-$i || return 1
18 + done &&
19 +
20 + # Create a server that updates each branch by one commit
21 + git init server &&
22 + test_commit -C server initial &&
23 + git remote add server ./server &&
24 + for i in 1 2 3 4
25 + do
26 + git -C server checkout -b wt-$i &&
27 + test_commit -C server A-$i || return 1
28 + done &&
29 + for i in 1 2
30 + do
31 + git -C server checkout -b fake-$i &&
32 + test_commit -C server f-$i || return 1
33 done
34 '
35
@@ -48,4 +63,36 @@ test_expect_success 'refuse to overwrite: worktree in rebase' '
63 grep "cannot force update the branch '\''fake-1'\'' checked out at.*wt-3" err
64 '
65
66 +test_expect_success !SANITIZE_LEAK 'refuse to fetch over ref: checked out' '
67 + test_must_fail git fetch server +refs/heads/wt-3:refs/heads/wt-3 2>err &&
68 + grep "refusing to fetch into branch '\''refs/heads/wt-3'\''" err &&
69 +
70 + # General fetch into refs/heads/ will fail on first ref,
71 + # so use a generic error message check.
72 + test_must_fail git fetch server +refs/heads/*:refs/heads/* 2>err &&
73 + grep "refusing to fetch into branch" err
74 +'
75 +
76 +test_expect_success !SANITIZE_LEAK 'refuse to fetch over ref: worktree in bisect' '
77 + test_when_finished rm -rf .git/worktrees/wt-*/BISECT_* &&
78 +
79 + touch .git/worktrees/wt-4/BISECT_LOG &&
80 + echo refs/heads/fake-2 >.git/worktrees/wt-4/BISECT_START &&
81 +
82 + test_must_fail git fetch server +refs/heads/fake-2:refs/heads/fake-2 2>err &&
83 + grep "refusing to fetch into branch" err
84 +'
85 +
86 +test_expect_success !SANITIZE_LEAK 'refuse to fetch over ref: worktree in rebase' '
87 + test_when_finished rm -rf .git/worktrees/wt-*/rebase-merge &&
88 +
89 + mkdir -p .git/worktrees/wt-4/rebase-merge &&
90 + touch .git/worktrees/wt-4/rebase-merge/interactive &&
91 + echo refs/heads/fake-1 >.git/worktrees/wt-4/rebase-merge/head-name &&
92 + echo refs/heads/fake-2 >.git/worktrees/wt-4/rebase-merge/onto &&
93 +
94 + test_must_fail git fetch server +refs/heads/fake-1:refs/heads/fake-1 2>err &&
95 + grep "refusing to fetch into branch" err
96 +'
97 +
98 test_done