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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 10, 2014, 16:46 UTC
Message-ID: <xmqqvbr588mh.fsf@gitster.dls.corp.google.com>
In-Reply-To: <vpqa98hwnxp.fsf@anie.imag.fr>

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

Show 17 quoted lines

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

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.