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

[Junio C Hamano <gitster@pobox.com>]

Stefan Beller <sbeller@google.com> writes:

>>> +When the command is run without pathspec, it errors out,
>>> +instead of deinit-ing everything, to prevent mistakes. In
>>> +version 2.8 and before 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 it in pathological cases and is
>>> +no longer recommended.
>>
>> Why tell the user what happened in 2.8 and earlier?  It's not clear what
>> the reader would do with that information.
>
> Because people may wonder what happened to '.' ?

I am to blame on that final text, but I think Jonathan is right.
"In version 2.8 and earlier..." can just go.  Users may need to 
understand why no-arg form is not a silent no-op but an error,
and they need to know how to de-init everything with the version
of Git they have (i.e. with "--all").  Compared to these two,
"Your fingers may have been trained to say '.', but it was found
not to work in pathological cases" is of much lessor importance,
especially because with or without this patch, the definition of
"pathological" cases does not change.

>> I think this paragraph could be removed.  --all is explained lower
>> down and the error message points it out to users who need it.
>
> When we want to keep supporting '.' forever, I would remove this section.

I am not sure what you mean by "keep supporting '.'".  If your
repository has any tracked path, "deinit ." would deinit all
submodules, with or without this change.

Are you worried about the future change you are planning to that
involves reverting 84ba959b (submodule: fix regression for deinit
without submodules, 2016-03-22), after which a pathspec that does
not match any submodule would become a "possible typo" error?

It is true that '.' would error out if there is no submodule in the
repository, as opposed to erroring out only when there is no tracked
path, which is what you get with today's version (and the version
with this fix in the patch under discussion).  But '.' is not
special with respect to that change.  'README' would also error out
if there is no submodule whose path matches that pathspec in that
future version, as opposed to erroring out only if 'README' is not
tracked at all in today's version.

Or are you thinking that it may be better to give '.' a special
meaning, iow, not treating it as just a regular pathspec?  Perhaps
make '.' to mean "everything but it is not an error if there is none
to begin with"?  I fear that going in that direction would deform
the mental model the users would form from seeing how commands
behave when given a pathspec.  The "." would still look like any
other pathspec elements, and I am sure you will not special case "."
in the usage string but will claim that it is covered by the mention
of <pathspec> at the end of the command line in the usage string,
so you are making them expect that "." used as a pathspec would
behave like that for all other places that we take pathspec, when
in reality, only "submodule deinit" make it behave differently.

Which I do not think is particularly a good idea.

>> Not about this patch: the organization of options is a little strange.
>> A separate section with options for each subcommand would be easier to
>> read.
>
> I agree.

I agree.

>> Do we want to claim the short-and-sweet option -a?  (I don't mind but it
>> doesn't seem necessary.)
>
> We do.

I don't, but I do not care too deeaply.


>>> @@ -257,8 +270,8 @@ OPTIONS
>>>  --force::
>>>       This option is only valid for add, deinit and update commands.
>>>       When running add, allow adding an otherwise ignored submodule path.
>>> -     When running deinit the submodule work trees will be removed even if
>>> -     they contain local changes.
>>> +     When running deinit the submodule working trees will be removed even
>>> +     if they contain local changes.
>>
>> Unrelated change?
>
> It's close enough for deinit to squash it in here, no?

More importantly, the patch adds a new instance of "working tree" to
the documentation elsewhere; fixing this existing instance of "work
tree" is relevant from consistency's point of view.

>>> @@ -544,9 +548,13 @@ cmd_deinit()
>>>               shift
>>>       done
>>>
>>> -     if test $# = 0
>>> +     if test -n "$deinit_all" && test "$#" -ne 0
>>> +     then
>>> +             die "$(eval_gettext "--all and pathspec are incompatible")"
>>
>> This message still feels too low-level to me, but I might be swimming
>> uphill here.
>>
>> Another option would be to call 'usage' and be done.
>
> I had that idea as well, but I think pointing out the low level is better
> than giving the high level again, so the user immediately sees what's wrong.

I do not particularly see the message low-level.  Jonathan, what do
you have against pointing out the exact problem?  After seeing the
usage string that also talks about --quite, --force, etc., I have to
somehow realize that these are irrelevant noises that have nothing
to do with the error, and puzzle out that the (choose|from|here) is
telling me that I cannot give pathspec when I am giving --all
myself.

> Once we change how '.' is handled we can do that?

Again, I am worried about "Once we change how ...".