[PATCH v3 1/2] fetch: Protect branches checked out in all worktrees

[PATCH v3 1/2] fetch: Protect branches checked out in all worktrees

[Anders Kaseorg]

[-- Attachment #1: Type: text/plain, Size: 3633 bytes --]

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       | 27 +++++++++++++--------------
 t/t5516-fetch-push.sh | 11 +++++++++++
 2 files changed, 24 insertions(+), 14 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index f7abbc31ff..61c8fc9983 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)
 
@@ -854,7 +855,7 @@ static int update_local_ref(struct ref *ref,
 			    int summary_width)
 {
 	struct commit *current = NULL, *updated;
-	struct branch *current_branch = branch_get(NULL);
+	const struct worktree *wt;
 	const char *pretty_ref = prettify_refname(ref->name);
 	int fast_forward = 0;
 
@@ -868,16 +869,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()) &&
+	if (!update_head_ok &&
+	    (wt = find_shared_symref("HEAD", ref->name)) &&
 	    !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") :
+			       _("branch checked out in worktree"),
 			       remote, pretty_ref, summary_width);
 		return 1;
 	}
@@ -1387,16 +1389,13 @@ static int prune_refs(struct refspec *rs, struct ref *ref_map,
 
 static void check_not_current_branch(struct ref *ref_map)
 {
-	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("HEAD", ref_map->peer_ref->name)))
+			die(_("Refusing to fetch into branch '%s' "
+			      "checked out at '%s'"),
+			    ref_map->peer_ref->name, wt->path);
 }
 
 static int truncate_fetch_head(void)
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 8212ca56dc..4ef4ecbe71 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1771,4 +1771,15 @@ 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_commit -C cloned second &&
+	test_must_fail git fetch cloned HEAD:new-wt &&
+	git clone --bare . bare.git &&
+	git -C bare.git worktree add bare-wt &&
+	test_must_fail git -C bare.git fetch ../cloned HEAD:bare-wt &&
+	git fetch -u cloned HEAD:new-wt &&
+	git -C bare.git fetch -u ../cloned HEAD:bare-wt
+'
+
 test_done
-- 
2.33.1

Re: [PATCH v3 1/2] fetch: Protect branches checked out in all worktrees

[Junio C Hamano]

Anders Kaseorg <andersk@mit.edu> writes:

> 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.

True.

