Re: [PATCH v4 1/2] add `config_set` API for caching config files

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 2, 2014, 17:09 UTC
Message-ID: <xmqq8uobit5y.fsf@gitster.dls.corp.google.com>
In-Reply-To: <vpqoax8m8bh.fsf@anie.imag.fr>

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

Show 5 quoted lines

> I don't like the name "the_config_set". It's not the only one. Perhaps
> repo_config_set? (not totally satisfactory as it does not contain only
> the repo, but the repo+user+system)
>
> What do others think?

I actually do like "the_configset", which goes nicely parallel to "the_index". Neither is the only one, but most of the time our code operates on the primary one and "the_frotz" refers to it (and convenience wrappers that does not specify which set defaults to it).

> What is the reason to deal with `the_config_set` and other config sets
> differently? You're giving arguments to store `the_config_set` as a
> single hashmap, but what is the reason to store others as multiple
> hashmaps?

Confusion, probably. "the_configset" should be just a singleton instance used to store the values we use for the default config system, but its shape should be exactly the same as the other ones users can use to read .gitmodules and friends with.

Show 6 quoted lines

> And actually, I don't completely buy your arguments: having 3 or 4
> hashmaps (.git/config, ~/.gitconfig, ~/.config/git/config and
> /etc/gitconfig) would be a O(4) lookup, which is still O(1), and you
> could deal with include directives by having ".git/config and included
> files" in a hashmap, "~/.gitconfig and included files" in a second
> hashmap, ...

OK.

> I would personally find it much simpler to have a single hashmap. We'd
> lose the ability to invalidate the cache for only a single file, but I'm
> not sure it's worth the code complexity.

OK, too.