Re: git submodules implementation question

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 1, 2016, 21:02 UTC
Message-ID: <xmqqtwdzuye3.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <xmqq4m5zwevl.fsf@gitster.mtv.corp.google.com>

Junio C Hamano <gitster@pobox.com> writes:

If we add

	# "git diff" should terminate with an error.
	# NOTE: without fix this will recurse forever!
	test_must_fail git -C dst diff &&

after breaking the repository, we can also see "git diff" recurse forever, because it wants to know if "sub" submodule is modified, attempts to run "git status" in there, and ends up running that command in the context of the parent repository.

I am tempted to cheat and commit this, even though this test is no longer about fetching submodules.

-- >8 -- [PATCH] submodule: avoid auto-discovery in prepare_submodule_repo_env()

The function is used to set up the environment variable used in a subprocess we spawn in a submodule directory. The callers set up a child_process structure, find the working tree path of one submodule and set .dir field to it, and then use start_command() API to spawn the subprocess like "status", "fetch", etc.

When this happens, we expect that the ".git" (either a directory or a gitfile that points at the real location) in the current working directory of the subprocess MUST be the repository for the submodule.

If this ".git" thing is a corrupt repository, however, because prepare_submodule_repo_env() unsets GIT_DIR and GIT_WORK_TREE, the subprocess will see ".git", thinks it is not a repository, and attempt to find one by going up, likely to end up in finding the repository of the superproject. In some codepaths, this will cause a command run with the "--recurse-submodules" option to recurse forever.

By exporting GIT_DIR=.git, disable the auto-discovery logic in the subprocess, which would instead stop it and report an error.

The test illustrates existing problems in a few callsites of this function. Without this fix, "git fetch --recurse-submodules", "git status" and "git diff" keep recursing forever.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 submodule.c                 |  1 +
 t/t5526-fetch-submodules.sh | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+)

diff --git a/submodule.c b/submodule.c
index 4532b11..2801fbb 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1160,4 +1160,5 @@ void prepare_submodule_repo_env(struct argv_array *out)
 		if (strcmp(*var, CONFIG_DATA_ENVIRONMENT))
 			argv_array_push(out, *var);
 	}
+	argv_array_push(out, "GIT_DIR=.git");
 }
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index 954d0e4..f3b0a8d 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -485,4 +485,39 @@ test_expect_success 'fetching submodules respects parallel settings' '
 	)
 '
 
+test_expect_success 'fetching submodule into a broken repository' '
+	# Prepare src and src/sub nested in it
+	git init src &&
+	(
+		cd src &&
+		git init sub &&
+		git -C sub commit --allow-empty -m "initial in sub" &&
+		git submodule add -- ./sub sub &&
+		git commit -m "initial in top"
+	) &&
+
+	# Clone the old-fashoned way
+	git clone src dst &&
+	git -C dst clone ../src/sub sub &&
+
+	# Make sure that old-fashoned layout is still supported
+	git -C dst status &&
+
+	# "diff" would find no change
+	git -C dst diff --exit-code &&
+
+	# Recursive-fetch works fine
+	git -C dst fetch --recurse-submodules &&
+
+	# Break the receiving submodule
+	rm -f dst/sub/.git/HEAD &&
+
+	# NOTE: without the fix the following tests will recurse forever!
+	# They should terminate with an error.
+
+	test_must_fail git -C dst status &&
+	test_must_fail git -C dst diff &&
+	test_must_fail git -C dst fetch --recurse-submodules
+'
+
 test_done

-- 
2.10.0-rc2-314-g775ea9a