| Message ID | 1607442126-34705-1-git-send-email-peter.kaestle@nokia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] submodules: fix of regression on fetching of non-init subsub-repo | expand |
On 08.12.20 16:42, Peter Kaestle wrote: > 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 repository with a > submodule inside a submodule like this: > superproject/middle_repo/inner_repo > > 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" at the superproject level, 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 submodule and skipped by the git fetch command. > > 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. > > 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. > > Signed-off-by: Peter Kaestle <peter.kaestle@nokia.com> > CC: Junio C Hamano <gitster@pobox.com> > CC: Philippe Blain <levraiphilippeblain@gmail.com> > CC: Ralf Thielow <ralf.thielow@gmail.com> > CC: Philippe Blain <levraiphilippeblain@gmail.com> > --- > submodule.c | 7 ++- > t/t5526-fetch-submodules.sh | 124 ++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 130 insertions(+), 1 deletion(-) > > diff --git a/submodule.c b/submodule.c > index b3bb59f..b561445 100644 > --- a/submodule.c > +++ b/submodule.c [...] These are my test run outputs: final_fix [PATCH v3] ========== # passed all 41 test(s) recursion_regression [1b7ac4e6d4] ========== not ok 41 - recursive fetch after deinit a submodule no_fix [3a0b884c - origin/master] ========== not ok 38 - fetching a superproject containing an uninitialized sub/sub project
Peter Kaestle <peter.kaestle@nokia.com> writes: > 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 repository with a > submodule inside a submodule like this: > superproject/middle_repo/inner_repo > > 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" at the superproject level, 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 submodule and skipped by the git fetch command. > > 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. > > 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. > > Signed-off-by: Peter Kaestle <peter.kaestle@nokia.com> > CC: Junio C Hamano <gitster@pobox.com> > CC: Philippe Blain <levraiphilippeblain@gmail.com> > CC: Ralf Thielow <ralf.thielow@gmail.com> > CC: Philippe Blain <levraiphilippeblain@gmail.com> > --- Thanks, will replace.
Hi Peter, Le mar. 8 déc. 2020, à 10 h 43, Peter Kaestle <peter.kaestle@nokia.com> a écrit : > > -- 8< -- > > 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 think this paragraph could be removed as it's saying the same thing as the one below. > > 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. > > Signed-off-by: Peter Kaestle <peter.kaestle@nokia.com> > CC: Junio C Hamano <gitster@pobox.com> > CC: Philippe Blain <levraiphilippeblain@gmail.com> > CC: Ralf Thielow <ralf.thielow@gmail.com> > CC: Philippe Blain <levraiphilippeblain@gmail.com> > --- > submodule.c | 7 ++- > t/t5526-fetch-submodules.sh | 124 ++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 130 insertions(+), 1 deletion(-) > > diff --git a/submodule.c b/submodule.c > index b3bb59f..b561445 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); > } > } > > diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh > index dd8e423..495348a 100755 > --- a/t/t5526-fetch-submodules.sh > +++ b/t/t5526-fetch-submodules.sh > @@ -719,4 +719,128 @@ test_expect_success 'fetch new submodule commit intermittently referenced by sup > ) > ' > > +add_commit_push () { > + dir="$1" && > + msg="$2" && > + shift 2 && > + git -C "$dir" add "$@" && > + git -C "$dir" commit -a -m "$msg" && > + git -C "$dir" push > +} > + > +compare_refs_in_dir () { > + fail= && > + if test "x$1" = 'x!' > + then > + fail='!' && > + shift > + fi && > + git -C "$1" rev-parse --verify "$2" >expect && > + git -C "$3" rev-parse --verify "$4" >actual && > + eval $fail test_cmp expect actual > +} > + > + > +test_expect_success 'setup nested submodule fetch test' ' > + # does not depend on any previous test setups > + > + for repo in outer middle inner > + do > + git init --bare $repo && > + git clone $repo ${repo}_content && > + echo "$repo" >"${repo}_content/file" && > + add_commit_push ${repo}_content "initial" file || > + return 1 > + done && > + > + git clone outer A && > + git -C A submodule add "$pwd/middle" && > + git -C A/middle/ submodule add "$pwd/inner" && > + add_commit_push A/middle/ "adding inner sub" .gitmodules inner && > + add_commit_push A/ "adding middle sub" .gitmodules middle && > + > + git clone outer B && > + git -C B/ submodule update --init middle && > + > + compare_refs_in_dir A HEAD B HEAD && > + compare_refs_in_dir A/middle HEAD B/middle HEAD && > + test_path_is_file B/file && > + test_path_is_file B/middle/file && > + test_path_is_missing B/middle/inner/file && > + > + echo "change on inner repo of A" >"A/middle/inner/file" && > + add_commit_push A/middle/inner "change on inner" file && > + add_commit_push A/middle "change on inner" inner && > + add_commit_push A "change on inner" middle > +' > + > +test_expect_success 'fetching a superproject containing an uninitialized sub/sub project' ' > + # depends on previous test for setup > + > + git -C B/ fetch && > + compare_refs_in_dir A origin/HEAD B origin/HEAD > +' > + > +fetch_with_recusion_abort () { s/recusion/recursion/ > + # In a regression the following git call will run into infinite recursion. > + # To handle that, we connect the sed command to the git call by a pipe > + # so that sed can kill the infinite recusion when detected. s/recusion/recursion/ > + # The recursion creates git output like: > + # Fetching submodule sub > + # Fetching submodule sub/sub <-- [1] > + # Fetching submodule sub/sub/sub > + # ... > + # [1] sed will stop reading and cause git to eventually stop and die > + > + git -C "$1" fetch --recurse-submodules 2>&1 | > + sed "/Fetching submodule $2[^$]/q" >out && > + ! grep "Fetching submodule $2[^$]" out > +} > + > +test_expect_success 'setup recursive fetch with uninit submodule' ' > + # does not depend on any previous test setups > + > + # setup a remote superproject to make git fetch work with an uninit submodule > + git init --bare super_bare && > + git clone super_bare super && > + git init sub && > + > + >sub/file && > + git -C sub add file && > + git -C sub commit -m "add file" && > + git -C sub rev-parse HEAD >expect && > + > + # adding submodule without cloning > + echo "[submodule \"sub\"]" >super/.gitmodules && > + echo "path = sub" >>super/.gitmodules && > + echo "url = ../sub" >>super/.gitmodules && > + git -C super update-index --add --cacheinfo 160000 $(cat expect) sub && > + mkdir super/sub && > + > + git -C super submodule status >out && > + sed -e "s/^-//" -e "s/ sub.*$//" out >actual && > + test_cmp expect actual > +' I think this is overly complicated, what I was hinting at was adding the submodule in the superproject before cloning it, so something along these lines: test_create_repo super && test_commit -C super initial && test_create_repo sub && test_commit -C sub initial && git -C sub rev-parse HEAD >expect && git -C super submodule add ../sub && git -C super commit -m "add sub" && git clone super superclone && git -C superclone submodule status >out && sed -e "s/^-//" -e "s/ sub.*$//" out >actual && test_cmp expect actual And then running the two tests below in "superclone". > + > +test_expect_success 'recursive fetch with uninit submodule' ' > + # depends on previous test for setup > + > + fetch_with_recusion_abort super sub && s/recusion/recursion/ > + git -C super submodule status >out && > + sed -e "s/^-//" -e "s/ sub$//" out >actual && > + test_cmp expect actual > +' > + > +test_expect_success 'recursive fetch after deinit a submodule' ' > + # depends on previous test for setup > + > + git -C super submodule update --init sub && > + git -C super submodule deinit -f sub && > + > + fetch_with_recusion_abort super sub && s/recusion/recursion/ > + git -C super submodule status >out && > + sed -e "s/^-//" -e "s/ sub$//" out >actual && > + test_cmp expect actual > +' > + > test_done > -- > 2.6.2 > Philippe.
Hi Philippe, when sending the patch yesterday, I had the gut feeling to hold it back to double check it a day later. Should have done so, some of your findings were really stupid mistakes. On 09.12.20 00:25, Philippe Blain wrote: > Le mar. 8 déc. 2020, à 10 h 43, Peter Kaestle > <peter.kaestle@nokia.com> a écrit : >> >> -- 8< -- >> >> 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 think this paragraph could be removed as it's saying the same thing as > the one below. jip. [...] >> +fetch_with_recusion_abort () { > > s/recusion/recursion/ > yes. [...] >> +test_expect_success 'setup recursive fetch with uninit submodule' ' >> + # does not depend on any previous test setups >> + >> + # setup a remote superproject to make git fetch work with an uninit submodule >> + git init --bare super_bare && >> + git clone super_bare super && >> + git init sub && >> + >> + >sub/file && >> + git -C sub add file && >> + git -C sub commit -m "add file" && >> + git -C sub rev-parse HEAD >expect && >> + >> + # adding submodule without cloning >> + echo "[submodule \"sub\"]" >super/.gitmodules && >> + echo "path = sub" >>super/.gitmodules && >> + echo "url = ../sub" >>super/.gitmodules && >> + git -C super update-index --add --cacheinfo 160000 $(cat expect) sub && >> + mkdir super/sub && >> + >> + git -C super submodule status >out && >> + sed -e "s/^-//" -e "s/ sub.*$//" out >actual && >> + test_cmp expect actual >> +' > > I think this is overly complicated, what I was hinting at was adding > the submodule > in the superproject before cloning it, so something along these lines: > > test_create_repo super && > test_commit -C super initial && > test_create_repo sub && > test_commit -C sub initial && > git -C sub rev-parse HEAD >expect && > > git -C super submodule add ../sub && > git -C super commit -m "add sub" && > > git clone super superclone && > git -C superclone submodule status >out && > sed -e "s/^-//" -e "s/ sub.*$//" out >actual && > test_cmp expect actual > > And then running the two tests below in "superclone". Indeed, looks much simpler, I gave it a try run and it resulted in same behavior, will double check the logic of it and then send v4. Thanks a lot. [...]
diff --git a/submodule.c b/submodule.c index b3bb59f..b561445 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); } } diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh index dd8e423..495348a 100755 --- a/t/t5526-fetch-submodules.sh +++ b/t/t5526-fetch-submodules.sh @@ -719,4 +719,128 @@ test_expect_success 'fetch new submodule commit intermittently referenced by sup ) ' +add_commit_push () { + dir="$1" && + msg="$2" && + shift 2 && + git -C "$dir" add "$@" && + git -C "$dir" commit -a -m "$msg" && + git -C "$dir" push +} + +compare_refs_in_dir () { + fail= && + if test "x$1" = 'x!' + then + fail='!' && + shift + fi && + git -C "$1" rev-parse --verify "$2" >expect && + git -C "$3" rev-parse --verify "$4" >actual && + eval $fail test_cmp expect actual +} + + +test_expect_success 'setup nested submodule fetch test' ' + # does not depend on any previous test setups + + for repo in outer middle inner + do + git init --bare $repo && + git clone $repo ${repo}_content && + echo "$repo" >"${repo}_content/file" && + add_commit_push ${repo}_content "initial" file || + return 1 + done && + + git clone outer A && + git -C A submodule add "$pwd/middle" && + git -C A/middle/ submodule add "$pwd/inner" && + add_commit_push A/middle/ "adding inner sub" .gitmodules inner && + add_commit_push A/ "adding middle sub" .gitmodules middle && + + git clone outer B && + git -C B/ submodule update --init middle && + + compare_refs_in_dir A HEAD B HEAD && + compare_refs_in_dir A/middle HEAD B/middle HEAD && + test_path_is_file B/file && + test_path_is_file B/middle/file && + test_path_is_missing B/middle/inner/file && + + echo "change on inner repo of A" >"A/middle/inner/file" && + add_commit_push A/middle/inner "change on inner" file && + add_commit_push A/middle "change on inner" inner && + add_commit_push A "change on inner" middle +' + +test_expect_success 'fetching a superproject containing an uninitialized sub/sub project' ' + # depends on previous test for setup + + git -C B/ fetch && + compare_refs_in_dir A origin/HEAD B origin/HEAD +' + +fetch_with_recusion_abort () { + # In a regression the following git call will run into infinite recursion. + # To handle that, we connect the sed command to the git call by a pipe + # so that sed can kill the infinite recusion when detected. + # The recursion creates git output like: + # Fetching submodule sub + # Fetching submodule sub/sub <-- [1] + # Fetching submodule sub/sub/sub + # ... + # [1] sed will stop reading and cause git to eventually stop and die + + git -C "$1" fetch --recurse-submodules 2>&1 | + sed "/Fetching submodule $2[^$]/q" >out && + ! grep "Fetching submodule $2[^$]" out +} + +test_expect_success 'setup recursive fetch with uninit submodule' ' + # does not depend on any previous test setups + + # setup a remote superproject to make git fetch work with an uninit submodule + git init --bare super_bare && + git clone super_bare super && + git init sub && + + >sub/file && + git -C sub add file && + git -C sub commit -m "add file" && + git -C sub rev-parse HEAD >expect && + + # adding submodule without cloning + echo "[submodule \"sub\"]" >super/.gitmodules && + echo "path = sub" >>super/.gitmodules && + echo "url = ../sub" >>super/.gitmodules && + git -C super update-index --add --cacheinfo 160000 $(cat expect) sub && + mkdir super/sub && + + git -C super 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 + + fetch_with_recusion_abort super sub && + git -C super submodule status >out && + sed -e "s/^-//" -e "s/ sub$//" out >actual && + test_cmp expect actual +' + +test_expect_success 'recursive fetch after deinit a submodule' ' + # depends on previous test for setup + + git -C super submodule update --init sub && + git -C super submodule deinit -f sub && + + fetch_with_recusion_abort super sub && + git -C super submodule status >out && + sed -e "s/^-//" -e "s/ sub$//" out >actual && + test_cmp expect actual +' + test_done
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 repository with a submodule inside a submodule like this: superproject/middle_repo/inner_repo 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" at the superproject level, 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 submodule and skipped by the git fetch command. 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. 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. Signed-off-by: Peter Kaestle <peter.kaestle@nokia.com> CC: Junio C Hamano <gitster@pobox.com> CC: Philippe Blain <levraiphilippeblain@gmail.com> CC: Ralf Thielow <ralf.thielow@gmail.com> CC: Philippe Blain <levraiphilippeblain@gmail.com> --- submodule.c | 7 ++- t/t5526-fetch-submodules.sh | 124 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 130 insertions(+), 1 deletion(-)