Re: [PATCH 1/2] rm: changes in the '.gitmodules' are staged after using '--cached'

From: Philippe Blain <levraiphilippeblain@gmail.com>
Date: Feb 18, 2021, 20:14 UTC
Message-ID: <0577f84b-f594-6b8a-76a2-29fb9453ee25@gmail.com>
In-Reply-To: <20210218184931.83613-2-periperidip@gmail.com>

Hello Shourya,

Le 2021-02-18 à 13:49, Shourya Shukla a écrit :

Show 5 quoted lines

> Earlier, on doing a 'git rm --cached <submodule>' did not modify the
> '.gitmodules' entry of the submodule in question hence the file was not
> staged. Change this behaviour to remove the entry of the submodule from
> the '.gitmodules', something which might be more expected of the
> command.

We prefer using the imperative mood for the commit message title, the present tense for describing the actual state of the code, and finally the imperative mood again to give order to the code base to change its behaviour [1]. So something like the following would fit more into the project's conventions:

     rm: stage submodule removal from '.gitmodules' when using '--cached'

     Currently, using 'git rm --cached <submodule>' removes submodule <submodule> from the index
     and leaves the submodule working tree intact in the superproject working tree,
     but does not stage any changes to the '.gitmodules' file, in contrast to
     'git rm <submodule>', which removes both the submodule and its configuration
     in '.gitmodules' from the worktree and index.
     
     Fix this inconsistency by also staging the removal of the configuration of the
     submodule from the '.gitmodules' file, leaving the worktree copy intact, a behaviour
     which is more in line with what might be expected when using '--cached'.

However, this is *not* what you patch does; it also removes the relevant section from the '.gitmodules' file *in the worktree*, which is not acceptable because it is exactly contrary to what '--cached' means.

This was verified by running Javier's demonstration script that I included in the Gitgitgadget issue [2], which I copy here:

~~~ rm -rf some_submodule top_repo

mkdir some_submodule cd some_submodule git init echo hello > hello.txt git add hello.txt git commit -m 'First commit of submodule' cd .. mkdir top_repo cd top_repo git init echo world > world.txt git add world.txt git commit -m 'First commit of top repo' git submodule add ../some_submodule git status # both some_submodule and .gitmodules staged git commit -m 'Added submodule' git rm --cached some_submodule git status # only some_submodule staged ~~~

With your changes, at the end '.gitmodules' is modified in both the worktree and the index, whereas we would want it to be modified *only* in the index.

And we would want it to be staged for deletion (and only deleting the config entry and keeping an empty ".gitmodules' file in the index) if the user is removing the only submodule in the superproject.

> ---
>   builtin/rm.c | 48 +++++++++++++++++++++++++++---------------------
>   1 file changed, 27 insertions(+), 21 deletions(-)
>

Once implemeted correctly (leaving the worktree version of '.gitmodules' intact), that patch should also change the documentation to stay up-to-date, since the "Submodules" section of Documentation/git-rm.txt states [3]:

     If it exists the submodule.<name> section in the gitmodules[5] file will
     also be removed and that file will be staged (unless --cached or -n are used).

Cheers, Philippe.

[1] https://git-scm.com/docs/SubmittingPatches#describe-changes [2] https://github.com/gitgitgadget/git/issues/750 [3] https://git-scm.com/docs/git-rm#_submodules