Re: [PATCH 7/7] submodule-config: cleanup backward compatibility helpers

[Brandon Williams]

On 06/22, Antonio Ospite wrote:
> Use one callback per configuration setting to handle the generic options
> which have to be supported for backward compatibility.
> 
> This removes some duplication and some support code at the cost of
> parsing the .gitmodules file twice when calling the fetch command.
> 
> Signed-off-by: Antonio Ospite <a...@ao2.it>

I'm not sure how I feel about this patch, I'm leaning towards not
needing it but I like the idea of eliminating the duplicate code.  One
way you could get rid of having to read the .gitmodules file twice would
be something like the following but I don't really think its needed:


diff --git a/submodule-config.c b/submodule-config.c
index ce204fb53..7cc1864b5 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -681,19 +681,24 @@ void submodule_free(struct repository *r)
                submodule_cache_clear(r->submodule_cache);
 }
 
-struct fetch_config {
-       int *max_children;
+struct fetch_clone_config {
+       int *fetchjobs;
        int *recurse_submodules;
 };
 
-static int gitmodules_fetch_config(const char *var, const char *value, void 
*cb)
+static int gitmodules_fetch_clone_config(const char *var, const char *value,
+                                        void *cb)
 {
-       struct fetch_config *config = cb;
+       struct fetch_clone_config *config = cb;
        if (!strcmp(var, "submodule.fetchjobs")) {
-               *(config->max_children) = parse_submodule_fetchjobs(var, value);
+               if (config->fetchjobs)
+                       *(config->fetchjobs) =
+                               parse_submodule_fetchjobs(var, value);
                return 0;
        } else if (!strcmp(var, "fetch.recursesubmodules")) {
-               *(config ->recurse_submodules) = 
parse_fetch_recurse_submodules_arg(var, value);
+               if (config->recurse_submodules)
+                       *(config->recurse_submodules) =
+                               parse_fetch_recurse_submodules_arg(var, value);
                return 0;
        }
 
@@ -702,23 +707,20 @@ static int gitmodules_fetch_config(const char *var, const 
char *value, void *cb)
 
 void fetch_config_from_gitmodules(int *max_children, int *recurse_submodules)
 {
-       struct fetch_config config = {
-               .max_children = max_children,
-               .recurse_submodules = recurse_submodules
+       struct fetch_clone_config config = {
+               .fetchjobs = max_children,
+               .recurse_submodules = recurse_submodules,
        };
-       config_from_gitmodules(gitmodules_fetch_config, the_repository, 
&config);
-}
-
-static int gitmodules_update_clone_config(const char *var, const char *value,
-                                         void *cb)
-{
-       int *max_jobs = cb;
-       if (!strcmp(var, "submodule.fetchjobs"))
-               *max_jobs = parse_submodule_fetchjobs(var, value);
-       return 0;
+       config_from_gitmodules(gitmodules_fetch_clone_config, the_repository,
+                              &config);
 }
 
 void update_clone_config_from_gitmodules(int *max_jobs)
 {
-       config_from_gitmodules(gitmodules_update_clone_config, the_repository, 
&max_jobs);
+       struct fetch_clone_config config = {
+               .fetchjobs = max_jobs,
+               .recurse_submodules = NULL,
+       };
+       config_from_gitmodules(gitmodules_fetch_clone_config, the_repository,
+                              &config);
 }

-- 
Brandon Williams