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