| Message ID | 50dd7c7a-5656-e010-1c0b-819a40a1f1a0@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | c8fb01f6f726d362f263e9277c16cb646f2409f3 |
| Headers | show |
| Series | branch: operations on orphan branches | expand |
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.
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.
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.
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);
"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(-)