submodule: add 'deinit' command

With "git submodule init" the user is able to tell git he cares about one or more submodules and wants to have it populated on the next call to "git submodule update". But currently there is no easy way he could tell git he does not care about a submodule anymore and wants to get rid of his local work tree (except he knows a lot about submodule internals and removes the "submodule.$name.url" setting from .git/config together with the work tree himself).

Help those users by providing a 'deinit' command. This removes the whole submodule.<name> section from .git/config either for the given submodule(s) or for all those which have been initialized if '.' is given. Fail if the current work tree contains modifications unless forced. Complain when for a submodule given on the command line the url setting can't be found in .git/config, but nonetheless don't fail.

Add tests and link the man pages of "git submodule deinit" and "git rm" to assist the user in deciding whether removing or unregistering the submodule is the right thing to do for him.

Signed-off-by Jens Lehmann <[email protected]>

---

Changes since v3:
- Add deinit to the --force documentation of "git submodule"
- Never remove submodules containing a .git dir, even when forced
- Diagnostic output when "rm -rf" or "mkdir" fails
- More test cases

 Documentation/git-rm.txt        |   4 ++
 Documentation/git-submodule.txt |  18 +++++++-
 git-submodule.sh                |  78 ++++++++++++++++++++++++++++++-
 t/t7400-submodule-basic.sh      | 100 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 198 insertions(+), 2 deletions(-)

Documentation/git-rm.txt +4 -0

index 92bac27..1d876c2 100644
@@ -149,6 +149,10 @@ files that aren't ignored are present in the submodules work tree.
 Ignored files are deemed expendable and won't stop a submodule's work
 tree from being removed.
 
+If you only want to remove the local checkout of a submodule from your
+work tree without committing the removal,
+use linkgit:git-submodule[1] `deinit` instead.
+
 EXAMPLES
 --------
 `git rm Documentation/\*.txt`::

Documentation/git-submodule.txt +17 -1

index a0c9df8..bc06159 100644
@@ -13,6 +13,7 @@ SYNOPSIS
              [--reference <repository>] [--] <repository> [<path>]
 'git submodule' [--quiet] status [--cached] [--recursive] [--] [<path>...]
 'git submodule' [--quiet] init [--] [<path>...]
+'git submodule' [--quiet] deinit [-f|--force] [--] <path>...
 'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch] [--rebase]
              [--reference <repository>] [--merge] [--recursive] [--] [<path>...]
 'git submodule' [--quiet] summary [--cached|--files] [(-n|--summary-limit) <n>]

@@ -134,6 +135,19 @@ init::
        the explicit 'init' step if you do not intend to customize
        any submodule locations.
 
+deinit::
+       Unregister the given submodules, i.e. remove the whole
+       `submodule.$name` section from .git/config together with their work
+       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
+       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.
+
 update::
        Update the registered submodules, i.e. clone missing submodules and
        checkout the commit specified in the index of the containing repository.

@@ -213,8 +227,10 @@ OPTIONS
 
 -f::
 --force::
-       This option is only valid for add and update commands.
+       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 update, throw away local changes in submodules when
        switching to a different commit; and always run a checkout operation
        in the submodule, even if the commit listed in the index of the

git-submodule.sh +35 -0

index 004c034..f1b552f 100755
@@ -8,6 +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] update [--init] [--remote] [-N|--no-fetch] [-f|--force] [--rebase] [--reference <repository>] [--merge] [--recursive] [--] [<path>...]
    or: $dashless [--quiet] summary [--cached|--files] [--summary-limit <n>] [commit] [--] [<path>...]
    or: $dashless [--quiet] foreach [--recursive] <command>

@@ -547,6 +548,81 @@ cmd_init()
 }
 
 #
+# Unregister submodules from .git/config and remove their work tree
+#
+# $@ = requested paths (use '.' to deinit all submodules)
+#
+cmd_deinit()
+{
+       # parse $args after "submodule ... init".
+       while test $# -ne 0
+       do
+               case "$1" in
+               -f|--force)
+                       force=$1
+                       ;;
+               -q|--quiet)
+                       GIT_QUIET=1
+                       ;;
+               --)
+                       shift
+                       break
+                       ;;
+               -*)
+                       usage
+                       ;;
+               *)
+                       break
+                       ;;
+               esac
+               shift
+       done
+
+       if test $# = 0
+       then
+               die "$(eval_gettext "Use '.' if you really want to deinitialize all submodules")"
+       fi

