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

From: Anders Kaseorg <andersk@mit.edu>
Date: Nov 8, 2021, 23:31 UTC
Message-ID: <8709db5c-8ac0-b327-09ba-087e392c297b@mit.edu>
In-Reply-To: <xmqqa6ie5k3u.fsf@gitster.g>

On 11/8/21 15:00, Junio C Hamano wrote:

Show 13 quoted lines

> 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