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

From: Jonathan Tan <jonathantanmy@google.com>
Date: Feb 11, 2023, 04:16 UTC
Message-ID: <20230211041644.1848341-1-jonathantanmy@google.com>
In-Reply-To: <f284e163-5476-0c38-106c-094080340f71@gmail.com>

Rubén Justo <rjusto@gmail.com> writes:

Show 9 quoted lines

> "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).

Show 5 quoted lines

> 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.