Re: [PATCH 5/7] worktree.c: add clear_worktree()

From: Duy Nguyen <pclouds@gmail.com>
Date: May 22, 2016, 08:58 UTC
Message-ID: <CACsJy8Dkfm91g9ez2PYXHjPevPcQaeEXTPteEQgFExYt6SVd5A@mail.gmail.com>
In-Reply-To: <CAPig+cS-tb_hD3k2XsTioJrkzaB3FxSnrxM6JMq5-og1ADnoRg@mail.gmail.com>

On Wed, May 11, 2016 at 1:36 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 26 quoted lines

> On Tue, May 10, 2016 at 10:15 AM, Nguyễn Thái Ngọc Duy
> <pclouds@gmail.com> wrote:
>> The use case is keep some worktree and discard the rest of the worktree
>> list.
>
> So, you're saying that rather than a client freeing the entire
> worktree list like this:
>
>     free_worktrees(worktrees);
>
> it might instead want to keep some element ('n') and free all others
> plus the list itself, like this:
>
>     struct worktree *keep = worktrees[n];
>     struct worktree **p = worktrees;
>     for (; *p; p++)
>         if (*p != keep)
>             clear_worktree(*p);
>     free(worktrees);
>
> Is that correct?
>
> If so, then doesn't this require the client to have far too intimate
> knowledge of the internals of free_worktrees(). Or, was your idea that
> the client would simply leak the 'worktrees' array after freeing the
> unwanted elements?

I looked back in my trees to see why I needed it in the first place. It turns out my explanation was a lie. I needed it for refactoring builtin/worktree.c:add_worktree(), where I initialized struct worktree separately, not from get_worktrees() [1]. Because it's not clear of the exact use case for this commit, and because that commit I mentioned may take even longer time to be here (it adds "git worktree move --repository", a lot more complex operation than a "git worktree move"), I'll take this commit out of the series for now.

[1] https://github.com/pclouds/git/commit/528d81ce5609b5fbbe1ba193389f7aeecc83d992

Show 6 quoted lines

> In either case, a cleaner approach might be to provide a function for
> copying a worktree element. Perhaps:
>
>     struct worktree *copy_worktree(const struct worktree *);
>
> or something?

-- 
Duy