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

[Junio C Hamano <gitster@pobox.com>]

Rubén Justo <rjusto@gmail.com> writes:

> In bcfc82bd48 (branch: description for non-existent branch errors,
> 2022-10-08) we used "strcmp(head, branch)" to check if we're asked to
> operate in a branch that is the current HEAD, and
> "ref_exists(branch_ref)" to check if it points to a valid ref, i.e. it
> is an orphan branch.  We are doing this with the intention of avoiding
> the confusing error: "No branch named..."

I agree that it is good to notice "the user thinks the branch should
already exist, for they have 'checked out' that branch, but there is
no commit on the branch yet".  I am not sure checked out on a separate
worktree as an unborn branch is always the indication that the user
thinks the branch should exist (e.g. "you allowed somebody else, or
yourself, prepare a separate worktree to work on something a few
weeks ago, but no single commit has been made there and you forgot
about the worktree---should the branch still exist?"), but that is a
separate topic.  Let's assume that the two are equivalent.

> diff --git a/builtin/branch.c b/builtin/branch.c
> index f63fd45edb..954008e51d 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -528,8 +528,8 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
>  			die(_("Invalid branch name: '%s'"), oldname);
>  	}
>  
> -	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)) {
> +		if (copy && branch_checked_out(oldref.buf))
>  			die(_("No commit on branch '%s' yet."), oldname);
>  		else
>  			die(_("No branch named '%s'."), oldname);

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.

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