threads / patch / 49490

v6, 10 partst7411: merge tests 5 and 6

Subject: [PATCH v6 03/10] t7411: merge tests 5 and 6

## tl;dr

23 messages between Oct 5, 2018 and Oct 25, 2018. Diffs are folded; open one to read it.

replies: 22people: 3as markdown or json

Antonio Ospite· Oct 5, 2018, 13:05 UTC · lore

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

Hi,

this series teaches git to try and read the .gitmodules file from the index (:.gitmodules) or from the current branch (HEAD:.gitmodules) when the file is not readily available in the working tree.

This can be used, together with sparse checkouts, to enable submodule usage with programs like vcsh[1] which manage multiple repositories with their working trees sharing the same path.

[1] https://github.com/RichiH/vcsh

Thanks to SZEDER Gábor we found out that the changes in patch 9 could allow to access the object store in an inconsistent way when using multi-threading in "git grep --recurse-submodules", this has been dealt with in this revision.

BTW, with Stefan Beller we also identified some unneeded code which could have been removed to alleviate the issue, but that would not have solved it completely; so, I am not removing the unnecessary call to repo_read_gitmodules() builtin/grep.c in this series, possibly this can become a stand-alone change.

The problems from above also made me investigate what implications the current use of a global object store had on my new addition, and now I know that there is one case which I cannot support just yet: nested submodules without .gitmodules in their working tree.

This case has been documented with a warning and test_expect_failure items in tests, and hitting the unsupported case does not alter the current behavior of git.

Apart form patch 9 and 10 there are no major changes to the previous iteration.

IMHO we are in a place where the problem has been analyzed with enough depth, the limitations have been identified and dealt with in a way that should not affect current users nor diminish the usefulness of the new code.

v5 of the series is here: https://public-inbox.org/git/20180917140940.3839-1-ao2@ao2.it/

v4 of the series is here: https://public-inbox.org/git/20180824132951.8000-1-ao2@ao2.it/

v3 of the series is here: https://public-inbox.org/git/20180814110525.17801-1-ao2@ao2.it/

v2 of the series is here: https://public-inbox.org/git/20180802134634.10300-1-ao2@ao2.it/

v1 of the series, with some background info, is here: https://public-inbox.org/git/20180514105823.8378-1-ao2@ao2.it/

Changes since v5:
  * print_config_from_gitmodules() in patch 1 now accepts a struct
    repository argument.
  * submodule--helper config in patch 5 has been adjusted to use the new
    signature of print_config_from_gitmodules().
  * In patch 9 the grep read lock in builtin/grep.c now covers all code
    paths involving config_from_gitmodules().
    
    FTR git-grep is the only place where config_from_gitmodules() is
    called from multi-threaded code.
  * Patch 9 also documents the rare case that cannot be supported just
    yet, and adds a warning to the user.
  * In patch 9, config_from_gitmodules() now does not read any config
    when the config source is not specified.(I added a catch-all "else"
    block) This match more closely the behavior of the old code using
    git_config_from_file.
  * Added a new test tool in patch 10 to exercise config_read_config()
    in a more direct way, passing an arbitrary repository.
    
    Admittedly, patch 10 performs a similar test to the one added to
    t7814 in patch 9, so I'd be OK with dropping patch 10 if it is too
    specific.
Thank you,
   Antonio
Antonio Ospite (10):
  submodule: add a print_config_from_gitmodules() helper
  submodule: factor out a config_set_in_gitmodules_file_gently function
  t7411: merge tests 5 and 6
  t7411: be nicer to future tests and really clean things up
  submodule--helper: add a new 'config' subcommand
  submodule: use the 'submodule--helper config' command
  t7506: clean up .gitmodules properly before setting up new scenario
  submodule: add a helper to check if it is safe to write to .gitmodules
  submodule: support reading .gitmodules when it's not in the working
    tree
  t/helper: add test-submodule-nested-repo-config
 Makefile                                     |   1 +
 builtin/grep.c                               |  17 ++-
 builtin/submodule--helper.c                  |  40 ++++++
 cache.h                                      |   2 +
 git-submodule.sh                             |  13 +-
 submodule-config.c                           |  68 ++++++++-
 submodule-config.h                           |   2 +
 submodule.c                                  |  28 +++-
 submodule.h                                  |   1 +
 t/helper/test-submodule-nested-repo-config.c |  30 ++++
 t/helper/test-tool.c                         |   1 +
 t/helper/test-tool.h                         |   1 +
 t/t7411-submodule-config.sh                  | 141 +++++++++++++++++--
 t/t7416-submodule-sparse-gitmodules.sh       |  78 ++++++++++
 t/t7506-status-submodule.sh                  |   3 +-
 t/t7814-grep-recurse-submodules.sh           |  16 +++
 16 files changed, 410 insertions(+), 32 deletions(-)
 create mode 100644 t/helper/test-submodule-nested-repo-config.c
 create mode 100755 t/t7416-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?
Antonio Ospite· Oct 5, 2018, 13:05 UTC · re: Antonio Ospite · lore

Tests 5 and 6 check for the effects of the same commit, merge the two tests to make it more straightforward to clean things up after the test has finished.

The cleanup will be added in a future commit.
Signed-off-by: Antonio Ospite <ao2@ao2.it>
---
 t/t7411-submodule-config.sh | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)
Show changes to t/t7411-submodule-config.sh +5 −13
diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
index 0bde5850ac..f2cd1f4a2c 100755
--- a/t/t7411-submodule-config.sh
+++ b/t/t7411-submodule-config.sh
@@ -82,29 +82,21 @@ Submodule name: 'a' for path 'b'
 Submodule name: 'submodule' for path 'submodule'
 EOF
 
-test_expect_success 'error in one submodule config lets continue' '
+test_expect_success 'error in history of one submodule config lets continue, stderr message contains blob ref' '
 	(cd super &&
 		cp .gitmodules .gitmodules.bak &&
 		echo "	value = \"" >>.gitmodules &&
 		git add .gitmodules &&
 		mv .gitmodules.bak .gitmodules &&
 		git commit -m "add error" &&
-		test-tool submodule-config \
-			HEAD b \
-			HEAD submodule \
-				>actual &&
-		test_cmp expect_error actual
-	)
-'
-
-test_expect_success 'error message contains blob reference' '
-	(cd super &&
 		sha1=$(git rev-parse HEAD) &&
 		test-tool submodule-config \
 			HEAD b \
 			HEAD submodule \
-				2>actual_err &&
-		test_i18ngrep "submodule-blob $sha1:.gitmodules" actual_err >/dev/null
+				>actual \
+				2>actual_stderr &&
+		test_cmp expect_error actual &&
+		test_i18ngrep "submodule-blob $sha1:.gitmodules" actual_stderr >/dev/null
 	)
 '
 

-- 
2.19.0
Antonio Ospite· Oct 5, 2018, 13:06 UTC · re: Antonio Ospite · lore

[PATCH v6 09/10] submodule: support reading .gitmodules when it's not in the working tree

When the .gitmodules file is not available in the working tree, try using the content from the index and from the current branch. 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 working tree.

Writing to .gitmodules will still require that the file is checked out, so check for that before calling config_set_in_gitmodules_file_gently.

