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

The reason for the failure is memory corruption manifesting in various
ways: segfault, malloc() or use after free() errors from libc, corrupt
loose object, invalid ref, bogus output, etc.

Applying the following patch makes t7814 fail almost every time,
though sometimes that loop has to iterate over 1000 times until that
'git grep' finally fails...  so good luck with debugging ;)

diff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh
index 7184113b9b..93ae2e8e7c 100755
--- a/t/t7814-grep-recurse-submodules.sh
+++ b/t/t7814-grep-recurse-submodules.sh

@@ -337,6 +337,10 @@ test_expect_success 'grep --recurse-submodules should pass the pattern type alon
 	test_must_fail git -c grep.patternType=fixed grep --recurse-submodules -e "(.|.)[\d]" &&
 
 	# Basic
+	for i in $(seq 0 2000)
+	do
+		git grep --recurse-submodules 1 >/dev/null || return 1
+	done &&
 	git grep -G --recurse-submodules -e "(.|.)[\d]" >actual &&
 	cat >expect <<-\EOF &&
 	a:(1|2)d(3|4)

On first look I didn't notice anything that is obviously wrong in this
patch and could be responsible for the memory corruption, but there is
one thing I found strange, though:


On Mon, Sep 17, 2018 at 04:09:40PM +0200, Antonio Ospite wrote:

quoted

When the .gitmodules file is not available in the working tree, try
using the content from the index and from the current branch.

"from the index and from the current branch" of which repository?

quoted

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).

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.

Signed-off-by: Antonio Ospite <redacted>
---

quoted

diff --git a/submodule-config.c b/submodule-config.c
index 61a555e920..bdb1d0e2c9 100644
--- a/submodule-config.c
+++ b/submodule-config.c

quoted

@@ -603,8 +604,21 @@ 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 (get_oid(GITMODULES_INDEX, &oid) >= 0)
+			config_source.blob = GITMODULES_INDEX;

The repository used in t7814 contains nested submodules, which means
that config_from_gitmodules() is invoked three times.

Now, the first two of those calls look at the superproject and at
'submodule', and find the existing files '.../trash
directory.t7814-grep-recurse-submodules/.gitmodules' and '.../trash
directory.t7814-grep-recurse-submodules/submodule/.gitmodules',
respectively.  So far so good.

The third call, however, looks at the nested submodule at
'submodule/sub', which doesn't contain a '.gitmodules' file.  So this
function goes on with the second condition and calls
get_oid(GITMODULES_INDEX, &oid), which then appears to find the blob
in the _superproject's_ index.

I'm no expert on submodules, but my gut feeling says that this can't
be right.  But if it _is_ right, then I would say that the commit
message should explain in detail, why it is right.

Anyway, even if it is indeed wrong, I'm not sure whether this is the
root cause of the memory corruption.

quoted

+		else if (get_oid(GITMODULES_HEAD, &oid) >= 0)
+			config_source.blob = GITMODULES_HEAD;
+
+		config_with_options(fn, data, &config_source, &opts);
+
 		free(file);
 	}
 }