[PATCH 0/3] Add option to recurse into submodules

[PATCH 0/3] Add option to recurse into submodules

[Stefan Beller]

In a submodule heavy workflow it becomes tedious to pass in
--recurse-submodules all the time, so make an option for it.

Thanks,
Stefan

Stefan Beller (3):
  submodule.c: add has_submodules to check if we have any submodules
  submodule test invocation: only pass additional arguments
  Introduce submodule.recurse option

 Documentation/config.txt       |  5 +++
 builtin/checkout.c             |  8 ++--
 builtin/fetch.c                |  2 +-
 builtin/read-tree.c            |  2 +-
 builtin/reset.c                |  2 +-
 builtin/submodule--helper.c    |  6 +--
 submodule.c                    | 83 ++++++++++++++++++++++++++++++++++--------
 submodule.h                    |  8 +++-
 t/lib-submodule-update.sh      | 18 ++++++++-
 t/t1013-read-tree-submodule.sh |  4 +-
 t/t2013-checkout-submodule.sh  |  4 +-
 t/t5526-fetch-submodules.sh    | 10 +++++
 t/t7112-reset-submodule.sh     |  4 +-
 unpack-trees.c                 |  2 +-
 14 files changed, 121 insertions(+), 37 deletions(-)

-- 
2.13.0.18.g7d86cc8ba0

[PATCH 1/3] submodule.c: add has_submodules to check if we have any submodules

[Stefan Beller]

When submodules are involved, it often slows down the process, as most
submodule related handling is either done via a child process or by
iterating over the index finding all gitlinks.

For most commands that may interact with submodules, we need have a
quick check if we do have any submodules at all, such that we can
be fast in the case when no submodules are in use.  For this quick
check introduce a function that checks with different heuristics if
we do have submodules around, checking if
* anything related to submodules is configured,
* absorbed git dirs for submodules are present,
* the '.gitmodules' file exists
* gitlinks are recorded in the index.

Each heuristic has advantages and disadvantages.
For example in a later patch, when we first use this function in
git-clone, we'll just check for the existence of the '.gitmodules'
file, because at the time of running the clone command there will
be no absorbed git dirs or submodule configuration around.

Checking for any configuration related to submodules would be useful
in a later stage (after cloning) to see if the submodules are actually
in use.

Checking for absorbed git directories is good to see if the user has
actually cloned submodules already (i.e. not just initialized them by
configuring them).

The heuristic for checking the configuration requires this patch
to have have a global state, whether the submodule config has already
been read, and if there were any submodule related keys. Make
'submodule_config' private to the submodule code, and introduce
'load_submodule_config' that will take care of this global state.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/checkout.c          |  2 +-
 builtin/fetch.c             |  2 +-
 builtin/read-tree.c         |  2 +-
 builtin/reset.c             |  2 +-
 builtin/submodule--helper.c |  6 ++--
 submodule.c                 | 78 +++++++++++++++++++++++++++++++++++----------
 submodule.h                 |  8 ++++-
 unpack-trees.c              |  2 +-
 8 files changed, 77 insertions(+), 25 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index bfa5419f33..2787b343b1 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1215,7 +1215,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 	}
 
 	if (recurse_submodules != RECURSE_SUBMODULES_OFF) {
-		git_config(submodule_config, NULL);
+		load_submodule_config();
 		if (recurse_submodules != RECURSE_SUBMODULES_DEFAULT)
 			set_config_update_recurse_submodules(recurse_submodules);
 	}
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 4ef7a08afc..510ef1c9de 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1344,7 +1344,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 			set_config_fetch_recurse_submodules(arg);
 		}
 		gitmodules_config();
-		git_config(submodule_config, NULL);
+		load_submodule_config();
 	}
 
 	if (all) {
diff --git a/builtin/read-tree.c b/builtin/read-tree.c
index 23e212ee8c..fe2ec60a51 100644
--- a/builtin/read-tree.c
+++ b/builtin/read-tree.c
@@ -177,7 +177,7 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
 
 	if (recurse_submodules != RECURSE_SUBMODULES_DEFAULT) {
 		gitmodules_config();
-		git_config(submodule_config, NULL);
+		load_submodule_config();
 		set_config_update_recurse_submodules(RECURSE_SUBMODULES_ON);
 	}
 
diff --git a/builtin/reset.c b/builtin/reset.c
index 5ce27fcaed..f3aca487d9 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -321,7 +321,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 
 	if (recurse_submodules != RECURSE_SUBMODULES_DEFAULT) {
 		gitmodules_config();
-		git_config(submodule_config, NULL);
+		load_submodule_config();
 		set_config_update_recurse_submodules(RECURSE_SUBMODULES_ON);
 	}
 
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 85aafe46a4..770a95ca14 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1015,7 +1015,7 @@ static int update_clone(int argc, const char **argv, const char *prefix)
 
 	/* Overlay the parsed .gitmodules file with .git/config */
 	gitmodules_config();
-	git_config(submodule_config, NULL);
+	load_submodule_config();
 
 	if (max_jobs < 0)
 		max_jobs = parallel_submodules();
@@ -1058,7 +1058,7 @@ static const char *remote_submodule_branch(const char *path)
 {
 	const struct submodule *sub;
 	gitmodules_config();
-	git_config(submodule_config, NULL);
+	load_submodule_config();
 
 	sub = submodule_from_path(null_sha1, path);
 	if (!sub)
@@ -1130,7 +1130,7 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)
 			     git_submodule_helper_usage, 0);
 
 	gitmodules_config();
-	git_config(submodule_config, NULL);
+	load_submodule_config();
 
 	if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)
 		return 1;
diff --git a/submodule.c b/submodule.c
index 20ed5b5681..14ea405048 100644
--- a/submodule.c
+++ b/submodule.c
@@ -24,6 +24,12 @@ static int initialized_fetch_ref_tips;
 static struct sha1_array ref_tips_before_fetch;
 static struct sha1_array ref_tips_after_fetch;
 