Add a similar check also in git-submodule.sh::cmd_add() to anticipate the eventual failure of the "git submodule add" command when .gitmodules is not safely writeable; this prevents the command from leaving the repository in a spurious state (e.g. the submodule repository was cloned but .gitmodules was not updated because config_set_in_gitmodules_file_gently failed).

Moreover, since config_from_gitmodules() now accesses the global object store, it is necessary to protect all code paths which call the function against concurrent access to the global object store. Currently this only happens in builtin/grep.c::grep_submodules(), so call grep_read_lock() before invoking code involving config_from_gitmodules().

Finally, add t7416-submodule-sparse-gitmodules.sh to verify that reading from .gitmodules succeeds and that writing to it fails when the file is not checked out.

NOTE: there is one rare case where this new feature does not work
properly yet: nested submodules without .gitmodules in their working
tree.  This has been documented with a warning and a test_expect_failure
item in t7814, and in this case the current behavior is not altered: no
config is read.
Signed-off-by: Antonio Ospite <ao2@ao2.it>
---
 builtin/grep.c                         | 17 +++++-
 builtin/submodule--helper.c            |  6 +-
 git-submodule.sh                       |  5 ++
 submodule-config.c                     | 31 +++++++++-
 t/t7411-submodule-config.sh            | 26 ++++++++-
 t/t7416-submodule-sparse-gitmodules.sh | 78 ++++++++++++++++++++++++++
 t/t7814-grep-recurse-submodules.sh     | 16 ++++++
 7 files changed, 172 insertions(+), 7 deletions(-)
 create mode 100755 t/t7416-submodule-sparse-gitmodules.sh
Show changes to 7 files +172 −7

builtin/grep.c, builtin/submodule--helper.c, git-submodule.sh, submodule-config.c, t/t7411-submodule-config.sh, t/t7416-submodule-sparse-gitmodules.sh, t/t7814-grep-recurse-submodules.sh

diff --git a/builtin/grep.c b/builtin/grep.c
index 601f801158..7da8fef31a 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -421,11 +421,23 @@ static int grep_submodule(struct grep_opt *opt, struct repository *superproject,
 	struct repository submodule;
 	int hit;
 
-	if (!is_submodule_active(superproject, path))
+	/*
+	 * NEEDSWORK: submodules functions need to be protected because they
+	 * access the object store via config_from_gitmodules(): the latter
+	 * uses get_oid() which, for now, relies on the global the_repository
+	 * object.
+	 */
+	grep_read_lock();
+
+	if (!is_submodule_active(superproject, path)) {
+		grep_read_unlock();
 		return 0;
+	}
 
-	if (repo_submodule_init(&submodule, superproject, path))
+	if (repo_submodule_init(&submodule, superproject, path)) {
+		grep_read_unlock();
 		return 0;
+	}
 
 	repo_read_gitmodules(&submodule);
 
@@ -439,7 +451,6 @@ static int grep_submodule(struct grep_opt *opt, struct repository *superproject,
 	 * store is no longer global and instead is a member of the repository
 	 * object.
 	 */
-	grep_read_lock();
 	add_to_alternates_memory(submodule.objects->objectdir);
 	grep_read_unlock();
 
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 28f3ccca6d..5f8a804a6e 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2154,8 +2154,12 @@ static int module_config(int argc, const char **argv, const char *prefix)
 		return print_config_from_gitmodules(the_repository, argv[1]);
 
 	/* Equivalent to ACTION_SET in builtin/config.c */
-	if (argc == 3)
+	if (argc == 3) {
+		if (!is_writing_gitmodules_ok())
+			die(_("please make sure that the .gitmodules file is in the working tree"));
+
 		return config_set_in_gitmodules_file_gently(argv[1], argv[2]);
+	}
 
 	usage_with_options(git_submodule_helper_usage, module_config_options);
 }
diff --git a/git-submodule.sh b/git-submodule.sh
index 0805fadf47..f5124bbf23 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -159,6 +159,11 @@ cmd_add()
 		shift
 	done
 
+	if ! git submodule--helper config --check-writeable >/dev/null 2>&1
+	then
+		 die "$(eval_gettext "please make sure that the .gitmodules file is in the working tree")"
+	fi
+
 	if test -n "$reference_path"
 	then
 		is_absolute_path "$reference_path" ||
diff --git a/submodule-config.c b/submodule-config.c
index 8659d97e06..69bebb721e 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -1,4 +1,5 @@
 #include "cache.h"
+#include "dir.h"
 #include "repository.h"
 #include "config.h"
 #include "submodule-config.h"
@@ -603,8 +604,34 @@ static void submodule_cache_check_init(struct repository *repo)
 static 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 (repo->submodule_prefix) {
+			/*
+			 * When get_oid and config_with_options, used below,
+			 * become able to work on a specific repository, this
+			 * warning branch can be removed.
+			 */
+			warning("nested submodules without %s in the working tree are not supported yet",
+				GITMODULES_FILE);
+			goto out;
+		} else if (get_oid(GITMODULES_INDEX, &oid) >= 0) {
+			config_source.blob = GITMODULES_INDEX;
+		} else if (get_oid(GITMODULES_HEAD, &oid) >= 0) {
+			config_source.blob = GITMODULES_HEAD;
+		} else {
+			goto out;
+		}
+
+		config_with_options(fn, data, &config_source, &opts);
+
+out:
 		free(file);
 	}
 }
diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
index 45953f9300..2cfabb18bc 100755
--- a/t/t7411-submodule-config.sh
+++ b/t/t7411-submodule-config.sh
@@ -134,7 +134,7 @@ test_expect_success 'error in history in fetchrecursesubmodule lets continue' '
 	)
 '
 
-test_expect_success 'reading submodules config with "submodule--helper config"' '
+test_expect_success 'reading submodules config from the working tree with "submodule--helper config"' '
 	(cd super &&
 		echo "../submodule" >expect &&
 		git submodule--helper config submodule.submodule.url >actual &&
@@ -192,4 +192,28 @@ test_expect_success 'non-writeable .gitmodules when it is in the current branch
 	)
 '
 
