Re: [PATCHv4] submodule deinit: require '--all' instead of '.' for all submodules
- From
Jonathan Nieder <jrnieder@gmail.com>
- Date
- May 4, 2016, 23:59 UTC
- Message-ID
- <20160504235914.GD395@google.com>
- In-Reply-To
- <CAGZ79kbeCCcmGh57zUdQ=BzFOWUiwj8-3nM4dbK9yONbrmLaPw@mail.gmail.com>
Stefan Beller wrote:
> On Wed, May 4, 2016 at 4:26 PM, Jonathan Nieder <jrnieder@gmail.com> 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.
[...]
Show 15 quoted lines
>>> +-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. :)
[...]
Show 12 quoted lines
>>> @@ -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.
[...]
Show 11 quoted lines
>>> + 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.
[...]
Show 7 quoted lines
>> 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