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

Hello all,

In the series of mail exchanges between me, Junio and Phillipe:
https://lore.kernel.org/git/20210207144144.GA42182@konoha/

The outcome of the BUG(?) reported by Javier Mora in the following mail:
https://lore.kernel.org/git/ea91c2ea29064079914f6a522db5115a@UUSALE0Z.utcmail.com/

Was not fully decided (i.e., whether it should be fixed or not) due to
the potential "pollution" of the 'git rm' command. Here is my patch
series attempting to fix the situation reported by Javier and make sure
that doing a 'git rm --cached <submodule>' deletes the entry of the
submodule in question from the '.gitmodules'. I have tried to keep the
changes as precise as possible without adding much extra stuff.
Reviews and comments are appreciated.

Thank you Phillipe, Junio and Christian for their comments on the patch.

Regards,
Shourya Shukla

Shourya Shukla (2):
  rm: changes in the '.gitmodules' are staged after using '--cached'
  t3600: amend test 46 to check for '.gitmodules' modification

 builtin/rm.c  | 48 +++++++++++++++++++++++++++---------------------
 t/t3600-rm.sh |  7 +++----
 2 files changed, 30 insertions(+), 25 deletions(-)

-- 
2.25.1

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.

Reported-by: Javier Mora <redacted>
Signed-off-by: Shourya Shukla <redacted>
---
 builtin/rm.c | 48 +++++++++++++++++++++++++++---------------------
 1 file changed, 27 insertions(+), 21 deletions(-)

diff --git a/builtin/rm.c b/builtin/rm.c
index 4858631e0f..0b74f50bfe 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c