+test_expect_success 'reading submodules config from the index when .gitmodules is not in the working tree' '
+	ORIG=$(git -C super rev-parse HEAD) &&
+	test_when_finished "git -C super reset --hard $ORIG" &&
+	(cd super &&
+		git submodule--helper config submodule.submodule.url "staged_url" &&
+		git add .gitmodules &&
+		rm -f .gitmodules &&
+		echo "staged_url" >expect &&
+		git submodule--helper config submodule.submodule.url >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'reading submodules config from the current branch when .gitmodules is not in the index' '
+	ORIG=$(git -C super rev-parse HEAD) &&
+	test_when_finished "git -C super reset --hard $ORIG" &&
+	(cd super &&
+		git rm .gitmodules &&
+		echo "../submodule" >expect &&
+		git submodule--helper config submodule.submodule.url >actual &&
+		test_cmp expect actual
+	)
+'
+
 test_done
diff --git a/t/t7416-submodule-sparse-gitmodules.sh b/t/t7416-submodule-sparse-gitmodules.sh
new file mode 100755
index 0000000000..908a4e6958
--- /dev/null
+++ b/t/t7416-submodule-sparse-gitmodules.sh
@@ -0,0 +1,78 @@
+#!/bin/sh
+#
+# Copyright (C) 2018  Antonio Ospite <ao2@ao2.it>
+#
+
+test_description='Test reading/writing .gitmodules when not in the working tree
+
+This test verifies that, when .gitmodules is in the current branch but is not
+in the working tree reading from it still works but writing to it does not.
+
+The test setup uses a sparse checkout, however the same scenario can be set up
+also by committing .gitmodules and then just removing it from the filesystem.
+'
+
+. ./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_path_is_missing .gitmodules
+	)
+'
+
+test_expect_success 'reading gitmodules config file when it is not checked out' '
+	(cd super &&
+		echo "../submodule" >expect &&
+		git submodule--helper config submodule.submodule.url >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'not writing gitmodules config file when it is not checked out' '
+	 test_must_fail git -C super submodule--helper config submodule.submodule.url newurl
+'
+
+test_expect_success 'initialising submodule when the gitmodules config is not checked out' '
+	git -C super submodule init
+'
+
+test_expect_success 'showing submodule summary when the gitmodules config is not checked out' '
+	git -C super 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"
+	) &&
+	git -C super submodule update
+'
+
+test_expect_success 'not adding submodules when the gitmodules config is not checked out' '
+	test_must_fail git -C super submodule add ../new_submodule
+'
+
+# This test checks that the previous "git submodule add" did not leave the
+# repository in a spurious state when it failed.
+test_expect_success 'init submodule still works even after the previous add failed' '
+	git -C super submodule init
+'
+
+test_done
diff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh
index 7184113b9b..fa475d52fa 100755
--- a/t/t7814-grep-recurse-submodules.sh
+++ b/t/t7814-grep-recurse-submodules.sh
@@ -380,4 +380,20 @@ test_expect_success 'grep --recurse-submodules should pass the pattern type alon
 	fi
 '
 
+# Recursing down into nested submodules which do not have .gitmodules in their
+# working tree does not work yet. This is because config_from_gitmodules()
+# uses get_oid() and the latter is still not able to get objects from an
+# arbitrary repository (the nested submodule, in this case).
+test_expect_failure 'grep --recurse-submodules with submodules without .gitmodules in the working tree' '
+	test_when_finished "git -C submodule checkout .gitmodules" &&
+	rm submodule/.gitmodules &&
+	git grep --recurse-submodules -e "(.|.)[\d]" >actual &&
+	cat >expect <<-\EOF &&
+	a:(1|2)d(3|4)
+	submodule/a:(1|2)d(3|4)
+	submodule/sub/a:(1|2)d(3|4)
+	EOF
+	test_cmp expect actual
+'
+
 test_done

-- 
2.19.0
Stefan Beller· Oct 8, 2018, 22:19 UTC · re: Antonio Ospite · lore

Re: [PATCH v6 09/10] submodule: support reading .gitmodules when it's not in the working tree

> +test_expect_success 'not writing gitmodules config file when it is not checked out' '
> +        test_must_fail git -C super submodule--helper config submodule.submodule.url newurl
This only checks the exit code, do we also want to check for
    test_path_is_missing .gitmodules ?
Show 7 quoted lines
> +test_expect_success 'initialising submodule when the gitmodules config is not checked out' '
> +       git -C super submodule init
> +'
> +
> +test_expect_success 'showing submodule summary when the gitmodules config is not checked out' '
> +       git -C super submodule summary
> +'

Same for these, is the exit code enough, or do we want to look at specific things?

Show 8 quoted lines
> +
> +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"
> +       ) &&
> +       git -C super submodule update
git status would want to be clean afterwards?
Antonio Ospite· Oct 10, 2018, 18:56 UTC · re: Stefan Beller · lore

Re: [PATCH v6 09/10] submodule: support reading .gitmodules when it's not in the working tree

On Mon, 8 Oct 2018 15:19:00 -0700 Stefan Beller <sbeller@google.com> wrote:

Show 7 quoted lines
> > +test_expect_success 'not writing gitmodules config file when it is not checked out' '
> > +        test_must_fail git -C super submodule--helper config submodule.submodule.url newurl
> 
> This only checks the exit code, do we also want to check for
> 
>     test_path_is_missing .gitmodules ?
>

OK, I agree, let's re-check also *after* we tried and failed to set a config value, just to be sure that the code does not get accidentally changed in the future to create the file. I'll add the check.

Show 11 quoted lines
> > +test_expect_success 'initialising submodule when the gitmodules config is not checked out' '
> > +       git -C super submodule init
> > +'
> > +
> > +test_expect_success 'showing submodule summary when the gitmodules config is not checked out' '
> > +       git -C super submodule summary
> > +'
> 
> Same for these, is the exit code enough, or do we want to look at
> specific things?
>

Except for the "summary" test which was not even exercising the config_from_gitmodule path, checking exist status should be sufficient to verify that "submodule--helper config" does not fail, but we can surely do better.

I will add checks to confirm that not only the commands exited without errors but they also achieved the desired effect, to validate the actual high-level use case advertised by the test file. This should be more future-proof.

And I think I'll merge the summary and the update tests.
Show 10 quoted lines
> > +
> > +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"
> > +       ) &&
> > +       git -C super submodule update
> 
> git status would want to be clean afterwards?

Mmh, this should have been "submodule update --remote" in the first place to have any effect, I'll take the chance and rewrite this test in a different way and also check the effect of the update operation, and the repository status.

I'll be something like this:

ORIG_SUBMODULE=$(git -C submodule rev-parse HEAD) ORIG_UPSTREAM=$(git -C upstream rev-parse HEAD) ORIG_SUPER=$(git -C super rev-parse HEAD)

test_expect_success 're-updating submodule when the gitmodules config is not checked out' '
	test_when_finished "git -C submodule reset --hard $ORIG_SUBMODULE;
	                    git -C upstream reset --hard $ORIG_UPSTREAM;
	                    git -C super reset --hard $ORIG_SUPER;
	                    git -C upstream submodule update --remote;
	                    git -C super pull;
	                    git -C super submodule update --remote" &&
	(cd submodule &&
		echo file2 >file2 &&
		git add file2 &&
		test_tick &&
		git commit -m "add file2 to submodule"
	) &&
	(cd upstream &&
		git submodule update --remote &&
		git add submodule &&
		test_tick &&
		git commit -m "Update submodule"
	) &&
	git -C super pull &&
	# The --for-status options reads the gitmdoules config
	git -C super submodule summary --for-status >actual &&
	cat >expect <<-\EOF &&
	* submodule 951c301...a939200 (1):
	  < add file2 to submodule
	
	EOF
	test_cmp expect actual &&
	# Test that the update actually succeeds
	test_path_is_missing super/submodule/file2 &&
	git -C super submodule update &&
	test_cmp submodule/file2 super/submodule/file2 &&
	git -C super status --short >output &&
	test_must_be_empty output
'
Maybe a little overkill?

The "upstream" repo will be added in test 1 to better clarify the roles of the involved repositories.

The commit ids should be stable because of test_tick, shouldn't they?

Thanks for the comments, they helped improving the quality of the tests once again.

I'll wait a few days before sending a v7, hopefully someone will find time to take another look at patch 9 and comment also on patch 10, and give an opinion on the "mergeability" status of the whole patchset.

