Re: [PATCH v5 9/9] submodule: support reading .gitmodules when it's not in the working tree
- From
SZEDER Gábor <szeder.dev@gmail.com>
- Date
- Sep 18, 2018, 17:12 UTC
- Message-ID
- <20180918171257.GC27036@localhost>
- In-Reply-To
- <20180917140940.3839-10-ao2@ao2.it>
Hi Antonio,
it appears that this patch (and its previous versions as well) is responsible for triggering occasional test failures in 't7814-grep-recurse-submodules.sh', more frequently, about once in every ten runs, on macOS on Travis CI, less frequently, about once in a couple of hundred runs on Linux on my machine.
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: > 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? > 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 <ao2@ao2.it> > --- > diff --git a/submodule-config.c b/submodule-config.c > index 61a555e920..bdb1d0e2c9 100644 > --- a/submodule-config.c > +++ b/submodule-config.c > @@ -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. > + else if (get_oid(GITMODULES_HEAD, &oid) >= 0) > + config_source.blob = GITMODULES_HEAD; > + > + config_with_options(fn, data, &config_source, &opts); > + > free(file); > } > }