Re: [PATCH v3 1/3] branch: avoid unnecessary worktrees traversals
- From
Rubén Justo <rjusto@gmail.com>
- Date
- Feb 15, 2023, 22:00 UTC
- Message-ID
- <b9464970-1ba5-e2f3-58c6-145ca45b1095@gmail.com>
- In-Reply-To
- <20230211041644.1848341-1-jonathantanmy@google.com>
On 10-feb-2023 20:16:44, Jonathan Tan wrote:
Show 11 quoted lines
> > 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.
Show 15 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
OK. I'll use names.
Show 15 quoted lines
> 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!