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

From: Rubén Justo <rjusto@gmail.com>
Date: Jan 3, 2023, 01:15 UTC
Message-ID: <18ca1e65-3e26-8352-cabd-daebdd0cf7f2@gmail.com>
In-Reply-To: <xmqqy1qmhq8k.fsf@gitster.g>

On 01-ene-2023 12:45:47, Junio C Hamano wrote:

Show 15 quoted lines

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

And again, I agree, the change has somewhat profound implications.

Show 27 quoted lines

> 
> > +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@gmail.com/

Thank you for reading the change this way.