Subject: [RFC] [BUDFIX] 'git rm --cached <submodule>' does not stage the changed .gitmodules

From: Shourya Shukla <[email-masked]>
Date: Sun, 7 Feb 2021 20:11:44 +0530

Hello all,

I was lurking around 'gitgitgadget/git' when I saw this potential BUG
added by Phillipe Blaine (reported by Javier Mora):
https://github.com/gitgitgadget/git/issues/750

Link to the original mail by Javier:
https://lore.kernel.org/git/[email-masked]/

In brief, 'git rm' does not stage the changed '.gitmodules' file when we
use the '--cached' option. Technically speaking, Git used to behave this
way only and hence this is not an unknown case. The test 45 of
't3600-rm.sh' already is prepared for this scenario and checks for
exactly the scenario as Javier describes.

So, my question is, do we need to fix this to make sure that the changed
'.gitmodules' is staged? I feel that we should because: since the SM
becomes irrelevant after executing 'git rm --cached', it's entry in
'.gitmodules' is a plain burden and is of no practical use.

The fault is in this section of 'builtin/rm.c':
https://github.com/git/git/blob/v2.30.0/builtin/rm.c#L378-L402

This part:

	const char *path = list.entry[i].name;
	if (list.entry[i].is_submodule) {
		strbuf_reset(&buf);
		strbuf_addstr(&buf, path);
		if (remove_dir_recursively(&buf, 0))
			die(_("could not remove '%s'"), path);

		removed = 1;
		if (!remove_path_from_gitmodules(path))
			gitmodules_modified = 1;
		continue;
	}

Needs to be executed irrespective of whether '--cached' is passed to the
command or not. In particular, the following if-statement is of utmost
importance:

	if (!remove_path_from_gitmodules(path))
		gitmodules_modified = 1;

Since the variable 'gitmodules_modified' is 0 when we pass 'cached', it
is not staged later here:

	if (gitmodules_modified)
		stage_updated_gitmodules(&the_index);

And its entry is not removed from the file. What should be done about
this? I would appreciate your opinions.

Regards,
Shourya Shukla