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

Stefan Beller wrote:

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

quoted

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.

[...]

quoted

quoted

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

[...]

quoted

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?

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.

[...]

quoted

quoted

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

[...]

quoted

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