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

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

[Stefan Beller]

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

> "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(-)

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

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

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> ...
> 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?

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

[Junio C Hamano]

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?

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

[Stefan Beller]

On Tue, May 3, 2016 at 3:28 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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?

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

[Junio C Hamano]

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?

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

[Stefan Beller]

On Tue, May 3, 2016 at 3:52 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

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

[Stefan Beller]

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

> "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:
> 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(-)

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

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

[Jonathan Nieder]

Stefan Beller wrote:

[...]
>>        $ 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.

[...]
> +++ 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()
[...]
>  		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

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

[Stefan Beller]

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

> "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(-)

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

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

[Junio C Hamano]

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?

>
>> "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./

> 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/.

> @@ -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.

>  -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).

> +	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.

>  	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.

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

[Stefan Beller]

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.

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

[Junio C Hamano]

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".

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

[Stefan Beller]

On Wed, May 4, 2016 at 2:43 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

>
> 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.

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

[Junio C Hamano]

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.

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

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

>> 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.

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

[Stefan Beller]

On Wed, May 4, 2016 at 2:49 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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?

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

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

>> 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.