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

From: Jens Lehmann <jens.lehmann@web.de>
Date: Jul 7, 2012, 12:51 UTC
Message-ID: <4FF830D6.7080708@web.de>
In-Reply-To: <7v1ukppear.fsf@alter.siamese.dyndns.org>

Am 06.07.2012 08:57, schrieb Junio C Hamano:

Show 14 quoted lines

> Jens Lehmann <Jens.Lehmann@web.de> writes:
>> 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.

Doh. I don't know how I got the idea it would be so, but a quick test with checkout and rebase showed they ignore if a submodules HEAD is different from the commit recorded. So plain rm should do the same as long as it doesn't touch the submodule work tree and I'll remove checking the HEAD from patch 1. I'll prepare v2 which will also include an updated commit message.

Show 20 quoted lines

> 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.

What you describe here is exactly how I think "git submodule rm" and "git rm --recurse-submodules" should behave.

The questions remaining for me are:

* What should a "git rm --no-recurse-submodules" do?

  I think it should try to follow the policy git core commands use:

  - don't touch the submodule's work tree

  - remove the submodule directory if it is empty and warn if not
    (currently it dies if not, to change that to a warning is the
    subject of patch 1)

  The more difficult question is if it should remove the submodule
  entry from .gitmodules (patch 2). As that file is part of the
  superproject's work tree and core git already learned to read
  configuration options for status, diff and fetch from it I think
  it's a good idea to help the user by doing so (but maybe we should
  make this configurable and/or add an option to enable/disable it).
  But on the other hand maybe users expect this only to happen when
  they use "git submodule rm" and "git rm" should leave .gitmodules
  alone?

* What should the default behavior of "git rm" be.

  I tend to think that as all other core git commands never
  manipulate a submodule's work tree without the --recurse-submodules
  option, rm should do the same. So I think we should default to
  the --no-recure-submodules case described above to not surprise the
  user. That makes checking the submodule for modifications unnecessary
  until the --recure-submodules option is implemented.

Does that make sense?