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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Dec 2, 2021, 00:13 UTC
Message-ID: <CAPig+cRX94vMLkW=dAKncPTuuk0Opaj+8-Yqe-qFD1b7mrZ89A@mail.gmail.com>
In-Reply-To: <f8262c3b-2ca5-4920-1379-41ff29b37d9f@mit.edu>

On Wed, Dec 1, 2021 at 6:48 PM Anders Kaseorg <andersk@mit.edu> wrote:

Show 8 quoted lines

> On 12/1/21 15:10, Eric Sunshine wrote:
> > 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().