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

On Wed, May 4, 2016 at 4:26 PM, Jonathan Nieder [off-list ref] wrote:

Hi,

Stefan Beller wrote:

quoted

Signed-off-by: Stefan Beller <redacted>
---
* reworded commit message slightly (realize, pathspec)
* reworded the documentation

Yay, thanks for your work on this.

[...]

quoted

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

quoted

 +
-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)

[...]

quoted

 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.

[...]

quoted

@@ -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?

[...]

quoted

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

[...]

quoted

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