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

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

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

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