Rubén Justo <rjusto@xxxxxxxxx> writes:
> 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.
I am not sure what you share with me after reading the above,
though. As I already said, I am not opposed to the use of
branch_checked_out(), or checking the state of other worktrees
connected to the same repository, at all.
I was merely looking at this:
> - if ((copy || strcmp(head, oldname)) && !ref_exists(oldref.buf)) {
> - if (copy && !strcmp(head, oldname))
> + if ((copy || !branch_checked_out(oldref.buf)) && !ref_exists(oldref.buf)) {
and wondering if the evaluation order to call branch_checked_out()
unconditionally and then calling ref_exists() still makes sense, or
now the strcmp() part of the original has become so much more
costly, if we are better off doing the same thing in a different
order, e.g.
if (!ref_exists(oldref.buf) && (copy || !branch_checked_out(oldref.buf))) {
>> 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).
If we already have checks that current code triggers the "no branch
named X" warning, and because the patch is changing the code to give
that warning to instead say "branch X has no commits yet" in some
cases, if the existing test coverage were thorough, some of the
existing tests that expect "no branch named X" warning should now
expect "branch X has no commits yet" warning. Your patch did not
have any such change in the tests, which was an indication that the
existing test coverage was lacking, no?
And given that, do we now have a complete test coverage for all
cases with the patch we are discussing?
Thanks.
