Re: [PATCH v4] submodule: add 'deinit' command
Jens Lehmann writes: > Am 12.03.2013 17:22, schrieb Junio C Hamano: >> Phil Hord writes: >> >>> I think this would be clearer if 'git deinit' said >>> >>> rm 'submodule/*' >>> >>> or maybe >>> >>> Removed workdir for 'submodule' >>> >>> Is it just me? >> >> The latter may probably be better. > > Hmm, it doesn't really remove the directory but only empties it > (it recreates it a few lines after removing it together with its > contents). So what about > > Cleared directory 'submodule' Sounds the cleanest among the suggested phrasing so far. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4] submodule: add 'deinit' command
Am 12.03.2013 17:22, schrieb Junio C Hamano: > Phil Hord writes: > >> I think this would be clearer if 'git deinit' said >> >> rm 'submodule/*' >> >> or maybe >> >> Removed workdir for 'submodule' >> >> Is it just me? > > The latter may probably be better. Hmm, it doesn't really remove the directory but only empties it (it recreates it a few lines after removing it together with its contents). So what about Cleared directory 'submodule' The attached interdiff suppresses the "rm 'submodule'" message and issues the "Cleared ..." message after it successfully removed the work tree. (But please note that it also prints this message even if the submodule work tree is already empty, e.g. when you deinit a submodule the second time) 8<- diff --git a/git-submodule.sh b/git-submodule.sh index 204bc78..d003e8a 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -601,10 +601,12 @@ cmd_deinit() if test -z "$force" then - git rm -n "$sm_path" || + git rm -qn "$sm_path" || 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'")" + rm -rf "$sm_path" && + say "$(eval_gettext "Cleared directory '\$sm_path'")" || + say "$(eval_gettext "Could not remove submodule work tree '\$sm_path'")" fi mkdir "$sm_path" || say "$(eval_gettext "Could not create empty submodule directory '\$sm_path'")" -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4] submodule: add 'deinit' command
Phil Hord writes: > I think this would be clearer if 'git deinit' said > > rm 'submodule/*' > > or maybe > > Removed workdir for 'submodule' > > Is it just me? The latter may probably be better. After cloning the superproject, you show interest in individual submodules by saying "git submodule init " and until then your .git/config in the superproject does not indicate that you are interested in that submodule, and you won't have a submodule checkout. "deinit" is a way to revert back to that original state; recording that you are no longer interested in the submodule is the primary effect, and removal of its checkout is a mere side effect of it. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4] submodule: add 'deinit' command
On Wed, Feb 6, 2013 at 4:11 PM, Jens Lehmann wrote: > With "git submodule init" the user is able to tell git he cares about one > or more submodules and wants to have it populated on the next call to "git > submodule update". But currently there is no easy way he could tell git he > does not care about a submodule anymore and wants to get rid of his local > work tree (except he knows a lot about submodule internals and removes the > "submodule.$name.url" setting from .git/config together with the work tree > himself). > > Help those users by providing a 'deinit' command. This removes the whole > submodule. section from .git/config either for the given > submodule(s) or for all those which have been initialized if '.' is > given. Fail if the current work tree contains modifications unless > forced. Complain when for a submodule given on the command line the url > setting can't be found in .git/config, but nonetheless don't fail. > > Add tests and link the man pages of "git submodule deinit" and "git rm" > to assist the user in deciding whether removing or unregistering the > submodule is the right thing to do for him. > > Signed-off-by: Jens Lehmann Probably because I was new to this command, I was confused by this output. $ git submodule deinit submodule rm 'submodule' Submodule 'submodule' (gerrit:foo/submodule) unregistered for path 'submodule' $ git rm submodule rm 'submodule' This line is confusing to me in the deinit command: rm 'submodule' It doesn't mean what git usually means when it says this to me. See how the 'git rm' command says the same thing but means something different in the next command. In the deinit case, git removes the workdir contents of 'submodule' and it reports "rm 'submodule'". In the rm case, git removes the submodule link from the tree and rmdirs the empty 'submodule' directory. I think this would be clearer if 'git deinit' said rm 'submodule/*' or maybe Removed workdir for 'submodule' Is it just me? Phil -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4] submodule: add 'deinit' command
Jens Lehmann writes: > 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? OK. I myself do not deeply care if we end up special casing "." or not; I'll leave it up to you and other submodule folks. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4] submodule: add 'deinit' command
Am 12.02.2013 18:11, schrieb Phil Hord: > On Wed, Feb 6, 2013 at 4:11 PM, Jens Lehmann 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' > 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? >> + 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 >> +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 modificatio
Re: [PATCH v4] submodule: add 'deinit' command
Phil Hord writes: >> + if test $# = 0 >> + then >> + die "$(eval_gettext "Use '.' if you really want to >> deinitialize all submodules")" >> + fi >> + >> + module_list "$@" | >> + while read mode sha1 stage sm_path >> + do >> + 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. Sounds like a sensible suggestion. > 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,... But isn't it the way to explicitly say "everything under the sun"? After all, what does the message say to "git submodule deinit"? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4] submodule: add 'deinit' command
I haven't tried it yet, but I have some comments. On Wed, Feb 6, 2013 at 4:11 PM, Jens Lehmann wrote: > With "git submodule init" the user is able to tell git he cares about one > or more submodules and wants to have it populated on the next call to "git > submodule update". But currently there is no easy way he could tell git he > does not care about a submodule anymore and wants to get rid of his local > work tree (except he knows a lot about submodule internals and removes the > "submodule.$name.url" setting from .git/config together with the work tree > himself). > > Help those users by providing a 'deinit' command. This removes the whole > submodule. section from .git/config either for the given > submodule(s) or for all those which have been initialized if '.' is > given. Fail if the current work tree contains modifications unless > forced. Complain when for a submodule given on the command line the url > setting can't be found in .git/config, but nonetheless don't fail. > > Add tests and link the man pages of "git submodule deinit" and "git rm" > to assist the user in deciding whether removing or unregistering the > submodule is the right thing to do for him. > > Signed-off-by: Jens Lehmann > --- > > Changes since v3: > - Add deinit to the --force documentation of "git submodule" > - Never remove submodules containing a .git dir, even when forced > - Diagnostic output when "rm -rf" or "mkdir" fails > - More test cases > > > Documentation/git-rm.txt| 4 ++ > Documentation/git-submodule.txt | 18 +++- > git-submodule.sh| 78 ++- > t/t7400-submodule-basic.sh | 100 > > 4 files changed, 198 insertions(+), 2 deletions(-) > > diff --git a/Documentation/git-rm.txt b/Documentation/git-rm.txt > index 92bac27..1d876c2 100644 > --- a/Documentation/git-rm.txt > +++ b/Documentation/git-rm.txt > @@ -149,6 +149,10 @@ files that aren't ignored are present in the submodules > work tree. > Ignored files are deemed expendable and won't stop a submodule's work > tree from being removed. > > +If you only want to remove the local checkout of a submodule from your > +work tree without committing the removal, > +use linkgit:git-submodule[1] `deinit` instead. > + > EXAMPLES > > `git rm Documentation/\*.txt`:: > diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt > index a0c9df8..bc06159 100644 > --- a/Documentation/git-submodule.txt > +++ b/Documentation/git-submodule.txt > @@ -13,6 +13,7 @@ SYNOPSIS > [--reference ] [--] [] > 'git submodule' [--quiet] status [--cached] [--recursive] [--] [...] > 'git submodule' [--quiet] init [--] [...] > +'git submodule' [--quiet] deinit [-f|--force] [--] ... > 'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch] > [--rebase] > [--reference ] [--merge] [--recursive] [--] > [...] > 'git submodule' [--quiet] summary [--cached|--files] [(-n|--summary-limit) > ] > @@ -134,6 +135,19 @@ init:: > the explicit 'init' step if you do not intend to customize > any submodule locations. > > +deinit:: > + Unregister the given submodules, i.e. remove the whole > + `submodule.$name` section from .git/config together with their work > + tree. Further calls to `git submodule update`, `git submodule foreach` > + and `git submodule sync` will skip any unregistered submodules until > + they are initialized again, so use this command if you don't want to > + have a local checkout of the submodule in your work tree anymore. If > + you really want to remove a submodule from the repository and commit > + that use linkgit:git-rm[1] instead. > ++ > +If `--force` is specified, the submodule's work tree will be removed even if > +it contains local modifications. > + > update:: > Update the registered submodules, i.e. clone missing submodules and > checkout the commit specified in the index of the containing > repository. > @@ -213,8 +227,10 @@ OPTIONS > > -f:: > --force:: > - This option is only valid for add and update commands. > + 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 update, throw away local changes in submodules when > switching to a different commit; and always run a checkout operation > in the submodule, even if the commit listed in the index of the > diff --git a/git-submodule.sh b/git-submodule.sh > index 004c034..f1b552f 100755 > --- a/git-submodule.sh > +++ b/git-submodule.sh > @@ -8,6 +8,7 @@ dashless=$(basename "$0" | sed -e 's/-/ /') > USAGE="[--quiet] add [-b ] [-f|--force] [--name ] [--reference > ] [--] [] > or: $dashless [--quiet] status [--ca
