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