git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v3] submodules: fix of regression on fetching of non-init subsub-repo

From
Philippe Blain <levraiphilippeblain@gmail.com>
Date
Dec 8, 2020, 23:25 UTC
Message-ID
<CADtb9DwwfAG69YmM0+0FC6qkO361-95Uy8dNZ8jXnrVcxHSrMQ@mail.gmail.com>
In-Reply-To
<1607442126-34705-1-git-send-email-peter.kaestle@nokia.com>
Hi Peter,

Le mar. 8 déc. 2020, à 10 h 43, Peter Kaestle <peter.kaestle@nokia.com> a écrit :

Show 6 quoted lines
>
>  -- 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.

Show 123 quoted lines
>
> 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/
Show 36 quoted lines
> +       # 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".
Show 5 quoted lines
> +
> +test_expect_success 'recursive fetch with uninit submodule' '
> +       # depends on previous test for setup
> +
> +       fetch_with_recusion_abort super sub &&
s/recusion/recursion/
Show 12 quoted lines
> +       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/
Show 9 quoted lines
> +       git -C super submodule status >out &&
> +       sed -e "s/^-//" -e "s/ sub$//" out >actual &&
> +       test_cmp expect actual
> +'
> +
>  test_done
> --
> 2.6.2
>
Philippe.
Previous: Junio C HamanoNext: Peter Kästle
Message 26 of 36 in “BUG in fetching non-checked out submodule”
  1. Ralf ThielowDec 2, 2020
  2. Philippe BlainDec 2, 2020
  3. Junio C HamanoDec 2, 2020
  4. Peter KästleDec 3, 2020
  5. Philippe BlainDec 3, 2020
  6. Peter KästleDec 3, 2020
  7. Junio C HamanoDec 3, 2020
  8. submodules: fix of regression on fetching of non-init subsub-repoPeter Kaestle, Dec 4, 2020
  9. Eric SunshineDec 4, 2020
  10. Peter KästleDec 7, 2020
  11. Eric SunshineDec 7, 2020
  12. submodules: fix of regression on fetching of non-init subsub-repoPeter Kaestle, Dec 7, 2020
  13. Philippe BlainDec 7, 2020
  14. Junio C HamanoDec 7, 2020
  15. Peter KästleDec 8, 2020
  16. Junio C HamanoDec 7, 2020
  17. Peter KästleDec 8, 2020
  18. Junio C HamanoDec 7, 2020
  19. Philippe BlainDec 7, 2020
  20. Junio C HamanoDec 7, 2020
  21. Junio C HamanoDec 7, 2020
  22. Peter KästleDec 8, 2020
  23. submodules: fix of regression on fetching of non-init subsub-repoPeter Kaestle, Dec 8, 2020
  24. Peter KästleDec 8, 2020
  25. Junio C HamanoDec 8, 2020
  26. Philippe BlainDec 8, 2020
  27. Peter KästleDec 9, 2020
  28. submodules: fix of regression on fetching of non-init subsub-repoPeter Kaestle, Dec 9, 2020
  29. Philippe BlainDec 9, 2020
  30. Ralf ThielowDec 3, 2020
  31. Peter KästleDec 3, 2020
  32. Ralf ThielowDec 3, 2020
  33. Peter KästleDec 3, 2020
  34. Ralf ThielowDec 3, 2020
  35. Peter KästleDec 3, 2020
  36. Ralf ThielowDec 3, 2020

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.