submodule: absorb git dir instead of dying on deinit

Commit Message

Mugdha Pattnaik Aug. 26, 2021, 6:33 p.m. UTC

From: mugdha <[e-mail]>

Currently, running 'git submodule deinit' on repos where the
submodule's '.git' is a folder aborts with a message that is not
exactly user friendly. Let's change this to instead warn the user
to rerun the command with '--force'.

This internally calls 'absorb_git_dir_into_superproject()', which
moves the '.git' folder into the superproject and replaces it with
a '.git' file. The rest of the deinit function can operate as it
already does with new-style submodules.

We also edit a test case such that it matches the new behaviour of
deinit.

Suggested-by: Atharva Raykar <[e-mail]>
Signed-off-by: Mugdha Pattnaik <[e-mail]>
---
    submodule: absorb git dir instead of dying on deinit
    
    We also edit a test case such that it matches the new behaviour of
    deinit.
    
    I have changed the 'cp -R ../.git/modules/example .git' to 'mv
    ../.git/modules/example .git' since, at the time of testing, the test
    would fail - deinit now would be moving the '.git' folder into the
    superproject's '.git/modules/' folder, and since this same folder
    already existed before, it was causing errors. So, before running
    deinit, instead of copying the '.git' folder into the submodule, if we
    move it there instead, this functionality can be appropriately tested.
    
    Thank you, Mugdha

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1078%2Fmugdhapattnaik%2Fsubmodule-deinit-absorbgitdirs-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1078/mugdhapattnaik/submodule-deinit-absorbgitdirs-v1
Pull-Request: https://github.com/git/git/pull/1078

 builtin/submodule--helper.c | 27 +++++++++++++++++----------
 t/t7400-submodule-basic.sh  | 10 +++++-----
 2 files changed, 22 insertions(+), 15 deletions(-)


base-commit: c4203212e360b25a1c69467b5a8437d45a373cac

Comments

Bagas Sanjaya Aug. 27, 2021, 7:51 a.m. UTC | #1

On 27/08/21 01.33, Mugdha Pattnaik via GitGitGadget wrote:
> Currently, running 'git submodule deinit' on repos where the
> submodule's '.git' is a folder aborts with a message that is not
> exactly user friendly. Let's change this to instead warn the user
> to rerun the command with '--force'.

Maybe the case of "repo inside the repo", with submodule as repo?

Mugdha Pattnaik Aug. 27, 2021, 1:13 p.m. UTC | #2

On Fri, Aug 27, 2021 at 1:21 PM Bagas Sanjaya <[e-mail]> wrote:
> Maybe the case of "repo inside the repo", with submodule as repo?

I am not certain what exactly you meant, but this patch deals with "Old-form"
submodules. This link (https://git-scm.com/docs/gitsubmodules#_forms)
states that old-style submodules, when deinitialized or deleted, have its Git
directory automatically moved to $GIT_DIR/modules/<name>/ of the
superproject. This is what this patch sets to achieve. Earlier, running deinit
on such a submodule would cause it to fail.

--
Mugdha

Patch

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index ef2776a9e45..253ddce1c41 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1539,16 +1539,23 @@  static void deinit_submodule(const char *path, const char *prefix,
 		struct strbuf sb_rm = STRBUF_INIT;
 		const char *format;
 
-		/*
-		 * protect submodules containing a .git directory
-		 * NEEDSWORK: instead of dying, automatically call
-		 * absorbgitdirs and (possibly) warn.
-		 */
-		if (is_directory(sub_git_dir))
-			die(_("Submodule work tree '%s' contains a .git "
-			      "directory (use 'rm -rf' if you really want "
-			      "to remove it including all of its history)"),
-			    displaypath);
+		if (is_directory(sub_git_dir)) {
+			if (!(flags & OPT_FORCE))
+					die(_("Submodule work tree '%s' contains a "
+						  ".git directory (use --force if you want "
+						  "to move its contents to superproject's "
+						  "module folder and convert .git to a file "
+						  "and then proceed with deinit)"),
+						displaypath);
+
+			if (!(flags & OPT_QUIET)) {
+					warning(_("Submodule work tree '%s' contains a .git "
+							  "directory (this will be replaced with a "
+							  ".git file by using absorbgitdirs"),
+							displaypath);
+					absorb_git_dir_into_superproject(displaypath, flags);
+			}
+		}
 
 		if (!(flags & OPT_FORCE)) {
 			struct child_process cp_rm = CHILD_PROCESS_INIT;
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index cb1b8e35dbf..3df71478d06 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -1182,18 +1182,18 @@  test_expect_success 'submodule deinit is silent when used on an uninitialized su
 	rmdir init example2
 '
 
-test_expect_success 'submodule deinit fails when submodule has a .git directory even when forced' '
+test_expect_success 'submodule deinit fails when submodule has a .git directory unless forced' '
 	git submodule update --init &&
 	(
 		cd init &&
 		rm .git &&
-		cp -R ../.git/modules/example .git &&
+		mv ../.git/modules/example .git &&
 		GIT_WORK_TREE=. git config --unset core.worktree
 	) &&
 	test_must_fail git submodule deinit init &&
-	test_must_fail git submodule deinit -f init &&
-	test -d init/.git &&
-	test -n "$(git config --get-regexp "submodule\.example\.")"
+	git submodule deinit -f init &&
+	! test -d init/.git &&
+	test -z "$(git config --get-regexp "submodule\.example\.")"
 '
 
 test_expect_success 'submodule with UTF-8 name' '