Re: [RFC PATCH 1/2] rm: don't fail when removing populated submodules

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 9, 2012, 02:17 UTC
Message-ID: <7vhathn0f4.fsf@alter.siamese.dyndns.org>
In-Reply-To: <4FF9A261.3040907@web.de>

Jens Lehmann <Jens.Lehmann@web.de> writes:

Show 10 quoted lines

>>> What you describe here is exactly how I think "git submodule rm" and
>>> "git rm --recurse-submodules" should behave.
>> 
>> If you have a directory A with a file B in it (i.e. A/B), "git rm A"
>> is refused and you have to say "git rm -r A".  So I can see why the
>> above description of the mine is wrong with respect to "-r" option
>> (all cases should fail if you did not give "-r" option).
>
> I think that depends on how you see submodules in the context of
> the superproject.

I am OK with "git rm path" removing the submodule working tree and the index entry for path without -r (of course "git rm dir" would not remove submodule "dir/path" in a plain directory "dir" and needs to be spelled "git rm -r dir" or "git rm dir/path" but that is the same if "path" were a regular file and a submodule is not special); thinking about it again, I think it makes more sense, because a submodule is just one single path entity in the context of the superproject.

Show 14 quoted lines

>> But I do not think "git rm" needs "--recurse-submodules".  Wasn't
>> "--recurse-submodules" the option to control, when you tell Git to
>> do something to submodule "A", what should happen to submodules
>> contained in the submodule "A" (e.g. "A/B" that appears at path "B"
>> that itself is a separate project bound as a submodule to "A")?
>
> Nope. Only the "--recursive" option to the git submodule script
> works like that (and almost everyone seems to use that option by
> default anyway). But for all commands that understand the
> "--recurse-submodule" option (currently these are clone, fetch,
> merge, pull and push) that means "include submodules in what you
> do and don't stop at the first level but recurse all the way down".
> Without this option they won't even touch the first level of
> submodules.

OK, but what does "rm --no-recurse-submodules path" could possibly mean in that case? If you remove "path" by definition anything underneath "path" cannot be remain, so in the context of "rm", once you decide to remove submodule at "path", not recursing is an option.

So I still think "--recurse-submodules" does not make any sense to the "rm" command. I would understand a "Do not attempt to remove submodules and ignore their existence altogether" option, even though I do not think it is useful.

> All other core commands happily change the index without updating
> the submodule's work tree.

What are "all other core commands"? "fetch" by definition does not touch working tree inside or outside working tree. "add" is about recording the state from the working tree to the index, and following the earlier point you raised (I unfortunately culled from the quote), as the rest of core Git considers a submodule a single path entity in the context of the superproject, "the state from the working tree" for the submodule is where its HEAD points at, so it is correct not to look at the working tree.

Without going outside the context of "rm", I think the current behaviour of "git rm path" for submodule is merely punting---erroring out against a request for an unimplemented feature.

If you ask an unpopulated submodule to be removed, we could simply rmdir() it and remove the entry from the index, but that is far insufficient for handling a submodule repository that is already "init"ed. And we did not want to plug the "check if removal will lose any state from the submodule repository" logic because the information is no use for us for a long time until we added the "gitfile" support to allow us to relocate path/.git for the submodule safely away when we remove the working tree of it.

But now we do have gitfile, so we could remove submodule working tree. I think not erroring out and removing only the index entry is a irresponsible thing to do. It would mean that we pretend as if a feature that was earlier unimplemented (hence errored out) is now supported, but it does not do what the user asked us to do, no?

I do not know what other commands you have in mind, but some of them may be similar "recursing down and performing an operation that potentially can fail is too complex, and we are not sure if we have enough sequencing infrastructure to guide the user to sort out the mess in the half-result, so let's punt and not to that part and have the user sort out" half-features, and if that is the case, I do not think it is prudent to model "rm", which is something that we now could implement properly, with the necessary infrastructure already in place, after them.