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

From: Jeff King <peff@peff.net>
Date: Feb 8, 2012, 15:59 UTC
Message-ID: <20120208155950.GD8773@sigill.intra.peff.net>
In-Reply-To: <CACsJy8B1cMBbrUznOp95u8YmfLf5bbzox8g9cuQUwgf-vY1XrQ@mail.gmail.com>

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

Show 19 quoted lines

> 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