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

From: Rubén Justo <rjusto@gmail.com>
Date: Feb 28, 2023, 00:11 UTC
Message-ID: <dee6ca9e-af3b-bc79-a09c-6bc0f0245017@gmail.com>
In-Reply-To: <20230227193012.2410973-1-jonathantanmy@google.com>

On 27/2/23 20:30, Jonathan Tan wrote:

Show 8 quoted lines

> Rubén Justo <rjusto@gmail.com> 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.

Show 8 quoted lines

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