On Wed, May 4, 2016 at 4:26 PM, Jonathan Nieder <jrnieder@xxxxxxxxx> wrote:
> Hi,
>
> Stefan Beller wrote:
>
>> Signed-off-by: Stefan Beller <sbeller@xxxxxxxxxx>
>> ---
>> * 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html