From 12d47e3b1fbdc3891ec9d2106a43809ce7fa1363 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Tue, 14 Jun 2022 19:27:31 +0000 Subject: 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 Signed-off-by: Junio C Hamano --- builtin/fetch.c | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-) (limited to 'builtin') diff --git a/builtin/fetch.c b/builtin/fetch.c index eeee5ac8f1..fa473fc394 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) -- cgit v1.3 From b489b9d9aa44bb0d347b3d7a142995ddd5c9d534 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Tue, 14 Jun 2022 19:27:32 +0000 Subject: branch: use branch_checked_out() when deleting refs This is the last current use of find_shared_symref() that can easily be replaced by branch_checked_out(). The benefit of this switch is that the code is a bit simpler, but also it is faster on repeated calls. The remaining uses of find_shared_symref() are non-trivial to remove, so we probably should not continue in that direction: * builtin/notes.c uses find_shared_symref() with "NOTES_MERGE_REF" instead of "HEAD", so it doesn't have an immediate analogue with branch_checked_out(). Perhaps we should consider extending it to include that symref in addition to HEAD, BISECT_HEAD, and REBASE_HEAD. * receive-pack.c checks to see if a worktree has a checkout for the ref that is being updated. The tricky part is that it can actually decide to update the worktree directly instead of just skipping the update. This all depends on the receive.denyCurrentBranch config option. The implementation currenty cares about receiving the worktree in the result, so the current branch_checked_out() prototype is insufficient currently. This is something to investigate later, though, since a large number of refs could be updated at the same time and using the strmap implementation of branch_checked_out() could be beneficial. Signed-off-by: Derrick Stolee Signed-off-by: Junio C Hamano --- builtin/branch.c | 7 +++---- t/t2407-worktree-heads.sh | 5 ++++- 2 files changed, 7 insertions(+), 5 deletions(-) (limited to 'builtin') diff --git a/builtin/branch.c b/builtin/branch.c index 5d00d0b8d3..f875952e7b 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -253,12 +253,11 @@ static int delete_branches(int argc, const char **argv, int force, int kinds, name = mkpathdup(fmt, bname.buf); if (kinds == FILTER_REFS_BRANCHES) { - const struct worktree *wt = - find_shared_symref(worktrees, "HEAD", name); - if (wt) { + const char *path; + if ((path = branch_checked_out(name))) { error(_("Cannot delete branch '%s' " "checked out at '%s'"), - bname.buf, wt->path); + bname.buf, path); ret = 1; continue; } diff --git a/t/t2407-worktree-heads.sh b/t/t2407-worktree-heads.sh index 1fbde05fe2..a5aec1486c 100755 --- a/t/t2407-worktree-heads.sh +++ b/t/t2407-worktree-heads.sh @@ -37,7 +37,10 @@ test_expect_success 'refuse to overwrite: checked out in worktree' ' for i in 1 2 3 4 do test_must_fail git branch -f wt-$i HEAD 2>err - grep "cannot force update the branch" err || return 1 + grep "cannot force update the branch" err && + + test_must_fail git branch -D wt-$i 2>err + grep "Cannot delete branch" err || return 1 done ' -- cgit v1.3 From b2463fc30a43a43b5de788fa1611603f84097873 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 18 Jun 2022 23:53:19 -0400 Subject: fetch: stop passing around unused worktrees variable In 12d47e3b1f (fetch: use new branch_checked_out() and add tests, 2022-06-14), fetch's update_local_ref() function stopped using its "worktrees" parameter. It doesn't need it, since the branch_checked_out() function examines the global worktrees under the hood. So we can not only drop the unused parameter from that function, but also from its entire call chain. And as we do so all the way up to do_fetch(), we can see that nobody uses it at all, and we can drop the local variable there entirely. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/fetch.c | 24 +++++++++--------------- 1 file changed, 9 insertions(+), 15 deletions(-) (limited to 'builtin') diff --git a/builtin/fetch.c b/builtin/fetch.c index fa473fc394..51d9c33f1e 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -881,8 +881,7 @@ static void format_display(struct strbuf *display, char code, static int update_local_ref(struct ref *ref, struct ref_transaction *transaction, const char *remote, const struct ref *remote_ref, - struct strbuf *display, int summary_width, - struct worktree **worktrees) + struct strbuf *display, int summary_width) { struct commit *current = NULL, *updated; const char *pretty_ref = prettify_refname(ref->name); @@ -1107,7 +1106,7 @@ N_("it took %.2f seconds to check forced updates; you can use\n" static int store_updated_refs(const char *raw_url, const char *remote_name, int connectivity_checked, struct ref_transaction *transaction, struct ref *ref_map, - struct fetch_head *fetch_head, struct worktree **worktrees) + struct fetch_head *fetch_head) { int url_len, i, rc = 0; struct strbuf note = STRBUF_INIT, err = STRBUF_INIT; @@ -1237,8 +1236,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name, strbuf_reset(¬e); if (ref) { rc |= update_local_ref(ref, transaction, what, - rm, ¬e, summary_width, - worktrees); + rm, ¬e, summary_width); free(ref); } else if (write_fetch_head || dry_run) { /* @@ -1329,8 +1327,7 @@ static int check_exist_and_connected(struct ref *ref_map) static int fetch_and_consume_refs(struct transport *transport, struct ref_transaction *transaction, struct ref *ref_map, - struct fetch_head *fetch_head, - struct worktree **worktrees) + struct fetch_head *fetch_head) { int connectivity_checked = 1; int ret; @@ -1353,7 +1350,7 @@ static int fetch_and_consume_refs(struct transport *transport, trace2_region_enter("fetch", "consume_refs", the_repository); ret = store_updated_refs(transport->url, transport->remote->name, connectivity_checked, transaction, ref_map, - fetch_head, worktrees); + fetch_head); trace2_region_leave("fetch", "consume_refs", the_repository); out: @@ -1543,8 +1540,7 @@ static struct transport *prepare_transport(struct remote *remote, int deepen) static int backfill_tags(struct transport *transport, struct ref_transaction *transaction, struct ref *ref_map, - struct fetch_head *fetch_head, - struct worktree **worktrees) + struct fetch_head *fetch_head) { int retcode, cannot_reuse; @@ -1565,7 +1561,7 @@ static int backfill_tags(struct transport *transport, transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL); transport_set_option(transport, TRANS_OPT_DEPTH, "0"); transport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL); - retcode = fetch_and_consume_refs(transport, transaction, ref_map, fetch_head, worktrees); + retcode = fetch_and_consume_refs(transport, transaction, ref_map, fetch_head); if (gsecondary) { transport_disconnect(gsecondary); @@ -1586,7 +1582,6 @@ static int do_fetch(struct transport *transport, struct transport_ls_refs_options transport_ls_refs_options = TRANSPORT_LS_REFS_OPTIONS_INIT; int must_list_refs = 1; - struct worktree **worktrees = get_worktrees(); struct fetch_head fetch_head = { 0 }; struct strbuf err = STRBUF_INIT; @@ -1677,7 +1672,7 @@ static int do_fetch(struct transport *transport, retcode = 1; } - if (fetch_and_consume_refs(transport, transaction, ref_map, &fetch_head, worktrees)) { + if (fetch_and_consume_refs(transport, transaction, ref_map, &fetch_head)) { retcode = 1; goto cleanup; } @@ -1700,7 +1695,7 @@ static int do_fetch(struct transport *transport, * the transaction and don't commit anything. */ if (backfill_tags(transport, transaction, tags_ref_map, - &fetch_head, worktrees)) + &fetch_head)) retcode = 1; } @@ -1785,7 +1780,6 @@ cleanup: close_fetch_head(&fetch_head); strbuf_release(&err); free_refs(ref_map); - free_worktrees(worktrees); return retcode; } -- cgit v1.3 From 9bef0b1e6ec371e786c2fba3edcc06ad040a536c Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 18 Jun 2022 23:55:16 -0400 Subject: branch: drop unused worktrees variable After b489b9d9aa (branch: use branch_checked_out() when deleting refs, 2022-06-14), we no longer look at our local "worktrees" variable, since branch_checked_out() handles it under the hood. The compiler didn't notice the unused variable because we call functions to initialize and free it (so it's not totally unused, it just doesn't do anything useful). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/branch.c | 4 ---- 1 file changed, 4 deletions(-) (limited to 'builtin') diff --git a/builtin/branch.c b/builtin/branch.c index f875952e7b..55cd9a6e99 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -204,7 +204,6 @@ static void delete_branch_config(const char *branchname) static int delete_branches(int argc, const char **argv, int force, int kinds, int quiet) { - struct worktree **worktrees; struct commit *head_rev = NULL; struct object_id oid; char *name = NULL; @@ -242,8 +241,6 @@ static int delete_branches(int argc, const char **argv, int force, int kinds, die(_("Couldn't look up commit object for HEAD")); } - worktrees = get_worktrees(); - for (i = 0; i < argc; i++, strbuf_reset(&bname)) { char *target = NULL; int flags = 0; @@ -314,7 +311,6 @@ static int delete_branches(int argc, const char **argv, int force, int kinds, free(name); strbuf_release(&bname); - free_worktrees(worktrees); return ret; } -- cgit v1.3