Stefan Beller <sbeller@xxxxxxxxxx> writes: >>> +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 am to blame on that final text, but I think Jonathan is right. "In version 2.8 and earlier..." can just go. Users may need to understand why no-arg form is not a silent no-op but an error, and they need to know how to de-init everything with the version of Git they have (i.e. with "--all"). Compared to these two, "Your fingers may have been trained to say '.', but it was found not to work in pathological cases" is of much lessor importance, especially because with or without this patch, the definition of "pathological" cases does not change. >> 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. I am not sure what you mean by "keep supporting '.'". If your repository has any tracked path, "deinit ." would deinit all submodules, with or without this change. Are you worried about the future change you are planning to that involves reverting 84ba959b (submodule: fix regression for deinit without submodules, 2016-03-22), after which a pathspec that does not match any submodule would become a "possible typo" error? It is true that '.' would error out if there is no submodule in the repository, as opposed to erroring out only when there is no tracked path, which is what you get with today's version (and the version with this fix in the patch under discussion). But '.' is not special with respect to that change. 'README' would also error out if there is no submodule whose path matches that pathspec in that future version, as opposed to erroring out only if 'README' is not tracked at all in today's version. Or are you thinking that it may be better to give '.' a special meaning, iow, not treating it as just a regular pathspec? Perhaps make '.' to mean "everything but it is not an error if there is none to begin with"? I fear that going in that direction would deform the mental model the users would form from seeing how commands behave when given a pathspec. The "." would still look like any other pathspec elements, and I am sure you will not special case "." in the usage string but will claim that it is covered by the mention of <pathspec> at the end of the command line in the usage string, so you are making them expect that "." used as a pathspec would behave like that for all other places that we take pathspec, when in reality, only "submodule deinit" make it behave differently. Which I do not think is particularly a good idea. >> 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. 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. I don't, but I do not care too deeaply. >>> @@ -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? More importantly, the patch adds a new instance of "working tree" to the documentation elsewhere; fixing this existing instance of "work tree" is relevant from consistency's point of view. >>> @@ -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. I do not particularly see the message low-level. Jonathan, what do you have against pointing out the exact problem? After seeing the usage string that also talks about --quite, --force, etc., I have to somehow realize that these are irrelevant noises that have nothing to do with the error, and puzzle out that the (choose|from|here) is telling me that I cannot give pathspec when I am giving --all myself. > Once we change how '.' is handled we can do that? Again, I am worried about "Once we change how ...". -- 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