Stefan Beller <[email protected]> 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 <[email protected]> > Signed-off-by: Junio C Hamano <[email protected]> > --- > 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
