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

[Jens Lehmann <Jens.Lehmann@web.de>]

Am 12.02.2013 18:11, schrieb Phil Hord:
> On Wed, Feb 6, 2013 at 4:11 PM, Jens Lehmann <Jens.Lehmann@web.de> wrote:
> +               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.

Yeah, that makes much more sense. But I'd rather use the name too:

   Submodule '\$name' is not initialized for path '\$sm_path'

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

Yes, that message gets printed for each uninitialized submodule. We
could easily suppress that for '.', but it would be really hard to
get that right for other wildcards like 'foo*'. (And e.g. running a
"submodule update" also lists all submodules it skips because of an
"update=none" setting, so I'm not sure if it's that important here)

But if people really want that, I'd suppress that for the '.' case.
Further opinions?

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

Hmm, I'm a bit torn about automagically moving the repo somewhere
else. While I think it is a sane solution for most users I suspect
some users might hate us for doing that without asking (and with no
option to turn that off). What about adding a separate "git submodule
to-gitfile" command which does that and hinting that here? However I
do have the feeling that this should be done in another commit.

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

Ok. And while looking at it I noticed that "$sm_path" should be
"'\$sm_path'" here, same a few lines above ... sigh

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

Thanks.

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?

My current diff to v4 looks like this:

-------------8<-------------
diff --git a/git-submodule.sh b/git-submodule.sh
index f1b552f..27f8e12 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -591,7 +591,7 @@ cmd_deinit()
 		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")"
+			say "$(eval_gettext "Submodule '\$name' is not initialized for path '\$sm_path'")"
 			continue
 		fi

@@ -601,14 +601,14 @@ cmd_deinit()
 			# 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")"
+				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)")"
 			fi

 			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")"
+				die "$(eval_gettext "Submodule work tree '\$sm_path' contains local modifications; use '-f' to discard them")"
 			fi
 			rm -rf "$sm_path" || say "$(eval_gettext "Could not remove submodule work tree '\$sm_path'")"
 		fi
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index f54a40d..e4b0a59 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -759,7 +759,7 @@ test_expect_success 'submodule add with an existing name fails unless forced' '

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

 test_expect_success 'submodule deinit should remove the whole submodule section from .git/config' '
@@ -837,7 +837,7 @@ test_expect_success 'submodule deinit complains but does not fail when used on a
 	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 &&
+	test_i18ngrep "Submodule .example. is not initialized for path .init" actual &&
 	git submodule deinit . >actual &&
 	test_i18ngrep "Submodule .example2. (.*) unregistered for path .example2" actual
 	rmdir init example2