threads / patch / 42212

patchsubmodule deinit: require '--all' instead of '.' for all submodules

Subject: [PATCH] submodule deinit: require '--all' instead of '.' for all submodules

## tl;dr

17 messages between May 3, 2016 and May 4, 2016. Diffs are folded; open one to read it.

replies: 16people: 3as markdown or json

Stefan Beller· May 3, 2016, 22:11 UTC · lore

The discussion in [1] realized that '.' is a faulty suggestion as there is a corner case where it fails:

Show 14 quoted lines
> "submodule deinit ." may have "worked" in the sense that you would
> have at least one path in your tree and avoided this "nothing
> matches" most of the time.  It would have still failed with the
> exactly same error if run in an empty repository, i.e.
>
>        $ E=/var/tmp/x/empty && rm -fr "$E" && mkdir -p "$E" && cd "$E"
>        $ git init
>        $ rungit v2.6.6 submodule deinit .
>        error: pathspec '.' did not match any file(s) known to git.
>        Did you forget to 'git add'?
>        $ >file && git add file
>        $ rungit v2.6.6 submodule deinit .
>        $ echo $?
>        0

So instead of a path spec add a parameter '--all' to deinit all submodules and add a test to check for the corner case of an empty repository.

There is no need to update the documentation as it did not describe the special case '.' to remove all submodules.

The code only needs to learn about the '--all' parameter and doesn't require further changes as `git submodule--helper list "$@"` will list all submodules in that case.

[1] http://news.gmane.org/gmane.comp.version-control.git/289535
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
This is the result of the discussion, I would think.
I developed it on top of 
    "submodule deinit test: fix broken && chain in subshell"
    on top of 2a4c8c36a7f6, 2016-03-24, Merge branch
    'sb/submodule-module-list-pathspec-fix'
but I think this would rather go in as a new feature, not on top
of a bugfix topic, so I think this could go on origin/master ?

Thanks, Stefan

 git-submodule.sh           | 12 ++++++++++--
 t/t7400-submodule-basic.sh |  4 ++++
 2 files changed, 14 insertions(+), 2 deletions(-)
Show changes to 2 files +14 −2

git-submodule.sh, t/t7400-submodule-basic.sh

diff --git a/git-submodule.sh b/git-submodule.sh
index 43c68de..301cd0d 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -521,6 +521,7 @@ cmd_init()
 cmd_deinit()
 {
 	# parse $args after "submodule ... deinit".
+	deinit_all=
 	while test $# -ne 0
 	do
 		case "$1" in
@@ -530,6 +531,9 @@ cmd_deinit()
 		-q|--quiet)
 			GIT_QUIET=1
 			;;
+		-a|--all)
+			deinit_all=t
+			;;
 		--)
 			shift
 			break
@@ -544,9 +548,13 @@ cmd_deinit()
 		shift
 	done
 
-	if test $# = 0
+	if test -n "$deinit_all" && test "$#" -ne 0
+	then
+		die "$(eval_gettext "'--all' is incompatible with pathspec arguments")"
+	fi
+	if test $# = 0 && test -z "$deinit_all"
 	then
-		die "$(eval_gettext "Use '.' if you really want to deinitialize all submodules")"
+		die "$(eval_gettext "Use '--all' if you really want to deinitialize all submodules")"
 	fi
 
 	git submodule--helper list --prefix "$wt_prefix" "$@" |
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index cf06b2f..a5b0dec 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -11,6 +11,10 @@ subcommands of git submodule.
 
 . ./test-lib.sh
 
+test_expect_success 'submodule deinit works on empty repository' '
+	git submodule deinit --all
+'
+
 test_expect_success 'setup - initial commit' '
 	>t &&
 	git add t &&

-- 
2.8.0.rc4.10.geb92688.dirty
Junio C Hamano· May 3, 2016, 22:25 UTC · re: Stefan Beller · lore

Re: [PATCH] submodule deinit: require '--all' instead of '.' for all submodules

Stefan Beller <sbeller@google.com> writes:
Show 6 quoted lines
> ...
> So instead of a path spec add a parameter '--all' to deinit all submodules
> and add a test to check for the corner case of an empty repository.
>
> There is no need to update the documentation as it did not describe the
> special case '.' to remove all submodules.
Now there is, isn't it?
Junio C Hamano· May 3, 2016, 22:28 UTC · re: Stefan Beller · lore

Re: [PATCH] submodule deinit: require '--all' instead of '.' for all submodules

Stefan Beller <sbeller@google.com> writes:
Show 6 quoted lines
> I developed it on top of 
>     "submodule deinit test: fix broken && chain in subshell"
>     on top of 2a4c8c36a7f6, 2016-03-24, Merge branch
>     'sb/submodule-module-list-pathspec-fix'
> but I think this would rather go in as a new feature, not on top
> of a bugfix topic, so I think this could go on origin/master ?

I do not particularly view it as a new feature. The way the old message suggested did not work in a pathological corner case, but we wanted to keep the "force user to be explicit when doing mass destruction", and a fix we happened to chose requires addition of a new option--that would still look like a fix to me.

It is not like we are forbidding the use of "submodule deinit ." that used to work in a tree with at least one tracked path. With the change, a script that has such a command will continue to work, no?

Stefan Beller· May 3, 2016, 22:43 UTC · re: Junio C Hamano · lore

Re: [PATCH] submodule deinit: require '--all' instead of '.' for all submodules

