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

Matthieu Moy [off-list ref] writes:

Junio C Hamano [off-list ref] writes:

quoted

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.

I agree that it is good to allow scan-all-config-items-with-callback
callers to learn <file, line>, but that is irrelevant.  Perhaps you
misread what I envision as the endgame of this thing to be and that
is where our differences come from.  One thing I think would be good
to see in the endgame will be to give the benefit of the caching
layer to the callback callers without having them change a single
line of their code.

One possible sequence of changes to make it happen would go like
this, roughly in this order:

 - rename the current git_config() to git_config_raw(), and
   make it static to the config.c.

 - write a thin wrapper git_config() around git_config_raw() in
   config.c, until caching layer learns the iterator.

 - write caching layer to read from git_config_raw().

 - teach git_config_raw() feed its callback functions to learn the
   <file, line> information.  git_configset_get_<type> can then
   start using this in their error reporting.

 - implement iterator in the caching layer.

 - rewrite git_config() that was a thin wrapper around
   git_config_raw() by using the iterator over the cached values.

 - (optional) think about ways to give <file, line> information to
   the callback style callers.  Perhaps we need git_config_2().
   Perhaps we can rewrite all callers of git_config().  We do not
   have to decide it now.

Between git_parse_source() and git_config_raw() we would need to
pass the line-number information, but there is no reason for us to
make public (all of these will be implementation details of the
config system, including the config caching).

My answer to "why shouldn't the callbacks have <file, line>
information?" is "there is no reason why they shouldn't.  It is a
good addition in the long run".  But the optionality of the last
step in the above list makes it an irrelevant question.