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

[Stefan Beller <sbeller@google.com>]

On Wed, May 4, 2016 at 4:26 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Hi,
>
> Stefan Beller wrote:
>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>> * reworded commit message slightly (realize, pathspec)
>> * reworded the documentation
>
> Yay, thanks for your work on this.
>
> [...]
>> +++ 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>...]
>> @@ -140,12 +140,20 @@ deinit::
>>       tree. Further calls to `git submodule update`, `git submodule foreach`
>>       and `git submodule sync` will skip any unregistered submodules until
>>       they are initialized again, so use this command if you don't want to
>> -     have a local checkout of the submodule in your work tree anymore. If
>> +     have a local checkout of the submodule in your working tree anymore. If
>>       you really want to remove a submodule from the repository and commit
>>       that use linkgit:git-rm[1] instead.
>> ++
>> +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 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.

>
>>  +
>> -If `--force` is specified, the submodule's work tree will be removed even if
>> -it contains local modifications.
>> +If `--force` is specified, the submodule's working tree will
>> +be removed even if it contains local modifications.
>
> (unnecessary rewrapping)
>
> [...]
>>  update::
>>  +
>> @@ -247,6 +255,11 @@ OPTIONS
>>  --quiet::
>>       Only print error messages.
>>
>> +-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.


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

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

We do.

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


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

>
> [...]
>> --- a/t/t7400-submodule-basic.sh
>> +++ b/t/t7400-submodule-basic.sh
>
> Makes sense.
>
> 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?

>
> Thanks and sorry for all the complication,
> Jonathan