Re: [PATCH v3 2/3] config: add hashtable for config parsing & retrieval

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Jun 26, 2014, 17:37 UTC
Message-ID: <vpq1tubinbx.fsf@anie.imag.fr>
In-Reply-To: <xmqq61joamcc.fsf@gitster.dls.corp.google.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 24 quoted lines

> When the submodule script that uses "git config -f .gitmodules" is
> converted into C, if the updated config API is ready, it may be able
> to do something like these in a single program:
>
> 	const char *url;
> 	struct config_set *gm_config;
>
>         /* read from $GIT_DIR/config */
>         url = git_config_get_string("remote.origin.url");
>
>         /*
>          * allow us to read from .gitmodules; the parameters are
>          * list of files that make up the configset, perhaps.
>          */
> 	gm_config = git_configset_new(".gitmodules", NULL);
>
>
>         if (!git_configset_get_bool(gm_config, "submodule.frotz.ignore")) {
> 		/* do a lot of stuff for the submodule */
>                 ...
> 	}
>
>         /* when we are done with the configset */
>         git_configset_clear(gm_config);

Isn't that a bit overkill? Why not just let the caller manage a hashmap directly instead of a config_set? Your code could become

  struct hashmap *gm_config;
  config_cache_init(&gm_config);
  config_cache_read_from_file(".gitmodule", gm_config);
  /* possibly more calls to
     config_cache_read_from_file("some-other-file", ...). */
  
and then

  if (!git_configset_get_bool(gm_config, "submodule.frotz.ignore")) {
     ...

Perhaps a bit more complicated to use, but much simpler to implement. Since there are very few callers, I'd say it's better to keep the implementation simple.

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