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

The discussion in [1] pointed out 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 pathspec add the '--all' option 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' option and doesn't
require further changes as `git submodule--helper list "$@"` will list
all submodules when "$@" is empty.

[1] http://news.gmane.org/gmane.comp.version-control.git/289535

Helped-by: Junio C Hamano [off-list ref]
Signed-off-by: Stefan Beller <redacted>
---
* reworded commit message slightly (realize, pathspec)
* reworded the documentation 
* no "usage:" to go thru l10n.

 Documentation/git-submodule.txt | 25 +++++++++++++++++++------
 git-submodule.sh                | 14 +++++++++++---
 t/t7400-submodule-basic.sh      | 28 +++++++++++++++++++++++++++-
 3 files changed, 57 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index 1572f05..aeeb2ed 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>...]

@@ -140,12 +140,20 @@ deinit::
 	tree. Further calls to `git submodule update`, `git submodule foreach`
 	and `git submodule sync` will skip any unregistered submodules until
 	they are initialized again, so use this command if you don't want to
-	have a local checkout of the submodule in your work tree anymore. If
+	have a local checkout of the submodule in your working tree anymore. If
 	you really want to remove a submodule from the repository and commit
 	that use linkgit:git-rm[1] instead.
 +
-If `--force` is specified, the submodule's work tree will be removed even if
-it contains local modifications.
+When the command is run without pathspec, it errors out,
+instead of deinit-ing everything, to prevent mistakes. In
+version 2.8 and before 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 it in pathological cases and is
+no longer recommended.
++
+If `--force` is specified, the submodule's working tree will
+be removed even if it contains local modifications.
 
 update::
 +

@@ -247,6 +255,11 @@ OPTIONS
 --quiet::
 	Only print error messages.
 
+-a::
+--all::
+	This option is only valid for the deinit command. Unregister all
+	submodules in the working tree.
+
 -b::
 --branch::
 	Branch of repository to add as submodule.

@@ -257,8 +270,8 @@ OPTIONS
 --force::
 	This option is only valid for add, deinit and update commands.
 	When running add, allow adding an otherwise ignored submodule path.
-	When running deinit the submodule work trees will be removed even if
-	they contain local changes.
+	When running deinit the submodule working trees will be removed even
+	if they contain local changes.
 	When running update (only effective with the checkout procedure),
 	throw away local changes in submodules when switching to a
 	different commit; and always run a checkout operation in the

diff --git a/git-submodule.sh b/git-submodule.sh
index 43c68de..caa80fe 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 "--all and pathspec are incompatible")"
+	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..bd0d229 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,8 @@ 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 . &&
+		git submodule deinit --all
 	)
 '
 

@@ -900,6 +905,19 @@ test_expect_success 'submodule deinit . deinits all initialized submodules' '
 	rmdir init example2
 '
 