On Tue, May 3, 2016 at 3:28 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 19 quoted lines
> Stefan Beller <sbeller@google.com> writes:
>
>> I developed it on top of
>>     "submodule deinit test: fix broken && chain in subshell"
>>     on top of 2a4c8c36a7f6, 2016-03-24, Merge branch
>>     'sb/submodule-module-list-pathspec-fix'
>> but I think this would rather go in as a new feature, not on top
>> of a bugfix topic, so I think this could go on origin/master ?
>
> I do not particularly view it as a new feature.  The way the old
> message suggested did not work in a pathological corner case, but we
> wanted to keep the "force user to be explicit when doing mass
> destruction", and a fix we happened to chose requires addition of a
> new option--that would still look like a fix to me.
>
> It is not like we are forbidding the use of "submodule deinit ."
> that used to work in a tree with at least one tracked path.  With
> the change, a script that has such a command will continue to work,
> no?
Maybe.
With just this patch, yes.

I'd like to revert submodule-module-list-pathspec-fix partially when redoing the groups support. That would break the '.' script case. So eventually scripts want to use

    git submodule deinit -f --all
instead of
    git submodule deinit -f .

When implementing the groups support, I'd change module_list in a way that you can give names, paths, or labels to it. In case of a user gives 'COPYIN*' we'd want to know if that is a path (or a name, label) or bogus, so I think we'd tighten the checks there just for the functionality not just performance as originally anticipated for the order of S_ISGITLINK and match_pathspec.

So eventually (i.e. after the submodule groups lands) "submodule deinit ." will start acting weird again?

Junio C Hamano· May 3, 2016, 22:52 UTC · re: Stefan Beller · lore

Re: [PATCH] submodule deinit: require '--all' instead of '.' for all submodules

Stefan Beller <sbeller@google.com> writes:
> So eventually (i.e. after the submodule groups lands)
> "submodule deinit ." will start acting weird again?

It would be nice if it never acts in a weird way, but that is all future development, not related to this fix, no?

Stefan Beller· May 3, 2016, 23:02 UTC · re: Junio C Hamano · lore

Re: [PATCH] submodule deinit: require '--all' instead of '.' for all submodules

On Tue, May 3, 2016 at 3:52 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 7 quoted lines
> Stefan Beller <sbeller@google.com> writes:
>
>> So eventually (i.e. after the submodule groups lands)
>> "submodule deinit ." will start acting weird again?
>
> It would be nice if it never acts in a weird way, but that is all
> future development, not related to this fix, no?

Yes, just this patch is fine. Though lacking documentation. I'll send an update.

Stefan Beller· May 4, 2016, 00:33 UTC · re: Stefan Beller · lore

[PATCHv2] submodule deinit: require '--all' instead of '.' for all submodules

The discussion in [1] realized that '.' is a faulty suggestion as there is a corner case where it fails:

Show 14 quoted lines
> "submodule deinit ." may have "worked" in the sense that you would
> have at least one path in your tree and avoided this "nothing
> matches" most of the time.  It would have still failed with the
> exactly same error if run in an empty repository, i.e.
>
>        $ E=/var/tmp/x/empty && rm -fr "$E" && mkdir -p "$E" && cd "$E"
>        $ git init
>        $ rungit v2.6.6 submodule deinit .
>        error: pathspec '.' did not match any file(s) known to git.
>        Did you forget to 'git add'?
>        $ >file && git add file
>        $ rungit v2.6.6 submodule deinit .
>        $ echo $?
>        0

So instead of a path spec add a parameter '--all' to deinit all submodules and add a test to check for the corner case of an empty repository.

The code only needs to learn about the '--all' parameter and doesn't require further changes as `git submodule--helper list "$@"` will list all submodules in case of "$@" being empty.

[1] http://news.gmane.org/gmane.comp.version-control.git/289535
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
Junio writes:
Show 5 quoted lines
> I do not particularly view it as a new feature.  The way the old
> message suggested did not work in a pathological corner case, but we
> wanted to keep the "force user to be explicit when doing mass
> destruction", and a fix we happened to chose requires addition of a
> new option--that would still look like a fix to me.

It is a fix for a corner case, but it is renaming the switch, so I would expect that we'd wait for a new version at least.

> It is not like we are forbidding the use of "submodule deinit ."
> that used to work in a tree with at least one tracked path.  With
> the change, a script that has such a command will continue to work,
> no?

But we are also not garantueeing it either? Before this patch "submodule deinit ." was the blessed way to deinit all submodules. Sure, `deinit "*"` may have worked as well, but that was never asserted via tests internally nor via documentation to the user.

So if someone was yelling at us because of a bug, we'd fix it if it was related to "deinit .", but not if it was "deinit '*'".

This change both gives guidance in the documentation on how to deinit all submodules as well as removing the tests for the '.' case (by replacing them with the 'all' case).

So I'd prefer if we'd spin this as a feature rather than a bug fix.

This was developed on 2a4c8c36a7f6ad, 2016-03-24 Merge branch 'sb/submodule-module-list-pathspec-fix'

Thanks, Stefan

 Documentation/git-submodule.txt | 12 +++++++++++-
 git-submodule.sh                | 12 ++++++++++--
 t/t7400-submodule-basic.sh      | 14 +++++++++-----
 3 files changed, 30 insertions(+), 8 deletions(-)
Show changes to 3 files +30 −8

Documentation/git-submodule.txt, git-submodule.sh, t/t7400-submodule-basic.sh

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index 1572f05..4dbf8d0 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -13,7 +13,7 @@ SYNOPSIS
 	      [--reference <repository>] [--depth <depth>] [--] <repository> [<path>]
 'git submodule' [--quiet] status [--cached] [--recursive] [--] [<path>...]
 'git submodule' [--quiet] init [--] [<path>...]
