git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCHv3] submodule deinit: require '--all' instead of '.' for all submodules

From
Junio C Hamano <gitster@pobox.com>
Date
May 4, 2016, 20:44 UTC
Message-ID
<xmqqlh3pft91.fsf@gitster.mtv.corp.google.com>
In-Reply-To
<1462324785-26389-1-git-send-email-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> The discussion in [1] realized that '.' is a faulty suggestion as
> there is a corner case where it fails:

A discussion does not "realize" (you may say "the discussion made me realize" but that gets personal and subjective description that is irrelevant in the project history), and this phrase has been bothering me since the original round.

Perhaps s/realized/pointed out/ or something?
Show 17 quoted lines
>
>> "submodule deinit ." may have "worked" in the sense that you would
>> have at least one path in your tree and avoided this "nothing
>> matches" most of the time.  It would have still failed with the
>> exactly same error if run in an empty repository, i.e.
>>
>>        $ E=/var/tmp/x/empty && rm -fr "$E" && mkdir -p "$E" && cd "$E"
>>        $ git init
>>        $ rungit v2.6.6 submodule deinit .
>>        error: pathspec '.' did not match any file(s) known to git.
>>        Did you forget to 'git add'?
>>        $ >file && git add file
>>        $ rungit v2.6.6 submodule deinit .
>>        $ echo $?
>>        0
>
> So instead of a path spec add a parameter '--all' to deinit all submodules

s/path spec/pathspec/; s/a parameter '--all'/the '--all' option/;

> and add a test to check for the corner case of an empty repository.
>
> The code only needs to learn about the '--all' parameter and doesn't
Likewise.
> require further changes as `git submodule--helper list "$@"` will list
> all submodules in case of "$@" being empty.
I'd propose doing s/in case of.../when "$@" is empty./
Show 18 quoted lines
> diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
> index 1572f05..24d7197 100644
> --- a/Documentation/git-submodule.txt
> +++ b/Documentation/git-submodule.txt
> @@ -13,7 +13,7 @@ SYNOPSIS
>  	      [--reference <repository>] [--depth <depth>] [--] <repository> [<path>]
>  'git submodule' [--quiet] status [--cached] [--recursive] [--] [<path>...]
>  'git submodule' [--quiet] init [--] [<path>...]
> -'git submodule' [--quiet] deinit [-f|--force] [--] <path>...
> +'git submodule' [--quiet] deinit [-f|--force] (-a|--all|[--] <path>...)
>  'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]
>  	      [-f|--force] [--rebase|--merge] [--reference <repository>]
>  	      [--depth <depth>] [--recursive] [--] [<path>...]
> @@ -144,6 +144,11 @@ deinit::
>  	you really want to remove a submodule from the repository and commit
>  	that use linkgit:git-rm[1] instead.
>  +
> +To unregister all submodules use `--all` with no path spec. In

s/path spec/pathspec/; But I'd rather see something more like this instead of the first sentence:

	When the command is run without pathspec, it errors out,
	instead of deinit-ing everything, to prevent mistakes.
> +version 2.8 and prior, you were advised to give '.' to unregister
> +all submodules. This may continue to work in the future, but as the
> +path spec '.' may include regular files, this could stop working.
	... the command gave a suggestion to use '.' to unregister
	all submodules when it was invoked without any argument, but
	this suggestion did not work and gave a wrong message if you
	followed in pathological cases and is no longer recommended.

Do not predict the future in the documentation when we ourselves have not committed to any concrete plan.

>  If `--force` is specified, the submodule's work tree will be removed even if
>  it contains local modifications.

I think this sentence talks about "working tree" (as opposed to "worktree"), so s/work tree/working tree/.

Show 8 quoted lines
> @@ -247,6 +252,11 @@ OPTIONS
>  --quiet::
>  	Only print error messages.
>  
> +-a::
> +--all::
> +	This option is only valid for the deinit command. Unregister all
> +	submodules in the work tree.
Likewise.
Show 42 quoted lines
>  -b::
>  --branch::
>  	Branch of repository to add as submodule.
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 43c68de..6dabb56 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -8,7 +8,7 @@ dashless=$(basename "$0" | sed -e 's/-/ /')
>  USAGE="[--quiet] add [-b <branch>] [-f|--force] [--name <name>] [--reference <repository>] [--] <repository> [<path>]
>     or: $dashless [--quiet] status [--cached] [--recursive] [--] [<path>...]
>     or: $dashless [--quiet] init [--] [<path>...]
> -   or: $dashless [--quiet] deinit [-f|--force] [--] <path>...
> +   or: $dashless [--quiet] deinit [-f|--force] (-a|--all| [--] <path>...)
>     or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch] [-f|--force] [--checkout|--merge|--rebase] [--reference <repository>] [--recursive] [--] [<path>...]
>     or: $dashless [--quiet] summary [--cached|--files] [--summary-limit <n>] [commit] [--] [<path>...]
>     or: $dashless [--quiet] foreach [--recursive] <command>
> @@ -521,6 +521,7 @@ cmd_init()
>  cmd_deinit()
>  {
>  	# parse $args after "submodule ... deinit".
> +	deinit_all=
>  	while test $# -ne 0
>  	do
>  		case "$1" in
> @@ -530,6 +531,9 @@ cmd_deinit()
>  		-q|--quiet)
>  			GIT_QUIET=1
>  			;;
> +		-a|--all)
> +			deinit_all=t
> +			;;
>  		--)
>  			shift
>  			break
> @@ -544,9 +548,13 @@ cmd_deinit()
>  		shift
>  	done
>  
> -	if test $# = 0
> +	if test -n "$deinit_all" && test "$#" -ne 0
> +	then
> +		die "$(eval_gettext "usage: $dashless [--quiet] deinit [-f|--force] (--all | [--] <path>...)")"
I doubt that "usage:" wants to go thru l10n.

