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

[Brandon Williams <bmwill@xxxxxxxxxx>]

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 <ao2@xxxxxx>

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