[PATCH] submodule: absorb git dir instead of dying on deinit

From: mugdha <redacted>

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 <redacted>
Signed-off-by: Mugdha Pattnaik <redacted>
---
    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(-)

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

base-commit: c4203212e360b25a1c69467b5a8437d45a373cac
-- 
gitgitgadget

From: mugdha <redacted>

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 <redacted>
Signed-off-by: Mugdha Pattnaik <redacted>
---
    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.
    
    Changes since v1:
    
     * Removed extra indent within the if statements
     * Moved absorb_git_dir_into_superproject() call outside the if
       condition checking for --quiet flag
    
    Thank you, Mugdha

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

Range-diff vs v1:

 1:  58814531d17 ! 1:  37c9b598fde submodule: absorb git dir instead of dying on deinit
     @@ builtin/submodule--helper.c: static void deinit_submodule(const char *path, cons
      -			    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)"),
     ++				die(_("Submodule work tree '%s' contains a "
     ++					  ".git directory.\nUse --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);
      +
     -+			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);
     -+			}
     ++			absorb_git_dir_into_superproject(displaypath, flags);
     ++
      +		}
       
       		if (!(flags & OPT_FORCE)) {


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

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index ef2776a9e45..4730dc141d4 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c

@@ -1539,16 +1539,24 @@ 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.\nUse --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' '

base-commit: c4203212e360b25a1c69467b5a8437d45a373cac
-- 
gitgitgadget

"Mugdha Pattnaik via GitGitGadget" [off-list ref] writes:

From: mugdha <redacted>

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 makes sense, a lot of commands seem to follow this pattern of
requiring a user to '--force' when the behaviour might not be different
from what is normally expected.

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.

Nice. Just like what the NEEDSWORK comment suggested.

Looks okay to me. The quiet flag suppresses the warning, which is nice.

The original test case was written with the expectation that the deinit
won't happen. But now that it will, I suppose an mv was necessary so
that absorbgitdirs can write the gitdir back to the original place
successfully.

 		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\.")"
 '

Likewise here. The test case has been flipped to reflect the new
behaviour.

 test_expect_success 'submodule with UTF-8 name' '

base-commit: c4203212e360b25a1c69467b5a8437d45a373cac

BTW, I see you linked downthread the documentation[1] which says:

  When [old-form submodules] deinitialized or deleted (see below), the
  submodule’s Git directory is automatically moved to
  $GIT_DIR/modules/<name>/ of the superproject.

This was not what Git was doing before this patch, it used to die() out
instead. So I think this actually is a bug fix (although I am not sure
why the test suite also did not agree with the documentation).

Either way, I think this is a welcome change. Thanks for the patch!

[1] https://git-scm.com/docs/gitsubmodules#_forms

From: mugdha <redacted>

Currently, running 'git submodule deinit' on repos where the
submodule's '.git' is a directory, 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 dir 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 <redacted>
Signed-off-by: Mugdha Pattnaik <redacted>
---
    submodule: absorb git dir instead of dying on deinit
    
    Changes since v2:
    
     * Replaced all instances of the word "folder" with either "directory"
       or "git dir"
    
    Thank you, Mugdha
    
    cc: Bagas Sanjaya bagasdotme@gmail.com
    
    cc: Atharva Raykar raykar.ath@gmail.com

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

Range-diff vs v2:

 1:  37c9b598fde ! 1:  1ac65b2458b submodule: absorb git dir instead of dying on deinit
     @@ Commit message
          submodule: absorb git dir instead of dying on deinit
      
          Currently, running 'git submodule deinit' on repos where the
     -    submodule's '.git' is a folder aborts with a message that is not
     +    submodule's '.git' is a directory, 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
     +    moves the git dir 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.
      


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

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index ef2776a9e45..4730dc141d4 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c

@@ -1539,16 +1539,24 @@ 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.\nUse --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' '

base-commit: c4203212e360b25a1c69467b5a8437d45a373cac
-- 
gitgitgadget

From: mugdha <redacted>

Currently, running 'git submodule deinit' on repos where the
submodule's '.git' is a directory, 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 dir 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 <redacted>
Signed-off-by: Mugdha Pattnaik <redacted>
---
    submodule: absorb git dir instead of dying on deinit
    
    Changes since v3:
    
     * Replaced 1 instance of the word "folder" with "directory"
     * Fixed tab spacing
    
    Thank you, Mugdha

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

Range-diff vs v3:

 1:  1ac65b2458b ! 1:  7460fc0e12a submodule: absorb git dir instead of dying on deinit
     @@ builtin/submodule--helper.c: static void deinit_submodule(const char *path, cons
      +		if (is_directory(sub_git_dir)) {
      +			if (!(flags & OPT_FORCE))
      +				die(_("Submodule work tree '%s' contains a "
     -+					  ".git directory.\nUse --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);
     ++				      ".git directory.\nUse --force if you want "
     ++				      "to move its contents to superproject's "
     ++				      "module directory 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);
     ++					  "directory. This will be replaced with a "
     ++					  ".git file by using absorbgitdirs."),
     ++					displaypath);
      +
      +			absorb_git_dir_into_superproject(displaypath, flags);
      +


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

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index ef2776a9e45..040b26f149d 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c

@@ -1539,16 +1539,24 @@ 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.\nUse --force if you want "
+				      "to move its contents to superproject's "
+				      "module directory 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' '

base-commit: c4203212e360b25a1c69467b5a8437d45a373cac
-- 
gitgitgadget

On Fri, Aug 27, 2021 at 8:53 PM Mugdha Pattnaik via GitGitGadget
[off-list ref] wrote:

From: mugdha <redacted>

Currently, running 'git submodule deinit' on repos where the
submodule's '.git' is a directory, 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'.

It seems to me that this patch fell into the cracks. Or did I miss something?

This internally calls 'absorb_git_dir_into_superproject()', which
moves the git dir 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 <redacted>
Signed-off-by: Mugdha Pattnaik <redacted>

On Fri, Aug 27 2021, Mugdha Pattnaik via GitGitGadget wrote:

I see we don't have a "test_dir_is_missing" for the pre-image, but
shouldn't the post-image "! test -d" just be a test_path_is_missing?
I.e. should this pass if .git is a file? Probably not...

On Tue, Sep 28, 2021 Ævar Arnfjörð Bjarmason [off-list ref] wrote:

shouldn't the post-image "! test -d" just be a test_path_is_missing?
I.e. should this pass if .git is a file? Probably not...

I agree, this test should not pass if .git is a file. I'll change it to
"test_path_is_missing" and send an update to the patch.

From: mugdha <redacted>

Currently, running 'git submodule deinit' on repos where the
submodule's '.git' is a directory, 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 dir 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 <redacted>
Signed-off-by: Mugdha Pattnaik <redacted>
---
    submodule: absorb git dir instead of dying on deinit
    
    Changes since v4:
    
     * Changed test case from "! test -d" to "test_path_is_missing"
    
    Thanks
    Mugdha

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

Range-diff vs v4:

 1:  7460fc0e12a = 1:  c39cd681e71 submodule: absorb git dir instead of dying on deinit


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

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index ef2776a9e45..040b26f149d 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c

@@ -1539,16 +1539,24 @@ 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.\nUse --force if you want "
+				      "to move its contents to superproject's "
+				      "module directory 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' '

base-commit: c4203212e360b25a1c69467b5a8437d45a373cac
-- 
gitgitgadget

From: mugdha <redacted>

Currently, running 'git submodule deinit' on repos where the
submodule's '.git' is a directory, 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 dir 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 <redacted>
Signed-off-by: Mugdha Pattnaik <redacted>
---
    submodule: absorb git dir instead of dying on deinit
    
    Changes since v5:
    
     * Fixed accidental submission of old version
    
    Changes since v4:
    
     * Changed test case from "! test -d" to "test_path_is_missing"
    
    Thank you,
    Mugdha

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

Range-diff vs v5:

 1:  c39cd681e71 ! 1:  384a6742388 submodule: absorb git dir instead of dying on deinit
     @@ t/t7400-submodule-basic.sh: test_expect_success 'submodule deinit is silent when
      -	test -d init/.git &&
      -	test -n "$(git config --get-regexp "submodule\.example\.")"
      +	git submodule deinit -f init &&
     -+	! test -d init/.git &&
     ++	test_path_is_missing init/.git &&
      +	test -z "$(git config --get-regexp "submodule\.example\.")"
       '
       


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

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index ef2776a9e45..040b26f149d 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c

@@ -1539,16 +1539,24 @@ 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.\nUse --force if you want "
+				      "to move its contents to superproject's "
+				      "module directory 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..f35479ea634 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_path_is_missing init/.git &&
+	test -z "$(git config --get-regexp "submodule\.example\.")"
 '
 
 test_expect_success 'submodule with UTF-8 name' '

base-commit: c4203212e360b25a1c69467b5a8437d45a373cac
-- 
gitgitgadget

From: Mugdha Pattnaik <redacted>

Currently, running 'git submodule deinit' on repos where the
submodule's '.git' is a directory, aborts with a message that is not
exactly user friendly.

Let's change this to instead warn the user that the .git/ directory
has been absorbed into the superproject.
The rest of the deinit function can operate as it already does with
new-style submodules.

In one test, we used to require "git submodule deinit" to fail even
with the "--force" option when the submodule's .git/ directory is not
absorbed. Adjust it to expect the operation to pass.

Suggested-by: Atharva Raykar <redacted>
Signed-off-by: Mugdha Pattnaik <redacted>
---
    submodule: absorb git dir instead of dying on deinit
    
    Changes since v6:
    
     * Edited commit message based on suggestions given.
     * Passed correct arguments to absorb gitdir function; path and recurse
       flag
     * Modified behaviour of deinit such that it absorbs the gitdir even
       when not forced.
    
    Changes since v5:
    
     * Fixed accidental submission of old version
    
    Changes since v4:
    
     * Changed test case from "! test -d" to "test_path_is_missing"
    
    Changes since v3:
    
     * Replaced 1 instance of the word "folder" with "directory"
     * Fixed tab spacing
    
    Changes since v2:
    
     * Replaced all instances of the word "folder" with either "directory"
       or "git dir"
    
    Changes since v1:
    
     * Removed extra indent within the if statements
     * Moved absorb_git_dir_into_superproject() call outside the if
       condition checking for --quiet flag
    
    ------------------------------------------------------------------------
    
    Currently, running 'git submodule deinit' on repos where the submodule's
    '.git' is a directory, aborts with a message that is not exactly user
    friendly.
    
    Let's change this to instead warn the user that the .git/ directory has
    been absorbed into the superproject. The rest of the deinit function can
    operate as it already does with new-style submodules.
    
    In one test, we used to require "git submodule deinit" to fail even with
    the "--force" option when the submodule's .git/ directory is not
    absorbed. Adjust it to expect the operation to pass.
    
    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' directory into the
    superproject's '.git/modules/' directory, and since this same directory
    already existed before, it was causing errors. So, before running
    deinit, instead of copying the '.git' directory 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-v7
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1078/mugdhapattnaik/submodule-deinit-absorbgitdirs-v7
Pull-Request: https://github.com/git/git/pull/1078

Range-diff vs v6:

 1:  384a6742388 ! 1:  f37724016f0 submodule: absorb git dir instead of dying on deinit
     @@
       ## Metadata ##
     -Author: mugdha [off-list ref]
     +Author: Mugdha Pattnaik [off-list ref]
      
       ## Commit message ##
          submodule: absorb git dir instead of dying on deinit
      
          Currently, running 'git submodule deinit' on repos where the
          submodule's '.git' is a directory, 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'.
     +    exactly user friendly.
      
     -    This internally calls 'absorb_git_dir_into_superproject()', which
     -    moves the git dir 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.
     +    Let's change this to instead warn the user that the .git/ directory
     +    has been absorbed into the superproject.
     +    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.
     +    In one test, we used to require "git submodule deinit" to fail even
     +    with the "--force" option when the submodule's .git/ directory is not
     +    absorbed. Adjust it to expect the operation to pass.
      
          Suggested-by: Atharva Raykar [off-list ref]
          Signed-off-by: Mugdha Pattnaik [off-list ref]
     @@ builtin/submodule--helper.c: static void deinit_submodule(const char *path, cons
      -			      "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.\nUse --force if you want "
     -+				      "to move its contents to superproject's "
     -+				      "module directory 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);
     ++			absorb_git_dir_into_superproject(path,
     ++							 ABSORB_GITDIR_RECURSE_SUBMODULES);
      +
      +		}
       
     @@ t/t7400-submodule-basic.sh: test_expect_success 'submodule deinit is silent when
       '
       
      -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' '
     ++test_expect_success 'submodule deinit absorbs .git directory if .git is a directory' '
       	git submodule update --init &&
       	(
       		cd init &&
     @@ t/t7400-submodule-basic.sh: test_expect_success 'submodule deinit is silent when
      +		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 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 &&
     ++	git submodule deinit init &&
      +	test_path_is_missing init/.git &&
      +	test -z "$(git config --get-regexp "submodule\.example\.")"
       '


 builtin/submodule--helper.c | 21 +++++++++++----------
 t/t7400-submodule-basic.sh  | 11 +++++------
 2 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index ef2776a9e45..bbab562dec6 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c

@@ -1539,16 +1539,17 @@ 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_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(path,
+							 ABSORB_GITDIR_RECURSE_SUBMODULES);
+
+		}
 
 		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..e7cec2e457a 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh

@@ -1182,18 +1182,17 @@ 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 absorbs .git directory if .git is a directory' '
 	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 init &&
+	test_path_is_missing init/.git &&
+	test -z "$(git config --get-regexp "submodule\.example\.")"
 '
 
 test_expect_success 'submodule with UTF-8 name' '

base-commit: c4203212e360b25a1c69467b5a8437d45a373cac
-- 
gitgitgadget

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?

-- 
An old man doll... just what I always wanted! - Clara

On Fri, Aug 27, 2021 at 1:21 PM Bagas Sanjaya [off-list ref] 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