From: Peter Kästle Date: Thu, 03 Dec 2020 15:33:37 GMT Subject: Re: BUG in fetching non-checked out submodule Message-ID: <0b6a34a0-428e-5fc4-307d-1217b112659c@nokia.com> In-Reply-To: Hi Philippe, On 03.12.20 16:25, Philippe Blain wrote: >> On 03.12.20 00:06, Junio C Hamano wrote: >>> Philippe Blain writes: >>> >>>> Thanks for bisecting it. That commit wanted to fix a different bug >>>> related to nested submodules, and the route taken was simply >>>> reverting an earlier commit (a62387b (submodule.c: fetch in >>>> submodules git directory instead of in worktree, 2018-11-28). >>>> >>>> As you discovered, it breaks other scenarios. >>>> >>>>> >>>>> $ git version >>>>> git version 2.29.2.435.g72ffeb997e >>>>> >>>>> $ git config --get submodule.recurse >>>>> true >>> >>> I think the current situation is probably worse. >>> >>> As a short-term fix, we should revert 1b7ac4e6d4 until we can come >>> up with a real fix, probably. >> >> Junio: This is why I originally intended to commit the test case for the >> testsuite separated from the revert and wanted to start a discussion >> about the actual real fix for the issue: >> https://public-inbox.org/git/1604413399-63090-1-git-send-email-peter.kaestle@nokia.com/ >> >> My proposal would be to revert 1b7ac4e6d4 and isolate the test case >> "test_expect_success 'setup nested submodule fetch test' '" make it >> "test_expect_failure" and apply it instead, until we come up with a real >> solution. > > > I think I have the real solution. I did some debugging and I think it > is quite easy: > In 'get_next_submodule', 'get_submodule_repo_for(spf->r, task->sub)' > fails to get a repo pointer for the submodule repository, since it is > not initialized. That > is normal. Then we go in the "else" branch, and hit this code: > > /* > * An empty directory is normal, > * the submodule is not initialized > */ > if (S_ISGITLINK(ce->ce_mode) && > !is_empty_dir(ce->name)) { > > 'is_empty_dir' receives ce->name, but the current working directory is the > Git directory of 'middle', so clearly is_empty_dir returns false, as > > /path/to/git/t/trash > directory.t5526-fetch-submodules/B/.git/modules/middle/inner > > is a non-existent path. The path that we should send is the worktree > of inner, ie. > the concatenation of spf->r->worktree and ce->name. This would give > > /path/to/git/t/trash directory.t5526-fetch-submodules/B/middle/inner, > > which is an empty directory since the inner submodule is not initialized, > and we would not get the "Could not access submodule inner" error > that you wanted to solve. On quick glance this sounds plausible, but to fully understand it I need to put some effort in reading this code again. I hope to do so tomorrow. We can then compile a new set of patches including this real fix and Ralf's and my test case. Thanks for digging into it. -- kind regards --peter;