Re: [PATCH v3] submodules: fix of regression on fetching of non-init subsub-repo
From: Philippe Blain <hidden>
Date: 2020-12-08 23:26:40
Possibly related (same subject, not in this thread)
- 2020-12-08 · Re: [PATCH v3] submodules: fix of regression on fetching of non-init subsub-repo · Junio C Hamano <hidden>
Hi Peter, Le mar. 8 déc. 2020, à 10 h 43, Peter Kaestle [off-list ref] 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.
quoted hunk ↗ jump to hunk
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 <redacted> CC: Junio C Hamano <redacted> CC: Philippe Blain <redacted> CC: Ralf Thielow <redacted> CC: Philippe Blain <redacted> --- 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.