Re: [PATCH 1/2] submodule--helper: set alternateLocation for cloned submodules

From: Stefan Beller <sbeller@google.com>
Date: Dec 8, 2016, 00:02 UTC
Message-ID: <CAGZ79kaSVcf1zk4P1hWsU=aJG7G99qohhavBftmPxnrkjW+nfQ@mail.gmail.com>
In-Reply-To: <20161207224948.7957-1-vi0oss@gmail.com>

On Wed, Dec 7, 2016 at 2:49 PM,  <vi0oss@gmail.com> wrote:

> Notes:
>     Resolved issues pointed by Stefan Beller except of
>     the one about loosened path check, which he aggreed
>     to be relaxed for this case.

I am sorry to have given an incomplete review at the first time. :/ More below.

Show 6 quoted lines

> +       /* setup alternateLocation and alternateErrorStrategy in the cloned submodule if needed */
> +       git_config_get_string("submodule.alternateLocation", &sm_alternate);
> +       if (sm_alternate) {
> +               git_config_set_in_file(p, "submodule.alternateLocation",
> +                                          sm_alternate);
> +       }

For a single command, we usually omit the braces for an if clause, i.e.

    if (foo)
        bar(...);

    /* code goes on */

Show 9 quoted lines

> +       git_config_get_string("submodule.alternateErrorStrategy", &error_strategy);
> +       if (error_strategy) {
> +               git_config_set_in_file(p, "submodule.alternateErrorStrategy",
> +                                          error_strategy);
> +       }
> +
> +               free(sm_alternate);
> +               free(error_strategy);
> +

The indentation seems a bit off for the free here? (The main nit that motivated me writing the email)

Show 81 quoted lines

>         strbuf_release(&sb);
>         strbuf_release(&rel_path);
>         free(sm_gitdir);
> diff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh
> index 1c1e289..ef7771b 100755
> --- a/t/t7408-submodule-reference.sh
> +++ b/t/t7408-submodule-reference.sh
> @@ -125,4 +125,76 @@ test_expect_success 'ignoring missing submodule alternates passes clone and subm
>         )
>  '
>
> +test_expect_success 'preparing second superproject with a nested submodule' '
> +       test_create_repo supersuper &&
> +       (
> +               cd supersuper &&
> +               echo "I am super super." >file &&
> +               git add file &&
> +               git commit -m B-super-super-initial
> +               git submodule add "file://$base_dir/super" subwithsub &&
> +               git commit -m B-super-super-added &&
> +               git submodule update --init --recursive &&
> +               git repack -ad
> +       )
> +'
> +
> +# At this point there are three root-level positories: A, B, super and super2
> +
> +test_expect_success 'nested submodule alternate in works and is actually used' '
> +       test_when_finished "rm -rf supersuper-clone" &&
> +       git clone --recursive --reference supersuper supersuper supersuper-clone &&
> +       (
> +               cd supersuper-clone &&
> +               # test superproject has alternates setup correctly
> +               test_alternate_is_used .git/objects/info/alternates . &&
> +               # immediate submodule has alternate:
> +               test_alternate_is_used .git/modules/subwithsub/objects/info/alternates subwithsub
> +               # nested submodule also has alternate:
> +               test_alternate_is_used .git/modules/subwithsub/modules/sub/objects/info/alternates subwithsub/sub
> +       )
> +'
> +
> +test_expect_success 'missing nested submodule alternate fails clone and submodule update' '
> +       test_when_finished "rm -rf supersuper-clone supersuper2" &&
> +       git clone supersuper supersuper2 &&
> +       (
> +               cd supersuper2 &&
> +               git submodule update --init
> +       ) &&
> +       test_must_fail git clone --recursive --reference supersuper2 supersuper2 supersuper-clone &&
> +       (
> +               cd supersuper-clone &&
> +               # test superproject has alternates setup correctly
> +               test_alternate_is_used .git/objects/info/alternates . &&
> +               # update of the submodule fails
> +               test_must_fail git submodule update --init --recursive &&
> +               # immediate submodule has alternate:
> +               test_alternate_is_used .git/modules/subwithsub/objects/info/alternates subwithsub
> +               # but nested submodule has no alternate:
> +               test_must_fail test_alternate_is_used .git/modules/subwithsub/modules/sub/objects/info/alternates subwithsub/sub
> +       )
> +'
> +
> +test_expect_success 'missing nested submodule alternate in --reference-if-able mode' '
> +       test_when_finished "rm -rf supersuper-clone supersuper2" &&
> +       git clone supersuper supersuper2 &&
> +       (
> +               cd supersuper2 &&
> +               git submodule update --init
> +       ) &&
> +       git clone --recursive --reference-if-able supersuper2 supersuper2 supersuper-clone &&
> +       (
> +               cd supersuper-clone &&
> +               # test superproject has alternates setup correctly
> +               test_alternate_is_used .git/objects/info/alternates . &&
> +               # update of the submodule fails
> +               test_must_fail git submodule update --init --recursive &&
> +               # immediate submodule has alternate:
> +               test_alternate_is_used .git/modules/subwithsub/objects/info/alternates subwithsub
> +               # but nested submodule has no alternate:
> +               test_must_fail test_alternate_is_used .git/modules/subwithsub/modules/sub/objects/info/alternates subwithsub/sub
> +       )

Both the first and the last part are the same in the last two tests, the only difference is the line with git clone --reference ... Maybe we can use a function somehow to make this a bit more obvious?

Otherwise the tests look good to me.

Thanks, Stefan