-'git submodule' [--quiet] deinit [-f|--force] [--] <path>...
+'git submodule' [--quiet] deinit [-f|--force] [-a|--all] [--] <path>...
 'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]
 	      [-f|--force] [--rebase|--merge] [--reference <repository>]
 	      [--depth <depth>] [--recursive] [--] [<path>...]
@@ -144,6 +144,11 @@ deinit::
 	you really want to remove a submodule from the repository and commit
 	that use linkgit:git-rm[1] instead.
 +
+	To unregister all submodules use `--all` with no path spec. In
+	version 2.8 and prior, you were advised to give '.' to unregister
+	all submodules. This may continue to work in the future, but as the
+	path spec '.' may include regular files, this could stop working.
++
 If `--force` is specified, the submodule's work tree will be removed even if
 it contains local modifications.
 
@@ -247,6 +252,11 @@ OPTIONS
 --quiet::
 	Only print error messages.
 
+-a::
+--all::
+	This option is only valid for the deinit command. Unregister all
+	submodules in the work tree.
+
 -b::
 --branch::
 	Branch of repository to add as submodule.
diff --git a/git-submodule.sh b/git-submodule.sh
index 43c68de..301cd0d 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -521,6 +521,7 @@ cmd_init()
 cmd_deinit()
 {
 	# parse $args after "submodule ... deinit".
+	deinit_all=
 	while test $# -ne 0
 	do
 		case "$1" in
@@ -530,6 +531,9 @@ cmd_deinit()
 		-q|--quiet)
 			GIT_QUIET=1
 			;;
+		-a|--all)
+			deinit_all=t
+			;;
 		--)
 			shift
 			break
@@ -544,9 +548,13 @@ cmd_deinit()
 		shift
 	done
 
-	if test $# = 0
+	if test -n "$deinit_all" && test "$#" -ne 0
+	then
+		die "$(eval_gettext "'--all' is incompatible with pathspec arguments")"
+	fi
+	if test $# = 0 && test -z "$deinit_all"
 	then
-		die "$(eval_gettext "Use '.' if you really want to deinitialize all submodules")"
+		die "$(eval_gettext "Use '--all' if you really want to deinitialize all submodules")"
 	fi
 
 	git submodule--helper list --prefix "$wt_prefix" "$@" |
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index e1abd19..6e28ea5 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -11,6 +11,10 @@ subcommands of git submodule.
 
 . ./test-lib.sh
 
+test_expect_success 'submodule deinit works on empty repository' '
+	git submodule deinit --all
+'
+
 test_expect_success 'setup - initial commit' '
 	>t &&
 	git add t &&
@@ -858,7 +862,7 @@ 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 --all
 	)
 '
 
@@ -887,12 +891,12 @@ test_expect_success 'submodule deinit from subdirectory' '
 	rmdir init
 '
 
-test_expect_success 'submodule deinit . deinits all initialized submodules' '
+test_expect_success 'submodule deinit --all deinits all initialized submodules' '
 	git submodule update --init &&
 	git config submodule.example.foo bar &&
 	git config submodule.example2.frotz nitfol &&
 	test_must_fail git submodule deinit &&
-	git submodule deinit . >actual &&
+	git submodule deinit --all >actual &&
 	test -z "$(git config --get-regexp "submodule\.example\.")" &&
 	test -z "$(git config --get-regexp "submodule\.example2\.")" &&
 	test_i18ngrep "Cleared directory .init" actual &&
@@ -958,11 +962,11 @@ test_expect_success 'submodule deinit is silent when used on an uninitialized su
 	git submodule deinit init >actual &&
 	test_i18ngrep ! "Submodule .example. (.*) unregistered for path .init" actual &&
 	test_i18ngrep "Cleared directory .init" actual &&
-	git submodule deinit . >actual &&
+	git submodule deinit --all >actual &&
 	test_i18ngrep ! "Submodule .example. (.*) unregistered for path .init" actual &&
 	test_i18ngrep "Submodule .example2. (.*) unregistered for path .example2" actual &&
 	test_i18ngrep "Cleared directory .init" actual &&
-	git submodule deinit . >actual &&
+	git submodule deinit --all >actual &&
 	test_i18ngrep ! "Submodule .example. (.*) unregistered for path .init" actual &&
 	test_i18ngrep ! "Submodule .example2. (.*) unregistered for path .example2" actual &&
 	test_i18ngrep "Cleared directory .init" actual &&

-- 
2.8.0.rc4.10.geb92688.dirty
Jonathan Nieder· May 4, 2016, 00:47 UTC · re: Stefan Beller · lore

Re: [PATCHv2] submodule deinit: require '--all' instead of '.' for all submodules

Stefan Beller wrote:
[...]
Show 10 quoted lines
>>        $ rungit v2.6.6 submodule deinit .
>>        error: pathspec '.' did not match any file(s) known to git.
>>        Did you forget to 'git add'?
>>        $ >file && git add file
>>        $ rungit v2.6.6 submodule deinit .
>>        $ echo $?
>>        0
>
> So instead of a path spec add a parameter '--all' to deinit all submodules
> and add a test to check for the corner case of an empty repository.
Makes sense.
[...]
> It is a fix for a corner case, but it is renaming the switch, so I would
> expect that we'd wait for a new version at least.

The bug was that the documentation and error message gave advice that didn't work.