Ciao 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?
Stefan Beller· Oct 10, 2018, 22:55 UTC · re: Antonio Ospite · lore

Re: [PATCH v6 09/10] submodule: support reading .gitmodules when it's not in the working tree

On Wed, Oct 10, 2018 at 11:56 AM Antonio Ospite <ao2@ao2.it> wrote:
Show 83 quoted lines
>
> On Mon, 8 Oct 2018 15:19:00 -0700
> Stefan Beller <sbeller@google.com> wrote:
>
> > > +test_expect_success 'not writing gitmodules config file when it is not checked out' '
> > > +        test_must_fail git -C super submodule--helper config submodule.submodule.url newurl
> >
> > This only checks the exit code, do we also want to check for
> >
> >     test_path_is_missing .gitmodules ?
> >
>
> OK, I agree, let's re-check also *after* we tried and failed to set
> a config value, just to be sure that the code does not get accidentally
> changed in the future to create the file. I'll add the check.
>
> > > +test_expect_success 'initialising submodule when the gitmodules config is not checked out' '
> > > +       git -C super submodule init
> > > +'
> > > +
> > > +test_expect_success 'showing submodule summary when the gitmodules config is not checked out' '
> > > +       git -C super submodule summary
> > > +'
> >
> > Same for these, is the exit code enough, or do we want to look at
> > specific things?
> >
>
> Except for the "summary" test which was not even exercising the
> config_from_gitmodule path,  checking exist status should be sufficient
> to verify that "submodule--helper config" does not fail, but we can
> surely do better.
>
> I will add checks to confirm that not only the commands exited without
> errors but they also achieved the desired effect, to validate the actual
> high-level use case advertised by the test file. This should be more
> future-proof.
>
> And I think I'll merge the summary and the update tests.
>
> > > +
> > > +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"
> > > +       ) &&
> > > +       git -C super submodule update
> >
> > git status would want to be clean afterwards?
>
> Mmh, this should have been "submodule update --remote" in the first
> place to have any effect, I'll take the chance and rewrite this test in
> a different way and also check the effect of the update operation, and
> the repository status.
>
> I'll be something like this:
>
> ORIG_SUBMODULE=$(git -C submodule rev-parse HEAD)
> ORIG_UPSTREAM=$(git -C upstream rev-parse HEAD)
> ORIG_SUPER=$(git -C super rev-parse HEAD)
>
> test_expect_success 're-updating submodule when the gitmodules config is not checked out' '
>         test_when_finished "git -C submodule reset --hard $ORIG_SUBMODULE;
>                             git -C upstream reset --hard $ORIG_UPSTREAM;
>                             git -C super reset --hard $ORIG_SUPER;
>                             git -C upstream submodule update --remote;
>                             git -C super pull;
>                             git -C super submodule update --remote" &&
>         (cd submodule &&
>                 echo file2 >file2 &&
>                 git add file2 &&
>                 test_tick &&
>                 git commit -m "add file2 to submodule"
>         ) &&
>         (cd upstream &&
>                 git submodule update --remote &&
>                 git add submodule &&
>                 test_tick &&
>                 git commit -m "Update submodule"
>         ) &&
>         git -C super pull &&
>         # The --for-status options reads the gitmdoules config
gitmodules
>         git -C super submodule summary --for-status >actual &&
>         cat >expect <<-\EOF &&
>         * submodule 951c301...a939200 (1):

hardcoding hash values burdens the plan to migrate to another hash function,

    rev1=$(git -C submodule rev-parse --short HEAD^)
    rev2=$(git -C submodule rev-parse --short HEAD)
and then use ${rev1}..${rev2} ?
Show 13 quoted lines
>           < add file2 to submodule
>
>         EOF
>         test_cmp expect actual &&
>         # Test that the update actually succeeds
>         test_path_is_missing super/submodule/file2 &&
>         git -C super submodule update &&
>         test_cmp submodule/file2 super/submodule/file2 &&
>         git -C super status --short >output &&
>         test_must_be_empty output
> '
>
> Maybe a little overkill?
Wow, very thorough! You might call it overkill, but now that you have it...
> The "upstream" repo will be added in test 1 to better clarify the roles
> of the involved repositories.
>
> The commit ids should be stable because of test_tick, shouldn't they?

Yes, but see Documentation/technical/hash-function-transition.txt that a couple people are working on. Let's be nice to them. :-)

Stefan
Junio C Hamano· Oct 9, 2018, 03:39 UTC · re: Antonio Ospite · lore

Re: [PATCH v6 09/10] submodule: support reading .gitmodules when it's not in the working tree

Antonio Ospite <ao2@ao2.it> writes:
Show 5 quoted lines
> Finally, add t7416-submodule-sparse-gitmodules.sh to verify that reading
> from .gitmodules succeeds and that writing to it fails when the file is
> not checked out.
> ...
>  t/t7416-submodule-sparse-gitmodules.sh | 78 ++++++++++++++++++++++++++

This now triggers test-lint errors as the most recent maintenance release took t/t7416 for something else. I'll do s/t7416-/t7418-/g on the mailbox before running "git am -s" on this series.

Junio C Hamano· Oct 9, 2018, 03:48 UTC · re: Junio C Hamano · lore

Re: [PATCH v6 09/10] submodule: support reading .gitmodules when it's not in the working tree

Junio C Hamano <gitster@pobox.com> writes:
Show 11 quoted lines
> Antonio Ospite <ao2@ao2.it> writes:
>
>> Finally, add t7416-submodule-sparse-gitmodules.sh to verify that reading
>> from .gitmodules succeeds and that writing to it fails when the file is
>> not checked out.
>> ...
>>  t/t7416-submodule-sparse-gitmodules.sh | 78 ++++++++++++++++++++++++++
>
> This now triggers test-lint errors as the most recent maintenance
> release took t/t7416 for something else.  I'll do s/t7416-/t7418-/g
> on the mailbox before running "git am -s" on this series.

This is an unrelated tangent to the topic, but running "range-diff" on what has been queued on 'pu' since mid September and this replacement after doing the renaming was a surprisingly pleasant experience. In its comparison between 09/10 of the two iterations, it showed that 7416's name has been changed to 7418 but otherwise there is no change in the contents of that test script.

FWIW, tbdiff also gets this right, so the pleasant experience was inherited without getting broken. Kudos should go to both Thomas and Dscho ;-).

Antonio Ospite· Oct 5, 2018, 13:05 UTC · re: Antonio Ospite · lore

[PATCH v6 04/10] t7411: be nicer to future tests and really clean things up

Tests 5 and 7 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 future subsequent tests if they assume that the .gitmodules file has no errors.

Remove both the commits as soon as they are not needed anymore.
Signed-off-by: Antonio Ospite <ao2@ao2.it>
---
 t/t7411-submodule-config.sh | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
Show changes to t/t7411-submodule-config.sh +5 −2
diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
index f2cd1f4a2c..b1f3c6489b 100755
--- a/t/t7411-submodule-config.sh
+++ b/t/t7411-submodule-config.sh
@@ -83,6 +83,8 @@ Submodule name: 'submodule' for path 'submodule'
 EOF
 
 test_expect_success 'error in history of one submodule config lets continue, stderr message contains blob ref' '