+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 --all >actual &&
+	test -z "$(git config --get-regexp "submodule\.example\.")" &&
+	test -z "$(git config --get-regexp "submodule\.example2\.")" &&
+	test_i18ngrep "Cleared directory .init" actual &&
+	test_i18ngrep "Cleared directory .example2" actual &&
+	rmdir init example2
+'
+
 test_expect_success 'submodule deinit deinits a submodule when its work tree is missing or empty' '
 	git submodule update --init &&
 	rm -rf init example2/* example2/.git &&

@@ -966,6 +984,14 @@ test_expect_success 'submodule deinit is silent when used on an uninitialized su
 	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 --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 -a >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 &&
 	rmdir init example2
 '
 

-- 
2.8.0.rc4.9.g082ee08.dirty

Hi,

Stefan Beller wrote:

Signed-off-by: Stefan Beller <redacted>
---
* reworded commit message slightly (realize, pathspec)
* reworded the documentation 

Yay, thanks for your work on this.

[...]

Why tell the user what happened in 2.8 and earlier?  It's not clear what
the reader would do with that information.

I think this paragraph could be removed.  --all is explained lower
down and the error message points it out to users who need it.

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

(unnecessary rewrapping)

[...]

This could use an explanation of why I'd want to use it.  E.g.

	This option is only valid for the deinit command. Unregister all
	submodules. Scripts should use this option instead of passing '.'
	to deinit because it works even in an empty repository with no
	submodules present.

Not about this patch: the organization of options is a little strange.
A separate section with options for each subcommand would be easier to
read.

Do we want to claim the short-and-sweet option -a?  (I don't mind but it
doesn't seem necessary.)

[...]

Unrelated change?

[...]

This message still feels too low-level to me, but I might be swimming
uphill here.

Another option would be to call 'usage' and be done.

[...]

Makes sense.

In the context of the original motivation: this patch improves the
advice printed by plain "git submodule deinit" but doesn't help with
existing callers that might have run "git submodule deinit .".  It might
make sense to handle '.' as a historical special case in a separate
patch.

Thanks and sorry for all the complication,
Jonathan

On Wed, May 4, 2016 at 4:26 PM, Jonathan Nieder [off-list ref] wrote:

Hi,

Stefan Beller wrote:

quoted

Signed-off-by: Stefan Beller <redacted>
---
* reworded commit message slightly (realize, pathspec)
* reworded the documentation

Yay, thanks for your work on this.

[...]

quoted

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

@@ -140,12 +140,20 @@ deinit::
      tree. Further calls to `git submodule update`, `git submodule foreach`
      and `git submodule sync` will skip any unregistered submodules until
      they are initialized again, so use this command if you don't want to
-     have a local checkout of the submodule in your work tree anymore. If
+     have a local checkout of the submodule in your working tree anymore. If
      you really want to remove a submodule from the repository and commit
      that use linkgit:git-rm[1] instead.
++
+When the command is run without pathspec, it errors out,
+instead of deinit-ing everything, to prevent mistakes. In
+version 2.8 and before 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 it in pathological cases and is
+no longer recommended.

Why tell the user what happened in 2.8 and earlier?  It's not clear what
the reader would do with that information.

Because people may wonder what happened to '.' ?

I think this paragraph could be removed.  --all is explained lower
down and the error message points it out to users who need it.

When we want to keep supporting '.' forever, I would remove this section.

quoted

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

(unnecessary rewrapping)

[...]

quoted

 update::
 +

@@ -247,6 +255,11 @@ OPTIONS
 --quiet::
      Only print error messages.

+-a::
+--all::
+     This option is only valid for the deinit command. Unregister all
+     submodules in the working tree.

This could use an explanation of why I'd want to use it.  E.g.

        This option is only valid for the deinit command. Unregister all
        submodules. Scripts should use this option instead of passing '.'
        to deinit because it works even in an empty repository with no
        submodules present.

I would not want to mention '.' in the documentation. this can read:

    As a user I am fine to use '.' and then I wonder when it breaks.

Not about this patch: the organization of options is a little strange.
A separate section with options for each subcommand would be easier to
read.

I agree.

Do we want to claim the short-and-sweet option -a?  (I don't mind but it
doesn't seem necessary.)

We do.

[...]

quoted

@@ -257,8 +270,8 @@ OPTIONS
 --force::
      This option is only valid for add, deinit and update commands.
      When running add, allow adding an otherwise ignored submodule path.
-     When running deinit the submodule work trees will be removed even if
-     they contain local changes.
+     When running deinit the submodule working trees will be removed even
+     if they contain local changes.

Unrelated change?

It's close enough for deinit to squash it in here, no?

[...]

quoted

@@ -544,9 +548,13 @@ cmd_deinit()
              shift
      done

-     if test $# = 0
+     if test -n "$deinit_all" && test "$#" -ne 0
+     then
+             die "$(eval_gettext "--all and pathspec are incompatible")"

This message still feels too low-level to me, but I might be swimming
uphill here.

Another option would be to call 'usage' and be done.

I had that idea as well, but I think pointing out the low level is better
than giving the high level again, so the user immediately sees what's wrong.

[...]

quoted

--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh

Makes sense.

In the context of the original motivation: this patch improves the
advice printed by plain "git submodule deinit" but doesn't help with
existing callers that might have run "git submodule deinit .".  It might
make sense to handle '.' as a historical special case in a separate
patch.

Once we change how '.' is handled we can do that?

Thanks and sorry for all the complication,
Jonathan

Stefan Beller wrote:

On Wed, May 4, 2016 at 4:26 PM, Jonathan Nieder [off-list ref] wrote:

quoted

I think this paragraph could be removed.  --all is explained lower
down and the error message points it out to users who need it.

When we want to keep supporting '.' forever, I would remove this section.

Yes, please.  We can't remove something like that without a deprecation
process that I don't think is worth it here.

[...]

quoted

quoted

+-a::
+--all::
+     This option is only valid for the deinit command. Unregister all
+     submodules in the working tree.

This could use an explanation of why I'd want to use it.  E.g.

        This option is only valid for the deinit command. Unregister all
        submodules. Scripts should use this option instead of passing '.'
        to deinit because it works even in an empty repository with no
        submodules present.

I would not want to mention '.' in the documentation. this can read:

    As a user I am fine to use '.' and then I wonder when it breaks.

Sorry for the lack of clarity.  By referencing scripts I was referring
to "callers that want to be able to run the same command in all
situations, even the edge case of no files present".  I agree with you
that humans should care just as much as scripts about things that will
break and that we shouldn't break them. :)

[...]

quoted

quoted

@@ -257,8 +270,8 @@ OPTIONS
 --force::
      This option is only valid for add, deinit and update commands.
      When running add, allow adding an otherwise ignored submodule path.
-     When running deinit the submodule work trees will be removed even if
-     they contain local changes.
+     When running deinit the submodule working trees will be removed even
+     if they contain local changes.

Unrelated change?

It's close enough for deinit to squash it in here, no?

My preference would be to have a separate patch since its commit message
can explain the purpose.  I don't care much --- it was just something I
noticed while reviewing the rest.

[...]

quoted

quoted

+     if test -n "$deinit_all" && test "$#" -ne 0
+     then
+             die "$(eval_gettext "--all and pathspec are incompatible")"

This message still feels too low-level to me, but I might be swimming
uphill here.

Another option would be to call 'usage' and be done.

I had that idea as well, but I think pointing out the low level is better
than giving the high level again, so the user immediately sees what's wrong.

I mean low level as in implementation detail.  The human user would
wonder "what is incompatible about them?  Why are you stopping me from
what I am trying to do?"  Most likely the user was trying to do
something other than specify a path, since they also passed --all.  If
I run something like

	git submodule deinit force --all

and the output tells me that --all and pathspec are incompatible then
I just scratch my head more.

We can do

	USAGE="$dashless [--quiet] deinit [-f|--force] (--all | [--] <path>...)"
	usage

to print the subcommand's usage.  git commandline tools don't
translate any usage strings today, so not getting translation here
wouldn't feel out of place.

[...]

quoted

In the context of the original motivation: this patch improves the
advice printed by plain "git submodule deinit" but doesn't help with
existing callers that might have run "git submodule deinit .".  It might
make sense to handle '.' as a historical special case in a separate
patch.

Once we change how '.' is handled we can do that?

It's harmless even before then.  Anyway, I meant "as a preparatory step
before such a change that would otherwise be backward incompatible."

Thanks,
Jonathan

On Wed, May 4, 2016 at 4:59 PM, Jonathan Nieder [off-list ref] wrote:

quoted

quoted

Another option would be to call 'usage' and be done.

I had that idea as well, but I think pointing out the low level is better
than giving the high level again, so the user immediately sees what's wrong.

I mean low level as in implementation detail.  The human user would
wonder "what is incompatible about them?  Why are you stopping me from
what I am trying to do?"  Most likely the user was trying to do
something other than specify a path, since they also passed --all.  If
I run something like

    $ git submodule deinit force --all
    error: unknown option `all'
    usage: git submodule--helper list [--prefix=<path>] [<path>...]

        --prefix <path>       alternative anchor for relative paths

`force` is seen as the first pathspec, so "force --all" is given to the
`submodule--helper list`, which gives a less than optimal error message

    $ git submodule deinit --all force
gettext: unrecognized option '--all and pathspec are incompatible'
Try 'gettext --help' for more information.
envsubst: unrecognized option '--all and pathspec are incompatible'
Try 'envsubst --help' for more information.
envsubst: unrecognized option '--all and pathspec are incompatible'
Try 'envsubst --help' for more information.

We should not put --all as the first thing in the error message.

    $ git submodule deinit --all force
    pathspec and --all are incompatible

With the next patch this is the error message. I think the missing
dashes for force are quite visible as force is after --all.

and the output tells me that --all and pathspec are incompatible then
I just scratch my head more.

We can do

        USAGE="$dashless [--quiet] deinit [-f|--force] (--all | [--] <path>...)"
        usage

to print the subcommand's usage.  git commandline tools don't
translate any usage strings today, so not getting translation here
wouldn't feel out of place.

We can have both? I'd prefer not rewriting the USAGE string here
as it would easily be out of sync in the future?
I tried to just grep the deinit line from the USAGe though, but that doesn't
look right as it would need some post processing. (remove the "or:")
and processing the USAGE string also doesn't sound future proof.