[...]
Show 21 quoted lines
> +++ b/Documentation/git-submodule.txt
> @@ -13,7 +13,7 @@ SYNOPSIS
>  	      [--reference <repository>] [--depth <depth>] [--] <repository> [<path>]
>  'git submodule' [--quiet] status [--cached] [--recursive] [--] [<path>...]
>  'git submodule' [--quiet] init [--] [<path>...]
> -'git submodule' [--quiet] deinit [-f|--force] [--] <path>...
> +'git submodule' [--quiet] deinit [-f|--force] [-a|--all] [--] <path>...
>  'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]
>  	      [-f|--force] [--rebase|--merge] [--reference <repository>]
>  	      [--depth <depth>] [--recursive] [--] [<path>...]
> @@ -144,6 +144,11 @@ deinit::
>  	you really want to remove a submodule from the repository and commit
>  	that use linkgit:git-rm[1] instead.
>  +
> +	To unregister all submodules use `--all` with no path spec. In
> +	version 2.8 and prior, you were advised to give '.' to unregister
> +	all submodules. This may continue to work in the future, but as the
> +	path spec '.' may include regular files, this could stop working.
> ++
>  If `--force` is specified, the submodule's work tree will be removed even if
>  it contains local modifications.
Inconsistent indentation.  Does asciidoc format this correctly?
[...]
> +++ b/git-submodule.sh
> @@ -521,6 +521,7 @@ cmd_init()
[...]
Show 8 quoted lines
>  		shift
>  	done
>  
> -	if test $# = 0
> +	if test -n "$deinit_all" && test "$#" -ne 0
> +	then
> +		die "$(eval_gettext "'--all' is incompatible with pathspec arguments")"
> +	fi

This message is giving implementation details. Most other git commands just dump usage when passed too many arguments --- perhaps we can do something more targeted like the following:

	die "$(eval_gettext "usage: $dashless [--quiet] deinit [-f|--force] (--all | [--] <path>...)")"

Speaking of which: please also update the USAGE string at the top of git-submodule.sh.

[...]
> --- a/t/t7400-submodule-basic.sh
> +++ b/t/t7400-submodule-basic.sh
Makes sense.
Thanks for a pleasant fix.
Jonathan
Stefan Beller· May 4, 2016, 01:19 UTC · re: Stefan Beller · lore

[PATCHv3] submodule deinit: require '--all' instead of '.' for all submodules

The discussion in [1] realized that '.' is a faulty suggestion as there is a corner case where it fails:

Show 14 quoted lines
> "submodule deinit ." may have "worked" in the sense that you would
> have at least one path in your tree and avoided this "nothing
> matches" most of the time.  It would have still failed with the
> exactly same error if run in an empty repository, i.e.
>
>        $ E=/var/tmp/x/empty && rm -fr "$E" && mkdir -p "$E" && cd "$E"
>        $ git init
>        $ rungit v2.6.6 submodule deinit .
>        error: pathspec '.' did not match any file(s) known to git.
>        Did you forget to 'git add'?
>        $ >file && git add file
>        $ rungit v2.6.6 submodule deinit .
>        $ echo $?
>        0

So instead of a path spec add a parameter '--all' to deinit all submodules and add a test to check for the corner case of an empty repository.

The code only needs to learn about the '--all' parameter and doesn't require further changes as `git submodule--helper list "$@"` will list all submodules in case of "$@" being empty.

[1] http://news.gmane.org/gmane.comp.version-control.git/289535
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
Changes since v2:
* dedented section in documentation for --all, as it broke asciidoc.
* Using a consistent usage string in git-modules.sh (both at top as well as in
  the die message) as well as in the documentation.
Jonathan writes:
>> It is a fix for a corner case, but it is renaming the switch, so I would
>> expect that we'd wait for a new version at least.
> The bug was that the documentation and error message gave advice that
> didn't work.

By having the old way untested, we do not give guarantees any more about the '.' case, which may be used in scripts and break, no?

Thanks, Stefan

 Documentation/git-submodule.txt | 12 +++++++++++-
 git-submodule.sh                | 14 +++++++++++---
 t/t7400-submodule-basic.sh      | 14 +++++++++-----
 3 files changed, 31 insertions(+), 9 deletions(-)
Show changes to 3 files +31 −9

Documentation/git-submodule.txt, git-submodule.sh, t/t7400-submodule-basic.sh

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index 1572f05..24d7197 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -13,7 +13,7 @@ SYNOPSIS
 	      [--reference <repository>] [--depth <depth>] [--] <repository> [<path>]
 'git submodule' [--quiet] status [--cached] [--recursive] [--] [<path>...]
 'git submodule' [--quiet] init [--] [<path>...]
-'git submodule' [--quiet] deinit [-f|--force] [--] <path>...
+'git submodule' [--quiet] deinit [-f|--force] (-a|--all|[--] <path>...)
 'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]
 	      [-f|--force] [--rebase|--merge] [--reference <repository>]
 	      [--depth <depth>] [--recursive] [--] [<path>...]
@@ -144,6 +144,11 @@ deinit::
 	you really want to remove a submodule from the repository and commit
 	that use linkgit:git-rm[1] instead.
 +
+To unregister all submodules use `--all` with no path spec. In
+version 2.8 and prior, you were advised to give '.' to unregister
+all submodules. This may continue to work in the future, but as the
+path spec '.' may include regular files, this could stop working.
++
 If `--force` is specified, the submodule's work tree will be removed even if
 it contains local modifications.
 
@@ -247,6 +252,11 @@ OPTIONS
 --quiet::
 	Only print error messages.
 