git-submodule.sh +15 -1

+                       fi
+                       rm -rf "$sm_path" || say "$(eval_gettext "Could not remove submodule work tree '\$sm_path'")"
+               fi
+
+               mkdir "$sm_path" || say "$(eval_gettext "Could not create empty submodule directory '\$sm_path'")"
+
+               # Remove the whole section so we have a clean state when the
+               # user later decides to init this submodule again
+               git config --remove-section submodule."$name" &&
+               say "$(eval_gettext "Submodule '\$name' (\$url) unregistered for path '\$sm_path'")"
+       done
+}
+
+#
 # Update each submodule path to correct revision, using clone and checkout as needed
 #
 # $@ = requested paths (default to all)

@@ -1157,7 +1233,7 @@ cmd_sync()
 while test $# != 0 && test -z "$command"
 do
        case "$1" in
-       add | foreach | init | update | status | summary | sync)
+       add | foreach | init | deinit | update | status | summary | sync)
                command=$1
                ;;
        -q|--quiet)

t/t7400-submodule-basic.sh +0 -0

index 2683cba..f54a40d 100755

t/t7400-submodule-basic.sh +97 -1

+'
+
+test_expect_success 'submodule deinit should remove the whole submodule section from .git/config' '
+       git config submodule.example.foo bar &&
+       git config submodule.example2.frotz nitfol &&
+       git submodule deinit init &&
+       test -z "$(git config submodule.example.url)" &&
+       test -z "$(git config submodule.example.foo)" &&
+       test -n "$(git config submodule.example2.url)" &&
+       test -n "$(git config submodule.example2.frotz)" &&
+       test -f example2/.git &&
+       rmdir init
+'
+
+test_expect_success 'submodule deinit . 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 . &&
+       test -z "$(git config submodule.example.url)" &&
+       test -z "$(git config submodule.example.foo)" &&
+       test -z "$(git config submodule.example2.url)" &&
+       test -z "$(git config submodule.example2.frotz)" &&
+       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 &&
+       git submodule deinit init example2 &&
+       test -z "$(git config submodule.example.url)" &&
+       test -z "$(git config submodule.example2.url)" &&
+       rmdir init
+'
+
+test_expect_success 'submodule deinit fails when the submodule contains modifications unless forced' '
+       git submodule update --init &&
+       echo X >>init/s &&
+       test_must_fail git submodule deinit init &&
+       test -n "$(git config submodule.example.url)" &&
+       test -f example2/.git &&
+       git submodule deinit -f init &&
+       test -z "$(git config submodule.example.url)" &&
+       rmdir init
+'
+
+test_expect_success 'submodule deinit fails when the submodule contains untracked files unless forced' '
+       git submodule update --init &&
+       echo X >>init/untracked &&
+       test_must_fail git submodule deinit init &&
+       test -n "$(git config submodule.example.url)" &&
+       test -f example2/.git &&
+       git submodule deinit -f init &&
+       test -z "$(git config submodule.example.url)" &&
+       rmdir init
+'
+
+test_expect_success 'submodule deinit fails when the submodule HEAD does not match unless forced' '
+       git submodule update --init &&
+       (
+               cd init &&
+               git checkout HEAD^
+       ) &&
+       test_must_fail git submodule deinit init &&
+       test -n "$(git config submodule.example.url)" &&
+       test -f example2/.git &&
+       git submodule deinit -f init &&
+       test -z "$(git config submodule.example.url)" &&
+       rmdir init
+'
+
+test_expect_success 'submodule deinit complains but does not fail when used on an uninitialized submodule' '
+       git submodule update --init &&
+       git submodule deinit init >actual &&
+       test_i18ngrep "Submodule .example. (.*) unregistered for path .init" actual
+       git submodule deinit init >actual &&
+       test_i18ngrep "No url found for submodule path .init. in .git/config" actual &&
+       git submodule deinit . >actual &&
+       test_i18ngrep "Submodule .example2. (.*) unregistered for path .example2" actual
+       rmdir init example2
+'
+
+test_expect_success 'submodule deinit fails when submodule has a .git directory even when forced' '
+       git submodule update --init &&
+       (
+               cd init &&
+               rm .git &&
+               cp -R ../.git/modules/example .git &&
+               GIT_WORK_TREE=. git config --unset core.worktree
+       ) &&
+       test_must_fail git submodule deinit init &&
+       test_must_fail git submodule deinit -f init &&
+       test -d init/.git &&
+       test -n "$(git config submodule.example.url)"
+'
+
 test_done