+static enum {
+	SUBMODULE_CONFIG_NOT_READ = 0,
+	SUBMODULE_CONFIG_NO_CONFIG,
+	SUBMODULE_CONFIG_EXISTS,
+} submodule_config_reading;
+
 /*
  * The following flag is set if the .gitmodules file is unmerged. We then
  * disable recursion for all submodules where .git/config doesn't have a
@@ -83,6 +89,62 @@ int update_path_in_gitmodules(const char *oldpath, const char *newpath)
 	return 0;
 }
 
+static int submodule_config(const char *var, const char *value, void *cb)
+{
+	if (!strcmp(var, "submodule.fetchjobs")) {
+		submodule_config_reading = SUBMODULE_CONFIG_EXISTS;
+		parallel_jobs = git_config_int(var, value);
+		if (parallel_jobs < 0)
+			die(_("negative values not allowed for submodule.fetchJobs"));
+		return 0;
+	} else if (starts_with(var, "submodule.")) {
+		submodule_config_reading = SUBMODULE_CONFIG_EXISTS;
+		return parse_submodule_config_option(var, value);
+	} else if (!strcmp(var, "fetch.recursesubmodules")) {
+		submodule_config_reading = SUBMODULE_CONFIG_EXISTS;
+		config_fetch_recurse_submodules = parse_fetch_recurse_submodules_arg(var, value);
+		return 0;
+	}
+	return 0;
+}
+
+void load_submodule_config(void)
+{
+	submodule_config_reading = SUBMODULE_CONFIG_NO_CONFIG;
+	git_config(submodule_config, NULL);
+}
+
+int has_submodules(unsigned what_to_check)
+{
+	if (what_to_check & SUBMODULE_CHECK_ANY_CONFIG) {
+		if (submodule_config_reading == SUBMODULE_CONFIG_NOT_READ)
+			load_submodule_config();
+		if (submodule_config_reading == SUBMODULE_CONFIG_EXISTS)
+			return 1;
+	}
+
+	if ((what_to_check & SUBMODULE_CHECK_ABSORBED_GIT_DIRS) &&
+	    file_exists(git_path("modules")))
+		return 1;
+
+	if ((what_to_check & SUBMODULE_CHECK_GITMODULES_IN_WT) &&
+	    (!is_bare_repository() && file_exists(".gitmodules")))
+		return 1;
+
+	if (what_to_check & SUBMODULE_CHECK_GITLINKS_IN_TREE) {
+		int i;
+
+		if (read_cache() < 0)
+			die(_("index file corrupt"));
+
+		for (i = 0; i < active_nr; i++)
+			if (S_ISGITLINK(active_cache[i]->ce_mode))
+				return 1;
+	}
+
+	return 0;
+}
+
 /*
  * Try to remove the "submodule.<name>" section from .gitmodules where the given
  * path is configured. Return 0 only if a .gitmodules file was found, a section
@@ -152,22 +214,6 @@ void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,
 	}
 }
 
-int submodule_config(const char *var, const char *value, void *cb)
-{
-	if (!strcmp(var, "submodule.fetchjobs")) {
-		parallel_jobs = git_config_int(var, value);
-		if (parallel_jobs < 0)
-			die(_("negative values not allowed for submodule.fetchJobs"));
-		return 0;
-	} else if (starts_with(var, "submodule."))
-		return parse_submodule_config_option(var, value);
-	else if (!strcmp(var, "fetch.recursesubmodules")) {
-		config_fetch_recurse_submodules = parse_fetch_recurse_submodules_arg(var, value);
-		return 0;
-	}
-	return 0;
-}
-
 void gitmodules_config(void)
 {
 	const char *work_tree = get_git_work_tree();
diff --git a/submodule.h b/submodule.h
index 8a8bc49dc9..5ec72fbb16 100644
--- a/submodule.h
+++ b/submodule.h
@@ -1,6 +1,12 @@
 #ifndef SUBMODULE_H
 #define SUBMODULE_H
 
+#define SUBMODULE_CHECK_ANY_CONFIG		(1<<0)
+#define SUBMODULE_CHECK_ABSORBED_GIT_DIRS	(1<<1)
+#define SUBMODULE_CHECK_GITMODULES_IN_WT	(1<<2)
+#define SUBMODULE_CHECK_GITLINKS_IN_TREE 	(1<<3)
+int has_submodules(unsigned what_to_check);
+
 struct diff_options;
 struct argv_array;
 struct sha1_array;
@@ -37,7 +43,7 @@ extern int remove_path_from_gitmodules(const char *path);
 extern void stage_updated_gitmodules(void);
 extern void set_diffopt_flags_from_submodule_config(struct diff_options *,
 		const char *path);
-extern int submodule_config(const char *var, const char *value, void *cb);
+extern void load_submodule_config(void);
 extern void gitmodules_config(void);
 extern void gitmodules_config_sha1(const unsigned char *commit_sha1);
 extern int is_submodule_initialized(const char *path);
diff --git a/unpack-trees.c b/unpack-trees.c
index 4b3f9518e5..fe42d03a74 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -292,7 +292,7 @@ static void reload_gitmodules_file(struct index_state *index,
 				submodule_free();
 				checkout_entry(ce, state, NULL);
 				gitmodules_config();
-				git_config(submodule_config, NULL);
+				load_submodule_config();
 			} else
 				break;
 		}
-- 
2.13.0.18.g7d86cc8ba0

[PATCH 2/3] submodule test invocation: only pass additional arguments

[Stefan Beller]

In a later patch we want to test a command without the
'--recurse-submodules' given, but instead we'd give a '-c <option>.

To enable that we'll just pass the minimum required to the submodule
testing, such that we can construct the command with the option easier.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 t/lib-submodule-update.sh      | 6 ++++--
 t/t1013-read-tree-submodule.sh | 4 ++--
 t/t2013-checkout-submodule.sh  | 4 ++--
 t/t7112-reset-submodule.sh     | 4 ++--
 4 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index f0b1b18206..0f70b5ec7b 100755
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -782,7 +782,8 @@ test_submodule_forced_switch () {
 #   git directory first into the superproject.
 
 test_submodule_switch_recursing () {
-	command="$1"
+	cmd_args="$1"
+	command="git $cmd_args --recurse-submodules"
 	RESULTDS=success
 	if test "$KNOWN_FAILURE_DIRECTORY_SUBMODULE_CONFLICTS" = 1
 	then
@@ -1022,7 +1023,8 @@ test_submodule_switch_recursing () {
 # that change a submodule, but throwing away local changes in
 # the superproject as well as the submodule is allowed.
 test_submodule_forced_switch_recursing () {
-	command="$1"
+	cmd_args="$1"
+	command="git $cmd_args --recurse-submodules"
 	RESULT=success
 	if test "$KNOWN_FAILURE_DIRECTORY_SUBMODULE_CONFLICTS" = 1
 	then
diff --git a/t/t1013-read-tree-submodule.sh b/t/t1013-read-tree-submodule.sh
index de1ba02dc5..a779e6917c 100755
--- a/t/t1013-read-tree-submodule.sh
+++ b/t/t1013-read-tree-submodule.sh
@@ -9,9 +9,9 @@ KNOWN_FAILURE_SUBMODULE_RECURSIVE_NESTED=1
 KNOWN_FAILURE_DIRECTORY_SUBMODULE_CONFLICTS=1
 KNOWN_FAILURE_SUBMODULE_OVERWRITE_IGNORED_UNTRACKED=1
 
-test_submodule_switch_recursing "git read-tree --recurse-submodules -u -m"
+test_submodule_switch_recursing "read-tree -u -m"
 
-test_submodule_forced_switch_recursing "git read-tree --recurse-submodules -u --reset"
+test_submodule_forced_switch_recursing "read-tree -u --reset"
 
 test_submodule_switch "git read-tree -u -m"
 
diff --git a/t/t2013-checkout-submodule.sh b/t/t2013-checkout-submodule.sh
index e8f70b806f..2672f104cf 100755
--- a/t/t2013-checkout-submodule.sh
+++ b/t/t2013-checkout-submodule.sh
@@ -65,9 +65,9 @@ test_expect_success '"checkout <submodule>" honors submodule.*.ignore from .git/
 
 KNOWN_FAILURE_DIRECTORY_SUBMODULE_CONFLICTS=1
 KNOWN_FAILURE_SUBMODULE_RECURSIVE_NESTED=1
-test_submodule_switch_recursing "git checkout --recurse-submodules"
+test_submodule_switch_recursing "checkout"
 
-test_submodule_forced_switch_recursing "git checkout -f --recurse-submodules"
+test_submodule_forced_switch_recursing "checkout -f"
 
 test_submodule_switch "git checkout"
 
diff --git a/t/t7112-reset-submodule.sh b/t/t7112-reset-submodule.sh
index f86ccdf215..a000304221 100755
--- a/t/t7112-reset-submodule.sh
+++ b/t/t7112-reset-submodule.sh
@@ -9,9 +9,9 @@ KNOWN_FAILURE_SUBMODULE_RECURSIVE_NESTED=1
 KNOWN_FAILURE_DIRECTORY_SUBMODULE_CONFLICTS=1
 KNOWN_FAILURE_SUBMODULE_OVERWRITE_IGNORED_UNTRACKED=1
 
-test_submodule_switch_recursing "git reset --recurse-submodules --keep"
+test_submodule_switch_recursing "reset --keep"
 
-test_submodule_forced_switch_recursing "git reset --hard --recurse-submodules"
+test_submodule_forced_switch_recursing "reset --hard"
 
 test_submodule_switch "git reset --keep"
 
-- 
2.13.0.18.g7d86cc8ba0

[PATCH 3/3] Introduce submodule.recurse option

[Stefan Beller]

Any command that understands the boolean --recurse-submodule=[true/false]
can have its default changed to true, by setting the submodule.recurse
option.

git-push takes a --recurse-submodule argument but it is not boolean,
hence it is not included (yet?).

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 Documentation/config.txt    |  5 +++++
 builtin/checkout.c          |  8 +++-----
 submodule.c                 |  7 ++++++-
 t/lib-submodule-update.sh   | 12 ++++++++++++
 t/t5526-fetch-submodules.sh | 10 ++++++++++
 5 files changed, 36 insertions(+), 6 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 475e874d51..e367becf72 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -3063,6 +3063,11 @@ submodule.active::
 	submodule's path to determine if the submodule is of interest to git
 	commands.
 
+submodule.recurse::
+	Specifies if commands recurse into submodules by default. This
+	applies to all commands that have a `--recurse-submodules` option.
+	Defaults to false.
+
 submodule.fetchJobs::
 	Specifies how many submodules are fetched/cloned at the same time.
 	A positive integer allows up to that number of submodules fetched
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 2787b343b1..e4bd93c9cd 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1195,6 +1195,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 	opts.show_progress = -1;
 
 	gitmodules_config();
+	load_submodule_config();
 	git_config(git_checkout_config, &opts);
 
 	opts.track = BRANCH_TRACK_UNSPECIFIED;
@@ -1214,11 +1215,8 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 		git_xmerge_config("merge.conflictstyle", conflict_style, NULL);
 	}
 
-	if (recurse_submodules != RECURSE_SUBMODULES_OFF) {
-		load_submodule_config();
-		if (recurse_submodules != RECURSE_SUBMODULES_DEFAULT)
-			set_config_update_recurse_submodules(recurse_submodules);
-	}
+	if (recurse_submodules != RECURSE_SUBMODULES_DEFAULT)
+		set_config_update_recurse_submodules(recurse_submodules);
 
 	if ((!!opts.new_branch + !!opts.new_branch_force + !!opts.new_orphan_branch) > 1)
 		die(_("-b, -B and --orphan are mutually exclusive"));
diff --git a/submodule.c b/submodule.c
index 14ea405048..000060a253 100644
--- a/submodule.c
+++ b/submodule.c
@@ -91,7 +91,12 @@ int update_path_in_gitmodules(const char *oldpath, const char *newpath)
 
 static int submodule_config(const char *var, const char *value, void *cb)
 {
-	if (!strcmp(var, "submodule.fetchjobs")) {
+	if (!strcmp(var, "submodule.recurse")) {
+		int v = git_config_bool(var, value) ?
+			RECURSE_SUBMODULES_ON : RECURSE_SUBMODULES_OFF;
+		config_fetch_recurse_submodules = v;
+		config_update_recurse_submodules = v;
+	} else if (!strcmp(var, "submodule.fetchjobs")) {
 		submodule_config_reading = SUBMODULE_CONFIG_EXISTS;
 		parallel_jobs = git_config_int(var, value);
 		if (parallel_jobs < 0)
diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index 0f70b5ec7b..b30164339e 100755
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -990,6 +990,18 @@ test_submodule_switch_recursing () {
 		)
 	'
 
+	test_expect_success "git -c submodule.recurse=true $cmd_args --recurse-submodules: modified submodule updates submodule work tree" '
+		prolog &&
+		reset_work_tree_to_interested add_sub1 &&
+		(
+			cd submodule_update &&
+			git branch -t modify_sub1 origin/modify_sub1 &&
+			git -c submodule.recurse=true $cmd_args --recurse-submodules modify_sub1 &&
+			test_superproject_content origin/modify_sub1 &&
+			test_submodule_content sub1 origin/modify_sub1
+		)
+	'
+
 	# Updating a submodule to an invalid sha1 doesn't update the
 	# superproject nor the submodule's work tree.
 	test_expect_success "$command: updating to a missing submodule commit fails" '
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index f3b0a8d30a..162baf101f 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -71,6 +71,16 @@ test_expect_success "fetch --recurse-submodules recurses into submodules" '
 	test_i18ncmp expect.err actual.err
 '
 
+test_expect_success "submodule.recurse option triggers recursive fetch" '
+	add_upstream_commit &&
+	(
+		cd downstream &&
+		git -c submodule.recurse fetch >../actual.out 2>../actual.err
+	) &&
+	test_must_be_empty actual.out &&
+	test_i18ncmp expect.err actual.err
+'
+
 test_expect_success "fetch --recurse-submodules -j2 has the same output behaviour" '
 	add_upstream_commit &&
 	(
-- 
2.13.0.18.g7d86cc8ba0

Re: [PATCH 1/3] submodule.c: add has_submodules to check if we have any submodules

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 4ef7a08afc..510ef1c9de 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -1344,7 +1344,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
>  			set_config_fetch_recurse_submodules(arg);
>  		}
>  		gitmodules_config();
> -		git_config(submodule_config, NULL);
> +		load_submodule_config();
>  	}
>  
> ...
> +static enum {
> +	SUBMODULE_CONFIG_NOT_READ = 0,
> +	SUBMODULE_CONFIG_NO_CONFIG,
> +	SUBMODULE_CONFIG_EXISTS,
> +} submodule_config_reading;
> +
>  /*
>   * The following flag is set if the .gitmodules file is unmerged. We then
>   * disable recursion for all submodules where .git/config doesn't have a
> @@ -83,6 +89,62 @@ int update_path_in_gitmodules(const char *oldpath, const char *newpath)
>  	return 0;
>  }
>  
> +static int submodule_config(const char *var, const char *value, void *cb)
> +{
> +	if (!strcmp(var, "submodule.fetchjobs")) {
> +...
> +	}
> +	return 0;
> +}
> +
> +void load_submodule_config(void)
> +{
> +	submodule_config_reading = SUBMODULE_CONFIG_NO_CONFIG;
> +	git_config(submodule_config, NULL);
> +}

OK, so anybody who does the git_config(submodule_config) must
instead call this one, so that we can notice there is some
"submodule" stuff configured.  And that is ensured by making
submodule_config private to this module.

Nicely done.

On a possibly related tangent, I've often found it somewhat
irritating that that these two calls have to go hand-in-hand.

>  		gitmodules_config();
> -		git_config(submodule_config, NULL);

I wonder if it makes sense to roll that gitmodule_config() thing
into this function as well?  

Or do some callsites of load_submodule_config() need to omit call to
gitmodules_config()?  If so please disregard.

Thanks.

Re: [PATCH 1/3] submodule.c: add has_submodules to check if we have any submodules

[Brandon Williams]

On 05/17, Stefan Beller wrote:
> When submodules are involved, it often slows down the process, as most
> submodule related handling is either done via a child process or by
> iterating over the index finding all gitlinks.
> 
> For most commands that may interact with submodules, we need have a
> quick check if we do have any submodules at all, such that we can
> be fast in the case when no submodules are in use.  For this quick
> check introduce a function that checks with different heuristics if
> we do have submodules around, checking if
> * anything related to submodules is configured,
> * absorbed git dirs for submodules are present,
> * the '.gitmodules' file exists
> * gitlinks are recorded in the index.
> 
> Each heuristic has advantages and disadvantages.
> For example in a later patch, when we first use this function in
> git-clone, we'll just check for the existence of the '.gitmodules'
> file, because at the time of running the clone command there will
> be no absorbed git dirs or submodule configuration around.
> 
> Checking for any configuration related to submodules would be useful
> in a later stage (after cloning) to see if the submodules are actually
> in use.
> 
> Checking for absorbed git directories is good to see if the user has
> actually cloned submodules already (i.e. not just initialized them by
> configuring them).
> 
> The heuristic for checking the configuration requires this patch
> to have have a global state, whether the submodule config has already
> been read, and if there were any submodule related keys. Make
> 'submodule_config' private to the submodule code, and introduce
> 'load_submodule_config' that will take care of this global state.
> 
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  builtin/checkout.c          |  2 +-
>  builtin/fetch.c             |  2 +-
>  builtin/read-tree.c         |  2 +-
>  builtin/reset.c             |  2 +-
>  builtin/submodule--helper.c |  6 ++--
>  submodule.c                 | 78 +++++++++++++++++++++++++++++++++++----------
>  submodule.h                 |  8 ++++-
>  unpack-trees.c              |  2 +-
>  8 files changed, 77 insertions(+), 25 deletions(-)
> 
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index bfa5419f33..2787b343b1 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -1215,7 +1215,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
>  	}
>  
>  	if (recurse_submodules != RECURSE_SUBMODULES_OFF) {
> -		git_config(submodule_config, NULL);
> +		load_submodule_config();
>  		if (recurse_submodules != RECURSE_SUBMODULES_DEFAULT)
>  			set_config_update_recurse_submodules(recurse_submodules);
>  	}
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 4ef7a08afc..510ef1c9de 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -1344,7 +1344,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
>  			set_config_fetch_recurse_submodules(arg);
>  		}
>  		gitmodules_config();
> -		git_config(submodule_config, NULL);
> +		load_submodule_config();
>  	}
>  
>  	if (all) {
> diff --git a/builtin/read-tree.c b/builtin/read-tree.c
> index 23e212ee8c..fe2ec60a51 100644
> --- a/builtin/read-tree.c
> +++ b/builtin/read-tree.c
> @@ -177,7 +177,7 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
>  
>  	if (recurse_submodules != RECURSE_SUBMODULES_DEFAULT) {
>  		gitmodules_config();
> -		git_config(submodule_config, NULL);
> +		load_submodule_config();
>  		set_config_update_recurse_submodules(RECURSE_SUBMODULES_ON);
>  	}
>  
> diff --git a/builtin/reset.c b/builtin/reset.c
> index 5ce27fcaed..f3aca487d9 100644
> --- a/builtin/reset.c
> +++ b/builtin/reset.c
> @@ -321,7 +321,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
>  
>  	if (recurse_submodules != RECURSE_SUBMODULES_DEFAULT) {
>  		gitmodules_config();
> -		git_config(submodule_config, NULL);
> +		load_submodule_config();
>  		set_config_update_recurse_submodules(RECURSE_SUBMODULES_ON);
>  	}
>  
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 85aafe46a4..770a95ca14 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -1015,7 +1015,7 @@ static int update_clone(int argc, const char **argv, const char *prefix)
>  
>  	/* Overlay the parsed .gitmodules file with .git/config */
>  	gitmodules_config();
> -	git_config(submodule_config, NULL);
> +	load_submodule_config();
>  
>  	if (max_jobs < 0)
>  		max_jobs = parallel_submodules();
> @@ -1058,7 +1058,7 @@ static const char *remote_submodule_branch(const char *path)
>  {
>  	const struct submodule *sub;
>  	gitmodules_config();
> -	git_config(submodule_config, NULL);
> +	load_submodule_config();
>  
>  	sub = submodule_from_path(null_sha1, path);
>  	if (!sub)
> @@ -1130,7 +1130,7 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)
>  			     git_submodule_helper_usage, 0);
>  
>  	gitmodules_config();
> -	git_config(submodule_config, NULL);
> +	load_submodule_config();
>  
>  	if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)
>  		return 1;
> diff --git a/submodule.c b/submodule.c
> index 20ed5b5681..14ea405048 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -24,6 +24,12 @@ static int initialized_fetch_ref_tips;
>  static struct sha1_array ref_tips_before_fetch;
>  static struct sha1_array ref_tips_after_fetch;
>  
> +static enum {
> +	SUBMODULE_CONFIG_NOT_READ = 0,
> +	SUBMODULE_CONFIG_NO_CONFIG,
> +	SUBMODULE_CONFIG_EXISTS,
> +} submodule_config_reading;

