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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 1, 2023, 03:45 UTC
Message-ID: <xmqqy1qmhq8k.fsf@gitster.g>
In-Reply-To: <dd86016b-3232-563b-940e-03bc36af917a@gmail.com>

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

Show 6 quoted lines

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

Show 15 quoted lines

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

Show 13 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".