--

1.8.1.2.546.gea155c0

Quoted 4 lines

git-submodule.sh +4 -0

+       if test $# = 0
+       then
+               die "$(eval_gettext "Use '.' if you really want to deinitialize all submodules")"
+       fi

git-submodule.sh cmd_init() · line 594

Quoted 10 lines

git-submodule.sh +10 -0

+
+       module_list "$@" |
+       while read mode sha1 stage sm_path
+       do
+               die_if_unmatched "$mode"
+               name=$(module_name "$sm_path") || exit
+               url=$(git config submodule."$name".url)
+               if test -z "$url"
+               then
+                       say "$(eval_gettext "No url found for submodule path '\$sm_path' in .git/config")"

Is it safe to shelter the user a little bit more from the git internals here and say instead:

Submodule '\$sm_path' is not initialized.

Also, I think this code will show this message for each submodule on 'git submodule deinit .' But I think I would prefer to suppress it in that case. If I have not explicitly stated which submodules to deinit,...

+ die_if_unmatched "$mode"

Review

Quoted 5 lines

(quoted diff) +5 -0

+               name=$(module_name "$sm_path") || exit
+               url=$(git config submodule."$name".url)
+               if test -z "$url"
+               then
+                       say "$(eval_gettext "No url found for submodule path '\$sm_path' in .git/config")"

Is it safe to shelter the user a little bit more from the git internals here and say instead:

Submodule '\$sm_path' is not initialized.

Also, I think this code will show this message for each submodule on 'git submodule deinit .' But I think I would prefer to suppress it in that case. If I have not explicitly stated which submodules to deinit, then I do not think git should complain that some of them are not initialized.

Review

Quoted 11 lines

(quoted diff) +11 -0

+                       continue
+               fi
+
+               # Remove the submodule work tree (unless the user already did it)
+               if test -d "$sm_path"
+               then
+                       # Protect submodules containing a .git directory
+                       if test -d "$sm_path/.git"
+                       then
+                               echo >&2 "$(eval_gettext "Submodule work tree $sm_path contains a .git directory")"
+                               die "$(eval_gettext "(use 'rm -rf' if you really want to remove it including all of its history)")"

I expect this is the right thing to do for now. But I wonder if we can also move $sm_path/.git to $GIT_DIR/modules/$sm_path in this case (though I think I am not spelling this path correctly). Would that be ok? What extra work is needed to relocate the .git dir like this?

Quoted 1 line

+ die "$(eval_gettext "Submodule work tree $sm_path contains local modifications, use '-f' to discard them")"

Nit, grammar: use a semicolon instead of a comma.

Review

Quoted 3 lines

(quoted diff) +3 -0

+test_expect_success 'set up a second submodule' '
+       git submodule add ./init2 example2 &&
+       git commit -m "submodle example2 added"

Nit: submodule is misspelled

Junio, this looks like a we have v5 as soon as we decide what to do with the "not initialized" messages when '.' is used, right?

Quoted 2 lines · Jens Lehmann

Junio, this looks like a we have v5 as soon as we decide what to do with the "not initialized" messages when '.' is used, right?

OK. I myself do not deeply care if we end up special casing "." or not; I'll leave it up to you and other submodule folks.

Here we go, changes to v4 are:

- I decided to do the proposed special casing for "."; no messages about uninitialized submodules will show up anymore (this is also tested)
- The spelling fixes and better 'uninitialized' message Phil proposed
- Added the missing quotation for $sm_path in output strings
- "deinit" is added to the submodule completion list
- Added two missing "&&" in t7400

git-submodule.sh +5 -0

index 004c034..0fb6ee0 100755
@@ -547,6 +548,82 @@ cmd_init()
 }
 
 #
+# Unregister submodules from .git/config and remove their work tree
+#
+# $@ = requested paths (use '.' to deinit all submodules)
+#
+cmd_deinit()

Quoted 20 lines · Jens Lehmann

git-submodule.sh +11 -0

index 004c034..0fb6ee0 100755
@@ -547,6 +548,82 @@ cmd_init()
 }
 
 #
