Re: [PATCHv2 6/8] submodule: simplify memory handling in config parsing

[Jens Lehmann <Jens.Lehmann@web.de>]

Am 23.01.2013 07:26, schrieb Jeff King:
> We keep a strbuf for the name of the submodule, even though
> we only ever add one string to it. Let's just use xmemdupz
> instead, which is slightly more efficient and makes it
> easier to follow what is going on.
> 
> Unfortunately, we still end up having to deal with some
> memory ownership issues in some code branches, as we have to
> allocate the string in order to do a string list lookup, and
> then only sometimes want to hand ownership of that string
> over to the string_list. Still, making that explicit in the
> code (as opposed to sometimes detaching the strbuf, and then
> always releasing it) makes it a little more obvious what is
> going on.

Thanks, this helps until I some day find the time to refactor
that code into a more digestible shape ;-)

Acked-by: Jens Lehmann <Jens.Lehmann@web.de>

> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  submodule.c | 30 ++++++++++++++----------------
>  1 file changed, 14 insertions(+), 16 deletions(-)
> 
> diff --git a/submodule.c b/submodule.c
> index 25413de..9ba1496 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -127,7 +127,6 @@ int parse_submodule_config_option(const char *var, const char *value)
>  int parse_submodule_config_option(const char *var, const char *value)
>  {
>  	struct string_list_item *config;
> -	struct strbuf submodname = STRBUF_INIT;
>  	const char *name, *key;
>  	int namelen;
>  
> @@ -135,37 +134,36 @@ int parse_submodule_config_option(const char *var, const char *value)
>  		return 0;
>  
>  	if (!strcmp(key, "path")) {
> -		strbuf_add(&submodname, name, namelen);
>  		config = unsorted_string_list_lookup(&config_name_for_path, value);
>  		if (config)
>  			free(config->util);
>  		else
>  			config = string_list_append(&config_name_for_path, xstrdup(value));
> -		config->util = strbuf_detach(&submodname, NULL);
> -		strbuf_release(&submodname);
> +		config->util = xmemdupz(name, namelen);
>  	} else if (!strcmp(key, "fetchrecursesubmodules")) {
> -		strbuf_add(&submodname, name, namelen);
> -		config = unsorted_string_list_lookup(&config_fetch_recurse_submodules_for_name, submodname.buf);
> +		char *name_cstr = xmemdupz(name, namelen);
> +		config = unsorted_string_list_lookup(&config_fetch_recurse_submodules_for_name, name_cstr);
>  		if (!config)
> -			config = string_list_append(&config_fetch_recurse_submodules_for_name,
> -						    strbuf_detach(&submodname, NULL));
> +			config = string_list_append(&config_fetch_recurse_submodules_for_name, name_cstr);
> +		else
> +			free(name_cstr);
>  		config->util = (void *)(intptr_t)parse_fetch_recurse_submodules_arg(var, value);
> -		strbuf_release(&submodname);
>  	} else if (!strcmp(key, "ignore")) {
> +		char *name_cstr;
> +
>  		if (strcmp(value, "untracked") && strcmp(value, "dirty") &&
>  		    strcmp(value, "all") && strcmp(value, "none")) {
>  			warning("Invalid parameter \"%s\" for config option \"submodule.%s.ignore\"", value, var);
>  			return 0;
>  		}
>  
> -		strbuf_add(&submodname, name, namelen);
> -		config = unsorted_string_list_lookup(&config_ignore_for_name, submodname.buf);
> -		if (config)
> +		name_cstr = xmemdupz(name, namelen);
> +		config = unsorted_string_list_lookup(&config_ignore_for_name, name_cstr);
> +		if (config) {
>  			free(config->util);
> -		else
> -			config = string_list_append(&config_ignore_for_name,
> -						    strbuf_detach(&submodname, NULL));
> -		strbuf_release(&submodname);
> +			free(name_cstr);
> +		} else
> +			config = string_list_append(&config_ignore_for_name, name_cstr);
>  		config->util = xstrdup(value);
>  		return 0;
>  	}
>