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

[Stefan Beller]

On Thu, Oct 29, 2015 at 6:53 PM, Eric Sunshine  wrote:
> On Wed, Oct 28, 2015 at 7:21 PM, Stefan Beller  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 
>>
>> # 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", ,
>> +_len, ) < 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Eric Sunshine]

On Wed, Oct 28, 2015 at 7:21 PM, Stefan Beller  wrote:
> submodule-config: parse_config

Um, what?

> 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 
>
> # Conflicts:
> #   submodule-config.c

Interesting.

> Signed-off-by: Stefan Beller 
> ---
> diff --git a/submodule-config.c b/submodule-config.c
> index 4d0563c..1cea404 100644
> --- a/submodule-config.c
> +++ b/submodule-config.c
> @@ -231,27 +231,23 @@ struct parse_config_parameter {
> int overwrite;
>  };
>
> -static int parse_config(const char *var, const char *value, void *data)
> +static int parse_generic_submodule_config(const char *var,
> + const char *key,
> + const char *value)
>  {
> -   struct parse_config_parameter *me = data;
> -   struct submodule *submodule;
> -   int subsection_len, ret = 0;
> -   const char *subsection, *key;
> -   char *name;
> -
> -   if (parse_config_key(var, "submodule", ,
> -_len, ) < 0)
> -   return 0;
> -
> -   if (!subsection_len)
> -   return 0;
> +   return 0;
> +}
>
> -   /* subsection is not null terminated */
> -   name = xmemdupz(subsection, subsection_len);
> -   submodule = lookup_or_create_by_name(me->cache,
> -me->gitmodules_sha1,
> -name);
> -   free(name);
> +static int parse_specific_submodule_config(struct parse_config_parameter *me,
> +  const char *name,
> +  const char *key,
> +  const char *value,
> +  const char *var)

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?

> +{
> +   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", ,
> +_len, ) < 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.

if (!subsection_len)
return parse_generic_submodule_config(...);

int ret;
...
return ret;

This might give you a less noisy diff and would be a bit more
consistent with the early part of the function where you don't bother
giving the if (parse_config_key(...)) an 'else' body.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv2 4/8] submodule-config: parse_config

[Stefan Beller]

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 

# Conflicts:
#   submodule-config.c

Signed-off-by: Stefan Beller 
---
 submodule-config.c | 58 --
 1 file changed, 39 insertions(+), 19 deletions(-)

diff --git a/submodule-config.c b/submodule-config.c
index 4d0563c..1cea404 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -231,27 +231,23 @@ struct parse_config_parameter {
int overwrite;
 };
 
-static int parse_config(const char *var, const char *value, void *data)
+static int parse_generic_submodule_config(const char *var,
+ const char *key,
+ const char *value)
 {
-   struct parse_config_parameter *me = data;
-   struct submodule *submodule;
-   int subsection_len, ret = 0;
-   const char *subsection, *key;
-   char *name;
-
-   if (parse_config_key(var, "submodule", ,
-_len, ) < 0)
-   return 0;
-
-   if (!subsection_len)
-   return 0;
+   return 0;
+}
 
-   /* subsection is not null terminated */
-   name = xmemdupz(subsection, subsection_len);
-   submodule = lookup_or_create_by_name(me->cache,
-me->gitmodules_sha1,
-name);
-   free(name);
+static int parse_specific_submodule_config(struct parse_config_parameter *me,
+  const char *name,
+  const char *key,
+  const char *value,
+  const char *var)
+{
+   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", ,
+_len, ) < 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;
+   }
+}
+
 static int gitmodule_sha1_from_commit(const unsigned char *commit_sha1,
  unsigned char *gitmodules_sha1)
 {
-- 
2.5.0.281.g4ed9cdb

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html