[PATCH 0/2] branch: operations on orphan branches

[PATCH 0/2] branch: operations on orphan branches

[Rubén Justo]

Avoid some confusing errors operating on orphan branches when
working with worktrees


Rubén Justo (2):
  branch: description for orphan branch errors
  branch: rename orphan branches in any worktree

 builtin/branch.c       | 10 +++++-----
 t/t3200-branch.sh      | 12 ++++++++++++
 t/t3202-show-branch.sh | 36 ++++++++++++++++++++++++++++++++++++
 3 files changed, 53 insertions(+), 5 deletions(-)

-- 
2.39.0

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

[Rubén Justo]

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

If we're asked to operate with an orphan branch, but it is on a
different worktree, "strcmp(head, branch)" is not going to match:

	$ git worktree add orphan-worktree --detach
	$ git -C orphan-worktree checkout --orphan orpha-branch
	$ git branch -m orpha-branch orphan-branch
	fatal: No branch named 'orpha-branch'.

Let's do the check for HEAD in any worktree in the repository, removing
the "strcmp" and using the helper introduced in 31ad6b61bd (branch: add
branch_checked_out() helper, 2022-06-15)

This commit also extends the tests introduced on bcfc82bd48, in
t3202-show-branch, to cover not just the initial branch but any orphan
branch.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 builtin/branch.c       |  8 ++++----
 t/t3202-show-branch.sh | 36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+), 4 deletions(-)

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);
@@ -806,7 +806,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 
 		strbuf_addf(&branch_ref, "refs/heads/%s", branch_name);
 		if (!ref_exists(branch_ref.buf))
-			error((!argc || !strcmp(head, branch_name))
+			error((!argc || branch_checked_out(branch_ref.buf))
 			      ? _("No commit on branch '%s' yet.")
 			      : _("No branch named '%s'."),
 			      branch_name);
@@ -851,7 +851,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		}
 
 		if (!ref_exists(branch->refname)) {
-			if (!argc || !strcmp(head, branch->name))
+			if (!argc || branch_checked_out(branch->refname))
 				die(_("No commit on branch '%s' yet."), branch->name);
 			die(_("branch '%s' does not exist"), branch->name);
 		}
diff --git a/t/t3202-show-branch.sh b/t/t3202-show-branch.sh
index ea7cfd1951..acaedac34e 100755
--- a/t/t3202-show-branch.sh
+++ b/t/t3202-show-branch.sh
@@ -221,4 +221,40 @@ test_expect_success 'fatal descriptions on non-existent branch' '
 	test_cmp expect actual
 '
 
+test_expect_success 'error descriptions on orphan or unborn-yet branch' '
+	cat >expect <<-EOF &&
+	error: No commit on branch '\''orphan-branch'\'' yet.
+	EOF
+	test_when_finished git worktree remove -f orphan-worktree &&
+	git worktree add orphan-worktree --detach &&
+	git -C orphan-worktree checkout --orphan orphan-branch &&
+	test_must_fail git -C orphan-worktree branch --edit-description 2>actual && # implicit branch
+	test_cmp expect actual &&
+	test_must_fail git -C orphan-worktree branch --edit-description orphan-branch 2>actual && # explicit branch
+	test_cmp expect actual &&
+	test_must_fail git branch --edit-description orphan-branch 2>actual && # different worktree
+	test_cmp expect actual
+'
+
+test_expect_success 'fatal descriptions on orphan or unborn-yet branch' '
+	cat >expect <<-EOF &&
+	fatal: No commit on branch '\''orphan-branch'\'' yet.
+	EOF
+	test_when_finished git worktree remove -f orphan-worktree &&
+	git worktree add orphan-worktree --detach &&
+	git -C orphan-worktree checkout --orphan orphan-branch &&
+	test_must_fail git -C orphan-worktree branch --set-upstream-to=non-existent 2>actual && # implicit branch
+	test_cmp expect actual &&
+	test_must_fail git -C orphan-worktree branch --set-upstream-to=non-existent orphan-branch 2>actual && # explicit branch
+	test_cmp expect actual &&
+	test_must_fail git branch --set-upstream-to=non-existent orphan-branch 2>actual && # different worktree
+	test_cmp expect actual &&
+	test_must_fail git -C orphan-worktree branch -c new-branch 2>actual && # implicit branch
+	test_cmp expect actual &&
+	test_must_fail git -C orphan-worktree branch -c orphan-branch new-branch 2>actual && # explicit branch
+	test_cmp expect actual &&
+	test_must_fail git branch -c orphan-branch new-branch 2>actual && # different worktree
+	test_cmp expect actual
+'
+
 test_done
-- 
2.39.0

[PATCH 2/2] branch: rename orphan branches in any worktree

[Rubén Justo]

In cfaff3aac (branch -m: allow renaming a yet-unborn branch, 2020-12-13)
we added support for renaming an orphan branch in the current working
tree, avoiding the rename_ref() error because an orphan branch does not
point to any ref.

We did this with two checks (note that head cannot be null due to a
precondition in cmd_branch):

 	 if (branch_is_not_head)
 	 	// Only HEAD can be orphan. Do the rename_ref
 	 else if (head_ref_is_valid)
 		// There is a valid ref. Do the rename_ref
 	 else
 		// Nothing to do here. Continue

With worktrees, we can try to rename an orphan branch while we are in a
different worktree, and so the orphan branch we want to rename is not
the HEAD in the current worktree:

	$ git worktree add orphan-worktree --detach
	$ git -C orphan-worktree checkout --orphan orphn-branch
	$ git branch -m orphn-branch orphan-branch
	fatal: No branch named 'orphn-branch'.

Lets fix this considering all HEADs in all worktrees in the repository,
changing the checks introduced in cfaff3aac8 to:

 	 if (branch_is_not_head_in_any_worktree)
 	 	// Only HEADs can be orphan. Do the rename_ref
 	 else if (branch_ref_is_valid)
 		// There is a valid ref. Do the rename_ref
 	 else
 		// Nothing to do here. Continue

Let's also add to t3200-branch tests for this and for normal renames of
orphan branches (other than the initial branch) for which we did not yet
have tests.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 builtin/branch.c  |  2 +-
 t/t3200-branch.sh | 12 ++++++++++++
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 954008e51d..151541e1c2 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -559,7 +559,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
 			    oldref.buf, newref.buf);
 
 	if (!copy &&
-	    (!head || strcmp(oldname, head) || !is_null_oid(&head_oid)) &&
+	    (!branch_checked_out(oldref.buf) || ref_exists(oldref.buf)) &&
 	    rename_ref(oldref.buf, newref.buf, logmsg.buf))
 		die(_("Branch rename failed"));
 	if (copy && copy_existing_ref(oldref.buf, newref.buf, logmsg.buf))
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 5a169b68d6..01d616af05 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -279,6 +279,18 @@ test_expect_success 'git branch -M and -C fail on detached HEAD' '
 	test_cmp expect err
 '
 
+test_expect_success 'git branch -m should work with orphan branches' '
+	test_when_finished git worktree remove -f orphan-worktree &&
+	test_when_finished git checkout - &&
+	git worktree add orphan-worktree --detach &&
+	git -C orphan-worktree checkout --orphan orphan-foo &&
+	git branch -m orphan-foo orphan-bar-wt &&
+	test orphan-bar-wt=$(git -C orphan-worktree branch --show-current) &&
+	git checkout --orphan orphan-foo &&
+	git branch -m orphan-foo orphan-bar &&
+	test orphan-bar=$(git branch --show-current)
+'
+
 test_expect_success 'git branch -d on orphan HEAD (merged)' '
 	test_when_finished git checkout main &&
 	git checkout --orphan orphan &&
-- 
2.39.0

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

[Junio C Hamano]

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

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

[Rubén Justo]

On 01-ene-2023 12:45:47, Junio C Hamano wrote:
> 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.

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

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

[Junio C Hamano]

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

> On 01-ene-2023 12:45:47, Junio C Hamano wrote:
>> 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.

I am not sure what you share with me after reading the above,
though.  As I already said, I am not opposed to the use of
branch_checked_out(), or checking the state of other worktrees
connected to the same repository, at all.

I was merely looking at this:

> -	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))) {

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

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

Thanks.

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

[Rubén Justo]

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

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

[Junio C Hamano]

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

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

Why "after"?  If we already know that the existing patches are
making things worse and need to fix the regression with a future
patch to make it usable again, why introduce a regression in the
first place?

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

[Junio C Hamano]

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

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

No, I am trying to make sure that you have thought it through.

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

[Rubén Justo]

On 7/1/23 0:59, Junio C Hamano wrote:
> Rubén Justo <rjusto@gmail.com> writes:
> 
>> Thinking of this as a whole, perhaps after this series we can add:
> 
> Why "after"?  If we already know that the existing patches are
> making things worse and need to fix the regression with a future
> patch to make it usable again, why introduce a regression in the
> first place?
> 

I'm not sure if it is so worse, and if the optimization is a fix.

We're actually paying for worktrees twice in:
reject_rebase_or_bisedt_branch() and
replace_each_worktree_head_symref().

Making the change this way makes more obvious IMHO what we are moving
and why.

Start moving the ref_exists() in 1/2, easily leads to 2/1 and this patch
squashed with 1/2, for little gain (IMHO) and worse history.

This is why I think it's a good sequence.  But I understand your point
and I'm not opposed to doing it as you suggest if you think the current
way doesn't pay off.

[PATCH v2 0/3] branch: operations on orphan branches

[Rubén Justo]

Avoid some confusing errors operating on orphan branches when
working with worktrees.

Thanks.



Rubén Justo (3):
  branch: avoid unnecessary worktrees traversing
  branch: description for orphan branch errors
  branch: rename orphan branches in any worktree

 builtin/branch.c       | 40 ++++++++++++++++++++--------------------
 t/t3200-branch.sh      | 16 ++++++++++++++++
 t/t3202-show-branch.sh | 18 ++++++++++++++++++
 3 files changed, 54 insertions(+), 20 deletions(-)

-- 
2.39.0

[PATCH v2 1/3] avoid unnecessary worktrees traversing

[Rubén Justo]

"reject_rebase_or_bisect_branch()" was introduced [1] to prevent a
branch under bisect or rebase from being renamed or copied.  It
traverses all worktrees in the repository and dies if the specified
branch is being rebased or bisected in any them.

"replace_each_worktree_head_symref()" was introduced [2] to adjust the
HEAD in all worktrees after a branch rename succeeded.  It traverses all
worktrees in the repository and if any of them have their HEAD pointing
to the renamed ref, it adjusts it.

If we could know in advance if the renamed branch is not HEAD in any
worktree we could avoid calling "replace_each_worktree_head_symref()".

Let's have "reject_rebase_or_bisect_branch()" check and return whether
the specified branch is HEAD in any worktree, and use this information
to avoid unnecessary calls to "replace_each_worktree_head_symref()".

  1.- 14ace5b (branch: do not rename a branch under bisect or rebase,
      2016-04-22)

  2.- 70999e9 (branch -m: update all per-worktree HEADs, 2016-03-27)

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

diff --git a/builtin/branch.c b/builtin/branch.c
index f63fd45edb..a1ee728ca3 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -486,14 +486,17 @@ static void print_current_branch_name(void)
 		die(_("HEAD (%s) points outside of refs/heads/"), refname);
 }
 
-static void reject_rebase_or_bisect_branch(const char *target)
+static int ishead_and_reject_rebase_or_bisect_branch(const char *target)
 {
 	struct worktree **worktrees = get_worktrees();
-	int i;
+	int i, ret = 0;
 
 	for (i = 0; worktrees[i]; i++) {
 		struct worktree *wt = worktrees[i];
 
+		if (wt->head_ref && !strcmp(target, wt->head_ref))
+			ret = 1;
+
 		if (!wt->is_detached)
 			continue;
 
@@ -507,6 +510,7 @@ static void reject_rebase_or_bisect_branch(const char *target)
 	}
 
 	free_worktrees(worktrees);
+	return ret;
 }
 
 static void copy_or_rename_branch(const char *oldname, const char *newname, int copy, int force)
