| Message ID | 20211113033358.2179376-6-andersk@mit.edu (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | protect branches checked out in all worktrees | expand |
Anders Kaseorg <andersk@mit.edu> writes: > Refuse to fetch into the currently checked out branch of any working > tree, not just the current one. > > Fixes this previously reported bug: > > https://public-inbox.org/git/cb957174-5e9a-5603-ea9e-ac9b58a2eaad@mathema.de > > As a side effect of using find_shared_symref, we’ll also refuse the > fetch when we’re on a detached HEAD because we’re rebasing or bisecting > on the branch in question. This seems like a sensible change. Indeed. > Signed-off-by: Anders Kaseorg <andersk@mit.edu> > --- > builtin/fetch.c | 75 +++++++++++++++++++++++-------------------- > t/t5516-fetch-push.sh | 18 +++++++++++ > 2 files changed, 58 insertions(+), 35 deletions(-) > > diff --git a/builtin/fetch.c b/builtin/fetch.c > index e5971fa6e5..f373252490 100644 > --- a/builtin/fetch.c > +++ b/builtin/fetch.c > @@ -28,6 +28,7 @@ > #include "promisor-remote.h" > #include "commit-graph.h" > #include "shallow.h" > +#include "worktree.h" > > #define FORCED_UPDATES_DELAY_WARNING_IN_MS (10 * 1000) > > @@ -840,14 +841,13 @@ 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) > + const char *remote, const struct ref *remote_ref, > + struct strbuf *display, int summary_width, > + struct worktree **worktrees) > { > struct commit *current = NULL, *updated; > enum object_type type; > - struct branch *current_branch = branch_get(NULL); > + const struct worktree *wt; > const char *pretty_ref = prettify_refname(ref->name); > int fast_forward = 0; Having to pass the parameter down to here through the ->do_fetch() ->backfill_tags() (or do_fetch() itself) ->consume_refs() ->store_updated_refs() ->update_local_ref() callchain makes the "damage to the code" by the patch look larger than it actually is. The real change is ... > @@ -862,16 +862,17 @@ static int update_local_ref(struct ref *ref, > return 0; > } > > - if (current_branch && > - !strcmp(ref->name, current_branch->name) && > - !(update_head_ok || is_bare_repository()) && > - !is_null_oid(&ref->old_oid)) { > + if (!update_head_ok && > + (wt = find_shared_symref(worktrees, "HEAD", ref->name)) && > + !wt->is_bare && !is_null_oid(&ref->old_oid)) { ... this part, which looks very sensible. > * 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]"), > - _("can't fetch in current branch"), > + wt->is_current ? > + _("can't fetch in current branch") : > + _("checked out in another worktree"), > remote, pretty_ref, summary_width); > return 1; > } > @@ -1643,7 +1647,7 @@ static int do_fetch(struct transport *transport, > "you need to specify exactly one branch with the --set-upstream option")); > } > } > - skip: > +skip: > free_refs(ref_map); ;-) I count 30 hits of "^ [a-z0-9]*:" and 255 hits of "^[a-z0-9]*:" in our codebase. It must be some developers used to subscribe to "don't place the label abut the left edge" school but no longer, or something like that. The code changes all look good to me. Thanks. > diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh > index 4db8edd9c8..36fb90f4b0 100755 > --- a/t/t5516-fetch-push.sh > +++ b/t/t5516-fetch-push.sh > @@ -1770,4 +1770,22 @@ test_expect_success 'denyCurrentBranch and worktrees' ' > git -C cloned push origin HEAD:new-wt && > test_must_fail git -C cloned push --delete origin new-wt > ' > + > +test_expect_success 'refuse fetch to current branch of worktree' ' > + test_when_finished "git worktree remove --force wt && git branch -D wt" && > + git worktree add wt && > + test_commit apple && > + test_must_fail git fetch . HEAD:wt && > + git fetch -u . HEAD:wt > +' > + > +test_expect_success 'refuse fetch to current branch of bare repository worktree' ' > + test_when_finished "rm -fr bare.git" && > + git clone --bare . bare.git && > + git -C bare.git worktree add wt && > + test_commit banana && > + test_must_fail git -C bare.git fetch .. HEAD:wt && > + git -C bare.git fetch -u .. HEAD:wt > +' > + > test_done
On 11/15/21 21:49, Junio C Hamano wrote: >> - skip: >> +skip: > > ;-) (This was to satisfy ‘make style’. Otherwise it wanted the lines following the 1-space indent to have a 9-space indent.) Anders
Hi Anders, tl;dr the patch looks nice! A few remarks below, to prove that I did not merely glance over the patch. On Fri, 12 Nov 2021, Anders Kaseorg wrote: > Refuse to fetch into the currently checked out branch of any working > tree, not just the current one. > > Fixes this previously reported bug: > > https://public-inbox.org/git/cb957174-5e9a-5603-ea9e-ac9b58a2eaad@mathema.de These days, I think the lore.kernel.org/git/ links are slightly preferred. > > As a side effect of using find_shared_symref, we’ll also refuse the > fetch when we’re on a detached HEAD because we’re rebasing or bisecting > on the branch in question. This seems like a sensible change. > > Signed-off-by: Anders Kaseorg <andersk@mit.edu> > --- > builtin/fetch.c | 75 +++++++++++++++++++++++-------------------- > t/t5516-fetch-push.sh | 18 +++++++++++ > 2 files changed, 58 insertions(+), 35 deletions(-) > > diff --git a/builtin/fetch.c b/builtin/fetch.c > index e5971fa6e5..f373252490 100644 > --- a/builtin/fetch.c > +++ b/builtin/fetch.c > @@ -28,6 +28,7 @@ > #include "promisor-remote.h" > #include "commit-graph.h" > #include "shallow.h" > +#include "worktree.h" > > #define FORCED_UPDATES_DELAY_WARNING_IN_MS (10 * 1000) > > @@ -840,14 +841,13 @@ 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) > + const char *remote, const struct ref *remote_ref, > + struct strbuf *display, int summary_width, As a reviewer, I prefer to spend my time reviewing the actual change. In this case, the re-wrapping is a bit distracting. Not really important here, but maybe for future contributions. The easier a contributor makes it on reviewers, the better (it definitely helps avoid the grumpy judge bias[*1*]). > + struct worktree **worktrees) > { > struct commit *current = NULL, *updated; > enum object_type type; > - struct branch *current_branch = branch_get(NULL); > + const struct worktree *wt; > const char *pretty_ref = prettify_refname(ref->name); > int fast_forward = 0; > > @@ -862,16 +862,17 @@ static int update_local_ref(struct ref *ref, > return 0; > } > > - if (current_branch && > - !strcmp(ref->name, current_branch->name) && > - !(update_head_ok || is_bare_repository()) && > - !is_null_oid(&ref->old_oid)) { > + if (!update_head_ok && > + (wt = find_shared_symref(worktrees, "HEAD", ref->name)) && > + !wt->is_bare && !is_null_oid(&ref->old_oid)) { > /* > * 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]"), > - _("can't fetch in current branch"), > + wt->is_current ? > + _("can't fetch in current branch") : > + _("checked out in another worktree"), > remote, pretty_ref, summary_width); > return 1; > } > @@ -1071,7 +1072,8 @@ N_("it took %.2f seconds to check forced updates; you can use\n" > " to avoid this check\n"); > > static int store_updated_refs(const char *raw_url, const char *remote_name, > - int connectivity_checked, struct ref *ref_map) > + int connectivity_checked, struct ref *ref_map, > + struct worktree **worktrees) > { > struct fetch_head fetch_head; > struct commit *commit; > @@ -1188,7 +1190,8 @@ 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); > + rm, ¬e, summary_width, > + worktrees); > free(ref); > } else if (write_fetch_head || dry_run) { > /* > @@ -1301,16 +1304,15 @@ static int fetch_refs(struct transport *transport, struct ref *ref_map) > } > > /* Update local refs based on the ref values fetched from a remote */ > -static int consume_refs(struct transport *transport, struct ref *ref_map) > +static int consume_refs(struct transport *transport, struct ref *ref_map, > + struct worktree **worktrees) > { > int connectivity_checked = transport->smart_options > ? transport->smart_options->connectivity_checked : 0; > int ret; > trace2_region_enter("fetch", "consume_refs", the_repository); > - ret = store_updated_refs(transport->url, > - transport->remote->name, > - connectivity_checked, > - ref_map); > + ret = store_updated_refs(transport->url, transport->remote->name, > + connectivity_checked, ref_map, worktrees); Again, this rewrapping is slightly distracting to my brain. > transport_unlock_pack(transport); > trace2_region_leave("fetch", "consume_refs", the_repository); > return ret; > @@ -1643,7 +1647,7 @@ static int do_fetch(struct transport *transport, > "you need to specify exactly one branch with the --set-upstream option")); > } > } > - skip: > +skip: Okay, this one is distracting _but also_ pleasing. I am only pointing this out to prove that I did not go over your patch sloppily. :-) > @@ -1653,11 +1657,12 @@ static int do_fetch(struct transport *transport, > ref_map = NULL; > find_non_local_tags(remote_refs, &ref_map, &tail); > if (ref_map) > - backfill_tags(transport, ref_map); > + backfill_tags(transport, ref_map, worktrees); > free_refs(ref_map); > } > > - cleanup: > +cleanup: Same here. > + free_worktrees(worktrees); > return retcode; > } > > diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh > index 4db8edd9c8..36fb90f4b0 100755 > --- a/t/t5516-fetch-push.sh > +++ b/t/t5516-fetch-push.sh > @@ -1770,4 +1770,22 @@ test_expect_success 'denyCurrentBranch and worktrees' ' > git -C cloned push origin HEAD:new-wt && > test_must_fail git -C cloned push --delete origin new-wt > ' > + > +test_expect_success 'refuse fetch to current branch of worktree' ' > + test_when_finished "git worktree remove --force wt && git branch -D wt" && > + git worktree add wt && > + test_commit apple && > + test_must_fail git fetch . HEAD:wt && > + git fetch -u . HEAD:wt > +' > + > +test_expect_success 'refuse fetch to current branch of bare repository worktree' ' > + test_when_finished "rm -fr bare.git" && > + git clone --bare . bare.git && > + git -C bare.git worktree add wt && > + test_commit banana && > + test_must_fail git -C bare.git fetch .. HEAD:wt && > + git -C bare.git fetch -u .. HEAD:wt > +' > + > test_done Reads nicely. Thanks, Dscho Footnote: https://en.wikipedia.org/wiki/Hungry_judge_effect
diff --git a/builtin/fetch.c b/builtin/fetch.c index e5971fa6e5..f373252490 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -28,6 +28,7 @@ #include "promisor-remote.h" #include "commit-graph.h" #include "shallow.h" +#include "worktree.h" #define FORCED_UPDATES_DELAY_WARNING_IN_MS (10 * 1000) @@ -840,14 +841,13 @@ 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) + const char *remote, const struct ref *remote_ref, + struct strbuf *display, int summary_width, + struct worktree **worktrees) { struct commit *current = NULL, *updated; enum object_type type; - struct branch *current_branch = branch_get(NULL); + const struct worktree *wt; const char *pretty_ref = prettify_refname(ref->name); int fast_forward = 0; @@ -862,16 +862,17 @@ static int update_local_ref(struct ref *ref, return 0; } - if (current_branch && - !strcmp(ref->name, current_branch->name) && - !(update_head_ok || is_bare_repository()) && - !is_null_oid(&ref->old_oid)) { + if (!update_head_ok && + (wt = find_shared_symref(worktrees, "HEAD", ref->name)) && + !wt->is_bare && !is_null_oid(&ref->old_oid)) { /* * 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]"), - _("can't fetch in current branch"), + wt->is_current ? + _("can't fetch in current branch") : + _("checked out in another worktree"), remote, pretty_ref, summary_width); return 1; } @@ -1071,7 +1072,8 @@ N_("it took %.2f seconds to check forced updates; you can use\n" " to avoid this check\n"); static int store_updated_refs(const char *raw_url, const char *remote_name, - int connectivity_checked, struct ref *ref_map) + int connectivity_checked, struct ref *ref_map, + struct worktree **worktrees) { struct fetch_head fetch_head; struct commit *commit; @@ -1188,7 +1190,8 @@ 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); + rm, ¬e, summary_width, + worktrees); free(ref); } else if (write_fetch_head || dry_run) { /* @@ -1301,16 +1304,15 @@ static int fetch_refs(struct transport *transport, struct ref *ref_map) } /* Update local refs based on the ref values fetched from a remote */ -static int consume_refs(struct transport *transport, struct ref *ref_map) +static int consume_refs(struct transport *transport, struct ref *ref_map, + struct worktree **worktrees) { int connectivity_checked = transport->smart_options ? transport->smart_options->connectivity_checked : 0; int ret; trace2_region_enter("fetch", "consume_refs", the_repository); - ret = store_updated_refs(transport->url, - transport->remote->name, - connectivity_checked, - ref_map); + ret = store_updated_refs(transport->url, transport->remote->name, + connectivity_checked, ref_map, worktrees); transport_unlock_pack(transport); trace2_region_leave("fetch", "consume_refs", the_repository); return ret; @@ -1371,19 +1373,18 @@ static int prune_refs(struct refspec *rs, struct ref *ref_map, return result; } -static void check_not_current_branch(struct ref *ref_map) +static void check_not_current_branch(struct ref *ref_map, + struct worktree **worktrees) { - struct branch *current_branch = branch_get(NULL); - - if (is_bare_repository() || !current_branch) - return; - + const struct worktree *wt; for (; ref_map; ref_map = ref_map->next) - if (ref_map->peer_ref && !strcmp(current_branch->refname, - ref_map->peer_ref->name)) - die(_("refusing to fetch into current branch %s " - "of non-bare repository"), - current_branch->refname); + if (ref_map->peer_ref && + (wt = find_shared_symref(worktrees, "HEAD", + ref_map->peer_ref->name)) && + !wt->is_bare) + die(_("refusing to fetch into branch '%s' " + "checked out at '%s'"), + ref_map->peer_ref->name, wt->path); } static int truncate_fetch_head(void) @@ -1481,7 +1482,8 @@ static struct transport *prepare_transport(struct remote *remote, int deepen) return transport; } -static void backfill_tags(struct transport *transport, struct ref *ref_map) +static void backfill_tags(struct transport *transport, struct ref *ref_map, + struct worktree **worktrees) { int cannot_reuse; @@ -1503,7 +1505,7 @@ static void backfill_tags(struct transport *transport, struct ref *ref_map) transport_set_option(transport, TRANS_OPT_DEPTH, "0"); transport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL); if (!fetch_refs(transport, ref_map)) - consume_refs(transport, ref_map); + consume_refs(transport, ref_map, worktrees); if (gsecondary) { transport_disconnect(gsecondary); @@ -1521,6 +1523,7 @@ 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(); if (tags == TAGS_DEFAULT) { if (transport->remote->fetch_tags == 2) @@ -1576,7 +1579,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); + check_not_current_branch(ref_map, worktrees); if (tags == TAGS_DEFAULT && autotags) transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, "1"); @@ -1594,7 +1597,8 @@ static int do_fetch(struct transport *transport, transport->url); } } - if (fetch_refs(transport, ref_map) || consume_refs(transport, ref_map)) { + if (fetch_refs(transport, ref_map) || + consume_refs(transport, ref_map, worktrees)) { free_refs(ref_map); retcode = 1; goto cleanup; @@ -1643,7 +1647,7 @@ static int do_fetch(struct transport *transport, "you need to specify exactly one branch with the --set-upstream option")); } } - skip: +skip: free_refs(ref_map); /* if neither --no-tags nor --tags was specified, do automated tag @@ -1653,11 +1657,12 @@ static int do_fetch(struct transport *transport, ref_map = NULL; find_non_local_tags(remote_refs, &ref_map, &tail); if (ref_map) - backfill_tags(transport, ref_map); + backfill_tags(transport, ref_map, worktrees); free_refs(ref_map); } - cleanup: +cleanup: + free_worktrees(worktrees); return retcode; } diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh index 4db8edd9c8..36fb90f4b0 100755 --- a/t/t5516-fetch-push.sh +++ b/t/t5516-fetch-push.sh @@ -1770,4 +1770,22 @@ test_expect_success 'denyCurrentBranch and worktrees' ' git -C cloned push origin HEAD:new-wt && test_must_fail git -C cloned push --delete origin new-wt ' + +test_expect_success 'refuse fetch to current branch of worktree' ' + test_when_finished "git worktree remove --force wt && git branch -D wt" && + git worktree add wt && + test_commit apple && + test_must_fail git fetch . HEAD:wt && + git fetch -u . HEAD:wt +' + +test_expect_success 'refuse fetch to current branch of bare repository worktree' ' + test_when_finished "rm -fr bare.git" && + git clone --bare . bare.git && + git -C bare.git worktree add wt && + test_commit banana && + test_must_fail git -C bare.git fetch .. HEAD:wt && + git -C bare.git fetch -u .. HEAD:wt +' + test_done
Refuse to fetch into the currently checked out branch of any working tree, not just the current one. Fixes this previously reported bug: https://public-inbox.org/git/cb957174-5e9a-5603-ea9e-ac9b58a2eaad@mathema.de As a side effect of using find_shared_symref, we’ll also refuse the fetch when we’re on a detached HEAD because we’re rebasing or bisecting on the branch in question. This seems like a sensible change. Signed-off-by: Anders Kaseorg <andersk@mit.edu> --- builtin/fetch.c | 75 +++++++++++++++++++++++-------------------- t/t5516-fetch-push.sh | 18 +++++++++++ 2 files changed, 58 insertions(+), 35 deletions(-)