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

[Tanay Abhra <tanayabh@gmail.com>]

On 7/9/2014 7:49 PM, Matthieu Moy wrote:
> 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).

Noted. I will send the revised series tomorrow ASAP.

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

Okay, I have updated the series and will send the new one by friday.
Still, I am aiming to rewrite at least half of total calls next weeks
when (1) is in pu.

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

Noted, still I want to add that even when GSoC finishes, I won't leave the
work unfinished. I had said that I wanted to continue being part of the Git
community and I mean that. :)

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

Yes, the only problem I see is that the future readers of config.c might
read two versions of the git_config_type helpers and may get confused,
as they have similar names as git_config_*() & git_config_get_*().
That was the reason in the first place that I moved the code into a new file.

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

Nope, just for using struct cf. All old API functions raise error by
accessing "cf" globally for the file name and line number.

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

I have to decide on what to do next on moving the contents to config.c or not.
Seeing Junio's comments on the topic seems that he wasn't convinced in the
first place that we needed a new file. What should we do, as I am sending the
revised patch tomorrow? The move will be trivial, just cutting and pasting the
contents. Other approaches you mentioned are also doable but, after a certain
amount of retooling. I am open to any method you think would be best.

Cheers,
Tanay Abhra.