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

[Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>]

Tanay Abhra <tanayabh@gmail.com> writes:

> Also, I tried implementing Junio's request about saving the line
> number and the file name for each
> key-value pair. I had some success, but with some changes,

My opinion on this:

* It's low priority. I think the order of priority should be (high to
  low)

  1) Finish and get the current series into pu (we're almost there, it's
     not time to back off and restart something new).

  2) Work on the other series that uses the new API. We don't need to
     change _all_ the uses, but we do need a few tens of them to
     validate the fact that the new API is nice and convenient to use.

  3) Get new actual features for the user (tidy up config files, give
     error messages based on numbers).

  You probably won't finish everything, so just think: if the GSoC ends
  between 1) and 2), how serious is it? if it ends between 2) and 3),
  how serious? If reverse the order, then the risk of having nothing
  finished and mergeable at the end is high. If it happens, the
  community may or may not take over and finish it ...

* Still, making sure that the (file, line) is doable later without too
  much change is good. So, if indeed, moving all code to another file is
  required, then it may make sense to do it now to avoid code move
  later.

> 1. config-hash.c had to be shifted to config.c entirely.

Why? I guess one reason is the use of struct cf (are there others?), but
moving just

config_hash_callback
config_hash_add_value
git_configset_add_file

to config.c should do it. Then, config.c could use config-hash.c.

But a cleaner way would be to allow the callback to receive the
config_file struct without accessing it as a global variable (currently,
we have no way to parse two config files in parallel for example).

In practice, it should be possible to pass a 4th pointer argument to the
callback, and keep compatibility with 3 arguments callback (having too
many arguments in not a problem will all ABI I know), but I'm don't
think this is allowed in theory.

On overall, I'm not convinced we should move the code. When the argument
"I need to merge these two things otherwise it doesn't compile" comes in
a discussion, it usually means there's an architecture issue
somewhere ;-).

> One more thing, Karsten's string-intern API can be used for saving
> file names as they are repeated a
> lot.

This, or storing the list of filenames in struct config_set (more or
less as you did in previous patch), and store pointers to the same
string in each config_hash_entry.

But strdup-ing all filenames seems a bit heavy to me. Even though we're
not talking about performance-critical part of Git, I don't like the
idea of wasting too much ;-).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/