[RFC PATCH 00/10] Make submodules work if .gitmodules is not checked out

[RFC PATCH 00/10] Make submodules work if .gitmodules is not checked out

[Antonio Ospite]

Hi,

vcsh[1] uses bare git repositories and detached work-trees to manage
*distinct* sets of configuration files directly into $HOME.

In this setup multiple repositories share the same directory (namely
$HOME) as their work dir, so the sets of checked out files would also
need to be *disjoint* to avoid collisions (examples of collisions can be
README or LICENSE files, or even .gitignore and .gitattributes).

Amongst vcsh users, a popular solution to this problem is to use sparse
checkouts, representing the *intersection* of all the repositories in
a common sparse-checkout file[2].

This works well but there are still limitations about the ability to use
submodules because git expects the .gitmodules file to be checked out.

The user (or vcsh itself) might learn to fully populate one repository
at a time when working with submodules but this is unhandy and would
introduce serialization even when it's not strictly needed like in the
case of _reading_ .gitmodules.

As a side note, git submodules have worked perfectly fine with detached
work-trees for some time[3,4,5] so extending them to also play nice with
sparse checkouts seems the next logical step to cover the vcsh use case.

This series teaches git to try and read the .gitmodules file from the
index (HEAD:.gitmodules) when it's not available in the work dir.

It does so by first providing an opaque way to access the submodules
configuration, and then extends the access mechanism behind the scenes.

Writing to .gitmodules still requires it to be checked out.

This series should be in line with what Stefan and Jonathan proposed;
although it's not perfect yet:

  - naming of functions can be improved,
  - code can be moved around to better places,
  - maybe some notes should be added to Documentation/git-submodule.txt,
  - my git terminology may still be a little off: do "work tree" and
    "work directory" mean the same thing?

the functionality is there and we should have a decent baseline to work
on.

The patchset is based on the current master (ccdcbd54c447), the
test-suite passes after each commit and there are some per-patch
annotations.

If anyone wanted to pick up and finish the work feel free to do so,
otherwise please comment and I'll try to address issues as time permits.

Thanks,
   Antonio

[1] https://github.com/RichiH/vcsh
[2] https://github.com/RichiH/vcsh/issues/120#issuecomment-387335765
[3] http://git.661346.n2.nabble.com/git-submodule-vs-GIT-WORK-TREE-td7562165.html
[4] http://git.661346.n2.nabble.com/PATCH-Solve-git-submodule-issues-with-detached-work-trees-td7563377.html
[5] https://github.com/git/git/commit/be8779f7ac9a3be9aa783df008d59082f4054f67

Antonio Ospite (10):
  config: make config_from_gitmodules generally useful
  submodule: factor out a config_gitmodules_set function
  t7411: be nicer to other tests and really clean things up
  submodule--helper: add a new 'config' subcommand
  submodule: use the 'submodule--helper config' command
  submodule--helper: add a '--stage' option to the 'config' sub command
  submodule: use 'submodule--helper config --stage' to stage .gitmodules
  t7506: cleanup .gitmodules properly before setting up new scenario
  submodule: support reading .gitmodules even when it's not checked out
  t7415: add new test about using HEAD:.gitmodules from the index

 builtin/fetch.c                        |   2 +-
 builtin/mv.c                           |   2 +
 builtin/rm.c                           |   7 +-
 builtin/submodule--helper.c            | 100 +++++++++++++++++++-
 cache.h                                |   1 +
 config.c                               |  26 ++++--
 config.h                               |  10 +-
 git-submodule.sh                       |  10 +-
 submodule-config.c                     |  16 +---
 submodule.c                            |  37 ++++++--
 submodule.h                            |   2 +
 t/t7411-submodule-config.sh            |  63 ++++++++++++-
 t/t7415-submodule-sparse-gitmodules.sh | 124 +++++++++++++++++++++++++
 t/t7506-status-submodule.sh            |   3 +-
 14 files changed, 357 insertions(+), 46 deletions(-)
 create mode 100755 t/t7415-submodule-sparse-gitmodules.sh

-- 
Antonio Ospite
https://ao2.it
https://twitter.com/ao2it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

[RFC PATCH 01/10] config: make config_from_gitmodules generally useful

[Antonio Ospite]

The config_from_gitmodules() function is a good candidate for
a centralized point where to read the gitmodules configuration file.

Add a repo argument to it to make the function more general, and adjust
the current callers in cmd_fetch and update-clone.

As a proof of the utility of the change, start using the function also
in repo_read_gitmodules which was basically doing the same operations.

Signed-off-by: Antonio Ospite <ao2@ao2.it>
---
 builtin/fetch.c             |  2 +-
 builtin/submodule--helper.c |  2 +-
 config.c                    | 13 +++++++------
 config.h                    | 10 +---------
 submodule-config.c          | 16 ++++------------
 5 files changed, 14 insertions(+), 29 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 7ee83ac0f..a67ee7c39 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1445,7 +1445,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 	for (i = 1; i < argc; i++)
 		strbuf_addf(&default_rla, " %s", argv[i]);
 
-	config_from_gitmodules(gitmodules_fetch_config, NULL);
+	config_from_gitmodules(gitmodules_fetch_config, the_repository, NULL);
 	git_config(git_fetch_config, NULL);
 
 	argc = parse_options(argc, argv, prefix,
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index c2403a915..9e8f2acd5 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1602,7 +1602,7 @@ static int update_clone(int argc, const char **argv, const char *prefix)
 	};
 	suc.prefix = prefix;
 
-	config_from_gitmodules(gitmodules_update_clone_config, &max_jobs);
+	config_from_gitmodules(gitmodules_update_clone_config, the_repository, &max_jobs);
 	git_config(gitmodules_update_clone_config, &max_jobs);
 
 	argc = parse_options(argc, argv, prefix, module_update_clone_options,
diff --git a/config.c b/config.c
index 6f8f1d8c1..8ffe29330 100644
--- a/config.c
+++ b/config.c
@@ -2173,17 +2173,18 @@ int git_config_get_pathname(const char *key, const char **dest)
 }
 
 /*
- * Note: This function exists solely to maintain backward compatibility with
- * 'fetch' and 'update_clone' storing configuration in '.gitmodules' and should
- * NOT be used anywhere else.
+ * Note: Initially this function existed solely to maintain backward
+ * compatibility with 'fetch' and 'update_clone' storing configuration in
+ * '.gitmodules' but it turns out it can be useful as a centralized point to
+ * read the gitmodules config file.
  *
  * Runs the provided config function on the '.gitmodules' file found in the
  * working directory.
  */