+-a::
+--all::
+	This option is only valid for the deinit command. Unregister all
+	submodules in the work tree.
+
 -b::
 --branch::
 	Branch of repository to add as submodule.
diff --git a/git-submodule.sh b/git-submodule.sh
index 43c68de..6dabb56 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -8,7 +8,7 @@ dashless=$(basename "$0" | sed -e 's/-/ /')
 USAGE="[--quiet] add [-b <branch>] [-f|--force] [--name <name>] [--reference <repository>] [--] <repository> [<path>]
    or: $dashless [--quiet] status [--cached] [--recursive] [--] [<path>...]
    or: $dashless [--quiet] init [--] [<path>...]
-   or: $dashless [--quiet] deinit [-f|--force] [--] <path>...
+   or: $dashless [--quiet] deinit [-f|--force] (-a|--all| [--] <path>...)
    or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch] [-f|--force] [--checkout|--merge|--rebase] [--reference <repository>] [--recursive] [--] [<path>...]
    or: $dashless [--quiet] summary [--cached|--files] [--summary-limit <n>] [commit] [--] [<path>...]
    or: $dashless [--quiet] foreach [--recursive] <command>
@@ -521,6 +521,7 @@ cmd_init()
 cmd_deinit()
 {
 	# parse $args after "submodule ... deinit".
+	deinit_all=
 	while test $# -ne 0
 	do
 		case "$1" in
@@ -530,6 +531,9 @@ cmd_deinit()
 		-q|--quiet)
 			GIT_QUIET=1
 			;;
+		-a|--all)
+			deinit_all=t
+			;;
 		--)
 			shift
 			break
@@ -544,9 +548,13 @@ cmd_deinit()
 		shift
 	done
 
-	if test $# = 0
+	if test -n "$deinit_all" && test "$#" -ne 0
+	then
+		die "$(eval_gettext "usage: $dashless [--quiet] deinit [-f|--force] (--all | [--] <path>...)")"
+	fi
+	if test $# = 0 && test -z "$deinit_all"
 	then
-		die "$(eval_gettext "Use '.' if you really want to deinitialize all submodules")"
+		die "$(eval_gettext "Use '--all' if you really want to deinitialize all submodules")"
 	fi
 
 	git submodule--helper list --prefix "$wt_prefix" "$@" |
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index e1abd19..6e28ea5 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -11,6 +11,10 @@ subcommands of git submodule.
 
 . ./test-lib.sh
 
+test_expect_success 'submodule deinit works on empty repository' '
+	git submodule deinit --all
+'
+
 test_expect_success 'setup - initial commit' '
 	>t &&
 	git add t &&
@@ -858,7 +862,7 @@ 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 --all
 	)
 '
 
@@ -887,12 +891,12 @@ test_expect_success 'submodule deinit from subdirectory' '
 	rmdir init
 '
 
-test_expect_success 'submodule deinit . deinits all initialized submodules' '
+test_expect_success 'submodule deinit --all deinits all initialized submodules' '
 	git submodule update --init &&
 	git config submodule.example.foo bar &&
 	git config submodule.example2.frotz nitfol &&
 	test_must_fail git submodule deinit &&
-	git submodule deinit . >actual &&
+	git submodule deinit --all >actual &&
 	test -z "$(git config --get-regexp "submodule\.example\.")" &&
 	test -z "$(git config --get-regexp "submodule\.example2\.")" &&
 	test_i18ngrep "Cleared directory .init" actual &&
@@ -958,11 +962,11 @@ test_expect_success 'submodule deinit is silent when used on an uninitialized su
 	git submodule deinit init >actual &&
 	test_i18ngrep ! "Submodule .example. (.*) unregistered for path .init" actual &&
 	test_i18ngrep "Cleared directory .init" actual &&
-	git submodule deinit . >actual &&
+	git submodule deinit --all >actual &&
 	test_i18ngrep ! "Submodule .example. (.*) unregistered for path .init" actual &&
 	test_i18ngrep "Submodule .example2. (.*) unregistered for path .example2" actual &&
 	test_i18ngrep "Cleared directory .init" actual &&
-	git submodule deinit . >actual &&
+	git submodule deinit --all >actual &&
 	test_i18ngrep ! "Submodule .example. (.*) unregistered for path .init" actual &&
 	test_i18ngrep ! "Submodule .example2. (.*) unregistered for path .example2" actual &&
 	test_i18ngrep "Cleared directory .init" actual &&

-- 
2.8.0.rc4.9.g9db9a47
Junio C Hamano· May 4, 2016, 20:44 UTC · re: Stefan Beller · lore

Re: [PATCHv3] submodule deinit: require '--all' instead of '.' for all submodules

Stefan Beller <sbeller@google.com> writes:
> The discussion in [1] realized that '.' is a faulty suggestion as
> there is a corner case where it fails:

A discussion does not "realize" (you may say "the discussion made me realize" but that gets personal and subjective description that is irrelevant in the project history), and this phrase has been bothering me since the original round.

Perhaps s/realized/pointed out/ or something?
Show 17 quoted lines
>
>> "submodule deinit ." may have "worked" in the sense that you would
>> have at least one path in your tree and avoided this "nothing
>> matches" most of the time.  It would have still failed with the
>> exactly same error if run in an empty repository, i.e.
>>
>>        $ E=/var/tmp/x/empty && rm -fr "$E" && mkdir -p "$E" && cd "$E"
>>        $ git init
>>        $ rungit v2.6.6 submodule deinit .
>>        error: pathspec '.' did not match any file(s) known to git.
>>        Did you forget to 'git add'?
>>        $ >file && git add file
>>        $ rungit v2.6.6 submodule deinit .
>>        $ echo $?
>>        0
>
> So instead of a path spec add a parameter '--all' to deinit all submodules

