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

On Thu, Dec 2, 2021 at 4:07 AM Anders Kaseorg [off-list ref] wrote:

On Wed, 1 Dec 2021, Eric Sunshine wrote:

quoted

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.