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

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

On Mon, Mar 20, 2017 at 1:59 PM, Michael Haggerty [off-list ref] wrote:

quoted

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 :-)

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