Re: [PATCH v4 1/9] submodule: add a print_config_from_gitmodules() helper

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Aug 24, 2018, 14:32 UTC
Message-ID: <87wosfesxl.fsf@evledraar.gmail.com>
In-Reply-To: <20180824132951.8000-2-ao2@ao2.it>

On Fri, Aug 24 2018, Antonio Ospite wrote:

Show 29 quoted lines

> 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@ao2.it>
> ---
>  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.

Show 8 quoted lines

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

Show 22 quoted lines

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