Re: [PATCH v5] submodule: add 'deinit' command

Am 17.02.2013 23:32, schrieb Junio C Hamano:

Jens Lehmann [off-list ref] writes:

quoted

diff --git a/git-submodule.sh b/git-submodule.sh
index 004c034..0fb6ee0 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh

@@ -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
+
+	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?

"git submodule" fails when not run in the top level directory.

quoted

+	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'")"
+			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.

I used $@ here because it makes the code more robust against
someone accidentally removing the "test $# -ne 1 ||" (as it then
will fail when $# > 1 in one of the tests). But looking at it
again I agree that "$1" might better show the intention here and
the "$@" can easily make people suspicious.

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.

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.

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?

That is a very good point. It makes sense that if we don't nuke the
whole test to at least remove the "continue" there (in case someone
fiddled with .git/config but wants to get rid of the work tree in a
safe manner later).