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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 8, 2021, 23:00 UTC
Message-ID: <xmqqa6ie5k3u.fsf@gitster.g>
In-Reply-To: <alpine.DEB.2.21.999.2111081514460.100671@scrubbing-bubbles.mit.edu>

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.

Show 6 quoted lines

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

Show 9 quoted lines

>  		/*
>  		 * 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