From: Jonathan Nieder <jrnieder@gmail.com>
To: Stefan Beller <sbeller@google.com>
Cc: Junio C Hamano <gitster@pobox.com>,
"git@vger.kernel.org" <git@vger.kernel.org>,
Jens Lehmann <Jens.Lehmann@web.de>
Subject: Re: [PATCHv4] submodule deinit: require '--all' instead of '.' for all submodules
Date: Wed, 4 May 2016 16:59:14 -0700 [thread overview]
Message-ID: <20160504235914.GD395@google.com> (raw)
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.
[...]
>>> +-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
next prev parent reply other threads:[~2016-05-04 23:59 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-05-04 22:40 [PATCHv4] submodule deinit: require '--all' instead of '.' for all submodules Stefan Beller
2016-05-04 23:08 ` Junio C Hamano
2016-05-04 23:26 ` Jonathan Nieder
2016-05-04 23:38 ` Stefan Beller
2016-05-04 23:59 ` Jonathan Nieder [this message]
2016-05-05 18:03 ` Junio C Hamano
2016-05-05 19:20 ` Jonathan Nieder
2016-05-05 19:35 ` Stefan Beller
2016-05-05 19:02 ` Stefan Beller
2016-05-05 17:59 ` Junio C Hamano
2016-05-05 18:11 ` Stefan Beller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160504235914.GD395@google.com \
--to=jrnieder@gmail.com \
--cc=Jens.Lehmann@web.de \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=sbeller@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).