Re: [PATCH v2] fetch: Protect branches checked out in all worktrees
From: Junio C Hamano <hidden>
Date: 2021-11-05 18:39:16
Jeff King [off-list ref] writes:
quoted hunk
But all of that means we could actually drop check_not_current_branch() in favor of the update_local_ref() check. Doing this:diff --git a/builtin/fetch.c b/builtin/fetch.c index f7abbc31ff..c52c44684a 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c@@ -869,7 +869,7 @@ static int update_local_ref(struct ref *ref, } if (current_branch && - !strcmp(ref->name, current_branch->name) && + !strcmp(ref->name, current_branch->refname) && !(update_head_ok || is_bare_repository()) && !is_null_oid(&ref->old_oid)) { /*@@ -1385,20 +1385,6 @@ static int prune_refs(struct refspec *rs, struct ref *ref_map, return result; } -static void check_not_current_branch(struct ref *ref_map) -{ - struct branch *current_branch = branch_get(NULL); - - if (is_bare_repository() || !current_branch) - return; - - 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); -} - static int truncate_fetch_head(void) { const char *filename = git_path_fetch_head(the_repository);@@ -1587,8 +1573,6 @@ 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); if (tags == TAGS_DEFAULT && autotags) transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, "1");passes the entire test suite except for one test which expects: git fetch . side:main to fail. But that is only because "side" and "main" point to the same commit, and thus the fetch is a noop. The code in update_local_ref() covers that case before checking the HEAD case (which I would argue is a completely reasonable outcome).
So we can lose redundant code that was added there only because the original safety was broken by actually fixing the original safety? That is very nice.
The reason I bring this up is that I think doing the check in update_local_ref() makes much more sense. We don't abort the whole fetch, but just treat it as a normal per-ref failure. That gives us the usual status-table output (I thought it might also avoid wasting some work of actually fetching objects, but I think the current check kicks in before we actually fetch anything).
Yes I agree with that reasoning. Thanks for digging this to the bottom, both of you.