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
