Re: [PATCH v4] submodule: add 'deinit' command
- From
Jens Lehmann <jens.lehmann@web.de>
- Date
- Feb 13, 2013, 19:33 UTC
- Message-ID
- <511BEA75.6000002@web.de>
- In-Reply-To
- <CABURp0oQcPotK20QcqCG1pGQPVoa4RnN2nDA=iQoKS99gnPEAQ@mail.gmail.com>
Am 12.02.2013 18:11, schrieb Phil Hord:
Show 12 quoted lines
> On Wed, Feb 6, 2013 at 4:11 PM, Jens Lehmann <Jens.Lehmann@web.de> wrote: > + die_if_unmatched "$mode" >> + name=$(module_name "$sm_path") || exit >> + url=$(git config submodule."$name".url) >> + if test -z "$url" >> + then >> + say "$(eval_gettext "No url found for submodule path '\$sm_path' in .git/config")" > > Is it safe to shelter the user a little bit more from the git > internals here and say instead: > > Submodule '\$sm_path' is not initialized.
Yeah, that makes much more sense. But I'd rather use the name too:
Submodule '\$name' is not initialized for path '\$sm_path'
Show 5 quoted lines
> Also, I think this code will show this message for each submodule on > 'git submodule deinit .' But I think I would prefer to suppress it in > that case. If I have not explicitly stated which submodules to > deinit, then I do not think git should complain that some of them are > not initialized.
Yes, that message gets printed for each uninitialized submodule. We could easily suppress that for '.', but it would be really hard to get that right for other wildcards like 'foo*'. (And e.g. running a "submodule update" also lists all submodules it skips because of an "update=none" setting, so I'm not sure if it's that important here)
But if people really want that, I'd suppress that for the '.' case. Further opinions?
Show 16 quoted lines
>> + continue >> + fi >> + >> + # Remove the submodule work tree (unless the user already did it) >> + if test -d "$sm_path" >> + then >> + # Protect submodules containing a .git directory >> + if test -d "$sm_path/.git" >> + then >> + echo >&2 "$(eval_gettext "Submodule work tree $sm_path contains a .git directory")" >> + die "$(eval_gettext "(use 'rm -rf' if you really want to remove it including all of its history)")" > > I expect this is the right thing to do for now. But I wonder if we > can also move $sm_path/.git to $GIT_DIR/modules/$sm_path in this case > (though I think I am not spelling this path correctly). Would that be > ok? What extra work is needed to relocate the .git dir like this?
Hmm, I'm a bit torn about automagically moving the repo somewhere else. While I think it is a sane solution for most users I suspect some users might hate us for doing that without asking (and with no option to turn that off). What about adding a separate "git submodule to-gitfile" command which does that and hinting that here? However I do have the feeling that this should be done in another commit.
>> + die "$(eval_gettext "Submodule work tree $sm_path contains local modifications, use '-f' to discard them")" > > Nit, grammar: use a semicolon instead of a comma.
Ok. And while looking at it I noticed that "$sm_path" should be "'\$sm_path'" here, same a few lines above ... sigh
Show 5 quoted lines
>> +test_expect_success 'set up a second submodule' ' >> + git submodule add ./init2 example2 && >> + git commit -m "submodle example2 added" > > Nit: submodule is misspelled
Thanks.
Junio, this looks like a we have v5 as soon as we decide what to do with the "not initialized" messages when '.' is used, right?
My current diff to v4 looks like this:
-------------8<-------------
diff --git a/git-submodule.sh b/git-submodule.sh index f1b552f..27f8e12 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -591,7 +591,7 @@ cmd_deinit() url=$(git config submodule."$name".url) if test -z "$url" then - say "$(eval_gettext "No url found for submodule path '\$sm_path' in .git/config")" + say "$(eval_gettext "Submodule '\$name' is not initialized for path '\$sm_path'")" continue fi @@ -601,14 +601,14 @@ cmd_deinit() # Protect submodules containing a .git directory if test -d "$sm_path/.git" then - echo >&2 "$(eval_gettext "Submodule work tree $sm_path contains a .git directory")" + echo >&2 "$(eval_gettext "Submodule work tree '\$sm_path' contains a .git directory")" die "$(eval_gettext "(use 'rm -rf' if you really want to remove it including all of its history)")" fi if test -z "$force" then git rm -n "$sm_path" || - die "$(eval_gettext "Submodule work tree $sm_path contains local modifications, use '-f' to discard them")" + die "$(eval_gettext "Submodule work tree '\$sm_path' contains local modifications; use '-f' to discard them")" fi rm -rf "$sm_path" || say "$(eval_gettext "Could not remove submodule work tree '\$sm_path'")" fi diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh index f54a40d..e4b0a59 100755 --- a/t/t7400-submodule-basic.sh +++ b/t/t7400-submodule-basic.sh @@ -759,7 +759,7 @@ test_expect_success 'submodule add with an existing name fails unless forced' ' test_expect_success 'set up a second submodule' ' git submodule add ./init2 example2 && - git commit -m "submodle example2 added" + git commit -m "submodule example2 added" ' test_expect_success 'submodule deinit should remove the whole submodule section from .git/config' ' @@ -837,7 +837,7 @@ test_expect_success 'submodule deinit complains but does not fail when used on a git submodule deinit init >actual && test_i18ngrep "Submodule .example. (.*) unregistered for path .init" actual git submodule deinit init >actual && - test_i18ngrep "No url found for submodule path .init. in .git/config" actual && + test_i18ngrep "Submodule .example. is not initialized for path .init" actual && git submodule deinit . >actual && test_i18ngrep "Submodule .example2. (.*) unregistered for path .example2" actual rmdir init example2