| Message ID | bc82dd95-c968-146d-0dea-f82631b74765@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | branch: operations on orphan branches | expand |
Rubén Justo <rjusto@gmail.com> writes: > -static void reject_rebase_or_bisect_branch(const char *target) > +static int ishead_and_reject_rebase_or_bisect_branch(const char *target) The original name was already horrible but it became much worse by acquiring a non-word "ishead" as part of it X-<. At least let's replace "rebase or bisect" with something a bit more generic, extensible, and shorter phrase. For example, isn't the point of having the function was to give us a mechansim to see if the branch with the given name is not to be modified because it is being worked on elsewhere? "The branch is in use" would be a good phrase to express such a concept, so die_if_branch_is_in_use() or something along that line may be easier to grok.
On 19-ene-2023 13:24:45, Junio C Hamano wrote: > Rubén Justo <rjusto@gmail.com> writes: > > > -static void reject_rebase_or_bisect_branch(const char *target) > > +static int ishead_and_reject_rebase_or_bisect_branch(const char *target) > > The original name was already horrible but it became much worse by > acquiring a non-word "ishead" as part of it X-<. > > At least let's replace "rebase or bisect" with something a bit more > generic, extensible, and shorter phrase. For example, isn't the > point of having the function was to give us a mechansim to see if > the branch with the given name is not to be modified because it is > being worked on elsewhere? "The branch is in use" would be a good > phrase to express such a concept, so die_if_branch_is_in_use() or > something along that line may be easier to grok. I agree, the naming is ugly. The idea is to return, if not die(), from the iteration we are doing in that function, whether the branch is checked out in any worktree. That information allows us later, if we know in advance that no HEAD needs to be adjusted, to avoid calling replace_each_worktree_head_symref(), saving us a new and unnecessary traversal of the worktrees. There is a second idea to, in next commits, return also if the branch is an unborn branch. die_if_branch_is_in_use() is a better name for reject_rebase_or_... but don't know how it fits with these ideas. I'm open to suggestions. I'll reroll with a better approach. Thank you.
diff --git a/builtin/branch.c b/builtin/branch.c index f63fd45edb..a1ee728ca3 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -486,14 +486,17 @@ static void print_current_branch_name(void) die(_("HEAD (%s) points outside of refs/heads/"), refname); } -static void reject_rebase_or_bisect_branch(const char *target) +static int ishead_and_reject_rebase_or_bisect_branch(const char *target) { struct worktree **worktrees = get_worktrees(); - int i; + int i, ret = 0; for (i = 0; worktrees[i]; i++) { struct worktree *wt = worktrees[i]; + if (wt->head_ref && !strcmp(target, wt->head_ref)) + ret = 1; + if (!wt->is_detached) continue; @@ -507,6 +510,7 @@ static void reject_rebase_or_bisect_branch(const char *target) } free_worktrees(worktrees); + return ret; } static void copy_or_rename_branch(const char *oldname, const char *newname, int copy, int force) @@ -515,7 +519,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int struct strbuf oldsection = STRBUF_INIT, newsection = STRBUF_INIT; const char *interpreted_oldname = NULL; const char *interpreted_newname = NULL; - int recovery = 0; + int recovery = 0, ishead; if (strbuf_check_branch_ref(&oldref, oldname)) { /* @@ -544,7 +548,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int else validate_new_branchname(newname, &newref, force); - reject_rebase_or_bisect_branch(oldref.buf); + ishead = ishead_and_reject_rebase_or_bisect_branch(oldref.buf); if (!skip_prefix(oldref.buf, "refs/heads/", &interpreted_oldname) || !skip_prefix(newref.buf, "refs/heads/", &interpreted_newname)) { @@ -574,7 +578,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int interpreted_oldname); } - if (!copy && + if (!copy && ishead && replace_each_worktree_head_symref(oldref.buf, newref.buf, logmsg.buf)) die(_("Branch renamed to %s, but HEAD is not updated!"), newname);
"reject_rebase_or_bisect_branch()" was introduced [1] to prevent a branch under bisect or rebase from being renamed or copied. It traverses all worktrees in the repository and dies if the specified branch is being rebased or bisected in any them. "replace_each_worktree_head_symref()" was introduced [2] to adjust the HEAD in all worktrees after a branch rename succeeded. It traverses all worktrees in the repository and if any of them have their HEAD pointing to the renamed ref, it adjusts it. If we could know in advance if the renamed branch is not HEAD in any worktree we could avoid calling "replace_each_worktree_head_symref()". Let's have "reject_rebase_or_bisect_branch()" check and return whether the specified branch is HEAD in any worktree, and use this information to avoid unnecessary calls to "replace_each_worktree_head_symref()". 1.- 14ace5b (branch: do not rename a branch under bisect or rebase, 2016-04-22) 2.- 70999e9 (branch -m: update all per-worktree HEADs, 2016-03-27) Signed-off-by: Rubén Justo <rjusto@gmail.com> --- builtin/branch.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)