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

On Mon, May 2, 2016 at 1:26 AM, Per Cederqvist [off-list ref] wrote:

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.

Oh right. I'll add the --ignore-unmatch option when rerolling this series.

Thanks,
Stefan

    /ceder


On Sat, Apr 30, 2016 at 2:40 AM, Stefan Beller [off-list ref] wrote:

quoted

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 <redacted>
Signed-off-by: Stefan Beller <redacted>
---

quoted

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