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

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

Am 17.02.2013 23:32, schrieb Junio C Hamano:
> Jens Lehmann <Jens.Lehmann@web.de> writes:
>> 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.

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