+# Unregister submodules from .git/config and remove their work tree
+#
+# $@ = requested paths (use '.' to deinit all submodules)
+#
+cmd_deinit()
+{
+	# parse $args after "submodule ... init".
+	while test $# -ne 0
+	do
..
+	done
+

git-submodule.sh cmd_init() · line 583

Quoted 3 lines

git-submodule.sh +3 -0

+	if test $# = 0
+	then
+		die "$(eval_gettext "Use '.' if you really want to deinitialize all submodules")"

I do not think I saw anybody mentioned this so far, but how is "deinit" supposed to work inside a subdirectory of a superproject? If the answer is to work on submodules appear in that subdirectory, '.' should probably not mean "all in the superproject" I think?

Quoted 10 lines

git-submodule.sh +10 -0

+	module_list "$@" |
+	while read mode sha1 stage sm_path
+	do
+		die_if_unmatched "$mode"
+		name=$(module_name "$sm_path") || exit
+		url=$(git config submodule."$name".url)
+		if test -z "$url"
+		then
+			test $# -ne 1 || test "$@" = "." ||
+			say "$(eval_gettext "Submodule '\$name' is not initialized for path '\$sm_path'")"

git-submodule.sh

Quoted 2 lines

git-submodule.sh +2 -0

+			continue
+		fi

This 'test "$@" = "."' makes readers feel uneasy. This particular invocation happens to be safe only because it is protected with "test $# -ne 1 ||", but for all other values of $# this will result in a syntax error. 'test "$1" = "."' would make the intention of the check more clear.

But stepping back a bit, is the condition this test is trying to warn against really worth warning?

It seems that this "warn if user told us to deinitialize a submodule that hasn't been initialized" is from the very initial round of this series, and not something other people asked for during the review. If somebody did

git submodule deinit foo bar

and then later said:

git submodule deinit foo

would it a mistake that the user wants to be warned about?

Perhaps the user did not mean to deinitialize foo (e.g. wanted to *initialize* foo instead, or wanted to deinitialize *foz* instead) and that is worth warning about? I am not sure, but I have a feeling that we can do without this check.

Also the value of submodule.$name.url is not used in the later codepath to ensure that the named submodule is in the pristine state in the superproject's working tree (i.e. no submodule.$name section in the local configuration, no working tree for that submodule), so it may be even a good change to remove the "does submodule.$name.url exist" check and always do the "deinitialize" process. That would give the users a way to recover from a state where a submodule is only half initialized for some reason safely, no?

Am 17.02.2013 23:32, schrieb Junio C Hamano:

Quoted 9 lines

It seems that this "warn if user told us to deinitialize a submodule that hasn't been initialized" is from the very initial round of this series, and not something other people asked for during the review. If somebody did

git submodule deinit foo bar

and then later said:

Quoted 8 lines

git submodule deinit foo

would it a mistake that the user wants to be warned about?

Perhaps the user did not mean to deinitialize foo (e.g. wanted to *initialize* foo instead, or wanted to deinitialize *foz* instead) and that is worth warning about? I am not sure, but I have a feeling that we can do without this check.

Hmm, if you would replace "submodule deinit" with "rm" in the above example, the "rm" would not only warn but error out the second time. But on the other hand doing a "git submodule init" again on already initialized submodules will silently do nothing, which seems to be the better analogy here. So yes, it looks like we can do without.

Ok, unless someone speaks up and argues in favor of this message I'll remove it in v6.

git-submodule.sh +0 -0

index 004c034..44f70c4 100755
@@ -547,6 +548,80 @@ cmd_init()
 }
 

git-submodule.sh +11 -0

+
+	module_list "$@" |
+	while read mode sha1 stage sm_path
+	do
+		die_if_unmatched "$mode"
+		name=$(module_name "$sm_path") || exit
+
+		# Remove the submodule work tree (unless the user already did it)
+		if test -d "$sm_path"
+		then
+			# Protect submodules containing a .git directory

git-submodule.sh cmd_init() · line 605

Quoted 4 lines

git-submodule.sh +4 -0

+			if test -z "$force"
+			then
+				git rm -n "$sm_path" ||
+				die "$(eval_gettext "Submodule work tree '\$sm_path' contains local modifications; use '-f' to discard them")"

Minor nit. IMO, there is an indentation for the || missing here. Maybe Junio can squash that in on his side?

Quoted 7 lines · Heiko Voigt

git-submodule.sh cmd_init() · line 605

Quoted 4 lines

git-submodule.sh +4 -0

