Re: [PATCH v3 2/4] refs: introduce get_worktree_ref_store()

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Mar 20, 2017, 14:25 UTC
Message-ID: <0d437f43-30d6-3920-5298-0c426b098c1a@alum.mit.edu>
In-Reply-To: <CACsJy8B85TH0DOViEmfh8mOz_u7rVotKJa-HYk6TJ81kvV4xPg@mail.gmail.com>

On 03/20/2017 01:01 PM, Duy Nguyen wrote:

Show 5 quoted lines

> On Mon, Mar 20, 2017 at 1:59 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> I guess I can hold my nose and accept storing worktree and submodule
>> `ref_store`s together in a single hashmap
> 
> Release your nose, I'll add a new hashmap :)

Thanks :-)

Show 9 quoted lines

> But while we're here,
> what are your thoughts about moving submodule-specific to submodule.c?
> That includes the hashmap, get_submodule_ref_store() and all other
> submodule stuff, to submodule.c where submodule-specific stuff stays.
> The same for worktree stuff, to worktree.c. That keeps refs.c to core
> refs business. And the hashmap can be used for more than just refs.
> For example, submodule has configs, and worktree also has bunch of
> other stuff that I would like to just cache and not readdir() and
> parse every time.

I can't think of any objections, though there might be some devils in the details. I assume you mean that the submodule-specific hashmap wouldn't be `{name -> struct ref_store}` anymore, but rather `{name -> struct submodule}`, and `struct submodule` would hold the associated `ref_store`? That sounds very reasonable.

Instead of moving all of the `for_each_*_submodule()` functions over, I encourage you to consider getting rid of them entirely and let the end-users call the `refs_for_each_*()` versions of the functions. Again, I'm not sure that there won't be friction in doing so, but it seems like it's worth a try.

Michael