Re: [PATCH v3 4/5] clone: add a --no-tags-submodules to pass --no-tags to submodules

From: Stefan Beller <sbeller@google.com>
Date: Apr 27, 2017, 18:27 UTC
Message-ID: <CAGZ79kbsRv058PVwaWNtxKOH2LFnGk=VFTtt39FTqujkbz5E5A@mail.gmail.com>
In-Reply-To: <20170426231236.27219-5-avarab@gmail.com>

On Wed, Apr 26, 2017 at 4:12 PM, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 9 quoted lines

> From: Brandon Williams <bmwill@google.com>
>
> Add a --no-tags-submodules which does for --no-tags what the existing
> --shallow-submodules does for --depth, i.e. doing:
>
>     git clone --recurse-submodules --no-tags --no-tags-submodules <url>
>
> Will clone the superproject and all submodules with --no-tags
> semantics.

My gut reaction to this is that it is ugly and maybe we should rather have a --no-tags[=<repo-spec>].

We have had the discussion for e.g. git-push, for which we would like to have a way to specify to push the superproject, or some submodules or all of them, or just the superproject and changed submodules, such that there we have a "git push --recurse-submodules= \ [check|on-demand|only|no]" which comes close to what I mean by "repo-spec".

> This change does not implement a submodule.*.tags config .gitmodules
> configuration option corresponding to the existing submodule.*.shallow
> facility, which would make --no-tags have full feature parity with
> --shallow-submodules.

Okay, that can be added later if desired.

Show 7 quoted lines

> Signed-off-by: Brandon Williams <bmwill@google.com>
> Code-by: Brandon Williams <bmwill@google.com>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Commit-message-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Git-Completion-Code-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Docs-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Tests-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

Quite a lot of collaboration. ;)

Show 6 quoted lines

> @@ -499,6 +499,8 @@ static int clone_submodule(const char *path, const char *gitdir, const char *url
>                 argv_array_push(&cp.args, "--quiet");
>         if (progress)
>                 argv_array_push(&cp.args, "--progress");
> +       if (no_tags)
> +               argv_array_push(&cp.args, "--no-tags");

Here you would need to also push --no-tags-submodules to keep it recursive?

Show 5 quoted lines

> diff --git a/t/t5616-clone-submodules-tags.sh b/t/t5616-clone-submodules-tags.sh
> new file mode 100755
> index 0000000000..3c88265352
> --- /dev/null
> +++ b/t/t5616-clone-submodules-tags.sh

Name is good as it describes the niche we're looking at. (previous commit sounded as if you wanted to introduce a plain clone-submodules.sh)

Show 21 quoted lines

> @@ -0,0 +1,72 @@
> +#!/bin/sh
> +
> +test_description='Test cloning of repos with submodules & the --[no-]tags option'
> +
> +. ./test-lib.sh
> +
> +pwd=$(pwd)
> +
> +test_expect_success 'setup' '
> +       git checkout -b master &&
> +       test_commit commit1 &&
> +       test_commit commit2 &&
> +       mkdir sub &&
> +       (
> +               cd sub &&
> +               git init &&
> +               test_commit subcommit1 &&
> +               test_commit subcommit2 &&
> +               test_commit subcommit3
> +       ) &&

This can be written easier with

    test_create_repo sub &&
    test_commit -C sub subcommit1 &&
    test_commit -C sub subcommit2 &&
    test_commit -C sub subcommit3 &&

Most submodule code thought naming a submodule "sub" was a good idea. I also wrote such code. But please let's stop with this tradition. Name the submodule after your favorite dish (that hopefully doesn't have "sub" in its name), as it is much easier to read the test code when there are fewer strings "sub".

Show 9 quoted lines

> +       git submodule add "file://$pwd/sub" sub &&
> +       git commit -m "add submodule" &&
> +       git tag addsubcommit1
> +'
> +
> +test_expect_success 'tags clone implies tags submodule' '
> +       test_when_finished "rm -rf super_clone" &&
> +       git clone --recurse-submodules "file://$pwd/." super_clone &&
> +       git -C super_clone for-each-ref --format="%(refname:strip=2)" refs/tags/ >tags &&

Why do we need to strip the refnames here? Full ref names ought to be fine? Specifically when testing in combination of single-branch (does that imply any tags setting), I'd rather expect an rstrip and then counting refs/heads and refs/tags.

Thanks, Stefan