Re: [PATCH 3/8] submodule-config: remove name_and_item_from_var

[Junio C Hamano <gitster@pobox.com>]

Stefan Beller <sbeller@google.com> writes:

> `name_and_item_from_var` does not provide the proper abstraction
> we need here in a later patch.

... so the idea is to first open-code the calling site in this
patch, and bring a different abstraction that is better suited to
this machinery in a later step?  Makes sense, especially it only
have a single callsite--let's continue reading...

> Signed-off-by: Stefan Beller <sbeller@google.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  submodule-config.c | 48 ++++++++++++++++--------------------------------
>  1 file changed, 16 insertions(+), 32 deletions(-)
>
> diff --git a/submodule-config.c b/submodule-config.c
> index 6d01941..b826841 100644
> --- a/submodule-config.c
> +++ b/submodule-config.c
> @@ -161,31 +161,17 @@ static struct submodule *cache_lookup_name(struct submodule_cache *cache,
>  	return NULL;
>  }
>  
> -static int name_and_item_from_var(const char *var, struct strbuf *name,
> -				  struct strbuf *item)
> -{
> -	const char *subsection, *key;
> -	int subsection_len, parse;
> -	parse = parse_config_key(var, "submodule", &subsection,
> -			&subsection_len, &key);
> -	if (parse < 0 || !subsection)
> -		return 0;
> -
> -	strbuf_add(name, subsection, subsection_len);
> -	strbuf_addstr(item, key);
> -
> -	return 1;
> -}
> -
>  static struct submodule *lookup_or_create_by_name(struct submodule_cache *cache,
> -		const unsigned char *gitmodules_sha1, const char *name)
> +						  const unsigned char *gitmodules_sha1,
> +						  const char *name_ptr, int name_len)
>  {
>  	struct submodule *submodule;
>  	struct strbuf name_buf = STRBUF_INIT;
> +	char *name = xmemdupz(name_ptr, name_len);

This used to be fed the whole name, but now the caller uses
parse_config_key() and we get a <ptr,len> pair, hence a pair of new
variable allocation and release in this function.  OK.

>  	submodule = cache_lookup_name(cache, gitmodules_sha1, name);
>  	if (submodule)
> -		return submodule;
> +		goto out;
>  
>  	submodule = xmalloc(sizeof(*submodule));
>  
> @@ -201,7 +187,8 @@ static struct submodule *lookup_or_create_by_name(struct submodule_cache *cache,
>  	hashcpy(submodule->gitmodules_sha1, gitmodules_sha1);
>  
>  	cache_add(cache, submodule);
> -
> +out:
> +	free(name);
>  	return submodule;
>  }
>  
> @@ -251,18 +238,18 @@ static int parse_config(const char *var, const char *value, void *data)
>  {
>  	struct parse_config_parameter *me = data;
>  	struct submodule *submodule;
> -	struct strbuf name = STRBUF_INIT, item = STRBUF_INIT;
> -	int ret = 0;
> +	int subsection_len, ret = 0;
> +	const char *subsection, *key;
>  
> -	/* this also ensures that we only parse submodule entries */
> -	if (!name_and_item_from_var(var, &name, &item))
> +	if (parse_config_key(var, "submodule", &subsection,
> +			     &subsection_len, &key) < 0 || !subsection_len)
>  		return 0;

The strbuf name became subsection ptr,len pair [*1*].
>  	submodule = lookup_or_create_by_name(me->cache,
>  					     me->gitmodules_sha1,
> -					     name.buf);
> +					     subsection, subsection_len);

... and that is used here.

>  
> -	if (!strcmp(item.buf, "path")) {
> +	if (!strcmp(key, "path")) {

... and the "item" that used to hold the third-level variable name
is now a variable "key".  Looks correct.


[Footnote]

*1* I notice that the error detection has been slightly changed.
    The original code used to detect a two-level variable name by
    checking if the subsection pointer is NULL.  Now you check the
    length.

    I didn't check the implementation of parse_config_key(), but it
    feels to me that the original would be "more" correct.  Even
    though we may not accept an empty second level name (or do we
    already?), it would be a good idea to get the code prepared to
    differentiate between the lack of subsection and having a
    subsection that happens to be an empty string, i.e.

	[section ""]
        	variable = value