@@ -515,7 +519,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, ishead;
 
 	if (strbuf_check_branch_ref(&oldref, oldname)) {
 		/*
@@ -544,7 +548,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
 	else
 		validate_new_branchname(newname, &newref, force);
 
-	reject_rebase_or_bisect_branch(oldref.buf);
+	ishead = ishead_and_reject_rebase_or_bisect_branch(oldref.buf);
 
 	if (!skip_prefix(oldref.buf, "refs/heads/", &interpreted_oldname) ||
 	    !skip_prefix(newref.buf, "refs/heads/", &interpreted_newname)) {
@@ -574,7 +578,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
 				interpreted_oldname);
 	}
 
-	if (!copy &&
+	if (!copy && ishead &&
 	    replace_each_worktree_head_symref(oldref.buf, newref.buf, logmsg.buf))
 		die(_("Branch renamed to %s, but HEAD is not updated!"), newname);
 
-- 
2.39.0

[PATCH v2 2/3] branch: description for orphan branch errors

[Rubén Justo]

In bcfc82bd48 (branch: description for non-existent branch errors,
2022-10-08) we check the current HEAD to detect if the branch to operate
with is an orphan branch, to avoid the confusing error: "No branch
named...".

If we are asked to operate with an orphan branch in a different working
tree than the current one, we need to check the HEAD in that different
working tree.

Let's extend the check we did in bcfc82db48, to all HEADs in the
repository, using the helper introduced in 31ad6b61bd (branch: add
branch_checked_out() helper, 2022-06-15)

"ishead_reject_rebase_or_bised_branch()" already returns whether the
branch to operate with is HEAD in any working tree in the repository.
Let's use this information in "copy_or_rename_branch()", instead of the
helper.

Note that we now call reject_rebase_or_bisect_branch() earlier, which
introduces a change in the error displayed when a combination of
unsupported conditions occur simultaneously: the desired destination
name is invalid, and the branch to operate on is being overrun or
bisected.  With "foo" being rebased or bisected, this:

	$ git branch -m foo HEAD
	fatal: 'HEAD' is not a valid branch name.

... becomes:

	$ git branch -m foo HEAD
	fatal: Branch refs/heads/foo is being ...

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 builtin/branch.c       | 25 +++++++++++--------------
 t/t3202-show-branch.sh | 18 ++++++++++++++++++
 2 files changed, 29 insertions(+), 14 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index a1ee728ca3..6bb5f50950 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -532,12 +532,12 @@ 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))
-			die(_("No commit on branch '%s' yet."), oldname);
-		else
-			die(_("No branch named '%s'."), oldname);
-	}
+	ishead = ishead_and_reject_rebase_or_bisect_branch(oldref.buf);
+
+	if ((copy || !ishead) && !ref_exists(oldref.buf))
+		die(ishead
+		    ? _("No commit on branch '%s' yet.")
+		    : _("No branch named '%s'."), oldname);
 
 	/*
 	 * A command like "git branch -M currentbranch currentbranch" cannot
@@ -548,8 +548,6 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
 	else
 		validate_new_branchname(newname, &newref, force);
 
-	ishead = ishead_and_reject_rebase_or_bisect_branch(oldref.buf);
-
 	if (!skip_prefix(oldref.buf, "refs/heads/", &interpreted_oldname) ||
 	    !skip_prefix(newref.buf, "refs/heads/", &interpreted_newname)) {
 		BUG("expected prefix missing for refs");
@@ -810,7 +808,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 
 		strbuf_addf(&branch_ref, "refs/heads/%s", branch_name);
 		if (!ref_exists(branch_ref.buf))
-			error((!argc || !strcmp(head, branch_name))
+			error((!argc || branch_checked_out(branch_ref.buf))
 			      ? _("No commit on branch '%s' yet.")
 			      : _("No branch named '%s'."),
 			      branch_name);
@@ -854,11 +852,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 			die(_("no such branch '%s'"), argv[0]);
 		}
 
-		if (!ref_exists(branch->refname)) {
-			if (!argc || !strcmp(head, branch->name))
-				die(_("No commit on branch '%s' yet."), branch->name);
-			die(_("branch '%s' does not exist"), branch->name);
-		}
+		if (!ref_exists(branch->refname))
+			die((!argc || branch_checked_out(branch->refname))
+			    ? _("No commit on branch '%s' yet.")
+			    : _("branch '%s' does not exist"), branch->name);
 
 		dwim_and_setup_tracking(the_repository, branch->name,
 					new_upstream, BRANCH_TRACK_OVERRIDE,
diff --git a/t/t3202-show-branch.sh b/t/t3202-show-branch.sh
index ea7cfd1951..be20ebe1d5 100755
--- a/t/t3202-show-branch.sh
+++ b/t/t3202-show-branch.sh
@@ -221,4 +221,22 @@ test_expect_success 'fatal descriptions on non-existent branch' '
 	test_cmp expect actual
 '
 
+test_expect_success 'error descriptions on orphan branch' '
+	test_when_finished git worktree remove -f wt &&
+	git worktree add wt --detach &&
+	git -C wt checkout --orphan orphan-branch &&
+	test_branch_op_in_wt() {
+		test_orphan_error() {
+			test_must_fail git $* 2>actual &&
+			test_i18ngrep "No commit on branch .orphan-branch. yet.$" actual
+		} &&
+		test_orphan_error -C wt branch $1 $2 &&                # implicit branch
+		test_orphan_error -C wt branch $1 orphan-branch $2 &&  # explicit branch
+		test_orphan_error branch $1 orphan-branch $2           # different worktree
+	} &&
+	test_branch_op_in_wt --edit-description &&
+	test_branch_op_in_wt --set-upstream-to=ne &&
+	test_branch_op_in_wt -c new-branch
+'
+
 test_done
-- 
2.39.0

[PATCH 3/3] branch: rename orphan branches in any worktree

[Rubén Justo]

In cfaff3aac (branch -m: allow renaming a yet-unborn branch, 2020-12-13)
we added support for renaming an orphan branch, skipping rename_ref() if
the branch to rename is an orphan branch, checking the current HEAD.

But the orphan branch to be renamed can be an orphan branch in a
different working tree than the current one, i.e. not the current HEAD.

Let's make "ishead_reject_rebase_or_bisect_branch()" return a flag
indicating if the returned value refers to an orphan branch, and use it
to extend what we did in cfaff3aac, to all HEADs in the repository.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 builtin/branch.c  |  5 ++---
 t/t3200-branch.sh | 16 ++++++++++++++++
 2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 6bb5f50950..7e6baa291a 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -495,7 +495,7 @@ static int ishead_and_reject_rebase_or_bisect_branch(const char *target)
 		struct worktree *wt = worktrees[i];
 
 		if (wt->head_ref && !strcmp(target, wt->head_ref))
-			ret = 1;
+			ret = 1 + (is_null_oid(&wt->head_oid) ? 1 : 0);
 
 		if (!wt->is_detached)
 			continue;
@@ -560,8 +560,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 &&
-	    (!head || strcmp(oldname, head) || !is_null_oid(&head_oid)) &&
+	if (!copy && !(ishead > 1) &&
 	    rename_ref(oldref.buf, newref.buf, logmsg.buf))
 		die(_("Branch rename failed"));
 	if (copy && copy_existing_ref(oldref.buf, newref.buf, logmsg.buf))
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 5a169b68d6..966583dc7d 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -279,6 +279,22 @@ test_expect_success 'git branch -M and -C fail on detached HEAD' '
 	test_cmp expect err
 '
 
+test_expect_success 'git branch -m should work with orphan branches' '
+	test_when_finished git checkout - &&
+	test_when_finished git worktree remove -f wt &&
+	git worktree add wt --detach &&
+
+	# rename orphan in another worktreee
+	git -C wt checkout --orphan orphan-foo-wt &&
+	git branch -m orphan-foo-wt orphan-bar-wt &&
+	test orphan-bar-wt=$(git -C orphan-worktree branch --show-current) &&
+
+	# rename orphan in the current worktree
+	git checkout --orphan orphan-foo &&
+	git branch -m orphan-foo orphan-bar &&
+	test orphan-bar=$(git branch --show-current)
+'
+
 test_expect_success 'git branch -d on orphan HEAD (merged)' '
 	test_when_finished git checkout main &&
 	git checkout --orphan orphan &&
-- 
2.39.0

[PATCH v2 3/3] branch: rename orphan branches in any worktree

[Rubén Justo]

In cfaff3aac (branch -m: allow renaming a yet-unborn branch, 2020-12-13)
we added support for renaming an orphan branch, skipping rename_ref() if
the branch to rename is an orphan branch, checking the current HEAD.

But the orphan branch to be renamed can be an orphan branch in a
different working tree than the current one, i.e. not the current HEAD.

Let's make "ishead_reject_rebase_or_bisect_branch()" return a flag
indicating if the returned value refers to an orphan branch, and use it
to extend what we did in cfaff3aac, to all HEADs in the repository.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 builtin/branch.c  |  5 ++---
 t/t3200-branch.sh | 16 ++++++++++++++++
 2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 6bb5f50950..7e6baa291a 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -495,7 +495,7 @@ static int ishead_and_reject_rebase_or_bisect_branch(const char *target)
 		struct worktree *wt = worktrees[i];
 
 		if (wt->head_ref && !strcmp(target, wt->head_ref))
-			ret = 1;
+			ret = 1 + (is_null_oid(&wt->head_oid) ? 1 : 0);
 
 		if (!wt->is_detached)
 			continue;
@@ -560,8 +560,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 &&
-	    (!head || strcmp(oldname, head) || !is_null_oid(&head_oid)) &&
+	if (!copy && !(ishead > 1) &&
 	    rename_ref(oldref.buf, newref.buf, logmsg.buf))
 		die(_("Branch rename failed"));
 	if (copy && copy_existing_ref(oldref.buf, newref.buf, logmsg.buf))
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 5a169b68d6..966583dc7d 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -279,6 +279,22 @@ test_expect_success 'git branch -M and -C fail on detached HEAD' '
 	test_cmp expect err
 '
 
+test_expect_success 'git branch -m should work with orphan branches' '
+	test_when_finished git checkout - &&
+	test_when_finished git worktree remove -f wt &&
+	git worktree add wt --detach &&
+
+	# rename orphan in another worktreee
+	git -C wt checkout --orphan orphan-foo-wt &&
+	git branch -m orphan-foo-wt orphan-bar-wt &&
+	test orphan-bar-wt=$(git -C orphan-worktree branch --show-current) &&
+
+	# rename orphan in the current worktree
+	git checkout --orphan orphan-foo &&
+	git branch -m orphan-foo orphan-bar &&
+	test orphan-bar=$(git branch --show-current)
+'
+
 test_expect_success 'git branch -d on orphan HEAD (merged)' '
 	test_when_finished git checkout main &&
 	git checkout --orphan orphan &&
-- 
2.39.0

Re: [PATCH v2 1/3] avoid unnecessary worktrees traversing

[Junio C Hamano]

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

> -static void reject_rebase_or_bisect_branch(const char *target)
> +static int ishead_and_reject_rebase_or_bisect_branch(const char *target)

The original name was already horrible but it became much worse by
acquiring a non-word "ishead" as part of it X-<.

At least let's replace "rebase or bisect" with something a bit more
generic, extensible, and shorter phrase.  For example, isn't the
point of having the function was to give us a mechansim to see if
the branch with the given name is not to be modified because it is
being worked on elsewhere?  "The branch is in use" would be a good
phrase to express such a concept, so die_if_branch_is_in_use() or
something along that line may be easier to grok.

Re: [PATCH 3/3] branch: rename orphan branches in any worktree

[Junio C Hamano]

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

> +	if (!copy && !(ishead > 1) &&

Logically it might be necessary to be able to extend "is that branch
what we have checked out, yes or no?" bool into something else that
can be something other than 0 or 1, but as soon as you did so,
"is_head" is no longer a Boolean "is it a HEAD, yes or no?".

Now what does that value really _mean_?  Please rename the variable
and helper function appropriately to make it clear what is going on.

Re: [PATCH v2 1/3] avoid unnecessary worktrees traversing

[Rubén Justo]

On 19-ene-2023 13:24:45, Junio C Hamano wrote:
> Rubén Justo <rjusto@gmail.com> writes:
> 
> > -static void reject_rebase_or_bisect_branch(const char *target)
> > +static int ishead_and_reject_rebase_or_bisect_branch(const char *target)
> 
> The original name was already horrible but it became much worse by
> acquiring a non-word "ishead" as part of it X-<.
> 
> At least let's replace "rebase or bisect" with something a bit more
> generic, extensible, and shorter phrase.  For example, isn't the
> point of having the function was to give us a mechansim to see if
> the branch with the given name is not to be modified because it is
> being worked on elsewhere?  "The branch is in use" would be a good
> phrase to express such a concept, so die_if_branch_is_in_use() or
> something along that line may be easier to grok.

I agree, the naming is ugly.

The idea is to return, if not die(), from the iteration we are doing in
that function, whether the branch is checked out in any worktree.  That
information allows us later, if we know in advance that no HEAD needs to
be adjusted, to avoid calling replace_each_worktree_head_symref(),
saving us a new and unnecessary traversal of the worktrees.

There is a second idea to, in next commits, return also if the branch
is an unborn branch.

die_if_branch_is_in_use() is a better name for reject_rebase_or_... but
don't know how it fits with these ideas.  I'm open to suggestions.

I'll reroll with a better approach.

Thank you.

Re: [PATCH 3/3] branch: rename orphan branches in any worktree

[Rubén Justo]

On 19-ene-2023 13:33:06, Junio C Hamano wrote:
> Rubén Justo <rjusto@gmail.com> writes:
> 
> > +	if (!copy && !(ishead > 1) &&
> 
> Logically it might be necessary to be able to extend "is that branch
> what we have checked out, yes or no?" bool into something else that
> can be something other than 0 or 1, but as soon as you did so,
> "is_head" is no longer a Boolean "is it a HEAD, yes or no?".
> 
> Now what does that value really _mean_?  Please rename the variable
> and helper function appropriately to make it clear what is going on.

The idea is that an unborn branch needs to be a HEAD, so (head > 1)
codifies that information.

As I said in another reply in this thread, I'm going to reroll.  I hope
to make it clearer then.

Thank you.

[PATCH v3 0/3] branch: operations on orphan branches

[Rubén Justo]

Avoid some confusing errors operating with orphan branches when
working with worktrees.

Changes from v2:

 - Renamed "ishead_and_reject_rebase_or_bisect_branch()" to
   "die_if_branch_is_being_rebased_or_bisected()"

   A proposed name "die_if_branch_is_is_use()" has not been used because
   it could lead to confusion.  We don't yet support copying or renaming
   a branch being rebased or bisected, but we do under other uses.

 - Comments added and variables renamed to make more clear the intention and
   the functioning.


Rubén Justo (3):
  branch: avoid unnecessary worktrees traversals
  branch: description for orphan branch errors
  branch: rename orphan branches in any worktree

 builtin/branch.c       | 47 ++++++++++++++++++++++++------------------
 t/t3200-branch.sh      | 14 +++++++++++++
 t/t3202-show-branch.sh | 18 ++++++++++++++++
 3 files changed, 59 insertions(+), 20 deletions(-)

Range-diff against v2:
1:  d16819bc61 ! 1:  50fa7ed076 avoid unnecessary worktrees traversing
    @@ Metadata
     Author: Rubén Justo <rjusto@gmail.com>
     
      ## Commit message ##
    -    avoid unnecessary worktrees traversing
    +    branch: avoid unnecessary worktrees traversals
     
         "reject_rebase_or_bisect_branch()" was introduced [1] to prevent a
         branch under bisect or rebase from being renamed or copied.  It
         traverses all worktrees in the repository and dies if the specified
    -    branch is being rebased or bisected in any them.
    +    branch is being rebased or bisected in any of them.
     
         "replace_each_worktree_head_symref()" was introduced [2] to adjust the
         HEAD in all worktrees after a branch rename succeeded.  It traverses all
    @@ Commit message
         to the renamed ref, it adjusts it.
     
         If we could know in advance if the renamed branch is not HEAD in any
    -    worktree we could avoid calling "replace_each_worktree_head_symref()".
    +    worktree we could avoid calling "replace_each_worktree_head_symref()",
    +    and so avoid that unnecessary second traversing.
     
    -    Let's have "reject_rebase_or_bisect_branch()" check and return whether
    -    the specified branch is HEAD in any worktree, and use this information
    -    to avoid unnecessary calls to "replace_each_worktree_head_symref()".
    +    Let's rename "reject_rebase_or_bisect_branch()" to a more meaningful
    +    name "die_if_branch_is_being_rebased_or_bisected()" and make it return,
    +    if it does not die(), if the specified branch is HEAD in any worktree.
    +    Use this new information to avoid unnecessary calls to
    +    "replace_each_worktree_head_symref()".
     
           1.- 14ace5b (branch: do not rename a branch under bisect or rebase,
               2016-04-22)
    @@ builtin/branch.c: static void print_current_branch_name(void)
      }
      
     -static void reject_rebase_or_bisect_branch(const char *target)
    -+static int ishead_and_reject_rebase_or_bisect_branch(const char *target)
    ++/*
    ++ * Dies if the specified branch is being rebased or bisected.  Otherwise returns
    ++ * 0 or, if the branch is HEAD in any worktree, returns 1.
    ++ */
    ++static int die_if_branch_is_being_rebased_or_bisected(const char *target)
      {
      	struct worktree **worktrees = get_worktrees();
     -	int i;
    @@ builtin/branch.c: static void copy_or_rename_branch(const char *oldname, const c
      	const char *interpreted_oldname = NULL;
      	const char *interpreted_newname = NULL;
     -	int recovery = 0;
    -+	int recovery = 0, ishead;
    ++	int recovery = 0, oldref_is_head;
      
      	if (strbuf_check_branch_ref(&oldref, oldname)) {
      		/*
    @@ builtin/branch.c: static void copy_or_rename_branch(const char *oldname, const c
      		validate_new_branchname(newname, &newref, force);
      
     -	reject_rebase_or_bisect_branch(oldref.buf);
    -+	ishead = ishead_and_reject_rebase_or_bisect_branch(oldref.buf);
    ++	oldref_is_head = die_if_branch_is_being_rebased_or_bisected(oldref.buf);
      
      	if (!skip_prefix(oldref.buf, "refs/heads/", &interpreted_oldname) ||
      	    !skip_prefix(newref.buf, "refs/heads/", &interpreted_newname)) {
    @@ builtin/branch.c: static void copy_or_rename_branch(const char *oldname, const c
      	}
      
     -	if (!copy &&
    -+	if (!copy && ishead &&
    ++	if (!copy && oldref_is_head &&
      	    replace_each_worktree_head_symref(oldref.buf, newref.buf, logmsg.buf))
      		die(_("Branch renamed to %s, but HEAD is not updated!"), newname);
      
2:  bc0ac43932 ! 2:  ab277e5fcb branch: description for orphan branch errors
    @@ Commit message
         branch: description for orphan branch errors
     
         In bcfc82bd48 (branch: description for non-existent branch errors,
    -    2022-10-08) we check the current HEAD to detect if the branch to operate
    -    with is an orphan branch, to avoid the confusing error: "No branch
    -    named...".
    +    2022-10-08) we checked the current HEAD to detect if the branch to
    +    operate with is an orphan branch, so as to avoid the confusing error:
    +    "No branch named...".
     
         If we are asked to operate with an orphan branch in a different working
         tree than the current one, we need to check the HEAD in that different
    @@ Commit message
         repository, using the helper introduced in 31ad6b61bd (branch: add
         branch_checked_out() helper, 2022-06-15)
     
    -    "ishead_reject_rebase_or_bised_branch()" already returns whether the
    -    branch to operate with is HEAD in any working tree in the repository.
    +    "die_if_branch_is_being_rebased_or_bisected()" already returns whether
    +    the branch to operate with is HEAD in any worktree in the repository.
         Let's use this information in "copy_or_rename_branch()", instead of the
         helper.
     
    -    Note that we now call reject_rebase_or_bisect_branch() earlier, which
    -    introduces a change in the error displayed when a combination of
    -    unsupported conditions occur simultaneously: the desired destination
    -    name is invalid, and the branch to operate on is being overrun or
    -    bisected.  With "foo" being rebased or bisected, this:
    +    Note that we now call "die_if_branch_is_being_rebased_or_bisected()"
    +    earlier, which introduces a change in the error displayed when a
    +    combination of unsupported conditions occur simultaneously: the desired
    +    destination name is invalid, and the branch to operate with is being
    +    rebased or bisected.  i.e. with "foo" being rebased or bisected, this:
     
    -            $ git branch -m foo HEAD
    -            fatal: 'HEAD' is not a valid branch name.
    +            $ git branch -m foo /
    +            fatal: '/' is not a valid branch name.
     
         ... becomes:
     
    -            $ git branch -m foo HEAD
    +            $ git branch -m foo /
                 fatal: Branch refs/heads/foo is being ...
     
         Signed-off-by: Rubén Justo <rjusto@gmail.com>
    @@ builtin/branch.c: static void copy_or_rename_branch(const char *oldname, const c
     -		else
     -			die(_("No branch named '%s'."), oldname);
     -	}
    -+	ishead = ishead_and_reject_rebase_or_bisect_branch(oldref.buf);
    ++	oldref_is_head = die_if_branch_is_being_rebased_or_bisected(oldref.buf);
     +
    -+	if ((copy || !ishead) && !ref_exists(oldref.buf))
    -+		die(ishead
    ++	if ((copy || !oldref_is_head) && !ref_exists(oldref.buf))
    ++		die(oldref_is_head
     +		    ? _("No commit on branch '%s' yet.")
     +		    : _("No branch named '%s'."), oldname);
      
    @@ builtin/branch.c: static void copy_or_rename_branch(const char *oldname, const c
      	else
      		validate_new_branchname(newname, &newref, force);
      
    --	ishead = ishead_and_reject_rebase_or_bisect_branch(oldref.buf);
    +-	oldref_is_head = die_if_branch_is_being_rebased_or_bisected(oldref.buf);
     -
      	if (!skip_prefix(oldref.buf, "refs/heads/", &interpreted_oldname) ||
      	    !skip_prefix(newref.buf, "refs/heads/", &interpreted_newname)) {
3:  29f8b6044d ! 3:  9742b4ff1f branch: rename orphan branches in any worktree
    @@ Commit message
     
         In cfaff3aac (branch -m: allow renaming a yet-unborn branch, 2020-12-13)
         we added support for renaming an orphan branch, skipping rename_ref() if
    -    the branch to rename is an orphan branch, checking the current HEAD.
    +    the branch being renamed is an orphan branch; checking the current HEAD.
     
         But the orphan branch to be renamed can be an orphan branch in a
    -    different working tree than the current one, i.e. not the current HEAD.
    +    different worktree than the current one, i.e. not the current HEAD.
     
    -    Let's make "ishead_reject_rebase_or_bisect_branch()" return a flag
    -    indicating if the returned value refers to an orphan branch, and use it
    -    to extend what we did in cfaff3aac, to all HEADs in the repository.
    +    Let's make "die_if_branch_is_being_rebased_or_bisected()" return if the
    +    specified branch is HEAD and orphan, and use it to extend what we did in
    +    cfaff3aac, to check all HEADs in the repository.
     
         Signed-off-by: Rubén Justo <rjusto@gmail.com>
     
      ## builtin/branch.c ##
    -@@ builtin/branch.c: static int ishead_and_reject_rebase_or_bisect_branch(const char *target)
    +@@ builtin/branch.c: static void print_current_branch_name(void)
    + 
    + /*
    +  * Dies if the specified branch is being rebased or bisected.  Otherwise returns
    +- * 0 or, if the branch is HEAD in any worktree, returns 1.
    ++ * 0 or, if the branch is HEAD in any worktree, returns 1. If the branch is HEAD
    ++ * and also orphan, returns 2.
    +  */
    + static int die_if_branch_is_being_rebased_or_bisected(const char *target)
    + {
    +@@ builtin/branch.c: static int die_if_branch_is_being_rebased_or_bisected(const char *target)
      		struct worktree *wt = worktrees[i];
      
      		if (wt->head_ref && !strcmp(target, wt->head_ref))
     -			ret = 1;
    -+			ret = 1 + (is_null_oid(&wt->head_oid) ? 1 : 0);
    ++			ret = is_null_oid(&wt->head_oid) ? 2 : 1;
      
      		if (!wt->is_detached)
      			continue;
    +@@ builtin/branch.c: 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, oldref_is_head;
    ++	int recovery = 0, oldref_is_head, oldref_is_orphan;
    + 
    + 	if (strbuf_check_branch_ref(&oldref, oldname)) {
    + 		/*
    +@@ builtin/branch.c: static void copy_or_rename_branch(const char *oldname, const char *newname, int
    + 	}
    + 
    + 	oldref_is_head = die_if_branch_is_being_rebased_or_bisected(oldref.buf);
    ++	oldref_is_orphan = (oldref_is_head > 1);
    + 
    +-	if ((copy || !oldref_is_head) && !ref_exists(oldref.buf))
    +-		die(oldref_is_head
    ++	if ((copy || !oldref_is_head) &&
    ++	    (oldref_is_orphan || !ref_exists(oldref.buf)))
    ++		die(oldref_is_orphan
    + 		    ? _("No commit on branch '%s' yet.")
    + 		    : _("No branch named '%s'."), oldname);
    + 
     @@ builtin/branch.c: 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 &&
     -	    (!head || strcmp(oldname, head) || !is_null_oid(&head_oid)) &&
    -+	if (!copy && !(ishead > 1) &&
    ++	if (!copy && !oldref_is_orphan &&
      	    rename_ref(oldref.buf, newref.buf, logmsg.buf))
      		die(_("Branch rename failed"));
      	if (copy && copy_existing_ref(oldref.buf, newref.buf, logmsg.buf))
    @@ t/t3200-branch.sh: test_expect_success 'git branch -M and -C fail on detached HE
     +	test_when_finished git checkout - &&
     +	test_when_finished git worktree remove -f wt &&
     +	git worktree add wt --detach &&
    -+
     +	# rename orphan in another worktreee
     +	git -C wt checkout --orphan orphan-foo-wt &&
     +	git branch -m orphan-foo-wt orphan-bar-wt &&
     +	test orphan-bar-wt=$(git -C orphan-worktree branch --show-current) &&
    -+
     +	# rename orphan in the current worktree
     +	git checkout --orphan orphan-foo &&
     +	git branch -m orphan-foo orphan-bar &&
-- 
2.39.0

[PATCH v3 1/3] branch: avoid unnecessary worktrees traversals

[Rubén Justo]

"reject_rebase_or_bisect_branch()" was introduced [1] to prevent a
branch under bisect or rebase from being renamed or copied.  It
traverses all worktrees in the repository and dies if the specified
branch is being rebased or bisected in any of them.

"replace_each_worktree_head_symref()" was introduced [2] to adjust the
HEAD in all worktrees after a branch rename succeeded.  It traverses all
worktrees in the repository and if any of them have their HEAD pointing
to the renamed ref, it adjusts it.

If we could know in advance if the renamed branch is not HEAD in any
worktree we could avoid calling "replace_each_worktree_head_symref()",
and so avoid that unnecessary second traversing.

Let's rename "reject_rebase_or_bisect_branch()" to a more meaningful
name "die_if_branch_is_being_rebased_or_bisected()" and make it return,
if it does not die(), if the specified branch is HEAD in any worktree.
Use this new information to avoid unnecessary calls to
"replace_each_worktree_head_symref()".

  1.- 14ace5b (branch: do not rename a branch under bisect or rebase,
      2016-04-22)

  2.- 70999e9 (branch -m: update all per-worktree HEADs, 2016-03-27)

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

diff --git a/builtin/branch.c b/builtin/branch.c
index f63fd45edb..89198fa5bf 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -486,14 +486,21 @@ static void print_current_branch_name(void)
 		die(_("HEAD (%s) points outside of refs/heads/"), refname);
 }
 
-static void reject_rebase_or_bisect_branch(const char *target)
+/*
+ * Dies if the specified branch is being rebased or bisected.  Otherwise returns
+ * 0 or, if the branch is HEAD in any worktree, returns 1.
+ */
+static int die_if_branch_is_being_rebased_or_bisected(const char *target)
 {
 	struct worktree **worktrees = get_worktrees();
-	int i;
+	int i, ret = 0;
 
 	for (i = 0; worktrees[i]; i++) {
 		struct worktree *wt = worktrees[i];
 
+		if (wt->head_ref && !strcmp(target, wt->head_ref))
+			ret = 1;
+
 		if (!wt->is_detached)
 			continue;
 
@@ -507,6 +514,7 @@ static void reject_rebase_or_bisect_branch(const char *target)
 	}
 
 	free_worktrees(worktrees);
+	return ret;
 }
 
 static void copy_or_rename_branch(const char *oldname, const char *newname, int copy, int force)
@@ -515,7 +523,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_is_head;
 
 	if (strbuf_check_branch_ref(&oldref, oldname)) {
 		/*
@@ -544,7 +552,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
 	else
 		validate_new_branchname(newname, &newref, force);
 
-	reject_rebase_or_bisect_branch(oldref.buf);
+	oldref_is_head = die_if_branch_is_being_rebased_or_bisected(oldref.buf);
 
 	if (!skip_prefix(oldref.buf, "refs/heads/", &interpreted_oldname) ||
 	    !skip_prefix(newref.buf, "refs/heads/", &interpreted_newname)) {
@@ -574,7 +582,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
 				interpreted_oldname);
 	}
 
-	if (!copy &&
+	if (!copy && oldref_is_head &&
 	    replace_each_worktree_head_symref(oldref.buf, newref.buf, logmsg.buf))
 		die(_("Branch renamed to %s, but HEAD is not updated!"), newname);
 
-- 
2.39.0

[PATCH v3 2/3] branch: description for orphan branch errors

[Rubén Justo]

In bcfc82bd48 (branch: description for non-existent branch errors,
2022-10-08) we checked the current HEAD to detect if the branch to
operate with is an orphan branch, so as to avoid the confusing error:
"No branch named...".

If we are asked to operate with an orphan branch in a different working
tree than the current one, we need to check the HEAD in that different
working tree.

Let's extend the check we did in bcfc82db48, to all HEADs in the
repository, using the helper introduced in 31ad6b61bd (branch: add
branch_checked_out() helper, 2022-06-15)

"die_if_branch_is_being_rebased_or_bisected()" already returns whether
the branch to operate with is HEAD in any worktree in the repository.
Let's use this information in "copy_or_rename_branch()", instead of the
helper.

Note that we now call "die_if_branch_is_being_rebased_or_bisected()"
earlier, which introduces a change in the error displayed when a
combination of unsupported conditions occur simultaneously: the desired
destination name is invalid, and the branch to operate with is being
rebased or bisected.  i.e. with "foo" being rebased or bisected, this:

	$ git branch -m foo /
	fatal: '/' is not a valid branch name.

... becomes:

	$ git branch -m foo /
	fatal: Branch refs/heads/foo is being ...

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 builtin/branch.c       | 25 +++++++++++--------------
 t/t3202-show-branch.sh | 18 ++++++++++++++++++
 2 files changed, 29 insertions(+), 14 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 89198fa5bf..28cec344ad 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -536,12 +536,12 @@ 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))
-			die(_("No commit on branch '%s' yet."), oldname);
-		else
-			die(_("No branch named '%s'."), oldname);
-	}
+	oldref_is_head = die_if_branch_is_being_rebased_or_bisected(oldref.buf);
+
+	if ((copy || !oldref_is_head) && !ref_exists(oldref.buf))
+		die(oldref_is_head
+		    ? _("No commit on branch '%s' yet.")
+		    : _("No branch named '%s'."), oldname);
 
 	/*
 	 * A command like "git branch -M currentbranch currentbranch" cannot
@@ -552,8 +552,6 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
 	else
 		validate_new_branchname(newname, &newref, force);
 
-	oldref_is_head = die_if_branch_is_being_rebased_or_bisected(oldref.buf);
-
 	if (!skip_prefix(oldref.buf, "refs/heads/", &interpreted_oldname) ||
 	    !skip_prefix(newref.buf, "refs/heads/", &interpreted_newname)) {
 		BUG("expected prefix missing for refs");
@@ -814,7 +812,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 
 		strbuf_addf(&branch_ref, "refs/heads/%s", branch_name);
 		if (!ref_exists(branch_ref.buf))
-			error((!argc || !strcmp(head, branch_name))
+			error((!argc || branch_checked_out(branch_ref.buf))
 			      ? _("No commit on branch '%s' yet.")
 			      : _("No branch named '%s'."),
 			      branch_name);
@@ -858,11 +856,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 			die(_("no such branch '%s'"), argv[0]);
 		}
 
-		if (!ref_exists(branch->refname)) {
-			if (!argc || !strcmp(head, branch->name))
-				die(_("No commit on branch '%s' yet."), branch->name);
-			die(_("branch '%s' does not exist"), branch->name);
-		}
+		if (!ref_exists(branch->refname))
+			die((!argc || branch_checked_out(branch->refname))
+			    ? _("No commit on branch '%s' yet.")
+			    : _("branch '%s' does not exist"), branch->name);
 
 		dwim_and_setup_tracking(the_repository, branch->name,
 					new_upstream, BRANCH_TRACK_OVERRIDE,
diff --git a/t/t3202-show-branch.sh b/t/t3202-show-branch.sh
index ea7cfd1951..be20ebe1d5 100755
--- a/t/t3202-show-branch.sh
+++ b/t/t3202-show-branch.sh
@@ -221,4 +221,22 @@ test_expect_success 'fatal descriptions on non-existent branch' '
 	test_cmp expect actual
 '
 
+test_expect_success 'error descriptions on orphan branch' '
+	test_when_finished git worktree remove -f wt &&
+	git worktree add wt --detach &&
+	git -C wt checkout --orphan orphan-branch &&
+	test_branch_op_in_wt() {
+		test_orphan_error() {
+			test_must_fail git $* 2>actual &&
+			test_i18ngrep "No commit on branch .orphan-branch. yet.$" actual
+		} &&
+		test_orphan_error -C wt branch $1 $2 &&                # implicit branch
+		test_orphan_error -C wt branch $1 orphan-branch $2 &&  # explicit branch
+		test_orphan_error branch $1 orphan-branch $2           # different worktree
+	} &&
+	test_branch_op_in_wt --edit-description &&
+	test_branch_op_in_wt --set-upstream-to=ne &&
+	test_branch_op_in_wt -c new-branch
+'
+
 test_done
-- 
2.39.0

[PATCH v3 3/3] branch: rename orphan branches in any worktree

[Rubén Justo]

In cfaff3aac (branch -m: allow renaming a yet-unborn branch, 2020-12-13)
we added support for renaming an orphan branch, skipping rename_ref() if
the branch being renamed is an orphan branch; checking the current HEAD.

But the orphan branch to be renamed can be an orphan branch in a
different worktree than the current one, i.e. not the current HEAD.

Let's make "die_if_branch_is_being_rebased_or_bisected()" return if the
specified branch is HEAD and orphan, and use it to extend what we did in
cfaff3aac, to check all HEADs in the repository.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 builtin/branch.c  | 16 +++++++++-------
 t/t3200-branch.sh | 14 ++++++++++++++
 2 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 28cec344ad..7efda62224 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -488,7 +488,8 @@ static void print_current_branch_name(void)
 
 /*
  * Dies if the specified branch is being rebased or bisected.  Otherwise returns
- * 0 or, if the branch is HEAD in any worktree, returns 1.
+ * 0 or, if the branch is HEAD in any worktree, returns 1. If the branch is HEAD
+ * and also orphan, returns 2.
  */
 static int die_if_branch_is_being_rebased_or_bisected(const char *target)
 {
@@ -499,7 +500,7 @@ static int die_if_branch_is_being_rebased_or_bisected(const char *target)
 		struct worktree *wt = worktrees[i];
 
 		if (wt->head_ref && !strcmp(target, wt->head_ref))
-			ret = 1;
+			ret = is_null_oid(&wt->head_oid) ? 2 : 1;
 
 		if (!wt->is_detached)
 			continue;
@@ -523,7 +524,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, oldref_is_head;
+	int recovery = 0, oldref_is_head, oldref_is_orphan;
 
 	if (strbuf_check_branch_ref(&oldref, oldname)) {
 		/*
@@ -537,9 +538,11 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
 	}
 
 	oldref_is_head = die_if_branch_is_being_rebased_or_bisected(oldref.buf);
+	oldref_is_orphan = (oldref_is_head > 1);
 
-	if ((copy || !oldref_is_head) && !ref_exists(oldref.buf))
-		die(oldref_is_head
+	if ((copy || !oldref_is_head) &&
+	    (oldref_is_orphan || !ref_exists(oldref.buf)))
+		die(oldref_is_orphan
 		    ? _("No commit on branch '%s' yet.")
 		    : _("No branch named '%s'."), oldname);
 
@@ -564,8 +567,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 &&
-	    (!head || strcmp(oldname, head) || !is_null_oid(&head_oid)) &&
+	if (!copy && !oldref_is_orphan &&
 	    rename_ref(oldref.buf, newref.buf, logmsg.buf))
 		die(_("Branch rename failed"));
 	if (copy && copy_existing_ref(oldref.buf, newref.buf, logmsg.buf))
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 5a169b68d6..5aef00efde 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -279,6 +279,20 @@ test_expect_success 'git branch -M and -C fail on detached HEAD' '
 	test_cmp expect err
 '
 
+test_expect_success 'git branch -m should work with orphan branches' '
+	test_when_finished git checkout - &&
+	test_when_finished git worktree remove -f wt &&
+	git worktree add wt --detach &&
+	# rename orphan in another worktreee
+	git -C wt checkout --orphan orphan-foo-wt &&
+	git branch -m orphan-foo-wt orphan-bar-wt &&
+	test orphan-bar-wt=$(git -C orphan-worktree branch --show-current) &&
+	# rename orphan in the current worktree
+	git checkout --orphan orphan-foo &&
+	git branch -m orphan-foo orphan-bar &&
+	test orphan-bar=$(git branch --show-current)
+'
+
 test_expect_success 'git branch -d on orphan HEAD (merged)' '
 	test_when_finished git checkout main &&
 	git checkout --orphan orphan &&
-- 
2.39.0

Re: [PATCH v3 0/3] branch: operations on orphan branches

[Junio C Hamano]

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

> Avoid some confusing errors operating with orphan branches when
> working with worktrees.
>
> Changes from v2:
>
>  - Renamed "ishead_and_reject_rebase_or_bisect_branch()" to
>    "die_if_branch_is_being_rebased_or_bisected()"
>
>    A proposed name "die_if_branch_is_is_use()" has not been used because
>    it could lead to confusion.  We don't yet support copying or renaming
>    a branch being rebased or bisected, but we do under other uses.

Quite sensible.

>  - Comments added and variables renamed to make more clear the intention and
>    the functioning.

OK.

Will update what has been queued.  Let's see if we get positive
reviews on this round and start merging it down.

Thanks.

Re: [PATCH v3 0/3] branch: operations on orphan branches

[Ævar Arnfjörð Bjarmason]

On Tue, Feb 07 2023, Rubén Justo wrote:

> Avoid some confusing errors operating with orphan branches when
> working with worktrees.
>
> Changes from v2:
>
>  - Renamed "ishead_and_reject_rebase_or_bisect_branch()" to
>    "die_if_branch_is_being_rebased_or_bisected()"

Looking this over holistically, I think this is a great example of where
factoring something out into a function is just making readbility
worse. This function is only used in copy_or_rename_branch(), and the
overloaded name & semantics are making things quite confusing.

Whereas if we just start by pulling it into its only caller I think this
gets much better, at the end of this message is a diff-on-top these
three patches where I do that (I kept the "target" variable to minimize
the diff with the move detection, but we probalby want the strbuf
directly instead).

>    A proposed name "die_if_branch_is_is_use()" has not been used because
>    it could lead to confusion.  We don't yet support copying or renaming
>    a branch being rebased or bisected, but we do under other uses.

Another thing that I think could be improved in this series is if you
skip the refactoring-while-at-it of changing the existing
"if/if/die/die" into a "if/die/?:".

In the below diff I have that proposed change on top, but this snippet
here shows the diff to "origin/master":
	
	@@ -806,7 +806,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
	 
	 		strbuf_addf(&branch_ref, "refs/heads/%s", branch_name);
	 		if (!ref_exists(branch_ref.buf))
	-			error((!argc || !strcmp(head, branch_name))
	+			error((!argc || branch_checked_out(branch_ref.buf))
	 			      ? _("No commit on branch '%s' yet.")
	 			      : _("No branch named '%s'."),
	 			      branch_name);
	@@ -851,10 +851,11 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
	 		}
	 
	 		if (!ref_exists(branch->refname)) {
	-			if (!argc || !strcmp(head, branch->name))
	+			if (!argc || branch_checked_out(branch->refname))
	 				die(_("No commit on branch '%s' yet."), branch->name);
	 			die(_("branch '%s' does not exist"), branch->name);
	 		}

I.e. your refactoring of this in 2/3 turns out to in the end have just
been inflating the code change, for no functional benefit.

I wouldn't mind if this were in some pre-cleanup, or if it actually made
the code easier to read, but IMO this pattern of using a ternary to
select the format to "error" or "die" makes things worse for
readability. It's a few bytes less code, but makes things harder to follow overall.

And even if you disagree with that as far as the end state is concerned,
I think it's unarguable that it makes the 2/3 harder to follow, since
it's sticking a refactoring that's not neede dfor the end-goal here into
an otherwise functional change.

I'm aware that some of the code in the context uses this pattern, and
you probably changed the "if" block you modified to be consistent with
the code above, but I think in this case it's better not to follow the
existing style (which is used in that function, but is a rare exception
overall in this codebase).

The diff-on-top, mentioned above:

== BEGIN
	
diff --git a/builtin/branch.c b/builtin/branch.c
index 7efda622241..dc7a3e3dde1 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -486,45 +486,16 @@ static void print_current_branch_name(void)
 		die(_("HEAD (%s) points outside of refs/heads/"), refname);
 }
 
-/*
- * Dies if the specified branch is being rebased or bisected.  Otherwise returns
- * 0 or, if the branch is HEAD in any worktree, returns 1. If the branch is HEAD
- * and also orphan, returns 2.
- */
-static int die_if_branch_is_being_rebased_or_bisected(const char *target)
-{
-	struct worktree **worktrees = get_worktrees();
-	int i, ret = 0;
-
-	for (i = 0; worktrees[i]; i++) {
-		struct worktree *wt = worktrees[i];
-
-		if (wt->head_ref && !strcmp(target, wt->head_ref))
-			ret = is_null_oid(&wt->head_oid) ? 2 : 1;
-
-		if (!wt->is_detached)
-			continue;
-
-		if (is_worktree_being_rebased(wt, target))
-			die(_("Branch %s is being rebased at %s"),
-			    target, wt->path);
-
-		if (is_worktree_being_bisected(wt, target))
-			die(_("Branch %s is being bisected at %s"),
-			    target, wt->path);
-	}
-
-	free_worktrees(worktrees);
-	return ret;
-}
-
 static void copy_or_rename_branch(const char *oldname, const char *newname, int copy, int force)
 {
 	struct strbuf oldref = STRBUF_INIT, newref = STRBUF_INIT, logmsg = STRBUF_INIT;
 	struct strbuf oldsection = STRBUF_INIT, newsection = STRBUF_INIT;
 	const char *interpreted_oldname = NULL;
 	const char *interpreted_newname = NULL;
-	int recovery = 0, oldref_is_head, oldref_is_orphan;
+	int recovery = 0, oldref_is_head = 0, oldref_is_orphan = 0;
+	struct worktree **worktrees;
+	int i;
+	const char *target;
 
 	if (strbuf_check_branch_ref(&oldref, oldname)) {
 		/*
@@ -537,8 +508,29 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
 			die(_("Invalid branch name: '%s'"), oldname);
 	}
 
-	oldref_is_head = die_if_branch_is_being_rebased_or_bisected(oldref.buf);
-	oldref_is_orphan = (oldref_is_head > 1);
+	worktrees = get_worktrees();
+	target = oldref.buf;
+	for (i = 0; worktrees[i]; i++) {
+		struct worktree *wt = worktrees[i];
+
+		if (wt->head_ref && !strcmp(target, wt->head_ref)) {
+			oldref_is_head = 1;
+			if (is_null_oid(&wt->head_oid))
+				oldref_is_orphan = 1;
+		}
+
+		if (!wt->is_detached)
+			continue;
+
+		if (is_worktree_being_rebased(wt, target))
+			die(_("Branch %s is being rebased at %s"),
+			    target, wt->path);
+
+		if (is_worktree_being_bisected(wt, target))
+			die(_("Branch %s is being bisected at %s"),
+			    target, wt->path);
+	}
+	free_worktrees(worktrees);
 
 	if ((copy || !oldref_is_head) &&
 	    (oldref_is_orphan || !ref_exists(oldref.buf)))
@@ -858,10 +850,12 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 			die(_("no such branch '%s'"), argv[0]);
 		}
 
-		if (!ref_exists(branch->refname))
-			die((!argc || branch_checked_out(branch->refname))
-			    ? _("No commit on branch '%s' yet.")
-			    : _("branch '%s' does not exist"), branch->name);
+		if (!ref_exists(branch->refname)) {
+			if (!argc || branch_checked_out(branch->refname))
+				die(_("No commit on branch '%s' yet."), branch->name);
+			die(_("branch '%s' does not exist"), branch->name);
+		}
+		
 
 		dwim_and_setup_tracking(the_repository, branch->name,
 					new_upstream, BRANCH_TRACK_OVERRIDE,

== END

P.S. if I were refactoring those ?: for style in that function I'd
probably go for this on-top. The N_() followed by _() pattern is
probably overdoing it, but included to show that one way out of this
sort of thing with i18n is that you can pre-mark the string with N_(),
then use it with _() to emit the message (right now the code uses
"copy?" over "copy ?" instead to align them):

diff --git a/builtin/branch.c b/builtin/branch.c
index dc7a3e3dde1..e42f9bc4900 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -805,31 +805,35 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		}
 
 		strbuf_addf(&branch_ref, "refs/heads/%s", branch_name);
-		if (!ref_exists(branch_ref.buf))
-			error((!argc || branch_checked_out(branch_ref.buf))
-			      ? _("No commit on branch '%s' yet.")
-			      : _("No branch named '%s'."),
-			      branch_name);
-		else if (!edit_branch_description(branch_name))
+		if (!ref_exists(branch_ref.buf)) {
+			if (!argc || branch_checked_out(branch_ref.buf))
+				error(_("No commit on branch '%s' yet."),
+				      branch_name);
+			else
+				error(_("No branch named '%s'."), branch_name);
+		} else if (!edit_branch_description(branch_name)) {
 			ret = 0; /* happy */
+		}
 
 		strbuf_release(&branch_ref);
 		strbuf_release(&buf);
 
 		return ret;
 	} else if (copy || rename) {
+		static const char *cannot_copy = N_("cannot copy the current branch while not on any.");
+		static const char *cannot_rename = N_("cannot rename the current branch while not on any.");
 		if (!argc)
 			die(_("branch name required"));
 		else if ((argc == 1) && filter.detached)
-			die(copy? _("cannot copy the current branch while not on any.")
-				: _("cannot rename the current branch while not on any."));
+			die("%s", copy ? _(cannot_copy) : _(cannot_rename));
 		else if (argc == 1)
 			copy_or_rename_branch(head, argv[0], copy, copy + rename > 1);
 		else if (argc == 2)
 			copy_or_rename_branch(argv[0], argv[1], copy, copy + rename > 1);
+		else if (copy)
+			die(_("too many branches for a copy operation"));
 		else
-			die(copy? _("too many branches for a copy operation")
-				: _("too many arguments for a rename operation"));
+			die(_("too many arguments for a rename operation"));
 	} else if (new_upstream) {
 		struct branch *branch;
 		struct strbuf buf = STRBUF_INIT;
 

Re: [PATCH v3 0/3] branch: operations on orphan branches

[Rubén Justo]

On 07-feb-2023 09:33:39, Ævar Arnfjörð Bjarmason wrote:

Thank you reviewing this.

> 
> On Tue, Feb 07 2023, Rubén Justo wrote:
> 
> > Avoid some confusing errors operating with orphan branches when
> > working with worktrees.
> >
> > Changes from v2:
> >
> >  - Renamed "ishead_and_reject_rebase_or_bisect_branch()" to
> >    "die_if_branch_is_being_rebased_or_bisected()"
> 
> Looking this over holistically, I think this is a great example of where
> factoring something out into a function is just making readbility
> worse. This function is only used in copy_or_rename_branch(), and the
> overloaded name & semantics are making things quite confusing.
> 
> Whereas if we just start by pulling it into its only caller I think this
> gets much better, at the end of this message is a diff-on-top these
> three patches where I do that (I kept the "target" variable to minimize
> the diff with the move detection, but we probalby want the strbuf
> directly instead).

I'm sorry, but I don't agree.  copy_or_rename_branch() already does some
heterogeneous things.  IMHO, making it also iterate worktrees makes
things worse, in terms of readability.  I could agree with maybe the
function die_if_branch_is_being_rebased_or_bisected() could be part of a
more general use, maybe something in worktree.h.
 
> >    A proposed name "die_if_branch_is_is_use()" has not been used because
> >    it could lead to confusion.  We don't yet support copying or renaming
> >    a branch being rebased or bisected, but we do under other uses.
> 
> Another thing that I think could be improved in this series is if you
> skip the refactoring-while-at-it of changing the existing
> "if/if/die/die" into a "if/die/?:".

I'm sorry, but I don't agree with this reasoning either.  The ternary op
here allows the code to be more clear, IMHO, in the intention: there is
no conditional die() or error(), the conditional is for the message.

> 
> In the below diff I have that proposed change on top, but this snippet
> here shows the diff to "origin/master":
> 	
> 	@@ -806,7 +806,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
> 	 
> 	 		strbuf_addf(&branch_ref, "refs/heads/%s", branch_name);
> 	 		if (!ref_exists(branch_ref.buf))
> 	-			error((!argc || !strcmp(head, branch_name))
> 	+			error((!argc || branch_checked_out(branch_ref.buf))
> 	 			      ? _("No commit on branch '%s' yet.")
> 	 			      : _("No branch named '%s'."),
> 	 			      branch_name);
> 	@@ -851,10 +851,11 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
> 	 		}
> 	 
> 	 		if (!ref_exists(branch->refname)) {
> 	-			if (!argc || !strcmp(head, branch->name))
> 	+			if (!argc || branch_checked_out(branch->refname))
> 	 				die(_("No commit on branch '%s' yet."), branch->name);
> 	 			die(_("branch '%s' does not exist"), branch->name);
> 	 		}
> 
> I.e. your refactoring of this in 2/3 turns out to in the end have just
> been inflating the code change, for no functional benefit.
> 
> I wouldn't mind if this were in some pre-cleanup, or if it actually made
> the code easier to read, but IMO this pattern of using a ternary to
> select the format to "error" or "die" makes things worse for
> readability. It's a few bytes less code, but makes things harder to follow overall.
> 
> And even if you disagree with that as far as the end state is concerned,
> I think it's unarguable that it makes the 2/3 harder to follow, since
> it's sticking a refactoring that's not neede dfor the end-goal here into
> an otherwise functional change.
> 
> I'm aware that some of the code in the context uses this pattern, and
> you probably changed the "if" block you modified to be consistent with
> the code above, but I think in this case it's better not to follow the
> existing style (which is used in that function, but is a rare exception
> overall in this codebase).
> 
> The diff-on-top, mentioned above:
> 
> == BEGIN
> 	
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 7efda622241..dc7a3e3dde1 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -486,45 +486,16 @@ static void print_current_branch_name(void)
>  		die(_("HEAD (%s) points outside of refs/heads/"), refname);
>  }
>  
> -/*
> - * Dies if the specified branch is being rebased or bisected.  Otherwise returns
> - * 0 or, if the branch is HEAD in any worktree, returns 1. If the branch is HEAD
> - * and also orphan, returns 2.
> - */
> -static int die_if_branch_is_being_rebased_or_bisected(const char *target)
> -{
> -	struct worktree **worktrees = get_worktrees();
> -	int i, ret = 0;
> -
> -	for (i = 0; worktrees[i]; i++) {
> -		struct worktree *wt = worktrees[i];
> -
> -		if (wt->head_ref && !strcmp(target, wt->head_ref))
> -			ret = is_null_oid(&wt->head_oid) ? 2 : 1;
> -
> -		if (!wt->is_detached)
> -			continue;
> -
> -		if (is_worktree_being_rebased(wt, target))
> -			die(_("Branch %s is being rebased at %s"),
> -			    target, wt->path);
> -
> -		if (is_worktree_being_bisected(wt, target))
> -			die(_("Branch %s is being bisected at %s"),
> -			    target, wt->path);
> -	}
> -
> -	free_worktrees(worktrees);
> -	return ret;
> -}
> -
>  static void copy_or_rename_branch(const char *oldname, const char *newname, int copy, int force)
>  {
>  	struct strbuf oldref = STRBUF_INIT, newref = STRBUF_INIT, logmsg = STRBUF_INIT;
>  	struct strbuf oldsection = STRBUF_INIT, newsection = STRBUF_INIT;
>  	const char *interpreted_oldname = NULL;
>  	const char *interpreted_newname = NULL;
> -	int recovery = 0, oldref_is_head, oldref_is_orphan;
> +	int recovery = 0, oldref_is_head = 0, oldref_is_orphan = 0;
> +	struct worktree **worktrees;
> +	int i;
> +	const char *target;
>  
>  	if (strbuf_check_branch_ref(&oldref, oldname)) {
>  		/*
> @@ -537,8 +508,29 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
>  			die(_("Invalid branch name: '%s'"), oldname);
>  	}
>  
> -	oldref_is_head = die_if_branch_is_being_rebased_or_bisected(oldref.buf);
> -	oldref_is_orphan = (oldref_is_head > 1);
> +	worktrees = get_worktrees();
> +	target = oldref.buf;
> +	for (i = 0; worktrees[i]; i++) {
> +		struct worktree *wt = worktrees[i];
> +
> +		if (wt->head_ref && !strcmp(target, wt->head_ref)) {
> +			oldref_is_head = 1;
> +			if (is_null_oid(&wt->head_oid))
> +				oldref_is_orphan = 1;
> +		}
> +
> +		if (!wt->is_detached)
> +			continue;
> +
> +		if (is_worktree_being_rebased(wt, target))
> +			die(_("Branch %s is being rebased at %s"),
> +			    target, wt->path);
> +
> +		if (is_worktree_being_bisected(wt, target))
> +			die(_("Branch %s is being bisected at %s"),
> +			    target, wt->path);
> +	}
> +	free_worktrees(worktrees);
>  
>  	if ((copy || !oldref_is_head) &&
>  	    (oldref_is_orphan || !ref_exists(oldref.buf)))
> @@ -858,10 +850,12 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>  			die(_("no such branch '%s'"), argv[0]);
>  		}
>  
> -		if (!ref_exists(branch->refname))
> -			die((!argc || branch_checked_out(branch->refname))
> -			    ? _("No commit on branch '%s' yet.")
> -			    : _("branch '%s' does not exist"), branch->name);
> +		if (!ref_exists(branch->refname)) {
> +			if (!argc || branch_checked_out(branch->refname))
> +				die(_("No commit on branch '%s' yet."), branch->name);
> +			die(_("branch '%s' does not exist"), branch->name);
> +		}
> +		
>  
>  		dwim_and_setup_tracking(the_repository, branch->name,
>  					new_upstream, BRANCH_TRACK_OVERRIDE,
> 
> == END
> 
> P.S. if I were refactoring those ?: for style in that function I'd
> probably go for this on-top. The N_() followed by _() pattern is
> probably overdoing it, but included to show that one way out of this
> sort of thing with i18n is that you can pre-mark the string with N_(),
> then use it with _() to emit the message (right now the code uses
> "copy?" over "copy ?" instead to align them):
> 
> diff --git a/builtin/branch.c b/builtin/branch.c
> index dc7a3e3dde1..e42f9bc4900 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -805,31 +805,35 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>  		}
>  
>  		strbuf_addf(&branch_ref, "refs/heads/%s", branch_name);
> -		if (!ref_exists(branch_ref.buf))
> -			error((!argc || branch_checked_out(branch_ref.buf))
> -			      ? _("No commit on branch '%s' yet.")
> -			      : _("No branch named '%s'."),
> -			      branch_name);
> -		else if (!edit_branch_description(branch_name))
> +		if (!ref_exists(branch_ref.buf)) {
> +			if (!argc || branch_checked_out(branch_ref.buf))
> +				error(_("No commit on branch '%s' yet."),
> +				      branch_name);
> +			else
> +				error(_("No branch named '%s'."), branch_name);
> +		} else if (!edit_branch_description(branch_name)) {
>  			ret = 0; /* happy */
> +		}
>  
>  		strbuf_release(&branch_ref);
>  		strbuf_release(&buf);
>  
>  		return ret;
>  	} else if (copy || rename) {
> +		static const char *cannot_copy = N_("cannot copy the current branch while not on any.");
> +		static const char *cannot_rename = N_("cannot rename the current branch while not on any.");
>  		if (!argc)
>  			die(_("branch name required"));
>  		else if ((argc == 1) && filter.detached)
> -			die(copy? _("cannot copy the current branch while not on any.")
> -				: _("cannot rename the current branch while not on any."));
> +			die("%s", copy ? _(cannot_copy) : _(cannot_rename));
>  		else if (argc == 1)
>  			copy_or_rename_branch(head, argv[0], copy, copy + rename > 1);
>  		else if (argc == 2)
>  			copy_or_rename_branch(argv[0], argv[1], copy, copy + rename > 1);
> +		else if (copy)
> +			die(_("too many branches for a copy operation"));
>  		else
> -			die(copy? _("too many branches for a copy operation")
> -				: _("too many arguments for a rename operation"));
> +			die(_("too many arguments for a rename operation"));
>  	} else if (new_upstream) {
>  		struct branch *branch;
>  		struct strbuf buf = STRBUF_INIT;
>  

Re: [PATCH v3 0/3] branch: operations on orphan branches

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

>>  - Renamed "ishead_and_reject_rebase_or_bisect_branch()" to
>>    "die_if_branch_is_being_rebased_or_bisected()"
>
> Looking this over holistically, I think this is a great example of where
> factoring something out into a function is just making readbility
> worse. This function is only used in copy_or_rename_branch(), and the
> overloaded name & semantics are making things quite confusing.
>
> Whereas if we just start by pulling it into its only caller I think this
> gets much better,...

Hmph, I hadn't considered it, but with only a single caller that
becomes a viable alternative.

> Another thing that I think could be improved in this series is if you
> skip the refactoring-while-at-it of changing the existing
> "if/if/die/die" into a "if/die/?:".
> ...
> I.e. your refactoring of this in 2/3 turns out to in the end have just
> been inflating the code change, for no functional benefit.
>
> I wouldn't mind if this were in some pre-cleanup, or if it actually made
> the code easier to read, but IMO this pattern of using a ternary to
> select the format to "error" or "die" makes things worse for
> readability. It's a few bytes less code, but makes things harder to follow overall.

Good.

Thanks for carefully reading.

Re: [PATCH v3 1/3] branch: avoid unnecessary worktrees traversals

[Jonathan Tan]

Rubén Justo <rjusto@gmail.com> writes:
> "reject_rebase_or_bisect_branch()" was introduced [1] to prevent a
> branch under bisect or rebase from being renamed or copied.  It
> traverses all worktrees in the repository and dies if the specified
> branch is being rebased or bisected in any of them.
> 
> "replace_each_worktree_head_symref()" was introduced [2] to adjust the
> HEAD in all worktrees after a branch rename succeeded.  It traverses all
> worktrees in the repository and if any of them have their HEAD pointing
> to the renamed ref, it adjusts it.

Thanks for the references to why these were introduced!

> If we could know in advance if the renamed branch is not HEAD in any
> worktree we could avoid calling "replace_each_worktree_head_symref()",
> and so avoid that unnecessary second traversing.

When I first read this paragraph, I thought that the traversing involved
was just a loop through an in-memory data structure, which is not that
costly. It turns out that a travesal involves not only constructing
said data structure but also reading from disk to get the necessary
information, which indeed is very costly. I would include that in the
commit message, but won't insist on that (perhaps it's clear to others
what is meant by traversal).

> Let's rename "reject_rebase_or_bisect_branch()" to a more meaningful
> name "die_if_branch_is_being_rebased_or_bisected()" and make it return,
> if it does not die(), if the specified branch is HEAD in any worktree.
> Use this new information to avoid unnecessary calls to
> "replace_each_worktree_head_symref()".

In later patches, I see that the return value can also indicate that a
branch is an orphan, and that for the sake of code clarity, the calling
function had to have a variable assignment of the form oldref_is_orphan
= (oldref_is_head > 1). If this is so, it is probably better to have
this function return something with names. So something like

  #define IS_HEAD 4
  #define IS_ORPHAN 8
  int get_branch_usage_in_worktrees(...) {...}

and then the caller can use these constants whenever it needs to know
what kind of branch this is.

I also see in patch 2 that we're changing what the user sees under
certain inputs. That can be avoided if we move the dying to the caller,
and have this function merely return when the branch is being rebased
or bisected.

  #define IS_BISECTED 1
  #define IS_REBASED 2

or something like that. I would prefer if user-visible behavior didn't
change unnecessarily, and this does not seem like a necessary case.

Other than that, everything looks good.

Re: [PATCH v3 1/3] branch: avoid unnecessary worktrees traversals

[Rubén Justo]

On 10-feb-2023 20:16:44, Jonathan Tan wrote:

> > If we could know in advance if the renamed branch is not HEAD in any
> > worktree we could avoid calling "replace_each_worktree_head_symref()",
> > and so avoid that unnecessary second traversing.
> 
> When I first read this paragraph, I thought that the traversing involved
> was just a loop through an in-memory data structure, which is not that
> costly. It turns out that a travesal involves not only constructing
> said data structure but also reading from disk to get the necessary
> information, which indeed is very costly. I would include that in the
> commit message, but won't insist on that (perhaps it's clear to others
> what is meant by traversal).

Sorry, I should have included details about why it's costly.  I'll
include some in the message.

> 
> > Let's rename "reject_rebase_or_bisect_branch()" to a more meaningful
> > name "die_if_branch_is_being_rebased_or_bisected()" and make it return,
> > if it does not die(), if the specified branch is HEAD in any worktree.
> > Use this new information to avoid unnecessary calls to
> > "replace_each_worktree_head_symref()".
> 
> In later patches, I see that the return value can also indicate that a
> branch is an orphan, and that for the sake of code clarity, the calling
> function had to have a variable assignment of the form oldref_is_orphan
> = (oldref_is_head > 1). If this is so, it is probably better to have
> this function return something with names. So something like
> 
>   #define IS_HEAD 4
>   #define IS_ORPHAN 8

OK.  I'll use names.

>   int get_branch_usage_in_worktrees(...) {...}
> 
> and then the caller can use these constants whenever it needs to know
> what kind of branch this is.
> 
> I also see in patch 2 that we're changing what the user sees under
> certain inputs. That can be avoided if we move the dying to the caller,
> and have this function merely return when the branch is being rebased
> or bisected.
> 
>   #define IS_BISECTED 1
>   #define IS_REBASED 2
> 
> or something like that. I would prefer if user-visible behavior didn't
> change unnecessarily, and this does not seem like a necessary case.

OK.

> 
> Other than that, everything looks good.

Thanks for your review and suggestions!

[PATCH v4 0/3] branch: operations on orphan branches

[Rubén Justo]

Avoid some confusing errors operating with orphan branches when working
with worktrees.

Changes from v3:

This is a major refactoring based on the comments from the reviews of
v3.  The intention is to make no functional changes in this iteration,
except a change to maintain unchanged the user-visible behavior, as
Jonathan suggested.

Summary of changes:

  - Inlined reject_rebase_or_bisect_branch() and
    replace_each_head_symref() into copy_or_rename_branch(). 

  - Used names to define HEAD states.

  - Moved die("Branch foo is being rebased/bisected...") after the call
    to validate_branchname(), to maintain the user-visible behavior.

  - Removed the use of ternary operators to avoid introducing unneeded
    noise in the patch.
 

Rubén Justo (3):
  branch: avoid unnecessary worktrees traversals
  branch: description for orphan branch errors
  branch: rename orphan branches in any worktree

 branch.c               | 27 ------------
 branch.h               |  8 ----
 builtin/branch.c       | 99 ++++++++++++++++++++++++++++--------------
 t/t3200-branch.sh      | 14 ++++++
 t/t3202-show-branch.sh | 18 ++++++++
 5 files changed, 98 insertions(+), 68 deletions(-)

-- 
2.34.1

[PATCH v4 1/3] branch: avoid unnecessary worktrees traversals

[Rubén Justo]

"reject_rebase_or_bisect_branch()" was introduced [1] to prevent a
branch under bisect or rebase from being renamed or copied.  It
traverses all worktrees in the repository and dies if the specified
branch is being rebased or bisected in any of them.

"replace_each_worktree_head_symref()" was introduced [2] to adjust the
HEAD in all worktrees after a branch rename succeeded.  It traverses all
worktrees in the repository and if any of them have their HEAD pointing
to the renamed ref, it adjusts it.

Considering that both functions are only called from within
copy_or_rename_branch() and each of them traverses all worktrees, which
might involve disk access and resolving multiple references, inlining
these two functions to do the traversing once, makes sense.

  1.- 14ace5b (branch: do not rename a branch under bisect or rebase,
      2016-04-22)

  2.- 70999e9 (branch -m: update all per-worktree HEADs, 2016-03-27)

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 branch.c         | 27 --------------------
 branch.h         |  8 ------
 builtin/branch.c | 64 ++++++++++++++++++++++++++++--------------------
 3 files changed, 38 insertions(+), 61 deletions(-)

diff --git a/branch.c b/branch.c
index e5614b53b3..f64062be71 100644
--- a/branch.c
+++ b/branch.c
@@ -830,30 +830,3 @@ void die_if_checked_out(const char *branch, int ignore_current_worktree)
 
 	free_worktrees(worktrees);
 }
-
-int replace_each_worktree_head_symref(const char *oldref, const char *newref,
-				      const char *logmsg)
-{
-	int ret = 0;
-	struct worktree **worktrees = get_worktrees();
-	int i;
-
-	for (i = 0; worktrees[i]; i++) {
-		struct ref_store *refs;
-
-		if (worktrees[i]->is_detached)
-			continue;
-		if (!worktrees[i]->head_ref)
-			continue;
-		if (strcmp(oldref, worktrees[i]->head_ref))
-			continue;
-
-		refs = get_worktree_ref_store(worktrees[i]);
-		if (refs_create_symref(refs, "HEAD", newref, logmsg))
-			ret = error(_("HEAD of working tree %s is not updated"),
-				    worktrees[i]->path);
-	}
-
-	free_worktrees(worktrees);
-	return ret;
-}
diff --git a/branch.h b/branch.h
index ef56103c05..30c01aed73 100644
--- a/branch.h
+++ b/branch.h
@@ -155,12 +155,4 @@ int read_branch_desc(struct strbuf *, const char *branch_name);
  */
 void die_if_checked_out(const char *branch, int ignore_current_worktree);
 
-/*
- * Update all per-worktree HEADs pointing at the old ref to point the new ref.
- * This will be used when renaming a branch. Returns 0 if successful, non-zero
- * otherwise.
- */
-int replace_each_worktree_head_symref(const char *oldref, const char *newref,
-				      const char *logmsg);
-
 #endif
diff --git a/builtin/branch.c b/builtin/branch.c
index f63fd45edb..a32ae64006 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -486,28 +486,6 @@ static void print_current_branch_name(void)
 		die(_("HEAD (%s) points outside of refs/heads/"), refname);
 }
 
-static void reject_rebase_or_bisect_branch(const char *target)
-{
-	struct worktree **worktrees = get_worktrees();
-	int i;
-
-	for (i = 0; worktrees[i]; i++) {
-		struct worktree *wt = worktrees[i];
-
-		if (!wt->is_detached)
-			continue;
-
-		if (is_worktree_being_rebased(wt, target))
-			die(_("Branch %s is being rebased at %s"),
-			    target, wt->path);
-
-		if (is_worktree_being_bisected(wt, target))
-			die(_("Branch %s is being bisected at %s"),
-			    target, wt->path);
-	}
-
-	free_worktrees(worktrees);
-}
 
 static void copy_or_rename_branch(const char *oldname, const char *newname, int copy, int force)
 {
@@ -516,6 +494,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
 	const char *interpreted_oldname = NULL;
 	const char *interpreted_newname = NULL;
 	int recovery = 0;
+	struct worktree **worktrees = get_worktrees();
 
 	if (strbuf_check_branch_ref(&oldref, oldname)) {
 		/*
@@ -544,7 +523,20 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
 	else
 		validate_new_branchname(newname, &newref, force);
 
-	reject_rebase_or_bisect_branch(oldref.buf);
+	for (int i = 0; worktrees[i]; i++) {
+		struct worktree *wt = worktrees[i];
+
+		if (!wt->is_detached)
+			continue;
+
+		if (is_worktree_being_rebased(wt, oldref.buf))
+			die(_("Branch %s is being rebased at %s"),
+			    oldref.buf, wt->path);
+
+		if (is_worktree_being_bisected(wt, oldref.buf))
+			die(_("Branch %s is being bisected at %s"),
+			    oldref.buf, wt->path);
+	}
 
 	if (!skip_prefix(oldref.buf, "refs/heads/", &interpreted_oldname) ||
 	    !skip_prefix(newref.buf, "refs/heads/", &interpreted_newname)) {
@@ -574,9 +566,29 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
 				interpreted_oldname);
 	}
 
-	if (!copy &&
-	    replace_each_worktree_head_symref(oldref.buf, newref.buf, logmsg.buf))
-		die(_("Branch renamed to %s, but HEAD is not updated!"), newname);
+	if (!copy) {
+		/*
+		 * Update all per-worktree HEADs pointing at the old ref to
+		 * point the new ref.
+		 */
+		for (int i = 0; worktrees[i]; i++) {
+			struct ref_store *refs;
+
+			if (worktrees[i]->is_detached)
+				continue;
+			if (!worktrees[i]->head_ref)
+				continue;
+			if (strcmp(oldref.buf, worktrees[i]->head_ref))
+				continue;
+
+			refs = get_worktree_ref_store(worktrees[i]);
+			if (refs_create_symref(refs, "HEAD", newref.buf, logmsg.buf))
+				die(_("Branch renamed to %s, but HEAD is not updated!"),
+					newname);
+		}
+	}
+
+	free_worktrees(worktrees);
 
 	strbuf_release(&logmsg);
 
-- 
2.34.1

[PATCH v4 2/3] branch: description for orphan branch errors

[Rubén Justo]

In bcfc82bd48 (branch: description for non-existent branch errors,
2022-10-08) we checked the current HEAD to detect if the branch to
operate with is an orphan branch, so as to avoid the confusing error:
"No branch named...".

If we are asked to operate with an orphan branch in a different working
tree than the current one, we need to check the HEAD in that different
working tree.

Let's extend the check we did in bcfc82bd48, to all HEADs in the
repository, using the helper introduced in 31ad6b61bd (branch: add
branch_checked_out() helper, 2022-06-15)

We are already traversing all worktrees in "copy_or_rename_branch()"
checking if the branch to be copied or renamed is being bisected or
rebased.  Let's include also a check for being HEAD, and use this
information within the function rather than the helper.  This implies
doing the worktrees iteration earlier in the function; to keep the
user-visible behavior unchanged lets maintain the die("Branch foo is
being rebased/bisected...") in the same position within the function. 

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 builtin/branch.c       | 53 ++++++++++++++++++++++++++++--------------
 t/t3202-show-branch.sh | 18 ++++++++++++++
 2 files changed, 53 insertions(+), 18 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index a32ae64006..aecf009993 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -486,6 +486,9 @@ static void print_current_branch_name(void)
 		die(_("HEAD (%s) points outside of refs/heads/"), refname);
 }
 
+#define IS_BISECTED 1
+#define IS_REBASED 2
+#define IS_HEAD 4
 
 static void copy_or_rename_branch(const char *oldname, const char *newname, int copy, int force)
 {
@@ -493,8 +496,9 @@ 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_usage = 0;
 	struct worktree **worktrees = get_worktrees();
+	struct worktree *oldref_wt = NULL;
 
 	if (strbuf_check_branch_ref(&oldref, oldname)) {
 		/*
@@ -507,8 +511,28 @@ 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))
+	for (int i = 0; worktrees[i]; i++) {
+		struct worktree *wt = worktrees[i];
+
+		if (wt->head_ref && !strcmp(oldref.buf, wt->head_ref))
+			oldref_usage |= IS_HEAD;
+
+		if (!wt->is_detached)
+			continue;
+
+		if (is_worktree_being_rebased(wt, oldref.buf)) {
+			oldref_usage |= IS_REBASED;
+			oldref_wt = wt;
+		}
+
+		if (is_worktree_being_bisected(wt, oldref.buf)) {
+			oldref_usage |= IS_BISECTED;
+			oldref_wt = wt;
+		}
+	}
+
+	if ((copy || !(oldref_usage & IS_HEAD)) && !ref_exists(oldref.buf)) {
+		if (oldref_usage & IS_HEAD)
 			die(_("No commit on branch '%s' yet."), oldname);
 		else
 			die(_("No branch named '%s'."), oldname);
@@ -523,20 +547,13 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
 	else
 		validate_new_branchname(newname, &newref, force);
 
-	for (int i = 0; worktrees[i]; i++) {
-		struct worktree *wt = worktrees[i];
+	if (oldref_usage & IS_BISECTED)
+		die(_("Branch %s is being rebased at %s"),
+		    oldref.buf, oldref_wt->path);
 
-		if (!wt->is_detached)
-			continue;
-
-		if (is_worktree_being_rebased(wt, oldref.buf))
-			die(_("Branch %s is being rebased at %s"),
-			    oldref.buf, wt->path);
-
-		if (is_worktree_being_bisected(wt, oldref.buf))
-			die(_("Branch %s is being bisected at %s"),
-			    oldref.buf, wt->path);
-	}
+	if (oldref_usage & IS_REBASED)
+		die(_("Branch %s is being bisected at %s"),
+		    oldref.buf, oldref_wt->path);
 
 	if (!skip_prefix(oldref.buf, "refs/heads/", &interpreted_oldname) ||
 	    !skip_prefix(newref.buf, "refs/heads/", &interpreted_newname)) {
@@ -818,7 +835,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 
 		strbuf_addf(&branch_ref, "refs/heads/%s", branch_name);
 		if (!ref_exists(branch_ref.buf))
-			error((!argc || !strcmp(head, branch_name))
+			error((!argc || branch_checked_out(branch_ref.buf))
 			      ? _("No commit on branch '%s' yet.")
 			      : _("No branch named '%s'."),
 			      branch_name);
@@ -863,7 +880,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		}
 
 		if (!ref_exists(branch->refname)) {
-			if (!argc || !strcmp(head, branch->name))
+			if (!argc || branch_checked_out(branch->refname))
 				die(_("No commit on branch '%s' yet."), branch->name);
 			die(_("branch '%s' does not exist"), branch->name);
 		}
diff --git a/t/t3202-show-branch.sh b/t/t3202-show-branch.sh
index ea7cfd1951..be20ebe1d5 100755
--- a/t/t3202-show-branch.sh
+++ b/t/t3202-show-branch.sh
@@ -221,4 +221,22 @@ test_expect_success 'fatal descriptions on non-existent branch' '
 	test_cmp expect actual
 '
 
+test_expect_success 'error descriptions on orphan branch' '
+	test_when_finished git worktree remove -f wt &&
+	git worktree add wt --detach &&
+	git -C wt checkout --orphan orphan-branch &&
+	test_branch_op_in_wt() {
+		test_orphan_error() {
+			test_must_fail git $* 2>actual &&
+			test_i18ngrep "No commit on branch .orphan-branch. yet.$" actual
+		} &&
+		test_orphan_error -C wt branch $1 $2 &&                # implicit branch
+		test_orphan_error -C wt branch $1 orphan-branch $2 &&  # explicit branch
+		test_orphan_error branch $1 orphan-branch $2           # different worktree
+	} &&
+	test_branch_op_in_wt --edit-description &&
+	test_branch_op_in_wt --set-upstream-to=ne &&
+	test_branch_op_in_wt -c new-branch
+'
+
 test_done
-- 
2.34.1

[PATCH v4 3/3] branch: rename orphan branches in any worktree

[Rubén Justo]

In cfaff3aac (branch -m: allow renaming a yet-unborn branch, 2020-12-13)
we added support for renaming an orphan branch, skipping rename_ref() if
the branch being renamed is an orphan branch; checking the current HEAD.

However the branch to be renamed can be an orphan branch in a different
worktree than the current one, i.e. not the current HEAD.

In "copy_or_rename_branch()" we are traversing the worktrees checking if
the branch to be renamed is HEAD in any worktree.  Let's include also a
check for HEAD being NULL, which is the indication of an orphan branch,
and use it to extend what we did in cfaff3aac, to all HEADs in the
repository.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 builtin/branch.c  | 16 ++++++++++------
 t/t3200-branch.sh | 14 ++++++++++++++
 2 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index aecf009993..24cd66bae7 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -489,6 +489,7 @@ static void print_current_branch_name(void)
 #define IS_BISECTED 1
 #define IS_REBASED 2
 #define IS_HEAD 4
+#define IS_ORPHAN 8
 
 static void copy_or_rename_branch(const char *oldname, const char *newname, int copy, int force)
 {
@@ -514,8 +515,11 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
 	for (int i = 0; worktrees[i]; i++) {
 		struct worktree *wt = worktrees[i];
 
-		if (wt->head_ref && !strcmp(oldref.buf, wt->head_ref))
+		if (wt->head_ref && !strcmp(oldref.buf, wt->head_ref)) {
 			oldref_usage |= IS_HEAD;
+			if (is_null_oid(&wt->head_oid))
+				oldref_usage |= IS_ORPHAN;
+		}
 
 		if (!wt->is_detached)
 			continue;
@@ -531,8 +535,9 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
 		}
 	}
 
-	if ((copy || !(oldref_usage & IS_HEAD)) && !ref_exists(oldref.buf)) {
-		if (oldref_usage & IS_HEAD)
+	if ((copy || !(oldref_usage & IS_HEAD)) &&
+	    ((oldref_usage & IS_ORPHAN) || !ref_exists(oldref.buf))) {
+		if (oldref_usage & IS_ORPHAN)
 			die(_("No commit on branch '%s' yet."), oldname);
 		else
 			die(_("No branch named '%s'."), oldname);
@@ -567,8 +572,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 &&
-	    (!head || strcmp(oldname, head) || !is_null_oid(&head_oid)) &&
+	if (!copy && !(oldref_usage & IS_ORPHAN) &&
 	    rename_ref(oldref.buf, newref.buf, logmsg.buf))
 		die(_("Branch rename failed"));
 	if (copy && copy_existing_ref(oldref.buf, newref.buf, logmsg.buf))
@@ -583,7 +587,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
 				interpreted_oldname);
 	}
 
-	if (!copy) {
+	if (!copy && (oldref_usage & IS_HEAD)) {
 		/*
 		 * Update all per-worktree HEADs pointing at the old ref to
 		 * point the new ref.
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 5a169b68d6..5aef00efde 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -279,6 +279,20 @@ test_expect_success 'git branch -M and -C fail on detached HEAD' '
 	test_cmp expect err
 '
 
+test_expect_success 'git branch -m should work with orphan branches' '
+	test_when_finished git checkout - &&
+	test_when_finished git worktree remove -f wt &&
+	git worktree add wt --detach &&
+	# rename orphan in another worktreee
+	git -C wt checkout --orphan orphan-foo-wt &&
+	git branch -m orphan-foo-wt orphan-bar-wt &&
+	test orphan-bar-wt=$(git -C orphan-worktree branch --show-current) &&
+	# rename orphan in the current worktree
+	git checkout --orphan orphan-foo &&
+	git branch -m orphan-foo orphan-bar &&
+	test orphan-bar=$(git branch --show-current)
+'
+
 test_expect_success 'git branch -d on orphan HEAD (merged)' '
 	test_when_finished git checkout main &&
 	git checkout --orphan orphan &&
-- 
2.34.1

Re: [PATCH v4 1/3] branch: avoid unnecessary worktrees traversals

[Rubén Justo]

On 22-feb-2023 23:52:51, Rubén Justo wrote:
> "reject_rebase_or_bisect_branch()" was introduced [1] to prevent a
> branch under bisect or rebase from being renamed or copied.  It
> traverses all worktrees in the repository and dies if the specified
> branch is being rebased or bisected in any of them.
> 
> "replace_each_worktree_head_symref()" was introduced [2] to adjust the
> HEAD in all worktrees after a branch rename succeeded.  It traverses all
> worktrees in the repository and if any of them have their HEAD pointing
> to the renamed ref, it adjusts it.
> 
> Considering that both functions are only called from within
> copy_or_rename_branch() and each of them traverses all worktrees, which
> might involve disk access and resolving multiple references, inlining
> these two functions to do the traversing once, makes sense.
> 
>   1.- 14ace5b (branch: do not rename a branch under bisect or rebase,
>       2016-04-22)
> 
>   2.- 70999e9 (branch -m: update all per-worktree HEADs, 2016-03-27)
> 
> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
>  branch.c         | 27 --------------------
>  branch.h         |  8 ------
>  builtin/branch.c | 64 ++++++++++++++++++++++++++++--------------------
>  3 files changed, 38 insertions(+), 61 deletions(-)
> 
> diff --git a/branch.c b/branch.c
> index e5614b53b3..f64062be71 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -830,30 +830,3 @@ void die_if_checked_out(const char *branch, int ignore_current_worktree)
>  
>  	free_worktrees(worktrees);
>  }
> -
> -int replace_each_worktree_head_symref(const char *oldref, const char *newref,
> -				      const char *logmsg)
> -{
> -	int ret = 0;
> -	struct worktree **worktrees = get_worktrees();
> -	int i;
> -
> -	for (i = 0; worktrees[i]; i++) {
> -		struct ref_store *refs;
> -
> -		if (worktrees[i]->is_detached)
> -			continue;
> -		if (!worktrees[i]->head_ref)
> -			continue;
> -		if (strcmp(oldref, worktrees[i]->head_ref))
> -			continue;
> -
> -		refs = get_worktree_ref_store(worktrees[i]);
> -		if (refs_create_symref(refs, "HEAD", newref, logmsg))
> -			ret = error(_("HEAD of working tree %s is not updated"),
> -				    worktrees[i]->path);
> -	}
> -
> -	free_worktrees(worktrees);
> -	return ret;
> -}
> diff --git a/branch.h b/branch.h
> index ef56103c05..30c01aed73 100644
> --- a/branch.h
> +++ b/branch.h
> @@ -155,12 +155,4 @@ int read_branch_desc(struct strbuf *, const char *branch_name);
>   */
>  void die_if_checked_out(const char *branch, int ignore_current_worktree);
>  
> -/*
> - * Update all per-worktree HEADs pointing at the old ref to point the new ref.
> - * This will be used when renaming a branch. Returns 0 if successful, non-zero
> - * otherwise.
> - */
> -int replace_each_worktree_head_symref(const char *oldref, const char *newref,
> -				      const char *logmsg);
> -
>  #endif
> diff --git a/builtin/branch.c b/builtin/branch.c
> index f63fd45edb..a32ae64006 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -486,28 +486,6 @@ static void print_current_branch_name(void)
>  		die(_("HEAD (%s) points outside of refs/heads/"), refname);
>  }
>  
> -static void reject_rebase_or_bisect_branch(const char *target)
> -{
> -	struct worktree **worktrees = get_worktrees();
> -	int i;
> -
> -	for (i = 0; worktrees[i]; i++) {
> -		struct worktree *wt = worktrees[i];
> -
> -		if (!wt->is_detached)
> -			continue;
> -
> -		if (is_worktree_being_rebased(wt, target))
> -			die(_("Branch %s is being rebased at %s"),
> -			    target, wt->path);
> -
> -		if (is_worktree_being_bisected(wt, target))
> -			die(_("Branch %s is being bisected at %s"),
> -			    target, wt->path);
> -	}
> -
> -	free_worktrees(worktrees);
> -}
>  
>  static void copy_or_rename_branch(const char *oldname, const char *newname, int copy, int force)
>  {
> @@ -516,6 +494,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
>  	const char *interpreted_oldname = NULL;
>  	const char *interpreted_newname = NULL;
>  	int recovery = 0;
> +	struct worktree **worktrees = get_worktrees();
>  
>  	if (strbuf_check_branch_ref(&oldref, oldname)) {
>  		/*
> @@ -544,7 +523,20 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
>  	else
>  		validate_new_branchname(newname, &newref, force);
>  
> -	reject_rebase_or_bisect_branch(oldref.buf);
> +	for (int i = 0; worktrees[i]; i++) {
> +		struct worktree *wt = worktrees[i];
> +
> +		if (!wt->is_detached)
> +			continue;
> +
> +		if (is_worktree_being_rebased(wt, oldref.buf))
> +			die(_("Branch %s is being rebased at %s"),
> +			    oldref.buf, wt->path);
> +
> +		if (is_worktree_being_bisected(wt, oldref.buf))
> +			die(_("Branch %s is being bisected at %s"),
> +			    oldref.buf, wt->path);
> +	}
>  
>  	if (!skip_prefix(oldref.buf, "refs/heads/", &interpreted_oldname) ||
>  	    !skip_prefix(newref.buf, "refs/heads/", &interpreted_newname)) {
> @@ -574,9 +566,29 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
>  				interpreted_oldname);
>  	}
>  
> -	if (!copy &&
> -	    replace_each_worktree_head_symref(oldref.buf, newref.buf, logmsg.buf))
> -		die(_("Branch renamed to %s, but HEAD is not updated!"), newname);
> +	if (!copy) {
> +		/*
> +		 * Update all per-worktree HEADs pointing at the old ref to
> +		 * point the new ref.
> +		 */
> +		for (int i = 0; worktrees[i]; i++) {
> +			struct ref_store *refs;
> +
> +			if (worktrees[i]->is_detached)
> +				continue;
> +			if (!worktrees[i]->head_ref)
> +				continue;
> +			if (strcmp(oldref.buf, worktrees[i]->head_ref))
> +				continue;
> +
> +			refs = get_worktree_ref_store(worktrees[i]);
> +			if (refs_create_symref(refs, "HEAD", newref.buf, logmsg.buf))
> +				die(_("Branch renamed to %s, but HEAD is not updated!"),
> +					newname);

Reviewing this, I noticed I made a mistake here.  The original code
doesn't stop iterating whenever refs_create_symref() fails; it continues
trying to update the remaining worktrees.  When the iteration ends, if
any of the updates failed, then die().  Also, the error message "HEAD of
working tree %s is not updated" is lost.  I'll reroll with this errors
fixed but maybe some review is already underway, so I'll wait some
time.  Sorry for the inconvenience.

Re: [PATCH v4 1/3] branch: avoid unnecessary worktrees traversals

[Jonathan Tan]

Rubén Justo <rjusto@gmail.com> writes:
> Reviewing this, I noticed I made a mistake here.  The original code
> doesn't stop iterating whenever refs_create_symref() fails; it continues
> trying to update the remaining worktrees.  When the iteration ends, if
> any of the updates failed, then die().  Also, the error message "HEAD of
> working tree %s is not updated" is lost.  

Ah yes, I noticed this too.

Besides that, a reviewer, upon reading the commit message, might ask:
why not take the worktrees as a parameter then, if we're so worried
about performance? I think that the real reason for inlining is that the
code being inlined needs to communicate more information to its calling
function in subsequent patches; the performance improvement is only a
beneficial side effect.

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

[Jonathan Tan]

Firstly, the subject could be more precise. Maybe "branch: check all
worktrees for orphan branches" (47 characters) or something like that.

Rubén Justo <rjusto@gmail.com> writes:
> In bcfc82bd48 (branch: description for non-existent branch errors,
> 2022-10-08) we checked the current HEAD

Probably clearer to say "HEAD in the current worktree" instead of
"current HEAD".

> to detect if the branch to
> operate with is an orphan branch, so as to avoid the confusing error:
> "No branch named...".
> 
> If we are asked to operate with an orphan branch in a different working
> tree than the current one, we need to check the HEAD in that different
> working tree.

Probably clearer to just say "But there might be orphan branches in
other worktrees".

> Let's extend the check we did in bcfc82bd48, to all HEADs in the
> repository, using the helper introduced in 31ad6b61bd (branch: add
> branch_checked_out() helper, 2022-06-15)

s/HEADs/worktrees/

> @@ -493,8 +496,9 @@ 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_usage = 0;
>  	struct worktree **worktrees = get_worktrees();
> +	struct worktree *oldref_wt = NULL;

Better to have 2 variables (one for rebased, and one for bisected) to
avoid the situation in which the last problematic worktree seen was a
bisected one, but a prior one was a rebased one.

> @@ -818,7 +835,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>  
>  		strbuf_addf(&branch_ref, "refs/heads/%s", branch_name);
>  		if (!ref_exists(branch_ref.buf))
> -			error((!argc || !strcmp(head, branch_name))
> +			error((!argc || branch_checked_out(branch_ref.buf))
>  			      ? _("No commit on branch '%s' yet.")
>  			      : _("No branch named '%s'."),
>  			      branch_name);
> @@ -863,7 +880,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>  		}
>  
>  		if (!ref_exists(branch->refname)) {
> -			if (!argc || !strcmp(head, branch->name))
> +			if (!argc || branch_checked_out(branch->refname))
>  				die(_("No commit on branch '%s' yet."), branch->name);
>  			die(_("branch '%s' does not exist"), branch->name);
>  		}

What is the relevance of these changes?

Re: [PATCH v4 3/3] branch: rename orphan branches in any worktree

[Jonathan Tan]

Rubén Justo <rjusto@gmail.com> writes:
> @@ -583,7 +587,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
>  				interpreted_oldname);
>  	}
>  
> -	if (!copy) {
> +	if (!copy && (oldref_usage & IS_HEAD)) {
>  		/*
>  		 * Update all per-worktree HEADs pointing at the old ref to
>  		 * point the new ref.

What is the relevance of this change?

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

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

> Firstly, the subject could be more precise. Maybe "branch: check all
> worktrees for orphan branches" (47 characters) or something like that.
>
> Rubén Justo <rjusto@gmail.com> writes:
>> In bcfc82bd48 (branch: description for non-existent branch errors,
>> 2022-10-08) we checked the current HEAD
>
> Probably clearer to say "HEAD in the current worktree" instead of
> "current HEAD".
>
>> to detect if the branch to
>> operate with is an orphan branch, so as to avoid the confusing error:
>> "No branch named...".
>> 
>> If we are asked to operate with an orphan branch in a different working
>> tree than the current one, we need to check the HEAD in that different
>> working tree.
>
> Probably clearer to just say "But there might be orphan branches in
> other worktrees".
>
>> Let's extend the check we did in bcfc82bd48, to all HEADs in the
>> repository, using the helper introduced in 31ad6b61bd (branch: add
>> branch_checked_out() helper, 2022-06-15)
>
> s/HEADs/worktrees/
>
>> @@ -493,8 +496,9 @@ 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_usage = 0;
>>  	struct worktree **worktrees = get_worktrees();
>> +	struct worktree *oldref_wt = NULL;
>
> Better to have 2 variables (one for rebased, and one for bisected) to
> avoid the situation in which the last problematic worktree seen was a
> bisected one, but a prior one was a rebased one.

All good suggestions.

>> @@ -818,7 +835,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>>  
>>  		strbuf_addf(&branch_ref, "refs/heads/%s", branch_name);
>>  		if (!ref_exists(branch_ref.buf))
>> -			error((!argc || !strcmp(head, branch_name))
>> +			error((!argc || branch_checked_out(branch_ref.buf))
>>  			      ? _("No commit on branch '%s' yet.")
>>  			      : _("No branch named '%s'."),
>>  			      branch_name);
>> @@ -863,7 +880,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>>  		}
>>  
>>  		if (!ref_exists(branch->refname)) {
>> -			if (!argc || !strcmp(head, branch->name))
>> +			if (!argc || branch_checked_out(branch->refname))
>>  				die(_("No commit on branch '%s' yet."), branch->name);
>>  			die(_("branch '%s' does not exist"), branch->name);
>>  		}
>
> What is the relevance of these changes?

Thanks for reading very carefully and asking clarification for
relevant parts of the changes.

Re: [PATCH v4 1/3] branch: avoid unnecessary worktrees traversals

[Rubén Justo]

On 27/2/23 20:30, Jonathan Tan wrote:
> Rubén Justo <rjusto@gmail.com> writes:
>> Reviewing this, I noticed I made a mistake here.  The original code
>> doesn't stop iterating whenever refs_create_symref() fails; it continues
>> trying to update the remaining worktrees.  When the iteration ends, if
>> any of the updates failed, then die().  Also, the error message "HEAD of
>> working tree %s is not updated" is lost.  
> 
> Ah yes, I noticed this too.

I'll re-roll with the fix in the next iteration.

Thank you for reading carefully.

> 
> Besides that, a reviewer, upon reading the commit message, might ask:
> why not take the worktrees as a parameter then, if we're so worried
> about performance? I think that the real reason for inlining is that the
> code being inlined needs to communicate more information to its calling
> function in subsequent patches; the performance improvement is only a
> beneficial side effect.
> 

Certainly, that's a way to see the modification within this series.
Actually, this modification wasn't present in v1, but once it was
introduced in v2, it made sense on its own: to eliminate the redundant
traversals of worktrees when renaming a branch, even if the branch isn't
HEAD in any worktree.

Therefore, I think the change can also be seen in another way: the
increased ease in the next modifications is a beneficial side effect
of this patch.

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

[Rubén Justo]

Thank you for your review!

On 27/2/23 20:38, Jonathan Tan wrote:
> Firstly, the subject could be more precise. Maybe "branch: check all
> worktrees for orphan branches" (47 characters) or something like that.

The main intention in this series is to stop giving the user a confusing
error "No branch named..." for a branch he may have just created.  I
think the current subject states that better.  But I'm open to change it
in that direction.

> Rubén Justo <rjusto@gmail.com> writes:
>> In bcfc82bd48 (branch: description for non-existent branch errors,
>> 2022-10-08) we checked the current HEAD
> 
> Probably clearer to say "HEAD in the current worktree" instead of
> "current HEAD".

OK. I'll reword with that.

>> to detect if the branch to
>> operate with is an orphan branch, so as to avoid the confusing error:
>> "No branch named...".
>>
>> If we are asked to operate with an orphan branch in a different working
>> tree than the current one, we need to check the HEAD in that different
>> working tree.
> 
> Probably clearer to just say "But there might be orphan branches in
> other worktrees".

That loses important details IMHO, the intention: "avoid the
confusing..", and the reasoning on why we need to check HEAD in all
worktrees.

>> Let's extend the check we did in bcfc82bd48, to all HEADs in the
>> repository, using the helper introduced in 31ad6b61bd (branch: add
>> branch_checked_out() helper, 2022-06-15)
> 
> s/HEADs/worktrees/

I understand your suggestion, but my intention along the message is to
maintain the reasoning on the "HEAD", due to an orphan branch is a HEAD
pointing to a non-existing ref.  Maybe "the HEADs in all worktrees"
could be better?

>> @@ -493,8 +496,9 @@ 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_usage = 0;
>>  	struct worktree **worktrees = get_worktrees();
>> +	struct worktree *oldref_wt = NULL;
> 
> Better to have 2 variables (one for rebased, and one for bisected) to
> avoid the situation in which the last problematic worktree seen was a
> bisected one, but a prior one was a rebased one.

Well seen.  Thanks for reading carefully.

I'll re-roll with that.

>> @@ -818,7 +835,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>>  
>>  		strbuf_addf(&branch_ref, "refs/heads/%s", branch_name);
>>  		if (!ref_exists(branch_ref.buf))
>> -			error((!argc || !strcmp(head, branch_name))
>> +			error((!argc || branch_checked_out(branch_ref.buf))
>>  			      ? _("No commit on branch '%s' yet.")
>>  			      : _("No branch named '%s'."),
>>  			      branch_name);
>> @@ -863,7 +880,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>>  		}
>>  
>>  		if (!ref_exists(branch->refname)) {
>> -			if (!argc || !strcmp(head, branch->name))
>> +			if (!argc || branch_checked_out(branch->refname))
>>  				die(_("No commit on branch '%s' yet."), branch->name);
>>  			die(_("branch '%s' does not exist"), branch->name);
>>  		}
> 
> What is the relevance of these changes?
> 

This is the main intention in the patch: not showing the confusing error
"No branch named..." for orphan branches.  I'm not sure if I understand
your question...

Re: [PATCH v4 3/3] branch: rename orphan branches in any worktree

[Rubén Justo]

On 27/2/23 20:41, Jonathan Tan wrote:
> Rubén Justo <rjusto@gmail.com> writes:
>> @@ -583,7 +587,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
>>  				interpreted_oldname);
>>  	}
>>  
>> -	if (!copy) {
>> +	if (!copy && (oldref_usage & IS_HEAD)) {
>>  		/*
>>  		 * Update all per-worktree HEADs pointing at the old ref to
>>  		 * point the new ref.
> 
> What is the relevance of this change?
> 

Maybe this change makes more sense in 2/3?

[PATCH v5 0/5] branch: operations on orphan branches

[Rubén Justo]

The initial and main intention in this series is to avoid some confusing
errors operating with orphan branches, when working with worktrees.

It's been a while since the previous iteration, sorry for that.  Here is
a quick overview of the previous iterations:

In v2 a change was introduced, to avoid some unnecessary worktrees
traversals, which implies disk access.

In v3 and v4 some refactorings were done, such as making some names
more meaningful and, mainly, inlining some helper functions in its only
one caller.  No functional changes were expected.  However, those
refactorings introduced some undesired behavior changes; some were noted
in the round of reviews, some others during the preparation of this new
iteration.

In this iteration, v5, I've reverted some of those major refactorings,
mainly the inlining, because it ended up introducing unnecessary
complexity for minimal benefit in this series.  Maybe those refactorings
make more sense in future series.

A new test has been introduced, in 1/5, to notice if a behavior change
similar to that observed in v4, is reintroduced in the future.

Other than that, no functional changes are expected from v2.

Rubén Justo (5):
  branch: test for failures while renaming branches
  branch: use get_worktrees() in copy_or_rename_branch()
  branch: description for orphan branch errors
  branch: rename orphan branches in any worktree
  branch: avoid unnecessary worktrees traversals

 branch.c               | 27 ----------------
 branch.h               |  8 -----
 builtin/branch.c       | 71 ++++++++++++++++++++++++++++++++++--------
 t/t3200-branch.sh      | 29 +++++++++++++++++
 t/t3202-show-branch.sh | 18 +++++++++++
 5 files changed, 105 insertions(+), 48 deletions(-)

-- 
2.39.2

[PATCH v5 1/5] branch: test for failures while renaming branches

[Rubén Justo]

When we introduced replace_each_worktree_head_symref() in 70999e9cec
(branch -m: update all per-worktree HEADs, 2016-03-27), we implemented a
best effort approach.

If we are asked to rename a branch that is simultaneously checked out in
multiple worktrees, we try to update all of those worktrees.  If we fail
updating any of them, we die() as a signal that something has gone
wrong.  However, at this point, the branch ref has already been renamed
and also updated the HEADs of the successfully updated worktrees.
Despite returning an error, we do not try to rollback those changes.

Let's add a test to notice if we change this behavior in the future.

In next commits we will change replace_each_worktree_head_symref() to
work more closely with its only caller, copy_or_rename_branch().  Let's
move the former closer to its caller, to facilitate those changes.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 branch.c          | 27 ---------------------------
 branch.h          |  8 --------
 builtin/branch.c  | 32 ++++++++++++++++++++++++++++++++
 t/t3200-branch.sh | 15 +++++++++++++++
 4 files changed, 47 insertions(+), 35 deletions(-)

diff --git a/branch.c b/branch.c
index eacef62b7c..b45454593a 100644
--- a/branch.c
+++ b/branch.c
@@ -835,30 +835,3 @@ void die_if_checked_out(const char *branch, int ignore_current_worktree)
 
 	free_worktrees(worktrees);
 }
-
-int replace_each_worktree_head_symref(const char *oldref, const char *newref,
-				      const char *logmsg)
-{
-	int ret = 0;
-	struct worktree **worktrees = get_worktrees();
-	int i;
-
-	for (i = 0; worktrees[i]; i++) {
-		struct ref_store *refs;
-
-		if (worktrees[i]->is_detached)
-			continue;
-		if (!worktrees[i]->head_ref)
-			continue;
-		if (strcmp(oldref, worktrees[i]->head_ref))
-			continue;
-
-		refs = get_worktree_ref_store(worktrees[i]);
-		if (refs_create_symref(refs, "HEAD", newref, logmsg))
-			ret = error(_("HEAD of working tree %s is not updated"),
-				    worktrees[i]->path);
-	}
-
-	free_worktrees(worktrees);
-	return ret;
-}
diff --git a/branch.h b/branch.h
index ef56103c05..30c01aed73 100644
--- a/branch.h
+++ b/branch.h
@@ -155,12 +155,4 @@ int read_branch_desc(struct strbuf *, const char *branch_name);
  */
 void die_if_checked_out(const char *branch, int ignore_current_worktree);
 
-/*
- * Update all per-worktree HEADs pointing at the old ref to point the new ref.
- * This will be used when renaming a branch. Returns 0 if successful, non-zero
- * otherwise.
- */
-int replace_each_worktree_head_symref(const char *oldref, const char *newref,
-				      const char *logmsg);
-
 #endif
diff --git a/builtin/branch.c b/builtin/branch.c
index f63fd45edb..c7ace2f2da 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -509,6 +509,38 @@ static void reject_rebase_or_bisect_branch(const char *target)
 	free_worktrees(worktrees);
 }
 
+/*
+ * Update all per-worktree HEADs pointing at the old ref to point the new ref.
+ * This will be used when renaming a branch. Returns 0 if successful, non-zero
+ * otherwise.
+ */
+static int replace_each_worktree_head_symref(const char *oldref, const char *newref,
+					     const char *logmsg)
+{
+	int ret = 0;
+	struct worktree **worktrees = get_worktrees();
+	int i;
+
+	for (i = 0; worktrees[i]; i++) {
+		struct ref_store *refs;
+
+		if (worktrees[i]->is_detached)
+			continue;
+		if (!worktrees[i]->head_ref)
+			continue;
+		if (strcmp(oldref, worktrees[i]->head_ref))
+			continue;
+
+		refs = get_worktree_ref_store(worktrees[i]);
+		if (refs_create_symref(refs, "HEAD", newref, logmsg))
+			ret = error(_("HEAD of working tree %s is not updated"),
+				    worktrees[i]->path);
+	}
+
+	free_worktrees(worktrees);
+	return ret;
+}
+
 static void copy_or_rename_branch(const char *oldname, const char *newname, int copy, int force)
 {
 	struct strbuf oldref = STRBUF_INIT, newref = STRBUF_INIT, logmsg = STRBUF_INIT;
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 5a8a48287c..7abd938e15 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -239,6 +239,21 @@ test_expect_success 'git branch -M baz bam should succeed when baz is checked ou
 	git worktree prune
 '
 
+test_expect_success 'git branch -M fails if updating any linked working tree fails' '
+	git worktree add -b baz bazdir1 &&
+	git worktree add -f bazdir2 baz &&
+	touch .git/worktrees/bazdir1/HEAD.lock &&
+	test_must_fail git branch -M baz bam &&
+	test $(git -C bazdir2 rev-parse --abbrev-ref HEAD) = bam &&
+	git branch -M bam baz &&
+	rm .git/worktrees/bazdir1/HEAD.lock &&
+	touch .git/worktrees/bazdir2/HEAD.lock &&
+	test_must_fail git branch -M baz bam &&
+	test $(git -C bazdir1 rev-parse --abbrev-ref HEAD) = bam &&
+	rm -rf bazdir1 bazdir2 &&
+	git worktree prune
+'
+
 test_expect_success 'git branch -M baz bam should succeed within a worktree in which baz is checked out' '
 	git checkout -b baz &&
 	git worktree add -f bazdir baz &&
-- 
2.39.2

[PATCH v5 2/5] branch: use get_worktrees() in copy_or_rename_branch()

[Rubén Justo]

Obtaining the list of worktrees, using get_worktrees(), is not a
lightweight operation, because it involves reading from disk.

Let's stop calling get_worktrees() in reject_rebase_or_bisect_branch()
and in replace_each_worktree_head_symref().  Make them receive the list
of worktrees from their only caller, copy_or_rename_branch().

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

diff --git a/builtin/branch.c b/builtin/branch.c
index c7ace2f2da..bac67c27d5 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -486,9 +486,9 @@ static void print_current_branch_name(void)
 		die(_("HEAD (%s) points outside of refs/heads/"), refname);
 }
 
-static void reject_rebase_or_bisect_branch(const char *target)
+static void reject_rebase_or_bisect_branch(struct worktree **worktrees,
+					   const char *target)
 {
-	struct worktree **worktrees = get_worktrees();
 	int i;
 
 	for (i = 0; worktrees[i]; i++) {
@@ -505,8 +505,6 @@ static void reject_rebase_or_bisect_branch(const char *target)
 			die(_("Branch %s is being bisected at %s"),
 			    target, wt->path);
 	}
-
-	free_worktrees(worktrees);
 }
 
 /*
@@ -514,11 +512,11 @@ static void reject_rebase_or_bisect_branch(const char *target)
  * This will be used when renaming a branch. Returns 0 if successful, non-zero
  * otherwise.
  */
-static int replace_each_worktree_head_symref(const char *oldref, const char *newref,
+static int replace_each_worktree_head_symref(struct worktree **worktrees,
+					     const char *oldref, const char *newref,
 					     const char *logmsg)
 {
 	int ret = 0;
-	struct worktree **worktrees = get_worktrees();
 	int i;
 
 	for (i = 0; worktrees[i]; i++) {
@@ -537,7 +535,6 @@ static int replace_each_worktree_head_symref(const char *oldref, const char *new
 				    worktrees[i]->path);
 	}
 
-	free_worktrees(worktrees);
 	return ret;
 }
 
@@ -548,6 +545,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
 	const char *interpreted_oldname = NULL;
 	const char *interpreted_newname = NULL;
 	int recovery = 0;
+	struct worktree **worktrees = get_worktrees();
 
 	if (strbuf_check_branch_ref(&oldref, oldname)) {
 		/*
@@ -576,7 +574,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
 	else
 		validate_new_branchname(newname, &newref, force);
 
-	reject_rebase_or_bisect_branch(oldref.buf);
+	reject_rebase_or_bisect_branch(worktrees, oldref.buf);
 
 	if (!skip_prefix(oldref.buf, "refs/heads/", &interpreted_oldname) ||
 	    !skip_prefix(newref.buf, "refs/heads/", &interpreted_newname)) {
@@ -607,7 +605,8 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
 	}
 
 	if (!copy &&
-	    replace_each_worktree_head_symref(oldref.buf, newref.buf, logmsg.buf))
+	    replace_each_worktree_head_symref(worktrees, oldref.buf, newref.buf,
+					      logmsg.buf))
 		die(_("Branch renamed to %s, but HEAD is not updated!"), newname);
 
 	strbuf_release(&logmsg);
@@ -622,6 +621,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
 	strbuf_release(&newref);
 	strbuf_release(&oldsection);
 	strbuf_release(&newsection);
+	free_worktrees(worktrees);
 }
 
 static GIT_PATH_FUNC(edit_description, "EDIT_DESCRIPTION")
-- 
2.39.2

[PATCH v5 3/5] branch: description for orphan branch errors

[Rubén Justo]

In bcfc82bd48 (branch: description for non-existent branch errors,
2022-10-08) we checked the HEAD in the current worktree to detect if the
branch to operate with is an orphan branch, so as to avoid the confusing
error: "No branch named...".

If we are asked to operate with an orphan branch in a different working
tree than the current one, we need to check the HEAD in that different
working tree.

Let's extend the check we did in bcfc82bd48, to check the HEADs in all
worktrees linked to the current repository, using the helper introduced
in 31ad6b61bd (branch: add branch_checked_out() helper, 2022-06-15).

The helper, branch_checked_out(), does its work obtaining internally a
list of worktrees linked to the current repository.  Obtaining that list
is not a lightweight work because it implies disk access.

In copy_or_rename_branch() we already have a list of worktrees.  Let's
use that already obtained list, and avoid using here the helper.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 builtin/branch.c       | 21 ++++++++++++++++-----
 t/t3202-show-branch.sh | 18 ++++++++++++++++++
 2 files changed, 34 insertions(+), 5 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index bac67c27d5..90dcbb0c6e 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -538,13 +538,15 @@ static int replace_each_worktree_head_symref(struct worktree **worktrees,
 	return ret;
 }
 
+#define IS_HEAD 1
+
 static void copy_or_rename_branch(const char *oldname, const char *newname, int copy, int force)
 {
 	struct strbuf oldref = STRBUF_INIT, newref = STRBUF_INIT, logmsg = STRBUF_INIT;
 	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_usage = 0;
 	struct worktree **worktrees = get_worktrees();
 
 	if (strbuf_check_branch_ref(&oldref, oldname)) {
@@ -558,8 +560,17 @@ 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))
+	for (int i = 0; worktrees[i]; i++) {
+		struct worktree *wt = worktrees[i];
+
+		if (wt->head_ref && !strcmp(oldref.buf, wt->head_ref)) {
+			oldref_usage |= IS_HEAD;
+			break;
+		}
+	}
+
+	if ((copy || !(oldref_usage & IS_HEAD)) && !ref_exists(oldref.buf)) {
+		if (oldref_usage & IS_HEAD)
 			die(_("No commit on branch '%s' yet."), oldname);
 		else
 			die(_("No branch named '%s'."), oldname);
@@ -838,7 +849,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 
 		strbuf_addf(&branch_ref, "refs/heads/%s", branch_name);
 		if (!ref_exists(branch_ref.buf))
-			error((!argc || !strcmp(head, branch_name))
+			error((!argc || branch_checked_out(branch_ref.buf))
 			      ? _("No commit on branch '%s' yet.")
 			      : _("No branch named '%s'."),
 			      branch_name);
@@ -883,7 +894,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		}
 
 		if (!ref_exists(branch->refname)) {
-			if (!argc || !strcmp(head, branch->name))
+			if (!argc || branch_checked_out(branch->refname))
 				die(_("No commit on branch '%s' yet."), branch->name);
 			die(_("branch '%s' does not exist"), branch->name);
 		}
diff --git a/t/t3202-show-branch.sh b/t/t3202-show-branch.sh
index ea7cfd1951..be20ebe1d5 100755
--- a/t/t3202-show-branch.sh
+++ b/t/t3202-show-branch.sh
@@ -221,4 +221,22 @@ test_expect_success 'fatal descriptions on non-existent branch' '
 	test_cmp expect actual
 '
 
+test_expect_success 'error descriptions on orphan branch' '
+	test_when_finished git worktree remove -f wt &&
+	git worktree add wt --detach &&
+	git -C wt checkout --orphan orphan-branch &&
+	test_branch_op_in_wt() {
+		test_orphan_error() {
+			test_must_fail git $* 2>actual &&
+			test_i18ngrep "No commit on branch .orphan-branch. yet.$" actual
+		} &&
+		test_orphan_error -C wt branch $1 $2 &&                # implicit branch
+		test_orphan_error -C wt branch $1 orphan-branch $2 &&  # explicit branch
+		test_orphan_error branch $1 orphan-branch $2           # different worktree
+	} &&
+	test_branch_op_in_wt --edit-description &&
+	test_branch_op_in_wt --set-upstream-to=ne &&
+	test_branch_op_in_wt -c new-branch
+'
+
 test_done
-- 
2.39.2

[PATCH v5 4/5] branch: rename orphan branches in any worktree

[Rubén Justo]

In cfaff3aac (branch -m: allow renaming a yet-unborn branch, 2020-12-13)
we added support for renaming an orphan branch when that branch is
checked out in the current worktree.

Let's also allow renaming an orphan branch checked out in a worktree
different than the current one.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 builtin/branch.c  |  6 ++++--
 t/t3200-branch.sh | 14 ++++++++++++++
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 90dcbb0c6e..a93b9fc0ab 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -539,6 +539,7 @@ static int replace_each_worktree_head_symref(struct worktree **worktrees,
 }
 
 #define IS_HEAD 1
+#define IS_ORPHAN 2
 
 static void copy_or_rename_branch(const char *oldname, const char *newname, int copy, int force)
 {
@@ -565,6 +566,8 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
 
 		if (wt->head_ref && !strcmp(oldref.buf, wt->head_ref)) {
 			oldref_usage |= IS_HEAD;
+			if (is_null_oid(&wt->head_oid))
+				oldref_usage |= IS_ORPHAN;
 			break;
 		}
 	}
@@ -599,8 +602,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 &&
-	    (!head || strcmp(oldname, head) || !is_null_oid(&head_oid)) &&
+	if (!copy && !(oldref_usage & IS_ORPHAN) &&
 	    rename_ref(oldref.buf, newref.buf, logmsg.buf))
 		die(_("Branch rename failed"));
 	if (copy && copy_existing_ref(oldref.buf, newref.buf, logmsg.buf))
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 7abd938e15..98b6c8ac34 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -298,6 +298,20 @@ test_expect_success 'git branch -M and -C fail on detached HEAD' '
 	test_cmp expect err
 '
 
+test_expect_success 'git branch -m should work with orphan branches' '
+	test_when_finished git checkout - &&
+	test_when_finished git worktree remove -f wt &&
+	git worktree add wt --detach &&
+	# rename orphan in another worktreee
+	git -C wt checkout --orphan orphan-foo-wt &&
+	git branch -m orphan-foo-wt orphan-bar-wt &&
+	test orphan-bar-wt=$(git -C orphan-worktree branch --show-current) &&
+	# rename orphan in the current worktree
+	git checkout --orphan orphan-foo &&
+	git branch -m orphan-foo orphan-bar &&
+	test orphan-bar=$(git branch --show-current)
+'
+
 test_expect_success 'git branch -d on orphan HEAD (merged)' '
 	test_when_finished git checkout main &&
 	git checkout --orphan orphan &&
-- 
2.39.2

[PATCH v5 5/5] branch: avoid unnecessary worktrees traversals

[Rubén Justo]

When we rename a branch ref, we need to update any worktree that have
its HEAD pointing to the branch ref being renamed, so to make it use the
new ref name.

If we know in advance that we're renaming a branch that is not currently
checked out in any worktree, we can skip this step entirely.  Let's do
it so.

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

diff --git a/builtin/branch.c b/builtin/branch.c
index a93b9fc0ab..6a564b22b0 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -617,7 +617,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
 				interpreted_oldname);
 	}
 
-	if (!copy &&
+	if (!copy && (oldref_usage & IS_HEAD) &&
 	    replace_each_worktree_head_symref(worktrees, oldref.buf, newref.buf,
 					      logmsg.buf))
 		die(_("Branch renamed to %s, but HEAD is not updated!"), newname);
-- 
2.39.2

Re: [PATCH v5 0/5] branch: operations on orphan branches

[Junio C Hamano]

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

> In this iteration, v5, I've reverted some of those major refactorings,
> mainly the inlining, because it ended up introducing unnecessary
> complexity for minimal benefit in this series.  Maybe those refactorings
> make more sense in future series.

The previous one has been cooking in 'next' already for some time
(even before the feature freeze for 2.40), so we'd usually do not
take such a rewrite and rather go with incremental refinements, but
let's discard it out of 'next' and queue this intereation instead.

We're probably overdue to rewind and rebuild 'next' this round
anyway.

Thanks for an updated version.  Will queue.

Re: [PATCH v5 0/5] branch: operations on orphan branches

[Junio C Hamano]

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

> The initial and main intention in this series is to avoid some confusing
> errors operating with orphan branches, when working with worktrees.
> ...
> In this iteration, v5, I've reverted some of those major refactorings,
> mainly the inlining, because it ended up introducing unnecessary
> complexity for minimal benefit in this series.  Maybe those refactorings
> make more sense in future series.
>
> A new test has been introduced, in 1/5, to notice if a behavior change
> similar to that observed in v4, is reintroduced in the future.
>
> Other than that, no functional changes are expected from v2.

This has not seen any activities since it was posted; presumably the
issues raised during the review of the previous round have all been
addressed?

Is everybody happy to see us declare victory and merge it down to
'next'?  I see everybody who commented on earlier iterations of the
series are CC'ed, so let's hear from them (and others who may be
interested).

Thanks.