On 01-ene-2023 12:45:47, Junio C Hamano wrote:
> Rubén Justo <rjusto@xxxxxxxxx> writes:
>
> Isn't branch_checked_out() a fairly heavyweight operation when you
> have multiple worktrees? The original went to the filesystem
> (i.e. check ref_exists()) only after seeing that a condition that
> can be computed using only in-core data holds (i.e. the branch names
> are the same or we are doing a copy), which is an optimum order to
> avoid doing unnecessary work in most common cases, but I am not sure
> if the order the checks are done in the updated code optimizes for
> the common case. If branch_checked_out() is more expensive than a
> call to ref_exists() for a single brnch, that would change the
> equation. Calling such a heavyweight operation twice would make it
> even more expensive but that is a perfectly fine thing to do in the
> error codepath, inside the block that is entered after we noticed an
> error condition.
I share your concern, I thought about it.
My thoughts evaluating the change were more or less:
- presumably there should be many more references than worktrees, and
more repositories with 0 or 1 workdirs than more, so, arbitrarily,
calling ref_exists() last still sounds sensible
- strcmp() to branch_checked_out() introduces little change in the
logic
- I like branch_checked_out(), it expresses better what we want there
- branch_checked_out() considers refs, strcmp considers branch names
(we have a corner case with @{-1} pointing to HEAD, that this
resolves)
- finally, perhaps branch_checked_out() has optimization possibilities.
Maybe in the common case we can get close to the amount of work we
are doing now. Here we can alleviate a bit by removing the
unconditional resolve_refdup(HEAD) we are doing at the beginning of
cmd_branch().
In the end, it seems to me that we have some places where we are
considering HEAD and we may need to consider HEADs.
And again, I agree, the change has somewhat profound implications.
>
> > +test_expect_success 'error descriptions on orphan or unborn-yet branch' '
> > + cat >expect <<-EOF &&
> > + error: No commit on branch '\''orphan-branch'\'' yet.
> > + EOF
> > ...
> > +'
> > +
> > +test_expect_success 'fatal descriptions on orphan or unborn-yet branch' '
> > + cat >expect <<-EOF &&
> > + fatal: No commit on branch '\''orphan-branch'\'' yet.
> > + EOF
> > ...
> > +'
>
> Do we already cover existing "No branch named" case the same way in
> this test script, so that it is OK for these new tests to cover only
> the "not yet" cases? I am asking because if we have existing
> coverage, before and after the change to the C code in this patch,
> some of the existing tests would change the behaviour (i.e. they
> would have said "No branch named X" when somebody else created an
> unborn branch in a separate worktree, but now they would say "No
> commit on branch X yet"), but I see no such change in the test. If
> we lack existing coverage, we probably should --- otherwise we would
> not notice when somebody breaks the command to say "No commit on
> branch X yet" when it should say "No such branch X".
>
I think we do, bcfc82bd (branch: description for non-existent branch
errors). We have some pending changes to follow the CodingGuideLines in
this messages that maybe we can resume:
https://lore.kernel.org/git/eb3c689e-efeb-4468-a10f-dd32bc0ee37b@xxxxxxxxx/
Thank you for reading the change this way.
