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@xxxxxxxxxx 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@xxxxxxx> > --- > 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
