Re: [PATCH 10/10] submodule deinit: complain when given a file instead of a submodule

From: Per Cederqvist <cederp@opera.com>
Date: May 2, 2016, 08:26 UTC
Message-ID: <CAP=KgsStNm7eUWRfzDPje8mAQ2hFCMJ6MpCeF_OgPvir244vgQ@mail.gmail.com>
In-Reply-To: <1461976845-18228-11-git-send-email-sbeller@google.com>

After this change, what is the simplest way to programmatically deinit any submodule that may exist, without failing if there are none?

"git commit" by default refuses to make an empty commit, but it has the --allow-empty option.

"git rm -r ." by default fails if there are no files in the repository, but it has the --ignore-unmatch option.

It makes sense that "git submodule deinit ." should fail if there are no submodules, but please add support for --ignore-unmatch at the same time.

    /ceder

On Sat, Apr 30, 2016 at 2:40 AM, Stefan Beller <sbeller@google.com> wrote:

Show 70 quoted lines

> This also improves performance for listing submodules, because
> S_ISGITLINK is both faster as match_pathspec as well as expected to
> be true in fewer cases, so putting it first in the condition will speed
> up the loop to compute all submodules.
>
> As this partially reverts 84ba959bbdf0 (submodule: fix regression for
> deinit without submodules, 2016-03-22), this also disallows the use
> of `git submodule deinit .` to deinit all submodules, when no
> submodules are present. `deinit .` continues to work on repositories,
> which have at least one submodule.
>
> CC: Per Cederqvist <cederp@opera.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>
>
>> Patch 10 is a controversial thing I'd assume as it breaks existing users.
>> We should take it for the next major release (i.e. 3.0)
>> I just want to put it out here now.
>
>  builtin/submodule--helper.c |  6 +++---
>  t/t7400-submodule-basic.sh  | 15 ++++++++++++++-
>  2 files changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 7f0941d..e41de3e 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -242,9 +242,9 @@ static int module_list_compute(int argc, const char **argv,
>         for (i = 0; i < active_nr; i++) {
>                 const struct cache_entry *ce = active_cache[i];
>
> -               if (!match_pathspec(pathspec, ce->name, ce_namelen(ce),
> -                                   0, ps_matched, 1) ||
> -                   !S_ISGITLINK(ce->ce_mode))
> +               if (!S_ISGITLINK(ce->ce_mode) ||
> +                   !match_pathspec(pathspec, ce->name, ce_namelen(ce),
> +                                   0, ps_matched, 1))
>                         continue;
>
>                 ALLOC_GROW(list->entries, list->nr + 1, list->alloc);
> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> index 53644da..361e6f6 100755
> --- a/t/t7400-submodule-basic.sh
> +++ b/t/t7400-submodule-basic.sh
> @@ -915,7 +915,20 @@ test_expect_success 'submodule deinit works on repository without submodules' '
>                 >file &&
>                 git add file &&
>                 git commit -m "repo should not be empty" &&
> -               git submodule deinit .
> +               git submodule deinit
> +       )
> +'
> +
> +test_expect_success 'submodule deinit refuses to deinit a file' '
> +       test_when_finished "rm -rf newdirectory" &&
> +       mkdir newdirectory &&
> +       (
> +               cd newdirectory &&
> +               git init &&
> +               >file &&
> +               git add file &&
> +               git commit -m "repo should not be empty" &&
> +               test_must_fail git submodule deinit file
>         )
>  '
>
> --
> 2.8.0.32.g71f8beb.dirty
>