Re: [PATCH 1/2] branch: description for orphan branch errors

> On 01-ene-2023 12:45:47, Junio C Hamano wrote:
>> Rubén Justo <rjusto@gmail.com> 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.

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