Re: [PATCH v3 1/3] branch: avoid unnecessary worktrees traversals

["Rubén Justo" <rjusto@gmail.com>]

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!