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

On Wed, Dec 1, 2021 at 6:48 PM Anders Kaseorg [off-list ref] wrote:

On 12/1/21 15:10, Eric Sunshine wrote:

quoted

As far as I can see, this code only cares whether find_shared_symref()
returned a result; it doesn't actually consult the returned worktree
at all, thus it semantically considers `worktree` as a boolean, not as
a `struct worktree`.

No, the update_worktree(new_oid->hash, worktree) call uses it in a
non-boolean way, so we do need to keep it around.

Okay, I missed that call to update_worktree() in the "noise" of all
those other changes, but looking at the code a bit more carefully, it
seems as if we might still be able to drop all the noise changes
anyhow by hoisting ownership of `worktrees` up a level or two. In
particular, both execute_commands_atomic() and
execute_commands_non_atomic() are calling update() in a loop, and
update() is calling get_worktrees() / free_worktrees() each time it is
called within those loops, which seems unnecessarily expensive.

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`. For instance:

    static void execute_commands(...)
    {
        struct worktree **worktrees;
        ...
        worktrees = get_worktrees();
         if (use_atomic)
            execute_commands_atomic(commands, si, worktrees);
        else
            execute_commands_non_atomic(commands, si, worktrees);
        free_worktrees(worktrees);
        ...
    }

and then execute_commands_atomic() and execute_commands_non_atomic()
can pass `worktrees` along to update().