-void config_from_gitmodules(config_fn_t fn, void *data)
+void config_from_gitmodules(config_fn_t fn, struct repository *repo, void *data)
 {
-	if (the_repository->worktree) {
-		char *file = repo_worktree_path(the_repository, GITMODULES_FILE);
+	if (repo->worktree) {
+		char *file = repo_worktree_path(repo, GITMODULES_FILE);
 		git_config_from_file(fn, file, data);
 		free(file);
 	}
diff --git a/config.h b/config.h
index cdac2fc73..43ce76c0f 100644
--- a/config.h
+++ b/config.h
@@ -215,15 +215,7 @@ extern int repo_config_get_maybe_bool(struct repository *repo,
 extern int repo_config_get_pathname(struct repository *repo,
 				    const char *key, const char **dest);
 
-/*
- * Note: This function exists solely to maintain backward compatibility with
- * 'fetch' and 'update_clone' storing configuration in '.gitmodules' and should
- * NOT be used anywhere else.
- *
- * Runs the provided config function on the '.gitmodules' file found in the
- * working directory.
- */
-extern void config_from_gitmodules(config_fn_t fn, void *data);
+extern void config_from_gitmodules(config_fn_t fn, struct repository *repo, void *data);
 
 extern int git_config_get_value(const char *key, const char **value);
 extern const struct string_list *git_config_get_value_multi(const char *key);
diff --git a/submodule-config.c b/submodule-config.c
index d87c3ff63..f39c71dfb 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -577,19 +577,11 @@ void repo_read_gitmodules(struct repository *repo)
 {
 	submodule_cache_check_init(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);
+	if (repo_read_index(repo) < 0)
+		return;
 
-		free(gitmodules);
-	}
+	if (!is_gitmodules_unmerged(repo->index))
+		config_from_gitmodules(gitmodules_cb, repo, repo);
 
 	repo->submodule_cache->gitmodules_read = 1;
 }
-- 
2.17.0

[RFC PATCH 02/10] submodule: factor out a config_gitmodules_set function

[Antonio Ospite]

Introduce a new config_gitmodules_set function to write config values to the
.gitmodules file.

This is in preparation for a future change which will use the function
to write to the .gitmodules file in a more controlled way instead of
using "git config -f .gitmodules".

Signed-off-by: Antonio Ospite <ao2@ao2.it>
---

Not sure about the name, and maybe it can go in config.c for symmetry with
config_from_gitmodules?

 submodule.c | 22 +++++++++++++++-------
 submodule.h |  1 +
 2 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/submodule.c b/submodule.c
index 74d35b257..7cfae89b6 100644
--- a/submodule.c
+++ b/submodule.c
@@ -80,6 +80,18 @@ static int for_each_remote_ref_submodule(const char *submodule,
 					fn, cb_data);
 }
 
+int config_gitmodules_set(const char *key, const char *value)
+{
+	int ret;
+
+	ret = git_config_set_in_file_gently(GITMODULES_FILE, key, value);
+	if (ret < 0)
+		/* Maybe the user already did that, don't error out here */
+		warning(_("Could not update .gitmodules entry %s"), key);
+
+	return ret;
+}
+
 /*
  * Try to update the "path" entry in the "submodule.<name>" section of the
  * .gitmodules file. Return 0 only if a .gitmodules file was found, a section
@@ -89,6 +101,7 @@ int update_path_in_gitmodules(const char *oldpath, const char *newpath)
 {
 	struct strbuf entry = STRBUF_INIT;
 	const struct submodule *submodule;
+	int ret;
 
 	if (!file_exists(GITMODULES_FILE)) /* Do nothing without .gitmodules */
 		return -1;
@@ -104,14 +117,9 @@ int update_path_in_gitmodules(const char *oldpath, const char *newpath)
 	strbuf_addstr(&entry, "submodule.");
 	strbuf_addstr(&entry, submodule->name);
 	strbuf_addstr(&entry, ".path");
-	if (git_config_set_in_file_gently(GITMODULES_FILE, entry.buf, newpath) < 0) {
-		/* Maybe the user already did that, don't error out here */
-		warning(_("Could not update .gitmodules entry %s"), entry.buf);
-		strbuf_release(&entry);
-		return -1;
-	}
+	ret = config_gitmodules_set(entry.buf, newpath);
 	strbuf_release(&entry);
-	return 0;
+	return ret;
 }
 
 /*
diff --git a/submodule.h b/submodule.h
index e5526f6aa..8a252e514 100644
--- a/submodule.h
+++ b/submodule.h
@@ -35,6 +35,7 @@ struct submodule_update_strategy {
 
 extern int is_gitmodules_unmerged(const struct index_state *istate);
 extern int is_staging_gitmodules_ok(struct index_state *istate);
+extern int config_gitmodules_set(const char *key, const char *value);
 extern int update_path_in_gitmodules(const char *oldpath, const char *newpath);
 extern int remove_path_from_gitmodules(const char *path);
 extern void stage_updated_gitmodules(struct index_state *istate);
-- 
2.17.0

[RFC PATCH 03/10] t7411: be nicer to other tests and really clean things up

[Antonio Ospite]

Tests 5 and 8 in t/t7411-submodule-config.sh add two commits with
invalid lines in .gitmodules but then only the second commit is removed.

This may affect subsequent tests if they assume that the .gitmodules
file has no errors.

Since those commits are not needed anymore remove both of them.

Signed-off-by: Antonio Ospite <ao2@ao2.it>
---

I am putting these fixups to the test-suite before the patch that actually
needs them so that the test-suite passes after each commit.

 t/t7411-submodule-config.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
index 0bde5850a..a648de6a9 100755
--- a/t/t7411-submodule-config.sh
+++ b/t/t7411-submodule-config.sh
@@ -135,7 +135,7 @@ test_expect_success 'error in history in fetchrecursesubmodule lets continue' '
 			HEAD submodule \
 				>actual &&
 		test_cmp expect_error actual  &&
-		git reset --hard HEAD^
+		git reset --hard HEAD~2
 	)
 '
 
-- 
2.17.0

[RFC PATCH 04/10] submodule--helper: add a new 'config' subcommand

[Antonio Ospite]

Add a new 'config' subcommand to 'submodule--helper', this extra level
of indirection makes it possible to add some flexibility to how the
submodules configuration is handled.

Signed-off-by: Antonio Ospite <ao2@ao2.it>
---
 builtin/submodule--helper.c | 39 +++++++++++++++++++++++++++++++++++++
 t/t7411-submodule-config.sh | 26 +++++++++++++++++++++++++
 2 files changed, 65 insertions(+)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 9e8f2acd5..b32110e3b 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1825,6 +1825,44 @@ static int is_active(int argc, const char **argv, const char *prefix)
 	return !is_submodule_active(the_repository, argv[1]);
 }
 
+static int config_print_callback(const char *key_, const char *value_, void *cb_data)
+{
+	char *key = cb_data;
+
+	if (!strcmp(key, key_))
+		printf("%s\n", value_);
+
+	return 0;
+}
+
+static int module_config(int argc, const char **argv, const char *prefix)
+{
+	int ret;
+
+	if (argc < 2 || argc > 3)
+		die("submodule--helper config takes 1 or 2 arguments: name [value]");
+
+	/* Equivalent to ACTION_GET in builtin/config.c */
+	if (argc == 2) {
+		char *key;
+
+		ret = git_config_parse_key(argv[1], &key, NULL);
+		if (ret < 0)
+			return CONFIG_INVALID_KEY;
+
+		config_from_gitmodules(config_print_callback, the_repository, key);
+
+		free(key);
+		return 0;
+	}
+
+	/* Equivalent to ACTION_SET in builtin/config.c */
+	if (argc == 3)
+		return config_gitmodules_set(argv[1], argv[2]);
+
+	return 0;
+}
+
 #define SUPPORT_SUPER_PREFIX (1<<0)
 
 struct cmd_struct {
@@ -1850,6 +1888,7 @@ static struct cmd_struct commands[] = {
 	{"push-check", push_check, 0},
 	{"absorb-git-dirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
 	{"is-active", is_active, 0},
+	{"config", module_config, 0},
 };
 
 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
index a648de6a9..dfe019f05 100755
--- a/t/t7411-submodule-config.sh
+++ b/t/t7411-submodule-config.sh
@@ -139,4 +139,30 @@ test_expect_success 'error in history in fetchrecursesubmodule lets continue' '
 	)
 '
 
+test_expect_success 'reading submodules config with "submodule--helper config"' '
+	(cd super &&
+		echo "../submodule" >expected &&
+		git submodule--helper config submodule.submodule.url >actual &&
+		test_cmp expected actual
+	)
+'
+
+test_expect_success 'writing submodules config with "submodule--helper config"' '
+	(cd super &&
+		echo "new_url" >expected &&
+		git submodule--helper config submodule.submodule.url "new_url" &&
+		git submodule--helper config submodule.submodule.url >actual &&
+		test_cmp expected actual
+	)
+'
+
+test_expect_success 'overwriting unstaged submodules config with "submodule--helper config"' '
+	(cd super &&
+		echo "newer_url" >expected &&
+		git submodule--helper config submodule.submodule.url "newer_url" &&
+		git submodule--helper config submodule.submodule.url >actual &&
+		test_cmp expected actual
+	)
+'
+
 test_done
-- 
2.17.0

[RFC PATCH 05/10] submodule: use the 'submodule--helper config' command

[Antonio Ospite]

Use the 'submodule--helper config' command in git-modules.sh to avoid
referring explicitly to .gitmodules by the hardcoded file path.

This makes it possible to access the submodules configuration in a more
flexible way.

Signed-off-by: Antonio Ospite <ao2@ao2.it>
---
 git-submodule.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 24914963c..0286b029f 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -71,7 +71,7 @@ get_submodule_config () {
 	value=$(git config submodule."$name"."$option")
 	if test -z "$value"
 	then
-		value=$(git config -f .gitmodules submodule."$name"."$option")
+		value=$(git submodule--helper config submodule."$name"."$option")
 	fi
 	printf '%s' "${value:-$default}"
 }
@@ -271,11 +271,11 @@ or you are unsure what this means choose another name with the '--name' option."
 	git add --no-warn-embedded-repo $force "$sm_path" ||
 	die "$(eval_gettext "Failed to add submodule '\$sm_path'")"
 
-	git config -f .gitmodules submodule."$sm_name".path "$sm_path" &&
-	git config -f .gitmodules submodule."$sm_name".url "$repo" &&
+	git submodule--helper config submodule."$sm_name".path "$sm_path" &&
+	git submodule--helper config submodule."$sm_name".url "$repo" &&
 	if test -n "$branch"
 	then
-		git config -f .gitmodules submodule."$sm_name".branch "$branch"
+		git submodule--helper config submodule."$sm_name".branch "$branch"
 	fi &&
 	git add --force .gitmodules ||
 	die "$(eval_gettext "Failed to register submodule '\$sm_path'")"
-- 
2.17.0

[RFC PATCH 06/10] submodule--helper: add a '--stage' option to the 'config' sub command

[Antonio Ospite]

Add a --stage option to the 'submodule--helper config' command so that
the .gitmodules file can be staged without referring to it explicitly by
file path.

In practice the config will still be written to .gitmoudules, there are
no plans to change the file path, but having this level of indirection
makes it possible to perform possible additional checks before staging
the file.

Signed-off-by: Antonio Ospite <ao2@ao2.it>
---
 builtin/submodule--helper.c | 44 +++++++++++++++++++++++++++++++++++--
 t/t7411-submodule-config.sh | 35 +++++++++++++++++++++++++++++
 2 files changed, 77 insertions(+), 2 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index b32110e3b..de5caa776 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -5,6 +5,7 @@
 #include "parse-options.h"
 #include "quote.h"
 #include "pathspec.h"
+#include "lockfile.h"
 #include "dir.h"
 #include "submodule.h"
 #include "submodule-config.h"
@@ -1837,10 +1838,49 @@ static int config_print_callback(const char *key_, const char *value_, void *cb_
 
 static int module_config(int argc, const char **argv, const char *prefix)
 {
+	int stage_gitmodules = 0;
 	int ret;
 
-	if (argc < 2 || argc > 3)
-		die("submodule--helper config takes 1 or 2 arguments: name [value]");
+	struct option module_config_options[] = {
+		OPT_BOOL(0, "stage", &stage_gitmodules,
+			    N_("stage the .gitmodules file content")),
+		OPT_END()
+	};
+	const char *const git_submodule_helper_usage[] = {
+		N_("git submodule--helper config name [value]"),
+		N_("git submodule--helper config --stage"),
+		NULL
+	};
+
+	argc = parse_options(argc, argv, prefix, module_config_options,
+			     git_submodule_helper_usage, PARSE_OPT_KEEP_ARGV0);
+
+	if ((stage_gitmodules && argc > 1) ||
+	    (!stage_gitmodules && (argc < 2 || argc > 3)))
+		usage_with_options(git_submodule_helper_usage, module_config_options);
+
+	/*
+	 * Equivalent to "git add --force .gitmodules".
+	 *
+	 * Silently override already staged changes, to support consecutive
+	 * invocations of "git submodule add".
+	 */
+	if (stage_gitmodules) {
+		static struct lock_file lock_file;
+
+		hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);
+
+		if (read_cache() < 0)
+			die(_("index file corrupt"));
+
+		stage_updated_gitmodules(&the_index);
+
+		if (write_locked_index(&the_index, &lock_file,
+				       COMMIT_LOCK | SKIP_IF_UNCHANGED))
+			die(_("Unable to write new index file"));
+
+		return 0;
+	}
 
 	/* Equivalent to ACTION_GET in builtin/config.c */
 	if (argc == 2) {
diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
index dfe019f05..a0b2f7cd6 100755
--- a/t/t7411-submodule-config.sh
+++ b/t/t7411-submodule-config.sh
@@ -165,4 +165,39 @@ test_expect_success 'overwriting unstaged submodules config with "submodule--hel
 	)
 '
 
+test_expect_success 'staging submodules config with "submodule--helper config"' '
+	(cd super &&
+		: >expected &&
+		git diff --name-only --cached >actual &&
+		test_cmp expected actual &&
+		git submodule--helper config --stage &&
+		echo ".gitmodules" >expected &&
+		git diff --name-only --cached >actual &&
+		test_cmp expected actual
+	)
+'
+
+test_expect_success 'overwriting staged submodules config with "submodule--helper config"' '
+	(cd super &&
+		echo "even_newer_url" >expected &&
+		git submodule--helper config submodule.submodule.url "even_newer_url" &&
+		git submodule--helper config submodule.submodule.url >actual &&
+		test_cmp expected actual
+	)
+'
+
+test_expect_success 're-staging overridden submodule config with "submodule--helper config"' '
+	(cd super &&
+		echo ".gitmodules" >expected &&
+		git diff --name-only --cached >actual &&
+		test_cmp expected actual &&
+		echo "bogus config" >.gitmodules &&
+		git submodule--helper config --stage &&
+		git diff --name-only --cached >actual &&
+		test_cmp expected actual &&
+		git reset HEAD .gitmodules &&
+		git checkout .gitmodules
+	)
+'
+
 test_done
-- 
2.17.0

[RFC PATCH 07/10] submodule: use 'submodule--helper config --stage' to stage .gitmodules

[Antonio Ospite]

Use 'git submodule--helper config --stage' in git-submodule.sh to remove
the last place where the .gitmodules file is mentioned explicitly by its
file path.

Signed-off-by: Antonio Ospite <ao2@ao2.it>
---
 git-submodule.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 0286b029f..61e62ef17 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -277,7 +277,7 @@ or you are unsure what this means choose another name with the '--name' option."
 	then
 		git submodule--helper config submodule."$sm_name".branch "$branch"
 	fi &&
-	git add --force .gitmodules ||
+	git submodule--helper config --stage ||
 	die "$(eval_gettext "Failed to register submodule '\$sm_path'")"
 
 	# NEEDSWORK: In a multi-working-tree world, this needs to be
-- 
2.17.0

[RFC PATCH 08/10] t7506: cleanup .gitmodules properly before setting up new scenario

[Antonio Ospite]

In t/t7506-status-submodule.sh at some point a new scenario is set up to
test different things, in particular new submodules are added which are
meant to completely replace the previous ones.

However the code just removes .gitmodules from the work tree, still
leaving it in the index.

This will break when "submodule--helper config" learns to handle
.gitmodules from the index and performs some check when doing that.

Since the test means to get rid of .gitmodules anyways, let's completely
remove it from the index, to actually start afresh in the new scenario.

This is more future-proof without breaking current tests.

Signed-off-by: Antonio Ospite <ao2@ao2.it>
---
 t/t7506-status-submodule.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh
index 9edf6572e..389173294 100755
--- a/t/t7506-status-submodule.sh
+++ b/t/t7506-status-submodule.sh
@@ -325,7 +325,8 @@ test_expect_success 'setup superproject with untracked file in nested submodule'
 	(
 		cd super &&
 		git clean -dfx &&
-		rm .gitmodules &&
+		git rm .gitmodules &&
+		git commit -m "remove .gitmodules" &&
 		git submodule add -f ./sub1 &&
 		git submodule add -f ./sub2 &&
 		git submodule add -f ./sub1 sub3 &&
-- 
2.17.0

[RFC PATCH 09/10] submodule: support reading .gitmodules even when it's not checked out

[Antonio Ospite]

When the .gitmodules file is not available in the working directory, try
using HEAD:.gitmodules from the index. This covers the case when the
file is part of the repository but for some reason it is not checked
out, for example because of a sparse checkout.

This makes it possible to use at least the 'git submodule' commands
which *read* the gitmodules configuration file without fully populating
the work dir.

Writing to .gitmodules wills still require that the file is checked out,
so check for that in config_gitmodules_set.

Signed-off-by: Antonio Ospite <ao2@ao2.it>
---

I am doing the is_gitmodules_hidden() check in the open for now, I am not sure
whether it is approprate to do that inside stage_updated_gitmodules.

 builtin/mv.c                |  2 ++
 builtin/rm.c                |  7 +++++--
 builtin/submodule--helper.c | 21 ++++++++++++++++++++-
 cache.h                     |  1 +
 config.c                    | 15 +++++++++++++--
 submodule.c                 | 15 +++++++++++++++
 submodule.h                 |  1 +
 7 files changed, 57 insertions(+), 5 deletions(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index 7a63667d6..41fd9b7be 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -83,6 +83,8 @@ static void prepare_move_submodule(const char *src, int first,
 		die(_("Directory %s is in index and no submodule?"), src);
 	if (!is_staging_gitmodules_ok(&the_index))
 		die(_("Please stage your changes to .gitmodules or stash them to proceed"));
+	if (is_gitmodules_hidden(&the_index))
+		die(_("cannot work with hidden submodule config"));
 	strbuf_addf(&submodule_dotgit, "%s/.git", src);
 	*submodule_gitfile = read_gitfile(submodule_dotgit.buf);
 	if (*submodule_gitfile)
diff --git a/builtin/rm.c b/builtin/rm.c
index 5b6fc7ee8..e3526a342 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -284,9 +284,12 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 		ALLOC_GROW(list.entry, list.nr + 1, list.alloc);
 		list.entry[list.nr].name = xstrdup(ce->name);
 		list.entry[list.nr].is_submodule = S_ISGITLINK(ce->ce_mode);
-		if (list.entry[list.nr++].is_submodule &&
-		    !is_staging_gitmodules_ok(&the_index))
+		if (list.entry[list.nr++].is_submodule) {
+		    if (!is_staging_gitmodules_ok(&the_index))
 			die (_("Please stage your changes to .gitmodules or stash them to proceed"));
+		    if (is_gitmodules_hidden(&the_index))
+			die(_("cannot work with hidden submodule config"));
+		}
 	}
 
 	if (pathspec.nr) {
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index de5caa776..b3bdb4b66 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1873,6 +1873,9 @@ static int module_config(int argc, const char **argv, const char *prefix)
 		if (read_cache() < 0)
 			die(_("index file corrupt"));
 
+		if (is_gitmodules_hidden(&the_index))
+			die(_("cannot stage changes to hidden submodule config"));
+
 		stage_updated_gitmodules(&the_index);
 
 		if (write_locked_index(&the_index, &lock_file,
@@ -1897,8 +1900,24 @@ static int module_config(int argc, const char **argv, const char *prefix)
 	}
 
 	/* Equivalent to ACTION_SET in builtin/config.c */
-	if (argc == 3)
+	if (argc == 3) {
+		struct object_id oid;
+
+		/* 
+		 * If the .gitmodules file is not in the work tree but it is
+		 * in the index, stop, as writing new values and staging them
+		 * would blindly overwrite ALL the old content.
+		 *
+		 * Do not use is_gitmodules_hidden() here, to gracefully
+		 * handle the case when .gitmodules is neither in the work
+		 * tree nor in the index, i.e.: a new GITMODULES_FILE is going
+		 * to be created.
+		 */
+		if (!file_exists(GITMODULES_FILE) && get_oid(GITMODULES_BLOB, &oid) >= 0)
+			die(_("cannot change unchecked out .gitmodules, check it out first"));
+
 		return config_gitmodules_set(argv[1], argv[2]);
+	}
 
 	return 0;
 }
diff --git a/cache.h b/cache.h
index 0c1fb9fbc..6d45b0cbb 100644
--- a/cache.h
+++ b/cache.h
@@ -417,6 +417,7 @@ static inline enum object_type object_type(unsigned int mode)
 #define INFOATTRIBUTES_FILE "info/attributes"
 #define ATTRIBUTE_MACRO_PREFIX "[attr]"
 #define GITMODULES_FILE ".gitmodules"
+#define GITMODULES_BLOB "HEAD:.gitmodules"
 #define GIT_NOTES_REF_ENVIRONMENT "GIT_NOTES_REF"
 #define GIT_NOTES_DEFAULT_REF "refs/notes/commits"
 #define GIT_NOTES_DISPLAY_REF_ENVIRONMENT "GIT_NOTES_DISPLAY_REF"
diff --git a/config.c b/config.c
index 8ffe29330..7d9744622 100644
--- a/config.c
+++ b/config.c
@@ -2184,8 +2184,19 @@ int git_config_get_pathname(const char *key, const char **dest)
 void config_from_gitmodules(config_fn_t fn, struct repository *repo, void *data)
 {
 	if (repo->worktree) {
-		char *file = repo_worktree_path(repo, GITMODULES_FILE);
-		git_config_from_file(fn, file, data);
+		struct git_config_source config_source = { 0 };
+		const struct config_options opts = { 0 };
+		struct object_id oid;
+		char *file;
+
+		file = repo_worktree_path(repo, GITMODULES_FILE);
+		if (file_exists(file))
+			config_source.file = file;
+		else if (get_oid(GITMODULES_BLOB, &oid) >= 0)
+			config_source.blob = GITMODULES_BLOB;
+
+		config_with_options(fn, data, &config_source, &opts);
+
 		free(file);
 	}
 }
diff --git a/submodule.c b/submodule.c
index 7cfae89b6..83d0ca5a6 100644
--- a/submodule.c
+++ b/submodule.c
@@ -50,6 +50,21 @@ int is_gitmodules_unmerged(const struct index_state *istate)
 	return 0;
 }
 
+/*
+ * Check if the .gitmodules file is in the index but it is marked as hidden,
+ * like for example via a sparse checkout.
+ */
+int is_gitmodules_hidden(const struct index_state *istate)
+{
+	int pos = index_name_pos(istate, GITMODULES_FILE, strlen(GITMODULES_FILE));
+
+	if (pos >= 0) {
+		return ce_skip_worktree(istate->cache[pos]);
+	}
+
+	return 0;
+}
+
 /*
  * Check if the .gitmodules file has unstaged modifications.  This must be
  * checked before allowing modifications to the .gitmodules file with the
diff --git a/submodule.h b/submodule.h
index 8a252e514..4ec9a3781 100644
--- a/submodule.h
+++ b/submodule.h
@@ -34,6 +34,7 @@ struct submodule_update_strategy {
 #define SUBMODULE_UPDATE_STRATEGY_INIT {SM_UPDATE_UNSPECIFIED, NULL}
 
 extern int is_gitmodules_unmerged(const struct index_state *istate);
+extern int is_gitmodules_hidden(const struct index_state *istate);
 extern int is_staging_gitmodules_ok(struct index_state *istate);
 extern int config_gitmodules_set(const char *key, const char *value);
 extern int update_path_in_gitmodules(const char *oldpath, const char *newpath);
-- 
2.17.0

[RFC PATCH 10/10] t7415: add new test about using HEAD:.gitmodules from the index

[Antonio Ospite]

git submodule commands can now access .gitmodules from the index when
it's not checked out in the work tree, add some tests for that scenario.

Signed-off-by: Antonio Ospite <ao2@ao2.it>
---
 t/t7415-submodule-sparse-gitmodules.sh | 124 +++++++++++++++++++++++++
 1 file changed, 124 insertions(+)
 create mode 100755 t/t7415-submodule-sparse-gitmodules.sh

diff --git a/t/t7415-submodule-sparse-gitmodules.sh b/t/t7415-submodule-sparse-gitmodules.sh
new file mode 100755
index 000000000..3ae269b3a
--- /dev/null
+++ b/t/t7415-submodule-sparse-gitmodules.sh
@@ -0,0 +1,124 @@
+#!/bin/sh
+#
+# Copyright (C) 2018  Antonio Ospite <ao2@ao2.it>
+#
+
+test_description=' Test reading/writing the gitmodules config file when not checked out
+
+This test verifies that reading the gitmodules config file from the index when
+it is not checked out works, and that writing to it does not.
+'
+
+. ./test-lib.sh
+
+test_expect_success 'sparse checkout setup which hides .gitmodules' '
+	echo file > file &&
+	git add file &&
+	test_tick &&
+	git commit -m upstream &&
+	git clone . super &&
+	git clone super submodule &&
+	git clone super new_submodule &&
+	(cd super &&
+		git submodule add ../submodule
+		test_tick &&
+		git commit -m submodule &&
+		cat >.git/info/sparse-checkout <<\EOF &&
+/*
+!/.gitmodules
+EOF
+		git config core.sparsecheckout true &&
+		git read-tree -m -u HEAD &&
+		test ! -e .gitmodules
+	)
+'
+
+test_expect_success 'reading gitmodules config file when it is not checked out' '
+	(cd super &&
+		echo "../submodule" >expected &&
+		git submodule--helper config submodule.submodule.url >actual &&
+		test_cmp expected actual
+	)
+'
+
+test_expect_success 'not writing gitmodules config file when it is not checked out' '
+	(cd super &&
+		test_must_fail git submodule--helper config submodule.submodule.url newurl
+	)
+'
+
+test_expect_success 'not staging gitmodules config when it is not checked out' '
+	(cd super &&
+		test_must_fail git submodule--helper config --stage
+	)
+'
+
+test_expect_success 'not even staging manually crafted .gitmodules when it is not supposed to be checked out' '
+	(cd super &&
+		echo "bogus content" > .gitmodules &&
+		test_must_fail git submodule--helper config --stage &&
+		rm .gitmodules
+	)
+'
+
+test_expect_success 'initialising submodule when the gitmodules config is not checked out' '
+	(cd super &&
+		git submodule init
+	)
+'
+
+test_expect_success 'showing submodule summary when the gitmodules config is not checked out' '
+	(cd super &&
+		git submodule summary
+	)
+'
+
+test_expect_success 'updating submodule when the gitmodules config is not checked out' '
+	(cd submodule &&
+		echo file2 >file2 &&
+		git add file2 &&
+		git commit -m "add file2 to submodule"
+	) &&
+	(cd super &&
+		git submodule update
+	)
+'
+
+test_expect_success 'not moving submodule when the gitmodules config is not checked out' '
+	(cd super &&
+		test_must_fail git mv submodule moved_submodule
+	)
+'
+
+test_expect_success 'not removing submodule when the gitmodules config is not checked out' '
+	(cd super &&
+		test_must_fail git rm -r submodule
+	)
+'
+
+test_expect_success 'not adding submodules when the gitmodules config is not checked out' '
+	(cd super &&
+		test_must_fail git submodule add ../new_submodule
+	)
+'
+
+# "git add" in the test above fails as expected, however it still leaves the
+# cloned directory there and adds a config entry to .git/config. This is
+# because no cleanup is done by cmd_add in git-submodule.sh when "git
+# submodule--helper config" fails to add a new config setting.
+#
+# If we added the following commands to the test above:
+#
+#   rm -rf .git/modules/new_submodule &&
+#   git reset HEAD new_submodule &&
+#   rm -rf new_submodule
+#
+# then the repository would be in a clean status and the test below would
+# pass, but maybe cmd_add should do that.
+test_expect_failure 'init submodule after adding failed when the gitmodules config is not checked out' '
+	(cd super &&
+		git submodule init
+	)
+'
+
+test_done
-- 
2.17.0

Re: [RFC PATCH 01/10] config: make config_from_gitmodules generally useful

[Brandon Williams]

On 05/14, Antonio Ospite wrote:
> The config_from_gitmodules() function is a good candidate for
> a centralized point where to read the gitmodules configuration file.
> 
> Add a repo argument to it to make the function more general, and adjust
> the current callers in cmd_fetch and update-clone.
> 
> As a proof of the utility of the change, start using the function also
> in repo_read_gitmodules which was basically doing the same operations.
> 
> Signed-off-by: Antonio Ospite <ao2@ao2.it>
> ---
>  builtin/fetch.c             |  2 +-
>  builtin/submodule--helper.c |  2 +-
>  config.c                    | 13 +++++++------
>  config.h                    | 10 +---------
>  submodule-config.c          | 16 ++++------------
>  5 files changed, 14 insertions(+), 29 deletions(-)
> 
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 7ee83ac0f..a67ee7c39 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -1445,7 +1445,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
>  	for (i = 1; i < argc; i++)
>  		strbuf_addf(&default_rla, " %s", argv[i]);
>  
> -	config_from_gitmodules(gitmodules_fetch_config, NULL);
> +	config_from_gitmodules(gitmodules_fetch_config, the_repository, NULL);
>  	git_config(git_fetch_config, NULL);
>  
>  	argc = parse_options(argc, argv, prefix,
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index c2403a915..9e8f2acd5 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -1602,7 +1602,7 @@ static int update_clone(int argc, const char **argv, const char *prefix)
>  	};
>  	suc.prefix = prefix;
>  
> -	config_from_gitmodules(gitmodules_update_clone_config, &max_jobs);
> +	config_from_gitmodules(gitmodules_update_clone_config, the_repository, &max_jobs);
>  	git_config(gitmodules_update_clone_config, &max_jobs);
>  
>  	argc = parse_options(argc, argv, prefix, module_update_clone_options,
> diff --git a/config.c b/config.c
> index 6f8f1d8c1..8ffe29330 100644
> --- a/config.c
> +++ b/config.c
> @@ -2173,17 +2173,18 @@ int git_config_get_pathname(const char *key, const char **dest)
>  }
>  
>  /*
> - * Note: This function exists solely to maintain backward compatibility with
> - * 'fetch' and 'update_clone' storing configuration in '.gitmodules' and should
> - * NOT be used anywhere else.
> + * Note: Initially this function existed solely to maintain backward
> + * compatibility with 'fetch' and 'update_clone' storing configuration in
> + * '.gitmodules' but it turns out it can be useful as a centralized point to
> + * read the gitmodules config file.

I'm all for what you're trying to accomplish in this patch series but I
think a little more care needs to be taken here.  Maybe about a year ago
I did some refactoring with how the gitmodules file was loaded and it
was decided that allowing arbitrary configuration in the .gitmodules
file was a bad thing, so I tried to make sure that it was very difficult
to sneak in more of that and limiting it to the places where it was
already done (fetch and update_clone).  Now this patch is explicitly
changing the comment on this function to loosen the requirements I made
when it was introduced, which could be problematic in the future.

So here's what I suggest doing:
  1. Move this function from config.{c,h} to submodule-config.{c,h} to
     make it clear who owns this.
  2. Move the gitmodules_set function you created to live in submodule-config.
  3. You can still use this function as the main driver for reading the
     submodule config BUT the comment above the function in both the .c and
     .h files should be adapted so that it is clearly spells out that the
     function is to be used only by the submodule config code (reading it
     in repo_read_gitmodules and reading/writing it in the
     submodule-helper config function you've added) and that the only
     exceptions to this are to maintain backwards compatibility with the
     existing callers and that new callers shouldn't be added.

This is just a long way to say that if new callers to this function are
added in the future that it is made very clear in the code that the
.gitmodules file exists for a specific purpose and that it shouldn't be
exploited to ship config with a repository. (If we end up allowing
config to be shipped with a repository at a later date that will need to
be handled with great care due to security concerns).

Thanks for working on this, the end result is definitely a step in the
direction I've wanted the submodule config to head to.

>   *
>   * Runs the provided config function on the '.gitmodules' file found in the
>   * working directory.
>   */
> -void config_from_gitmodules(config_fn_t fn, void *data)
> +void config_from_gitmodules(config_fn_t fn, struct repository *repo, void *data)
>  {
> -	if (the_repository->worktree) {
> -		char *file = repo_worktree_path(the_repository, GITMODULES_FILE);
> +	if (repo->worktree) {
> +		char *file = repo_worktree_path(repo, GITMODULES_FILE);
>  		git_config_from_file(fn, file, data);
>  		free(file);
>  	}
> diff --git a/config.h b/config.h
> index cdac2fc73..43ce76c0f 100644
> --- a/config.h
> +++ b/config.h
> @@ -215,15 +215,7 @@ extern int repo_config_get_maybe_bool(struct repository *repo,
>  extern int repo_config_get_pathname(struct repository *repo,
>  				    const char *key, const char **dest);
>  
> -/*
> - * Note: This function exists solely to maintain backward compatibility with
> - * 'fetch' and 'update_clone' storing configuration in '.gitmodules' and should
> - * NOT be used anywhere else.
> - *
> - * Runs the provided config function on the '.gitmodules' file found in the
> - * working directory.
> - */
> -extern void config_from_gitmodules(config_fn_t fn, void *data);
> +extern void config_from_gitmodules(config_fn_t fn, struct repository *repo, void *data);
>  
>  extern int git_config_get_value(const char *key, const char **value);
>  extern const struct string_list *git_config_get_value_multi(const char *key);
> diff --git a/submodule-config.c b/submodule-config.c
> index d87c3ff63..f39c71dfb 100644
> --- a/submodule-config.c
> +++ b/submodule-config.c
> @@ -577,19 +577,11 @@ void repo_read_gitmodules(struct repository *repo)
>  {
>  	submodule_cache_check_init(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);
> +	if (repo_read_index(repo) < 0)
> +		return;
>  
> -		free(gitmodules);
> -	}
> +	if (!is_gitmodules_unmerged(repo->index))
> +		config_from_gitmodules(gitmodules_cb, repo, repo);
>  
>  	repo->submodule_cache->gitmodules_read = 1;
>  }
> -- 
> 2.17.0
> 

-- 
Brandon Williams

Re: [RFC PATCH 01/10] config: make config_from_gitmodules generally useful

[Stefan Beller]

On Mon, May 14, 2018 at 3:58 AM, Antonio Ospite <ao2@ao2.it> wrote:
> The config_from_gitmodules() function is a good candidate for
> a centralized point where to read the gitmodules configuration file.

It is very tempting to use that function. However it was introduced
specifically to not do that. ;)

See the series that was merged at 5aa0b6c506c (Merge branch
'bw/grep-recurse-submodules', 2017-08-22), specifically
f20e7c1ea24 (submodule: remove submodule.fetchjobs from
submodule-config parsing, 2017-08-02), where both
builtin/fetch as well as the submodule helper use the pattern to
read from the .gitmodules file va this function and then overlay it
with the read from config.

> Add a repo argument to it to make the function more general, and adjust
> the current callers in cmd_fetch and update-clone.

This could be a preparatory patch, but including it here is fine, too.

> As a proof of the utility of the change, start using the function also
> in repo_read_gitmodules which was basically doing the same operations.

I think they were separated for the reason outlined above, or what Brandon
said in his reply.

I think extending 'repo_read_gitmodules' makes sense, as that is
used everywhere for the loading of submodule configuration.

Re: [RFC PATCH 00/10] Make submodules work if .gitmodules is not checked out

[Stefan Beller]

Hi Antonio,

thanks for sending this series! Happy to review it!

>   - my git terminology may still be a little off: do "work tree" and
>     "work directory" mean the same thing?

Back in the old days, you had a "worktree" which is a directory where
things are checked out and you modify files. It sort of the opposite of the
"git directory", which git uses to store all its information.

Then quite some time later, the command git-worktree was invented.
Now we had 2 things with the same name, which is unfortunate.
But as the command git-worktree added more of these directories
that you could work in, the name collision was not apparent.

Later when people noticed the subtle difference between the
command and the thing on the file system, consensus seemed to be
that the thing on the file system should rather be called "working tree"
such that it sounds very similar but is distinguishable from the command.

However the outcome of the discussion did not yield a bi&complete
refactoring of the code base, such that there are still places with "worktree"
referring to the thing on the FS, "working trees".

I am not aware that "working directory" is an official term we use in
any documentation for Git, but it sounds like you mean a "working tree".
(From a point of view not based on the version control, "working directory"
may sound more correct, note however as the directories in Git are
named trees, the working "tree" sounds as if you can make changes to
Git trees, ... which you can. :) )

> If anyone wanted to pick up and finish the work feel free to do so,
> otherwise please comment and I'll try to address issues as time permits.

First let's review this series. :)

Thanks,
Stefan

Re: [RFC PATCH 02/10] submodule: factor out a config_gitmodules_set function

[Stefan Beller]

On Mon, May 14, 2018 at 3:58 AM, Antonio Ospite <ao2@ao2.it> wrote:
> Introduce a new config_gitmodules_set function to write config values to the
> .gitmodules file.
>
> This is in preparation for a future change which will use the function
> to write to the .gitmodules file in a more controlled way instead of
> using "git config -f .gitmodules".
>
> Signed-off-by: Antonio Ospite <ao2@ao2.it>
> ---
>
> Not sure about the name, and maybe it can go in config.c for symmetry with
> config_from_gitmodules?

What is the function about (in the end state and now) ?
is it more of a
* configure_submodules_config()
  which would convey it is a generic function to configure submodules
  (i.e. it may not even write to *the* .gitmodules file but somewhere else,
  such as a helper ref)
  This doesn't sound like it as we make use of the function in
  update_path_in_gitmodules() that is used from git-mv, which would want
  to ensure that the specific .gitmodules file is changed.
* gitmodules_file_set()
  that focuses on the specific file that we want to modify?
* ...

Let's continue reading the series to see the end state for
a good name.

Stefan

Re: [RFC PATCH 03/10] t7411: be nicer to other tests and really clean things up

[Stefan Beller]

On Mon, May 14, 2018 at 3:58 AM, Antonio Ospite <ao2@ao2.it> wrote:
> Tests 5 and 8 in t/t7411-submodule-config.sh add two commits with
> invalid lines in .gitmodules but then only the second commit is removed.
>
> This may affect subsequent tests if they assume that the .gitmodules
> file has no errors.
>
> Since those commits are not needed anymore remove both of them.
>
> Signed-off-by: Antonio Ospite <ao2@ao2.it>
> ---
>
> I am putting these fixups to the test-suite before the patch that actually
> needs them so that the test-suite passes after each commit.
>
>  t/t7411-submodule-config.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
> index 0bde5850a..a648de6a9 100755
> --- a/t/t7411-submodule-config.sh
> +++ b/t/t7411-submodule-config.sh
> @@ -135,7 +135,7 @@ test_expect_success 'error in history in fetchrecursesubmodule lets continue' '
>                         HEAD submodule \
>                                 >actual &&
>                 test_cmp expect_error actual  &&
> -               git reset --hard HEAD^
> +               git reset --hard HEAD~2
>         )
>  '

As this is the last test in this file, we do not change any subsequent
tests in a subtle way.
Good!

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

FYI:
This test -- of course -- doesn't quite follow the latest coding guidelines,
as usually we'd prefer a test_when_finished "<cmd to restore>"
at the beginning of a test.

Re: [RFC PATCH 04/10] submodule--helper: add a new 'config' subcommand

[Stefan Beller]

On Mon, May 14, 2018 at 3:58 AM, Antonio Ospite <ao2@ao2.it> wrote:
> Add a new 'config' subcommand to 'submodule--helper', this extra level
> of indirection makes it possible to add some flexibility to how the
> submodules configuration is handled.
>
> Signed-off-by: Antonio Ospite <ao2@ao2.it>
> ---
>  builtin/submodule--helper.c | 39 +++++++++++++++++++++++++++++++++++++
>  t/t7411-submodule-config.sh | 26 +++++++++++++++++++++++++
>  2 files changed, 65 insertions(+)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 9e8f2acd5..b32110e3b 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -1825,6 +1825,44 @@ static int is_active(int argc, const char **argv, const char *prefix)
>         return !is_submodule_active(the_repository, argv[1]);
>  }
>
> +static int config_print_callback(const char *key_, const char *value_, void *cb_data)
> +{
> +       char *key = cb_data;
> +
> +       if (!strcmp(key, key_))
> +               printf("%s\n", value_);
> +
> +       return 0;
> +}
> +
> +static int module_config(int argc, const char **argv, const char *prefix)
> +{
> +       int ret;
> +
> +       if (argc < 2 || argc > 3)
> +               die("submodule--helper config takes 1 or 2 arguments: name [value]");
> +
> +       /* Equivalent to ACTION_GET in builtin/config.c */
> +       if (argc == 2) {
> +               char *key;
> +
> +               ret = git_config_parse_key(argv[1], &key, NULL);
> +               if (ret < 0)
> +                       return CONFIG_INVALID_KEY;
> +
> +               config_from_gitmodules(config_print_callback, the_repository, key);
> +
> +               free(key);
> +               return 0;
> +       }
> +
> +       /* Equivalent to ACTION_SET in builtin/config.c */
> +       if (argc == 3)
> +               return config_gitmodules_set(argv[1], argv[2]);

Ah, here we definitely want to set it in the .gitmodules file?
(Or does that change later in this series?)

> +
> +       return 0;
> +}
> +
>  #define SUPPORT_SUPER_PREFIX (1<<0)
>
>  struct cmd_struct {
> @@ -1850,6 +1888,7 @@ static struct cmd_struct commands[] = {
>         {"push-check", push_check, 0},
>         {"absorb-git-dirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
>         {"is-active", is_active, 0},
> +       {"config", module_config, 0},
>  };
>
>  int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
> diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
> index a648de6a9..dfe019f05 100755
> --- a/t/t7411-submodule-config.sh
> +++ b/t/t7411-submodule-config.sh
> @@ -139,4 +139,30 @@ test_expect_success 'error in history in fetchrecursesubmodule lets continue' '
>         )
>  '
>
> +test_expect_success 'reading submodules config with "submodule--helper config"' '
> +       (cd super &&

I think the project prefers a style
of the cd at the same level of the echo and the following commands.

However we might not need the (cd super && ...) via

  echo "../submodule"  >expected
  git -C super ubmodule--helper config submodule.submodule.url >../actual
  test_cmp expected actual

Our friends developing git on Windows will thank us for saving
to spawn a shell as spawning processes is expensive on Windows. :)
Also we have fewer lines of code.

The patch looks good to me,
Thanks,
Stefan

Re: [RFC PATCH 09/10] submodule: support reading .gitmodules even when it's not checked out

[Stefan Beller]

On Mon, May 14, 2018 at 3:58 AM, Antonio Ospite <ao2@ao2.it> wrote:
> When the .gitmodules file is not available in the working directory, try
> using HEAD:.gitmodules from the index.

I think HEAD:.gitmodules is different than the index (the former is
part of the latest commit, whereas the index could have changed via
git-add, that is not committed yet).

> This covers the case when the
> file is part of the repository but for some reason it is not checked
> out, for example because of a sparse checkout.
>
> This makes it possible to use at least the 'git submodule' commands
> which *read* the gitmodules configuration file without fully populating
> the work dir.

Instead of checking for an explicit sparse "hidden" could we just rely on
the file missing? Then I could continue using submodules if I just
"rm .gitmodules".

>
> Writing to .gitmodules wills still require that the file is checked out,
> so check for that in config_gitmodules_set.

That makes sense!

>
> Signed-off-by: Antonio Ospite <ao2@ao2.it>
> ---
>
> I am doing the is_gitmodules_hidden() check in the open for now, I am not sure
> whether it is approprate to do that inside stage_updated_gitmodules.

Why do we need that check at all?

In your use case, you want to checkout *a* .gitmodules file, not necessarily
the .gitmodules file of that repo you're currently working on. So it
sort of makes
sense to prevent cross-repo changes (i.e. committing the .gitmodules
accidentally
into the wrong repo)

Stefan

Re: [RFC PATCH 00/10] Make submodules work if .gitmodules is not checked out

[Junio C Hamano]

Antonio Ospite <ao2@ao2.it> writes:

>   - my git terminology may still be a little off: do "work tree" and
>     "work directory" mean the same thing?

Just on this tangent.  

When we talk about the current working directory of a process
returned by getcwd((3) call, we typically spell that word fully (or
say $cwd).

We lived without the modern "git worktree" layout for a long time,
but there was a hack in contrib/ called "git-new-workdir".  This
creates something similar (in spirit, not in implementation) to
"worktree" in modern Git lingo, but because not many people use the
contrib feature (at least there is only few who get confused by it
and ask questions publicly, anyway), we do not hear "workdir" very
often.

Since very early days of Git, we had "working tree" that is the
directory hierarchy that corresponds to what you place in the index,
and "Git repository" which is the ".git" directory that has that
index.  Even though most everybody else was calling it the "working
tree", primarily because I was young(er) and (more) stupid, I often
called the same thing "work tree", and made things worse by
introducing GIT_WORK_TREE environment variable etc.  But "working
tree" is the official terminology to denote a directory hierarchy
that corresponds to (and controlled by) a single index file as
opposed to what is in ".git/" directory..

The official term "worktree" came much later, with "git worktree"
command.  This allow multiple working trees to be associated with a
single ".git/" directory.  Most of the time "worktree" and "working
tree" can be used interchangeably, but we tend to say "working tree"
when we have more emphasis on the non-bareness of the repository and
talking about checked-out files, and say "worktree" when we are
mostly interested in the layout that have more than one working trees
associated to a single Git repository.  What you get by "git clone",
for example, is just a single repository with a single working tree,
and nobody sane (other than young(er) and (more) stupid version of
me 10 years ago) would call the latter a "worktree" these days, as
there is not yet a secondary worktree to contrast it with.

Re: [RFC PATCH 01/10] config: make config_from_gitmodules generally useful

[Antonio Ospite]

On Mon, 14 May 2018 11:19:28 -0700
Brandon Williams <bmwill@google.com> wrote:

Hi Brandon,

sorry for the delay, some comments below.

> On 05/14, Antonio Ospite wrote:
> > The config_from_gitmodules() function is a good candidate for
> > a centralized point where to read the gitmodules configuration file.
> > 
> > Add a repo argument to it to make the function more general, and adjust
> > the current callers in cmd_fetch and update-clone.
> > 
> > As a proof of the utility of the change, start using the function also
> > in repo_read_gitmodules which was basically doing the same operations.
> > 
> > Signed-off-by: Antonio Ospite <ao2@ao2.it>
> > ---
> >  builtin/fetch.c             |  2 +-
> >  builtin/submodule--helper.c |  2 +-
> >  config.c                    | 13 +++++++------
> >  config.h                    | 10 +---------
> >  submodule-config.c          | 16 ++++------------
> >  5 files changed, 14 insertions(+), 29 deletions(-)
> > 
> > diff --git a/builtin/fetch.c b/builtin/fetch.c
> > index 7ee83ac0f..a67ee7c39 100644
> > --- a/builtin/fetch.c
> > +++ b/builtin/fetch.c
> > @@ -1445,7 +1445,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
> >  	for (i = 1; i < argc; i++)
> >  		strbuf_addf(&default_rla, " %s", argv[i]);
> >  
> > -	config_from_gitmodules(gitmodules_fetch_config, NULL);
> > +	config_from_gitmodules(gitmodules_fetch_config, the_repository, NULL);
> >  	git_config(git_fetch_config, NULL);
> >  
> >  	argc = parse_options(argc, argv, prefix,
> > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> > index c2403a915..9e8f2acd5 100644
> > --- a/builtin/submodule--helper.c
> > +++ b/builtin/submodule--helper.c
> > @@ -1602,7 +1602,7 @@ static int update_clone(int argc, const char **argv, const char *prefix)
> >  	};
> >  	suc.prefix = prefix;
> >  
> > -	config_from_gitmodules(gitmodules_update_clone_config, &max_jobs);
> > +	config_from_gitmodules(gitmodules_update_clone_config, the_repository, &max_jobs);
> >  	git_config(gitmodules_update_clone_config, &max_jobs);
> >  
> >  	argc = parse_options(argc, argv, prefix, module_update_clone_options,
> > diff --git a/config.c b/config.c
> > index 6f8f1d8c1..8ffe29330 100644
> > --- a/config.c
> > +++ b/config.c
> > @@ -2173,17 +2173,18 @@ int git_config_get_pathname(const char *key, const char **dest)
> >  }
> >  
> >  /*
> > - * Note: This function exists solely to maintain backward compatibility with
> > - * 'fetch' and 'update_clone' storing configuration in '.gitmodules' and should
> > - * NOT be used anywhere else.
> > + * Note: Initially this function existed solely to maintain backward
> > + * compatibility with 'fetch' and 'update_clone' storing configuration in
> > + * '.gitmodules' but it turns out it can be useful as a centralized point to
> > + * read the gitmodules config file.
> 
> I'm all for what you're trying to accomplish in this patch series but I
> think a little more care needs to be taken here.  Maybe about a year ago
> I did some refactoring with how the gitmodules file was loaded and it
> was decided that allowing arbitrary configuration in the .gitmodules
> file was a bad thing, so I tried to make sure that it was very difficult
> to sneak in more of that and limiting it to the places where it was
> already done (fetch and update_clone).  Now this patch is explicitly
> changing the comment on this function to loosen the requirements I made
> when it was introduced, which could be problematic in the future.
>

Yes, it was a little inconsiderate of me, sorry.

> So here's what I suggest doing:
>   1. Move this function from config.{c,h} to submodule-config.{c,h} to
>      make it clear who owns this.
>   2. Move the gitmodules_set function you created to live in submodule-config.
>   3. You can still use this function as the main driver for reading the
>      submodule config BUT the comment above the function in both the .c and
>      .h files should be adapted so that it is clearly spells out that the
>      function is to be used only by the submodule config code (reading it
>      in repo_read_gitmodules and reading/writing it in the
>      submodule-helper config function you've added) and that the only
>      exceptions to this are to maintain backwards compatibility with the
>      existing callers and that new callers shouldn't be added.
>

I fully agree, I am going to send a new RFC soon.

> This is just a long way to say that if new callers to this function are
> added in the future that it is made very clear in the code that the
> .gitmodules file exists for a specific purpose and that it shouldn't be
> exploited to ship config with a repository. (If we end up allowing
> config to be shipped with a repository at a later date that will need to
> be handled with great care due to security concerns).
>

Got it.

> Thanks for working on this, the end result is definitely a step in the
> direction I've wanted the submodule config to head to.
>

Thank you for the review.

Ciao,
   Antonio

> >   *
> >   * Runs the provided config function on the '.gitmodules' file found in the
> >   * working directory.
> >   */
> > -void config_from_gitmodules(config_fn_t fn, void *data)
> > +void config_from_gitmodules(config_fn_t fn, struct repository *repo, void *data)
> >  {
> > -	if (the_repository->worktree) {
> > -		char *file = repo_worktree_path(the_repository, GITMODULES_FILE);
> > +	if (repo->worktree) {
> > +		char *file = repo_worktree_path(repo, GITMODULES_FILE);
> >  		git_config_from_file(fn, file, data);
> >  		free(file);
> >  	}
> > diff --git a/config.h b/config.h
> > index cdac2fc73..43ce76c0f 100644
> > --- a/config.h
> > +++ b/config.h
> > @@ -215,15 +215,7 @@ extern int repo_config_get_maybe_bool(struct repository *repo,
> >  extern int repo_config_get_pathname(struct repository *repo,
> >  				    const char *key, const char **dest);
> >  
> > -/*
> > - * Note: This function exists solely to maintain backward compatibility with
> > - * 'fetch' and 'update_clone' storing configuration in '.gitmodules' and should
> > - * NOT be used anywhere else.
> > - *
> > - * Runs the provided config function on the '.gitmodules' file found in the
> > - * working directory.
> > - */
> > -extern void config_from_gitmodules(config_fn_t fn, void *data);
> > +extern void config_from_gitmodules(config_fn_t fn, struct repository *repo, void *data);
> >  
> >  extern int git_config_get_value(const char *key, const char **value);
> >  extern const struct string_list *git_config_get_value_multi(const char *key);
> > diff --git a/submodule-config.c b/submodule-config.c
> > index d87c3ff63..f39c71dfb 100644
> > --- a/submodule-config.c
> > +++ b/submodule-config.c
> > @@ -577,19 +577,11 @@ void repo_read_gitmodules(struct repository *repo)
> >  {
> >  	submodule_cache_check_init(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);
> > +	if (repo_read_index(repo) < 0)
> > +		return;
> >  
> > -		free(gitmodules);
> > -	}
> > +	if (!is_gitmodules_unmerged(repo->index))
> > +		config_from_gitmodules(gitmodules_cb, repo, repo);
> >  
> >  	repo->submodule_cache->gitmodules_read = 1;
> >  }
> > -- 
> > 2.17.0
> > 
> 
> -- 
> Brandon Williams


-- 
Antonio Ospite
https://ao2.it
https://twitter.com/ao2it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

Re: [RFC PATCH 01/10] config: make config_from_gitmodules generally useful

[Antonio Ospite]

On Mon, 14 May 2018 18:05:19 -0700
Stefan Beller <sbeller@google.com> wrote:

Hi again Stefan,

> On Mon, May 14, 2018 at 3:58 AM, Antonio Ospite <ao2@ao2.it> wrote:
> > The config_from_gitmodules() function is a good candidate for
> > a centralized point where to read the gitmodules configuration file.
> 
> It is very tempting to use that function. However it was introduced
> specifically to not do that. ;)
> 
> See the series that was merged at 5aa0b6c506c (Merge branch
> 'bw/grep-recurse-submodules', 2017-08-22), specifically
> f20e7c1ea24 (submodule: remove submodule.fetchjobs from
> submodule-config parsing, 2017-08-02), where both
> builtin/fetch as well as the submodule helper use the pattern to
> read from the .gitmodules file va this function and then overlay it
> with the read from config.
>

I get that the _content_ of .gitmodules is not meant to be generic
config, but I still see some value in having a single point when its
_location_ is decided.

> > Add a repo argument to it to make the function more general, and adjust
> > the current callers in cmd_fetch and update-clone.
> 
> This could be a preparatory patch, but including it here is fine, too.
>

I agree, having this as a preparatory change will help focusing the
review, thanks.

> > As a proof of the utility of the change, start using the function also
> > in repo_read_gitmodules which was basically doing the same operations.
> 
> I think they were separated for the reason outlined above, or what Brandon
> said in his reply.
>
> I think extending 'repo_read_gitmodules' makes sense, as that is
> used everywhere for the loading of submodule configuration.

I would follow Brandon's suggestion here and still use
'config_from_gitmodules' from 'repo_read_gitmodules' to avoid code
duplication.

I will try to be clear in the comments and in commit message when
motivating the decision.

Thanks for the review Stefan.

Ciao,
   Antonio

-- 
Antonio Ospite
https://ao2.it
https://twitter.com/ao2it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

Re: [RFC PATCH 02/10] submodule: factor out a config_gitmodules_set function

[Antonio Ospite]

On Mon, 14 May 2018 18:20:21 -0700
Stefan Beller <sbeller@google.com> wrote:

> On Mon, May 14, 2018 at 3:58 AM, Antonio Ospite <ao2@ao2.it> wrote:
> > Introduce a new config_gitmodules_set function to write config values to the
> > .gitmodules file.
> >
> > This is in preparation for a future change which will use the function
> > to write to the .gitmodules file in a more controlled way instead of
> > using "git config -f .gitmodules".
> >
> > Signed-off-by: Antonio Ospite <ao2@ao2.it>
> > ---
> >
> > Not sure about the name, and maybe it can go in config.c for symmetry with
> > config_from_gitmodules?
> 
> What is the function about (in the end state and now) ?
> is it more of a
> * configure_submodules_config()
>   which would convey it is a generic function to configure submodules
>   (i.e. it may not even write to *the* .gitmodules file but somewhere else,
>   such as a helper ref)
>   This doesn't sound like it as we make use of the function in
>   update_path_in_gitmodules() that is used from git-mv, which would want
>   to ensure that the specific .gitmodules file is changed.
> * gitmodules_file_set()
>   that focuses on the specific file that we want to modify?
> * ...
> 
> Let's continue reading the series to see the end state for
> a good name.
> 

OK, that helped to clarify one point: eventually there will be some
asymmetry between reading the submodule config and writing it.

1. reading will be either form .gitmodules or HEAD:.gitmodules;
2. writing will be only to .gitmodules.

I'll try to consider that when naming the functions.

If a more generic mechanism to write the submodules configuration is
added in the future (e.g. the special ref you were mentioning) the
functions can always be renamed accordingly.

Thanks,
   Antonio

-- 
Antonio Ospite
https://ao2.it
https://twitter.com/ao2it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

Re: [RFC PATCH 01/10] config: make config_from_gitmodules generally useful

[Stefan Beller]

Hi Antonio!

On Wed, Jun 20, 2018 at 11:06 AM Antonio Ospite <ao2@ao2.it> wrote:
> I get that the _content_ of .gitmodules is not meant to be generic
> config, but I still see some value in having a single point when its
> _location_ is decided.

I agree that a single point for the _location_ as well as the _order_
(in case there will be multiple files; as of now we have the layering
of .gitmodules overlayed by .git/config; IIRC I explained having
an intermediate layer in our conversation to be useful; See one of the latest
bug reports[1], where an intermediate layer outside a single branch would
prove to be useful.) parsing are useful.

[1] https://public-inbox.org/git/DB6PR0101MB2344D682511891E4E9528598D97D0@DB6PR0101MB2344.eurprd01.prod.exchangelabs.com/

Sorry for not explaining my point of view well enough, let me try again:

Historically we did not store any config in the repository itself.
There are some exceptions, but these configurations are content related,
i.e. .gitmodules or .gitattributes can tell you meta data about the
content itself.

However other config was kept out: One could have a .gitconfig that
pre-populates the .git/config file, right? That was intentionally avoided
as there are many configurations that are easy to abuse security wise,
e.g. setting core.pager = "rm -rf /"

And then submodules entered the big picture. Submodules need quite
a lot of configuration, and hence the .gitmodules file was born. Initially
IIRC there was only a very simple config like url store:

  [submodule.<path>]
    url = <url>

and that was it as it was just like the .gitignore and .gitattributes
related to the content and transporting this configuration with the
content itself seemed so natural.

However then a lot of settings were invented for submodules and some
made it into the .gitmodules file; only recently there was a discussion
on list how these settings may or may not pose a security issue. It turns out
we had a CVE (with sumodule names) and that got fixed but we really want
to keep the .gitmodules file simple and ignore any additional things in there
as they may pose security issues later.

With that said, how would you write the code while keeping this in mind?
If you look at builtin/submodule--helper.c and builtin/fetch.c, you'll see that
they use

  config_from_gitmodules(their_parse_function);
  git_config(their_parse_function);

and config_from_gitmodules is documented to not be expanded;
the config_from_gitmodules is singled out to treat those settings
that snuck in and are kept around as we don't want to break existing
users. But we'd really not want to expand the use of that function
again for its implications on security. Right now it is nailed down beautifully
and it is easy to tell which commands actually look at config from
the .gitmodules file (not to be confused with the submodule parsing,
that is in submodule-config.{h, c}. That is actually about finding
submodule specific name/path/url etc instead of the generic
"submodule.fetchjobs" or "fetch.recursesubmodules".

----
So far about the background story. I would rather replicate the code in
repo_read_gitmodules in the submodule-config.c as to mix those
two lines (reading generic config vs. reading submodule specific config,
like name/url/path). And to not mix them, I would not reuse that function
but rather replicate (or have a static helper) in submodule helper,
as then we cannot pass in an arbitrary config parsing callback to
that, but are bound to the submodule helper code.

> > I think extending 'repo_read_gitmodules' makes sense, as that is
> > used everywhere for the loading of submodule configuration.
>
> I would follow Brandon's suggestion here and still use
> 'config_from_gitmodules' from 'repo_read_gitmodules' to avoid code
> duplication.
>
> I will try to be clear in the comments and in commit message when
> motivating the decision.

Rereading what Brandon said, I agree with this approach, sorry for writing
out the story above in such lengthy detail.

Thanks for picking up this series again!
Stefan

Re: [RFC PATCH 03/10] t7411: be nicer to other tests and really clean things up

[Antonio Ospite]

On Mon, 14 May 2018 18:23:22 -0700
Stefan Beller <sbeller@google.com> wrote:

> On Mon, May 14, 2018 at 3:58 AM, Antonio Ospite <ao2@ao2.it> wrote:
> > Tests 5 and 8 in t/t7411-submodule-config.sh add two commits with
> > invalid lines in .gitmodules but then only the second commit is removed.
> >
> > This may affect subsequent tests if they assume that the .gitmodules
> > file has no errors.
> >
> > Since those commits are not needed anymore remove both of them.
> >
> > Signed-off-by: Antonio Ospite <ao2@ao2.it>
> > ---
> >
> > I am putting these fixups to the test-suite before the patch that actually
> > needs them so that the test-suite passes after each commit.
> >
> >  t/t7411-submodule-config.sh | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
> > index 0bde5850a..a648de6a9 100755
> > --- a/t/t7411-submodule-config.sh
> > +++ b/t/t7411-submodule-config.sh
> > @@ -135,7 +135,7 @@ test_expect_success 'error in history in fetchrecursesubmodule lets continue' '
> >                         HEAD submodule \
> >                                 >actual &&
> >                 test_cmp expect_error actual  &&
> > -               git reset --hard HEAD^
> > +               git reset --hard HEAD~2
> >         )
> >  '
> 
> As this is the last test in this file, we do not change any subsequent
> tests in a subtle way.
> Good!
> 
> This is
> Reviewed-by: Stefan Beller <sbeller@google.com>
> 
> FYI:
> This test -- of course -- doesn't quite follow the latest coding guidelines,
> as usually we'd prefer a test_when_finished "<cmd to restore>"
> at the beginning of a test.

I'll keep that in mind for new tests, trying to remember that
'test_when_finished' does not work in a subshell.

BTW I can use 'test_when_finished' here as well, maybe adding a comment
to clarify the the command also cleans up something from a previous
test.

Thanks,
   Antonio

-- 
Antonio Ospite
https://ao2.it
https://twitter.com/ao2it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

Re: [RFC PATCH 04/10] submodule--helper: add a new 'config' subcommand

[Antonio Ospite]

On Mon, 14 May 2018 18:33:44 -0700
Stefan Beller <sbeller@google.com> wrote:

> On Mon, May 14, 2018 at 3:58 AM, Antonio Ospite <ao2@ao2.it> wrote:
> > Add a new 'config' subcommand to 'submodule--helper', this extra level
> > of indirection makes it possible to add some flexibility to how the
> > submodules configuration is handled.
> >
> > Signed-off-by: Antonio Ospite <ao2@ao2.it>
> > ---
> >  builtin/submodule--helper.c | 39 +++++++++++++++++++++++++++++++++++++
> >  t/t7411-submodule-config.sh | 26 +++++++++++++++++++++++++
> >  2 files changed, 65 insertions(+)
> >
> > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> > index 9e8f2acd5..b32110e3b 100644
> > --- a/builtin/submodule--helper.c
> > +++ b/builtin/submodule--helper.c
> > @@ -1825,6 +1825,44 @@ static int is_active(int argc, const char **argv, const char *prefix)
> >         return !is_submodule_active(the_repository, argv[1]);
> >  }
> >
> > +static int config_print_callback(const char *key_, const char *value_, void *cb_data)
> > +{
> > +       char *key = cb_data;
> > +
> > +       if (!strcmp(key, key_))
> > +               printf("%s\n", value_);
> > +
> > +       return 0;
> > +}
> > +
> > +static int module_config(int argc, const char **argv, const char *prefix)
> > +{
> > +       int ret;
> > +
> > +       if (argc < 2 || argc > 3)
> > +               die("submodule--helper config takes 1 or 2 arguments: name [value]");
> > +
> > +       /* Equivalent to ACTION_GET in builtin/config.c */
> > +       if (argc == 2) {
> > +               char *key;
> > +
> > +               ret = git_config_parse_key(argv[1], &key, NULL);
> > +               if (ret < 0)
> > +                       return CONFIG_INVALID_KEY;
> > +
> > +               config_from_gitmodules(config_print_callback, the_repository, key);
> > +
> > +               free(key);
> > +               return 0;
> > +       }
> > +
> > +       /* Equivalent to ACTION_SET in builtin/config.c */
> > +       if (argc == 3)
> > +               return config_gitmodules_set(argv[1], argv[2]);
> 
> Ah, here we definitely want to set it in the .gitmodules file?
> (Or does that change later in this series?)
> 
> > +
> > +       return 0;
> > +}
> > +
> >  #define SUPPORT_SUPER_PREFIX (1<<0)
> >
> >  struct cmd_struct {
> > @@ -1850,6 +1888,7 @@ static struct cmd_struct commands[] = {
> >         {"push-check", push_check, 0},
> >         {"absorb-git-dirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
> >         {"is-active", is_active, 0},
> > +       {"config", module_config, 0},
> >  };
> >
> >  int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
> > diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
> > index a648de6a9..dfe019f05 100755
> > --- a/t/t7411-submodule-config.sh
> > +++ b/t/t7411-submodule-config.sh
> > @@ -139,4 +139,30 @@ test_expect_success 'error in history in fetchrecursesubmodule lets continue' '
> >         )
> >  '
> >
> > +test_expect_success 'reading submodules config with "submodule--helper config"' '
> > +       (cd super &&
> 
> I think the project prefers a style
> of the cd at the same level of the echo and the following commands.
>

There is mixed style about that, so for new tests in existing files I'd
stick to the predominant style in the file.

For new test files I'll use the recommended style of cd on the same
level of the following commands.

> However we might not need the (cd super && ...) via
> 
>   echo "../submodule"  >expected
>   git -C super ubmodule--helper config submodule.submodule.url >../actual
>   test_cmp expected actual
> 
> Our friends developing git on Windows will thank us for saving
> to spawn a shell as spawning processes is expensive on Windows. :)
> Also we have fewer lines of code.
>

I'll see how that looks, thanks for the suggestion.

Again, I'd use a subshell if that's what the other tests in a file do,
and use "git -C" in new files.

Thanks,
   Antonio

-- 
Antonio Ospite
https://ao2.it
https://twitter.com/ao2it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

Re: [RFC PATCH 01/10] config: make config_from_gitmodules generally useful

[Antonio Ospite]

On Wed, 20 Jun 2018 12:10:42 -0700
Stefan Beller <sbeller@google.com> wrote:

> Hi Antonio!
>
> On Wed, Jun 20, 2018 at 11:06 AM Antonio Ospite <ao2@ao2.it> wrote:
> > I get that the _content_ of .gitmodules is not meant to be generic
> > config, but I still see some value in having a single point when its
> > _location_ is decided.
> 
> I agree that a single point for the _location_ as well as the _order_
> (in case there will be multiple files; as of now we have the layering
> of .gitmodules overlayed by .git/config; IIRC I explained having
> an intermediate layer in our conversation to be useful; See one of the latest
> bug reports[1], where an intermediate layer outside a single branch would
> prove to be useful.) parsing are useful.
> 
> [1] https://public-inbox.org/git/DB6PR0101MB2344D682511891E4E9528598D97D0@DB6PR0101MB2344.eurprd01.prod.exchangelabs.com/
> 
> Sorry for not explaining my point of view well enough, let me try again:
> 
> Historically we did not store any config in the repository itself.
> There are some exceptions, but these configurations are content related,
> i.e. .gitmodules or .gitattributes can tell you meta data about the
> content itself.
> 
> However other config was kept out: One could have a .gitconfig that
> pre-populates the .git/config file, right? That was intentionally avoided
> as there are many configurations that are easy to abuse security wise,
> e.g. setting core.pager = "rm -rf /"
> 
> And then submodules entered the big picture. Submodules need quite
> a lot of configuration, and hence the .gitmodules file was born. Initially
> IIRC there was only a very simple config like url store:
> 
>   [submodule.<path>]
>     url = <url>
> 
> and that was it as it was just like the .gitignore and .gitattributes
> related to the content and transporting this configuration with the
> content itself seemed so natural.
> 
> However then a lot of settings were invented for submodules and some
> made it into the .gitmodules file; only recently there was a discussion
> on list how these settings may or may not pose a security issue. It turns out
> we had a CVE (with sumodule names) and that got fixed but we really want
> to keep the .gitmodules file simple and ignore any additional things in there
> as they may pose security issues later.
> 
> With that said, how would you write the code while keeping this in mind?
> If you look at builtin/submodule--helper.c and builtin/fetch.c, you'll see that
> they use
> 
>   config_from_gitmodules(their_parse_function);
>   git_config(their_parse_function);
> 
> and config_from_gitmodules is documented to not be expanded;
> the config_from_gitmodules is singled out to treat those settings
> that snuck in and are kept around as we don't want to break existing
> users. But we'd really not want to expand the use of that function
> again for its implications on security. Right now it is nailed down beautifully
> and it is easy to tell which commands actually look at config from
> the .gitmodules file (not to be confused with the submodule parsing,
> that is in submodule-config.{h, c}. That is actually about finding
> submodule specific name/path/url etc instead of the generic
> "submodule.fetchjobs" or "fetch.recursesubmodules".
> 
> ----
> So far about the background story. I would rather replicate the code in
> repo_read_gitmodules in the submodule-config.c as to mix those
> two lines (reading generic config vs. reading submodule specific config,
> like name/url/path). And to not mix them, I would not reuse that function
> but rather replicate (or have a static helper) in submodule helper,
> as then we cannot pass in an arbitrary config parsing callback to
> that, but are bound to the submodule helper code.
>

OK, the fact I was overlooking was that the "config_fn_t" argument
passed to config_from_gitmodules is what we are actually worried about,
it's the config callback which could allow generic config in .gitmodules
to sneak in. I still have some blind spots on git internals, sorry.

So, from Brandon's message I derive that using the gitmodules_cb from
submodules-config.c as a callback would be safe, as that's what
repo_read_gitmodules already uses anyways, and it only allows submodules
configuration.

However, what about avoiding exposing the dangerous interface altogether
and adding ad-hoc helpers when needed?

I mean:

  0. Move config_from_gitmodules to submodule-config.c as per Bradon's
     suggestion.

  1. Add public helpers in submodule-config.[ch] to handle backwards
     compatibility, like: fetch_config_from_gitmodules() and
     update_clone_config_from_gitmodules() these would be used by fetch
     and update-clone function and would not accept callbacks.

     This would mean moving the callback functions
     gitmodules_fetch_config() and gitmodules_update_clone_config() into
     submodule-config.c and making them private. The helpers will call
     config_from_gitmodules() with them.

  2. Now that config_from_gitmodules it's not used in the open it can be
     made private too, so it can only be used in submodule-config.c

  3. Use config_from_gitmodules in repo_read_gitmodules as the
     gitmodules_cb function should be safe to use as a config callback.

  4. Add a new gitmodules_get_value() helper in submodule-config.c which
     calls config_from_gitmodules with a "print" callback and use that
     helper for "submodule--helper config",

  5. At this point we shouldn't worry too much about the .gitmodules
     content anymore, and we can possibly extend config_from_gitmodules
     to read from other locations like HEAD:.gitmodules.

This way the number of users of config_from_gitmodules remains strictly
controlled and confined in submodule-config.c

I know, we could end up adding more code with the helpers but that could
be justified by the more protective approach: we would be using symbols
scoping rules instead of comments to ensure something.

If you think this may be worth a shot I can send a series which covers
items from 0 to 3.

Ciao,
   Antonio

P.S. Always relevant: https://youtu.be/8fnfeuoh4s8 (I swear it's not
Rick Astley)

> > > I think extending 'repo_read_gitmodules' makes sense, as that is
> > > used everywhere for the loading of submodule configuration.
> >
> > I would follow Brandon's suggestion here and still use
> > 'config_from_gitmodules' from 'repo_read_gitmodules' to avoid code
> > duplication.
> >
> > I will try to be clear in the comments and in commit message when
> > motivating the decision.
> 
> Rereading what Brandon said, I agree with this approach, sorry for writing
> out the story above in such lengthy detail.
> 
> Thanks for picking up this series again!
> Stefan


-- 
Antonio Ospite
https://ao2.it
https://twitter.com/ao2it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

Re: [RFC PATCH 01/10] config: make config_from_gitmodules generally useful

[Stefan Beller]

> OK, the fact I was overlooking was that the "config_fn_t" argument
> passed to config_from_gitmodules is what we are actually worried about,
> it's the config callback which could allow generic config in .gitmodules
> to sneak in.

That is the precise point that I was trying to communicate. :)

>
> So, from Brandon's message I derive that using the gitmodules_cb from
> submodules-config.c as a callback would be safe, as that's what
> repo_read_gitmodules already uses anyways, and it only allows submodules
> configuration.

I agree.

>
> However, what about avoiding exposing the dangerous interface altogether
> and adding ad-hoc helpers when needed?
>
> I mean:
>
>   0. Move config_from_gitmodules to submodule-config.c as per Bradon's
>      suggestion.

That sounds good to me.

>   1. Add public helpers in submodule-config.[ch] to handle backwards
>      compatibility, like: fetch_config_from_gitmodules() and
>      update_clone_config_from_gitmodules() these would be used by fetch
>      and update-clone function and would not accept callbacks.
>
>      This would mean moving the callback functions
>      gitmodules_fetch_config() and gitmodules_update_clone_config() into
>      submodule-config.c and making them private. The helpers will call
>      config_from_gitmodules() with them.
>
>   2. Now that config_from_gitmodules it's not used in the open it can be
>      made private too, so it can only be used in submodule-config.c
>
>   3. Use config_from_gitmodules in repo_read_gitmodules as the
>      gitmodules_cb function should be safe to use as a config callback.

That sounds all good to me.

>
>   4. Add a new gitmodules_get_value() helper in submodule-config.c which
>      calls config_from_gitmodules with a "print" callback and use that
>      helper for "submodule--helper config",
>
>   5. At this point we shouldn't worry too much about the .gitmodules
>      content anymore, and we can possibly extend config_from_gitmodules
>      to read from other locations like HEAD:.gitmodules.

These two would actually align with your goal of the original series. :)

>
> This way the number of users of config_from_gitmodules remains strictly
> controlled and confined in submodule-config.c
>
> I know, we could end up adding more code with the helpers but that could
> be justified by the more protective approach: we would be using symbols
> scoping rules instead of comments to ensure something.
>
> If you think this may be worth a shot I can send a series which covers
> items from 0 to 3.

That would be great!

Thanks,
Stefan

>
> Ciao,
>    Antonio
>
> P.S. Always relevant: https://youtu.be/8fnfeuoh4s8 (I swear it's not
> Rick Astley)

heh!