Hi Peter,
Le mar. 8 déc. 2020, à 10 h 43, Peter Kaestle
<peter.kaestle@xxxxxxxxx> 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@xxxxxxxxx>
> CC: Junio C Hamano <gitster@xxxxxxxxx>
> CC: Philippe Blain <levraiphilippeblain@xxxxxxxxx>
> CC: Ralf Thielow <ralf.thielow@xxxxxxxxx>
> CC: Philippe Blain <levraiphilippeblain@xxxxxxxxx>
> ---
> 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.