@@ -254,7 +254,7 @@ static struct option builtin_rm_options[] = {
 int cmd_rm(int argc, const char **argv, const char *prefix)
 {
 	struct lock_file lock_file = LOCK_INIT;
-	int i;
+	int i, removed = 0, gitmodules_modified = 0;
 	struct pathspec pathspec;
 	char *seen;
 

@@ -365,30 +365,32 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 	if (show_only)
 		return 0;
 
-	/*
-	 * Then, unless we used "--cached", remove the filenames from
-	 * the workspace. If we fail to remove the first one, we
-	 * abort the "git rm" (but once we've successfully removed
-	 * any file at all, we'll go ahead and commit to it all:
-	 * by then we've already committed ourselves and can't fail
-	 * in the middle)
-	 */
-	if (!index_only) {
-		int removed = 0, gitmodules_modified = 0;
-		struct strbuf buf = STRBUF_INIT;
-		for (i = 0; i < list.nr; i++) {
-			const char *path = list.entry[i].name;
-			if (list.entry[i].is_submodule) {
+	for (i = 0; i < list.nr; i++) {
+		const char *path = list.entry[i].name;
+		if (list.entry[i].is_submodule) {
+			/*
+			 * Then, unless we used "--cached", remove the filenames from
+			 * the workspace. If we fail to remove the first one, we
+			 * abort the "git rm" (but once we've successfully removed
+			 * any file at all, we'll go ahead and commit to it all:
+			 * by then we've already committed ourselves and can't fail
+			 * in the middle)
+			 */
+			if (!index_only) {
+				struct strbuf buf = STRBUF_INIT;
 				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;
+				strbuf_release(&buf);
 			}
+			if (!remove_path_from_gitmodules(path))
+				gitmodules_modified = 1;
+			continue;
+		}
+		if (!index_only) {
 			if (!remove_path(path)) {
 				removed = 1;
 				continue;

@@ -396,11 +398,15 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 			if (!removed)
 				die_errno("git rm: '%s'", path);
 		}
-		strbuf_release(&buf);
-		if (gitmodules_modified)
-			stage_updated_gitmodules(&the_index);
 	}
 
+	/*
+	 * Remove the entry of the submodule from the ".gitmodules" irrespective
+	 * whether "--cached" was passed or not.
+	 */
+	if (gitmodules_modified)
+		stage_updated_gitmodules(&the_index);
+
 	if (write_locked_index(&the_index, &lock_file,
 			       COMMIT_LOCK | SKIP_IF_UNCHANGED))
 		die(_("Unable to write new index file"));

-- 
2.25.1

Following commit e5a439dc71 (rm: changes in the '.gitmodules' are
staged after using '--cached', 2021-02-18), amend test 46 of the script
to ensure that the test also checks for '.gitmodules' modification after
a 'git rm --cached <submodule>' i.e., the entry of the submodule in
question is removed from the file.

Signed-off-by: Shourya Shukla <redacted>
---
 t/t3600-rm.sh | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index 7547f11a5c..45aff97b90 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh

@@ -309,6 +309,7 @@ cat >expect.modified_untracked <<EOF
 EOF
 
 cat >expect.cached <<EOF
+M  .gitmodules
 D  submod
 EOF
 

@@ -390,16 +391,14 @@ test_expect_success 'rm of a populated submodule with different HEAD fails unles
 	test_must_fail git config -f .gitmodules submodule.sub.path
 '
 
-test_expect_success 'rm --cached leaves work tree of populated submodules and .gitmodules alone' '
+test_expect_success 'rm --cached leaves work tree of populated submodules alone' '
 	git reset --hard &&
 	git submodule update &&
 	git rm --cached submod &&
 	test_path_is_dir submod &&
 	test_path_is_file submod/.git &&
 	git status -s -uno >actual &&
-	test_cmp expect.cached actual &&
-	git config -f .gitmodules submodule.sub.url &&
-	git config -f .gitmodules submodule.sub.path
+	test_cmp expect.cached actual
 '
 
 test_expect_success 'rm --dry-run does not touch the submodule or .gitmodules' '

-- 
2.25.1

Hello Shourya,

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

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

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

Following commit e5a439dc71 (rm: changes in the '.gitmodules' are
staged after using '--cached', 2021-02-18), amend test 46 of the script
to ensure that the test also checks for '.gitmodules' modification after
a 'git rm --cached <submodule>' i.e., the entry of the submodule in
question is removed from the file.

You can't reference your previous commit by hash, since it has not yet
made its way to Git's master branch. Usually what is done in that case is writing
"In the previous commit, we fixed *** so that *** now does ***. Change *** accordingly"
or something like this.

However, in the present case the changes to the test should be squashed into
the changes to the code, if not the tests are broken when they are run
on patch 1/2. In this project *all* commits of a topic branch should pass
the test suite before the topic is merged, not just the tip commit.

Regarding the changes themselves, they should be tweaked along with patch 1/2
to test the correct behaviour (not modifying the working tree copy of '.gitmodules'.

Le 2021-02-18 à 15:14, Philippe Blain a écrit :

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.

Sorry for the typo, a "not" is missing:

And we would want it to be staged for deletion (and *not* 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.

P.S. I CC'ed "cousteaulecommandant" who was CC'ed on the original bug report
since Javier's @rtx.com address now bounces.

On 18/02 03:14, Philippe Blain wrote:

Hello Shourya,

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

quoted

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:

I have no idea how I missed that one. Apologies, will make the change.

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

Okay. I will use the above message.

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.

Correct.

quoted

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

Understood. I have to let the working tree '.gitmodules' be left as-is
and only change the copy in the index.

Hello all,

This is the v2 of the patch with the same title. After suggestions from
Phillipe and Junio, I have improved the commit messages, squashed the
two commits and did the following:

	1. Change the definition and declaration of
	   'remove_path_from_gitmodules()' to account in for the
	   'index_only' variable denoting the presence of '--cached'
	   option in the 'git rm' command. In case of the variable being
	   1, remove the submodule entry from the index copy of the
	   '.gitmodules' else do the same for the working tree copy.

	2. Remove the 'gitmodules_modified' variable and instead call
	   'stage_updated_gitmodules()' just after the
	   'remove_path_from_gitmodules()' call.

	3. Account for the above changes in 't3600' and make changes in
	   the same.

I am facing some problem with point (2) in the sense that what Junio
suggested in his mail:
https://lore.kernel.org/git/xmqqblchdoej.fsf@gitster.g/

-----8<-----
 - Since the previous point will maintain the correct contents in
   the index in all cases, get rid of gitmodules_modified and calls
   to stage_updated_gitmodules().  The call to write_locked_index()
   at the end will take care of the actual writing out of the index.
----->8-----

I am not able to get rid of the 'stage_updated_gitmodules()' call
without failing tests in t3600 (t3600.4 is the first one to fail). What
am I doing wrong here?

Comments and reviews are appreciated. Thank you Phillipe and Junio for
the constructive feedback on the v1!

Regards,
Shourya Shukla

Shourya Shukla (1):
  rm: stage submodule removal from '.gitmodules' when using '--cached'

 builtin/rm.c  | 42 +++++++++++++++++++++---------------------
 submodule.c   |  5 +++--
 submodule.h   |  2 +-
 t/t3600-rm.sh |  6 ++----
 4 files changed, 27 insertions(+), 28 deletions(-)

-- 
2.25.1

Currently, using 'git rm --cached <submodule>' removes the 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 entry 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'.

Achieve this by modifying the function 'remove_path_from_gitmodules()'
to also take in the parameter 'index_only' denoting the presence of
the '--cached' option. If present, remove the submodule entry from the
copy of the '.gitmodules' in the index otherwise, do the same for the
working tree copy.

While at it, also change the test 46 of the test script 't3600-rm.sh' to
incorporate for the above changes.

Reported-by: Javier Mora <redacted>
Helped-by: Phillipe Blain [off-list ref]
Helped-by: Junio C Hamano [off-list ref]
Signed-off-by: Shourya Shukla <redacted>
---
 builtin/rm.c  | 42 +++++++++++++++++++++---------------------
 submodule.c   |  5 +++--
 submodule.h   |  2 +-
 t/t3600-rm.sh |  6 ++----
 4 files changed, 27 insertions(+), 28 deletions(-)

diff --git a/builtin/rm.c b/builtin/rm.c
index 4858631e0f..5854ef0996 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c

@@ -254,7 +254,7 @@ static struct option builtin_rm_options[] = {
 int cmd_rm(int argc, const char **argv, const char *prefix)
 {
 	struct lock_file lock_file = LOCK_INIT;
-	int i;
+	int i, removed = 0;
 	struct pathspec pathspec;
 	char *seen;
 

@@ -365,30 +365,33 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 	if (show_only)
 		return 0;
 
-	/*
-	 * Then, unless we used "--cached", remove the filenames from
-	 * the workspace. If we fail to remove the first one, we
-	 * abort the "git rm" (but once we've successfully removed
-	 * any file at all, we'll go ahead and commit to it all:
-	 * by then we've already committed ourselves and can't fail
-	 * in the middle)
-	 */
-	if (!index_only) {
-		int removed = 0, gitmodules_modified = 0;
-		struct strbuf buf = STRBUF_INIT;
-		for (i = 0; i < list.nr; i++) {
-			const char *path = list.entry[i].name;
-			if (list.entry[i].is_submodule) {
+	for (i = 0; i < list.nr; i++) {
+		const char *path = list.entry[i].name;
+		if (list.entry[i].is_submodule) {
+			/*
+			 * Then, unless we used "--cached", remove the filenames from
+			 * the workspace. If we fail to remove the first one, we
+			 * abort the "git rm" (but once we've successfully removed
+			 * any file at all, we'll go ahead and commit to it all:
+			 * by then we've already committed ourselves and can't fail
+			 * in the middle)
+			 */
+			if (!index_only) {
+				struct strbuf buf = STRBUF_INIT;
 				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;
+				strbuf_release(&buf);
 			}
+			if (!remove_path_from_gitmodules(path, index_only))
+				stage_updated_gitmodules(&the_index);
+
+			continue;
+		}
+		if (!index_only) {
 			if (!remove_path(path)) {
 				removed = 1;
 				continue;

@@ -396,9 +399,6 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 			if (!removed)
 				die_errno("git rm: '%s'", path);
 		}
-		strbuf_release(&buf);
-		if (gitmodules_modified)
-			stage_updated_gitmodules(&the_index);
 	}
 
 	if (write_locked_index(&the_index, &lock_file,

diff --git a/submodule.c b/submodule.c
index 9767ba9893..6ce8c8d0d8 100644
--- a/submodule.c
+++ b/submodule.c

@@ -131,7 +131,7 @@ int update_path_in_gitmodules(const char *oldpath, const char *newpath)
  * path is configured. Return 0 only if a .gitmodules file was found, a section
  * with the correct path=<path> setting was found and we could remove it.
  */
-int remove_path_from_gitmodules(const char *path)
+int remove_path_from_gitmodules(const char *path, int index_only)
 {
 	struct strbuf sect = STRBUF_INIT;
 	const struct submodule *submodule;

@@ -149,7 +149,8 @@ int remove_path_from_gitmodules(const char *path)
 	}
 	strbuf_addstr(&sect, "submodule.");
 	strbuf_addstr(&sect, submodule->name);
-	if (git_config_rename_section_in_file(GITMODULES_FILE, sect.buf, NULL) < 0) {
+	if (git_config_rename_section_in_file(index_only ? GITMODULES_INDEX :
+					      GITMODULES_FILE, sect.buf, NULL) < 0) {
 		/* Maybe the user already did that, don't error out here */
 		warning(_("Could not remove .gitmodules entry for %s"), path);
 		strbuf_release(&sect);

diff --git a/submodule.h b/submodule.h
index 4ac6e31cf1..4d8707d911 100644
--- a/submodule.h
+++ b/submodule.h

@@ -43,7 +43,7 @@ int is_gitmodules_unmerged(const struct index_state *istate);
 int is_writing_gitmodules_ok(void);
 int is_staging_gitmodules_ok(struct index_state *istate);
 int update_path_in_gitmodules(const char *oldpath, const char *newpath);
-int remove_path_from_gitmodules(const char *path);
+int remove_path_from_gitmodules(const char *path, int index_only);
 void stage_updated_gitmodules(struct index_state *istate);
 void set_diffopt_flags_from_submodule_config(struct diff_options *,
 					     const char *path);

diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index 7547f11a5c..c0ca4be5a1 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh

@@ -390,16 +390,14 @@ test_expect_success 'rm of a populated submodule with different HEAD fails unles
 	test_must_fail git config -f .gitmodules submodule.sub.path
 '
 
-test_expect_success 'rm --cached leaves work tree of populated submodules and .gitmodules alone' '
+test_expect_success 'rm --cached leaves work tree of populated submodules alone' '
 	git reset --hard &&
 	git submodule update &&
 	git rm --cached submod &&
 	test_path_is_dir submod &&
 	test_path_is_file submod/.git &&
 	git status -s -uno >actual &&
-	test_cmp expect.cached actual &&
-	git config -f .gitmodules submodule.sub.url &&
-	git config -f .gitmodules submodule.sub.path
+	test_cmp expect.cached actual
 '
 
 test_expect_success 'rm --dry-run does not touch the submodule or .gitmodules' '

-- 
2.25.1