Re: [RFC PATCH 000/194] Moving global state into the repository object

From: Duy Nguyen <pclouds@gmail.com>
Date: Feb 10, 2018, 10:36 UTC
Message-ID: <CACsJy8DvkjqL0i7mGbFQmAm0Ncvr4SeRMeZTzUzGdMB0Y86aeA@mail.gmail.com>
In-Reply-To: <CAGZ79kabQ1WvxxjFV+fgq8b142yNrtGZC_Vqac+5b3CkUpbi2w@mail.gmail.com>

On Thu, Feb 8, 2018 at 1:06 AM, Stefan Beller <sbeller@google.com> wrote:

Show 12 quoted lines

>> 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?

I think it's a good thing to do. We need to make these implicit dependencies explicit sooner or later.

Also, perhaps at the earlier steps you don't need to add everything to the_respository yet. You can have the_object_store, the_parsed_object (and we already have get_main_ref_store()), then use them internally without touching the API, which helps reduce code change. For example, read_sha1_file() could be converted to take "struct object_store *" all the way

void *read_sha1_file_extended(const unsigned char *sha1,
      enum object_type *type,
      unsigned long *size,
      int lookup_replace)
{
        struct object_store *store = &the_object_store;
        ...
        // more access to "store"
}

When every piece is in place, the API change can be made be removing that "store = &the_object_store" line and make "store" an argument of read_sha1_file().

-- 
Duy