On Wed, Feb 7, 2018 at 3:48 AM, Duy Nguyen <pclo...@gmail.com> wrote:
> On Tue, Feb 6, 2018 at 6:51 AM, Stefan Beller <sbel...@google.com> wrote:
>> This series moves a lot of global state into the repository struct.
>> It applies on top of 2512f15446149235156528dafbe75930c712b29e (2.16.0)
>> It can be found at https://github.com/stefanbeller/git/tree/object-store
>>
>> Motivation for this series:
>> * Easier to reason about code when all state is stored in one place,
>> for example for multithreading
>> * Easier to move to processing submodules in-process instead of
>> calling a processes for the submodule handling.
>> The last patch demonstrates this.
>
> I have a mixed feeling about this. The end game is to keep
> "the_repository" references as minimum as possible, right? Like we
> only need to mention in in cmd_xxx() and not all over the place. If
> so, yay!!
Yes. And the super-end-game long after this series is to have
cmd_xxx(struct repository *r, argc, argv)
or so.
> Something else.. maybe "struct repository *" should not be a universal
> function argument to avoid global states. Like sha1_file.c is mostly about the
> object store, and I see that you added object store struct (nice!).
> These sha1 related function should take the object_store* (which
> probably is a combination of both packed and loose stores since they
> depend on each other), not repository*. This way if a function needs
> both access to object store and ref store, we can see the two
> dependencies from function arguments.
I tried that in the beginning and it blew up a couple of times when I realized
that I forgot to pass through one of these dependencies.
Maybe we can go to the repository and shrink the scope in a follow up?
> The alternate object store, if modeled right, could share the same
> object store interface. But I don't think we should do anything that
> big right now, just put alternate object store inside object_store.
yup that is the case, see struct raw_object_store at the end of the series
https://github.com/stefanbeller/git/blob/object-store-v2/object-store.h
> Similarly those functions in blob.c, commit.c, tree.c... should take
> object_parser* as argument instead of repository*. Maybe there's a
> better name for this than object_parser. parsed_object_store I guess.
> Or maybe just object_pool.
Note that the initial few patches are misleading in the name,
https://public-inbox.org/git/20180205235735.216710-59-sbel...@google.com/
which splits up the object handling into two layers, the "physical" layer
(where to get the data from, mmaping pack files, alternates, streams of bytes),
which is "struct raw_object_store objects;" in the repository, and the
"object" layer, which is about parsing the objects and making sense of the data
(which tree belongs to a commit, dereferencing tags)
So maybe I'll try to make the first layer into its own series, such
that we'll have a smaller series.
Thanks,
Stefan
> --
> Duy
