Thread (1 message) 1 message, 1 author, 2021-11-05

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.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help