Re: [PATCHv2 4/8] submodule-config: parse_config

[Stefan Beller <sbeller@google.com>]

On Thu, Oct 29, 2015 at 6:53 PM, Eric Sunshine <ericsunshine@gmail.com> wrote:
> On Wed, Oct 28, 2015 at 7:21 PM, Stefan Beller <sbeller@google.com> wrote:
>> submodule-config: parse_config
>
> Um, what?

submodule-config: Introduce parse_generic_submodule_config

>
>> This rewrites parse_config to distinguish between configs specific to
>> one submodule and configs which apply generically to all submodules.
>> We do not have generic submodule configs yet, but the next patch will
>> introduce "submodule.jobs".
>>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>>
>> # Conflicts:
>> #       submodule-config.c
>
> Interesting.

fixed

>
> Minor: Are these 'key', 'value', 'var' arguments analogous to the
> like-named arguments of parse_generic_submodule_config()? If so, why
> is the order of arguments different?

Reordered. I thought how they made most sense individually, but consistency
across functions is better.

>
>> +{
>> +       int ret = 0;
>> +       struct submodule *submodule = lookup_or_create_by_name(me->cache,
>> +                                                              me->gitmodules_sha1,
>> +                                                              name);
>>
>>         if (!strcmp(key, "path")) {
>>                 if (!value)
>> @@ -318,6 +314,30 @@ static int parse_config(const char *var, const char *value, void *data)
>>         return ret;
>>  }
>>
>> +static int parse_config(const char *var, const char *value, void *data)
>> +{
>> +       struct parse_config_parameter *me = data;
>> +
>> +       int subsection_len;
>> +       const char *subsection, *key;
>> +       char *name;
>> +
>> +       if (parse_config_key(var, "submodule", &subsection,
>> +                            &subsection_len, &key) < 0)
>> +               return 0;
>> +
>> +       if (!subsection_len)
>> +               return parse_generic_submodule_config(var, key, value);
>> +       else {
>> +               int ret;
>> +               /* subsection is not null terminated */
>> +               name = xmemdupz(subsection, subsection_len);
>> +               ret = parse_specific_submodule_config(me, name, key, value, var);
>> +               free(name);
>> +               return ret;
>> +       }
>> +}
>
> Minor: You could drop the 'else' and outdent its body, thus losing one
> indentation level.

By passing on the subsection, subsection_len, we only have one statement there

     if (!subsection_len)
         return parse_generic_submodule_config(key, var, value, me);
     else
         return parse_specific_submodule_config(subsection,
               subsection_len, key,
                  var, value, me);

will do without dedenting I guess.