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

[Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>]

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

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

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

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