From: Junio C Hamano <gitster@pobox.com>
To: Stefan Beller <sbeller@google.com>
Cc: jrnieder@gmail.com, git@vger.kernel.org, Jens.Lehmann@web.de
Subject: Re: [PATCHv3] submodule deinit: require '--all' instead of '.' for all submodules
Date: Wed, 04 May 2016 13:44:26 -0700 [thread overview]
Message-ID: <xmqqlh3pft91.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <1462324785-26389-1-git-send-email-sbeller@google.com> (Stefan Beller's message of "Tue, 3 May 2016 18:19:45 -0700")
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?
>
>> "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./
> 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/.
> @@ -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.
> -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).
> + 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.
> 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.
next prev parent reply other threads:[~2016-05-04 20:44 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-05-03 22:11 [PATCH] submodule deinit: require '--all' instead of '.' for all submodules Stefan Beller
2016-05-03 22:25 ` Junio C Hamano
2016-05-03 22:28 ` Junio C Hamano
2016-05-03 22:43 ` Stefan Beller
2016-05-03 22:52 ` Junio C Hamano
2016-05-03 23:02 ` Stefan Beller
2016-05-04 0:33 ` [PATCHv2] " Stefan Beller
2016-05-04 0:47 ` Jonathan Nieder
2016-05-04 1:19 ` [PATCHv3] " Stefan Beller
2016-05-04 20:44 ` Junio C Hamano [this message]
2016-05-04 21:34 ` Stefan Beller
2016-05-04 21:43 ` Junio C Hamano
2016-05-04 21:47 ` Stefan Beller
2016-05-04 21:51 ` Junio C Hamano
2016-05-04 21:49 ` Junio C Hamano
2016-05-04 21:57 ` Stefan Beller
2016-05-04 22:06 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqlh3pft91.fsf@gitster.mtv.corp.google.com \
--to=gitster@pobox.com \
--cc=Jens.Lehmann@web.de \
--cc=git@vger.kernel.org \
--cc=jrnieder@gmail.com \
--cc=sbeller@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).