Re: [PATCHv2 3/8] submodule config: remove name_and_item_from_var

From: Eric Sunshine <ericsunshine@gmail.com>
Date: Oct 30, 2015, 01:23 UTC
Message-ID: <CAPig+cSpnGE5Acgvd+b0arcFx8oStuRRKR4fcSwZG2fbEZ6wSQ@mail.gmail.com>
In-Reply-To: <1446074504-6014-4-git-send-email-sbeller@google.com>

On Wed, Oct 28, 2015 at 7:21 PM, Stefan Beller <sbeller@google.com> wrote:

> submodule config: remove name_and_item_from_var
>
> By inlining `name_and_item_from_var` it is easy to add later options
> which are not required to have a submodule name.

I guess you're trying to say that name_and_item_from_var() didn't provide a proper abstraction, thus wasn't as useful as expected. Perhaps that commit message could make this shortcoming clearer.

Show 24 quoted lines

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> diff --git a/submodule-config.c b/submodule-config.c
> index 8b8c7d1..4d0563c 100644
> --- a/submodule-config.c
> +++ b/submodule-config.c
> @@ -251,18 +235,25 @@ 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;
> +       char *name;
>
> -       /* 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)
>                 return 0;
>
> +       if (!subsection_len)
> +               return 0;

Alternately:

    if (parse_config_key(var, "submodule", &subsection,
            &subsection_len, &key) < 0 || !subsection_len)
        return 0;

Show 8 quoted lines

> +
> +       /* subsection is not null terminated */
> +       name = xmemdupz(subsection, subsection_len);
>         submodule = lookup_or_create_by_name(me->cache,
>                                              me->gitmodules_sha1,
> -                                            name.buf);
> +                                            name);
> +       free(name);

Since this is all private to submodule-config.c, I wonder if it would be cleaner to change lookup_or_create_by_name() to accept a name_length argument?

Show 5 quoted lines

> -       if (!strcmp(item.buf, "path")) {
> +       if (!strcmp(key, "path")) {
>                 if (!value)
>                         ret = config_error_nonbool(var);
>                 else if (!me->overwrite && submodule->path != NULL)