On Fri, Aug 24 2018, Antonio Ospite wrote:
> Add a new print_config_from_gitmodules() helper function to print values
> from .gitmodules just like "git config -f .gitmodules" would.
>
> This will be used by a new submodule--helper subcommand to be able to
> access the .gitmodules file in a more controlled way.
>
> Signed-off-by: Antonio Ospite <ao2@xxxxxx>
> ---
> submodule-config.c | 25 +++++++++++++++++++++++++
> submodule-config.h | 2 ++
> 2 files changed, 27 insertions(+)
>
> diff --git a/submodule-config.c b/submodule-config.c
> index fc2c41b947..eef96c4198 100644
> --- a/submodule-config.c
> +++ b/submodule-config.c
> @@ -682,6 +682,31 @@ void submodule_free(struct repository *r)
> submodule_cache_clear(r->submodule_cache);
> }
>
> +static int config_print_callback(const char *key_, const char *value_, void *cb_data)
> +{
> + char *key = cb_data;
> +
> + if (!strcmp(key, key_))
> + printf("%s\n", value_);
> +
> + return 0;
> +}
No problem with the code itself, but I'd find this a lot easier to read
in context if it was:
key_ -> var
value_ -> value
key -> wanted_key, perhaps?
I.e. the rest of the file uses the convention of the
config_from_gitmodules callbacks getting "var" and "value",
respectively.
We don't use this convention of suffixing variables with "_" if they're
arguments to the function anywhere else.
> +int print_config_from_gitmodules(const char *key)
> +{
> + int ret;
> + char *store_key;
> +
> + ret = git_config_parse_key(key, &store_key, NULL);
> + if (ret < 0)
> + return CONFIG_INVALID_KEY;
Isn't this a memory leak? I.e. we should free() and return here, no?
> + config_from_gitmodules(config_print_callback, the_repository, store_key);
> +
> + free(store_key);
> + return 0;
> +}
> +
> struct fetch_config {
> int *max_children;
> int *recurse_submodules;
> diff --git a/submodule-config.h b/submodule-config.h
> index dc7278eea4..ed40e9a478 100644
> --- a/submodule-config.h
> +++ b/submodule-config.h
> @@ -56,6 +56,8 @@ void submodule_free(struct repository *r);
> */
> int check_submodule_name(const char *name);
>
> +int print_config_from_gitmodules(const char *key);
> +
> /*
> * Note: these helper functions exist solely to maintain backward
> * compatibility with 'fetch' and 'update_clone' storing configuration in
Another style nit: Makes more sense to put this new function right
underneath "submodule_free" above, instead of under 1/2 of the functions
that have a big comment describing how they work, since that comment
doesn't apply to this new function.