> -	if (current_branch &&
> -	    !strcmp(ref->name, current_branch->name) &&
> -	    !(update_head_ok || is_bare_repository()) &&
> +	if (!update_head_ok &&
> +	    (wt = find_shared_symref("HEAD", ref->name)) &&
>  	    !is_null_oid(&ref->old_oid)) {

We used to allow "git fetch" into a bare repository to update the
branch that happens to be pointed at by the HEAD symref.  The new
code still allow it, but the way it does so is subtle [*].

    Side note: The new code only works because find-shared-symref
    ignores a bare repository or a bare worktree.  I would not be
    surprised if somebody starts arguing that the behaviour to
    ignore bare worktrees is a bug in that function and may accept a
    patch to correct it, and when I do so, I may not remember that
    this new code depends on that "bug".

I would sleep better if we were one bit more careful, perhaps like
so:

+	if (!update_head_ok &&
+	    (wt = find_shared_symref(...)) &&
+	    !wt->is_bare &&
	    !is_null_oid(...)) {

to make sure we do not rely on that particular aspect of how
find_shared_symref() works.  The function asks "please find a
worktree, if any, whose HEAD points at this ref", and it feels
unnatural for the answer to the question is affected by the
bare-ness of the worktree.

>  		/*
>  		 * 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") :
> +			       _("branch checked out in worktree"),

OK, the former is about this worktree, and the latter is about
worktree somewhere else.  It may clarify if we phrased the latter a
bit differently, e.g. "checked out in another worktree".  Once we
say "check(ed) out", we know we are talking about a branch, and
format_display() would be showing the name of the branch on the same
line anyway, so we could save the 6 letter spaces and tell the user
that it is not happening here, but some other place.

> +test_expect_success 'refuse fetch to current branch of worktree' '
> +	test_commit -C cloned second &&
> +	test_must_fail git fetch cloned HEAD:new-wt &&

This is because at this point in the test sequence, new-wt is the
current branch for the worktree we added in the test immediately
before this one.  And we refuse unless update-head-ok is given.  OK.

> +	git clone --bare . bare.git &&
> +	git -C bare.git worktree add bare-wt &&
> +	test_must_fail git -C bare.git fetch ../cloned HEAD:bare-wt &&

What is being tested here?  We created a bare clone bare.git and
added a worktree bare-wt to it.  And we try to fetch into that bare
repository, which would allow overwriting the branch pointed at by
HEAD (which is new-wt) or any branch if there weren't a worktree
that has a working tree.  But because it has a working tree attached
to it, namely, bare-wt, overwriting the current branch for that
worktree is prevented.  Good.

> +	git fetch -u cloned HEAD:new-wt &&
> +	git -C bare.git fetch -u ../cloned HEAD:bare-wt

These are to ensure that overriding the safety still works fine.
Good.

I cannot shake the feeling that this single test step is testing way
too many things and burden future developers who break one of the
steps to understand which step was broken, but these three are good
things to test.

Overall, looks quite good.

Thanks.


> +'
> +
>  test_done

Re: [PATCH v3 1/2] fetch: Protect branches checked out in all worktrees

[Anders Kaseorg]

On 11/8/21 15:00, Junio C Hamano wrote:
> I would sleep better if we were one bit more careful, perhaps like
> so:
> 
> +	if (!update_head_ok &&
> +	    (wt = find_shared_symref(...)) &&
> +	    !wt->is_bare &&
> 	    !is_null_oid(...)) {
> 
> to make sure we do not rely on that particular aspect of how
> find_shared_symref() works.  The function asks "please find a
> worktree, if any, whose HEAD points at this ref", and it feels
> unnatural for the answer to the question is affected by the
> bare-ness of the worktree.

Not unreasonable to be explicit at the call site.  Will update.

I note that branch.c already relies on this aspect of find_shared_symref().

And, now that I’m reading branch.c again, I see that it also has a 
worktree-awareness bug in its safety check, deserving of a third patch 
in this series.

$ git init
$ git commit --allow-empty -am foo
$ git worktree add wt
$ git branch -M wt main
   # correctly disallowed
fatal: Cannot force update the current branch.

$ git branch -M wt  # incorrectly allowed
$ git worktree list

/tmp/test     6de7a5d [wt]

/tmp/test/wt  6de7a5d [wt]


> OK, the former is about this worktree, and the latter is about
> worktree somewhere else.  It may clarify if we phrased the latter a
> bit differently, e.g. "checked out in another worktree".

Alright.  (Of course, as noted in previous discussion, this error path 
is currently reachable.)

> I cannot shake the feeling that this single test step is testing way
> too many things and burden future developers who break one of the
> steps to understand which step was broken, but these three are good
> things to test.

I can split the bare repository part into a separate test_expect_success 
step?  This duplicates some commands but maybe helps readability overall.

Anders

Re: [PATCH v3 1/2] fetch: Protect branches checked out in all worktrees

[Bagas Sanjaya]

On 09/11/21 03.15, Anders Kaseorg wrote:
> 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.
> 

We can say "we'll also refuse fetching when we're on a detached HEAD (for 
example if we're rebasing on the branch in question or bisecting)."

-- 
An old man doll... just what I always wanted! - Clara

Re: [PATCH v3 1/2] fetch: Protect branches checked out in all worktrees

[Anders Kaseorg]

On 11/8/21 21:19, Bagas Sanjaya wrote:
> On 09/11/21 03.15, Anders Kaseorg wrote:
>> 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.
> 
> We can say "we'll also refuse fetching when we're on a detached HEAD 
> (for example if we're rebasing on the branch in question or bisecting)."

No, I meant what I wrote.  We don’t refuse the fetch if we’re on a 
detached HEAD for some other reason.  That would be quite unhelpful.

Anders