Any way we can have this not be a global, but rather a parameter?  You
could pass in a pointer to this value via the callback data parameter in
the submodule_config function.

> +
>  /*
>   * The following flag is set if the .gitmodules file is unmerged. We then
>   * disable recursion for all submodules where .git/config doesn't have a
> @@ -83,6 +89,62 @@ int update_path_in_gitmodules(const char *oldpath, const char *newpath)
>  	return 0;
>  }
>  
> +static int submodule_config(const char *var, const char *value, void *cb)
> +{
> +	if (!strcmp(var, "submodule.fetchjobs")) {
> +		submodule_config_reading = SUBMODULE_CONFIG_EXISTS;
> +		parallel_jobs = git_config_int(var, value);
> +		if (parallel_jobs < 0)
> +			die(_("negative values not allowed for submodule.fetchJobs"));
> +		return 0;
> +	} else if (starts_with(var, "submodule.")) {
> +		submodule_config_reading = SUBMODULE_CONFIG_EXISTS;
> +		return parse_submodule_config_option(var, value);
> +	} else if (!strcmp(var, "fetch.recursesubmodules")) {
> +		submodule_config_reading = SUBMODULE_CONFIG_EXISTS;
> +		config_fetch_recurse_submodules = parse_fetch_recurse_submodules_arg(var, value);
> +		return 0;
> +	}
> +	return 0;
> +}
> +
> +void load_submodule_config(void)
> +{
> +	submodule_config_reading = SUBMODULE_CONFIG_NO_CONFIG;
> +	git_config(submodule_config, NULL);
> +}
> +
> +int has_submodules(unsigned what_to_check)
> +{
> +	if (what_to_check & SUBMODULE_CHECK_ANY_CONFIG) {
> +		if (submodule_config_reading == SUBMODULE_CONFIG_NOT_READ)
> +			load_submodule_config();
> +		if (submodule_config_reading == SUBMODULE_CONFIG_EXISTS)
> +			return 1;
> +	}
> +
> +	if ((what_to_check & SUBMODULE_CHECK_ABSORBED_GIT_DIRS) &&
> +	    file_exists(git_path("modules")))
> +		return 1;
> +
> +	if ((what_to_check & SUBMODULE_CHECK_GITMODULES_IN_WT) &&
> +	    (!is_bare_repository() && file_exists(".gitmodules")))
> +		return 1;
> +
> +	if (what_to_check & SUBMODULE_CHECK_GITLINKS_IN_TREE) {
> +		int i;
> +
> +		if (read_cache() < 0)
> +			die(_("index file corrupt"));
> +
> +		for (i = 0; i < active_nr; i++)
> +			if (S_ISGITLINK(active_cache[i]->ce_mode))
> +				return 1;
> +	}
> +
> +	return 0;
> +}
> +
>  /*
>   * Try to remove the "submodule.<name>" section from .gitmodules where the given
>   * path is configured. Return 0 only if a .gitmodules file was found, a section
> @@ -152,22 +214,6 @@ void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,
>  	}
>  }
>  
> -int submodule_config(const char *var, const char *value, void *cb)
> -{
> -	if (!strcmp(var, "submodule.fetchjobs")) {
> -		parallel_jobs = git_config_int(var, value);
> -		if (parallel_jobs < 0)
> -			die(_("negative values not allowed for submodule.fetchJobs"));
> -		return 0;
> -	} else if (starts_with(var, "submodule."))
> -		return parse_submodule_config_option(var, value);
> -	else if (!strcmp(var, "fetch.recursesubmodules")) {
> -		config_fetch_recurse_submodules = parse_fetch_recurse_submodules_arg(var, value);
> -		return 0;
> -	}
> -	return 0;
> -}
> -
>  void gitmodules_config(void)
>  {
>  	const char *work_tree = get_git_work_tree();
> diff --git a/submodule.h b/submodule.h
> index 8a8bc49dc9..5ec72fbb16 100644
> --- a/submodule.h
> +++ b/submodule.h
> @@ -1,6 +1,12 @@
>  #ifndef SUBMODULE_H
>  #define SUBMODULE_H
>  
> +#define SUBMODULE_CHECK_ANY_CONFIG		(1<<0)
> +#define SUBMODULE_CHECK_ABSORBED_GIT_DIRS	(1<<1)
> +#define SUBMODULE_CHECK_GITMODULES_IN_WT	(1<<2)
> +#define SUBMODULE_CHECK_GITLINKS_IN_TREE 	(1<<3)
> +int has_submodules(unsigned what_to_check);
> +
>  struct diff_options;
>  struct argv_array;
>  struct sha1_array;
> @@ -37,7 +43,7 @@ extern int remove_path_from_gitmodules(const char *path);
>  extern void stage_updated_gitmodules(void);
>  extern void set_diffopt_flags_from_submodule_config(struct diff_options *,
>  		const char *path);
> -extern int submodule_config(const char *var, const char *value, void *cb);
> +extern void load_submodule_config(void);
>  extern void gitmodules_config(void);
>  extern void gitmodules_config_sha1(const unsigned char *commit_sha1);
>  extern int is_submodule_initialized(const char *path);
> diff --git a/unpack-trees.c b/unpack-trees.c
> index 4b3f9518e5..fe42d03a74 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -292,7 +292,7 @@ static void reload_gitmodules_file(struct index_state *index,
>  				submodule_free();
>  				checkout_entry(ce, state, NULL);
>  				gitmodules_config();
> -				git_config(submodule_config, NULL);
> +				load_submodule_config();
>  			} else
>  				break;
>  		}
> -- 
> 2.13.0.18.g7d86cc8ba0
> 

-- 
Brandon Williams

Re: [PATCH 3/3] Introduce submodule.recurse option

[Brandon Williams]

On 05/17, Stefan Beller wrote:
> Any command that understands the boolean --recurse-submodule=[true/false]
> can have its default changed to true, by setting the submodule.recurse
> option.
> 
> git-push takes a --recurse-submodule argument but it is not boolean,
> hence it is not included (yet?).

ls-files and grep could also be added.  They don't need to be added in
this patch though.

> 
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  Documentation/config.txt    |  5 +++++
>  builtin/checkout.c          |  8 +++-----
>  submodule.c                 |  7 ++++++-
>  t/lib-submodule-update.sh   | 12 ++++++++++++
>  t/t5526-fetch-submodules.sh | 10 ++++++++++
>  5 files changed, 36 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 475e874d51..e367becf72 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -3063,6 +3063,11 @@ submodule.active::
>  	submodule's path to determine if the submodule is of interest to git
>  	commands.
>  
> +submodule.recurse::
> +	Specifies if commands recurse into submodules by default. This
> +	applies to all commands that have a `--recurse-submodules` option.
> +	Defaults to false.
> +
>  submodule.fetchJobs::
>  	Specifies how many submodules are fetched/cloned at the same time.
>  	A positive integer allows up to that number of submodules fetched
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 2787b343b1..e4bd93c9cd 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -1195,6 +1195,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
>  	opts.show_progress = -1;
>  
>  	gitmodules_config();
> +	load_submodule_config();
>  	git_config(git_checkout_config, &opts);
>  
>  	opts.track = BRANCH_TRACK_UNSPECIFIED;
> @@ -1214,11 +1215,8 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
>  		git_xmerge_config("merge.conflictstyle", conflict_style, NULL);
>  	}
>  
> -	if (recurse_submodules != RECURSE_SUBMODULES_OFF) {
> -		load_submodule_config();
> -		if (recurse_submodules != RECURSE_SUBMODULES_DEFAULT)
> -			set_config_update_recurse_submodules(recurse_submodules);
> -	}
> +	if (recurse_submodules != RECURSE_SUBMODULES_DEFAULT)
> +		set_config_update_recurse_submodules(recurse_submodules);
>  
>  	if ((!!opts.new_branch + !!opts.new_branch_force + !!opts.new_orphan_branch) > 1)
>  		die(_("-b, -B and --orphan are mutually exclusive"));
> diff --git a/submodule.c b/submodule.c
> index 14ea405048..000060a253 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -91,7 +91,12 @@ int update_path_in_gitmodules(const char *oldpath, const char *newpath)
>  
>  static int submodule_config(const char *var, const char *value, void *cb)
>  {
> -	if (!strcmp(var, "submodule.fetchjobs")) {
> +	if (!strcmp(var, "submodule.recurse")) {
> +		int v = git_config_bool(var, value) ?
> +			RECURSE_SUBMODULES_ON : RECURSE_SUBMODULES_OFF;
> +		config_fetch_recurse_submodules = v;
> +		config_update_recurse_submodules = v;
> +	} else if (!strcmp(var, "submodule.fetchjobs")) {
>  		submodule_config_reading = SUBMODULE_CONFIG_EXISTS;
>  		parallel_jobs = git_config_int(var, value);
>  		if (parallel_jobs < 0)
> diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
> index 0f70b5ec7b..b30164339e 100755
> --- a/t/lib-submodule-update.sh
> +++ b/t/lib-submodule-update.sh
> @@ -990,6 +990,18 @@ test_submodule_switch_recursing () {
>  		)
>  	'
>  
> +	test_expect_success "git -c submodule.recurse=true $cmd_args --recurse-submodules: modified submodule updates submodule work tree" '
> +		prolog &&
> +		reset_work_tree_to_interested add_sub1 &&
> +		(
> +			cd submodule_update &&
> +			git branch -t modify_sub1 origin/modify_sub1 &&
> +			git -c submodule.recurse=true $cmd_args --recurse-submodules modify_sub1 &&
> +			test_superproject_content origin/modify_sub1 &&
> +			test_submodule_content sub1 origin/modify_sub1
> +		)
> +	'
> +
>  	# Updating a submodule to an invalid sha1 doesn't update the
>  	# superproject nor the submodule's work tree.
>  	test_expect_success "$command: updating to a missing submodule commit fails" '
> diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
> index f3b0a8d30a..162baf101f 100755
> --- a/t/t5526-fetch-submodules.sh
> +++ b/t/t5526-fetch-submodules.sh
> @@ -71,6 +71,16 @@ test_expect_success "fetch --recurse-submodules recurses into submodules" '
>  	test_i18ncmp expect.err actual.err
>  '
>  
> +test_expect_success "submodule.recurse option triggers recursive fetch" '
> +	add_upstream_commit &&
> +	(
> +		cd downstream &&
> +		git -c submodule.recurse fetch >../actual.out 2>../actual.err
> +	) &&
> +	test_must_be_empty actual.out &&
> +	test_i18ncmp expect.err actual.err
> +'
> +
>  test_expect_success "fetch --recurse-submodules -j2 has the same output behaviour" '
>  	add_upstream_commit &&
>  	(
> -- 
> 2.13.0.18.g7d86cc8ba0
> 

-- 
Brandon Williams

Re: [PATCH 3/3] Introduce submodule.recurse option

[Stefan Beller]

On Thu, May 18, 2017 at 8:39 AM, Brandon Williams <bmwill@google.com> wrote:
> On 05/17, Stefan Beller wrote:
>> Any command that understands the boolean --recurse-submodule=[true/false]
>> can have its default changed to true, by setting the submodule.recurse
>> option.
>>
>> git-push takes a --recurse-submodule argument but it is not boolean,
>> hence it is not included (yet?).
>
> ls-files and grep could also be added.  They don't need to be added in
> this patch though.

I agree for git-grep.
git-ls-files is a pure plumbing command, which usually are not very accepting
of options. Then it is easier for script writers to rely on its expected output.

I just missed those two (at least grep) and will add them in this
patch if we think
this series is a good idea going forward.

Thanks,
Stefan

Re: [PATCH 1/3] submodule.c: add has_submodules to check if we have any submodules

[Stefan Beller]

On Wed, May 17, 2017 at 10:38 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> diff --git a/builtin/fetch.c b/builtin/fetch.c
>> index 4ef7a08afc..510ef1c9de 100644
>> --- a/builtin/fetch.c
>> +++ b/builtin/fetch.c
>> @@ -1344,7 +1344,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
>>                       set_config_fetch_recurse_submodules(arg);
>>               }
>>               gitmodules_config();
>> -             git_config(submodule_config, NULL);
>> +             load_submodule_config();
>>       }
>>
>> ...
>> +static enum {
>> +     SUBMODULE_CONFIG_NOT_READ = 0,
>> +     SUBMODULE_CONFIG_NO_CONFIG,
>> +     SUBMODULE_CONFIG_EXISTS,
>> +} submodule_config_reading;
>> +
>>  /*
>>   * The following flag is set if the .gitmodules file is unmerged. We then
>>   * disable recursion for all submodules where .git/config doesn't have a
>> @@ -83,6 +89,62 @@ int update_path_in_gitmodules(const char *oldpath, const char *newpath)
>>       return 0;
>>  }
>>
>> +static int submodule_config(const char *var, const char *value, void *cb)
>> +{
>> +     if (!strcmp(var, "submodule.fetchjobs")) {
>> +...
>> +     }
>> +     return 0;
>> +}
>> +
>> +void load_submodule_config(void)
>> +{
>> +     submodule_config_reading = SUBMODULE_CONFIG_NO_CONFIG;
>> +     git_config(submodule_config, NULL);
>> +}
>
> OK, so anybody who does the git_config(submodule_config) must
> instead call this one, so that we can notice there is some
> "submodule" stuff configured.  And that is ensured by making
> submodule_config private to this module.

Exactly.

> Nicely done.

Thanks.

>
> On a possibly related tangent, I've often found it somewhat
> irritating that that these two calls have to go hand-in-hand.
>
>>               gitmodules_config();
>> -             git_config(submodule_config, NULL);
>
> I wonder if it makes sense to roll that gitmodule_config() thing
> into this function as well?

This patch has been sitting on my hard drive for quite a while
and I just vaguely remember that I did not do that for some
complication along the way. I'll re-examine the situation.

Thanks,
Stefan

Re: [PATCH 1/3] submodule.c: add has_submodules to check if we have any submodules

[Stefan Beller]

>> +static enum {
>> +     SUBMODULE_CONFIG_NOT_READ = 0,
>> +     SUBMODULE_CONFIG_NO_CONFIG,
>> +     SUBMODULE_CONFIG_EXISTS,
>> +} submodule_config_reading;
>
> Any way we can have this not be a global, but rather a parameter?  You
> could pass in a pointer to this value via the callback data parameter in
> the submodule_config function.

As said in the reply to Junio, this patch has been sitting on my hard drive
for a while and was written before you started the attempt to de-globalize
the state of git.

Ideally this setting would be part of the repository object. For example
the repository object would have a "submodule_config" pointer, initialized
to NULL, which can then be set to the read config or a static empty_config
if no such config exists.

Re: [PATCH 1/3] submodule.c: add has_submodules to check if we have any submodules

[Brandon Williams]

On 05/18, Stefan Beller wrote:
> >> +static enum {
> >> +     SUBMODULE_CONFIG_NOT_READ = 0,
> >> +     SUBMODULE_CONFIG_NO_CONFIG,
> >> +     SUBMODULE_CONFIG_EXISTS,
> >> +} submodule_config_reading;
> >
> > Any way we can have this not be a global, but rather a parameter?  You
> > could pass in a pointer to this value via the callback data parameter in
> > the submodule_config function.
> 
> As said in the reply to Junio, this patch has been sitting on my hard drive
> for a while and was written before you started the attempt to de-globalize
> the state of git.
> 
> Ideally this setting would be part of the repository object. For example
> the repository object would have a "submodule_config" pointer, initialized
> to NULL, which can then be set to the read config or a static empty_config
> if no such config exists.

I'm not quite sure I agree, or rather we may be talking about two
different things or I'm misinterpreting the patches.  From these patches
it seems like 'submodule_config' that you are refering to is not the
actual submodule configuration but rather some options that are stored
in .git/config or other various config locations (home, system, etc).
What would need to be part of the repository object (and is in my WIP
that I'll hopefully send out so i can get some feedback) would be the
submodule_cache which is the internal representation of a repository's
.gitmodules files.

-- 
Brandon Williams

Re: [PATCH 0/3] Add option to recurse into submodules

[Brandon Williams]

On 05/17, Stefan Beller wrote:
> In a submodule heavy workflow it becomes tedious to pass in
> --recurse-submodules all the time, so make an option for it.

I'm all for adding the config which applies to multiple commands.  I
think its probably necessary as we increase the number of commands which
can recurse.

I'm not as thrilled over the first patch in the series though.  Least
that's my first impression.  I probably need think about it a bit more
though.

> 
> Thanks,
> Stefan
> 
> Stefan Beller (3):
>   submodule.c: add has_submodules to check if we have any submodules
>   submodule test invocation: only pass additional arguments
>   Introduce submodule.recurse option
> 
>  Documentation/config.txt       |  5 +++
>  builtin/checkout.c             |  8 ++--
>  builtin/fetch.c                |  2 +-
>  builtin/read-tree.c            |  2 +-
>  builtin/reset.c                |  2 +-
>  builtin/submodule--helper.c    |  6 +--
>  submodule.c                    | 83 ++++++++++++++++++++++++++++++++++--------
>  submodule.h                    |  8 +++-
>  t/lib-submodule-update.sh      | 18 ++++++++-
>  t/t1013-read-tree-submodule.sh |  4 +-
>  t/t2013-checkout-submodule.sh  |  4 +-
>  t/t5526-fetch-submodules.sh    | 10 +++++
>  t/t7112-reset-submodule.sh     |  4 +-
>  unpack-trees.c                 |  2 +-
>  14 files changed, 121 insertions(+), 37 deletions(-)
> 
> -- 
> 2.13.0.18.g7d86cc8ba0
> 

-- 
Brandon Williams

Re: [PATCH 1/3] submodule.c: add has_submodules to check if we have any submodules

[Stefan Beller]

On Thu, May 18, 2017 at 12:00 PM, Brandon Williams <bmwill@google.com> wrote:
> On 05/18, Stefan Beller wrote:
>> >> +static enum {
>> >> +     SUBMODULE_CONFIG_NOT_READ = 0,
>> >> +     SUBMODULE_CONFIG_NO_CONFIG,
>> >> +     SUBMODULE_CONFIG_EXISTS,
>> >> +} submodule_config_reading;
>> >
>> > Any way we can have this not be a global, but rather a parameter?  You
>> > could pass in a pointer to this value via the callback data parameter in
>> > the submodule_config function.
>>
>> As said in the reply to Junio, this patch has been sitting on my hard drive
>> for a while and was written before you started the attempt to de-globalize
>> the state of git.
>>
>> Ideally this setting would be part of the repository object. For example
>> the repository object would have a "submodule_config" pointer, initialized
>> to NULL, which can then be set to the read config or a static empty_config
>> if no such config exists.
>
> I'm not quite sure I agree, or rather we may be talking about two
> different things or I'm misinterpreting the patches.  From these patches
> it seems like 'submodule_config' that you are refering to is not the
> actual submodule configuration but rather some options that are stored
> in .git/config or other various config locations (home, system, etc).

You are reading the patch correctly.

> What would need to be part of the repository object (and is in my WIP
> that I'll hopefully send out so i can get some feedback) would be the
> submodule_cache which is the internal representation of a repository's
> .gitmodules files.

and in the light of this patch, we'd want to have a cache the flag if the
regular config contains any submodule related things, so for now a static
seems to be the best option and once we have the repo object we'd
have a bit from the flags section (assuming we'll have a flags
section down the road):

struct repo {
    ....
    struct regular_config *config_ptr;
    unsigned config_has_submodule_stuff : 1;
    unsigned config_loaded_submodule_config : 1;
}

Then the caching decision would be:

static int submodule_config(const char *var, const char *value, void *cb)
{
       struct repo *r = cb;
       if (!strcmp(var, "submodule.fetchjobs")) {
               r->config_has_submodule_stuff = 1;
               parallel_jobs = git_config_int(var, value);
               if (parallel_jobs < 0)
                       die(_("negative values not allowed for
submodule.fetchJobs"));
               return 0;
       } else if (starts_with(var, "submodule.")) {
               r->config_has_submodule_stuff = 1;
               return parse_submodule_config_option(var, value);
       } else if (!strcmp(var, "fetch.recursesubmodules")) {
               r->config_has_submodule_stuff = 1;
               config_fetch_recurse_submodules =
parse_fetch_recurse_submodules_arg(var, value);
               return 0;
       }
       return 0;
}

void load_submodule_config(struct repo *r)
{
      # assume r->config_has_submodule_stuff and config_loaded_submodule_config
      # was set to 0 on repo init
      if (r->config_loaded_submodule_config)
            return;

      git_config(submodule_config, r);
      r->config_loaded_submodule_config = 1;
}

That said, I agree that the

>> >> +static enum {
>> >> +     SUBMODULE_CONFIG_NOT_READ = 0,
>> >> +     SUBMODULE_CONFIG_NO_CONFIG,
>> >> +     SUBMODULE_CONFIG_EXISTS,
>> >> +} submodule_config_reading;

with its global state is counterproductive for your series, but I see
an easy integration path. As we do not have the repo struct,
I proposed it this way to make submodule progress.

Thanks,
Stefan