+	ORIG=$(git -C super rev-parse HEAD) &&
+	test_when_finished "git -C super reset --hard $ORIG" &&
 	(cd super &&
 		cp .gitmodules .gitmodules.bak &&
 		echo "	value = \"" >>.gitmodules &&
@@ -115,6 +117,8 @@ test_expect_success 'using different treeishs works' '
 '
 
 test_expect_success 'error in history in fetchrecursesubmodule lets continue' '
+	ORIG=$(git -C super rev-parse HEAD) &&
+	test_when_finished "git -C super reset --hard $ORIG" &&
 	(cd super &&
 		git config -f .gitmodules \
 			submodule.submodule.fetchrecursesubmodules blabla &&
@@ -126,8 +130,7 @@ test_expect_success 'error in history in fetchrecursesubmodule lets continue' '
 			HEAD b \
 			HEAD submodule \
 				>actual &&
-		test_cmp expect_error actual  &&
-		git reset --hard HEAD^
+		test_cmp expect_error actual
 	)
 '
 

-- 
2.19.0
Antonio Ospite· Oct 5, 2018, 13:06 UTC · re: Antonio Ospite · lore

[PATCH v6 10/10] t/helper: add test-submodule-nested-repo-config

Add a test tool to exercise config_from_gitmodules(), in particular for the case of nested submodules.

Add also a test to document that reading the submoudles config of nested submodules does not work yet when the .gitmodules file is not in the working tree but it still in the index.

This is because the git API does not always make it possible access the object store of an arbitrary repository (see get_oid() usage in config_from_gitmodules()).

When this git limitation gets fixed the aforementioned use case will be supported too.

Signed-off-by: Antonio Ospite <ao2@ao2.it>
---
 Makefile                                     |  1 +
 t/helper/test-submodule-nested-repo-config.c | 30 +++++++++++++++++
 t/helper/test-tool.c                         |  1 +
 t/helper/test-tool.h                         |  1 +
 t/t7411-submodule-config.sh                  | 34 ++++++++++++++++++++
 5 files changed, 67 insertions(+)
 create mode 100644 t/helper/test-submodule-nested-repo-config.c
Show changes to 5 files +67 −0

Makefile, t/helper/test-submodule-nested-repo-config.c, t/helper/test-tool.c, t/helper/test-tool.h, t/t7411-submodule-config.sh

diff --git a/Makefile b/Makefile
index 13e1c52478..fe8587cd8c 100644
--- a/Makefile
+++ b/Makefile
@@ -737,6 +737,7 @@ TEST_BUILTINS_OBJS += test-sigchain.o
 TEST_BUILTINS_OBJS += test-strcmp-offset.o
 TEST_BUILTINS_OBJS += test-string-list.o
 TEST_BUILTINS_OBJS += test-submodule-config.o
+TEST_BUILTINS_OBJS += test-submodule-nested-repo-config.o
 TEST_BUILTINS_OBJS += test-subprocess.o
 TEST_BUILTINS_OBJS += test-urlmatch-normalization.o
 TEST_BUILTINS_OBJS += test-wildmatch.o
diff --git a/t/helper/test-submodule-nested-repo-config.c b/t/helper/test-submodule-nested-repo-config.c
new file mode 100644
index 0000000000..a31e2a9bea
--- /dev/null
+++ b/t/helper/test-submodule-nested-repo-config.c
@@ -0,0 +1,30 @@
+#include "test-tool.h"
+#include "submodule-config.h"
+
+static void die_usage(int argc, const char **argv, const char *msg)
+{
+	fprintf(stderr, "%s\n", msg);
+	fprintf(stderr, "Usage: %s <submodulepath> <config name>\n", argv[0]);
+	exit(1);
+}
+
+int cmd__submodule_nested_repo_config(int argc, const char **argv)
+{
+	struct repository submodule;
+
+	if (argc < 3)
+		die_usage(argc, argv, "Wrong number of arguments.");
+
+	setup_git_directory();
+
+	if (repo_submodule_init(&submodule, the_repository, argv[1])) {
+		die_usage(argc, argv, "Submodule not found.");
+	}
+
+	/* Read the config of _child_ submodules. */
+	print_config_from_gitmodules(&submodule, argv[2]);
+
+	submodule_free(the_repository);
+
+	return 0;
+}
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index b87a8c1f22..3b473bccd8 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -42,6 +42,7 @@ static struct test_cmd cmds[] = {
 	{ "strcmp-offset", cmd__strcmp_offset },
 	{ "string-list", cmd__string_list },
 	{ "submodule-config", cmd__submodule_config },
+	{ "submodule-nested-repo-config", cmd__submodule_nested_repo_config },
 	{ "subprocess", cmd__subprocess },
 	{ "urlmatch-normalization", cmd__urlmatch_normalization },
 	{ "wildmatch", cmd__wildmatch },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index e074957279..3ca351230c 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -38,6 +38,7 @@ int cmd__sigchain(int argc, const char **argv);
 int cmd__strcmp_offset(int argc, const char **argv);
 int cmd__string_list(int argc, const char **argv);
 int cmd__submodule_config(int argc, const char **argv);
+int cmd__submodule_nested_repo_config(int argc, const char **argv);
 int cmd__subprocess(int argc, const char **argv);
 int cmd__urlmatch_normalization(int argc, const char **argv);
 int cmd__wildmatch(int argc, const char **argv);
diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
index 2cfabb18bc..89690b7adb 100755
--- a/t/t7411-submodule-config.sh
+++ b/t/t7411-submodule-config.sh
@@ -216,4 +216,38 @@ test_expect_success 'reading submodules config from the current branch when .git
 	)
 '
 
+test_expect_success 'reading nested submodules config' '
+	(cd super &&
+		git init submodule/nested_submodule &&
+		echo "a" >submodule/nested_submodule/a &&
+		git -C submodule/nested_submodule add a &&
+		git -C submodule/nested_submodule commit -m "add a" &&
+		git -C submodule submodule add ./nested_submodule &&
+		git -C submodule add nested_submodule &&
+		git -C submodule commit -m "added nested_submodule" &&
+		git add submodule &&
+		git commit -m "updated submodule" &&
+		echo "./nested_submodule" >expect &&
+		test-tool submodule-nested-repo-config \
+			submodule submodule.nested_submodule.url >actual &&
+		test_cmp expect actual
+	)
+'
+
+# When this test eventually passes, before turning it into
+# test_expect_success, remember to replace the test_i18ngrep below with
+# a "test_must_be_empty warning" to be sure that the warning is actually
+# removed from the code.
+test_expect_failure 'reading nested submodules config when .gitmodules is not in the working tree' '
+	test_when_finished "git -C super/submodule checkout .gitmodules" &&
+	(cd super &&
+		echo "./nested_submodule" >expect &&
+		rm submodule/.gitmodules &&
+		test-tool submodule-nested-repo-config \
+			submodule submodule.nested_submodule.url >actual 2>warning &&
+		test_i18ngrep "nested submodules without %s in the working tree are not supported yet" warning &&
+		test_cmp expect actual
+	)
+'
+
 test_done

-- 
2.19.0
Antonio Ospite· Oct 5, 2018, 13:05 UTC · re: Antonio Ospite · lore

[PATCH v6 02/10] submodule: factor out a config_set_in_gitmodules_file_gently function

Introduce a new config_set_in_gitmodules_file_gently() 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".

The purpose of the change is mainly to centralize the code that writes to the .gitmodules file to avoid some duplication.

The naming follows git_config_set_in_file_gently() but the git_ prefix is removed to communicate that this is not a generic git-config API.

Signed-off-by: Antonio Ospite <ao2@ao2.it>
---
 submodule-config.c | 12 ++++++++++++
 submodule-config.h |  1 +
 submodule.c        | 10 +++-------
 3 files changed, 16 insertions(+), 7 deletions(-)
Show changes to 3 files +16 −7

submodule-config.c, submodule-config.h, submodule.c

diff --git a/submodule-config.c b/submodule-config.c
index 823bc76812..8659d97e06 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -707,6 +707,18 @@ int print_config_from_gitmodules(struct repository *repo, const char *key)
 	return 0;
 }
 
