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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 6, 2012, 06:57 UTC
Message-ID: <7v1ukppear.fsf@alter.siamese.dyndns.org>
In-Reply-To: <4FF4AB1B.60805@web.de>

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

Show 9 quoted lines

> Currently using "git rm" on a populated submodule produces this error:
> 	fatal: git rm: '<submodule path>': Is a directory
> Using it on an unpopulated submodule removes the empty directory silently
> and removes the gitlink from the index, while it doesn't do the latter
> when the submodule is populated but errors out.
>
> While the error technically correct (the submodule directory can't be
> removed because it still contains the checked out work tree) rm could do
> better because it knows it is a submodule.

Correct in principle, but the definition of "better" could be open to discussion.

> It should remove the gitlink
> from the index no matter if it is populated or not.

Why? If you have a regular file that has changes with respect to the index entry, the index version is kept as well as the working tree version. You can "rm --cached" to remove the index entry, and you can "rm --force" to remove both, nuking the change you made to the working tree, but we do not touch such a "dirty" entry without any explicit option.

> Also not being able to
> remove a submodule directory isn't an error but should only issue a
> warning to inform the user about that fact while removing the gitlink from
> the index nonetheless.

You are repeating yourself without justification.

> Change "git rm" so it only issues a warning if a populated submodule
> cannot be removed.

I find this part iffy due to the above. With --cached, perhaps, but without any option, I do not think I heard a sound justification behind this change.

> Also apply the same policy as for regular files and
> require forcing when the submodules HEAD is different than what is
> recorded in the index.

I think the "policy" for regular files is that "git rm $path" errors out to avoid losing information in $path. Even if the HEAD in the submodule points at the same commit as recorded in the index, if the submodule directory has other changes that (cd $path && git status) would report, we would not want to remove it, no?

I am not sure if the difference between $path/.git/HEAD and :$path (the version in the index) matters. Maybe it does, maybe it doesn't.

One possible sane behaviour of "git rm $path" might be:

 - If --force is given, remove it from the index and from the
   working tree (i.e. "rm -rf $path"), but use the "gitfile"
   facility to save $path/.git away to $GIT_DIR/modules/$name; error
   out if the submodule directory $path cannot be removed like this.
   We would probably want to remove "submodule.<name>.*" entries in
   .gitmodules for <name> for which "submodule.<name>.path" matches
   the $path.

 - If --cached is given, remove it from the index if the version in
   the index match either HEAD or the $path/.git/HEAD, without
   touching the working tree.  This is consistent with what happens
   to a regular file.

 - If neither --force nor --cached is given, run an equivalent of
   (cd $path && git status), and also check if $path/.git/HEAD
   matches the index version.  Error out if the submodule directory
   is dirty (again I am not sure about this part).  If the submodule
   directory is clean, do the same as the case with --force.

> While this changes behavior of "git rm", it only fixes an error where it
> never worked properly.

It stops an error from being issued, but I am not convinced that the new behaviour is necessarily a sensible one. A change that stops an error and performs a random operation is not necessarily a "fix".