Re: [PATCH 3/8] submodule-config: remove name_and_item_from_var
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 3, 2016, 23:09 UTC
- Message-ID
- <xmqqk2mll7c1.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <1454435497-26429-4-git-send-email-sbeller@google.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...
Show 38 quoted lines
> 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.
Show 31 quoted lines
> 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