+int config_set_in_gitmodules_file_gently(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;
+}
+
 struct fetch_config {
 	int *max_children;
 	int *recurse_submodules;
diff --git a/submodule-config.h b/submodule-config.h
index 031747ccf8..4dc9b0771c 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -49,6 +49,7 @@ const struct submodule *submodule_from_path(struct repository *r,
 					    const char *path);
 void submodule_free(struct repository *r);
 int print_config_from_gitmodules(struct repository *repo, const char *key);
+int config_set_in_gitmodules_file_gently(const char *key, const char *value);
 
 /*
  * Returns 0 if the name is syntactically acceptable as a submodule "name"
diff --git a/submodule.c b/submodule.c
index b53cb6e9c4..3b23e76e55 100644
--- a/submodule.c
+++ b/submodule.c
@@ -89,6 +89,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 +105,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_set_in_gitmodules_file_gently(entry.buf, newpath);
 	strbuf_release(&entry);
-	return 0;
+	return ret;
 }
 
 /*

-- 
2.19.0
Antonio Ospite· Oct 5, 2018, 13:05 UTC · re: Antonio Ospite · lore

[PATCH v6 01/10] submodule: add a print_config_from_gitmodules() helper

Add a new print_config_from_gitmodules() helper function to print values from .gitmodules just like "git config -f .gitmodules" would.

This will be used by a new submodule--helper subcommand to be able to access the .gitmodules file in a more controlled way.

Signed-off-by: Antonio Ospite <ao2@ao2.it>
---
 submodule-config.c | 25 +++++++++++++++++++++++++
 submodule-config.h |  1 +
 2 files changed, 26 insertions(+)
Show changes to 2 files +26 −0

submodule-config.c, submodule-config.h

diff --git a/submodule-config.c b/submodule-config.c
index e04ba756d9..823bc76812 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -682,6 +682,31 @@ void submodule_free(struct repository *r)
 		submodule_cache_clear(r->submodule_cache);
 }
 
+static int config_print_callback(const char *var, const char *value, void *cb_data)
+{
+	char *wanted_key = cb_data;
+
+	if (!strcmp(wanted_key, var))
+		printf("%s\n", value);
+
+	return 0;
+}
+
+int print_config_from_gitmodules(struct repository *repo, const char *key)
+{
+	int ret;
+	char *store_key;
+
+	ret = git_config_parse_key(key, &store_key, NULL);
+	if (ret < 0)
+		return CONFIG_INVALID_KEY;
+
+	config_from_gitmodules(config_print_callback, repo, store_key);
+
+	free(store_key);
+	return 0;
+}
+
 struct fetch_config {
 	int *max_children;
 	int *recurse_submodules;
diff --git a/submodule-config.h b/submodule-config.h
index dc7278eea4..031747ccf8 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -48,6 +48,7 @@ const struct submodule *submodule_from_path(struct repository *r,
 					    const struct object_id *commit_or_tree,
 					    const char *path);
 void submodule_free(struct repository *r);
+int print_config_from_gitmodules(struct repository *repo, const char *key);
 
 /*
  * Returns 0 if the name is syntactically acceptable as a submodule "name"

-- 
2.19.0
Antonio Ospite· Oct 5, 2018, 13:05 UTC · re: Antonio Ospite · lore

[PATCH v6 06/10] submodule: use the 'submodule--helper config' command

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

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

Signed-off-by: Antonio Ospite <ao2@ao2.it>
---
 git-submodule.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
Show changes to git-submodule.sh +4 −4
diff --git a/git-submodule.sh b/git-submodule.sh
index 1b568e29b9..0805fadf47 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -72,7 +72,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}"
 }
@@ -283,11 +283,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.19.0
Antonio Ospite· Oct 5, 2018, 13:05 UTC · re: Antonio Ospite · lore

[PATCH v6 05/10] submodule--helper: add a new 'config' subcommand

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 | 14 ++++++++++++++
 t/t7411-submodule-config.sh | 27 +++++++++++++++++++++++++++
 2 files changed, 41 insertions(+)
Show changes to 2 files +41 −0

builtin/submodule--helper.c, t/t7411-submodule-config.sh

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 40844870cf..e1bdca8f0b 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2125,6 +2125,19 @@ static int check_name(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
+static int module_config(int argc, const char **argv, const char *prefix)
+{
+	/* Equivalent to ACTION_GET in builtin/config.c */
+	if (argc == 2)
+		return print_config_from_gitmodules(the_repository, argv[1]);
+
+	/* Equivalent to ACTION_SET in builtin/config.c */
+	if (argc == 3)
+		return config_set_in_gitmodules_file_gently(argv[1], argv[2]);
+
+	die("submodule--helper config takes 1 or 2 arguments: name [value]");
+}
+
 #define SUPPORT_SUPER_PREFIX (1<<0)
 
 struct cmd_struct {
@@ -2154,6 +2167,7 @@ static struct cmd_struct commands[] = {
 	{"absorb-git-dirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
 	{"is-active", is_active, 0},
 	{"check-name", check_name, 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 b1f3c6489b..791245f18d 100755
--- a/t/t7411-submodule-config.sh
+++ b/t/t7411-submodule-config.sh
@@ -134,4 +134,31 @@ test_expect_success 'error in history in fetchrecursesubmodule lets continue' '
 	)
 '
 
+test_expect_success 'reading submodules config with "submodule--helper config"' '
+	(cd super &&
+		echo "../submodule" >expect &&
+		git submodule--helper config submodule.submodule.url >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'writing submodules config with "submodule--helper config"' '
+	(cd super &&
+		echo "new_url" >expect &&
+		git submodule--helper config submodule.submodule.url "new_url" &&
+		git submodule--helper config submodule.submodule.url >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'overwriting unstaged submodules config with "submodule--helper config"' '
+	test_when_finished "git -C super checkout .gitmodules" &&
+	(cd super &&
+		echo "newer_url" >expect &&
+		git submodule--helper config submodule.submodule.url "newer_url" &&
+		git submodule--helper config submodule.submodule.url >actual &&
+		test_cmp expect actual
+	)
+'
+
 test_done

-- 
2.19.0
Antonio Ospite· Oct 5, 2018, 13:05 UTC · re: Antonio Ospite · lore

[PATCH v6 08/10] submodule: add a helper to check if it is safe to write to .gitmodules

Introduce a helper function named is_writing_gitmodules_ok() to verify that the .gitmodules file is safe to write.

The function name follows the scheme of is_staging_gitmodules_ok().

The two symbolic constants GITMODULES_INDEX and GITMODULES_HEAD are used to get help from the C preprocessor in preventing typos, especially for future users.

This is in preparation for a future change which teaches git how to read .gitmodules from the index or from the current branch if the file is not available in the working tree.

The rationale behind the check is that writing to .gitmodules requires the file to be present in the working tree, unless a brand new .gitmodules is being created (in which case the .gitmodules file would not exist at all: neither in the working tree nor in the index or in the current branch).

Expose the functionality also via a "submodule-helper config --check-writeable" command, as git scripts may want to perform the check before modifying submodules configuration.

Signed-off-by: Antonio Ospite <ao2@ao2.it>
---
 builtin/submodule--helper.c | 24 +++++++++++++++++++++++-
 cache.h                     |  2 ++
 submodule.c                 | 18 ++++++++++++++++++
 submodule.h                 |  1 +
 t/t7411-submodule-config.sh | 31 +++++++++++++++++++++++++++++++
 5 files changed, 75 insertions(+), 1 deletion(-)
Show changes to 5 files +75 −1

builtin/submodule--helper.c, cache.h, submodule.c, submodule.h, t/t7411-submodule-config.sh

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index e1bdca8f0b..28f3ccca6d 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2127,6 +2127,28 @@ static int check_name(int argc, const char **argv, const char *prefix)
 
 static int module_config(int argc, const char **argv, const char *prefix)
 {
+	enum {
+		CHECK_WRITEABLE = 1
+	} command = 0;
+
+	struct option module_config_options[] = {
+		OPT_CMDMODE(0, "check-writeable", &command,
+			    N_("check if it is safe to write to the .gitmodules file"),
+			    CHECK_WRITEABLE),
+		OPT_END()
+	};
+	const char *const git_submodule_helper_usage[] = {
+		N_("git submodule--helper config name [value]"),
+		N_("git submodule--helper config --check-writeable"),
+		NULL
+	};
+
+	argc = parse_options(argc, argv, prefix, module_config_options,
+			     git_submodule_helper_usage, PARSE_OPT_KEEP_ARGV0);
+
+	if (argc == 1 && command == CHECK_WRITEABLE)
+		return is_writing_gitmodules_ok() ? 0 : -1;
+
 	/* Equivalent to ACTION_GET in builtin/config.c */
 	if (argc == 2)
 		return print_config_from_gitmodules(the_repository, argv[1]);
@@ -2135,7 +2157,7 @@ static int module_config(int argc, const char **argv, const char *prefix)
 	if (argc == 3)
 		return config_set_in_gitmodules_file_gently(argv[1], argv[2]);
 
-	die("submodule--helper config takes 1 or 2 arguments: name [value]");
+	usage_with_options(git_submodule_helper_usage, module_config_options);
 }
 
 #define SUPPORT_SUPER_PREFIX (1<<0)
diff --git a/cache.h b/cache.h
index d508f3d4f8..2d495fc800 100644
--- a/cache.h
+++ b/cache.h
@@ -486,6 +486,8 @@ 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_INDEX ":.gitmodules"
+#define GITMODULES_HEAD "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/submodule.c b/submodule.c
index 3b23e76e55..bd2506c5ba 100644
--- a/submodule.c
+++ b/submodule.c
@@ -51,6 +51,24 @@ int is_gitmodules_unmerged(const struct index_state *istate)
 	return 0;
 }
 
+/*
+ * Check if the .gitmodules file is safe to write.
+ *
+ * Writing to the .gitmodules file requires that the file exists in the
+ * working tree or, if it doesn't, that a brand new .gitmodules file is going
+ * to be created (i.e. it's neither in the index nor in the current branch).
+ *
+ * It is not safe to write to .gitmodules if it's not in the working tree but
+ * it is in the index or in the current branch, because writing new values
+ * (and staging them) would blindly overwrite ALL the old content.
+ */
+int is_writing_gitmodules_ok(void)
+{
+	struct object_id oid;
+	return file_exists(GITMODULES_FILE) ||
+		(get_oid(GITMODULES_INDEX, &oid) < 0 && get_oid(GITMODULES_HEAD, &oid) < 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 e452919aa4..7a22f71cb9 100644
--- a/submodule.h
+++ b/submodule.h
@@ -40,6 +40,7 @@ struct submodule_update_strategy {
 #define SUBMODULE_UPDATE_STRATEGY_INIT {SM_UPDATE_UNSPECIFIED, NULL}
 
 int is_gitmodules_unmerged(const struct index_state *istate);
+int is_writing_gitmodules_ok(void);
 int is_staging_gitmodules_ok(struct index_state *istate);
 int update_path_in_gitmodules(const char *oldpath, const char *newpath);
 int remove_path_from_gitmodules(const char *path);
diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
index 791245f18d..45953f9300 100755
--- a/t/t7411-submodule-config.sh
+++ b/t/t7411-submodule-config.sh
@@ -161,4 +161,35 @@ test_expect_success 'overwriting unstaged submodules config with "submodule--hel
 	)
 '
 
+test_expect_success 'writeable .gitmodules when it is in the working tree' '
+	git -C super submodule--helper config --check-writeable
+'
+
+test_expect_success 'writeable .gitmodules when it is nowhere in the repository' '
+	ORIG=$(git -C super rev-parse HEAD) &&
+	test_when_finished "git -C super reset --hard $ORIG" &&
+	(cd super &&
+		git rm .gitmodules &&
+		git commit -m "remove .gitmodules from the current branch" &&
+		git submodule--helper config --check-writeable
+	)
+'
+
+test_expect_success 'non-writeable .gitmodules when it is in the index but not in the working tree' '
+	test_when_finished "git -C super checkout .gitmodules" &&
+	(cd super &&
+		rm -f .gitmodules &&
+		test_must_fail git submodule--helper config --check-writeable
+	)
+'
+
+test_expect_success 'non-writeable .gitmodules when it is in the current branch but not in the index' '
+	ORIG=$(git -C super rev-parse HEAD) &&
+	test_when_finished "git -C super reset --hard $ORIG" &&
+	(cd super &&
+		git rm .gitmodules &&
+		test_must_fail git submodule--helper config --check-writeable
+	)
+'
+
 test_done

-- 
2.19.0
Stefan Beller· Oct 5, 2018, 23:50 UTC · re: Antonio Ospite · lore

Re: [PATCH v6 08/10] submodule: add a helper to check if it is safe to write to .gitmodules

Show 5 quoted lines
>  static int module_config(int argc, const char **argv, const char *prefix)
>  {
> +       enum {
> +               CHECK_WRITEABLE = 1
> +       } command = 0;

Can we have the default named? Then we would only use states from within the enum?

Antonio Ospite· Oct 6, 2018, 09:19 UTC · re: Stefan Beller · lore

Re: [PATCH v6 08/10] submodule: add a helper to check if it is safe to write to .gitmodules

On Fri, 5 Oct 2018 16:50:10 -0700 Stefan Beller <sbeller@google.com> wrote:

Show 8 quoted lines
> >  static int module_config(int argc, const char **argv, const char *prefix)
> >  {
> > +       enum {
> > +               CHECK_WRITEABLE = 1
> > +       } command = 0;
> 
> Can we have the default named? Then we would only use states
> from within the enum?
The default would mean:
  "no command passed as a CLI *option*"

I copied this style from builtin/bisect--helper.c::cmd_bisect__helper() and it's also used in builtin/rebase--helper.c

I can add a name for the default enum value but I am not sure what it should be: NO_COMMAND_OPTION, COMMAND_DEFAULT, MODE_DEFAULT?

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?
Junio C Hamano· Oct 6, 2018, 23:44 UTC · re: Stefan Beller · lore

Re: [PATCH v6 08/10] submodule: add a helper to check if it is safe to write to .gitmodules

Stefan Beller <sbeller@google.com> writes:
Show 8 quoted lines
>>  static int module_config(int argc, const char **argv, const char *prefix)
>>  {
>> +       enum {
>> +               CHECK_WRITEABLE = 1
>> +       } command = 0;
>
> Can we have the default named? Then we would only use states
> from within the enum?

Why? Do we use a half-intelligent "switch () { case ...: ... }" checker that would otherwise complain if we handled "case 0" in such a switch statement, or something like that?

Are we going to gain a lot more enum members, by the way? At this point, this looks more like a

	unsigned check_writable = 0; /* default is not to check */
to me.
Antonio Ospite· Oct 8, 2018, 12:37 UTC · re: Junio C Hamano · lore

Re: [PATCH v6 08/10] submodule: add a helper to check if it is safe to write to .gitmodules

On Sun, 07 Oct 2018 08:44:20 +0900 Junio C Hamano <gitster@pobox.com> wrote:

Show 21 quoted lines
> Stefan Beller <sbeller@google.com> writes:
> 
> >>  static int module_config(int argc, const char **argv, const char *prefix)
> >>  {
> >> +       enum {
> >> +               CHECK_WRITEABLE = 1
> >> +       } command = 0;
> >
> > Can we have the default named? Then we would only use states
> > from within the enum?
> 
> Why?  Do we use a half-intelligent "switch () { case ...: ... }"
> checker that would otherwise complain if we handled "case 0" in such
> a switch statement, or something like that?
> 
> Are we going to gain a lot more enum members, by the way?  At this
> point, this looks more like a
> 
> 	unsigned check_writable = 0; /* default is not to check */
> 
> to me.
Hi,

the CHECK_WRITEABLE operation is alternative to the get/set ones, not an addition, so I can see the rationale behind Stefan's suggestion: either have named enums members for all command "modes" or for none of them; however other users of enum+OPT_CMDMODE seems to think like the enum is for commands passed as *options* and the unnamed default is for actions derived from *arguments*. I don't have a strong opinion on this matter, tho, so just tell me what you prefer and I'll do it for v7.

Using an enum was to have a more explicit syntax in case other commands were going to be added in the future (I imagine "--stage" or "--list-all" as possible additions), and does not affect the generated code, so I though it was worth it.

Anyways, these are really details, let's concentrate on patches 9 and 10 which deserve much more attention. :)

Thanks you,
   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?
Antonio Ospite· Oct 5, 2018, 13:05 UTC · re: Antonio Ospite · lore

[PATCH v6 07/10] t7506: clean up .gitmodules properly before setting up new scenario

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 before calling the "git submodule add" commands for the new layout, the .gitmodules file is removed only from the working tree still leaving the previous content in current branch.

This can break if, in the future, "git submodule add" starts differentiating between the following two cases:

  - .gitmodules is not in the working tree but it is in the current
    branch (it may not be safe to add new submodules in this case);
  - .gitmodules is neither in the working tree nor anywhere in the
    current branch (it is safe to add new submodules).

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

This is more future-proof and does not break current tests.
Signed-off-by: Antonio Ospite <ao2@ao2.it>
---
 t/t7506-status-submodule.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Show changes to t/t7506-status-submodule.sh +2 −1
diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh
index 943708fb04..08629a6e70 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.19.0
Antonio Ospite· Oct 6, 2018, 09:20 UTC · re: Antonio Ospite · lore

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

On Fri, 5 Oct 2018 15:05:51 +0200 Antonio Ospite <ao2@ao2.it> wrote:

[...]
>  t/t7416-submodule-sparse-gitmodules.sh       |  78 ++++++++++
>  16 files changed, 410 insertions(+), 32 deletions(-)
>  create mode 100755 t/t7416-submodule-sparse-gitmodules.sh

I just saw that t7416 and t7417 have been added in the latest stable release, I'll wait some days before sending a v7 which renames the newly added test to t/t7418-submodule-sparse-gitmodules.sh

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?
Junio C Hamano· Oct 25, 2018, 08:40 UTC · re: Antonio Ospite · lore

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

Antonio Ospite <ao2@ao2.it> writes:
> this series teaches git to try and read the .gitmodules file from the
> index (:.gitmodules) or from the current branch (HEAD:.gitmodules) when
> the file is not readily available in the working tree.

What you said in [*1*] the discussion on [09/10] sounded like you are preparing an update of the series, so the topic is marked as "Expecting a reroll" in the recent "What's cooking" report. At least one topic now depends on the enhancement this topic makes, so I'd like to know what the current status and ETA of the reroll would be, in order to sort-of act as a traffic cop.

Your answer could even be "I have been too busy, and I do not think an update will come for some time"---in other words, I do not mean to tell you to drop other things and work on this instead.

If you are too busy, I can even see if other stakeholders (e.g. Stefan, whose topic now depends on this series) can take it over and update it after re-reading the discussion on the latest round.

Thanks.
[Reference]
*1* http://public-inbox.org/git/20181010205645.e1529eff9099805029b1d6ef@ao2.it/
Antonio Ospite· Oct 25, 2018, 13:20 UTC · re: Junio C Hamano · lore

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

On Thu, 25 Oct 2018 17:40:47 +0900 Junio C Hamano <gitster@pobox.com> wrote:

Show 13 quoted lines
> Antonio Ospite <ao2@ao2.it> writes:
> 
> > this series teaches git to try and read the .gitmodules file from the
> > index (:.gitmodules) or from the current branch (HEAD:.gitmodules) when
> > the file is not readily available in the working tree.
> 
> What you said in [*1*] the discussion on [09/10] sounded like you
> are preparing an update of the series, so the topic is marked as
> "Expecting a reroll" in the recent "What's cooking" report.  At
> least one topic now depends on the enhancement this topic makes, so
> I'd like to know what the current status and ETA of the reroll would
> be, in order to sort-of act as a traffic cop.
> 
Hi Junio,
I can send a v7 later today.

It will only contain the improvements to 7416-submodule-sparse-gitmodules.sh as discussed in [*1*], it won't contain changes to patch 8 as motivated in https://public-inbox.org/git/20181008143709.dfcc845ab393c9caea66035e@ao2.it/

I will also leave patch 10 unchanged, improvements can be made in follow-up patches.

BTW, what is the new topic which depends on this one?
Thank you,
   Antonio
> [Reference]
> 
> *1* http://public-inbox.org/git/20181010205645.e1529eff9099805029b1d6ef@ao2.it/
-- 
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?

← back to recent threads