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

["Rubén Justo" <rjusto@gmail.com>]

On 04-ene-2023 15:58:02, Junio C Hamano wrote:
> 
> > -	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))) {
> 

Thinking of this as a whole, perhaps after this series we can add:

-- >8 --
Subject: [PATCH] branch: copy_or_rename_branch() unconditionally calls

In previous commits we have introduced changes to
copy_or_rename_branch() that lead to unconditionally calling
ref_exists(), twice in some circumstances.

Optimize copy_or_rename_branch() so that it only calls ref_exists() once
and reorder some conditionals to consider ref_exists() first and avoid
unnecessarily calling other expensive functions.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 builtin/branch.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index c14a7a42e6..6e70377a1a 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -515,7 +515,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
 	struct strbuf oldsection = STRBUF_INIT, newsection = STRBUF_INIT;
 	const char *interpreted_oldname = NULL;
 	const char *interpreted_newname = NULL;
-	int recovery = 0;
+	int recovery = 0, oldref_exists;
 
 	if (strbuf_check_branch_ref(&oldref, oldname)) {
 		/*
@@ -523,12 +523,13 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
 		 * ref that we used to allow to be created by accident.
 		 */
 		if (ref_exists(oldref.buf))
-			recovery = 1;
+			oldref_exists = recovery = 1;
 		else
 			die(_("Invalid branch name: '%s'"), oldname);
-	}
+	} else
+		oldref_exists = ref_exists(oldref.buf);
 
-	if ((copy || !branch_checked_out(oldref.buf)) && !ref_exists(oldref.buf)) {
+	if (!oldref_exists && (copy || !branch_checked_out(oldref.buf))) {
 		if (copy && branch_checked_out(oldref.buf))
 			die(_("No commit on branch '%s' yet."), oldname);
 		else
@@ -558,8 +559,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
 		strbuf_addf(&logmsg, "Branch: renamed %s to %s",
 			    oldref.buf, newref.buf);
 
-	if (!copy &&
-	    (!branch_checked_out(oldref.buf) || ref_exists(oldref.buf)) &&
+	if (!copy && oldref_exists &&
 	    rename_ref(oldref.buf, newref.buf, logmsg.buf))
 		die(_("Branch rename failed"));
 	if (copy && copy_existing_ref(oldref.buf, newref.buf, logmsg.buf))

base-commit: 64b4d8c0eb1938fa10477b9bd9aead2773456e3e
--

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

Yes.  We did not have a test for 'No branch named' that implied an
orphan branch.  I think if we had tried that, we would have ended
up doing what we're doing now.

> 
> And given that, do we now have a complete test coverage for all
> cases with the patch we are discussing?

Considering 1/2 and 2/2, I think so. But if you're asking maybe
you're realizing something...