I suspect that it is more friendly to the user to say that in prose, i.e.e.g. "--all and pathspec cannot be given at the same time", than forcing them to grok the (alternative|possibilities).

Show 6 quoted lines
> +	fi
> +	if test $# = 0 && test -z "$deinit_all"
>  	then
> -		die "$(eval_gettext "Use '.' if you really want to deinitialize all submodules")"
> +		die "$(eval_gettext "Use '--all' if you really want to deinitialize all submodules")"
>  	fi
This is good.
Show 54 quoted lines
>  	git submodule--helper list --prefix "$wt_prefix" "$@" |
> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> index e1abd19..6e28ea5 100755
> --- a/t/t7400-submodule-basic.sh
> +++ b/t/t7400-submodule-basic.sh
> @@ -11,6 +11,10 @@ subcommands of git submodule.
>  
>  . ./test-lib.sh
>  
> +test_expect_success 'submodule deinit works on empty repository' '
> +	git submodule deinit --all
> +'
> +
>  test_expect_success 'setup - initial commit' '
>  	>t &&
>  	git add t &&
> @@ -858,7 +862,7 @@ test_expect_success 'submodule deinit works on repository without submodules' '
>  		>file &&
>  		git add file &&
>  		git commit -m "repo should not be empty"
> -		git submodule deinit .
> +		git submodule deinit --all
>  	)
>  '
>  
> @@ -887,12 +891,12 @@ test_expect_success 'submodule deinit from subdirectory' '
>  	rmdir init
>  '
>  
> -test_expect_success 'submodule deinit . deinits all initialized submodules' '
> +test_expect_success 'submodule deinit --all deinits all initialized submodules' '
>  	git submodule update --init &&
>  	git config submodule.example.foo bar &&
>  	git config submodule.example2.frotz nitfol &&
>  	test_must_fail git submodule deinit &&
> -	git submodule deinit . >actual &&
> +	git submodule deinit --all >actual &&
>  	test -z "$(git config --get-regexp "submodule\.example\.")" &&
>  	test -z "$(git config --get-regexp "submodule\.example2\.")" &&
>  	test_i18ngrep "Cleared directory .init" actual &&
> @@ -958,11 +962,11 @@ test_expect_success 'submodule deinit is silent when used on an uninitialized su
>  	git submodule deinit init >actual &&
>  	test_i18ngrep ! "Submodule .example. (.*) unregistered for path .init" actual &&
>  	test_i18ngrep "Cleared directory .init" actual &&
> -	git submodule deinit . >actual &&
> +	git submodule deinit --all >actual &&
>  	test_i18ngrep ! "Submodule .example. (.*) unregistered for path .init" actual &&
>  	test_i18ngrep "Submodule .example2. (.*) unregistered for path .example2" actual &&
>  	test_i18ngrep "Cleared directory .init" actual &&
> -	git submodule deinit . >actual &&
> +	git submodule deinit --all >actual &&
>  	test_i18ngrep ! "Submodule .example. (.*) unregistered for path .init" actual &&
>  	test_i18ngrep ! "Submodule .example2. (.*) unregistered for path .example2" actual &&
>  	test_i18ngrep "Cleared directory .init" actual &&

I would have expected that we'd be testing both '.' and '--all', by keeping the '.' tests as they were and adding tests for '--all'. It is not like we are discouraging use of '.' when the repository is known to have submodule(s) and '.' is expected to match.

Other than that, looks good to me.  Thanks.
Previous: Stefan BellerNext: Stefan Beller
Message 10 of 17 in “submodule deinit: require '--all' instead of '.' for all submodules”
  1. submodule deinit: require '--all' instead of '.' for all submodulesStefan Beller, May 3, 2016
  2. Junio C HamanoMay 3, 2016
  3. Junio C HamanoMay 3, 2016
  4. Stefan BellerMay 3, 2016
  5. Junio C HamanoMay 3, 2016
  6. Stefan BellerMay 3, 2016
  7. [PATCHv2] submodule deinit: require '--all' instead of '.' for all submodulesStefan Beller, May 4, 2016
  8. Jonathan NiederMay 4, 2016
  9. [PATCHv3] submodule deinit: require '--all' instead of '.' for all submodulesStefan Beller, May 4, 2016
  10. Junio C HamanoMay 4, 2016
  11. Stefan BellerMay 4, 2016
  12. Junio C HamanoMay 4, 2016
  13. Stefan BellerMay 4, 2016
  14. Junio C HamanoMay 4, 2016
  15. Junio C HamanoMay 4, 2016
  16. Stefan BellerMay 4, 2016
  17. Junio C HamanoMay 4, 2016

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.