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

[Junio C Hamano]

Stefan Beller  writes:

>> IOW, the latter part _might_ be better if it were
>>
>> if test $# = 0 && test -z "$deinit_all"
>> then
>> echo >&2 "info: not deinitializing anything."
>> echo >&2 "info: Use --all to deinitialize all submodules."
>> exit 0
>> fi
>>
>> given that this is really about preventing mistakes from doing mass
>> destruction.
>  ...
> Maybe:
>
>> - die "$(eval_gettext "Use '.' if you really want to 
>> deinitialize all submodules")"
>> +usage()
>
> instead?

I was primarily concerned about die giving a non-zero exit status
when "git submodule deinit" did not find anything worthwhile to do
(because we specified nothing on the command line to deinit).  IOW,
it was an attempt to do "You asked me a no-op, so I am doing a
no-op, but if I stayed silent, you wouldn't even notice it, and you
might have meant to deinit all, so I am telling you I didn't do
anything, and advising how to say 'deinit all' to me."

But what we see in the patch under discussion is perfectly fine; it
behaves just like "$ rm" and "$ git add" that give an
error message and exit with a non-zero status.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Stefan Beller]

On Wed, May 4, 2016 at 2:49 PM, Junio C Hamano  wrote:
> Junio C Hamano  writes:
>
>>> +if test -n "$deinit_all" && test "$#" -ne 0
>>> +then
>>> +die "$(eval_gettext "usage: $dashless [--quiet] deinit 
>>> [-f|--force] (--all | [--] ...)")"
>>
>> 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.
>
> By the way, while it is a very good idea to die upon
>
> $ git submodule deinit --all no-only-this-one
>
> it may not be too bad if we demoted the output to "info" with clean
> no-op exit when the user said
>
> $ git submodule deinit
>
> IOW, the latter part _might_ be better if it were
>
> if test $# = 0 && test -z "$deinit_all"
> then
> echo >&2 "info: not deinitializing anything."
> echo >&2 "info: Use --all to deinitialize all submodules."
> exit 0
> fi
>
> given that this is really about preventing mistakes from doing mass
> destruction.

Demote to a new class in a class of its own?

* grep -r "info:" gives no match for user facing code, so it's a
prefix you made up now.
* I assume we want to translate it?
* This would not respect the --quiet option?
* returning 0 as in "everyting is fine", while nothing is fine.
   There are two cases:
 - Either the user knows what they are doing (See Per complaining
about this)
   A very deliberate "I want it all gone or error out if you
cannot remove it all"
 - The user has no idea what they are doing. Exiting non zero
doesn't do any harm.

Maybe:

> - die "$(eval_gettext "Use '.' if you really want to deinitialize 
> all submodules")"
> +usage()

instead?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Stefan Beller  writes:

>> I think some attempts like 06cdac5a (git-reset.txt: use "working
>> tree" consistently, 2010-09-15) were made to clean things up even
>> before "worktree" started to mean an entirely different thing, and
>> we shouldn't make things worse by adding mentions of "work tree"
>> when we mean "working tree".
>
> Ok. I'll see if I can send a similar patch like that.

Please don't churn the documentation especially when there are more
important changes in flight that deserve larger share of reviewer
bandwidth.

My review comment on the documentation part of the patch was
primarily not to make things worse when you do not have to.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Junio C Hamano  writes:

>> +if test -n "$deinit_all" && test "$#" -ne 0
>> +then
>> +die "$(eval_gettext "usage: $dashless [--quiet] deinit 
>> [-f|--force] (--all | [--] ...)")"
>
> 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.

By the way, while it is a very good idea to die upon

$ git submodule deinit --all no-only-this-one

it may not be too bad if we demoted the output to "info" with clean
no-op exit when the user said

$ git submodule deinit

IOW, the latter part _might_ be better if it were

if test $# = 0 && test -z "$deinit_all"
then
echo >&2 "info: not deinitializing anything."
echo >&2 "info: Use --all to deinitialize all submodules."
exit 0
fi

given that this is really about preventing mistakes from doing mass
destruction.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Stefan Beller]

On Wed, May 4, 2016 at 2:43 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> (e.g. work tree for working tree?)
>
> This was probably primarily my fault, not just because I've written
> more than my share of documentation (compared to the code that
> touched), but I was deliberately writing "work tree" when both "work
> tree" and "working tree" terms meant the same thing.  Compared to
> the length of the timeperiod, the newcomer who is now known as
> "worktree" has only lived a very short period of time, so it is not
> surprising to see remaining "work tree" in our documentation set.

Sure.

>
> I think some attempts like 06cdac5a (git-reset.txt: use "working
> tree" consistently, 2010-09-15) were made to clean things up even
> before "worktree" started to mean an entirely different thing, and
> we shouldn't make things worse by adding mentions of "work tree"
> when we mean "working tree".

Ok. I'll see if I can send a similar patch like that.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Stefan Beller  writes:

> (e.g. work tree for working tree?)

This was probably primarily my fault, not just because I've written
more than my share of documentation (compared to the code that
touched), but I was deliberately writing "work tree" when both "work
tree" and "working tree" terms meant the same thing.  Compared to
the length of the timeperiod, the newcomer who is now known as
"worktree" has only lived a very short period of time, so it is not
surprising to see remaining "work tree" in our documentation set.

I think some attempts like 06cdac5a (git-reset.txt: use "working
tree" consistently, 2010-09-15) were made to clean things up even
before "worktree" started to mean an entirely different thing, and
we shouldn't make things worse by adding mentions of "work tree"
when we mean "working tree".
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Stefan Beller]

On Wed, May 4, 2016 at 1:44 PM, Junio C Hamano  wrote:

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

I'll fix this up in a resend, though it may be a fix on its own.

So the two "official" terms are working tree (files on your disk)
and worktree (the command) and we don't want to have anything in between?
(e.g. work tree for working tree?)

Or as `grep -r "work tree"` puts it, we may want to have an extra cleanup patch
and not do it here for this single occurrence.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Stefan Beller  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 ] [--depth ] [--]  
> []
>  'git submodule' [--quiet] status [--cached] [--recursive] [--] [...]
>  'git submodule' [--quiet] init [--] [...]
> -'git submodule' [--quiet] deinit [-f|--force] [--] ...
> +'git submodule' [--quiet] deinit [-f|--force] (-a|--all|[--] ...)
>  'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]
> [-f|--force] [--rebase|--merge] [--reference ]
> [--depth ] [--recursive] [--] [...]
> @@ -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 ] [-f|--force] [--name ] [--reference 
> ] [--]  []
> or: $dashless [--quiet] status [--cached] [--recursive] [--] [...]
> or: $dashless [--quiet] init [--] [...]
> -   or: $dashless [--quiet] deinit [-f|--force] [--] ...
> +   or: $dashless [--quiet] deinit [-f|--force] (-a|--all| [--] ...)
> or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch] 
> [-f|--force] [--checkout|--merge|--rebase] [--reference ] 
> [--recursive] [--] [...]
> or: $dashless [--quiet] summary [--cached|--files] [--summary-limit ] 
> [commit] [--] [...]
> or: $dashless [--quiet] foreach [--recursive] 
> @@ -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)
> +