Stefan Beller wrote: > On Wed, May 4, 2016 at 4:26 PM, Jonathan Nieder <jrnieder@xxxxxxxxx> wrote: >> 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. [...] >>> +-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. :) [...] >>> @@ -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. [...] >>> + 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. [...] >> 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 -- 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