From: Peter Kästle Date: Tue, 08 Dec 2020 14:06:08 GMT Subject: Re: [PATCH v2] submodules: fix of regression on fetching of non-init subsub-repo Message-ID: <1f0422a1-f6e7-8044-ed53-c9496fbd1ccb@nokia.com> In-Reply-To: <613FAD04-0D5A-4DE0-8FE8-0C5C5619B7BC@gmail.com> On 07.12.20 19:42, Philippe Blain wrote: > Hi Peter, > >> Le 7 déc. 2020 à 08:46, Peter Kaestle a écrit : >> >> A regression has been introduced by a62387b (submodule.c: fetch in >> submodules git directory instead of in worktree, 2018-11-28). >> >> The scenario in which it triggers is when one has a remote repository >> with a subrepository inside a subrepository like this: >> superproject/middle_repo/inner_repo > > The correct terminology is "submodule", not "subrepository". > > Also, (minor point) I would just write "when one has a repository", > as its simpler (the repository by itself is not "remote", it is only "remote" > in relation the repositories that are cloned from it). ok. >> Person A and B have both a clone of it, while Person B is not working >> with the inner_repo and thus does not have it initialized in his working >> copy. >> >> Now person A introduces a change to the inner_repo and propagates it >> through the middle_repo and the superproject. >> >> Once person A pushed the changes and person B wants to fetch them using >> "git fetch" on superproject level, > > s/on/at the/ ok. >> B's git call will return with error >> saying: >> >> Could not access submodule 'inner_repo' >> Errors during submodule fetch: >> middle_repo >> >> Expectation is that in this case the inner submodule will be recognized >> as uninitialized subrepository and skipped by the git fetch command. > > here again, terminology: "as an uninitialized submodule" ok. >> This used to work correctly before 'a62387b (submodule.c: fetch in >> submodules git directory instead of in worktree, 2018-11-28)'. >> >> Starting with a62387b the code wants to evaluate "is_empty_dir()" inside >> .git/modules for a directory only existing in the worktree, delivering >> then of course wrong return value. >> >> This patch ensures is_empty_dir() is getting the correct path of the >> uninitialized submodule by concatenation of the actual worktree and the >> name of the uninitialized submodule. >> >> Furthermore a regression test case is added, which tests for recursive >> fetches on a superproject with uninitialized sub repositories. >> This >> issue was leading to an infinite loop when doing a revert of a62387b. > > I would maybe add more details here, something like the following > (we can cite your previous attempt, because it was merged to 'master'): > > The first attempt to fix this regression, in 1b7ac4e6d4 (submodules: > fix of regression on fetching of non-init subsub-repo, 2020-11-12), by simply > reverting a62387b, resulted in > an infinite loop of submodule fetches in the simpler case of a recursive fetch of a superproject with > uninitialized submodules, and so this commit was reverted in 7091499bc0 (Revert > "submodules: fix of regression on fetching of non-init subsub-repo", 2020-12-02). > To prevent future breakages, also add a regression test for this scenario. Jip, I like that. >> >> Signed-off-by: Peter Kaestle >> CC: Junio C Hamano >> CC: Philippe Blain >> CC: Ralf Thielow >> CC: Eric Sunshine >> --- >> submodule.c | 7 ++- >> t/t5526-fetch-submodules.sh | 104 ++++++++++++++++++++++++++++++++++++ >> 2 files changed, 110 insertions(+), 1 deletion(-) >> >> diff --git a/submodule.c b/submodule.c >> index b3bb59f066..b561445329 100644 >> --- a/submodule.c >> +++ b/submodule.c >> @@ -1477,6 +1477,7 @@ static int get_next_submodule(struct child_process *cp, >> strbuf_release(&submodule_prefix); >> return 1; >> } else { >> + struct strbuf empty_submodule_path = STRBUF_INIT; >> >> fetch_task_release(task); >> free(task); >> @@ -1485,13 +1486,17 @@ static int get_next_submodule(struct child_process *cp, >> * An empty directory is normal, >> * the submodule is not initialized >> */ >> + strbuf_addf(&empty_submodule_path, "%s/%s/", >> + spf->r->worktree, >> + ce->name); >> if (S_ISGITLINK(ce->ce_mode) && >> - !is_empty_dir(ce->name)) { >> + !is_empty_dir(empty_submodule_path.buf)) { >> spf->result = 1; >> strbuf_addf(err, >> _("Could not access submodule '%s'\n"), >> ce->name); >> } >> + strbuf_release(&empty_submodule_path); >> } >> } > > > Maybe a personal preference, but I would have gone for something a little simpler, like the following: > > > diff --git a/submodule.c b/submodule.c > index b3bb59f066..4200865174 100644 > --- a/submodule.c > +++ b/submodule.c > @@ -1486,7 +1486,7 @@ static int get_next_submodule(struct child_process *cp, > * the submodule is not initialized > */ > if (S_ISGITLINK(ce->ce_mode) && > - !is_empty_dir(ce->name)) { > + !is_empty_dir(repo_worktree_path(spf->r, "%s", ce->name))) { I'm not deep enough into the git code to judge which approach is the better one. From my perspective, being a foreigner to the git code, I like my proposed code more, as for me it's much easier to understand what's happening by having a meaningful variable name and without being forced to dig into outer functions first. Also Junio C Hamano is having some concerns, which I can't judge: > But then you leak the return value from repo_worktree_path(), no? Thus for v3 I'll stick to my proposal and when you'll review it, please discuss with each other whether I should go for a v4 using repo_worktree_path(). [...] >> + >> +test_expect_success 'setup recursive fetch with uninit submodule' ' >> + # does not depend on any previous test setups >> + >> + git init main && >> + git init sub && >> + >> + >sub/file && >> + git -C sub add file && >> + git -C sub commit -m "add file" && >> + git -C sub rev-parse HEAD >expect && >> + >> + git -C main submodule add ../sub && >> + git -C main submodule init && >> + git -C main submodule update --checkout && > > These two steps are unnecessary as they are implicitly done by 'git submodule add'. > I think we could reflect real life a little bit more by cloning the superproject, and running > the 'recursive fetch with uninit submodule' test below in the clone. Yes, you're right, "...init" and "...update..." can be removed. >> + git -C main submodule status >out && >> + sed -e "s/^ //" -e "s/ sub .*$//" out >actual && >> + test_cmp expect actual >> +' >> + >> +test_expect_success 'recursive fetch with uninit submodule' ' >> + # depends on previous test for setup >> + >> + git -C main submodule deinit -f sub && > > Here you are deiniting the submodule, such that > the Git directory will stay in .git/modules/sub. This is not the same thing > as a submodule that was never initialized ("uninitialized"), for which .git/modules/sub > will not yet exist. So maybe we could harden the tests by also testing > for that scenario ? I don't know... maybe the infinite loop only happens > if .git/modules/sub actually already exists. If so, the test name should be > "recursive fetch with deinitialized submodule", I think. I added another test case for v3, which checks for this in case of never initialized submodule. When executing the test, I can see that the infinite loop regression only occurs after doing the init followed by a deinit. Thus renaming the test accordingly. -- best regards --peter;