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

Tanay Abhra <tanayabh@gmail.com> writes:

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.

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

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