Rubén Justo <rjusto@xxxxxxxxx> 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.
