[PATCH v2 1/1] rm: stage submodule removal from '.gitmodules' when using '--cached'

From

SSShourya Shukla <periperidip@gmail.com>

Date

Feb 22, 2021, 17:26 UTC

Message-ID

<20210222172623.69313-2-periperidip@gmail.com>

In-Reply-To

<20210222172623.69313-1-periperidip@gmail.com>

Reported-by: Javier Mora <javier.moradesambricio@rtx.com>
Helped-by: Phillipe Blain <levraiphilippeblain@gmail.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Shourya Shukla <periperidip@gmail.com>
---
 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