Re: [PATCH v5] submodule: add 'deinit' command
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 17, 2013, 22:32 UTC
- Message-ID
- <7v38wufu5t.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <5121384B.10009@web.de>
Jens Lehmann <Jens.Lehmann@web.de> writes:
Show 9 quoted lines
> 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
Thanks for being thorough. I honestly did not expect this topic to take this many cycles before becoming 'next-ready'.
Show 23 quoted lines
> 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?
Show 12 quoted lines
> + 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?