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

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Jul 10, 2014, 09:41 UTC
Message-ID: <vpqa98hwnxp.fsf@anie.imag.fr>
In-Reply-To: <xmqq38ea77gt.fsf@gitster.dls.corp.google.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 9 quoted lines

> After thinking about it a bit more, I think <file, line> support
> needs to be done not as a mere client of the generic, uncached
> git_config() API that we have had for forever, like the current
> iteration, but needs to know a bit more about the state the caller
> of the callback (namely, git_parse_source()), and we obviously do
> not want to expose that machinery to anybody other than the
> implementation of the config subsystem (to which the new facility
> this GSoC project adds belongs to), so in that sense you have to
> have your code in the same config.c file anyway.

I do not buy the argument. Why would callers of the callback-style API not be allowed to give <file, line> errors?

To me, it is a weakness of the API that <file, line> information is not available to callback functions, regardless of the config-cache.

Show 5 quoted lines

> It is somewhat dissapointing that this initial implementation was
> done as a client of the traditional git_config(), by the way.  I
> would have expected that it would be the other way around, in that
> the traditional callers of git_config() would behefit automatically
> by having the cache layer below it.

I disagree, and I agree ;-).

I disagree that it is disapointing to use git_config(), and I think the beauty of the current implementation is to allow this cache mechanism without changing the parsing code.

I agree that the callers of git_config() could benefit from the cache mechanism, i.e. use the in-memory pre-parsed config instead of re-parsing the file each time.

Show 5 quoted lines

> But we have to start from somewhere.  Perhaps the round after this
> one can rename the exisiting implementation of git_config() to
> something else internal to the caching layer and give the existing
> callers a compatible interface that is backed by this new caching
> layer in a transparent way.

In PATCH v4, there was a git_config_iter function that did exactly that. I didn't notice it was gone for v5, but could be rather easily resurected.

I suggest, on top of the current series:

PATCH 3 : (re)introduce git_config_iter, compatible with git_config
  (one variant with a configset argument, another working on the_config_set).

PATCH 4 : rename git_config to e.g. git_config_parse, and
  git_config_iter to git_config.

Then, check that tests still pass (PATCH 4 automatically uses the new code essentially in every place where Git deals with config, so existing tests will start to stress the code a lot more), and check with e.g. "strace -e open git ..." that config files are now parsed only once where they used to be parsed multiple times.

I'd do that as a separate series, to let the current one finally reach pu.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/