s/path spec/pathspec/; s/a parameter '--all'/the '--all' option/;

> and add a test to check for the corner case of an empty repository.
>
> The code only needs to learn about the '--all' parameter and doesn't
Likewise.
> require further changes as `git submodule--helper list "$@"` will list
> all submodules in case of "$@" being empty.
I'd propose doing s/in case of.../when "$@" is empty./
Show 18 quoted lines
> diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
> index 1572f05..24d7197 100644
> --- a/Documentation/git-submodule.txt
> +++ b/Documentation/git-submodule.txt
> @@ -13,7 +13,7 @@ SYNOPSIS
>  	      [--reference <repository>] [--depth <depth>] [--] <repository> [<path>]
>  'git submodule' [--quiet] status [--cached] [--recursive] [--] [<path>...]
>  'git submodule' [--quiet] init [--] [<path>...]
> -'git submodule' [--quiet] deinit [-f|--force] [--] <path>...
> +'git submodule' [--quiet] deinit [-f|--force] (-a|--all|[--] <path>...)
>  'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]
>  	      [-f|--force] [--rebase|--merge] [--reference <repository>]
>  	      [--depth <depth>] [--recursive] [--] [<path>...]
> @@ -144,6 +144,11 @@ deinit::
>  	you really want to remove a submodule from the repository and commit
>  	that use linkgit:git-rm[1] instead.
>  +
> +To unregister all submodules use `--all` with no path spec. In

s/path spec/pathspec/; But I'd rather see something more like this instead of the first sentence:

	When the command is run without pathspec, it errors out,
	instead of deinit-ing everything, to prevent mistakes.
> +version 2.8 and prior, you were advised to give '.' to unregister
> +all submodules. This may continue to work in the future, but as the
> +path spec '.' may include regular files, this could stop working.
	... the command gave a suggestion to use '.' to unregister
	all submodules when it was invoked without any argument, but
	this suggestion did not work and gave a wrong message if you
	followed in pathological cases and is no longer recommended.

Do not predict the future in the documentation when we ourselves have not committed to any concrete plan.

>  If `--force` is specified, the submodule's work tree will be removed even if
>  it contains local modifications.

I think this sentence talks about "working tree" (as opposed to "worktree"), so s/work tree/working tree/.

Show 8 quoted lines
> @@ -247,6 +252,11 @@ OPTIONS
>  --quiet::
>  	Only print error messages.
>  
> +-a::
> +--all::
> +	This option is only valid for the deinit command. Unregister all
> +	submodules in the work tree.
Likewise.
Show 42 quoted lines
>  -b::
>  --branch::
>  	Branch of repository to add as submodule.
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 43c68de..6dabb56 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -8,7 +8,7 @@ dashless=$(basename "$0" | sed -e 's/-/ /')
>  USAGE="[--quiet] add [-b <branch>] [-f|--force] [--name <name>] [--reference <repository>] [--] <repository> [<path>]
>     or: $dashless [--quiet] status [--cached] [--recursive] [--] [<path>...]
>     or: $dashless [--quiet] init [--] [<path>...]
> -   or: $dashless [--quiet] deinit [-f|--force] [--] <path>...
> +   or: $dashless [--quiet] deinit [-f|--force] (-a|--all| [--] <path>...)
>     or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch] [-f|--force] [--checkout|--merge|--rebase] [--reference <repository>] [--recursive] [--] [<path>...]
>     or: $dashless [--quiet] summary [--cached|--files] [--summary-limit <n>] [commit] [--] [<path>...]
>     or: $dashless [--quiet] foreach [--recursive] <command>
> @@ -521,6 +521,7 @@ cmd_init()
>  cmd_deinit()
>  {
>  	# parse $args after "submodule ... deinit".
> +	deinit_all=
>  	while test $# -ne 0
>  	do
>  		case "$1" in
> @@ -530,6 +531,9 @@ cmd_deinit()
>  		-q|--quiet)
>  			GIT_QUIET=1
>  			;;
> +		-a|--all)
> +			deinit_all=t
> +			;;
>  		--)
>  			shift
>  			break
> @@ -544,9 +548,13 @@ cmd_deinit()
>  		shift
>  	done
>  
> -	if test $# = 0
> +	if test -n "$deinit_all" && test "$#" -ne 0
> +	then
> +		die "$(eval_gettext "usage: $dashless [--quiet] deinit [-f|--force] (--all | [--] <path>...)")"
I doubt that "usage:" wants to go thru l10n.

I suspect that it is more friendly to the user to say that in prose, i.e.e.g. "--all and pathspec cannot be given at the same time", than forcing them to grok the (alternative|possibilities).

