On Thu, Dec 2, 2021 at 4:07 AM Anders Kaseorg <andersk@xxxxxxx> wrote: > 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.
