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!
