* [PATCH v3 1/3] branch: avoid unnecessary worktrees traversals
2023-02-06 23:01 ` [PATCH v3 0/3] branch: operations on orphan branches Rubén Justo
@ 2023-02-06 23:06 ` Rubén Justo
2023-02-11 4:16 ` Jonathan Tan
2023-02-06 23:06 ` [PATCH v3 2/3] branch: description for orphan branch errors Rubén Justo
` (4 subsequent siblings)
5 siblings, 1 reply; 49+ messages in thread
From: Rubén Justo @ 2023-02-06 23:06 UTC (permalink / raw)
To: Git List; +Cc: Junio C Hamano
"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
^ permalink raw reply related [flat|nested] 49+ messages in thread* Re: [PATCH v3 1/3] branch: avoid unnecessary worktrees traversals
2023-02-06 23:06 ` [PATCH v3 1/3] branch: avoid unnecessary worktrees traversals Rubén Justo
@ 2023-02-11 4:16 ` Jonathan Tan
2023-02-15 22:00 ` Rubén Justo
0 siblings, 1 reply; 49+ messages in thread
From: Jonathan Tan @ 2023-02-11 4:16 UTC (permalink / raw)
To: Rubén Justo; +Cc: Jonathan Tan, Git List, Junio C Hamano
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.
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [PATCH v3 1/3] branch: avoid unnecessary worktrees traversals
2023-02-11 4:16 ` Jonathan Tan
@ 2023-02-15 22:00 ` Rubén Justo
0 siblings, 0 replies; 49+ messages in thread
From: Rubén Justo @ 2023-02-15 22:00 UTC (permalink / raw)
To: Jonathan Tan; +Cc: Git List, Junio C Hamano
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!
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v3 2/3] branch: description for orphan branch errors
2023-02-06 23:01 ` [PATCH v3 0/3] branch: operations on orphan branches Rubén Justo
2023-02-06 23:06 ` [PATCH v3 1/3] branch: avoid unnecessary worktrees traversals Rubén Justo
@ 2023-02-06 23:06 ` Rubén Justo
2023-02-06 23:06 ` [PATCH v3 3/3] branch: rename orphan branches in any worktree Rubén Justo
` (3 subsequent siblings)
5 siblings, 0 replies; 49+ messages in thread
From: Rubén Justo @ 2023-02-06 23:06 UTC (permalink / raw)
To: Git List; +Cc: Junio C Hamano
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
^ permalink raw reply related [flat|nested] 49+ messages in thread* [PATCH v3 3/3] branch: rename orphan branches in any worktree
2023-02-06 23:01 ` [PATCH v3 0/3] branch: operations on orphan branches Rubén Justo
2023-02-06 23:06 ` [PATCH v3 1/3] branch: avoid unnecessary worktrees traversals Rubén Justo
2023-02-06 23:06 ` [PATCH v3 2/3] branch: description for orphan branch errors Rubén Justo
@ 2023-02-06 23:06 ` Rubén Justo
2023-02-07 0:11 ` [PATCH v3 0/3] branch: operations on orphan branches Junio C Hamano
` (2 subsequent siblings)
5 siblings, 0 replies; 49+ messages in thread
From: Rubén Justo @ 2023-02-06 23:06 UTC (permalink / raw)
To: Git List; +Cc: Junio C Hamano
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
^ permalink raw reply related [flat|nested] 49+ messages in thread* Re: [PATCH v3 0/3] branch: operations on orphan branches
2023-02-06 23:01 ` [PATCH v3 0/3] branch: operations on orphan branches Rubén Justo
` (2 preceding siblings ...)
2023-02-06 23:06 ` [PATCH v3 3/3] branch: rename orphan branches in any worktree Rubén Justo
@ 2023-02-07 0:11 ` Junio C Hamano
2023-02-07 8:33 ` Ævar Arnfjörð Bjarmason
2023-02-22 22:50 ` [PATCH v4 " Rubén Justo
5 siblings, 0 replies; 49+ messages in thread
From: Junio C Hamano @ 2023-02-07 0:11 UTC (permalink / raw)
To: Rubén Justo; +Cc: Git List
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.
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [PATCH v3 0/3] branch: operations on orphan branches
2023-02-06 23:01 ` [PATCH v3 0/3] branch: operations on orphan branches Rubén Justo
` (3 preceding siblings ...)
2023-02-07 0:11 ` [PATCH v3 0/3] branch: operations on orphan branches Junio C Hamano
@ 2023-02-07 8:33 ` Ævar Arnfjörð Bjarmason
2023-02-08 0:35 ` Rubén Justo
2023-02-08 18:37 ` Junio C Hamano
2023-02-22 22:50 ` [PATCH v4 " Rubén Justo
5 siblings, 2 replies; 49+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2023-02-07 8:33 UTC (permalink / raw)
To: Rubén Justo; +Cc: Git List, Junio C Hamano
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;
^ permalink raw reply related [flat|nested] 49+ messages in thread* Re: [PATCH v3 0/3] branch: operations on orphan branches
2023-02-07 8:33 ` Ævar Arnfjörð Bjarmason
@ 2023-02-08 0:35 ` Rubén Justo
2023-02-08 18:37 ` Junio C Hamano
1 sibling, 0 replies; 49+ messages in thread
From: Rubén Justo @ 2023-02-08 0:35 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: Git List, Junio C Hamano
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;
>
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [PATCH v3 0/3] branch: operations on orphan branches
2023-02-07 8:33 ` Ævar Arnfjörð Bjarmason
2023-02-08 0:35 ` Rubén Justo
@ 2023-02-08 18:37 ` Junio C Hamano
1 sibling, 0 replies; 49+ messages in thread
From: Junio C Hamano @ 2023-02-08 18:37 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: Rubén Justo, Git List
Æ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.
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v4 0/3] branch: operations on orphan branches
2023-02-06 23:01 ` [PATCH v3 0/3] branch: operations on orphan branches Rubén Justo
` (4 preceding siblings ...)
2023-02-07 8:33 ` Ævar Arnfjörð Bjarmason
@ 2023-02-22 22:50 ` Rubén Justo
2023-02-22 22:52 ` [PATCH v4 1/3] branch: avoid unnecessary worktrees traversals Rubén Justo
` (3 more replies)
5 siblings, 4 replies; 49+ messages in thread
From: Rubén Justo @ 2023-02-22 22:50 UTC (permalink / raw)
To: Git List
Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason,
Jonathan Tan
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
^ permalink raw reply [flat|nested] 49+ messages in thread* [PATCH v4 1/3] branch: avoid unnecessary worktrees traversals
2023-02-22 22:50 ` [PATCH v4 " Rubén Justo
@ 2023-02-22 22:52 ` Rubén Justo
2023-02-25 15:08 ` Rubén Justo
2023-02-22 22:55 ` [PATCH v4 2/3] branch: description for orphan branch errors Rubén Justo
` (2 subsequent siblings)
3 siblings, 1 reply; 49+ messages in thread
From: Rubén Justo @ 2023-02-22 22:52 UTC (permalink / raw)
To: Git List
Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason,
Jonathan Tan
"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
^ permalink raw reply related [flat|nested] 49+ messages in thread* Re: [PATCH v4 1/3] branch: avoid unnecessary worktrees traversals
2023-02-22 22:52 ` [PATCH v4 1/3] branch: avoid unnecessary worktrees traversals Rubén Justo
@ 2023-02-25 15:08 ` Rubén Justo
2023-02-27 19:30 ` Jonathan Tan
0 siblings, 1 reply; 49+ messages in thread
From: Rubén Justo @ 2023-02-25 15:08 UTC (permalink / raw)
To: Git List
Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason,
Jonathan Tan
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.
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [PATCH v4 1/3] branch: avoid unnecessary worktrees traversals
2023-02-25 15:08 ` Rubén Justo
@ 2023-02-27 19:30 ` Jonathan Tan
2023-02-28 0:11 ` Rubén Justo
0 siblings, 1 reply; 49+ messages in thread
From: Jonathan Tan @ 2023-02-27 19:30 UTC (permalink / raw)
To: Rubén Justo
Cc: Jonathan Tan, Git List, Junio C Hamano,
Ævar Arnfjörð Bjarmason
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.
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH v4 1/3] branch: avoid unnecessary worktrees traversals
2023-02-27 19:30 ` Jonathan Tan
@ 2023-02-28 0:11 ` Rubén Justo
0 siblings, 0 replies; 49+ messages in thread
From: Rubén Justo @ 2023-02-28 0:11 UTC (permalink / raw)
To: Jonathan Tan
Cc: Git List, Junio C Hamano, Ævar Arnfjörð Bjarmason
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.
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v4 2/3] branch: description for orphan branch errors
2023-02-22 22:50 ` [PATCH v4 " Rubén Justo
2023-02-22 22:52 ` [PATCH v4 1/3] branch: avoid unnecessary worktrees traversals Rubén Justo
@ 2023-02-22 22:55 ` Rubén Justo
2023-02-27 19:38 ` Jonathan Tan
2023-02-22 22:56 ` [PATCH v4 3/3] branch: rename orphan branches in any worktree Rubén Justo
2023-03-26 22:19 ` [PATCH v5 0/5] branch: operations on orphan branches Rubén Justo
3 siblings, 1 reply; 49+ messages in thread
From: Rubén Justo @ 2023-02-22 22:55 UTC (permalink / raw)
To: Git List
Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason,
Jonathan Tan
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
^ permalink raw reply related [flat|nested] 49+ messages in thread* Re: [PATCH v4 2/3] branch: description for orphan branch errors
2023-02-22 22:55 ` [PATCH v4 2/3] branch: description for orphan branch errors Rubén Justo
@ 2023-02-27 19:38 ` Jonathan Tan
2023-02-27 21:56 ` Junio C Hamano
2023-02-28 0:22 ` Rubén Justo
0 siblings, 2 replies; 49+ messages in thread
From: Jonathan Tan @ 2023-02-27 19:38 UTC (permalink / raw)
To: Rubén Justo
Cc: Jonathan Tan, Git List, Junio C Hamano,
Ævar Arnfjörð Bjarmason
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?
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [PATCH v4 2/3] branch: description for orphan branch errors
2023-02-27 19:38 ` Jonathan Tan
@ 2023-02-27 21:56 ` Junio C Hamano
2023-02-28 0:22 ` Rubén Justo
1 sibling, 0 replies; 49+ messages in thread
From: Junio C Hamano @ 2023-02-27 21:56 UTC (permalink / raw)
To: Jonathan Tan
Cc: Rubén Justo, Git List,
Ævar Arnfjörð Bjarmason
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.
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [PATCH v4 2/3] branch: description for orphan branch errors
2023-02-27 19:38 ` Jonathan Tan
2023-02-27 21:56 ` Junio C Hamano
@ 2023-02-28 0:22 ` Rubén Justo
1 sibling, 0 replies; 49+ messages in thread
From: Rubén Justo @ 2023-02-28 0:22 UTC (permalink / raw)
To: Jonathan Tan
Cc: Git List, Junio C Hamano, Ævar Arnfjörð Bjarmason
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...
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v4 3/3] branch: rename orphan branches in any worktree
2023-02-22 22:50 ` [PATCH v4 " Rubén Justo
2023-02-22 22:52 ` [PATCH v4 1/3] branch: avoid unnecessary worktrees traversals Rubén Justo
2023-02-22 22:55 ` [PATCH v4 2/3] branch: description for orphan branch errors Rubén Justo
@ 2023-02-22 22:56 ` Rubén Justo
2023-02-27 19:41 ` Jonathan Tan
2023-03-26 22:19 ` [PATCH v5 0/5] branch: operations on orphan branches Rubén Justo
3 siblings, 1 reply; 49+ messages in thread
From: Rubén Justo @ 2023-02-22 22:56 UTC (permalink / raw)
To: Git List
Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason,
Jonathan Tan
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
^ permalink raw reply related [flat|nested] 49+ messages in thread* Re: [PATCH v4 3/3] branch: rename orphan branches in any worktree
2023-02-22 22:56 ` [PATCH v4 3/3] branch: rename orphan branches in any worktree Rubén Justo
@ 2023-02-27 19:41 ` Jonathan Tan
2023-02-28 0:23 ` Rubén Justo
0 siblings, 1 reply; 49+ messages in thread
From: Jonathan Tan @ 2023-02-27 19:41 UTC (permalink / raw)
To: Rubén Justo
Cc: Jonathan Tan, Git List, Junio C Hamano,
Ævar Arnfjörð Bjarmason
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?
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [PATCH v4 3/3] branch: rename orphan branches in any worktree
2023-02-27 19:41 ` Jonathan Tan
@ 2023-02-28 0:23 ` Rubén Justo
0 siblings, 0 replies; 49+ messages in thread
From: Rubén Justo @ 2023-02-28 0:23 UTC (permalink / raw)
To: Jonathan Tan
Cc: Git List, Junio C Hamano, Ævar Arnfjörð Bjarmason
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?
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v5 0/5] branch: operations on orphan branches
2023-02-22 22:50 ` [PATCH v4 " Rubén Justo
` (2 preceding siblings ...)
2023-02-22 22:56 ` [PATCH v4 3/3] branch: rename orphan branches in any worktree Rubén Justo
@ 2023-03-26 22:19 ` Rubén Justo
2023-03-26 22:33 ` [PATCH v5 1/5] branch: test for failures while renaming branches Rubén Justo
` (6 more replies)
3 siblings, 7 replies; 49+ messages in thread
From: Rubén Justo @ 2023-03-26 22:19 UTC (permalink / raw)
To: Git List
Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason,
Jonathan Tan
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
^ permalink raw reply [flat|nested] 49+ messages in thread* [PATCH v5 1/5] branch: test for failures while renaming branches
2023-03-26 22:19 ` [PATCH v5 0/5] branch: operations on orphan branches Rubén Justo
@ 2023-03-26 22:33 ` Rubén Justo
2023-03-26 22:33 ` [PATCH v5 2/5] branch: use get_worktrees() in copy_or_rename_branch() Rubén Justo
` (5 subsequent siblings)
6 siblings, 0 replies; 49+ messages in thread
From: Rubén Justo @ 2023-03-26 22:33 UTC (permalink / raw)
To: Git List
Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason,
Jonathan Tan
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
^ permalink raw reply related [flat|nested] 49+ messages in thread* [PATCH v5 2/5] branch: use get_worktrees() in copy_or_rename_branch()
2023-03-26 22:19 ` [PATCH v5 0/5] branch: operations on orphan branches Rubén Justo
2023-03-26 22:33 ` [PATCH v5 1/5] branch: test for failures while renaming branches Rubén Justo
@ 2023-03-26 22:33 ` Rubén Justo
2023-03-26 22:33 ` [PATCH v5 3/5] branch: description for orphan branch errors Rubén Justo
` (4 subsequent siblings)
6 siblings, 0 replies; 49+ messages in thread
From: Rubén Justo @ 2023-03-26 22:33 UTC (permalink / raw)
To: Git List
Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason,
Jonathan Tan
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
^ permalink raw reply related [flat|nested] 49+ messages in thread* [PATCH v5 3/5] branch: description for orphan branch errors
2023-03-26 22:19 ` [PATCH v5 0/5] branch: operations on orphan branches Rubén Justo
2023-03-26 22:33 ` [PATCH v5 1/5] branch: test for failures while renaming branches Rubén Justo
2023-03-26 22:33 ` [PATCH v5 2/5] branch: use get_worktrees() in copy_or_rename_branch() Rubén Justo
@ 2023-03-26 22:33 ` Rubén Justo
2023-03-26 22:33 ` [PATCH v5 4/5] branch: rename orphan branches in any worktree Rubén Justo
` (3 subsequent siblings)
6 siblings, 0 replies; 49+ messages in thread
From: Rubén Justo @ 2023-03-26 22:33 UTC (permalink / raw)
To: Git List
Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason,
Jonathan Tan
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
^ permalink raw reply related [flat|nested] 49+ messages in thread* [PATCH v5 4/5] branch: rename orphan branches in any worktree
2023-03-26 22:19 ` [PATCH v5 0/5] branch: operations on orphan branches Rubén Justo
` (2 preceding siblings ...)
2023-03-26 22:33 ` [PATCH v5 3/5] branch: description for orphan branch errors Rubén Justo
@ 2023-03-26 22:33 ` Rubén Justo
2023-03-26 22:33 ` [PATCH v5 5/5] branch: avoid unnecessary worktrees traversals Rubén Justo
` (2 subsequent siblings)
6 siblings, 0 replies; 49+ messages in thread
From: Rubén Justo @ 2023-03-26 22:33 UTC (permalink / raw)
To: Git List
Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason,
Jonathan Tan
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
^ permalink raw reply related [flat|nested] 49+ messages in thread* [PATCH v5 5/5] branch: avoid unnecessary worktrees traversals
2023-03-26 22:19 ` [PATCH v5 0/5] branch: operations on orphan branches Rubén Justo
` (3 preceding siblings ...)
2023-03-26 22:33 ` [PATCH v5 4/5] branch: rename orphan branches in any worktree Rubén Justo
@ 2023-03-26 22:33 ` Rubén Justo
2023-03-27 19:49 ` [PATCH v5 0/5] branch: operations on orphan branches Junio C Hamano
2023-05-01 22:19 ` Junio C Hamano
6 siblings, 0 replies; 49+ messages in thread
From: Rubén Justo @ 2023-03-26 22:33 UTC (permalink / raw)
To: Git List
Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason,
Jonathan Tan
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
^ permalink raw reply related [flat|nested] 49+ messages in thread* Re: [PATCH v5 0/5] branch: operations on orphan branches
2023-03-26 22:19 ` [PATCH v5 0/5] branch: operations on orphan branches Rubén Justo
` (4 preceding siblings ...)
2023-03-26 22:33 ` [PATCH v5 5/5] branch: avoid unnecessary worktrees traversals Rubén Justo
@ 2023-03-27 19:49 ` Junio C Hamano
2023-05-01 22:19 ` Junio C Hamano
6 siblings, 0 replies; 49+ messages in thread
From: Junio C Hamano @ 2023-03-27 19:49 UTC (permalink / raw)
To: Rubén Justo
Cc: Git List, Ævar Arnfjörð Bjarmason, Jonathan Tan
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.
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [PATCH v5 0/5] branch: operations on orphan branches
2023-03-26 22:19 ` [PATCH v5 0/5] branch: operations on orphan branches Rubén Justo
` (5 preceding siblings ...)
2023-03-27 19:49 ` [PATCH v5 0/5] branch: operations on orphan branches Junio C Hamano
@ 2023-05-01 22:19 ` Junio C Hamano
6 siblings, 0 replies; 49+ messages in thread
From: Junio C Hamano @ 2023-05-01 22:19 UTC (permalink / raw)
To: Rubén Justo
Cc: Git List, Ævar Arnfjörð Bjarmason, Jonathan Tan
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.
^ permalink raw reply [flat|nested] 49+ messages in thread