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

[Junio C Hamano <gitster@pobox.com>]

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