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

[Jonathan Nieder <jrnieder@gmail.com>]

Stefan Beller wrote:
> On Wed, May 4, 2016 at 4:26 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

>> 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.

Yes, please.  We can't remove something like that without a deprecation
process that I don't think is worth it here.

[...]
>>> +-a::
>>> +--all::
>>> +     This option is only valid for the deinit command. Unregister all
>>> +     submodules in the working tree.
>>
>> This could use an explanation of why I'd want to use it.  E.g.
>>
>>         This option is only valid for the deinit command. Unregister all
>>         submodules. Scripts should use this option instead of passing '.'
>>         to deinit because it works even in an empty repository with no
>>         submodules present.
>
> I would not want to mention '.' in the documentation. this can read:
>
>     As a user I am fine to use '.' and then I wonder when it breaks.

Sorry for the lack of clarity.  By referencing scripts I was referring
to "callers that want to be able to run the same command in all
situations, even the edge case of no files present".  I agree with you
that humans should care just as much as scripts about things that will
break and that we shouldn't break them. :)

[...]
>>> @@ -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?

My preference would be to have a separate patch since its commit message
can explain the purpose.  I don't care much --- it was just something I
noticed while reviewing the rest.

[...]
>>> +     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 mean low level as in implementation detail.  The human user would
wonder "what is incompatible about them?  Why are you stopping me from
what I am trying to do?"  Most likely the user was trying to do
something other than specify a path, since they also passed --all.  If
I run something like

	git submodule deinit force --all

and the output tells me that --all and pathspec are incompatible then
I just scratch my head more.

We can do

	USAGE="$dashless [--quiet] deinit [-f|--force] (--all | [--] <path>...)"
	usage

to print the subcommand's usage.  git commandline tools don't
translate any usage strings today, so not getting translation here
wouldn't feel out of place.

[...]
>> In the context of the original motivation: this patch improves the
>> advice printed by plain "git submodule deinit" but doesn't help with
>> existing callers that might have run "git submodule deinit .".  It might
>> make sense to handle '.' as a historical special case in a separate
>> patch.
>
> Once we change how '.' is handled we can do that?

It's harmless even before then.  Anyway, I meant "as a preparatory step
before such a change that would otherwise be backward incompatible."

Thanks,
Jonathan