Re: [PATCH 1/2] docs: add a basic description of the config API

[Jeff King <peff@peff.net>]

On Wed, Feb 08, 2012 at 01:55:30PM +0700, Nguyen Thai Ngoc Duy wrote:

> Or allow multiple callbacks from git_config(). The problem with is it
> adds another common set of config vars. Now it's common to chain var
> sets by doing
> 
> int config_callback(...)
> {
>     ....
>     return yet_another_callback(...);
> }
> 
> int main()
> {
>    git_config(config_callback, ...)
>    ...
> 
> where yet_another_callback() hard codes another callback (usually
> git_default_config). This is inflexible. If we allow multiple
> callbacks, git_column_config() could be changed to return just 0 or -1
> (i.e. 1 remains unsuccessful error code).

I don't think we need multiple callbacks. You could convert any such
call of:

  git_config_multiple(cb1, cb2, cb3, NULL, NULL);

into:

  int combining_callback(const char *var, const char *value, void *data)
  {
          if (cb1(var, value, data) < 0)
                  return -1;
          if (cb2(var, value, data) < 0)
                  return -1;
          if (cb3(var, value, data) < 0)
                  return -1;
          return 0;
  }

But note that you get the same "data" pointer in each case. If you
wanted different data pointers, you would need more support from the
config machinery. However, I'd rather that be spelled:

  git_config(cb1, data1);
  git_config(cb2, data2);
  git_config(cb3, data3);

and if the efficiency isn't acceptable, then looking into caching the
key/value pairs.

> On second thought, I think calling git_config() multiple times, each
> time with one callback, may work too. We may want to cache config
> content to avoid accessing files too many times though.

Exactly. I would do that first, and then worry about efficiency later if
it comes up. The first time I saw that we cached config multiple times
in a program run (several years ago), I had the same thought. But I
don't think the performance impact for reading the config 2 or 3 times
instead of 1 was even measurable, so I stopped looking into it.

If were to adopt something like the "automatically run this program to
get the config value" proposal that has been floating around (and I am
not saying we should), _then_ I think it might make sense to cache the
values, because the sub-program might be expensive to run.

-Peff