+			if test -z "$force"
+			then
+				git rm -n "$sm_path" ||
+				die "$(eval_gettext "Submodule work tree '\$sm_path' contains local modifications; use '-f' to discard them")"

Minor nit. IMO, there is an indentation for the || missing here. Maybe Junio can squash that in on his side?

Sorry, but I do not see an indentation nit here. The format looks perfectly sane to me and in fact any other indentation would be wrong.

Puzzled...

With "git submodule init" the user is able to tell git he cares about one or more submodules and wants to have it populated on the next call to "git submodule update". But currently there is no easy way he could tell git he does not care about a submodule anymore and wants to get rid of his local work tree (except he knows a lot about submodule internals and removes the "submodule.$name.url" setting from .git/config together with the work tree himself).

Help those users by providing a 'deinit' command. This removes the whole submodule.<name> section from .git/config either for the given submodule(s) or for all those which have been initialized if '.' is given. Fail if the current work tree contains modifications unless forced. Complain when for a submodule given on the command line the url setting can't be found in .git/config, but nonetheless don't fail.

Add tests and link the man pages of "git submodule deinit" and "git rm" to assist the user in deciding whether removing or unregistering the submodule is the right thing to do for him.

Signed-off-by Jens Lehmann <[email protected]>

I think this would be clearer if 'git deinit' said

rm 'submodule/*'

or maybe

Removed workdir for 'submodule'

Is it just me?

Quoted 4 lines · Phil Hord

I think this would be clearer if 'git deinit' said

rm 'submodule/*'

Quoted 5 lines

or maybe

Removed workdir for 'submodule'

Is it just me?

The latter may probably be better.

Am 12.03.2013 17:22, schrieb Junio C Hamano:

Quoted 12 lines

Quoted 9 lines · Phil Hord

I think this would be clearer if 'git deinit' said

rm 'submodule/*'

or maybe

Removed workdir for 'submodule'

Is it just me?

Quoted 1 line

The latter may probably be better.

Hmm, it doesn't really remove the directory but only empties it (it recreates it a few lines after removing it together with its contents). So what about

Cleared directory 'submodule'

Quoted 15 lines · Jens Lehmann

Am 12.03.2013 17:22, schrieb Junio C Hamano:

Quoted 13 lines

Quoted 9 lines · Phil Hord

I think this would be clearer if 'git deinit' said

rm 'submodule/*'

or maybe

Removed workdir for 'submodule'

Is it just me?

The latter may probably be better.

Quoted 5 lines

Hmm, it doesn't really remove the directory but only empties it (it recreates it a few lines after removing it together with its contents). So what about

Cleared directory 'submodule'

Sounds the cleanest among the suggested phrasing so far.

Okay, so here is the patch for that. If someone could point out a portable and efficient way to check if a directory is already empty I would be happy to use that to silence the "Cleaned directory" message currently printed also when deinit is run on an already empty directory.

Quoted 5 lines

Okay, so here is the patch for that. If someone could point out a portable and efficient way to check if a directory is already empty I would be happy to use that to silence the "Cleaned directory" message currently printed also when deinit is run on an already empty directory.

   isemptydir() {
        test -d "$(find $1 -maxdepth 0 -empty)"
   }

Sorry for the late reply. I see this patch is already in master (which is fine with me).

Quoted 5 lines · Jens Lehmann

Okay, so here is the patch for that. If someone could point out a portable and efficient way to check if a directory is already empty I would be happy to use that to silence the "Cleaned directory" message currently printed also when deinit is run on an already empty directory.

   isemptydir() {
        test -d "$(find $1 -maxdepth 0 -empty)"
   }

Quoted 7 lines · Phil Hord

Quoted 5 lines · Jens Lehmann

Okay, so here is the patch for that. If someone could point out a portable and efficient way to check if a directory is already empty I would be happy to use that to silence the "Cleaned directory" message currently printed also when deinit is run on an already empty directory.

Quoted 3 lines

   isemptydir() {
        test -d "$(find $1 -maxdepth 0 -empty)"
   }

Hrm, -maxdepth and -empty are not even in POSIX. Folks on GNU platforms and BSDs (I checked NetBSD 6 and OpenBSD 5.2) should be fine, but it makes other platforms unhappy.

What is this check used for? To avoid running "rmdir" on non-empty ones? Saying "cleaning foo/" (or "cleaned foo/") when foo/ is already empty is not a crime; not omitting an empty one may actually be a better behaviour from the point of view of repeatability and uniformity.