Re: [PATCH v7 4/8] worktree: simplify find_shared_symref() memory ownership model

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Dec 2, 2021, 21:00 UTC
Message-ID: <CAPig+cS9n91nstp0zK-JywK3WHOVZ38+Ok850=aajx+hL4T2ZQ@mail.gmail.com>
In-Reply-To: <alpine.DEB.2.21.999.2112020357510.14727@scrubbing-bubbles.mit.edu>

On Thu, Dec 2, 2021 at 4:07 AM Anders Kaseorg <andersk@mit.edu> wrote:

Show 14 quoted lines

> On Wed, 1 Dec 2021, Eric Sunshine wrote:
> > If we instead hoist ownership of `worktrees` up to execute_commands()
> > -- which calls execute_commands_atomic() or
> > execute_commands_non_atomic() -- then we can get by with retrieving
> > the worktrees just once, and all those noise changes in update() can
> > be dropped since it will no longer be responsible for allocating or
> > freeing `worktrees`.
>
> Seems reasonable to me.  It’s a smaller textual change, potentially a
> larger conceptual change, but the efficiency improvement is probably
> worthwhile.
>
> That would modify patch 4 to the below.  Patches 5 through 8 cleanly
> rebase past this modification.

Yes, this version looks good. Thanks for accommodating my suggestion. Not only does it make for a less noisy diff (which is pleasing), but perhaps more importantly, it reduces cognitive load a small amount (at least for me[1]).

> Subject: [PATCH v7½ 4/8] worktree: simplify find_shared_symref() memory ownership model

I suspect that Junio will find it easier to take a full re-roll than dealing with a single patch replacement since he doesn't necessarily follow these discussions closely. It's more likely that he'll pick up this change if you post v8 in its entirety.

[1]: Although we regularly use `goto` for this sort of resource cleanup, most such functions are much shorter than the one here; somehow the length of this function made the new `goto`s a tiny bit less easy to grasp-at-a-glance. Purely subjective, I know.