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

Junio C Hamano [off-list ref] 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/