Show 6 quoted lines
> +	fi
> +	if test $# = 0 && test -z "$deinit_all"
>  	then
> -		die "$(eval_gettext "Use '.' if you really want to deinitialize all submodules")"
> +		die "$(eval_gettext "Use '--all' if you really want to deinitialize all submodules")"
>  	fi
This is good.
Show 54 quoted lines
>  	git submodule--helper list --prefix "$wt_prefix" "$@" |
> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> index e1abd19..6e28ea5 100755
> --- a/t/t7400-submodule-basic.sh
> +++ b/t/t7400-submodule-basic.sh
> @@ -11,6 +11,10 @@ subcommands of git submodule.
>  
>  . ./test-lib.sh
>  
> +test_expect_success 'submodule deinit works on empty repository' '
> +	git submodule deinit --all
> +'
> +
>  test_expect_success 'setup - initial commit' '
>  	>t &&
>  	git add t &&
> @@ -858,7 +862,7 @@ 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 --all
>  	)
>  '
>  
> @@ -887,12 +891,12 @@ test_expect_success 'submodule deinit from subdirectory' '
>  	rmdir init
>  '
>  
> -test_expect_success 'submodule deinit . deinits all initialized submodules' '
> +test_expect_success 'submodule deinit --all deinits all initialized submodules' '
>  	git submodule update --init &&
>  	git config submodule.example.foo bar &&
>  	git config submodule.example2.frotz nitfol &&
>  	test_must_fail git submodule deinit &&
> -	git submodule deinit . >actual &&
> +	git submodule deinit --all >actual &&
>  	test -z "$(git config --get-regexp "submodule\.example\.")" &&
>  	test -z "$(git config --get-regexp "submodule\.example2\.")" &&
>  	test_i18ngrep "Cleared directory .init" actual &&
> @@ -958,11 +962,11 @@ test_expect_success 'submodule deinit is silent when used on an uninitialized su
>  	git submodule deinit init >actual &&
>  	test_i18ngrep ! "Submodule .example. (.*) unregistered for path .init" actual &&
>  	test_i18ngrep "Cleared directory .init" actual &&
> -	git submodule deinit . >actual &&
> +	git submodule deinit --all >actual &&
>  	test_i18ngrep ! "Submodule .example. (.*) unregistered for path .init" actual &&
>  	test_i18ngrep "Submodule .example2. (.*) unregistered for path .example2" actual &&
>  	test_i18ngrep "Cleared directory .init" actual &&
> -	git submodule deinit . >actual &&
> +	git submodule deinit --all >actual &&
>  	test_i18ngrep ! "Submodule .example. (.*) unregistered for path .init" actual &&
>  	test_i18ngrep ! "Submodule .example2. (.*) unregistered for path .example2" actual &&
>  	test_i18ngrep "Cleared directory .init" actual &&

I would have expected that we'd be testing both '.' and '--all', by keeping the '.' tests as they were and adding tests for '--all'. It is not like we are discouraging use of '.' when the repository is known to have submodule(s) and '.' is expected to match.

Other than that, looks good to me.  Thanks.
Stefan Beller· May 4, 2016, 21:34 UTC · re: Junio C Hamano · lore

Re: [PATCHv3] submodule deinit: require '--all' instead of '.' for all submodules

On Wed, May 4, 2016 at 1:44 PM, Junio C Hamano <gitster@pobox.com> wrote:
> I think this sentence talks about "working tree" (as opposed to
> "worktree"), so s/work tree/working tree/.
I'll fix this up in a resend, though it may be a fix on its own.

So the two "official" terms are working tree (files on your disk) and worktree (the command) and we don't want to have anything in between? (e.g. work tree for working tree?)

Or as `grep -r "work tree"` puts it, we may want to have an extra cleanup patch and not do it here for this single occurrence.

Junio C Hamano· May 4, 2016, 21:43 UTC · re: Stefan Beller · lore

Re: [PATCHv3] submodule deinit: require '--all' instead of '.' for all submodules

Stefan Beller <sbeller@google.com> writes:
> (e.g. work tree for working tree?)

This was probably primarily my fault, not just because I've written more than my share of documentation (compared to the code that touched), but I was deliberately writing "work tree" when both "work tree" and "working tree" terms meant the same thing. Compared to the length of the timeperiod, the newcomer who is now known as "worktree" has only lived a very short period of time, so it is not surprising to see remaining "work tree" in our documentation set.

I think some attempts like 06cdac5a (git-reset.txt: use "working tree" consistently, 2010-09-15) were made to clean things up even before "worktree" started to mean an entirely different thing, and we shouldn't make things worse by adding mentions of "work tree" when we mean "working tree".

Stefan Beller· May 4, 2016, 21:47 UTC · re: Junio C Hamano · lore

Re: [PATCHv3] submodule deinit: require '--all' instead of '.' for all submodules

