[PATCH 00/15] submodule-config cleanup

[PATCH 00/15] submodule-config cleanup

[Brandon Williams]

The aim of this series is to cleanup the submodule-config and make it simpler
to use.  The two main parts to this series are:
(1) removing the ability to overlay the repository's config over the
    submodule-config.  This makes the API clunky as you don't really know when
    you want to overlay and when you don't.  So instead all the relevant
    sections (where you are interested in the repository's config) are patched
    to read the configuration directly from the repository's config.
(2) Add the ability to lazy-load the gitmodules file from the working
    directory.  Most callers are required to first populate the
    submodule-config by calling gitmodules_config.  Instead let's just
    lazy-load it if needed.  Only a couple callers will still require loading
    the gitmodules files by hand while the rest can have it lazy-loaded and no
    longer need to explicitly load it themselves.  This falls more in line with
    how specific revisions are already lazy-loaded.

As a side note, instead of having unpack-trees read configuration for the
'update' config (which is used by submodule update) we may just want to drop
respecting this all together as it doesn't make much sense in the context of a
checkout or reset.  If that's the case then we can make the parts of the code
which use 'update' even simpler.

This series is built on and requires the 'bw/grep-recurse-submodules' and
'bc/object-id' branches.

Brandon Williams (15):
  t7411: check configuration parsing errors
  submodule: don't use submodule_from_name
  add, reset: ensure submodules can be added or reset
  submodule--helper: don't overlay config in remote_submodule_branch
  submodule--helper: don't overlay config in update-clone
  fetch: don't overlay config with submodule-config
  submodule: don't rely on overlayed config when setting diffopts
  unpack-trees: don't rely on overlayed config
  submodule: remove submodule_config callback routine
  diff: stop allowing diff to have submodules configured in .git/config
  submodule-config: remove support for overlaying repository config
  submodule-config: move submodule-config functions to
    submodule-config.c
  submodule-config: lazy-load a repository's .gitmodules file
  unpack-trees: improve loading of .gitmodules
  submodule: remove gitmodules_config

 builtin/add.c                    |   1 +
 builtin/checkout.c               |   3 +-
 builtin/commit.c                 |   1 -
 builtin/diff-files.c             |   1 -
 builtin/diff-index.c             |   1 -
 builtin/diff-tree.c              |   1 -
 builtin/diff.c                   |   2 -
 builtin/fetch.c                  |   5 --
 builtin/grep.c                   |   4 --
 builtin/ls-files.c               |   6 +-
 builtin/mv.c                     |   1 -
 builtin/read-tree.c              |   2 -
 builtin/reset.c                  |   3 +-
 builtin/rm.c                     |   1 -
 builtin/submodule--helper.c      |  42 ++++++------
 diff.c                           |   3 -
 submodule-config.c               |  65 ++++++++++++++----
 submodule-config.h               |   8 +--
 submodule.c                      | 140 ++++++++++++++++-----------------------
 submodule.h                      |   8 +--
 t/helper/test-submodule-config.c |   7 --
 t/t4027-diff-submodule.sh        |  67 -------------------
 t/t7400-submodule-basic.sh       |  10 ---
 t/t7411-submodule-config.sh      |  87 +++++-------------------
 unpack-trees.c                   |  54 +++++++++------
 25 files changed, 189 insertions(+), 334 deletions(-)

-- 
2.14.0.rc0.400.g1c36432dff-goog

[PATCH 01/15] t7411: check configuration parsing errors

[Brandon Williams]

Check for configuration parsing errors in '.gitmodules' in t7411, which
is explicitly testing the submodule-config subsystem, instead of in
t7400.  Also explicitly use the test helper instead of relying on the
gitmodules file from being read in status.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 t/t7400-submodule-basic.sh  | 10 ----------
 t/t7411-submodule-config.sh | 15 +++++++++++++++
 2 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index dcac364c5..717447526 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -46,16 +46,6 @@ test_expect_success 'submodule update aborts on missing gitmodules url' '
 	test_must_fail git submodule init
 '
 
-test_expect_success 'configuration parsing' '
-	test_when_finished "rm -f .gitmodules" &&
-	cat >.gitmodules <<-\EOF &&
-	[submodule "s"]
-		path
-		ignore
-	EOF
-	test_must_fail git status
-'
-
 test_expect_success 'setup - repository in init subdirectory' '
 	mkdir init &&
 	(
diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
index eea36f1db..7d6b25ba2 100755
--- a/t/t7411-submodule-config.sh
+++ b/t/t7411-submodule-config.sh
@@ -31,6 +31,21 @@ test_expect_success 'submodule config cache setup' '
 	)
 '
 
+test_expect_success 'configuration parsing with error' '
+	test_when_finished "rm -rf repo" &&
+	test_create_repo repo &&
+	cat >repo/.gitmodules <<-\EOF &&
+	[submodule "s"]
+		path
+		ignore
+	EOF
+	(
+		cd repo &&
+		test_must_fail test-submodule-config "" s 2>actual &&
+		test_i18ngrep "bad config" actual
+	)
+'
+
 cat >super/expect <<EOF
 Submodule name: 'a' for path 'a'
 Submodule name: 'a' for path 'b'
-- 
2.14.0.rc0.400.g1c36432dff-goog

[PATCH 02/15] submodule: don't use submodule_from_name

[Brandon Williams]

The function 'submodule_from_name()' is being used incorrectly here as a
submodule path is being used instead of a submodule name.  Since the
correct function to use with a path to a submodule is already being used
('submodule_from_path()') let's remove the call to
'submodule_from_name()'.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 submodule.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/submodule.c b/submodule.c
index 7e87e4698..fd391aea6 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1177,8 +1177,6 @@ static int get_next_submodule(struct child_process *cp,
 			continue;
 
 		submodule = submodule_from_path(&null_oid, ce->name);
-		if (!submodule)
-			submodule = submodule_from_name(&null_oid, ce->name);
 
 		default_argv = "yes";
 		if (spf->command_line_option == RECURSE_SUBMODULES_DEFAULT) {
-- 
2.14.0.rc0.400.g1c36432dff-goog

[PATCH 03/15] add, reset: ensure submodules can be added or reset

[Brandon Williams]

Commit aee9c7d65 (Submodules: Add the new "ignore" config option for
diff and status) introduced the ignore configuration option for
submodules so that configured submodules could be omitted from the
status and diff commands.  Because this flag is respected in the diff
machinery it has the unintended consequence of potentially prohibiting
users from adding or resetting a submodule, even when a path to the
submodule is explicitly given.

Ensure that submodules can be added or set, even if they are configured
to be ignored, by setting the `DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG` diff
flag.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 builtin/add.c   | 1 +
 builtin/reset.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/builtin/add.c b/builtin/add.c
index e888fb8c5..6f271512f 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -116,6 +116,7 @@ int add_files_to_cache(const char *prefix,
 	rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;
 	rev.diffopt.format_callback = update_callback;
 	rev.diffopt.format_callback_data = &data;
+	rev.diffopt.flags |= DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG;
 	rev.max_count = 0; /* do not compare unmerged paths with stage #2 */
 	run_diff_files(&rev, DIFF_RACY_IS_MODIFIED);
 	return !!data.add_errors;
diff --git a/builtin/reset.c b/builtin/reset.c
index 046403ed6..772d078b8 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -156,6 +156,7 @@ static int read_from_tree(const struct pathspec *pathspec,
 	opt.output_format = DIFF_FORMAT_CALLBACK;
 	opt.format_callback = update_index_from_diff;
 	opt.format_callback_data = &intent_to_add;
+	opt.flags |= DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG;
 
 	if (do_diff_cache(tree_oid, &opt))
 		return 1;
-- 
2.14.0.rc0.400.g1c36432dff-goog

[PATCH 04/15] submodule--helper: don't overlay config in remote_submodule_branch

[Brandon Williams]

Don't rely on overlaying the repository's config on top of the
submodule-config, instead query the repository's config directly for the
branch field.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 builtin/submodule--helper.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 1e49ce580..f71f4270d 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1066,17 +1066,24 @@ static int resolve_relative_path(int argc, const char **argv, const char *prefix
 static const char *remote_submodule_branch(const char *path)
 {
 	const struct submodule *sub;
+	const char *branch = NULL;
+	char *key;
+
 	gitmodules_config();
-	git_config(submodule_config, NULL);
 
 	sub = submodule_from_path(&null_oid, path);
 	if (!sub)
 		return NULL;
 
-	if (!sub->branch)
+	key = xstrfmt("submodule.%s.branch", sub->name);
+	if (repo_config_get_string_const(the_repository, key, &branch))
+		branch = sub->branch;
+	free(key);
+
+	if (!branch)
 		return "master";
 
-	if (!strcmp(sub->branch, ".")) {
+	if (!strcmp(branch, ".")) {
 		unsigned char sha1[20];
 		const char *refname = resolve_ref_unsafe("HEAD", 0, sha1, NULL);
 
@@ -1094,7 +1101,7 @@ static const char *remote_submodule_branch(const char *path)
 		return refname;
 	}
 
-	return sub->branch;
+	return branch;
 }
 
 static int resolve_remote_submodule_branch(int argc, const char **argv,
-- 
2.14.0.rc0.400.g1c36432dff-goog

[PATCH 05/15] submodule--helper: don't overlay config in update-clone

[Brandon Williams]

Don't rely on overlaying the repository's config on top of the
submodule-config, instead query the repository's config directly for the
url and the update strategy configuration.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 builtin/submodule--helper.c | 14 ++++++++++----
 submodule.c                 | 30 ++++++++++++++++++++++++++++++
 submodule.h                 |  3 +++
 3 files changed, 43 insertions(+), 4 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index f71f4270d..25f471ba1 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -780,6 +780,8 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 					   struct strbuf *out)
 {
 	const struct submodule *sub = NULL;
+	const char *url = NULL;
+	struct submodule_update_strategy update;
 	struct strbuf displaypath_sb = STRBUF_INIT;
 	struct strbuf sb = STRBUF_INIT;
 	const char *displaypath = NULL;
@@ -808,9 +810,10 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 		goto cleanup;
 	}
 
+	update = submodule_strategy_with_config_overlayed(the_repository, sub);
 	if (suc->update.type == SM_UPDATE_NONE
 	    || (suc->update.type == SM_UPDATE_UNSPECIFIED
-		&& sub->update_strategy.type == SM_UPDATE_NONE)) {
+		&& update.type == SM_UPDATE_NONE)) {
 		strbuf_addf(out, _("Skipping submodule '%s'"), displaypath);
 		strbuf_addch(out, '\n');
 		goto cleanup;
@@ -822,6 +825,11 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 		goto cleanup;
 	}
 
+	strbuf_reset(&sb);
+	strbuf_addf(&sb, "submodule.%s.url", sub->name);
+	if (repo_config_get_string_const(the_repository, sb.buf, &url))
+		url = sub->url;
+
 	strbuf_reset(&sb);
 	strbuf_addf(&sb, "%s/.git", ce->name);
 	needs_cloning = !file_exists(sb.buf);
@@ -851,7 +859,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 		argv_array_push(&child->args, "--depth=1");
 	argv_array_pushl(&child->args, "--path", sub->path, NULL);
 	argv_array_pushl(&child->args, "--name", sub->name, NULL);
-	argv_array_pushl(&child->args, "--url", sub->url, NULL);
+	argv_array_pushl(&child->args, "--url", url, NULL);
 	if (suc->references.nr) {
 		struct string_list_item *item;
 		for_each_string_list_item(item, &suc->references)
@@ -1025,9 +1033,7 @@ static int update_clone(int argc, const char **argv, const char *prefix)
 	if (pathspec.nr)
 		suc.warn_if_uninitialized = 1;
 
-	/* Overlay the parsed .gitmodules file with .git/config */
 	gitmodules_config();
-	git_config(submodule_config, NULL);
 
 	run_processes_parallel(max_jobs,
 			       update_clone_get_next_task,
diff --git a/submodule.c b/submodule.c
index fd391aea6..8b9e48a61 100644
--- a/submodule.c
+++ b/submodule.c
@@ -440,6 +440,36 @@ const char *submodule_strategy_to_string(const struct submodule_update_strategy
 	return NULL;
 }
 
+struct submodule_update_strategy submodule_strategy_with_config_overlayed(struct repository *repo,
+									  const struct submodule *sub)
+{
+	struct submodule_update_strategy strat = sub->update_strategy;
+	const char *update;
+	char *key;
+
+	key = xstrfmt("submodule.%s.update", sub->name);
+	if (!repo_config_get_string_const(repo, key, &update)) {
+		strat.command = NULL;
+		if (!strcmp(update, "none")) {
+			strat.type = SM_UPDATE_NONE;
+		} else if (!strcmp(update, "checkout")) {
+			strat.type = SM_UPDATE_CHECKOUT;
+		} else if (!strcmp(update, "rebase")) {
+			strat.type = SM_UPDATE_REBASE;
+		} else if (!strcmp(update, "merge")) {
+			strat.type = SM_UPDATE_MERGE;
+		} else if (skip_prefix(update, "!", &update)) {
+			strat.type = SM_UPDATE_COMMAND;
+			strat.command = update;
+		} else {
+			die("invalid submodule update strategy '%s'", update);
+		}
+	}
+	free(key);
+
+	return strat;
+}
+
 void handle_ignore_submodules_arg(struct diff_options *diffopt,
 				  const char *arg)
 {
diff --git a/submodule.h b/submodule.h
index e402b004f..f17ca1e34 100644
--- a/submodule.h
+++ b/submodule.h
@@ -6,6 +6,7 @@ struct diff_options;
 struct argv_array;
 struct oid_array;
 struct remote;
+struct submodule;
 
 enum {
 	RECURSE_SUBMODULES_ONLY = -5,
@@ -65,6 +66,8 @@ extern void die_path_inside_submodule(const struct index_state *istate,
 extern int parse_submodule_update_strategy(const char *value,
 		struct submodule_update_strategy *dst);
 extern const char *submodule_strategy_to_string(const struct submodule_update_strategy *s);
+extern struct submodule_update_strategy submodule_strategy_with_config_overlayed(struct repository *repo,
+										 const struct submodule *sub);
 extern void handle_ignore_submodules_arg(struct diff_options *, const char *);
 extern void show_submodule_summary(FILE *f, const char *path,
 		const char *line_prefix,
-- 
2.14.0.rc0.400.g1c36432dff-goog

[PATCH 06/15] fetch: don't overlay config with submodule-config

[Brandon Williams]

Don't rely on overlaying the repository's config on top of the
submodule-config, instead query the repository's config directly for the
fetch_recurse field.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 builtin/fetch.c |  1 -
 submodule.c     | 24 +++++++++++++++++-------
 2 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index d84c26391..3fe99073d 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1362,7 +1362,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 
 	if (recurse_submodules != RECURSE_SUBMODULES_OFF) {
 		gitmodules_config();
-		git_config(submodule_config, NULL);
 	}
 
 	if (all) {
diff --git a/submodule.c b/submodule.c
index 8b9e48a61..c5058a4b8 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1210,14 +1210,24 @@ static int get_next_submodule(struct child_process *cp,
 
 		default_argv = "yes";
 		if (spf->command_line_option == RECURSE_SUBMODULES_DEFAULT) {
-			if (submodule &&
-			    submodule->fetch_recurse !=
-						RECURSE_SUBMODULES_NONE) {
-				if (submodule->fetch_recurse ==
-						RECURSE_SUBMODULES_OFF)
+			int fetch_recurse = RECURSE_SUBMODULES_NONE;
+
+			if (submodule) {
+				char *key;
+				const char *value;
+
+				fetch_recurse = submodule->fetch_recurse;
+				key = xstrfmt("submodule.%s.fetchRecurseSubmodules", submodule->name);
+				if (!repo_config_get_string_const(the_repository, key, &value)) {
+					fetch_recurse = parse_fetch_recurse_submodules_arg(key, value);
+				}
+				free(key);
+			}
+
+			if (fetch_recurse != RECURSE_SUBMODULES_NONE) {
+				if (fetch_recurse == RECURSE_SUBMODULES_OFF)
 					continue;
-				if (submodule->fetch_recurse ==
-						RECURSE_SUBMODULES_ON_DEMAND) {
+				if (fetch_recurse == RECURSE_SUBMODULES_ON_DEMAND) {
 					if (!unsorted_string_list_lookup(&changed_submodule_paths, ce->name))
 						continue;
 					default_argv = "on-demand";
-- 
2.14.0.rc0.400.g1c36432dff-goog

[PATCH 07/15] submodule: don't rely on overlayed config when setting diffopts

[Brandon Williams]

Don't rely on overlaying the repository's config on top of the
submodule-config, instead query the repository's config directory for
the ignore field.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 submodule.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/submodule.c b/submodule.c
index c5058a4b8..f86b82fbb 100644
--- a/submodule.c
+++ b/submodule.c
@@ -165,8 +165,16 @@ void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,
 {
 	const struct submodule *submodule = submodule_from_path(&null_oid, path);
 	if (submodule) {
-		if (submodule->ignore)
-			handle_ignore_submodules_arg(diffopt, submodule->ignore);
+		const char *ignore;
+		char *key;
+
+		key = xstrfmt("submodule.%s.ignore", submodule->name);
+		if (repo_config_get_string_const(the_repository, key, &ignore))
+			ignore = submodule->ignore;
+		free(key);
+
+		if (ignore)
+			handle_ignore_submodules_arg(diffopt, ignore);
 		else if (is_gitmodules_unmerged(&the_index))
 			DIFF_OPT_SET(diffopt, IGNORE_SUBMODULES);
 	}
-- 
2.14.0.rc0.400.g1c36432dff-goog

[PATCH 08/15] unpack-trees: don't rely on overlayed config

[Brandon Williams]

Don't rely on overlaying the repository's config on top of the
submodule-config, instead query the repository's config directory for
the submodule's update strategy.

Also remove the overlaying of the repository's config (via using
'submodule_config()') from the commands which use the unpack-trees
logic (checkout, read-tree, reset).

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 builtin/checkout.c |  2 +-
 submodule.c        |  1 -
 unpack-trees.c     | 12 +++++++++---
 3 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 9661e1bcb..246e0cd16 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -858,7 +858,7 @@ static int git_checkout_config(const char *var, const char *value, void *cb)
 	}
 
 	if (starts_with(var, "submodule."))
-		return submodule_config(var, value, NULL);
+		return git_default_submodule_config(var, value, NULL);
 
 	return git_xmerge_config(var, value, NULL);
 }
diff --git a/submodule.c b/submodule.c
index f86b82fbb..13380fed1 100644
--- a/submodule.c
+++ b/submodule.c
@@ -235,7 +235,6 @@ void load_submodule_cache(void)
 		return;
 
 	gitmodules_config();
-	git_config(submodule_config, NULL);
 }
 
 static int gitmodules_cb(const char *var, const char *value, void *data)
diff --git a/unpack-trees.c b/unpack-trees.c
index dd535bc84..dc66b880d 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1,5 +1,6 @@
 #define NO_THE_INDEX_COMPATIBILITY_MACROS
 #include "cache.h"
+#include "repository.h"
 #include "config.h"
 #include "dir.h"
 #include "tree.h"
@@ -255,13 +256,16 @@ static int check_submodule_move_head(const struct cache_entry *ce,
 {
 	unsigned flags = SUBMODULE_MOVE_HEAD_DRY_RUN;
 	const struct submodule *sub = submodule_from_ce(ce);
+	struct submodule_update_strategy update;
+
 	if (!sub)
 		return 0;
 
 	if (o->reset)
 		flags |= SUBMODULE_MOVE_HEAD_FORCE;
 
-	switch (sub->update_strategy.type) {
+	update = submodule_strategy_with_config_overlayed(the_repository, sub);
+	switch (update.type) {
 	case SM_UPDATE_UNSPECIFIED:
 	case SM_UPDATE_CHECKOUT:
 		if (submodule_move_head(ce->name, old_id, new_id, flags))
@@ -293,7 +297,6 @@ static void reload_gitmodules_file(struct index_state *index,
 				submodule_free();
 				checkout_entry(ce, state, NULL);
 				gitmodules_config();
-				git_config(submodule_config, NULL);
 			} else
 				break;
 		}
@@ -308,7 +311,10 @@ static void unlink_entry(const struct cache_entry *ce)
 {
 	const struct submodule *sub = submodule_from_ce(ce);
 	if (sub) {
-		switch (sub->update_strategy.type) {
+		struct submodule_update_strategy update =
+			submodule_strategy_with_config_overlayed(the_repository,
+								 sub);
+		switch (update.type) {
 		case SM_UPDATE_UNSPECIFIED:
 		case SM_UPDATE_CHECKOUT:
 		case SM_UPDATE_REBASE:
-- 
2.14.0.rc0.400.g1c36432dff-goog

[PATCH 09/15] submodule: remove submodule_config callback routine

[Brandon Williams]

Remove the last remaining caller of 'submodule_config()' as well as the
function itself.

With 'submodule_config()' being removed the submodule-config API can be
a little simpler as callers don't need to worry about whether or not
they need to overlay the repository's config on top of the
submodule-config.  This also makes it more difficult to accidentally
add non-submodule specific configuration to the .gitmodules file.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 builtin/submodule--helper.c |  1 -
 submodule.c                 | 25 ++-----------------------
 submodule.h                 |  1 -
 3 files changed, 2 insertions(+), 25 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 25f471ba1..c16249e30 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1196,7 +1196,6 @@ 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);
 
 	if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)
 		return 1;
diff --git a/submodule.c b/submodule.c
index 13380fed1..f63940347 100644
--- a/submodule.c
+++ b/submodule.c
@@ -180,27 +180,6 @@ void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,
 	}
 }
 
-/* For loading from the .gitmodules file. */
-static int git_modules_config(const char *var, const char *value, void *cb)
-{
-	if (starts_with(var, "submodule."))
-		return parse_submodule_config_option(var, value);
-	return 0;
-}
-
-/* Loads all submodule settings from the config. */
-int submodule_config(const char *var, const char *value, void *cb)
-{
-	if (!strcmp(var, "submodule.recurse")) {
-		int v = git_config_bool(var, value) ?
-			RECURSE_SUBMODULES_ON : RECURSE_SUBMODULES_OFF;
-		config_update_recurse_submodules = v;
-		return 0;
-	} else {
-		return git_modules_config(var, value, cb);
-	}
-}
-
 /* Cheap function that only determines if we're interested in submodules at all */
 int git_default_submodule_config(const char *var, const char *value, void *cb)
 {
@@ -271,8 +250,8 @@ void gitmodules_config_oid(const struct object_id *commit_oid)
 	struct object_id oid;
 
 	if (gitmodule_oid_from_commit(commit_oid, &oid, &rev)) {
-		git_config_from_blob_oid(submodule_config, rev.buf,
-					 &oid, NULL);
+		git_config_from_blob_oid(gitmodules_cb, rev.buf,
+					 &oid, the_repository);
 	}
 	strbuf_release(&rev);
 }
diff --git a/submodule.h b/submodule.h
index f17ca1e34..1c6b2ab4e 100644
--- a/submodule.h
+++ b/submodule.h
@@ -41,7 +41,6 @@ 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 int git_default_submodule_config(const char *var, const char *value, void *cb);
 
 struct option;
-- 
2.14.0.rc0.400.g1c36432dff-goog

[PATCH 10/15] diff: stop allowing diff to have submodules configured in .git/config

[Brandon Williams]

Traditionally a submodule is comprised of a gitlink as well as a
corresponding entry in the .gitmodules file.  Diff doesn't follow this
paradigm as its config callback routine falls back to populating the
submodule-config if a config entry starts with 'submodule.'.

Remove this behavior in order to be consistent with how the
submodule-config is populated, via calling 'gitmodules_config()' or
'repo_read_gitmodules()'.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 diff.c                    |  3 ---
 t/t4027-diff-submodule.sh | 67 -----------------------------------------------
 2 files changed, 70 deletions(-)

diff --git a/diff.c b/diff.c
index 85e714f6c..e43519b88 100644
--- a/diff.c
+++ b/diff.c
@@ -346,9 +346,6 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
-	if (starts_with(var, "submodule."))
-		return parse_submodule_config_option(var, value);
-
 	if (git_diff_heuristic_config(var, value, cb) < 0)
 		return -1;
 
diff --git a/t/t4027-diff-submodule.sh b/t/t4027-diff-submodule.sh
index 518bf9524..2ffd11a14 100755
--- a/t/t4027-diff-submodule.sh
+++ b/t/t4027-diff-submodule.sh
@@ -113,35 +113,6 @@ test_expect_success 'git diff HEAD with dirty submodule (work tree, refs match)'
 	! test -s actual4
 '
 
-test_expect_success 'git diff HEAD with dirty submodule (work tree, refs match) [.git/config]' '
-	git config diff.ignoreSubmodules all &&
-	git diff HEAD >actual &&
-	! test -s actual &&
-	git config submodule.subname.ignore none &&
-	git config submodule.subname.path sub &&
-	git diff HEAD >actual &&
-	sed -e "1,/^@@/d" actual >actual.body &&
-	expect_from_to >expect.body $subprev $subprev-dirty &&
-	test_cmp expect.body actual.body &&
-	git config submodule.subname.ignore all &&
-	git diff HEAD >actual2 &&
-	! test -s actual2 &&
-	git config submodule.subname.ignore untracked &&
-	git diff HEAD >actual3 &&
-	sed -e "1,/^@@/d" actual3 >actual3.body &&
-	expect_from_to >expect.body $subprev $subprev-dirty &&
-	test_cmp expect.body actual3.body &&
-	git config submodule.subname.ignore dirty &&
-	git diff HEAD >actual4 &&
-	! test -s actual4 &&
-	git diff HEAD --ignore-submodules=none >actual &&
-	sed -e "1,/^@@/d" actual >actual.body &&
-	expect_from_to >expect.body $subprev $subprev-dirty &&
-	test_cmp expect.body actual.body &&
-	git config --remove-section submodule.subname &&
-	git config --unset diff.ignoreSubmodules
-'
-
 test_expect_success 'git diff HEAD with dirty submodule (work tree, refs match) [.gitmodules]' '
 	git config diff.ignoreSubmodules dirty &&
 	git diff HEAD >actual &&
@@ -208,24 +179,6 @@ test_expect_success 'git diff HEAD with dirty submodule (untracked, refs match)'
 	! test -s actual4
 '
 
-test_expect_success 'git diff HEAD with dirty submodule (untracked, refs match) [.git/config]' '
-	git config submodule.subname.ignore all &&
-	git config submodule.subname.path sub &&
-	git diff HEAD >actual2 &&
-	! test -s actual2 &&
-	git config submodule.subname.ignore untracked &&
-	git diff HEAD >actual3 &&
-	! test -s actual3 &&
-	git config submodule.subname.ignore dirty &&
-	git diff HEAD >actual4 &&
-	! test -s actual4 &&
-	git diff --ignore-submodules=none HEAD >actual &&
-	sed -e "1,/^@@/d" actual >actual.body &&
-	expect_from_to >expect.body $subprev $subprev-dirty &&
-	test_cmp expect.body actual.body &&
-	git config --remove-section submodule.subname
-'
-
 test_expect_success 'git diff HEAD with dirty submodule (untracked, refs match) [.gitmodules]' '
 	git config --add -f .gitmodules submodule.subname.ignore all &&
 	git config --add -f .gitmodules submodule.subname.path sub &&
@@ -261,26 +214,6 @@ test_expect_success 'git diff between submodule commits' '
 	! test -s actual
 '
 
-test_expect_success 'git diff between submodule commits [.git/config]' '
-	git diff HEAD^..HEAD >actual &&
-	sed -e "1,/^@@/d" actual >actual.body &&
-	expect_from_to >expect.body $subtip $subprev &&
-	test_cmp expect.body actual.body &&
-	git config submodule.subname.ignore dirty &&
-	git config submodule.subname.path sub &&
-	git diff HEAD^..HEAD >actual &&
-	sed -e "1,/^@@/d" actual >actual.body &&
-	expect_from_to >expect.body $subtip $subprev &&
-	test_cmp expect.body actual.body &&
-	git config submodule.subname.ignore all &&
-	git diff HEAD^..HEAD >actual &&
-	! test -s actual &&
-	git diff --ignore-submodules=dirty HEAD^..HEAD >actual &&
-	sed -e "1,/^@@/d" actual >actual.body &&
-	expect_from_to >expect.body $subtip $subprev &&
-	git config --remove-section submodule.subname
-'
-
 test_expect_success 'git diff between submodule commits [.gitmodules]' '
 	git diff HEAD^..HEAD >actual &&
 	sed -e "1,/^@@/d" actual >actual.body &&
-- 
2.14.0.rc0.400.g1c36432dff-goog

[PATCH 11/15] submodule-config: remove support for overlaying repository config

[Brandon Williams]

All callers have been migrated to explicitly read any configuration they
need.  The support for handling it automatically in submodule-config is
no longer needed.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 submodule-config.h               |  1 -
 t/helper/test-submodule-config.c |  6 ----
 t/t7411-submodule-config.sh      | 72 ----------------------------------------
 3 files changed, 79 deletions(-)

diff --git a/submodule-config.h b/submodule-config.h
index cccd34b92..84c2cf515 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -34,7 +34,6 @@ extern int option_fetch_parse_recurse_submodules(const struct option *opt,
 						 const char *arg, int unset);
 extern int parse_update_recurse_submodules_arg(const char *opt, const char *arg);
 extern int parse_push_recurse_submodules_arg(const char *opt, const char *arg);
-extern int parse_submodule_config_option(const char *var, const char *value);
 extern int submodule_config_option(struct repository *repo,
 				   const char *var, const char *value);
 extern const struct submodule *submodule_from_name(
diff --git a/t/helper/test-submodule-config.c b/t/helper/test-submodule-config.c
index e13fbcc1b..f4a7c431c 100644
--- a/t/helper/test-submodule-config.c
+++ b/t/helper/test-submodule-config.c
@@ -10,11 +10,6 @@ static void die_usage(int argc, const char **argv, const char *msg)
 	exit(1);
 }
 
-static int git_test_config(const char *var, const char *value, void *cb)
-{
-	return parse_submodule_config_option(var, value);
-}
-
 int cmd_main(int argc, const char **argv)
 {
 	const char **arg = argv;
@@ -38,7 +33,6 @@ int cmd_main(int argc, const char **argv)
 
 	setup_git_directory();
 	gitmodules_config();
-	git_config(git_test_config, NULL);
 
 	while (*arg) {
 		struct object_id commit_oid;
diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
index 7d6b25ba2..46c09c776 100755
--- a/t/t7411-submodule-config.sh
+++ b/t/t7411-submodule-config.sh
@@ -122,78 +122,6 @@ test_expect_success 'using different treeishs works' '
 	)
 '
 
-cat >super/expect_url <<EOF
-Submodule url: 'git@somewhere.else.net:a.git' for path 'b'
-Submodule url: 'git@somewhere.else.net:submodule.git' for path 'submodule'
-EOF
-
-cat >super/expect_local_path <<EOF
-Submodule name: 'a' for path 'c'
-Submodule name: 'submodule' for path 'submodule'
-EOF
-
-test_expect_success 'reading of local configuration' '
-	(cd super &&
-		old_a=$(git config submodule.a.url) &&
-		old_submodule=$(git config submodule.submodule.url) &&
-		git config submodule.a.url git@somewhere.else.net:a.git &&
-		git config submodule.submodule.url git@somewhere.else.net:submodule.git &&
-		test-submodule-config --url \
-			"" b \
-			"" submodule \
-				>actual &&
-		test_cmp expect_url actual &&
-		git config submodule.a.path c &&
-		test-submodule-config \
-			"" c \
-			"" submodule \
-				>actual &&
-		test_cmp expect_local_path actual &&
-		git config submodule.a.url "$old_a" &&
-		git config submodule.submodule.url "$old_submodule" &&
-		git config --unset submodule.a.path c
-	)
-'
-
-cat >super/expect_url <<EOF
-Submodule url: '../submodule' for path 'b'
-Submodule url: 'git@somewhere.else.net:submodule.git' for path 'submodule'
-EOF
-
-test_expect_success 'reading of local configuration for uninitialized submodules' '
-	(
-		cd super &&
-		git submodule deinit -f b &&
-		old_submodule=$(git config submodule.submodule.url) &&
-		git config submodule.submodule.url git@somewhere.else.net:submodule.git &&
-		test-submodule-config --url \
-			"" b \
-			"" submodule \
-				>actual &&
-		test_cmp expect_url actual &&
-		git config submodule.submodule.url "$old_submodule" &&
-		git submodule init b
-	)
-'
-
-cat >super/expect_fetchrecurse_die.err <<EOF
-fatal: bad submodule.submodule.fetchrecursesubmodules argument: blabla
-EOF
-
-test_expect_success 'local error in fetchrecursesubmodule dies early' '
-	(cd super &&
-		git config submodule.submodule.fetchrecursesubmodules blabla &&
-		test_must_fail test-submodule-config \
-			"" b \
-			"" submodule \
-				>actual.out 2>actual.err &&
-		touch expect_fetchrecurse_die.out &&
-		test_cmp expect_fetchrecurse_die.out actual.out  &&
-		test_cmp expect_fetchrecurse_die.err actual.err  &&
-		git config --unset submodule.submodule.fetchrecursesubmodules
-	)
-'
-
 test_expect_success 'error in history in fetchrecursesubmodule lets continue' '
 	(cd super &&
 		git config -f .gitmodules \
-- 
2.14.0.rc0.400.g1c36432dff-goog

[PATCH 12/15] submodule-config: move submodule-config functions to submodule-config.c

[Brandon Williams]

Migrate the functions used to initialize the submodule-config to
submodule-config.c so that the callback routine used in the
initialization process can be static and prevent it from being used
outside of initializing the submodule-config through the main API.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 builtin/ls-files.c |  1 +
 submodule-config.c | 38 +++++++++++++++++++++++++++++++-------
 submodule-config.h |  7 ++-----
 submodule.c        | 35 -----------------------------------
 submodule.h        |  2 --
 5 files changed, 34 insertions(+), 49 deletions(-)

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index b8514a002..d14612057 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -19,6 +19,7 @@
 #include "pathspec.h"
 #include "run-command.h"
 #include "submodule.h"
+#include "submodule-config.h"
 
 static int abbrev;
 static int show_deleted;
diff --git a/submodule-config.c b/submodule-config.c
index 0b429e942..86636654b 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -449,9 +449,9 @@ static int parse_config(const char *var, const char *value, void *data)
 	return ret;
 }
 
-int gitmodule_oid_from_commit(const struct object_id *treeish_name,
-				      struct object_id *gitmodules_oid,
-				      struct strbuf *rev)
+static int gitmodule_oid_from_commit(const struct object_id *treeish_name,
+				     struct object_id *gitmodules_oid,
+				     struct strbuf *rev)
 {
 	int ret = 0;
 
@@ -552,9 +552,9 @@ static void submodule_cache_check_init(struct repository *repo)
 	submodule_cache_init(repo->submodule_cache);
 }
 
-int submodule_config_option(struct repository *repo,
-			    const char *var, const char *value)
+static int gitmodules_cb(const char *var, const char *value, void *data)
 {
+	struct repository *repo = data;
 	struct parse_config_parameter parameter;
 
 	submodule_cache_check_init(repo);
@@ -567,9 +567,33 @@ int submodule_config_option(struct repository *repo,
 	return parse_config(var, value, &parameter);
 }
 
-int parse_submodule_config_option(const char *var, const char *value)
+void repo_read_gitmodules(struct repository *repo)
 {
-	return submodule_config_option(the_repository, var, value);
+	if (repo->worktree) {
+		char *gitmodules;
+
+		if (repo_read_index(repo) < 0)
+			return;
+
+		gitmodules = repo_worktree_path(repo, GITMODULES_FILE);
+
+		if (!is_gitmodules_unmerged(repo->index))
+			git_config_from_file(gitmodules_cb, gitmodules, repo);
+
+		free(gitmodules);
+	}
+}
+
+void gitmodules_config_oid(const struct object_id *commit_oid)
+{
+	struct strbuf rev = STRBUF_INIT;
+	struct object_id oid;
+
+	if (gitmodule_oid_from_commit(commit_oid, &oid, &rev)) {
+		git_config_from_blob_oid(gitmodules_cb, rev.buf,
+					 &oid, the_repository);
+	}
+	strbuf_release(&rev);
 }
 
 const struct submodule *submodule_from_name(const struct object_id *treeish_name,
diff --git a/submodule-config.h b/submodule-config.h
index 84c2cf515..e3845831f 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -34,8 +34,8 @@ extern int option_fetch_parse_recurse_submodules(const struct option *opt,
 						 const char *arg, int unset);
 extern int parse_update_recurse_submodules_arg(const char *opt, const char *arg);
 extern int parse_push_recurse_submodules_arg(const char *opt, const char *arg);
-extern int submodule_config_option(struct repository *repo,
-				   const char *var, const char *value);
+extern void repo_read_gitmodules(struct repository *repo);
+extern void gitmodules_config_oid(const struct object_id *commit_oid);
 extern const struct submodule *submodule_from_name(
 		const struct object_id *commit_or_tree, const char *name);
 extern const struct submodule *submodule_from_path(
@@ -43,9 +43,6 @@ extern const struct submodule *submodule_from_path(
 extern const struct submodule *submodule_from_cache(struct repository *repo,
 						    const struct object_id *treeish_name,
 						    const char *key);
-extern int gitmodule_oid_from_commit(const struct object_id *commit_oid,
-				     struct object_id *gitmodules_oid,
-				     struct strbuf *rev);
 extern void submodule_free(void);
 
 #endif /* SUBMODULE_CONFIG_H */
diff --git a/submodule.c b/submodule.c
index f63940347..7ebd639f4 100644
--- a/submodule.c
+++ b/submodule.c
@@ -216,46 +216,11 @@ void load_submodule_cache(void)
 	gitmodules_config();
 }
 
-static int gitmodules_cb(const char *var, const char *value, void *data)
-{
-	struct repository *repo = data;
-	return submodule_config_option(repo, var, value);
-}
-
-void repo_read_gitmodules(struct repository *repo)
-{
-	if (repo->worktree) {
-		char *gitmodules;
-
-		if (repo_read_index(repo) < 0)
-			return;
-
-		gitmodules = repo_worktree_path(repo, GITMODULES_FILE);
-
-		if (!is_gitmodules_unmerged(repo->index))
-			git_config_from_file(gitmodules_cb, gitmodules, repo);
-
-		free(gitmodules);
-	}
-}
-
 void gitmodules_config(void)
 {
 	repo_read_gitmodules(the_repository);
 }
 
-void gitmodules_config_oid(const struct object_id *commit_oid)
-{
-	struct strbuf rev = STRBUF_INIT;
-	struct object_id oid;
-
-	if (gitmodule_oid_from_commit(commit_oid, &oid, &rev)) {
-		git_config_from_blob_oid(gitmodules_cb, rev.buf,
-					 &oid, the_repository);
-	}
-	strbuf_release(&rev);
-}
-
 /*
  * Determine if a submodule has been initialized at a given 'path'
  */
diff --git a/submodule.h b/submodule.h
index 1c6b2ab4e..36fc7f7cf 100644
--- a/submodule.h
+++ b/submodule.h
@@ -48,8 +48,6 @@ int option_parse_recurse_submodules_worktree_updater(const struct option *opt,
 						     const char *arg, int unset);
 void load_submodule_cache(void);
 extern void gitmodules_config(void);
-extern void repo_read_gitmodules(struct repository *repo);
-extern void gitmodules_config_oid(const struct object_id *commit_oid);
 extern int is_submodule_active(struct repository *repo, const char *path);
 /*
  * Determine if a submodule has been populated at a given 'path' by checking if
-- 
2.14.0.rc0.400.g1c36432dff-goog

[PATCH 13/15] submodule-config: lazy-load a repository's .gitmodules file

[Brandon Williams]

In order to use the submodule-config subsystem, callers first need to
initialize it by calling 'repo_read_gitmodules()' or
'gitmodules_config()' (which just redirects to
'repo_read_gitmodules()').  There are a couple of callers who need to
load an explicit revision of the repository's .gitmodules file (grep) or
need to modify the .gitmodules file so they would need to load it before
modify the file (checkout), but the majority of callers are simply
reading the .gitmodules file present in the working tree.  For the
common case it would be nice to avoid the boilerplate of initializing
the submodule-config system before using it, so instead let's perform
lazy-loading of the submodule-config system.

Remove the calls to reading the gitmodules file from ls-files to show
that lazy-loading the .gitmodules file works.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 builtin/ls-files.c |  5 -----
 submodule-config.c | 27 ++++++++++++++++++++++-----
 2 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index d14612057..bd74ee07d 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -211,8 +211,6 @@ static void show_submodule(struct repository *superproject,
 	if (repo_read_index(&submodule) < 0)
 		die("index file corrupt");
 
-	repo_read_gitmodules(&submodule);
-
 	show_files(&submodule, dir);
 
 	repo_clear(&submodule);
@@ -611,9 +609,6 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 	if (require_work_tree && !is_inside_work_tree())
 		setup_work_tree();
 
-	if (recurse_submodules)
-		repo_read_gitmodules(the_repository);
-
 	if (recurse_submodules &&
 	    (show_stage || show_deleted || show_others || show_unmerged ||
 	     show_killed || show_modified || show_resolve_undo || with_tree))
diff --git a/submodule-config.c b/submodule-config.c
index 86636654b..56d9d76d4 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -18,6 +18,7 @@ struct submodule_cache {
 	struct hashmap for_path;
 	struct hashmap for_name;
 	unsigned initialized:1;
+	unsigned gitmodules_read:1;
 };
 
 /*
@@ -93,6 +94,7 @@ static void submodule_cache_clear(struct submodule_cache *cache)
 	hashmap_free(&cache->for_path, 1);
 	hashmap_free(&cache->for_name, 1);
 	cache->initialized = 0;
+	cache->gitmodules_read = 0;
 }
 
 void submodule_cache_free(struct submodule_cache *cache)
@@ -557,8 +559,6 @@ static int gitmodules_cb(const char *var, const char *value, void *data)
 	struct repository *repo = data;
 	struct parse_config_parameter parameter;
 
-	submodule_cache_check_init(repo);
-
 	parameter.cache = repo->submodule_cache;
 	parameter.treeish_name = NULL;
 	parameter.gitmodules_sha1 = null_sha1;
@@ -569,6 +569,8 @@ static int gitmodules_cb(const char *var, const char *value, void *data)
 
 void repo_read_gitmodules(struct repository *repo)
 {
+	submodule_cache_check_init(repo);
+
 	if (repo->worktree) {
 		char *gitmodules;
 
@@ -582,6 +584,8 @@ void repo_read_gitmodules(struct repository *repo)
 
 		free(gitmodules);
 	}
+
+	repo->submodule_cache->gitmodules_read = 1;
 }
 
 void gitmodules_config_oid(const struct object_id *commit_oid)
@@ -589,24 +593,37 @@ void gitmodules_config_oid(const struct object_id *commit_oid)
 	struct strbuf rev = STRBUF_INIT;
 	struct object_id oid;
 
+	submodule_cache_check_init(the_repository);
+
 	if (gitmodule_oid_from_commit(commit_oid, &oid, &rev)) {
 		git_config_from_blob_oid(gitmodules_cb, rev.buf,
 					 &oid, the_repository);
 	}
 	strbuf_release(&rev);
+
+	the_repository->submodule_cache->gitmodules_read = 1;
+}
+
+static void gitmodules_read_check(struct repository *repo)
+{
+	submodule_cache_check_init(repo);
+
+	/* read the repo's .gitmodules file if it hasn't been already */
+	if (!repo->submodule_cache->gitmodules_read)
+		repo_read_gitmodules(repo);
 }
 
 const struct submodule *submodule_from_name(const struct object_id *treeish_name,
 		const char *name)
 {
-	submodule_cache_check_init(the_repository);
+	gitmodules_read_check(the_repository);
 	return config_from(the_repository->submodule_cache, treeish_name, name, lookup_name);
 }
 
 const struct submodule *submodule_from_path(const struct object_id *treeish_name,
 		const char *path)
 {
-	submodule_cache_check_init(the_repository);
+	gitmodules_read_check(the_repository);
 	return config_from(the_repository->submodule_cache, treeish_name, path, lookup_path);
 }
 
@@ -614,7 +631,7 @@ const struct submodule *submodule_from_cache(struct repository *repo,
 					     const struct object_id *treeish_name,
 					     const char *key)
 {
-	submodule_cache_check_init(repo);
+	gitmodules_read_check(repo);
 	return config_from(repo->submodule_cache, treeish_name,
 			   key, lookup_path);
 }
-- 
2.14.0.rc0.400.g1c36432dff-goog

[PATCH 14/15] unpack-trees: improve loading of .gitmodules

[Brandon Williams]

When recursing submodules 'check_updates()' needs to have strict control
over the submodule-config subsystem to ensure that the gitmodules file
has been read before checking cache entries which are marked for
removal as well ensuring the proper gitmodules file is read before
updating cache entries.

Because of this let's not rely on callers of 'check_updates()' to read
the gitmodules file before calling 'check_updates()' and handle the
reading explicitly.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 unpack-trees.c | 42 ++++++++++++++++++++++++++----------------
 1 file changed, 26 insertions(+), 16 deletions(-)

diff --git a/unpack-trees.c b/unpack-trees.c
index dc66b880d..144c556c8 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -283,22 +283,28 @@ static int check_submodule_move_head(const struct cache_entry *ce,
 	}
 }
 
-static void reload_gitmodules_file(struct index_state *index,
-				   struct checkout *state)
+/*
+ * Preform the loading of the repository's gitmodules file.  This function is
+ * used by 'check_update()' to perform loading of the gitmodules file in two
+ * differnt situations:
+ * (1) before removing entries from the working tree if the gitmodules file has
+ *     been marked for removal.  This situation is specified by 'state' == NULL.
+ * (2) before checking out entries to the working tree if the gitmodules file
+ *     has been marked for update.  This situation is specified by 'state' != NULL.
+ */
+static void load_gitmodules_file(struct index_state *index,
+				 struct checkout *state)
 {
-	int i;
-	for (i = 0; i < index->cache_nr; i++) {
-		struct cache_entry *ce = index->cache[i];
-		if (ce->ce_flags & CE_UPDATE) {
-			int r = strcmp(ce->name, ".gitmodules");
-			if (r < 0)
-				continue;
-			else if (r == 0) {
-				submodule_free();
-				checkout_entry(ce, state, NULL);
-				gitmodules_config();
-			} else
-				break;
+	int pos = index_name_pos(index, GITMODULES_FILE, strlen(GITMODULES_FILE));
+
+	if (pos >= 0) {
+		struct cache_entry *ce = index->cache[pos];
+		if (!state && ce->ce_flags & CE_WT_REMOVE) {
+			repo_read_gitmodules(the_repository);
+		} else if (state && (ce->ce_flags & CE_UPDATE)) {
+			submodule_free();
+			checkout_entry(ce, state, NULL);
+			repo_read_gitmodules(the_repository);
 		}
 	}
 }
@@ -371,6 +377,10 @@ static int check_updates(struct unpack_trees_options *o)
 
 	if (o->update)
 		git_attr_set_direction(GIT_ATTR_CHECKOUT, index);
+
+	if (should_update_submodules() && o->update && !o->dry_run)
+		load_gitmodules_file(index, NULL);
+
 	for (i = 0; i < index->cache_nr; i++) {
 		const struct cache_entry *ce = index->cache[i];
 
@@ -384,7 +394,7 @@ static int check_updates(struct unpack_trees_options *o)
 	remove_scheduled_dirs();
 
 	if (should_update_submodules() && o->update && !o->dry_run)
-		reload_gitmodules_file(index, &state);
+		load_gitmodules_file(index, &state);
 
 	for (i = 0; i < index->cache_nr; i++) {
 		struct cache_entry *ce = index->cache[i];
-- 
2.14.0.rc0.400.g1c36432dff-goog

[PATCH 15/15] submodule: remove gitmodules_config

[Brandon Williams]

Now that the submodule-config subsystem can lazily read the gitmodules
file we no longer need to explicitly pre-read the gitmodules by calling
'gitmodules_config()' so let's remove it.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 builtin/checkout.c               |  1 -
 builtin/commit.c                 |  1 -
 builtin/diff-files.c             |  1 -
 builtin/diff-index.c             |  1 -
 builtin/diff-tree.c              |  1 -
 builtin/diff.c                   |  2 --
 builtin/fetch.c                  |  4 ----
 builtin/grep.c                   |  4 ----
 builtin/mv.c                     |  1 -
 builtin/read-tree.c              |  2 --
 builtin/reset.c                  |  2 --
 builtin/rm.c                     |  1 -
 builtin/submodule--helper.c      | 14 --------------
 submodule.c                      | 15 ---------------
 submodule.h                      |  2 --
 t/helper/test-submodule-config.c |  1 -
 16 files changed, 53 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 246e0cd16..63ae16afc 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1179,7 +1179,6 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 	opts.prefix = prefix;
 	opts.show_progress = -1;
 
-	gitmodules_config();
 	git_config(git_checkout_config, &opts);
 
 	opts.track = BRANCH_TRACK_UNSPECIFIED;
diff --git a/builtin/commit.c b/builtin/commit.c
index 4bbac014a..18ad714d9 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -195,7 +195,6 @@ static void determine_whence(struct wt_status *s)
 static void status_init_config(struct wt_status *s, config_fn_t fn)
 {
 	wt_status_prepare(s);
-	gitmodules_config();
 	git_config(fn, s);
 	determine_whence(s);
 	init_diff_ui_defaults();
diff --git a/builtin/diff-files.c b/builtin/diff-files.c
index 17bf84d18..e88493ffe 100644
--- a/builtin/diff-files.c
+++ b/builtin/diff-files.c
@@ -26,7 +26,6 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)
 
 	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
 	init_revisions(&rev, prefix);
-	gitmodules_config();
 	rev.abbrev = 0;
 	precompose_argv(argc, argv);
 
diff --git a/builtin/diff-index.c b/builtin/diff-index.c
index 185e6f9b5..9d772f8f2 100644
--- a/builtin/diff-index.c
+++ b/builtin/diff-index.c
@@ -23,7 +23,6 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)
 
 	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
 	init_revisions(&rev, prefix);
-	gitmodules_config();
 	rev.abbrev = 0;
 	precompose_argv(argc, argv);
 
diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
index 31d2cb410..d66499909 100644
--- a/builtin/diff-tree.c
+++ b/builtin/diff-tree.c
@@ -110,7 +110,6 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
 
 	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
 	init_revisions(opt, prefix);
-	gitmodules_config();
 	opt->abbrev = 0;
 	opt->diff = 1;
 	opt->disable_stdin = 1;
diff --git a/builtin/diff.c b/builtin/diff.c
index 7cde6abbc..7e3ebcea3 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -315,8 +315,6 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 			no_index = DIFF_NO_INDEX_IMPLICIT;
 	}
 
-	if (!no_index)
-		gitmodules_config();
 	init_diff_ui_defaults();
 	git_config(git_diff_ui_config, NULL);
 	precompose_argv(argc, argv);
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 3fe99073d..132e3224e 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1360,10 +1360,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 	if (depth || deepen_since || deepen_not.nr)
 		deepen = 1;
 
-	if (recurse_submodules != RECURSE_SUBMODULES_OFF) {
-		gitmodules_config();
-	}
-
 	if (all) {
 		if (argc == 1)
 			die(_("fetch --all does not take a repository argument"));
diff --git a/builtin/grep.c b/builtin/grep.c
index ac06d2d33..2d65f27d0 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1048,10 +1048,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	}
 #endif
 
-	if (recurse_submodules) {
-		gitmodules_config();
-	}
-
 	if (show_in_pager && (cached || list.nr))
 		die(_("--open-files-in-pager only works on the worktree"));
 
diff --git a/builtin/mv.c b/builtin/mv.c
index 94fbaaa5d..ffdd5f01a 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -131,7 +131,6 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 	struct stat st;
 	struct string_list src_for_dst = STRING_LIST_INIT_NODUP;
 
-	gitmodules_config();
 	git_config(git_default_config, NULL);
 
 	argc = parse_options(argc, argv, prefix, builtin_mv_options,
diff --git a/builtin/read-tree.c b/builtin/read-tree.c
index d5f618d08..bf87a2710 100644
--- a/builtin/read-tree.c
+++ b/builtin/read-tree.c
@@ -164,8 +164,6 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
 	argc = parse_options(argc, argv, unused_prefix, read_tree_options,
 			     read_tree_usage, 0);
 
-	load_submodule_cache();
-
 	hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);
 
 	prefix_set = opts.prefix ? 1 : 0;
diff --git a/builtin/reset.c b/builtin/reset.c
index 772d078b8..50488d273 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -309,8 +309,6 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 						PARSE_OPT_KEEP_DASHDASH);
 	parse_args(&pathspec, argv, prefix, patch_mode, &rev);
 
-	load_submodule_cache();
-
 	unborn = !strcmp(rev, "HEAD") && get_oid("HEAD", &oid);
 	if (unborn) {
 		/* reset on unborn branch: treat as reset to empty tree */
diff --git a/builtin/rm.c b/builtin/rm.c
index 4057e73fa..d91451fea 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -255,7 +255,6 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 	struct pathspec pathspec;
 	char *seen;
 
-	gitmodules_config();
 	git_config(git_default_config, NULL);
 
 	argc = parse_options(argc, argv, prefix, builtin_rm_options,
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index c16249e30..d74855b2d 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -275,8 +275,6 @@ static void module_list_active(struct module_list *list)
 	int i;
 	struct module_list active_modules = MODULE_LIST_INIT;
 
-	gitmodules_config();
-
 	for (i = 0; i < list->nr; i++) {
 		const struct cache_entry *ce = list->entries[i];
 
@@ -337,9 +335,6 @@ static void init_submodule(const char *path, const char *prefix, int quiet)
 	struct strbuf sb = STRBUF_INIT;
 	char *upd = NULL, *url = NULL, *displaypath;
 
-	/* Only loads from .gitmodules, no overlay with .git/config */
-	gitmodules_config();
-
 	if (prefix && get_super_prefix())
 		die("BUG: cannot have prefix and superprefix");
 	else if (prefix)
@@ -475,7 +470,6 @@ static int module_name(int argc, const char **argv, const char *prefix)
 	if (argc != 2)
 		usage(_("git submodule--helper name <path>"));
 
-	gitmodules_config();
 	sub = submodule_from_path(&null_oid, argv[1]);
 
 	if (!sub)
@@ -1033,8 +1027,6 @@ static int update_clone(int argc, const char **argv, const char *prefix)
 	if (pathspec.nr)
 		suc.warn_if_uninitialized = 1;
 
-	gitmodules_config();
-
 	run_processes_parallel(max_jobs,
 			       update_clone_get_next_task,
 			       update_clone_start_failure,
@@ -1075,8 +1067,6 @@ static const char *remote_submodule_branch(const char *path)
 	const char *branch = NULL;
 	char *key;
 
-	gitmodules_config();
-
 	sub = submodule_from_path(&null_oid, path);
 	if (!sub)
 		return NULL;
@@ -1195,8 +1185,6 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, embed_gitdir_options,
 			     git_submodule_helper_usage, 0);
 
-	gitmodules_config();
-
 	if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)
 		return 1;
 
@@ -1212,8 +1200,6 @@ static int is_active(int argc, const char **argv, const char *prefix)
 	if (argc != 2)
 		die("submodule--helper is-active takes exactly 1 argument");
 
-	gitmodules_config();
-
 	return !is_submodule_active(the_repository, argv[1]);
 }
 
diff --git a/submodule.c b/submodule.c
index 7ebd639f4..1e4ff4e51 100644
--- a/submodule.c
+++ b/submodule.c
@@ -208,19 +208,6 @@ int option_parse_recurse_submodules_worktree_updater(const struct option *opt,
 	return 0;
 }
 
-void load_submodule_cache(void)
-{
-	if (config_update_recurse_submodules == RECURSE_SUBMODULES_OFF)
-		return;
-
-	gitmodules_config();
-}
-
-void gitmodules_config(void)
-{
-	repo_read_gitmodules(the_repository);
-}
-
 /*
  * Determine if a submodule has been initialized at a given 'path'
  */
@@ -1109,7 +1096,6 @@ int submodule_touches_in_range(struct object_id *excl_oid,
 	struct argv_array args = ARGV_ARRAY_INIT;
 	int ret;
 
-	gitmodules_config();
 	/* No need to check if there are no submodules configured */
 	if (!submodule_from_path(NULL, NULL))
 		return 0;
@@ -2016,7 +2002,6 @@ int submodule_to_gitdir(struct strbuf *buf, const char *submodule)
 		strbuf_addstr(buf, git_dir);
 	}
 	if (!is_git_directory(buf->buf)) {
-		gitmodules_config();
 		sub = submodule_from_path(&null_oid, submodule);
 		if (!sub) {
 			ret = -1;
diff --git a/submodule.h b/submodule.h
index 36fc7f7cf..7d0b5aa43 100644
--- a/submodule.h
+++ b/submodule.h
@@ -46,8 +46,6 @@ extern int git_default_submodule_config(const char *var, const char *value, void
 struct option;
 int option_parse_recurse_submodules_worktree_updater(const struct option *opt,
 						     const char *arg, int unset);
-void load_submodule_cache(void);
-extern void gitmodules_config(void);
 extern int is_submodule_active(struct repository *repo, const char *path);
 /*
  * Determine if a submodule has been populated at a given 'path' by checking if
diff --git a/t/helper/test-submodule-config.c b/t/helper/test-submodule-config.c
index f4a7c431c..f23db3b19 100644
--- a/t/helper/test-submodule-config.c
+++ b/t/helper/test-submodule-config.c
@@ -32,7 +32,6 @@ int cmd_main(int argc, const char **argv)
 		die_usage(argc, argv, "Wrong number of arguments.");
 
 	setup_git_directory();
-	gitmodules_config();
 
 	while (*arg) {
 		struct object_id commit_oid;
-- 
2.14.0.rc0.400.g1c36432dff-goog

Re: [PATCH 02/15] submodule: don't use submodule_from_name

[Stefan Beller]

On Tue, Jul 25, 2017 at 2:39 PM, Brandon Williams <bmwill@google.com> wrote:
> The function 'submodule_from_name()' is being used incorrectly here as a
> submodule path is being used instead of a submodule name.  Since the
> correct function to use with a path to a submodule is already being used
> ('submodule_from_path()') let's remove the call to
> 'submodule_from_name()'.

This blames to 851e18c385 (submodule: use new config API for worktree
configurations, 2015-08-17), but that is a refactoring. The issue of using
the path instead of a name was there before that. The actual issue
was introduced in 7dce19d374 (fetch/pull: Add the
--recurse-submodules option, 2010-11-12).

+     name = ce->name;
+     name_for_path =
unsorted_string_list_lookup(&config_name_for_path, ce->name);
+     if (name_for_path)
+         name = name_for_path->util;

Rereading the archives, there was quite some discussion on the design
of these patches, but these lines of code did not get any attention

    https://public-inbox.org/git/4CDB3063.5010801@web.de/

I cc'd Jens in the hope of him having a good memory why he
wrote the code that way. :)

Note that this is the last caller of submodule_from_name being
removed, so I would expect removal of submodule_from_name from
the t/helper/test-submodule-config.c as well as
Documentation/technical/api-submodule-config.txt
in a later part of this series. (Well technically it could go outside
of the series, but in the mean time we'd document and test
dead code)

> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
>  submodule.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/submodule.c b/submodule.c
> index 7e87e4698..fd391aea6 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1177,8 +1177,6 @@ static int get_next_submodule(struct child_process *cp,
>                         continue;
>
>                 submodule = submodule_from_path(&null_oid, ce->name);
> -               if (!submodule)
> -                       submodule = submodule_from_name(&null_oid, ce->name);
>
>                 default_argv = "yes";
>                 if (spf->command_line_option == RECURSE_SUBMODULES_DEFAULT) {
> --
> 2.14.0.rc0.400.g1c36432dff-goog
>

Re: [PATCH 03/15] add, reset: ensure submodules can be added or reset

[Stefan Beller]

On Tue, Jul 25, 2017 at 2:39 PM, Brandon Williams <bmwill@google.com> wrote:
> Commit aee9c7d65 (Submodules: Add the new "ignore" config option for
> diff and status) ...

introduced in 2010, so quite widely spread.

> ...  introduced the ignore configuration option for
> submodules so that configured submodules could be omitted from the
> status and diff commands.  Because this flag is respected in the diff
> machinery it has the unintended consequence of potentially prohibiting
> users from adding or resetting a submodule, even when a path to the
> submodule is explicitly given.
>
> Ensure that submodules can be added or set, even if they are configured
> to be ignored, by setting the `DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG` diff
> flag.
>
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
>  builtin/add.c   | 1 +
>  builtin/reset.c | 1 +
>  2 files changed, 2 insertions(+)
>
> diff --git a/builtin/add.c b/builtin/add.c
> index e888fb8c5..6f271512f 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -116,6 +116,7 @@ int add_files_to_cache(const char *prefix,
>         rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;
>         rev.diffopt.format_callback = update_callback;
>         rev.diffopt.format_callback_data = &data;
> +       rev.diffopt.flags |= DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG;


This flag occurs once in the code base, with the comment:
    /*
     * Unless the user did explicitly request a submodule
     * ignore mode by passing a command line option we do
     * not ignore any changed submodule SHA-1s when
     * comparing index and parent, no matter what is
     * configured. Otherwise we won't commit any
     * submodules which were manually staged, which would
     * be really confusing.
     */
    int diff_flags = DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG;

in prepare_commit, so commit ignores the .gitmodules file.

This allows git-add to add ignored submodules, currently ignored submodules
would have to be added using the plumbing
    git update-index --add --cacheinfo 160000,$SHA1,<gitlink>

This makes sense, though a test demonstrating the change in behavior
would be nice, but git-add doesn't seem to change as it doesn't even load
the git modules config?

>         rev.max_count = 0; /* do not compare unmerged paths with stage #2 */
>         run_diff_files(&rev, DIFF_RACY_IS_MODIFIED);
>         return !!data.add_errors;
> diff --git a/builtin/reset.c b/builtin/reset.c
> index 046403ed6..772d078b8 100644
> --- a/builtin/reset.c
> +++ b/builtin/reset.c
> @@ -156,6 +156,7 @@ static int read_from_tree(const struct pathspec *pathspec,
>         opt.output_format = DIFF_FORMAT_CALLBACK;
>         opt.format_callback = update_index_from_diff;
>         opt.format_callback_data = &intent_to_add;
> +       opt.flags |= DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG;

same here? Also as this is not failing any test, it may be worth adding one
to document the behavior of the "submodule.<name>.ignore" flag in tests?

>
>         if (do_diff_cache(tree_oid, &opt))
>                 return 1;
> --
> 2.14.0.rc0.400.g1c36432dff-goog
>

Re: [PATCH 04/15] submodule--helper: don't overlay config in remote_submodule_branch

[Stefan Beller]

On Tue, Jul 25, 2017 at 2:39 PM, Brandon Williams <bmwill@google.com> wrote:
> Don't rely on overlaying the repository's config on top of the
> submodule-config, instead query the repository's config directly for the
> branch field.
>
> Signed-off-by: Brandon Williams <bmwill@google.com>

Reviewed-by: Stefan Beller <sbeller@google.com>

Re: [PATCH 05/15] submodule--helper: don't overlay config in update-clone

[Stefan Beller]

On Tue, Jul 25, 2017 at 2:39 PM, Brandon Williams <bmwill@google.com> wrote:
> Don't rely on overlaying the repository's config on top of the
> submodule-config, instead query the repository's config directly for the
> url and the update strategy configuration.
>
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
...

> +struct submodule_update_strategy submodule_strategy_with_config_overlayed(struct repository *repo,
> +                                                                         const struct submodule *sub)
> +{
> +       struct submodule_update_strategy strat = sub->update_strategy;
> +       const char *update;
> +       char *key;
> +
> +       key = xstrfmt("submodule.%s.update", sub->name);
> +       if (!repo_config_get_string_const(repo, key, &update)) {
> +               strat.command = NULL;
> +               if (!strcmp(update, "none")) {
> +                       strat.type = SM_UPDATE_NONE;
> +               } else if (!strcmp(update, "checkout")) {
> +                       strat.type = SM_UPDATE_CHECKOUT;
> +               } else if (!strcmp(update, "rebase")) {
> +                       strat.type = SM_UPDATE_REBASE;
> +               } else if (!strcmp(update, "merge")) {
> +                       strat.type = SM_UPDATE_MERGE;
> +               } else if (skip_prefix(update, "!", &update)) {
> +                       strat.type = SM_UPDATE_COMMAND;
> +                       strat.command = update;
> +               } else {
> +                       die("invalid submodule update strategy '%s'", update);
> +               }
> +       }

Can this be simplified by reusing
    parse_submodule_update_strategy(value, dest)
?

Re: [PATCH 03/15] add, reset: ensure submodules can be added or reset

[Brandon Williams]

On 07/25, Stefan Beller wrote:
> On Tue, Jul 25, 2017 at 2:39 PM, Brandon Williams <bmwill@google.com> wrote:
> > Commit aee9c7d65 (Submodules: Add the new "ignore" config option for
> > diff and status) ...
> 
> introduced in 2010, so quite widely spread.
> 
> > ...  introduced the ignore configuration option for
> > submodules so that configured submodules could be omitted from the
> > status and diff commands.  Because this flag is respected in the diff
> > machinery it has the unintended consequence of potentially prohibiting
> > users from adding or resetting a submodule, even when a path to the
> > submodule is explicitly given.
> >
> > Ensure that submodules can be added or set, even if they are configured
> > to be ignored, by setting the `DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG` diff
> > flag.
> >
> > Signed-off-by: Brandon Williams <bmwill@google.com>
> > ---
> >  builtin/add.c   | 1 +
> >  builtin/reset.c | 1 +
> >  2 files changed, 2 insertions(+)
> >
> > diff --git a/builtin/add.c b/builtin/add.c
> > index e888fb8c5..6f271512f 100644
> > --- a/builtin/add.c
> > +++ b/builtin/add.c
> > @@ -116,6 +116,7 @@ int add_files_to_cache(const char *prefix,
> >         rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;
> >         rev.diffopt.format_callback = update_callback;
> >         rev.diffopt.format_callback_data = &data;
> > +       rev.diffopt.flags |= DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG;
> 
> 
> This flag occurs once in the code base, with the comment:
>     /*
>      * Unless the user did explicitly request a submodule
>      * ignore mode by passing a command line option we do
>      * not ignore any changed submodule SHA-1s when
>      * comparing index and parent, no matter what is
>      * configured. Otherwise we won't commit any
>      * submodules which were manually staged, which would
>      * be really confusing.
>      */
>     int diff_flags = DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG;
> 
> in prepare_commit, so commit ignores the .gitmodules file.
> 
> This allows git-add to add ignored submodules, currently ignored submodules
> would have to be added using the plumbing
>     git update-index --add --cacheinfo 160000,$SHA1,<gitlink>
> 
> This makes sense, though a test demonstrating the change in behavior
> would be nice, but git-add doesn't seem to change as it doesn't even load
> the git modules config?

I can add a comment to the code but its already being tested in the
submodule test suite.  The only reason this doesn't cause any changes
now is that the gitmodules config is never loaded, but that may
change if we decide to allow lazy-loading of the gitmodules file (like
the last couple patches in this series do).

-- 
Brandon Williams

Re: [PATCH 05/15] submodule--helper: don't overlay config in update-clone

[Brandon Williams]

On 07/25, Stefan Beller wrote:
> On Tue, Jul 25, 2017 at 2:39 PM, Brandon Williams <bmwill@google.com> wrote:
> > Don't rely on overlaying the repository's config on top of the
> > submodule-config, instead query the repository's config directly for the
> > url and the update strategy configuration.
> >
> > Signed-off-by: Brandon Williams <bmwill@google.com>
> > ---
> ...
> 
> > +struct submodule_update_strategy submodule_strategy_with_config_overlayed(struct repository *repo,
> > +                                                                         const struct submodule *sub)
> > +{
> > +       struct submodule_update_strategy strat = sub->update_strategy;
> > +       const char *update;
> > +       char *key;
> > +
> > +       key = xstrfmt("submodule.%s.update", sub->name);
> > +       if (!repo_config_get_string_const(repo, key, &update)) {
> > +               strat.command = NULL;
> > +               if (!strcmp(update, "none")) {
> > +                       strat.type = SM_UPDATE_NONE;
> > +               } else if (!strcmp(update, "checkout")) {
> > +                       strat.type = SM_UPDATE_CHECKOUT;
> > +               } else if (!strcmp(update, "rebase")) {
> > +                       strat.type = SM_UPDATE_REBASE;
> > +               } else if (!strcmp(update, "merge")) {
> > +                       strat.type = SM_UPDATE_MERGE;
> > +               } else if (skip_prefix(update, "!", &update)) {
> > +                       strat.type = SM_UPDATE_COMMAND;
> > +                       strat.command = update;
> > +               } else {
> > +                       die("invalid submodule update strategy '%s'", update);
> > +               }
> > +       }
> 
> Can this be simplified by reusing
>     parse_submodule_update_strategy(value, dest)
> ?

It would result in a memory leak if we did.  Really I'd like to just
remove this entirely. The only reason this needs to be done is for
checkout, which if we don't have respect the update config it can be
removed.

-- 
Brandon Williams

Re: [PATCH 06/15] fetch: don't overlay config with submodule-config

[Stefan Beller]

On Tue, Jul 25, 2017 at 2:39 PM, Brandon Williams <bmwill@google.com> wrote:
> Don't rely on overlaying the repository's config on top of the
> submodule-config, instead query the repository's config directly for the
> fetch_recurse field.
>
> Signed-off-by: Brandon Williams <bmwill@google.com>

Reviewed-by: Stefan Beller <sbeller@google.com>

> ---
>  builtin/fetch.c |  1 -
>  submodule.c     | 24 +++++++++++++++++-------
>  2 files changed, 17 insertions(+), 8 deletions(-)
>
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index d84c26391..3fe99073d 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -1362,7 +1362,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
>
>         if (recurse_submodules != RECURSE_SUBMODULES_OFF) {
>                 gitmodules_config();
> -               git_config(submodule_config, NULL);
>         }
>
>         if (all) {
> diff --git a/submodule.c b/submodule.c
> index 8b9e48a61..c5058a4b8 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1210,14 +1210,24 @@ static int get_next_submodule(struct child_process *cp,
>
>                 default_argv = "yes";
>                 if (spf->command_line_option == RECURSE_SUBMODULES_DEFAULT) {
> -                       if (submodule &&
> -                           submodule->fetch_recurse !=
> -                                               RECURSE_SUBMODULES_NONE) {
> -                               if (submodule->fetch_recurse ==
> -                                               RECURSE_SUBMODULES_OFF)
> +                       int fetch_recurse = RECURSE_SUBMODULES_NONE;
> +
> +                       if (submodule) {
> +                               char *key;
> +                               const char *value;
> +
> +                               fetch_recurse = submodule->fetch_recurse;
> +                               key = xstrfmt("submodule.%s.fetchRecurseSubmodules", submodule->name);
> +                               if (!repo_config_get_string_const(the_repository, key, &value)) {
> +                                       fetch_recurse = parse_fetch_recurse_submodules_arg(key, value);
> +                               }
> +                               free(key);
> +                       }

I wonder if it would be better to parse this in builtin/fetch.c#git_fetch_config
and then pass it in here as a parameter, instead of looking it up directly here?
That way it is easier to keep track of what a builtin pays attention to.


> +
> +                       if (fetch_recurse != RECURSE_SUBMODULES_NONE) {
> +                               if (fetch_recurse == RECURSE_SUBMODULES_OFF)
>                                         continue;
> -                               if (submodule->fetch_recurse ==
> -                                               RECURSE_SUBMODULES_ON_DEMAND) {
> +                               if (fetch_recurse == RECURSE_SUBMODULES_ON_DEMAND) {
>                                         if (!unsorted_string_list_lookup(&changed_submodule_paths, ce->name))
>                                                 continue;
>                                         default_argv = "on-demand";
> --
> 2.14.0.rc0.400.g1c36432dff-goog
>

Re: [PATCH 07/15] submodule: don't rely on overlayed config when setting diffopts

[Stefan Beller]

On Tue, Jul 25, 2017 at 2:39 PM, Brandon Williams <bmwill@google.com> wrote:
> Don't rely on overlaying the repository's config on top of the
> submodule-config, instead query the repository's config directory for
> the ignore field.
>
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
>  submodule.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/submodule.c b/submodule.c
> index c5058a4b8..f86b82fbb 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -165,8 +165,16 @@ void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,
>  {
>         const struct submodule *submodule = submodule_from_path(&null_oid, path);
>         if (submodule) {
> -               if (submodule->ignore)
> -                       handle_ignore_submodules_arg(diffopt, submodule->ignore);
> +               const char *ignore;
> +               char *key;
> +
> +               key = xstrfmt("submodule.%s.ignore", submodule->name);
> +               if (repo_config_get_string_const(the_repository, key, &ignore))
> +                       ignore = submodule->ignore;

Unlike the last patch, we have to use a direct lookup here as
the alternative is hugely painful.


> +               free(key);
> +
> +               if (ignore)
> +                       handle_ignore_submodules_arg(diffopt, ignore);
>                 else if (is_gitmodules_unmerged(&the_index))
>                         DIFF_OPT_SET(diffopt, IGNORE_SUBMODULES);
>         }
> --
> 2.14.0.rc0.400.g1c36432dff-goog
>

Re: [PATCH 06/15] fetch: don't overlay config with submodule-config

[Brandon Williams]

On 07/25, Stefan Beller wrote:
> On Tue, Jul 25, 2017 at 2:39 PM, Brandon Williams <bmwill@google.com> wrote:
> > Don't rely on overlaying the repository's config on top of the
> > submodule-config, instead query the repository's config directly for the
> > fetch_recurse field.
> >
> > Signed-off-by: Brandon Williams <bmwill@google.com>
> 
> Reviewed-by: Stefan Beller <sbeller@google.com>
> 
> > ---
> >  builtin/fetch.c |  1 -
> >  submodule.c     | 24 +++++++++++++++++-------
> >  2 files changed, 17 insertions(+), 8 deletions(-)
> >
> > diff --git a/builtin/fetch.c b/builtin/fetch.c
> > index d84c26391..3fe99073d 100644
> > --- a/builtin/fetch.c
> > +++ b/builtin/fetch.c
> > @@ -1362,7 +1362,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
> >
> >         if (recurse_submodules != RECURSE_SUBMODULES_OFF) {
> >                 gitmodules_config();
> > -               git_config(submodule_config, NULL);
> >         }
> >
> >         if (all) {
> > diff --git a/submodule.c b/submodule.c
> > index 8b9e48a61..c5058a4b8 100644
> > --- a/submodule.c
> > +++ b/submodule.c
> > @@ -1210,14 +1210,24 @@ static int get_next_submodule(struct child_process *cp,
> >
> >                 default_argv = "yes";
> >                 if (spf->command_line_option == RECURSE_SUBMODULES_DEFAULT) {
> > -                       if (submodule &&
> > -                           submodule->fetch_recurse !=
> > -                                               RECURSE_SUBMODULES_NONE) {
> > -                               if (submodule->fetch_recurse ==
> > -                                               RECURSE_SUBMODULES_OFF)
> > +                       int fetch_recurse = RECURSE_SUBMODULES_NONE;
> > +
> > +                       if (submodule) {
> > +                               char *key;
> > +                               const char *value;
> > +
> > +                               fetch_recurse = submodule->fetch_recurse;
> > +                               key = xstrfmt("submodule.%s.fetchRecurseSubmodules", submodule->name);
> > +                               if (!repo_config_get_string_const(the_repository, key, &value)) {
> > +                                       fetch_recurse = parse_fetch_recurse_submodules_arg(key, value);
> > +                               }
> > +                               free(key);
> > +                       }
> 
> I wonder if it would be better to parse this in builtin/fetch.c#git_fetch_config
> and then pass it in here as a parameter, instead of looking it up directly here?
> That way it is easier to keep track of what a builtin pays attention to.

Really the fact that you can configure individual submodules in
.gitmodules to be fetched recursively or not is a terrible design IMO.
Also this is a per-submodule configuration so having it in
builtin/fetch.c would be incredibly annoying to handle.

> 
> 
> > +
> > +                       if (fetch_recurse != RECURSE_SUBMODULES_NONE) {
> > +                               if (fetch_recurse == RECURSE_SUBMODULES_OFF)
> >                                         continue;
> > -                               if (submodule->fetch_recurse ==
> > -                                               RECURSE_SUBMODULES_ON_DEMAND) {
> > +                               if (fetch_recurse == RECURSE_SUBMODULES_ON_DEMAND) {
> >                                         if (!unsorted_string_list_lookup(&changed_submodule_paths, ce->name))
> >                                                 continue;
> >                                         default_argv = "on-demand";
> > --
> > 2.14.0.rc0.400.g1c36432dff-goog
> >

-- 
Brandon Williams

Re: [PATCH 01/15] t7411: check configuration parsing errors

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

> Check for configuration parsing errors in '.gitmodules' in t7411, which
> is explicitly testing the submodule-config subsystem, instead of in
> t7400.  Also explicitly use the test helper instead of relying on the
> gitmodules file from being read in status.

Makes sense.

> ...
> -	test_must_fail git status
> -'
> -...
> +test_expect_success 'configuration parsing with error' '
> +	test_when_finished "rm -rf repo" &&
> +	test_create_repo repo &&
> +	cat >repo/.gitmodules <<-\EOF &&
> +	[submodule "s"]
> +		path
> +		ignore
> +	EOF
> +	(
> +		cd repo &&
> +		test_must_fail test-submodule-config "" s 2>actual &&
> +		test_i18ngrep "bad config" actual
> +	)
> +'
> +
>  cat >super/expect <<EOF
>  Submodule name: 'a' for path 'a'
>  Submodule name: 'a' for path 'b'

Re: [PATCH 02/15] submodule: don't use submodule_from_name

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Rereading the archives, there was quite some discussion on the design
> of these patches, but these lines of code did not get any attention
>
>     https://public-inbox.org/git/4CDB3063.5010801@web.de/
>
> I cc'd Jens in the hope of him having a good memory why he
> wrote the code that way. :)

Thanks for digging.  I wouldn't be surprised if this were a fallback
to help a broken entry in .gitmodules that lack .path variable, but
we shouldn't be sweeping the problem under the rug like that.  

I wonder if we should barf loudly if there shouldn't be a submodule
at that path, i.e.

	if (!submodule)
		die("there is no submodule defined for path '%s'"...);

though.

> Note that this is the last caller of submodule_from_name being
> removed, so I would expect removal of submodule_from_name from
> the t/helper/test-submodule-config.c as well as
> Documentation/technical/api-submodule-config.txt
> in a later part of this series. (Well technically it could go outside
> of the series, but in the mean time we'd document and test
> dead code)

Good thinking.  As this is "cleanup" series, I think it is within
its scope to remove an API function that becomes unused.

>
>> Signed-off-by: Brandon Williams <bmwill@google.com>
>> ---
>>  submodule.c | 2 --
>>  1 file changed, 2 deletions(-)
>>
>> diff --git a/submodule.c b/submodule.c
>> index 7e87e4698..fd391aea6 100644
>> --- a/submodule.c
>> +++ b/submodule.c
>> @@ -1177,8 +1177,6 @@ static int get_next_submodule(struct child_process *cp,
>>                         continue;
>>
>>                 submodule = submodule_from_path(&null_oid, ce->name);
>> -               if (!submodule)
>> -                       submodule = submodule_from_name(&null_oid, ce->name);
>>
>>                 default_argv = "yes";
>>                 if (spf->command_line_option == RECURSE_SUBMODULES_DEFAULT) {
>> --
>> 2.14.0.rc0.400.g1c36432dff-goog
>>

Re: [PATCH 03/15] add, reset: ensure submodules can be added or reset

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> On Tue, Jul 25, 2017 at 2:39 PM, Brandon Williams <bmwill@google.com> wrote:
>> Commit aee9c7d65 (Submodules: Add the new "ignore" config option for
>> diff and status) ...
>
> introduced in 2010, so quite widely spread.
>
>> ...  introduced the ignore configuration option for
>> submodules so that configured submodules could be omitted from the
>> status and diff commands.  Because this flag is respected in the diff
>> machinery it has the unintended consequence of potentially prohibiting
>> users from adding or resetting a submodule, even when a path to the
>> submodule is explicitly given.
>>
>> Ensure that submodules can be added or set, even if they are configured
>> to be ignored, by setting the `DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG` diff
>> flag.
>>
>> Signed-off-by: Brandon Williams <bmwill@google.com>
>> ---
>>  builtin/add.c   | 1 +
>>  builtin/reset.c | 1 +
>>  2 files changed, 2 insertions(+)
>>
>> diff --git a/builtin/add.c b/builtin/add.c
>> index e888fb8c5..6f271512f 100644
>> --- a/builtin/add.c
>> +++ b/builtin/add.c
>> @@ -116,6 +116,7 @@ int add_files_to_cache(const char *prefix,
>>         rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;
>>         rev.diffopt.format_callback = update_callback;
>>         rev.diffopt.format_callback_data = &data;
>> +       rev.diffopt.flags |= DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG;
>
>
> This flag occurs once in the code base, with the comment:
>     /*
>      * Unless the user did explicitly request a submodule
>      * ignore mode by passing a command line option we do
>      * not ignore any changed submodule SHA-1s when
>      * comparing index and parent, no matter what is
>      * configured. Otherwise we won't commit any
>      * submodules which were manually staged, which would
>      * be really confusing.
>      */
>     int diff_flags = DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG;
>
> in prepare_commit, so commit ignores the .gitmodules file.
>
> This allows git-add to add ignored submodules, currently ignored submodules
> would have to be added using the plumbing
>     git update-index --add --cacheinfo 160000,$SHA1,<gitlink>

Let me play devil's advocate (as I have this suspicion that .ignore
thing specific for submodule is probably misdesigned and certainly
its implementation is backwards).  Is the primary use case for this
.ignore thing to be able to do

	git add .

without having to worry about adding the submodule marked as such?  
And if so, wouldn't it surprise these users who do use .ignore if
"git add" suddenly started adding them?

I think the right tool to use these days for excluding some paths
when adding all others is the negative pathspec; perhaps back when
the .ignore thing was added, it didn't exist or not widely known?  

I suspect that it may result in a better system overall if we can
deprecate and remove the submodule-specific .ignore thing.  At
least, I think the DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG is backwards
in that .ignore causes a submodule to be excluded from the diff by
default and forces paths that care about differences to opt into the
"override" thing, which is wrong---the specific UI thing that wants
not to show them should instead opt into ignoring, while keeping the
default not to special case such a flag that can only be set to a
submodule path.

> This makes sense, though a test demonstrating the change in behavior
> would be nice, but git-add doesn't seem to change as it doesn't even load
> the git modules config?
>
>>         rev.max_count = 0; /* do not compare unmerged paths with stage #2 */
>>         run_diff_files(&rev, DIFF_RACY_IS_MODIFIED);
>>         return !!data.add_errors;
>> diff --git a/builtin/reset.c b/builtin/reset.c
>> index 046403ed6..772d078b8 100644
>> --- a/builtin/reset.c
>> +++ b/builtin/reset.c
>> @@ -156,6 +156,7 @@ static int read_from_tree(const struct pathspec *pathspec,
>>         opt.output_format = DIFF_FORMAT_CALLBACK;
>>         opt.format_callback = update_index_from_diff;
>>         opt.format_callback_data = &intent_to_add;
>> +       opt.flags |= DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG;
>
> same here? Also as this is not failing any test, it may be worth adding one
> to document the behavior of the "submodule.<name>.ignore" flag in tests?
>
>>
>>         if (do_diff_cache(tree_oid, &opt))
>>                 return 1;
>> --
>> 2.14.0.rc0.400.g1c36432dff-goog
>>

Re: [PATCH 09/15] submodule: remove submodule_config callback routine

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

> Remove the last remaining caller of 'submodule_config()' as well as the
> function itself.
>
> With 'submodule_config()' being removed the submodule-config API can be
> a little simpler as callers don't need to worry about whether or not
> they need to overlay the repository's config on top of the
> submodule-config.  This also makes it more difficult to accidentally
> add non-submodule specific configuration to the .gitmodules file.

Nice.

Re: [PATCH 02/15] submodule: don't use submodule_from_name

[Jens Lehmann]

Am 26.07.2017 um 23:06 schrieb Junio C Hamano:
> Stefan Beller <sbeller@google.com> writes:
> 
>> Rereading the archives, there was quite some discussion on the design
>> of these patches, but these lines of code did not get any attention
>>
>>      https://public-inbox.org/git/4CDB3063.5010801@web.de/
>>
>> I cc'd Jens in the hope of him having a good memory why he
>> wrote the code that way. :)
> 
> Thanks for digging.  I wouldn't be surprised if this were a fallback
> to help a broken entry in .gitmodules that lack .path variable, but
> we shouldn't be sweeping the problem under the rug like that.

Sorry to disappoint you ;-) I added this in 7dce19d374 because
submodule by path lookup back then only parsed the checked out
.gitmodules file. So looking for it by name was a good guess to
fetch a new submodule that wasn't present in the current HEAD's
.gitmodules, as the path is used as the default name in "git
submodule add".

The refactoring in 851e18c385 could and should have removed that
because since then we use the .gitmodules path to name mapping
of the fetched commit.

> I wonder if we should barf loudly if there shouldn't be a submodule
> at that path, i.e.
> 
> 	if (!submodule)
> 		die("there is no submodule defined for path '%s'"...);
> 
> though.

Not sure if you want to die() or just issue a warning(), but yes.

Re: [PATCH 02/15] submodule: don't use submodule_from_name

[Junio C Hamano]

Jens Lehmann <Jens.Lehmann@web.de> writes:

>> I wonder if we should barf loudly if there shouldn't be a submodule
>> at that path, i.e.
>>
>> 	if (!submodule)
>> 		die("there is no submodule defined for path '%s'"...);
>>
>> though.
>
> Not sure if you want to die() or just issue a warning(), but yes.

As long as the code after that point is prepared to see a NULL
submodule and still behaves sensibly, then I would of course prefer
not dying.  Continuing with just a warning() may not be a safe thing
to do if we are not prepared to see a NULL submodule after that
point, though.

Re: [PATCH 02/15] submodule: don't use submodule_from_name

[Stefan Beller]

On Sun, Jul 30, 2017 at 6:43 AM, Jens Lehmann <Jens.Lehmann@web.de> wrote:
> Am 26.07.2017 um 23:06 schrieb Junio C Hamano:
>>
>> Stefan Beller <sbeller@google.com> writes:
>>
>>> Rereading the archives, there was quite some discussion on the design
>>> of these patches, but these lines of code did not get any attention
>>>
>>>      https://public-inbox.org/git/4CDB3063.5010801@web.de/
>>>
>>> I cc'd Jens in the hope of him having a good memory why he
>>> wrote the code that way. :)
>>
>>
>> Thanks for digging.  I wouldn't be surprised if this were a fallback
>> to help a broken entry in .gitmodules that lack .path variable, but
>> we shouldn't be sweeping the problem under the rug like that.
>
>
> Sorry to disappoint you ;-) I added this in 7dce19d374 because
> submodule by path lookup back then only parsed the checked out
> .gitmodules file.

This is still the case AFAICT, as we never ask for a specific .gitmodules
file identified by sha1 of the commit.

> So looking for it by name was a good guess to
> fetch a new submodule that wasn't present in the current HEAD's
> .gitmodules, as the path is used as the default name in "git
> submodule add".

3 things:
a) I think it is not as much a feature ('fallback to still make it work'),
   but rather a bug as when there is no (or wrong) entry in the .gitmodules
   file, reporting it is better than trying something.
b) in the case of moved submodules (2 submodules swapped their path)
   this may be harmful as we'd get a wrong submodule potentially.

c) I wonder if we want to use a different default for submodule names
   as I have seen people get confused by path and name being the same,
   e.g. to move a submodule they would have not just adapted the path,
   but any occurrence of the string that reads like the path.
   (i.e. also change the name, defeating the purpose of name/path
   separation).

   For a new name default, I would wager for some non-legible gibberish
   such as "hash( path/time )", as that sends a clear message to not mess
   with the value of the name.

>
> The refactoring in 851e18c385 could and should have removed that
> because since then we use the .gitmodules path to name mapping
> of the fetched commit.
>
>> I wonder if we should barf loudly if there shouldn't be a submodule
>> at that path, i.e.
>>
>>         if (!submodule)
>>                 die("there is no submodule defined for path '%s'"...);
>>
>> though.
>
>
> Not sure if you want to die() or just issue a warning(), but yes.

Either die() or "warning && return 0" is fine with me.

Re: [PATCH 03/15] add, reset: ensure submodules can be added or reset

[Brandon Williams]

On 07/26, Junio C Hamano wrote:
> Stefan Beller <sbeller@google.com> writes:
> 
> > On Tue, Jul 25, 2017 at 2:39 PM, Brandon Williams <bmwill@google.com> wrote:
> >> Commit aee9c7d65 (Submodules: Add the new "ignore" config option for
> >> diff and status) ...
> >
> > introduced in 2010, so quite widely spread.
> >
> >> ...  introduced the ignore configuration option for
> >> submodules so that configured submodules could be omitted from the
> >> status and diff commands.  Because this flag is respected in the diff
> >> machinery it has the unintended consequence of potentially prohibiting
> >> users from adding or resetting a submodule, even when a path to the
> >> submodule is explicitly given.
> >>
> >> Ensure that submodules can be added or set, even if they are configured
> >> to be ignored, by setting the `DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG` diff
> >> flag.
> >>
> >> Signed-off-by: Brandon Williams <bmwill@google.com>
> >> ---
> >>  builtin/add.c   | 1 +
> >>  builtin/reset.c | 1 +
> >>  2 files changed, 2 insertions(+)
> >>
> >> diff --git a/builtin/add.c b/builtin/add.c
> >> index e888fb8c5..6f271512f 100644
> >> --- a/builtin/add.c
> >> +++ b/builtin/add.c
> >> @@ -116,6 +116,7 @@ int add_files_to_cache(const char *prefix,
> >>         rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;
> >>         rev.diffopt.format_callback = update_callback;
> >>         rev.diffopt.format_callback_data = &data;
> >> +       rev.diffopt.flags |= DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG;
> >
> >
> > This flag occurs once in the code base, with the comment:
> >     /*
> >      * Unless the user did explicitly request a submodule
> >      * ignore mode by passing a command line option we do
> >      * not ignore any changed submodule SHA-1s when
> >      * comparing index and parent, no matter what is
> >      * configured. Otherwise we won't commit any
> >      * submodules which were manually staged, which would
> >      * be really confusing.
> >      */
> >     int diff_flags = DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG;
> >
> > in prepare_commit, so commit ignores the .gitmodules file.
> >
> > This allows git-add to add ignored submodules, currently ignored submodules
> > would have to be added using the plumbing
> >     git update-index --add --cacheinfo 160000,$SHA1,<gitlink>
> 
> Let me play devil's advocate (as I have this suspicion that .ignore
> thing specific for submodule is probably misdesigned and certainly
> its implementation is backwards).  Is the primary use case for this
> .ignore thing to be able to do
> 
> 	git add .
> 
> without having to worry about adding the submodule marked as such?  
> And if so, wouldn't it surprise these users who do use .ignore if
> "git add" suddenly started adding them?
> 
> I think the right tool to use these days for excluding some paths
> when adding all others is the negative pathspec; perhaps back when
> the .ignore thing was added, it didn't exist or not widely known?  
> 
> I suspect that it may result in a better system overall if we can
> deprecate and remove the submodule-specific .ignore thing.  At
> least, I think the DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG is backwards
> in that .ignore causes a submodule to be excluded from the diff by
> default and forces paths that care about differences to opt into the
> "override" thing, which is wrong---the specific UI thing that wants
> not to show them should instead opt into ignoring, while keeping the
> default not to special case such a flag that can only be set to a
> submodule path.

It looks like .ignore was added with aee9c7d65 (Submodules: Add the new
"ignore" config option for diff and status, 2010-08-06) in order to
ignore particular submodules with 'status' and 'diff' commands.  I don't
think it was intended to ignore submodules with commands like add and
reset.  Either way I agree that some of the things with most of the
submodules config seem a bit backwards and we may want to migrate away
from them completely as we begin to add more support for submodules into
the builtin commands.

> 
> > This makes sense, though a test demonstrating the change in behavior
> > would be nice, but git-add doesn't seem to change as it doesn't even load
> > the git modules config?
> >
> >>         rev.max_count = 0; /* do not compare unmerged paths with stage #2 */
> >>         run_diff_files(&rev, DIFF_RACY_IS_MODIFIED);
> >>         return !!data.add_errors;
> >> diff --git a/builtin/reset.c b/builtin/reset.c
> >> index 046403ed6..772d078b8 100644
> >> --- a/builtin/reset.c
> >> +++ b/builtin/reset.c
> >> @@ -156,6 +156,7 @@ static int read_from_tree(const struct pathspec *pathspec,
> >>         opt.output_format = DIFF_FORMAT_CALLBACK;
> >>         opt.format_callback = update_index_from_diff;
> >>         opt.format_callback_data = &intent_to_add;
> >> +       opt.flags |= DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG;
> >
> > same here? Also as this is not failing any test, it may be worth adding one
> > to document the behavior of the "submodule.<name>.ignore" flag in tests?
> >
> >>
> >>         if (do_diff_cache(tree_oid, &opt))
> >>                 return 1;
> >> --
> >> 2.14.0.rc0.400.g1c36432dff-goog
> >>

-- 
Brandon Williams

[PATCH v2 00/15] submodule-config cleanup

[Brandon Williams]

Changes in v2:
 * Rebased on latest 'bw/grep-recurse-submodules' branch (Still also requires
   the 'bc/object-id' series).
 * Changed unpack-trees.c (checkout command) so that it no longer respects the
   'submodule.<name>.update' config since it really didn't make much sense for
   it to respect it.
 * The above point also enabled me to fix some issues that coverity found with
   how I was overlaying the repo config with the submodule update strategy.
   Instead the update strategy parsing logic is separated into two functions so
   that just the enum can be determined from a string (which is all
   update-clone needed).

Brandon Williams (15):
  t7411: check configuration parsing errors
  submodule: don't use submodule_from_name
  add, reset: ensure submodules can be added or reset
  submodule--helper: don't overlay config in remote_submodule_branch
  submodule--helper: don't overlay config in update-clone
  fetch: don't overlay config with submodule-config
  submodule: don't rely on overlayed config when setting diffopts
  unpack-trees: don't respect submodule.update
  submodule: remove submodule_config callback routine
  diff: stop allowing diff to have submodules configured in .git/config
  submodule-config: remove support for overlaying repository config
  submodule-config: move submodule-config functions to
    submodule-config.c
  submodule-config: lazy-load a repository's .gitmodules file
  unpack-trees: improve loading of .gitmodules
  submodule: remove gitmodules_config

 builtin/add.c                    |   1 +
 builtin/checkout.c               |   3 +-
 builtin/commit.c                 |   1 -
 builtin/diff-files.c             |   1 -
 builtin/diff-index.c             |   1 -
 builtin/diff-tree.c              |   1 -
 builtin/diff.c                   |   2 -
 builtin/fetch.c                  |   5 --
 builtin/grep.c                   |   4 --
 builtin/ls-files.c               |   6 +-
 builtin/mv.c                     |   1 -
 builtin/read-tree.c              |   2 -
 builtin/reset.c                  |   3 +-
 builtin/rm.c                     |   1 -
 builtin/submodule--helper.c      |  51 ++++++++------
 diff.c                           |   3 -
 submodule-config.c               |  65 +++++++++++++----
 submodule-config.h               |   8 +--
 submodule.c                      | 148 ++++++++++++++-------------------------
 submodule.h                      |   6 +-
 t/helper/test-submodule-config.c |   7 --
 t/t4027-diff-submodule.sh        |  67 ------------------
 t/t7400-submodule-basic.sh       |  10 ---
 t/t7411-submodule-config.sh      |  87 ++++-------------------
 unpack-trees.c                   |  81 +++++++++------------
 25 files changed, 192 insertions(+), 373 deletions(-)

-- 
2.14.0.rc1.383.gd1ce394fe2-goog

[PATCH v2 01/15] t7411: check configuration parsing errors

[Brandon Williams]

Check for configuration parsing errors in '.gitmodules' in t7411, which
is explicitly testing the submodule-config subsystem, instead of in
t7400.  Also explicitly use the test helper instead of relying on the
gitmodules file from being read in status.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 t/t7400-submodule-basic.sh  | 10 ----------
 t/t7411-submodule-config.sh | 15 +++++++++++++++
 2 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index dcac364c5..717447526 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -46,16 +46,6 @@ test_expect_success 'submodule update aborts on missing gitmodules url' '
 	test_must_fail git submodule init
 '
 
-test_expect_success 'configuration parsing' '
-	test_when_finished "rm -f .gitmodules" &&
-	cat >.gitmodules <<-\EOF &&
-	[submodule "s"]
-		path
-		ignore
-	EOF
-	test_must_fail git status
-'
-
 test_expect_success 'setup - repository in init subdirectory' '
 	mkdir init &&
 	(
diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
index eea36f1db..7d6b25ba2 100755
--- a/t/t7411-submodule-config.sh
+++ b/t/t7411-submodule-config.sh
@@ -31,6 +31,21 @@ test_expect_success 'submodule config cache setup' '
 	)
 '
 
+test_expect_success 'configuration parsing with error' '
+	test_when_finished "rm -rf repo" &&
+	test_create_repo repo &&
+	cat >repo/.gitmodules <<-\EOF &&
+	[submodule "s"]
+		path
+		ignore
+	EOF
+	(
+		cd repo &&
+		test_must_fail test-submodule-config "" s 2>actual &&
+		test_i18ngrep "bad config" actual
+	)
+'
+
 cat >super/expect <<EOF
 Submodule name: 'a' for path 'a'
 Submodule name: 'a' for path 'b'
-- 
2.14.0.rc1.383.gd1ce394fe2-goog

[PATCH v2 02/15] submodule: don't use submodule_from_name

[Brandon Williams]

The function 'submodule_from_name()' is being used incorrectly here as a
submodule path is being used instead of a submodule name.  Since the
correct function to use with a path to a submodule is already being used
('submodule_from_path()') let's remove the call to
'submodule_from_name()'.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 submodule.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/submodule.c b/submodule.c
index 5139b9256..19bd13bb2 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1177,8 +1177,6 @@ static int get_next_submodule(struct child_process *cp,
 			continue;
 
 		submodule = submodule_from_path(&null_oid, ce->name);
-		if (!submodule)
-			submodule = submodule_from_name(&null_oid, ce->name);
 
 		default_argv = "yes";
 		if (spf->command_line_option == RECURSE_SUBMODULES_DEFAULT) {
-- 
2.14.0.rc1.383.gd1ce394fe2-goog

[PATCH v2 03/15] add, reset: ensure submodules can be added or reset

[Brandon Williams]

Commit aee9c7d65 (Submodules: Add the new "ignore" config option for
diff and status) introduced the ignore configuration option for
submodules so that configured submodules could be omitted from the
status and diff commands.  Because this flag is respected in the diff
machinery it has the unintended consequence of potentially prohibiting
users from adding or resetting a submodule, even when a path to the
submodule is explicitly given.

Ensure that submodules can be added or set, even if they are configured
to be ignored, by setting the `DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG` diff
flag.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 builtin/add.c   | 1 +
 builtin/reset.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/builtin/add.c b/builtin/add.c
index e888fb8c5..6f271512f 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -116,6 +116,7 @@ int add_files_to_cache(const char *prefix,
 	rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;
 	rev.diffopt.format_callback = update_callback;
 	rev.diffopt.format_callback_data = &data;
+	rev.diffopt.flags |= DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG;
 	rev.max_count = 0; /* do not compare unmerged paths with stage #2 */
 	run_diff_files(&rev, DIFF_RACY_IS_MODIFIED);
 	return !!data.add_errors;
diff --git a/builtin/reset.c b/builtin/reset.c
index 046403ed6..772d078b8 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -156,6 +156,7 @@ static int read_from_tree(const struct pathspec *pathspec,
 	opt.output_format = DIFF_FORMAT_CALLBACK;
 	opt.format_callback = update_index_from_diff;
 	opt.format_callback_data = &intent_to_add;
+	opt.flags |= DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG;
 
 	if (do_diff_cache(tree_oid, &opt))
 		return 1;
-- 
2.14.0.rc1.383.gd1ce394fe2-goog

[PATCH v2 04/15] submodule--helper: don't overlay config in remote_submodule_branch

[Brandon Williams]

Don't rely on overlaying the repository's config on top of the
submodule-config, instead query the repository's config directly for the
branch field.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 builtin/submodule--helper.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 1e49ce580..f71f4270d 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1066,17 +1066,24 @@ static int resolve_relative_path(int argc, const char **argv, const char *prefix
 static const char *remote_submodule_branch(const char *path)
 {
 	const struct submodule *sub;
+	const char *branch = NULL;
+	char *key;
+
 	gitmodules_config();
-	git_config(submodule_config, NULL);
 
 	sub = submodule_from_path(&null_oid, path);
 	if (!sub)
 		return NULL;
 
-	if (!sub->branch)
+	key = xstrfmt("submodule.%s.branch", sub->name);
+	if (repo_config_get_string_const(the_repository, key, &branch))
+		branch = sub->branch;
+	free(key);
+
+	if (!branch)
 		return "master";
 
-	if (!strcmp(sub->branch, ".")) {
+	if (!strcmp(branch, ".")) {
 		unsigned char sha1[20];
 		const char *refname = resolve_ref_unsafe("HEAD", 0, sha1, NULL);
 
@@ -1094,7 +1101,7 @@ static const char *remote_submodule_branch(const char *path)
 		return refname;
 	}
 
-	return sub->branch;
+	return branch;
 }
 
 static int resolve_remote_submodule_branch(int argc, const char **argv,
-- 
2.14.0.rc1.383.gd1ce394fe2-goog

[PATCH v2 05/15] submodule--helper: don't overlay config in update-clone

[Brandon Williams]

Don't rely on overlaying the repository's config on top of the
submodule-config, instead query the repository's config directly for the
url and the update strategy configuration.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 builtin/submodule--helper.c | 23 +++++++++++++++++++----
 submodule.c                 | 38 ++++++++++++++++++++++++++------------
 submodule.h                 |  1 +
 3 files changed, 46 insertions(+), 16 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index f71f4270d..36df7ab78 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -780,6 +780,10 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 					   struct strbuf *out)
 {
 	const struct submodule *sub = NULL;
+	const char *url = NULL;
+	const char *update_string;
+	enum submodule_update_type update_type;
+	char *key;
 	struct strbuf displaypath_sb = STRBUF_INIT;
 	struct strbuf sb = STRBUF_INIT;
 	const char *displaypath = NULL;
@@ -808,9 +812,17 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 		goto cleanup;
 	}
 
+	key = xstrfmt("submodule.%s.update", sub->name);
+	if (!repo_config_get_string_const(the_repository, key, &update_string)) {
+		update_type = parse_submodule_update_type(update_string);
+	} else {
+		update_type = sub->update_strategy.type;
+	}
+	free(key);
+
 	if (suc->update.type == SM_UPDATE_NONE
 	    || (suc->update.type == SM_UPDATE_UNSPECIFIED
-		&& sub->update_strategy.type == SM_UPDATE_NONE)) {
+		&& update_type == SM_UPDATE_NONE)) {
 		strbuf_addf(out, _("Skipping submodule '%s'"), displaypath);
 		strbuf_addch(out, '\n');
 		goto cleanup;
@@ -822,6 +834,11 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 		goto cleanup;
 	}
 
+	strbuf_reset(&sb);
+	strbuf_addf(&sb, "submodule.%s.url", sub->name);
+	if (repo_config_get_string_const(the_repository, sb.buf, &url))
+		url = sub->url;
+
 	strbuf_reset(&sb);
 	strbuf_addf(&sb, "%s/.git", ce->name);
 	needs_cloning = !file_exists(sb.buf);
@@ -851,7 +868,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 		argv_array_push(&child->args, "--depth=1");
 	argv_array_pushl(&child->args, "--path", sub->path, NULL);
 	argv_array_pushl(&child->args, "--name", sub->name, NULL);
-	argv_array_pushl(&child->args, "--url", sub->url, NULL);
+	argv_array_pushl(&child->args, "--url", url, NULL);
 	if (suc->references.nr) {
 		struct string_list_item *item;
 		for_each_string_list_item(item, &suc->references)
@@ -1025,9 +1042,7 @@ static int update_clone(int argc, const char **argv, const char *prefix)
 	if (pathspec.nr)
 		suc.warn_if_uninitialized = 1;
 
-	/* Overlay the parsed .gitmodules file with .git/config */
 	gitmodules_config();
-	git_config(submodule_config, NULL);
 
 	run_processes_parallel(max_jobs,
 			       update_clone_get_next_task,
diff --git a/submodule.c b/submodule.c
index 19bd13bb2..8a9b964ce 100644
--- a/submodule.c
+++ b/submodule.c
@@ -398,24 +398,38 @@ void die_path_inside_submodule(const struct index_state *istate,
 	}
 }
 
-int parse_submodule_update_strategy(const char *value,
-		struct submodule_update_strategy *dst)
+enum submodule_update_type parse_submodule_update_type(const char *value)
 {
-	free((void*)dst->command);
-	dst->command = NULL;
 	if (!strcmp(value, "none"))
-		dst->type = SM_UPDATE_NONE;
+		return SM_UPDATE_NONE;
 	else if (!strcmp(value, "checkout"))
-		dst->type = SM_UPDATE_CHECKOUT;
+		return SM_UPDATE_CHECKOUT;
 	else if (!strcmp(value, "rebase"))
-		dst->type = SM_UPDATE_REBASE;
+		return SM_UPDATE_REBASE;
 	else if (!strcmp(value, "merge"))
-		dst->type = SM_UPDATE_MERGE;
-	else if (skip_prefix(value, "!", &value)) {
-		dst->type = SM_UPDATE_COMMAND;
-		dst->command = xstrdup(value);
-	} else
+		return SM_UPDATE_MERGE;
+	else if (*value == '!')
+		return SM_UPDATE_COMMAND;
+	else
+		return SM_UPDATE_UNSPECIFIED;
+}
+
+int parse_submodule_update_strategy(const char *value,
+		struct submodule_update_strategy *dst)
+{
+	enum submodule_update_type type;
+
+	free((void*)dst->command);
+	dst->command = NULL;
+
+	type = parse_submodule_update_type(value);
+	if (type == SM_UPDATE_UNSPECIFIED)
 		return -1;
+
+	dst->type = type;
+	if (type == SM_UPDATE_COMMAND)
+		dst->command = xstrdup(value + 1);
+
 	return 0;
 }
 
diff --git a/submodule.h b/submodule.h
index e402b004f..48586efe7 100644
--- a/submodule.h
+++ b/submodule.h
@@ -62,6 +62,7 @@ extern void die_in_unpopulated_submodule(const struct index_state *istate,
 					 const char *prefix);
 extern void die_path_inside_submodule(const struct index_state *istate,
 				      const struct pathspec *ps);
+extern enum submodule_update_type parse_submodule_update_type(const char *value);
 extern int parse_submodule_update_strategy(const char *value,
 		struct submodule_update_strategy *dst);
 extern const char *submodule_strategy_to_string(const struct submodule_update_strategy *s);
-- 
2.14.0.rc1.383.gd1ce394fe2-goog

[PATCH v2 06/15] fetch: don't overlay config with submodule-config

[Brandon Williams]

Don't rely on overlaying the repository's config on top of the
submodule-config, instead query the repository's config directly for the
fetch_recurse field.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 builtin/fetch.c |  1 -
 submodule.c     | 24 +++++++++++++++++-------
 2 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index d84c26391..3fe99073d 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1362,7 +1362,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 
 	if (recurse_submodules != RECURSE_SUBMODULES_OFF) {
 		gitmodules_config();
-		git_config(submodule_config, NULL);
 	}
 
 	if (all) {
diff --git a/submodule.c b/submodule.c
index 8a9b964ce..59e3d0828 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1194,14 +1194,24 @@ static int get_next_submodule(struct child_process *cp,
 
 		default_argv = "yes";
 		if (spf->command_line_option == RECURSE_SUBMODULES_DEFAULT) {
-			if (submodule &&
-			    submodule->fetch_recurse !=
-						RECURSE_SUBMODULES_NONE) {
-				if (submodule->fetch_recurse ==
-						RECURSE_SUBMODULES_OFF)
+			int fetch_recurse = RECURSE_SUBMODULES_NONE;
+
+			if (submodule) {
+				char *key;
+				const char *value;
+
+				fetch_recurse = submodule->fetch_recurse;
+				key = xstrfmt("submodule.%s.fetchRecurseSubmodules", submodule->name);
+				if (!repo_config_get_string_const(the_repository, key, &value)) {
+					fetch_recurse = parse_fetch_recurse_submodules_arg(key, value);
+				}
+				free(key);
+			}
+
+			if (fetch_recurse != RECURSE_SUBMODULES_NONE) {
+				if (fetch_recurse == RECURSE_SUBMODULES_OFF)
 					continue;
-				if (submodule->fetch_recurse ==
-						RECURSE_SUBMODULES_ON_DEMAND) {
+				if (fetch_recurse == RECURSE_SUBMODULES_ON_DEMAND) {
 					if (!unsorted_string_list_lookup(&changed_submodule_paths, ce->name))
 						continue;
 					default_argv = "on-demand";
-- 
2.14.0.rc1.383.gd1ce394fe2-goog

[PATCH v2 07/15] submodule: don't rely on overlayed config when setting diffopts

[Brandon Williams]

Don't rely on overlaying the repository's config on top of the
submodule-config, instead query the repository's config directory for
the ignore field.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 submodule.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/submodule.c b/submodule.c
index 59e3d0828..a32043893 100644
--- a/submodule.c
+++ b/submodule.c
@@ -165,8 +165,16 @@ void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,
 {
 	const struct submodule *submodule = submodule_from_path(&null_oid, path);
 	if (submodule) {
-		if (submodule->ignore)
-			handle_ignore_submodules_arg(diffopt, submodule->ignore);
+		const char *ignore;
+		char *key;
+
+		key = xstrfmt("submodule.%s.ignore", submodule->name);
+		if (repo_config_get_string_const(the_repository, key, &ignore))
+			ignore = submodule->ignore;
+		free(key);
+
+		if (ignore)
+			handle_ignore_submodules_arg(diffopt, ignore);
 		else if (is_gitmodules_unmerged(&the_index))
 			DIFF_OPT_SET(diffopt, IGNORE_SUBMODULES);
 	}
-- 
2.14.0.rc1.383.gd1ce394fe2-goog

[PATCH v2 08/15] unpack-trees: don't respect submodule.update

[Brandon Williams]

The 'submodule.update' config was historically used and respected by the
'submodule update' command because update handled a variety of different
ways it updated a submodule.  As we begin teaching other commands about
submodules it makes more sense for the different settings of
'submodule.update' to be handled by the individual commands themselves
(checkout, rebase, merge, etc) so it shouldn't be respected by the
native checkout command.

Also remove the overlaying of the repository's config (via using
'submodule_config()') from the commands which use the unpack-trees
logic (checkout, read-tree, reset).

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 builtin/checkout.c |  2 +-
 submodule.c        |  1 -
 unpack-trees.c     | 38 ++++++++------------------------------
 3 files changed, 9 insertions(+), 32 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 9661e1bcb..246e0cd16 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -858,7 +858,7 @@ static int git_checkout_config(const char *var, const char *value, void *cb)
 	}
 
 	if (starts_with(var, "submodule."))
-		return submodule_config(var, value, NULL);
+		return git_default_submodule_config(var, value, NULL);
 
 	return git_xmerge_config(var, value, NULL);
 }
diff --git a/submodule.c b/submodule.c
index a32043893..f913c2341 100644
--- a/submodule.c
+++ b/submodule.c
@@ -235,7 +235,6 @@ void load_submodule_cache(void)
 		return;
 
 	gitmodules_config();
-	git_config(submodule_config, NULL);
 }
 
 static int gitmodules_cb(const char *var, const char *value, void *data)
diff --git a/unpack-trees.c b/unpack-trees.c
index 05335fe5b..5dce7ff7d 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -255,28 +255,17 @@ static int check_submodule_move_head(const struct cache_entry *ce,
 {
 	unsigned flags = SUBMODULE_MOVE_HEAD_DRY_RUN;
 	const struct submodule *sub = submodule_from_ce(ce);
+
 	if (!sub)
 		return 0;
 
 	if (o->reset)
 		flags |= SUBMODULE_MOVE_HEAD_FORCE;
 
-	switch (sub->update_strategy.type) {
-	case SM_UPDATE_UNSPECIFIED:
-	case SM_UPDATE_CHECKOUT:
-		if (submodule_move_head(ce->name, old_id, new_id, flags))
-			return o->gently ? -1 :
-				add_rejected_path(o, ERROR_WOULD_LOSE_SUBMODULE, ce->name);
-		return 0;
-	case SM_UPDATE_NONE:
-		return 0;
-	case SM_UPDATE_REBASE:
-	case SM_UPDATE_MERGE:
-	case SM_UPDATE_COMMAND:
-	default:
-		warning(_("submodule update strategy not supported for submodule '%s'"), ce->name);
-		return -1;
-	}
+	if (submodule_move_head(ce->name, old_id, new_id, flags))
+		return o->gently ? -1 :
+				   add_rejected_path(o, ERROR_WOULD_LOSE_SUBMODULE, ce->name);
+	return 0;
 }
 
 static void reload_gitmodules_file(struct index_state *index,
@@ -293,7 +282,6 @@ static void reload_gitmodules_file(struct index_state *index,
 				submodule_free();
 				checkout_entry(ce, state, NULL);
 				gitmodules_config();
-				git_config(submodule_config, NULL);
 			} else
 				break;
 		}
@@ -308,19 +296,9 @@ static void unlink_entry(const struct cache_entry *ce)
 {
 	const struct submodule *sub = submodule_from_ce(ce);
 	if (sub) {
-		switch (sub->update_strategy.type) {
-		case SM_UPDATE_UNSPECIFIED:
-		case SM_UPDATE_CHECKOUT:
-		case SM_UPDATE_REBASE:
-		case SM_UPDATE_MERGE:
-			/* state.force is set at the caller. */
-			submodule_move_head(ce->name, "HEAD", NULL,
-					    SUBMODULE_MOVE_HEAD_FORCE);
-			break;
-		case SM_UPDATE_NONE:
-		case SM_UPDATE_COMMAND:
-			return; /* Do not touch the submodule. */
-		}
+		/* state.force is set at the caller. */
+		submodule_move_head(ce->name, "HEAD", NULL,
+				    SUBMODULE_MOVE_HEAD_FORCE);
 	}
 	if (!check_leading_path(ce->name, ce_namelen(ce)))
 		return;
-- 
2.14.0.rc1.383.gd1ce394fe2-goog

[PATCH v2 09/15] submodule: remove submodule_config callback routine

[Brandon Williams]

Remove the last remaining caller of 'submodule_config()' as well as the
function itself.

With 'submodule_config()' being removed the submodule-config API can be
a little simpler as callers don't need to worry about whether or not
they need to overlay the repository's config on top of the
submodule-config.  This also makes it more difficult to accidentally
add non-submodule specific configuration to the .gitmodules file.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 builtin/submodule--helper.c |  1 -
 submodule.c                 | 25 ++-----------------------
 submodule.h                 |  1 -
 3 files changed, 2 insertions(+), 25 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 36df7ab78..ba767c704 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1205,7 +1205,6 @@ 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);
 
 	if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)
 		return 1;
diff --git a/submodule.c b/submodule.c
index f913c2341..3b383d8c4 100644
--- a/submodule.c
+++ b/submodule.c
@@ -180,27 +180,6 @@ void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,
 	}
 }
 
-/* For loading from the .gitmodules file. */
-static int git_modules_config(const char *var, const char *value, void *cb)
-{
-	if (starts_with(var, "submodule."))
-		return parse_submodule_config_option(var, value);
-	return 0;
-}
-
-/* Loads all submodule settings from the config. */
-int submodule_config(const char *var, const char *value, void *cb)
-{
-	if (!strcmp(var, "submodule.recurse")) {
-		int v = git_config_bool(var, value) ?
-			RECURSE_SUBMODULES_ON : RECURSE_SUBMODULES_OFF;
-		config_update_recurse_submodules = v;
-		return 0;
-	} else {
-		return git_modules_config(var, value, cb);
-	}
-}
-
 /* Cheap function that only determines if we're interested in submodules at all */
 int git_default_submodule_config(const char *var, const char *value, void *cb)
 {
@@ -271,8 +250,8 @@ void gitmodules_config_oid(const struct object_id *commit_oid)
 	struct object_id oid;
 
 	if (gitmodule_oid_from_commit(commit_oid, &oid, &rev)) {
-		git_config_from_blob_oid(submodule_config, rev.buf,
-					 &oid, NULL);
+		git_config_from_blob_oid(gitmodules_cb, rev.buf,
+					 &oid, the_repository);
 	}
 	strbuf_release(&rev);
 }
diff --git a/submodule.h b/submodule.h
index 48586efe7..4f70c4944 100644
--- a/submodule.h
+++ b/submodule.h
@@ -40,7 +40,6 @@ 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 int git_default_submodule_config(const char *var, const char *value, void *cb);
 
 struct option;
-- 
2.14.0.rc1.383.gd1ce394fe2-goog

[PATCH v2 10/15] diff: stop allowing diff to have submodules configured in .git/config

[Brandon Williams]

Traditionally a submodule is comprised of a gitlink as well as a
corresponding entry in the .gitmodules file.  Diff doesn't follow this
paradigm as its config callback routine falls back to populating the
submodule-config if a config entry starts with 'submodule.'.

Remove this behavior in order to be consistent with how the
submodule-config is populated, via calling 'gitmodules_config()' or
'repo_read_gitmodules()'.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 diff.c                    |  3 ---
 t/t4027-diff-submodule.sh | 67 -----------------------------------------------
 2 files changed, 70 deletions(-)

diff --git a/diff.c b/diff.c
index 85e714f6c..e43519b88 100644
--- a/diff.c
+++ b/diff.c
@@ -346,9 +346,6 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
-	if (starts_with(var, "submodule."))
-		return parse_submodule_config_option(var, value);
-
 	if (git_diff_heuristic_config(var, value, cb) < 0)
 		return -1;
 
diff --git a/t/t4027-diff-submodule.sh b/t/t4027-diff-submodule.sh
index 518bf9524..2ffd11a14 100755
--- a/t/t4027-diff-submodule.sh
+++ b/t/t4027-diff-submodule.sh
@@ -113,35 +113,6 @@ test_expect_success 'git diff HEAD with dirty submodule (work tree, refs match)'
 	! test -s actual4
 '
 
-test_expect_success 'git diff HEAD with dirty submodule (work tree, refs match) [.git/config]' '
-	git config diff.ignoreSubmodules all &&
-	git diff HEAD >actual &&
-	! test -s actual &&
-	git config submodule.subname.ignore none &&
-	git config submodule.subname.path sub &&
-	git diff HEAD >actual &&
-	sed -e "1,/^@@/d" actual >actual.body &&
-	expect_from_to >expect.body $subprev $subprev-dirty &&
-	test_cmp expect.body actual.body &&
-	git config submodule.subname.ignore all &&
-	git diff HEAD >actual2 &&
-	! test -s actual2 &&
-	git config submodule.subname.ignore untracked &&
-	git diff HEAD >actual3 &&
-	sed -e "1,/^@@/d" actual3 >actual3.body &&
-	expect_from_to >expect.body $subprev $subprev-dirty &&
-	test_cmp expect.body actual3.body &&
-	git config submodule.subname.ignore dirty &&
-	git diff HEAD >actual4 &&
-	! test -s actual4 &&
-	git diff HEAD --ignore-submodules=none >actual &&
-	sed -e "1,/^@@/d" actual >actual.body &&
-	expect_from_to >expect.body $subprev $subprev-dirty &&
-	test_cmp expect.body actual.body &&
-	git config --remove-section submodule.subname &&
-	git config --unset diff.ignoreSubmodules
-'
-
 test_expect_success 'git diff HEAD with dirty submodule (work tree, refs match) [.gitmodules]' '
 	git config diff.ignoreSubmodules dirty &&
 	git diff HEAD >actual &&
@@ -208,24 +179,6 @@ test_expect_success 'git diff HEAD with dirty submodule (untracked, refs match)'
 	! test -s actual4
 '
 
-test_expect_success 'git diff HEAD with dirty submodule (untracked, refs match) [.git/config]' '
-	git config submodule.subname.ignore all &&
-	git config submodule.subname.path sub &&
-	git diff HEAD >actual2 &&
-	! test -s actual2 &&
-	git config submodule.subname.ignore untracked &&
-	git diff HEAD >actual3 &&
-	! test -s actual3 &&
-	git config submodule.subname.ignore dirty &&
-	git diff HEAD >actual4 &&
-	! test -s actual4 &&
-	git diff --ignore-submodules=none HEAD >actual &&
-	sed -e "1,/^@@/d" actual >actual.body &&
-	expect_from_to >expect.body $subprev $subprev-dirty &&
-	test_cmp expect.body actual.body &&
-	git config --remove-section submodule.subname
-'
-
 test_expect_success 'git diff HEAD with dirty submodule (untracked, refs match) [.gitmodules]' '
 	git config --add -f .gitmodules submodule.subname.ignore all &&
 	git config --add -f .gitmodules submodule.subname.path sub &&
@@ -261,26 +214,6 @@ test_expect_success 'git diff between submodule commits' '
 	! test -s actual
 '
 
-test_expect_success 'git diff between submodule commits [.git/config]' '
-	git diff HEAD^..HEAD >actual &&
-	sed -e "1,/^@@/d" actual >actual.body &&
-	expect_from_to >expect.body $subtip $subprev &&
-	test_cmp expect.body actual.body &&
-	git config submodule.subname.ignore dirty &&
-	git config submodule.subname.path sub &&
-	git diff HEAD^..HEAD >actual &&
-	sed -e "1,/^@@/d" actual >actual.body &&
-	expect_from_to >expect.body $subtip $subprev &&
-	test_cmp expect.body actual.body &&
-	git config submodule.subname.ignore all &&
-	git diff HEAD^..HEAD >actual &&
-	! test -s actual &&
-	git diff --ignore-submodules=dirty HEAD^..HEAD >actual &&
-	sed -e "1,/^@@/d" actual >actual.body &&
-	expect_from_to >expect.body $subtip $subprev &&
-	git config --remove-section submodule.subname
-'
-
 test_expect_success 'git diff between submodule commits [.gitmodules]' '
 	git diff HEAD^..HEAD >actual &&
 	sed -e "1,/^@@/d" actual >actual.body &&
-- 
2.14.0.rc1.383.gd1ce394fe2-goog

[PATCH v2 11/15] submodule-config: remove support for overlaying repository config

[Brandon Williams]

All callers have been migrated to explicitly read any configuration they
need.  The support for handling it automatically in submodule-config is
no longer needed.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 submodule-config.h               |  1 -
 t/helper/test-submodule-config.c |  6 ----
 t/t7411-submodule-config.sh      | 72 ----------------------------------------
 3 files changed, 79 deletions(-)

diff --git a/submodule-config.h b/submodule-config.h
index cccd34b92..84c2cf515 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -34,7 +34,6 @@ extern int option_fetch_parse_recurse_submodules(const struct option *opt,
 						 const char *arg, int unset);
 extern int parse_update_recurse_submodules_arg(const char *opt, const char *arg);
 extern int parse_push_recurse_submodules_arg(const char *opt, const char *arg);
-extern int parse_submodule_config_option(const char *var, const char *value);
 extern int submodule_config_option(struct repository *repo,
 				   const char *var, const char *value);
 extern const struct submodule *submodule_from_name(
diff --git a/t/helper/test-submodule-config.c b/t/helper/test-submodule-config.c
index e13fbcc1b..f4a7c431c 100644
--- a/t/helper/test-submodule-config.c
+++ b/t/helper/test-submodule-config.c
@@ -10,11 +10,6 @@ static void die_usage(int argc, const char **argv, const char *msg)
 	exit(1);
 }
 
-static int git_test_config(const char *var, const char *value, void *cb)
-{
-	return parse_submodule_config_option(var, value);
-}
-
 int cmd_main(int argc, const char **argv)
 {
 	const char **arg = argv;
@@ -38,7 +33,6 @@ int cmd_main(int argc, const char **argv)
 
 	setup_git_directory();
 	gitmodules_config();
-	git_config(git_test_config, NULL);
 
 	while (*arg) {
 		struct object_id commit_oid;
diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
index 7d6b25ba2..46c09c776 100755
--- a/t/t7411-submodule-config.sh
+++ b/t/t7411-submodule-config.sh
@@ -122,78 +122,6 @@ test_expect_success 'using different treeishs works' '
 	)
 '
 
-cat >super/expect_url <<EOF
-Submodule url: 'git@somewhere.else.net:a.git' for path 'b'
-Submodule url: 'git@somewhere.else.net:submodule.git' for path 'submodule'
-EOF
-
-cat >super/expect_local_path <<EOF
-Submodule name: 'a' for path 'c'
-Submodule name: 'submodule' for path 'submodule'
-EOF
-
-test_expect_success 'reading of local configuration' '
-	(cd super &&
-		old_a=$(git config submodule.a.url) &&
-		old_submodule=$(git config submodule.submodule.url) &&
-		git config submodule.a.url git@somewhere.else.net:a.git &&
-		git config submodule.submodule.url git@somewhere.else.net:submodule.git &&
-		test-submodule-config --url \
-			"" b \
-			"" submodule \
-				>actual &&
-		test_cmp expect_url actual &&
-		git config submodule.a.path c &&
-		test-submodule-config \
-			"" c \
-			"" submodule \
-				>actual &&
-		test_cmp expect_local_path actual &&
-		git config submodule.a.url "$old_a" &&
-		git config submodule.submodule.url "$old_submodule" &&
-		git config --unset submodule.a.path c
-	)
-'
-
-cat >super/expect_url <<EOF
-Submodule url: '../submodule' for path 'b'
-Submodule url: 'git@somewhere.else.net:submodule.git' for path 'submodule'
-EOF
-
-test_expect_success 'reading of local configuration for uninitialized submodules' '
-	(
-		cd super &&
-		git submodule deinit -f b &&
-		old_submodule=$(git config submodule.submodule.url) &&
-		git config submodule.submodule.url git@somewhere.else.net:submodule.git &&
-		test-submodule-config --url \
-			"" b \
-			"" submodule \
-				>actual &&
-		test_cmp expect_url actual &&
-		git config submodule.submodule.url "$old_submodule" &&
-		git submodule init b
-	)
-'
-
-cat >super/expect_fetchrecurse_die.err <<EOF
-fatal: bad submodule.submodule.fetchrecursesubmodules argument: blabla
-EOF
-
-test_expect_success 'local error in fetchrecursesubmodule dies early' '
-	(cd super &&
-		git config submodule.submodule.fetchrecursesubmodules blabla &&
-		test_must_fail test-submodule-config \
-			"" b \
-			"" submodule \
-				>actual.out 2>actual.err &&
-		touch expect_fetchrecurse_die.out &&
-		test_cmp expect_fetchrecurse_die.out actual.out  &&
-		test_cmp expect_fetchrecurse_die.err actual.err  &&
-		git config --unset submodule.submodule.fetchrecursesubmodules
-	)
-'
-
 test_expect_success 'error in history in fetchrecursesubmodule lets continue' '
 	(cd super &&
 		git config -f .gitmodules \
-- 
2.14.0.rc1.383.gd1ce394fe2-goog

[PATCH v2 12/15] submodule-config: move submodule-config functions to submodule-config.c

[Brandon Williams]

Migrate the functions used to initialize the submodule-config to
submodule-config.c so that the callback routine used in the
initialization process can be static and prevent it from being used
outside of initializing the submodule-config through the main API.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 builtin/ls-files.c |  1 +
 submodule-config.c | 38 +++++++++++++++++++++++++++++++-------
 submodule-config.h |  7 ++-----
 submodule.c        | 35 -----------------------------------
 submodule.h        |  2 --
 5 files changed, 34 insertions(+), 49 deletions(-)

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index b8514a002..d14612057 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -19,6 +19,7 @@
 #include "pathspec.h"
 #include "run-command.h"
 #include "submodule.h"
+#include "submodule-config.h"
 
 static int abbrev;
 static int show_deleted;
diff --git a/submodule-config.c b/submodule-config.c
index 0b429e942..86636654b 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -449,9 +449,9 @@ static int parse_config(const char *var, const char *value, void *data)
 	return ret;
 }
 
-int gitmodule_oid_from_commit(const struct object_id *treeish_name,
-				      struct object_id *gitmodules_oid,
-				      struct strbuf *rev)
+static int gitmodule_oid_from_commit(const struct object_id *treeish_name,
+				     struct object_id *gitmodules_oid,
+				     struct strbuf *rev)
 {
 	int ret = 0;
 
@@ -552,9 +552,9 @@ static void submodule_cache_check_init(struct repository *repo)
 	submodule_cache_init(repo->submodule_cache);
 }
 
-int submodule_config_option(struct repository *repo,
-			    const char *var, const char *value)
+static int gitmodules_cb(const char *var, const char *value, void *data)
 {
+	struct repository *repo = data;
 	struct parse_config_parameter parameter;
 
 	submodule_cache_check_init(repo);
@@ -567,9 +567,33 @@ int submodule_config_option(struct repository *repo,
 	return parse_config(var, value, &parameter);
 }
 
-int parse_submodule_config_option(const char *var, const char *value)
+void repo_read_gitmodules(struct repository *repo)
 {
-	return submodule_config_option(the_repository, var, value);
+	if (repo->worktree) {
+		char *gitmodules;
+
+		if (repo_read_index(repo) < 0)
+			return;
+
+		gitmodules = repo_worktree_path(repo, GITMODULES_FILE);
+
+		if (!is_gitmodules_unmerged(repo->index))
+			git_config_from_file(gitmodules_cb, gitmodules, repo);
+
+		free(gitmodules);
+	}
+}
+
+void gitmodules_config_oid(const struct object_id *commit_oid)
+{
+	struct strbuf rev = STRBUF_INIT;
+	struct object_id oid;
+
+	if (gitmodule_oid_from_commit(commit_oid, &oid, &rev)) {
+		git_config_from_blob_oid(gitmodules_cb, rev.buf,
+					 &oid, the_repository);
+	}
+	strbuf_release(&rev);
 }
 
 const struct submodule *submodule_from_name(const struct object_id *treeish_name,
diff --git a/submodule-config.h b/submodule-config.h
index 84c2cf515..e3845831f 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -34,8 +34,8 @@ extern int option_fetch_parse_recurse_submodules(const struct option *opt,
 						 const char *arg, int unset);
 extern int parse_update_recurse_submodules_arg(const char *opt, const char *arg);
 extern int parse_push_recurse_submodules_arg(const char *opt, const char *arg);
-extern int submodule_config_option(struct repository *repo,
-				   const char *var, const char *value);
+extern void repo_read_gitmodules(struct repository *repo);
+extern void gitmodules_config_oid(const struct object_id *commit_oid);
 extern const struct submodule *submodule_from_name(
 		const struct object_id *commit_or_tree, const char *name);
 extern const struct submodule *submodule_from_path(
@@ -43,9 +43,6 @@ extern const struct submodule *submodule_from_path(
 extern const struct submodule *submodule_from_cache(struct repository *repo,
 						    const struct object_id *treeish_name,
 						    const char *key);
-extern int gitmodule_oid_from_commit(const struct object_id *commit_oid,
-				     struct object_id *gitmodules_oid,
-				     struct strbuf *rev);
 extern void submodule_free(void);
 
 #endif /* SUBMODULE_CONFIG_H */
diff --git a/submodule.c b/submodule.c
index 3b383d8c4..c1cef1c37 100644
--- a/submodule.c
+++ b/submodule.c
@@ -216,46 +216,11 @@ void load_submodule_cache(void)
 	gitmodules_config();
 }
 
-static int gitmodules_cb(const char *var, const char *value, void *data)
-{
-	struct repository *repo = data;
-	return submodule_config_option(repo, var, value);
-}
-
-void repo_read_gitmodules(struct repository *repo)
-{
-	if (repo->worktree) {
-		char *gitmodules;
-
-		if (repo_read_index(repo) < 0)
-			return;
-
-		gitmodules = repo_worktree_path(repo, GITMODULES_FILE);
-
-		if (!is_gitmodules_unmerged(repo->index))
-			git_config_from_file(gitmodules_cb, gitmodules, repo);
-
-		free(gitmodules);
-	}
-}
-
 void gitmodules_config(void)
 {
 	repo_read_gitmodules(the_repository);
 }
 
-void gitmodules_config_oid(const struct object_id *commit_oid)
-{
-	struct strbuf rev = STRBUF_INIT;
-	struct object_id oid;
-
-	if (gitmodule_oid_from_commit(commit_oid, &oid, &rev)) {
-		git_config_from_blob_oid(gitmodules_cb, rev.buf,
-					 &oid, the_repository);
-	}
-	strbuf_release(&rev);
-}
-
 /*
  * Determine if a submodule has been initialized at a given 'path'
  */
diff --git a/submodule.h b/submodule.h
index 4f70c4944..02195c24f 100644
--- a/submodule.h
+++ b/submodule.h
@@ -47,8 +47,6 @@ int option_parse_recurse_submodules_worktree_updater(const struct option *opt,
 						     const char *arg, int unset);
 void load_submodule_cache(void);
 extern void gitmodules_config(void);
-extern void repo_read_gitmodules(struct repository *repo);
-extern void gitmodules_config_oid(const struct object_id *commit_oid);
 extern int is_submodule_active(struct repository *repo, const char *path);
 /*
  * Determine if a submodule has been populated at a given 'path' by checking if
-- 
2.14.0.rc1.383.gd1ce394fe2-goog

[PATCH v2 13/15] submodule-config: lazy-load a repository's .gitmodules file

[Brandon Williams]

In order to use the submodule-config subsystem, callers first need to
initialize it by calling 'repo_read_gitmodules()' or
'gitmodules_config()' (which just redirects to
'repo_read_gitmodules()').  There are a couple of callers who need to
load an explicit revision of the repository's .gitmodules file (grep) or
need to modify the .gitmodules file so they would need to load it before
modify the file (checkout), but the majority of callers are simply
reading the .gitmodules file present in the working tree.  For the
common case it would be nice to avoid the boilerplate of initializing
the submodule-config system before using it, so instead let's perform
lazy-loading of the submodule-config system.

Remove the calls to reading the gitmodules file from ls-files to show
that lazy-loading the .gitmodules file works.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 builtin/ls-files.c |  5 -----
 submodule-config.c | 27 ++++++++++++++++++++++-----
 2 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index d14612057..bd74ee07d 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -211,8 +211,6 @@ static void show_submodule(struct repository *superproject,
 	if (repo_read_index(&submodule) < 0)
 		die("index file corrupt");
 
-	repo_read_gitmodules(&submodule);
-
 	show_files(&submodule, dir);
 
 	repo_clear(&submodule);
@@ -611,9 +609,6 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 	if (require_work_tree && !is_inside_work_tree())
 		setup_work_tree();
 
-	if (recurse_submodules)
-		repo_read_gitmodules(the_repository);
-
 	if (recurse_submodules &&
 	    (show_stage || show_deleted || show_others || show_unmerged ||
 	     show_killed || show_modified || show_resolve_undo || with_tree))
diff --git a/submodule-config.c b/submodule-config.c
index 86636654b..56d9d76d4 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -18,6 +18,7 @@ struct submodule_cache {
 	struct hashmap for_path;
 	struct hashmap for_name;
 	unsigned initialized:1;
+	unsigned gitmodules_read:1;
 };
 
 /*
@@ -93,6 +94,7 @@ static void submodule_cache_clear(struct submodule_cache *cache)
 	hashmap_free(&cache->for_path, 1);
 	hashmap_free(&cache->for_name, 1);
 	cache->initialized = 0;
+	cache->gitmodules_read = 0;
 }
 
 void submodule_cache_free(struct submodule_cache *cache)
@@ -557,8 +559,6 @@ static int gitmodules_cb(const char *var, const char *value, void *data)
 	struct repository *repo = data;
 	struct parse_config_parameter parameter;
 
-	submodule_cache_check_init(repo);
-
 	parameter.cache = repo->submodule_cache;
 	parameter.treeish_name = NULL;
 	parameter.gitmodules_sha1 = null_sha1;
@@ -569,6 +569,8 @@ static int gitmodules_cb(const char *var, const char *value, void *data)
 
 void repo_read_gitmodules(struct repository *repo)
 {
+	submodule_cache_check_init(repo);
+
 	if (repo->worktree) {
 		char *gitmodules;
 
@@ -582,6 +584,8 @@ void repo_read_gitmodules(struct repository *repo)
 
 		free(gitmodules);
 	}
+
+	repo->submodule_cache->gitmodules_read = 1;
 }
 
 void gitmodules_config_oid(const struct object_id *commit_oid)
@@ -589,24 +593,37 @@ void gitmodules_config_oid(const struct object_id *commit_oid)
 	struct strbuf rev = STRBUF_INIT;
 	struct object_id oid;
 
+	submodule_cache_check_init(the_repository);
+
 	if (gitmodule_oid_from_commit(commit_oid, &oid, &rev)) {
 		git_config_from_blob_oid(gitmodules_cb, rev.buf,
 					 &oid, the_repository);
 	}
 	strbuf_release(&rev);
+
+	the_repository->submodule_cache->gitmodules_read = 1;
+}
+
+static void gitmodules_read_check(struct repository *repo)
+{
+	submodule_cache_check_init(repo);
+
+	/* read the repo's .gitmodules file if it hasn't been already */
+	if (!repo->submodule_cache->gitmodules_read)
+		repo_read_gitmodules(repo);
 }
 
 const struct submodule *submodule_from_name(const struct object_id *treeish_name,
 		const char *name)
 {
-	submodule_cache_check_init(the_repository);
+	gitmodules_read_check(the_repository);
 	return config_from(the_repository->submodule_cache, treeish_name, name, lookup_name);
 }
 
 const struct submodule *submodule_from_path(const struct object_id *treeish_name,
 		const char *path)
 {
-	submodule_cache_check_init(the_repository);
+	gitmodules_read_check(the_repository);
 	return config_from(the_repository->submodule_cache, treeish_name, path, lookup_path);
 }
 
@@ -614,7 +631,7 @@ const struct submodule *submodule_from_cache(struct repository *repo,
 					     const struct object_id *treeish_name,
 					     const char *key)
 {
-	submodule_cache_check_init(repo);
+	gitmodules_read_check(repo);
 	return config_from(repo->submodule_cache, treeish_name,
 			   key, lookup_path);
 }
-- 
2.14.0.rc1.383.gd1ce394fe2-goog

[PATCH v2 14/15] unpack-trees: improve loading of .gitmodules

[Brandon Williams]

When recursing submodules 'check_updates()' needs to have strict control
over the submodule-config subsystem to ensure that the gitmodules file
has been read before checking cache entries which are marked for
removal as well ensuring the proper gitmodules file is read before
updating cache entries.

Because of this let's not rely on callers of 'check_updates()' to read
the gitmodules file before calling 'check_updates()' and handle the
reading explicitly.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 unpack-trees.c | 43 +++++++++++++++++++++++++++----------------
 1 file changed, 27 insertions(+), 16 deletions(-)

diff --git a/unpack-trees.c b/unpack-trees.c
index 5dce7ff7d..3c7f464fa 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1,5 +1,6 @@
 #define NO_THE_INDEX_COMPATIBILITY_MACROS
 #include "cache.h"
+#include "repository.h"
 #include "config.h"
 #include "dir.h"
 #include "tree.h"
@@ -268,22 +269,28 @@ static int check_submodule_move_head(const struct cache_entry *ce,
 	return 0;
 }
 
-static void reload_gitmodules_file(struct index_state *index,
-				   struct checkout *state)
+/*
+ * Preform the loading of the repository's gitmodules file.  This function is
+ * used by 'check_update()' to perform loading of the gitmodules file in two
+ * differnt situations:
+ * (1) before removing entries from the working tree if the gitmodules file has
+ *     been marked for removal.  This situation is specified by 'state' == NULL.
+ * (2) before checking out entries to the working tree if the gitmodules file
+ *     has been marked for update.  This situation is specified by 'state' != NULL.
+ */
+static void load_gitmodules_file(struct index_state *index,
+				 struct checkout *state)
 {
-	int i;
-	for (i = 0; i < index->cache_nr; i++) {
-		struct cache_entry *ce = index->cache[i];
-		if (ce->ce_flags & CE_UPDATE) {
-			int r = strcmp(ce->name, GITMODULES_FILE);
-			if (r < 0)
-				continue;
-			else if (r == 0) {
-				submodule_free();
-				checkout_entry(ce, state, NULL);
-				gitmodules_config();
-			} else
-				break;
+	int pos = index_name_pos(index, GITMODULES_FILE, strlen(GITMODULES_FILE));
+
+	if (pos >= 0) {
+		struct cache_entry *ce = index->cache[pos];
+		if (!state && ce->ce_flags & CE_WT_REMOVE) {
+			repo_read_gitmodules(the_repository);
+		} else if (state && (ce->ce_flags & CE_UPDATE)) {
+			submodule_free();
+			checkout_entry(ce, state, NULL);
+			repo_read_gitmodules(the_repository);
 		}
 	}
 }
@@ -343,6 +350,10 @@ static int check_updates(struct unpack_trees_options *o)
 
 	if (o->update)
 		git_attr_set_direction(GIT_ATTR_CHECKOUT, index);
+
+	if (should_update_submodules() && o->update && !o->dry_run)
+		load_gitmodules_file(index, NULL);
+
 	for (i = 0; i < index->cache_nr; i++) {
 		const struct cache_entry *ce = index->cache[i];
 
@@ -356,7 +367,7 @@ static int check_updates(struct unpack_trees_options *o)
 	remove_scheduled_dirs();
 
 	if (should_update_submodules() && o->update && !o->dry_run)
-		reload_gitmodules_file(index, &state);
+		load_gitmodules_file(index, &state);
 
 	for (i = 0; i < index->cache_nr; i++) {
 		struct cache_entry *ce = index->cache[i];
-- 
2.14.0.rc1.383.gd1ce394fe2-goog

[PATCH v2 15/15] submodule: remove gitmodules_config

[Brandon Williams]

Now that the submodule-config subsystem can lazily read the gitmodules
file we no longer need to explicitly pre-read the gitmodules by calling
'gitmodules_config()' so let's remove it.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 builtin/checkout.c               |  1 -
 builtin/commit.c                 |  1 -
 builtin/diff-files.c             |  1 -
 builtin/diff-index.c             |  1 -
 builtin/diff-tree.c              |  1 -
 builtin/diff.c                   |  2 --
 builtin/fetch.c                  |  4 ----
 builtin/grep.c                   |  4 ----
 builtin/mv.c                     |  1 -
 builtin/read-tree.c              |  2 --
 builtin/reset.c                  |  2 --
 builtin/rm.c                     |  1 -
 builtin/submodule--helper.c      | 14 --------------
 submodule.c                      | 15 ---------------
 submodule.h                      |  2 --
 t/helper/test-submodule-config.c |  1 -
 16 files changed, 53 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 246e0cd16..63ae16afc 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1179,7 +1179,6 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 	opts.prefix = prefix;
 	opts.show_progress = -1;
 
-	gitmodules_config();
 	git_config(git_checkout_config, &opts);
 
 	opts.track = BRANCH_TRACK_UNSPECIFIED;
diff --git a/builtin/commit.c b/builtin/commit.c
index 4bbac014a..18ad714d9 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -195,7 +195,6 @@ static void determine_whence(struct wt_status *s)
 static void status_init_config(struct wt_status *s, config_fn_t fn)
 {
 	wt_status_prepare(s);
-	gitmodules_config();
 	git_config(fn, s);
 	determine_whence(s);
 	init_diff_ui_defaults();
diff --git a/builtin/diff-files.c b/builtin/diff-files.c
index 17bf84d18..e88493ffe 100644
--- a/builtin/diff-files.c
+++ b/builtin/diff-files.c
@@ -26,7 +26,6 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)
 
 	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
 	init_revisions(&rev, prefix);
-	gitmodules_config();
 	rev.abbrev = 0;
 	precompose_argv(argc, argv);
 
diff --git a/builtin/diff-index.c b/builtin/diff-index.c
index 185e6f9b5..9d772f8f2 100644
--- a/builtin/diff-index.c
+++ b/builtin/diff-index.c
@@ -23,7 +23,6 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)
 
 	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
 	init_revisions(&rev, prefix);
-	gitmodules_config();
 	rev.abbrev = 0;
 	precompose_argv(argc, argv);
 
diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
index 31d2cb410..d66499909 100644
--- a/builtin/diff-tree.c
+++ b/builtin/diff-tree.c
@@ -110,7 +110,6 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
 
 	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
 	init_revisions(opt, prefix);
-	gitmodules_config();
 	opt->abbrev = 0;
 	opt->diff = 1;
 	opt->disable_stdin = 1;
diff --git a/builtin/diff.c b/builtin/diff.c
index 7cde6abbc..7e3ebcea3 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -315,8 +315,6 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 			no_index = DIFF_NO_INDEX_IMPLICIT;
 	}
 
-	if (!no_index)
-		gitmodules_config();
 	init_diff_ui_defaults();
 	git_config(git_diff_ui_config, NULL);
 	precompose_argv(argc, argv);
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 3fe99073d..132e3224e 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1360,10 +1360,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 	if (depth || deepen_since || deepen_not.nr)
 		deepen = 1;
 
-	if (recurse_submodules != RECURSE_SUBMODULES_OFF) {
-		gitmodules_config();
-	}
-
 	if (all) {
 		if (argc == 1)
 			die(_("fetch --all does not take a repository argument"));
diff --git a/builtin/grep.c b/builtin/grep.c
index ac06d2d33..2d65f27d0 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1048,10 +1048,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	}
 #endif
 
-	if (recurse_submodules) {
-		gitmodules_config();
-	}
-
 	if (show_in_pager && (cached || list.nr))
 		die(_("--open-files-in-pager only works on the worktree"));
 
diff --git a/builtin/mv.c b/builtin/mv.c
index 94fbaaa5d..ffdd5f01a 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -131,7 +131,6 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 	struct stat st;
 	struct string_list src_for_dst = STRING_LIST_INIT_NODUP;
 
-	gitmodules_config();
 	git_config(git_default_config, NULL);
 
 	argc = parse_options(argc, argv, prefix, builtin_mv_options,
diff --git a/builtin/read-tree.c b/builtin/read-tree.c
index d5f618d08..bf87a2710 100644
--- a/builtin/read-tree.c
+++ b/builtin/read-tree.c
@@ -164,8 +164,6 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
 	argc = parse_options(argc, argv, unused_prefix, read_tree_options,
 			     read_tree_usage, 0);
 
-	load_submodule_cache();
-
 	hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);
 
 	prefix_set = opts.prefix ? 1 : 0;
diff --git a/builtin/reset.c b/builtin/reset.c
index 772d078b8..50488d273 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -309,8 +309,6 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 						PARSE_OPT_KEEP_DASHDASH);
 	parse_args(&pathspec, argv, prefix, patch_mode, &rev);
 
-	load_submodule_cache();
-
 	unborn = !strcmp(rev, "HEAD") && get_oid("HEAD", &oid);
 	if (unborn) {
 		/* reset on unborn branch: treat as reset to empty tree */
diff --git a/builtin/rm.c b/builtin/rm.c
index 4057e73fa..d91451fea 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -255,7 +255,6 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 	struct pathspec pathspec;
 	char *seen;
 
-	gitmodules_config();
 	git_config(git_default_config, NULL);
 
 	argc = parse_options(argc, argv, prefix, builtin_rm_options,
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index ba767c704..c97fde439 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -275,8 +275,6 @@ static void module_list_active(struct module_list *list)
 	int i;
 	struct module_list active_modules = MODULE_LIST_INIT;
 
-	gitmodules_config();
-
 	for (i = 0; i < list->nr; i++) {
 		const struct cache_entry *ce = list->entries[i];
 
@@ -337,9 +335,6 @@ static void init_submodule(const char *path, const char *prefix, int quiet)
 	struct strbuf sb = STRBUF_INIT;
 	char *upd = NULL, *url = NULL, *displaypath;
 
-	/* Only loads from .gitmodules, no overlay with .git/config */
-	gitmodules_config();
-
 	if (prefix && get_super_prefix())
 		die("BUG: cannot have prefix and superprefix");
 	else if (prefix)
@@ -475,7 +470,6 @@ static int module_name(int argc, const char **argv, const char *prefix)
 	if (argc != 2)
 		usage(_("git submodule--helper name <path>"));
 
-	gitmodules_config();
 	sub = submodule_from_path(&null_oid, argv[1]);
 
 	if (!sub)
@@ -1042,8 +1036,6 @@ static int update_clone(int argc, const char **argv, const char *prefix)
 	if (pathspec.nr)
 		suc.warn_if_uninitialized = 1;
 
-	gitmodules_config();
-
 	run_processes_parallel(max_jobs,
 			       update_clone_get_next_task,
 			       update_clone_start_failure,
@@ -1084,8 +1076,6 @@ static const char *remote_submodule_branch(const char *path)
 	const char *branch = NULL;
 	char *key;
 
-	gitmodules_config();
-
 	sub = submodule_from_path(&null_oid, path);
 	if (!sub)
 		return NULL;
@@ -1204,8 +1194,6 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, embed_gitdir_options,
 			     git_submodule_helper_usage, 0);
 
-	gitmodules_config();
-
 	if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)
 		return 1;
 
@@ -1221,8 +1209,6 @@ static int is_active(int argc, const char **argv, const char *prefix)
 	if (argc != 2)
 		die("submodule--helper is-active takes exactly 1 argument");
 
-	gitmodules_config();
-
 	return !is_submodule_active(the_repository, argv[1]);
 }
 
diff --git a/submodule.c b/submodule.c
index c1cef1c37..77346da88 100644
--- a/submodule.c
+++ b/submodule.c
@@ -208,19 +208,6 @@ int option_parse_recurse_submodules_worktree_updater(const struct option *opt,
 	return 0;
 }
 
-void load_submodule_cache(void)
-{
-	if (config_update_recurse_submodules == RECURSE_SUBMODULES_OFF)
-		return;
-
-	gitmodules_config();
-}
-
-void gitmodules_config(void)
-{
-	repo_read_gitmodules(the_repository);
-}
-
 /*
  * Determine if a submodule has been initialized at a given 'path'
  */
@@ -1093,7 +1080,6 @@ int submodule_touches_in_range(struct object_id *excl_oid,
 	struct argv_array args = ARGV_ARRAY_INIT;
 	int ret;
 
-	gitmodules_config();
 	/* No need to check if there are no submodules configured */
 	if (!submodule_from_path(NULL, NULL))
 		return 0;
@@ -2000,7 +1986,6 @@ int submodule_to_gitdir(struct strbuf *buf, const char *submodule)
 		strbuf_addstr(buf, git_dir);
 	}
 	if (!is_git_directory(buf->buf)) {
-		gitmodules_config();
 		sub = submodule_from_path(&null_oid, submodule);
 		if (!sub) {
 			ret = -1;
diff --git a/submodule.h b/submodule.h
index 02195c24f..be103ad9d 100644
--- a/submodule.h
+++ b/submodule.h
@@ -45,8 +45,6 @@ extern int git_default_submodule_config(const char *var, const char *value, void
 struct option;
 int option_parse_recurse_submodules_worktree_updater(const struct option *opt,
 						     const char *arg, int unset);
-void load_submodule_cache(void);
-extern void gitmodules_config(void);
 extern int is_submodule_active(struct repository *repo, const char *path);
 /*
  * Determine if a submodule has been populated at a given 'path' by checking if
diff --git a/t/helper/test-submodule-config.c b/t/helper/test-submodule-config.c
index f4a7c431c..f23db3b19 100644
--- a/t/helper/test-submodule-config.c
+++ b/t/helper/test-submodule-config.c
@@ -32,7 +32,6 @@ int cmd_main(int argc, const char **argv)
 		die_usage(argc, argv, "Wrong number of arguments.");
 
 	setup_git_directory();
-	gitmodules_config();
 
 	while (*arg) {
 		struct object_id commit_oid;
-- 
2.14.0.rc1.383.gd1ce394fe2-goog

Re: [PATCH v2 02/15] submodule: don't use submodule_from_name

[Stefan Beller]

On Thu, Aug 3, 2017 at 11:19 AM, Brandon Williams <bmwill@google.com> wrote:
> The function 'submodule_from_name()' is being used incorrectly here as a
> submodule path is being used instead of a submodule name.  Since the
> correct function to use with a path to a submodule is already being used
> ('submodule_from_path()') let's remove the call to
> 'submodule_from_name()'.
>
> Signed-off-by: Brandon Williams <bmwill@google.com>

In case a reroll is needed, you could incorperate Jens feedback
stating that 851e18c385 should have done it.

> ---
>  submodule.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/submodule.c b/submodule.c
> index 5139b9256..19bd13bb2 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1177,8 +1177,6 @@ static int get_next_submodule(struct child_process *cp,
>                         continue;
>
>                 submodule = submodule_from_path(&null_oid, ce->name);
> -               if (!submodule)
> -                       submodule = submodule_from_name(&null_oid, ce->name);
>
>                 default_argv = "yes";
>                 if (spf->command_line_option == RECURSE_SUBMODULES_DEFAULT) {
> --
> 2.14.0.rc1.383.gd1ce394fe2-goog
>

Re: [PATCH v2 00/15] submodule-config cleanup

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

> Changes in v2:
>  * Rebased on latest 'bw/grep-recurse-submodules' branch (Still also requires
>    the 'bc/object-id' series).
>  * Changed unpack-trees.c (checkout command) so that it no longer respects the
>    'submodule.<name>.update' config since it really didn't make much sense for
>    it to respect it.
>  * The above point also enabled me to fix some issues that coverity found with
>    how I was overlaying the repo config with the submodule update strategy.
>    Instead the update strategy parsing logic is separated into two functions so
>    that just the enum can be determined from a string (which is all
>    update-clone needed).

Thanks.  I was wondering what the status of this series was when I
accepted the updated "grep --recurse-submodules" the other day.

Re: [PATCH v2 02/15] submodule: don't use submodule_from_name

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

> The function 'submodule_from_name()' is being used incorrectly here as a
> submodule path is being used instead of a submodule name.  Since the
> correct function to use with a path to a submodule is already being used
> ('submodule_from_path()') let's remove the call to
> 'submodule_from_name()'.
>
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
>  submodule.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/submodule.c b/submodule.c
> index 5139b9256..19bd13bb2 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1177,8 +1177,6 @@ static int get_next_submodule(struct child_process *cp,
>  			continue;
>  
>  		submodule = submodule_from_path(&null_oid, ce->name);
> -		if (!submodule)
> -			submodule = submodule_from_name(&null_oid, ce->name);
>  
>  		default_argv = "yes";
>  		if (spf->command_line_option == RECURSE_SUBMODULES_DEFAULT) {

It appears to me that the scope of the variable "submodule" in this
function can be narrowed to be limited to the block inside this "if"
statement we see in the post-context of this hunk.  That would make
it even easier to see why leaving submodule to NULL is a safe thing
to do.

This comment applies to the state of this function before or after
this patch.  It can be left outside the scope of this immediate
series, and instead be done as a follow-up (or preparatory) cleanup.

Thanks.

Re: [PATCH v2 08/15] unpack-trees: don't respect submodule.update

[Stefan Beller]

On Thu, Aug 3, 2017 at 11:19 AM, Brandon Williams <bmwill@google.com> wrote:
> The 'submodule.update' config was historically used and respected by the
> 'submodule update' command because update handled a variety of different
> ways it updated a submodule.  As we begin teaching other commands about
> submodules it makes more sense for the different settings of
> 'submodule.update' to be handled by the individual commands themselves
> (checkout, rebase, merge, etc) so it shouldn't be respected by the
> native checkout command.
>
> Also remove the overlaying of the repository's config (via using
> 'submodule_config()') from the commands which use the unpack-trees
> logic (checkout, read-tree, reset).

That was a mistake that I introduced with the checkout series.

Thanks for fixing it.

Re: [PATCH v2 08/15] unpack-trees: don't respect submodule.update

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

> The 'submodule.update' config was historically used and respected by the
> 'submodule update' command because update handled a variety of different
> ways it updated a submodule.  As we begin teaching other commands about
> submodules it makes more sense for the different settings of
> 'submodule.update' to be handled by the individual commands themselves
> (checkout, rebase, merge, etc) so it shouldn't be respected by the
> native checkout command.

Soooo... what's the externally observable effect of this change?  Is
it something that can be illustrated in a set of new tests?

IOW does this commit by itself want to change the behaviour of
"submodule update" and existing (indirect) users of unpack-trees?
Or does it want to keep the documented behaviour of "submodule
update" while correcting unintended triggering in other (indirect)
users of unpack-trees of the same machinery that is being removed in
this patch?

> -	switch (sub->update_strategy.type) {
> -	case SM_UPDATE_UNSPECIFIED:
> -	case SM_UPDATE_CHECKOUT:
> -		if (submodule_move_head(ce->name, old_id, new_id, flags))
> -			return o->gently ? -1 :
> -				add_rejected_path(o, ERROR_WOULD_LOSE_SUBMODULE, ce->name);
> -		return 0;
> -	case SM_UPDATE_NONE:
> -		return 0;
> -	case SM_UPDATE_REBASE:
> -	case SM_UPDATE_MERGE:
> -	case SM_UPDATE_COMMAND:
> -	default:
> -		warning(_("submodule update strategy not supported for submodule '%s'"), ce->name);
> -		return -1;
> -	}
> +	if (submodule_move_head(ce->name, old_id, new_id, flags))
> +		return o->gently ? -1 :
> +				   add_rejected_path(o, ERROR_WOULD_LOSE_SUBMODULE, ce->name);
> +	return 0;

With this update, we always behave as if update_strategy.type were
either left unspecified or explicitly set to checkout.  Other arms
in this switch (and the other switch too), especially "none", were
not expecting a call to submodule_move_head() to be made, but now
the call is unconditional.



>  }
>  
>  static void reload_gitmodules_file(struct index_state *index,
> @@ -293,7 +282,6 @@ static void reload_gitmodules_file(struct index_state *index,
>  				submodule_free();
>  				checkout_entry(ce, state, NULL);
>  				gitmodules_config();
> -				git_config(submodule_config, NULL);
>  			} else
>  				break;
>  		}
> @@ -308,19 +296,9 @@ static void unlink_entry(const struct cache_entry *ce)
>  {
>  	const struct submodule *sub = submodule_from_ce(ce);
>  	if (sub) {
> -		switch (sub->update_strategy.type) {
> -		case SM_UPDATE_UNSPECIFIED:
> -		case SM_UPDATE_CHECKOUT:
> -		case SM_UPDATE_REBASE:
> -		case SM_UPDATE_MERGE:
> -			/* state.force is set at the caller. */
> -			submodule_move_head(ce->name, "HEAD", NULL,
> -					    SUBMODULE_MOVE_HEAD_FORCE);
> -			break;
> -		case SM_UPDATE_NONE:
> -		case SM_UPDATE_COMMAND:
> -			return; /* Do not touch the submodule. */
> -		}
> +		/* state.force is set at the caller. */
> +		submodule_move_head(ce->name, "HEAD", NULL,
> +				    SUBMODULE_MOVE_HEAD_FORCE);
>  	}
>  	if (!check_leading_path(ce->name, ce_namelen(ce)))
>  		return;

Re: [PATCH v2 10/15] diff: stop allowing diff to have submodules configured in .git/config

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

> Traditionally a submodule is comprised of a gitlink as well as a
> corresponding entry in the .gitmodules file.  Diff doesn't follow this
> paradigm as its config callback routine falls back to populating the
> submodule-config if a config entry starts with 'submodule.'.
>
> Remove this behavior in order to be consistent with how the
> submodule-config is populated, via calling 'gitmodules_config()' or
> 'repo_read_gitmodules()'.

I am all for dropping special cases deep in the diff machinery, even
though there may be submodule users who care about submodule.*.ignore

Does this change mean we can eventually get rid of the ugly
DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG hack and also need for a patch
like 03/15?

>
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
>  diff.c                    |  3 ---
>  t/t4027-diff-submodule.sh | 67 -----------------------------------------------
>  2 files changed, 70 deletions(-)
>
> diff --git a/diff.c b/diff.c
> index 85e714f6c..e43519b88 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -346,9 +346,6 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)
>  		return 0;
>  	}
>  
> -	if (starts_with(var, "submodule."))
> -		return parse_submodule_config_option(var, value);
> -
>  	if (git_diff_heuristic_config(var, value, cb) < 0)
>  		return -1;
>  
> diff --git a/t/t4027-diff-submodule.sh b/t/t4027-diff-submodule.sh
> index 518bf9524..2ffd11a14 100755
> --- a/t/t4027-diff-submodule.sh
> +++ b/t/t4027-diff-submodule.sh
> @@ -113,35 +113,6 @@ test_expect_success 'git diff HEAD with dirty submodule (work tree, refs match)'
>  	! test -s actual4
>  '
>  
> -test_expect_success 'git diff HEAD with dirty submodule (work tree, refs match) [.git/config]' '
> -	git config diff.ignoreSubmodules all &&
> -	git diff HEAD >actual &&
> -	! test -s actual &&
> -	git config submodule.subname.ignore none &&
> -	git config submodule.subname.path sub &&
> -	git diff HEAD >actual &&
> -	sed -e "1,/^@@/d" actual >actual.body &&
> -	expect_from_to >expect.body $subprev $subprev-dirty &&
> -	test_cmp expect.body actual.body &&
> -	git config submodule.subname.ignore all &&
> -	git diff HEAD >actual2 &&
> -	! test -s actual2 &&
> -	git config submodule.subname.ignore untracked &&
> -	git diff HEAD >actual3 &&
> -	sed -e "1,/^@@/d" actual3 >actual3.body &&
> -	expect_from_to >expect.body $subprev $subprev-dirty &&
> -	test_cmp expect.body actual3.body &&
> -	git config submodule.subname.ignore dirty &&
> -	git diff HEAD >actual4 &&
> -	! test -s actual4 &&
> -	git diff HEAD --ignore-submodules=none >actual &&
> -	sed -e "1,/^@@/d" actual >actual.body &&
> -	expect_from_to >expect.body $subprev $subprev-dirty &&
> -	test_cmp expect.body actual.body &&
> -	git config --remove-section submodule.subname &&
> -	git config --unset diff.ignoreSubmodules
> -'
> -
>  test_expect_success 'git diff HEAD with dirty submodule (work tree, refs match) [.gitmodules]' '
>  	git config diff.ignoreSubmodules dirty &&
>  	git diff HEAD >actual &&
> @@ -208,24 +179,6 @@ test_expect_success 'git diff HEAD with dirty submodule (untracked, refs match)'
>  	! test -s actual4
>  '
>  
> -test_expect_success 'git diff HEAD with dirty submodule (untracked, refs match) [.git/config]' '
> -	git config submodule.subname.ignore all &&
> -	git config submodule.subname.path sub &&
> -	git diff HEAD >actual2 &&
> -	! test -s actual2 &&
> -	git config submodule.subname.ignore untracked &&
> -	git diff HEAD >actual3 &&
> -	! test -s actual3 &&
> -	git config submodule.subname.ignore dirty &&
> -	git diff HEAD >actual4 &&
> -	! test -s actual4 &&
> -	git diff --ignore-submodules=none HEAD >actual &&
> -	sed -e "1,/^@@/d" actual >actual.body &&
> -	expect_from_to >expect.body $subprev $subprev-dirty &&
> -	test_cmp expect.body actual.body &&
> -	git config --remove-section submodule.subname
> -'
> -
>  test_expect_success 'git diff HEAD with dirty submodule (untracked, refs match) [.gitmodules]' '
>  	git config --add -f .gitmodules submodule.subname.ignore all &&
>  	git config --add -f .gitmodules submodule.subname.path sub &&
> @@ -261,26 +214,6 @@ test_expect_success 'git diff between submodule commits' '
>  	! test -s actual
>  '
>  
> -test_expect_success 'git diff between submodule commits [.git/config]' '
> -	git diff HEAD^..HEAD >actual &&
> -	sed -e "1,/^@@/d" actual >actual.body &&
> -	expect_from_to >expect.body $subtip $subprev &&
> -	test_cmp expect.body actual.body &&
> -	git config submodule.subname.ignore dirty &&
> -	git config submodule.subname.path sub &&
> -	git diff HEAD^..HEAD >actual &&
> -	sed -e "1,/^@@/d" actual >actual.body &&
> -	expect_from_to >expect.body $subtip $subprev &&
> -	test_cmp expect.body actual.body &&
> -	git config submodule.subname.ignore all &&
> -	git diff HEAD^..HEAD >actual &&
> -	! test -s actual &&
> -	git diff --ignore-submodules=dirty HEAD^..HEAD >actual &&
> -	sed -e "1,/^@@/d" actual >actual.body &&
> -	expect_from_to >expect.body $subtip $subprev &&
> -	git config --remove-section submodule.subname
> -'
> -
>  test_expect_success 'git diff between submodule commits [.gitmodules]' '
>  	git diff HEAD^..HEAD >actual &&
>  	sed -e "1,/^@@/d" actual >actual.body &&

Re: [PATCH v2 08/15] unpack-trees: don't respect submodule.update

[Stefan Beller]

On Thu, Aug 3, 2017 at 1:37 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Brandon Williams <bmwill@google.com> writes:
>
>> The 'submodule.update' config was historically used and respected by the
>> 'submodule update' command because update handled a variety of different
>> ways it updated a submodule.  As we begin teaching other commands about
>> submodules it makes more sense for the different settings of
>> 'submodule.update' to be handled by the individual commands themselves
>> (checkout, rebase, merge, etc) so it shouldn't be respected by the
>> native checkout command.
>
> Soooo... what's the externally observable effect of this change?  Is
> it something that can be illustrated in a set of new tests?

The illustration can be as follows

    git config submodule.NAME.update none
    git checkout -f --recurse-submodules HEAD
    git status
    # observe dirty submodule, which is
    # not what checkout -f promises

> IOW does this commit by itself want to change the behaviour of
> "submodule update" and existing (indirect) users of unpack-trees?
> Or does it want to keep the documented behaviour of "submodule
> update" while correcting unintended triggering in other (indirect)
> users of unpack-trees of the same machinery that is being removed in
> this patch?

"submodule update" is unaffected, only the recently introduced submodule
awareness of checkout/reset/read-tree are changed.

This option is documented as
    submodule.<name>.update
    The default update procedure for a submodule. This variable is
    populated by git submodule init from the gitmodules(5) file. See
    description of update command in git-submodule(1).

which doesn't indicate that any other command apart from
"submodule update" should respect it.

>
>> -     switch (sub->update_strategy.type) {
>> -     case SM_UPDATE_UNSPECIFIED:
>> -     case SM_UPDATE_CHECKOUT:
>> -             if (submodule_move_head(ce->name, old_id, new_id, flags))
>> -                     return o->gently ? -1 :
>> -                             add_rejected_path(o, ERROR_WOULD_LOSE_SUBMODULE, ce->name);
>> -             return 0;
>> -     case SM_UPDATE_NONE:
>> -             return 0;
>> -     case SM_UPDATE_REBASE:
>> -     case SM_UPDATE_MERGE:
>> -     case SM_UPDATE_COMMAND:
>> -     default:
>> -             warning(_("submodule update strategy not supported for submodule '%s'"), ce->name);
>> -             return -1;
>> -     }
>> +     if (submodule_move_head(ce->name, old_id, new_id, flags))
>> +             return o->gently ? -1 :
>> +                                add_rejected_path(o, ERROR_WOULD_LOSE_SUBMODULE, ce->name);
>> +     return 0;
>
> With this update, we always behave as if update_strategy.type were
> either left unspecified or explicitly set to checkout.  Other arms
> in this switch (and the other switch too), especially "none", were
> not expecting a call to submodule_move_head() to be made, but now
> the call is unconditional.
>

Yes. This is because each command (reset/checkout) should provide
one expected behavior. It is not that we can configure reset to omit certain
(tracked) files from being reset?

Re: [PATCH v2 02/15] submodule: don't use submodule_from_name

[Brandon Williams]

On 08/03, Stefan Beller wrote:
> On Thu, Aug 3, 2017 at 11:19 AM, Brandon Williams <bmwill@google.com> wrote:
> > The function 'submodule_from_name()' is being used incorrectly here as a
> > submodule path is being used instead of a submodule name.  Since the
> > correct function to use with a path to a submodule is already being used
> > ('submodule_from_path()') let's remove the call to
> > 'submodule_from_name()'.
> >
> > Signed-off-by: Brandon Williams <bmwill@google.com>
> 
> In case a reroll is needed, you could incorperate Jens feedback
> stating that 851e18c385 should have done it.

K I'll add that into the commit message.

> 
> > ---
> >  submodule.c | 2 --
> >  1 file changed, 2 deletions(-)
> >
> > diff --git a/submodule.c b/submodule.c
> > index 5139b9256..19bd13bb2 100644
> > --- a/submodule.c
> > +++ b/submodule.c
> > @@ -1177,8 +1177,6 @@ static int get_next_submodule(struct child_process *cp,
> >                         continue;
> >
> >                 submodule = submodule_from_path(&null_oid, ce->name);
> > -               if (!submodule)
> > -                       submodule = submodule_from_name(&null_oid, ce->name);
> >
> >                 default_argv = "yes";
> >                 if (spf->command_line_option == RECURSE_SUBMODULES_DEFAULT) {
> > --
> > 2.14.0.rc1.383.gd1ce394fe2-goog
> >

-- 
Brandon Williams

Re: [PATCH v2 10/15] diff: stop allowing diff to have submodules configured in .git/config

[Brandon Williams]

On 08/03, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
> 
> > Traditionally a submodule is comprised of a gitlink as well as a
> > corresponding entry in the .gitmodules file.  Diff doesn't follow this
> > paradigm as its config callback routine falls back to populating the
> > submodule-config if a config entry starts with 'submodule.'.
> >
> > Remove this behavior in order to be consistent with how the
> > submodule-config is populated, via calling 'gitmodules_config()' or
> > 'repo_read_gitmodules()'.
> 
> I am all for dropping special cases deep in the diff machinery, even
> though there may be submodule users who care about submodule.*.ignore
> 
> Does this change mean we can eventually get rid of the ugly
> DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG hack and also need for a patch
> like 03/15?

I think that this is a step toward getting rid of that.  We can either
do two things: 1) deprecate submodule.*.ignore and don't respect it
anymore or 2) flip the polarity of that flag so that by default we
don't respect the submodule.*.ignore config and instead callers must opt
in instead of the current opt out behavior.

> 
> >
> > Signed-off-by: Brandon Williams <bmwill@google.com>
> > ---
> >  diff.c                    |  3 ---
> >  t/t4027-diff-submodule.sh | 67 -----------------------------------------------
> >  2 files changed, 70 deletions(-)
> >
> > diff --git a/diff.c b/diff.c
> > index 85e714f6c..e43519b88 100644
> > --- a/diff.c
> > +++ b/diff.c
> > @@ -346,9 +346,6 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)
> >  		return 0;
> >  	}
> >  
> > -	if (starts_with(var, "submodule."))
> > -		return parse_submodule_config_option(var, value);
> > -
> >  	if (git_diff_heuristic_config(var, value, cb) < 0)
> >  		return -1;
> >  
> > diff --git a/t/t4027-diff-submodule.sh b/t/t4027-diff-submodule.sh
> > index 518bf9524..2ffd11a14 100755
> > --- a/t/t4027-diff-submodule.sh
> > +++ b/t/t4027-diff-submodule.sh
> > @@ -113,35 +113,6 @@ test_expect_success 'git diff HEAD with dirty submodule (work tree, refs match)'
> >  	! test -s actual4
> >  '
> >  
> > -test_expect_success 'git diff HEAD with dirty submodule (work tree, refs match) [.git/config]' '
> > -	git config diff.ignoreSubmodules all &&
> > -	git diff HEAD >actual &&
> > -	! test -s actual &&
> > -	git config submodule.subname.ignore none &&
> > -	git config submodule.subname.path sub &&
> > -	git diff HEAD >actual &&
> > -	sed -e "1,/^@@/d" actual >actual.body &&
> > -	expect_from_to >expect.body $subprev $subprev-dirty &&
> > -	test_cmp expect.body actual.body &&
> > -	git config submodule.subname.ignore all &&
> > -	git diff HEAD >actual2 &&
> > -	! test -s actual2 &&
> > -	git config submodule.subname.ignore untracked &&
> > -	git diff HEAD >actual3 &&
> > -	sed -e "1,/^@@/d" actual3 >actual3.body &&
> > -	expect_from_to >expect.body $subprev $subprev-dirty &&
> > -	test_cmp expect.body actual3.body &&
> > -	git config submodule.subname.ignore dirty &&
> > -	git diff HEAD >actual4 &&
> > -	! test -s actual4 &&
> > -	git diff HEAD --ignore-submodules=none >actual &&
> > -	sed -e "1,/^@@/d" actual >actual.body &&
> > -	expect_from_to >expect.body $subprev $subprev-dirty &&
> > -	test_cmp expect.body actual.body &&
> > -	git config --remove-section submodule.subname &&
> > -	git config --unset diff.ignoreSubmodules
> > -'
> > -
> >  test_expect_success 'git diff HEAD with dirty submodule (work tree, refs match) [.gitmodules]' '
> >  	git config diff.ignoreSubmodules dirty &&
> >  	git diff HEAD >actual &&
> > @@ -208,24 +179,6 @@ test_expect_success 'git diff HEAD with dirty submodule (untracked, refs match)'
> >  	! test -s actual4
> >  '
> >  
> > -test_expect_success 'git diff HEAD with dirty submodule (untracked, refs match) [.git/config]' '
> > -	git config submodule.subname.ignore all &&
> > -	git config submodule.subname.path sub &&
> > -	git diff HEAD >actual2 &&
> > -	! test -s actual2 &&
> > -	git config submodule.subname.ignore untracked &&
> > -	git diff HEAD >actual3 &&
> > -	! test -s actual3 &&
> > -	git config submodule.subname.ignore dirty &&
> > -	git diff HEAD >actual4 &&
> > -	! test -s actual4 &&
> > -	git diff --ignore-submodules=none HEAD >actual &&
> > -	sed -e "1,/^@@/d" actual >actual.body &&
> > -	expect_from_to >expect.body $subprev $subprev-dirty &&
> > -	test_cmp expect.body actual.body &&
> > -	git config --remove-section submodule.subname
> > -'
> > -
> >  test_expect_success 'git diff HEAD with dirty submodule (untracked, refs match) [.gitmodules]' '
> >  	git config --add -f .gitmodules submodule.subname.ignore all &&
> >  	git config --add -f .gitmodules submodule.subname.path sub &&
> > @@ -261,26 +214,6 @@ test_expect_success 'git diff between submodule commits' '
> >  	! test -s actual
> >  '
> >  
> > -test_expect_success 'git diff between submodule commits [.git/config]' '
> > -	git diff HEAD^..HEAD >actual &&
> > -	sed -e "1,/^@@/d" actual >actual.body &&
> > -	expect_from_to >expect.body $subtip $subprev &&
> > -	test_cmp expect.body actual.body &&
> > -	git config submodule.subname.ignore dirty &&
> > -	git config submodule.subname.path sub &&
> > -	git diff HEAD^..HEAD >actual &&
> > -	sed -e "1,/^@@/d" actual >actual.body &&
> > -	expect_from_to >expect.body $subtip $subprev &&
> > -	test_cmp expect.body actual.body &&
> > -	git config submodule.subname.ignore all &&
> > -	git diff HEAD^..HEAD >actual &&
> > -	! test -s actual &&
> > -	git diff --ignore-submodules=dirty HEAD^..HEAD >actual &&
> > -	sed -e "1,/^@@/d" actual >actual.body &&
> > -	expect_from_to >expect.body $subtip $subprev &&
> > -	git config --remove-section submodule.subname
> > -'
> > -
> >  test_expect_success 'git diff between submodule commits [.gitmodules]' '
> >  	git diff HEAD^..HEAD >actual &&
> >  	sed -e "1,/^@@/d" actual >actual.body &&

-- 
Brandon Williams

Re: [PATCH 02/15] submodule: don't use submodule_from_name

[Heiko Voigt]

Hi,

sorry for the late reply, just stumpled upon this.

On Mon, Jul 31, 2017 at 01:43:04PM -0700, Stefan Beller wrote:
> On Sun, Jul 30, 2017 at 6:43 AM, Jens Lehmann <Jens.Lehmann@web.de> wrote:
> > Am 26.07.2017 um 23:06 schrieb Junio C Hamano:
> >>
> >> Stefan Beller <sbeller@google.com> writes:
> >>
> >>> Rereading the archives, there was quite some discussion on the design
> >>> of these patches, but these lines of code did not get any attention
> >>>
> >>>      https://public-inbox.org/git/4CDB3063.5010801@web.de/
> >>>
> >>> I cc'd Jens in the hope of him having a good memory why he
> >>> wrote the code that way. :)
> >>
> >>
> >> Thanks for digging.  I wouldn't be surprised if this were a fallback
> >> to help a broken entry in .gitmodules that lack .path variable, but
> >> we shouldn't be sweeping the problem under the rug like that.
> >
> >
> > Sorry to disappoint you ;-) I added this in 7dce19d374 because
> > submodule by path lookup back then only parsed the checked out
> > .gitmodules file.
> 
> This is still the case AFAICT, as we never ask for a specific .gitmodules
> file identified by sha1 of the commit.

This was actually part of my original approach[1] but it seems I never got
around to implement that last part for which I originally started the
submodule config API: Proper recursive fetch.

I still have a patch for moved submodules lying around which pass a commit id
for a gitmodules file. That particular patch is quite simple and finished but
I was planning to include that in the finished fetch series. So I can have a
look if I can quickly update that to the current state, so we can at least have
one proper user of the submodule config API.

> > So looking for it by name was a good guess to
> > fetch a new submodule that wasn't present in the current HEAD's
> > .gitmodules, as the path is used as the default name in "git
> > submodule add".

I will have a look whether we can easily replace this hack with the proper
lookup now. Lets see how many low hanging fruits we have lying around
for recursive fetch. The full blown implementation including cloning of
new submodules might still take some time...

Cheers Heiko

[1] https://public-inbox.org/git/f5baa2acc09531a16f4f693eebbe60706bb8ed1e.1361751905.git.hvoigt@hvoigt.net/

Re: [PATCH v2 02/15] submodule: don't use submodule_from_name

[Heiko Voigt]

On Fri, Aug 04, 2017 at 02:53:11PM -0700, Brandon Williams wrote:
> On 08/03, Stefan Beller wrote:
> > On Thu, Aug 3, 2017 at 11:19 AM, Brandon Williams <bmwill@google.com> wrote:
> > > The function 'submodule_from_name()' is being used incorrectly here as a
> > > submodule path is being used instead of a submodule name.  Since the
> > > correct function to use with a path to a submodule is already being used
> > > ('submodule_from_path()') let's remove the call to
> > > 'submodule_from_name()'.
> > >
> > > Signed-off-by: Brandon Williams <bmwill@google.com>
> > 
> > In case a reroll is needed, you could incorperate Jens feedback
> > stating that 851e18c385 should have done it.
> 
> K I'll add that into the commit message.

Well, thats not 100% correct... IMO, it should have been a follow up patch
which I never got to implement. See my other reply to the v1 of this
patch I just sent out.

As stated there I will have a look into where it makes sense to pass a
commit id and behave more correctly.

Cheers Heiko

Re: [PATCH v2 14/15] unpack-trees: improve loading of .gitmodules

[Heiko Voigt]

On Thu, Aug 03, 2017 at 11:19:59AM -0700, Brandon Williams wrote:
> diff --git a/unpack-trees.c b/unpack-trees.c
> index 5dce7ff7d..3c7f464fa 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -1,5 +1,6 @@
>  #define NO_THE_INDEX_COMPATIBILITY_MACROS
>  #include "cache.h"
> +#include "repository.h"
>  #include "config.h"
>  #include "dir.h"
>  #include "tree.h"
> @@ -268,22 +269,28 @@ static int check_submodule_move_head(const struct cache_entry *ce,
>  	return 0;
>  }
>  
> -static void reload_gitmodules_file(struct index_state *index,
> -				   struct checkout *state)
> +/*
> + * Preform the loading of the repository's gitmodules file.  This function is

s/Preform/Perform/

and a nit: There is some extra space after the end of this sentence.

Cheers Heiko