From: Rubén Justo Date: Tue, 28 Feb 2023 00:11:51 GMT Subject: Re: [PATCH v4 1/3] branch: avoid unnecessary worktrees traversals Message-ID: In-Reply-To: <20230227193012.2410973-1-jonathantanmy@google.com> On 27/2/23 20:30, Jonathan Tan wrote: > Rubén Justo writes: >> Reviewing this, I noticed I made a mistake here. The original code >> doesn't stop iterating whenever refs_create_symref() fails; it continues >> trying to update the remaining worktrees. When the iteration ends, if >> any of the updates failed, then die(). Also, the error message "HEAD of >> working tree %s is not updated" is lost. > > Ah yes, I noticed this too. I'll re-roll with the fix in the next iteration. Thank you for reading carefully. > > Besides that, a reviewer, upon reading the commit message, might ask: > why not take the worktrees as a parameter then, if we're so worried > about performance? I think that the real reason for inlining is that the > code being inlined needs to communicate more information to its calling > function in subsequent patches; the performance improvement is only a > beneficial side effect. > Certainly, that's a way to see the modification within this series. Actually, this modification wasn't present in v1, but once it was introduced in v2, it made sense on its own: to eliminate the redundant traversals of worktrees when renaming a branch, even if the branch isn't HEAD in any worktree. Therefore, I think the change can also be seen in another way: the increased ease in the next modifications is a beneficial side effect of this patch.