On Wed, May 4, 2016 at 2:43 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 11 quoted lines
> Stefan Beller <sbeller@google.com> writes:
>
>> (e.g. work tree for working tree?)
>
> This was probably primarily my fault, not just because I've written
> more than my share of documentation (compared to the code that
> touched), but I was deliberately writing "work tree" when both "work
> tree" and "working tree" terms meant the same thing.  Compared to
> the length of the timeperiod, the newcomer who is now known as
> "worktree" has only lived a very short period of time, so it is not
> surprising to see remaining "work tree" in our documentation set.
Sure.
Show 6 quoted lines
>
> I think some attempts like 06cdac5a (git-reset.txt: use "working
> tree" consistently, 2010-09-15) were made to clean things up even
> before "worktree" started to mean an entirely different thing, and
> we shouldn't make things worse by adding mentions of "work tree"
> when we mean "working tree".
Ok. I'll see if I can send a similar patch like that.
Junio C Hamano· May 4, 2016, 21:51 UTC · re: Stefan Beller · lore

Re: [PATCHv3] submodule deinit: require '--all' instead of '.' for all submodules

Stefan Beller <sbeller@google.com> writes:
Show 7 quoted lines
>> I think some attempts like 06cdac5a (git-reset.txt: use "working
>> tree" consistently, 2010-09-15) were made to clean things up even
>> before "worktree" started to mean an entirely different thing, and
>> we shouldn't make things worse by adding mentions of "work tree"
>> when we mean "working tree".
>
> Ok. I'll see if I can send a similar patch like that.

Please don't churn the documentation especially when there are more important changes in flight that deserve larger share of reviewer bandwidth.

My review comment on the documentation part of the patch was primarily not to make things worse when you do not have to.

Junio C Hamano· May 4, 2016, 21:49 UTC · re: Junio C Hamano · lore

Re: [PATCHv3] submodule deinit: require '--all' instead of '.' for all submodules

Junio C Hamano <gitster@pobox.com> writes:
Show 18 quoted lines
>> +	if test -n "$deinit_all" && test "$#" -ne 0
>> +	then
>> +		die "$(eval_gettext "usage: $dashless [--quiet] deinit [-f|--force] (--all | [--] <path>...)")"
>
> I doubt that "usage:" wants to go thru l10n.
>
> I suspect that it is more friendly to the user to say that in prose,
> i.e.e.g. "--all and pathspec cannot be given at the same time", than
> forcing them to grok the (alternative|possibilities).
>
>> +	fi
>> +	if test $# = 0 && test -z "$deinit_all"
>>  	then
>> -		die "$(eval_gettext "Use '.' if you really want to deinitialize all submodules")"
>> +		die "$(eval_gettext "Use '--all' if you really want to deinitialize all submodules")"
>>  	fi
>
> This is good.
By the way, while it is a very good idea to die upon
	$ git submodule deinit --all no-only-this-one

it may not be too bad if we demoted the output to "info" with clean no-op exit when the user said

	$ git submodule deinit
IOW, the latter part _might_ be better if it were
	if test $# = 0 && test -z "$deinit_all"
	then
		echo >&2 "info: not deinitializing anything."
                echo >&2 "info: Use --all to deinitialize all submodules."
                exit 0
	fi

given that this is really about preventing mistakes from doing mass destruction.

Stefan Beller· May 4, 2016, 21:57 UTC · re: Junio C Hamano · lore

Re: [PATCHv3] submodule deinit: require '--all' instead of '.' for all submodules

On Wed, May 4, 2016 at 2:49 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 41 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
>
>>> +    if test -n "$deinit_all" && test "$#" -ne 0
>>> +    then
>>> +            die "$(eval_gettext "usage: $dashless [--quiet] deinit [-f|--force] (--all | [--] <path>...)")"
>>
>> I doubt that "usage:" wants to go thru l10n.
>>
>> I suspect that it is more friendly to the user to say that in prose,
>> i.e.e.g. "--all and pathspec cannot be given at the same time", than
>> forcing them to grok the (alternative|possibilities).
>>
>>> +    fi
>>> +    if test $# = 0 && test -z "$deinit_all"
>>>      then
>>> -            die "$(eval_gettext "Use '.' if you really want to deinitialize all submodules")"
>>> +            die "$(eval_gettext "Use '--all' if you really want to deinitialize all submodules")"
>>>      fi
>>
>> This is good.
>
> By the way, while it is a very good idea to die upon
>
>         $ git submodule deinit --all no-only-this-one
>
> it may not be too bad if we demoted the output to "info" with clean
> no-op exit when the user said
>
>         $ git submodule deinit
>
> IOW, the latter part _might_ be better if it were
>
>         if test $# = 0 && test -z "$deinit_all"
>         then
>                 echo >&2 "info: not deinitializing anything."
>                 echo >&2 "info: Use --all to deinitialize all submodules."
>                 exit 0
>         fi
>
> given that this is really about preventing mistakes from doing mass
> destruction.
Demote to a new class in a class of its own?
* grep -r "info:" gives no match for user facing code, so it's a
prefix you made up now.
* I assume we want to translate it?
* This would not respect the --quiet option?
* returning 0 as in "everyting is fine", while nothing is fine.
   There are two cases:
     - Either the user knows what they are doing (See Per complaining
about this)
       A very deliberate "I want it all gone or error out if you
cannot remove it all"
     - The user has no idea what they are doing. Exiting non zero
doesn't do any harm.
Maybe:
> -             die "$(eval_gettext "Use '.' if you really want to deinitialize all submodules")"
> +            usage()
instead?
Junio C Hamano· May 4, 2016, 22:06 UTC · re: Stefan Beller · lore

Re: [PATCHv3] submodule deinit: require '--all' instead of '.' for all submodules

Stefan Beller <sbeller@google.com> writes:
Show 18 quoted lines
>> IOW, the latter part _might_ be better if it were
>>
>>         if test $# = 0 && test -z "$deinit_all"
>>         then
>>                 echo >&2 "info: not deinitializing anything."
>>                 echo >&2 "info: Use --all to deinitialize all submodules."
>>                 exit 0
>>         fi
>>
>> given that this is really about preventing mistakes from doing mass
>> destruction.
>  ...
> Maybe:
>
>> -             die "$(eval_gettext "Use '.' if you really want to deinitialize all submodules")"
>> +            usage()
>
> instead?

I was primarily concerned about die giving a non-zero exit status when "git submodule deinit" did not find anything worthwhile to do (because we specified nothing on the command line to deinit). IOW, it was an attempt to do "You asked me a no-op, so I am doing a no-op, but if I stayed silent, you wouldn't even notice it, and you might have meant to deinit all, so I am telling you I didn't do anything, and advising how to say 'deinit all' to me."

But what we see in the patch under discussion is perfectly fine; it behaves just like "$ rm<RET>" and "$ git add<RET>" that give an error message and exit with a non-zero status.

← back to recent threads