{"thread":{"id":"56384","subject":"[PATCH] submodule: absorb git dir instead of dying on deinit","startedAt":"2021-08-26T18:33:19Z","lastAt":"2021-11-19T10:56:33Z","messageCount":18,"participants":["Mugdha Pattnaik via GitGitGadget","Bagas Sanjaya","Mugdha Pattnaik","Atharva Raykar","Junio C Hamano","Christian Couder","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"433856","messageId":"pull.1078.git.git.1630002794889.gitgitgadget@gmail.com","threadId":"56384","inReplyTo":null,"subject":"[PATCH] submodule: absorb git dir instead of dying on deinit","fromName":"Mugdha Pattnaik via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-08-26T18:33:14Z","receivedAt":"2021-08-26T18:33:19Z","isPatch":true,"sender":{"key":"mugdhapattnaik@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53373985?v=4"},"body":"From: mugdha <mugdhapattnaik@gmail.com>\n\nCurrently, running 'git submodule deinit' on repos where the\nsubmodule's '.git' is a folder aborts with a message that is not\nexactly user friendly. Let's change this to instead warn the user\nto rerun the command with '--force'.\n\nThis internally calls 'absorb_git_dir_into_superproject()', which\nmoves the '.git' folder into the superproject and replaces it with\na '.git' file. The rest of the deinit function can operate as it\nalready does with new-style submodules.\n\nWe also edit a test case such that it matches the new behaviour of\ndeinit.\n\nSuggested-by: Atharva Raykar <raykar.ath@gmail.com>\nSigned-off-by: Mugdha Pattnaik <mugdhapattnaik@gmail.com>\n---\n    submodule: absorb git dir instead of dying on deinit\n    \n    We also edit a test case such that it matches the new behaviour of\n    deinit.\n    \n    I have changed the 'cp -R ../.git/modules/example .git' to 'mv\n    ../.git/modules/example .git' since, at the time of testing, the test\n    would fail - deinit now would be moving the '.git' folder into the\n    superproject's '.git/modules/' folder, and since this same folder\n    already existed before, it was causing errors. So, before running\n    deinit, instead of copying the '.git' folder into the submodule, if we\n    move it there instead, this functionality can be appropriately tested.\n    \n    Thank you, Mugdha\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1078%2Fmugdhapattnaik%2Fsubmodule-deinit-absorbgitdirs-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1078/mugdhapattnaik/submodule-deinit-absorbgitdirs-v1\nPull-Request: https://github.com/git/git/pull/1078\n\n builtin/submodule--helper.c | 27 +++++++++++++++++----------\n t/t7400-submodule-basic.sh  | 10 +++++-----\n 2 files changed, 22 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex ef2776a9e45..253ddce1c41 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1539,16 +1539,23 @@ static void deinit_submodule(const char *path, const char *prefix,\n \t\tstruct strbuf sb_rm = STRBUF_INIT;\n \t\tconst char *format;\n \n-\t\t/*\n-\t\t * protect submodules containing a .git directory\n-\t\t * NEEDSWORK: instead of dying, automatically call\n-\t\t * absorbgitdirs and (possibly) warn.\n-\t\t */\n-\t\tif (is_directory(sub_git_dir))\n-\t\t\tdie(_(\"Submodule work tree '%s' contains a .git \"\n-\t\t\t      \"directory (use 'rm -rf' if you really want \"\n-\t\t\t      \"to remove it including all of its history)\"),\n-\t\t\t    displaypath);\n+\t\tif (is_directory(sub_git_dir)) {\n+\t\t\tif (!(flags & OPT_FORCE))\n+\t\t\t\t\tdie(_(\"Submodule work tree '%s' contains a \"\n+\t\t\t\t\t\t  \".git directory (use --force if you want \"\n+\t\t\t\t\t\t  \"to move its contents to superproject's \"\n+\t\t\t\t\t\t  \"module folder and convert .git to a file \"\n+\t\t\t\t\t\t  \"and then proceed with deinit)\"),\n+\t\t\t\t\t\tdisplaypath);\n+\n+\t\t\tif (!(flags & OPT_QUIET)) {\n+\t\t\t\t\twarning(_(\"Submodule work tree '%s' contains a .git \"\n+\t\t\t\t\t\t\t  \"directory (this will be replaced with a \"\n+\t\t\t\t\t\t\t  \".git file by using absorbgitdirs\"),\n+\t\t\t\t\t\t\tdisplaypath);\n+\t\t\t\t\tabsorb_git_dir_into_superproject(displaypath, flags);\n+\t\t\t}\n+\t\t}\n \n \t\tif (!(flags & OPT_FORCE)) {\n \t\t\tstruct child_process cp_rm = CHILD_PROCESS_INIT;\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex cb1b8e35dbf..3df71478d06 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -1182,18 +1182,18 @@ test_expect_success 'submodule deinit is silent when used on an uninitialized su\n \trmdir init example2\n '\n \n-test_expect_success 'submodule deinit fails when submodule has a .git directory even when forced' '\n+test_expect_success 'submodule deinit fails when submodule has a .git directory unless forced' '\n \tgit submodule update --init &&\n \t(\n \t\tcd init &&\n \t\trm .git &&\n-\t\tcp -R ../.git/modules/example .git &&\n+\t\tmv ../.git/modules/example .git &&\n \t\tGIT_WORK_TREE=. git config --unset core.worktree\n \t) &&\n \ttest_must_fail git submodule deinit init &&\n-\ttest_must_fail git submodule deinit -f init &&\n-\ttest -d init/.git &&\n-\ttest -n \"$(git config --get-regexp \"submodule\\.example\\.\")\"\n+\tgit submodule deinit -f init &&\n+\t! test -d init/.git &&\n+\ttest -z \"$(git config --get-regexp \"submodule\\.example\\.\")\"\n '\n \n test_expect_success 'submodule with UTF-8 name' '\n\nbase-commit: c4203212e360b25a1c69467b5a8437d45a373cac\n-- \ngitgitgadget\n"},{"id":"433905","messageId":"pull.1078.v2.git.git.1630044219145.gitgitgadget@gmail.com","threadId":"56384","inReplyTo":"pull.1078.git.git.1630002794889.gitgitgadget@gmail.com","subject":"[PATCH v2] submodule: absorb git dir instead of dying on deinit","fromName":"Mugdha Pattnaik via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-08-27T06:03:38Z","receivedAt":"2021-08-27T06:03:43Z","isPatch":true,"sender":{"key":"mugdhapattnaik@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53373985?v=4"},"body":"From: mugdha <mugdhapattnaik@gmail.com>\n\nCurrently, running 'git submodule deinit' on repos where the\nsubmodule's '.git' is a folder aborts with a message that is not\nexactly user friendly. Let's change this to instead warn the user\nto rerun the command with '--force'.\n\nThis internally calls 'absorb_git_dir_into_superproject()', which\nmoves the '.git' folder into the superproject and replaces it with\na '.git' file. The rest of the deinit function can operate as it\nalready does with new-style submodules.\n\nWe also edit a test case such that it matches the new behaviour of\ndeinit.\n\nSuggested-by: Atharva Raykar <raykar.ath@gmail.com>\nSigned-off-by: Mugdha Pattnaik <mugdhapattnaik@gmail.com>\n---\n    submodule: absorb git dir instead of dying on deinit\n    \n    We also edit a test case such that it matches the new behaviour of\n    deinit.\n    \n    I have changed the 'cp -R ../.git/modules/example .git' to 'mv\n    ../.git/modules/example .git' since, at the time of testing, the test\n    would fail - deinit now would be moving the '.git' folder into the\n    superproject's '.git/modules/' folder, and since this same folder\n    already existed before, it was causing errors. So, before running\n    deinit, instead of copying the '.git' folder into the submodule, if we\n    move it there instead, this functionality can be appropriately tested.\n    \n    Changes since v1:\n    \n     * Removed extra indent within the if statements\n     * Moved absorb_git_dir_into_superproject() call outside the if\n       condition checking for --quiet flag\n    \n    Thank you, Mugdha\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1078%2Fmugdhapattnaik%2Fsubmodule-deinit-absorbgitdirs-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1078/mugdhapattnaik/submodule-deinit-absorbgitdirs-v2\nPull-Request: https://github.com/git/git/pull/1078\n\nRange-diff vs v1:\n\n 1:  58814531d17 ! 1:  37c9b598fde submodule: absorb git dir instead of dying on deinit\n     @@ builtin/submodule--helper.c: static void deinit_submodule(const char *path, cons\n      -\t\t\t    displaypath);\n      +\t\tif (is_directory(sub_git_dir)) {\n      +\t\t\tif (!(flags & OPT_FORCE))\n     -+\t\t\t\t\tdie(_(\"Submodule work tree '%s' contains a \"\n     -+\t\t\t\t\t\t  \".git directory (use --force if you want \"\n     -+\t\t\t\t\t\t  \"to move its contents to superproject's \"\n     -+\t\t\t\t\t\t  \"module folder and convert .git to a file \"\n     -+\t\t\t\t\t\t  \"and then proceed with deinit)\"),\n     ++\t\t\t\tdie(_(\"Submodule work tree '%s' contains a \"\n     ++\t\t\t\t\t  \".git directory.\\nUse --force if you want \"\n     ++\t\t\t\t\t  \"to move its contents to superproject's \"\n     ++\t\t\t\t\t  \"module folder and convert .git to a file \"\n     ++\t\t\t\t\t  \"and then proceed with deinit.\"),\n     ++\t\t\t\t\tdisplaypath);\n     ++\n     ++\t\t\tif (!(flags & OPT_QUIET))\n     ++\t\t\t\twarning(_(\"Submodule work tree '%s' contains a .git \"\n     ++\t\t\t\t\t\t  \"directory. This will be replaced with a \"\n     ++\t\t\t\t\t\t  \".git file by using absorbgitdirs.\"),\n      +\t\t\t\t\t\tdisplaypath);\n      +\n     -+\t\t\tif (!(flags & OPT_QUIET)) {\n     -+\t\t\t\t\twarning(_(\"Submodule work tree '%s' contains a .git \"\n     -+\t\t\t\t\t\t\t  \"directory (this will be replaced with a \"\n     -+\t\t\t\t\t\t\t  \".git file by using absorbgitdirs\"),\n     -+\t\t\t\t\t\t\tdisplaypath);\n     -+\t\t\t\t\tabsorb_git_dir_into_superproject(displaypath, flags);\n     -+\t\t\t}\n     ++\t\t\tabsorb_git_dir_into_superproject(displaypath, flags);\n     ++\n      +\t\t}\n       \n       \t\tif (!(flags & OPT_FORCE)) {\n\n\n builtin/submodule--helper.c | 28 ++++++++++++++++++----------\n t/t7400-submodule-basic.sh  | 10 +++++-----\n 2 files changed, 23 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex ef2776a9e45..4730dc141d4 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1539,16 +1539,24 @@ static void deinit_submodule(const char *path, const char *prefix,\n \t\tstruct strbuf sb_rm = STRBUF_INIT;\n \t\tconst char *format;\n \n-\t\t/*\n-\t\t * protect submodules containing a .git directory\n-\t\t * NEEDSWORK: instead of dying, automatically call\n-\t\t * absorbgitdirs and (possibly) warn.\n-\t\t */\n-\t\tif (is_directory(sub_git_dir))\n-\t\t\tdie(_(\"Submodule work tree '%s' contains a .git \"\n-\t\t\t      \"directory (use 'rm -rf' if you really want \"\n-\t\t\t      \"to remove it including all of its history)\"),\n-\t\t\t    displaypath);\n+\t\tif (is_directory(sub_git_dir)) {\n+\t\t\tif (!(flags & OPT_FORCE))\n+\t\t\t\tdie(_(\"Submodule work tree '%s' contains a \"\n+\t\t\t\t\t  \".git directory.\\nUse --force if you want \"\n+\t\t\t\t\t  \"to move its contents to superproject's \"\n+\t\t\t\t\t  \"module folder and convert .git to a file \"\n+\t\t\t\t\t  \"and then proceed with deinit.\"),\n+\t\t\t\t\tdisplaypath);\n+\n+\t\t\tif (!(flags & OPT_QUIET))\n+\t\t\t\twarning(_(\"Submodule work tree '%s' contains a .git \"\n+\t\t\t\t\t\t  \"directory. This will be replaced with a \"\n+\t\t\t\t\t\t  \".git file by using absorbgitdirs.\"),\n+\t\t\t\t\t\tdisplaypath);\n+\n+\t\t\tabsorb_git_dir_into_superproject(displaypath, flags);\n+\n+\t\t}\n \n \t\tif (!(flags & OPT_FORCE)) {\n \t\t\tstruct child_process cp_rm = CHILD_PROCESS_INIT;\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex cb1b8e35dbf..3df71478d06 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -1182,18 +1182,18 @@ test_expect_success 'submodule deinit is silent when used on an uninitialized su\n \trmdir init example2\n '\n \n-test_expect_success 'submodule deinit fails when submodule has a .git directory even when forced' '\n+test_expect_success 'submodule deinit fails when submodule has a .git directory unless forced' '\n \tgit submodule update --init &&\n \t(\n \t\tcd init &&\n \t\trm .git &&\n-\t\tcp -R ../.git/modules/example .git &&\n+\t\tmv ../.git/modules/example .git &&\n \t\tGIT_WORK_TREE=. git config --unset core.worktree\n \t) &&\n \ttest_must_fail git submodule deinit init &&\n-\ttest_must_fail git submodule deinit -f init &&\n-\ttest -d init/.git &&\n-\ttest -n \"$(git config --get-regexp \"submodule\\.example\\.\")\"\n+\tgit submodule deinit -f init &&\n+\t! test -d init/.git &&\n+\ttest -z \"$(git config --get-regexp \"submodule\\.example\\.\")\"\n '\n \n test_expect_success 'submodule with UTF-8 name' '\n\nbase-commit: c4203212e360b25a1c69467b5a8437d45a373cac\n-- \ngitgitgadget\n"},{"id":"433911","messageId":"10db0fb4-56c7-670f-c208-adf5cec4b0fc@gmail.com","threadId":"56384","inReplyTo":"pull.1078.git.git.1630002794889.gitgitgadget@gmail.com","subject":"Re: [PATCH] submodule: absorb git dir instead of dying on deinit","fromName":"Bagas Sanjaya","fromEmail":"bagasdotme@gmail.com","sentAt":"2021-08-27T07:51:04Z","receivedAt":"2021-08-27T07:51:12Z","isPatch":true,"sender":{"key":"bagasdotme@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40219486?v=4"},"body":"On 27/08/21 01.33, Mugdha Pattnaik via GitGitGadget wrote:\n> Currently, running 'git submodule deinit' on repos where the\n> submodule's '.git' is a folder aborts with a message that is not\n> exactly user friendly. Let's change this to instead warn the user\n> to rerun the command with '--force'.\n\nMaybe the case of \"repo inside the repo\", with submodule as repo?\n\n-- \nAn old man doll... just what I always wanted! - Clara\n"},{"id":"433930","messageId":"CAA4dvxj+TqWGW83yD0nvdZOuevwPC=LN0ByHrCTsz3sVX6_5Eg@mail.gmail.com","threadId":"56384","inReplyTo":"10db0fb4-56c7-670f-c208-adf5cec4b0fc@gmail.com","subject":"Re: [PATCH] submodule: absorb git dir instead of dying on deinit","fromName":"Mugdha Pattnaik","fromEmail":"mugdhapattnaik@gmail.com","sentAt":"2021-08-27T13:13:19Z","receivedAt":"2021-08-27T13:13:36Z","isPatch":true,"sender":{"key":"mugdhapattnaik@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53373985?v=4"},"body":"On Fri, Aug 27, 2021 at 1:21 PM Bagas Sanjaya <bagasdotme@gmail.com> wrote:\n> Maybe the case of \"repo inside the repo\", with submodule as repo?\n\nI am not certain what exactly you meant, but this patch deals with \"Old-form\"\nsubmodules. This link (https://git-scm.com/docs/gitsubmodules#_forms)\nstates that old-style submodules, when deinitialized or deleted, have its Git\ndirectory automatically moved to $GIT_DIR/modules/<name>/ of the\nsuperproject. This is what this patch sets to achieve. Earlier, running deinit\non such a submodule would cause it to fail.\n\n--\nMugdha\n"},{"id":"433931","messageId":"m28s0nc8gv.fsf@gmail.com","threadId":"56384","inReplyTo":"pull.1078.v2.git.git.1630044219145.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] submodule: absorb git dir instead of dying on deinit","fromName":"Atharva Raykar","fromEmail":"raykar.ath@gmail.com","sentAt":"2021-08-27T13:20:23Z","receivedAt":"2021-08-27T13:39:56Z","isPatch":true,"sender":{"key":"raykar.ath@gmail.com","avatar":"https://avatars.githubusercontent.com/u/24277692?v=4"},"body":"\n\"Mugdha Pattnaik via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: mugdha <mugdhapattnaik@gmail.com>\n>\n> Currently, running 'git submodule deinit' on repos where the\n> submodule's '.git' is a folder aborts with a message that is not\n> exactly user friendly. Let's change this to instead warn the user\n> to rerun the command with '--force'.\n\nThis makes sense, a lot of commands seem to follow this pattern of\nrequiring a user to '--force' when the behaviour might not be different\nfrom what is normally expected.\n\n> This internally calls 'absorb_git_dir_into_superproject()', which\n> moves the '.git' folder into the superproject and replaces it with\n> a '.git' file. The rest of the deinit function can operate as it\n> already does with new-style submodules.\n\nNice. Just like what the NEEDSWORK comment suggested.\n\n> We also edit a test case such that it matches the new behaviour of\n> deinit.\n>\n> Suggested-by: Atharva Raykar <raykar.ath@gmail.com>\n> Signed-off-by: Mugdha Pattnaik <mugdhapattnaik@gmail.com>\n> ---\n>     submodule: absorb git dir instead of dying on deinit\n>\n>     We also edit a test case such that it matches the new behaviour of\n>     deinit.\n>\n>     I have changed the 'cp -R ../.git/modules/example .git' to 'mv\n>     ../.git/modules/example .git' since, at the time of testing, the test\n>     would fail - deinit now would be moving the '.git' folder into the\n>     superproject's '.git/modules/' folder, and since this same folder\n>     already existed before, it was causing errors. So, before running\n>     deinit, instead of copying the '.git' folder into the submodule, if we\n>     move it there instead, this functionality can be appropriately tested.\n>\n>     Changes since v1:\n>\n>      * Removed extra indent within the if statements\n>      * Moved absorb_git_dir_into_superproject() call outside the if\n>        condition checking for --quiet flag\n>\n>     Thank you, Mugdha\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1078%2Fmugdhapattnaik%2Fsubmodule-deinit-absorbgitdirs-v2\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1078/mugdhapattnaik/submodule-deinit-absorbgitdirs-v2\n> Pull-Request: https://github.com/git/git/pull/1078\n>\n> Range-diff vs v1:\n>\n>  1:  58814531d17 ! 1:  37c9b598fde submodule: absorb git dir instead of dying on deinit\n>      @@ builtin/submodule--helper.c: static void deinit_submodule(const char *path, cons\n>       -\t\t\t    displaypath);\n>       +\t\tif (is_directory(sub_git_dir)) {\n>       +\t\t\tif (!(flags & OPT_FORCE))\n>      -+\t\t\t\t\tdie(_(\"Submodule work tree '%s' contains a \"\n>      -+\t\t\t\t\t\t  \".git directory (use --force if you want \"\n>      -+\t\t\t\t\t\t  \"to move its contents to superproject's \"\n>      -+\t\t\t\t\t\t  \"module folder and convert .git to a file \"\n>      -+\t\t\t\t\t\t  \"and then proceed with deinit)\"),\n>      ++\t\t\t\tdie(_(\"Submodule work tree '%s' contains a \"\n>      ++\t\t\t\t\t  \".git directory.\\nUse --force if you want \"\n>      ++\t\t\t\t\t  \"to move its contents to superproject's \"\n>      ++\t\t\t\t\t  \"module folder and convert .git to a file \"\n>      ++\t\t\t\t\t  \"and then proceed with deinit.\"),\n>      ++\t\t\t\t\tdisplaypath);\n>      ++\n>      ++\t\t\tif (!(flags & OPT_QUIET))\n>      ++\t\t\t\twarning(_(\"Submodule work tree '%s' contains a .git \"\n>      ++\t\t\t\t\t\t  \"directory. This will be replaced with a \"\n>      ++\t\t\t\t\t\t  \".git file by using absorbgitdirs.\"),\n>       +\t\t\t\t\t\tdisplaypath);\n>       +\n>      -+\t\t\tif (!(flags & OPT_QUIET)) {\n>      -+\t\t\t\t\twarning(_(\"Submodule work tree '%s' contains a .git \"\n>      -+\t\t\t\t\t\t\t  \"directory (this will be replaced with a \"\n>      -+\t\t\t\t\t\t\t  \".git file by using absorbgitdirs\"),\n>      -+\t\t\t\t\t\t\tdisplaypath);\n>      -+\t\t\t\t\tabsorb_git_dir_into_superproject(displaypath, flags);\n>      -+\t\t\t}\n>      ++\t\t\tabsorb_git_dir_into_superproject(displaypath, flags);\n>      ++\n>       +\t\t}\n>\n>        \t\tif (!(flags & OPT_FORCE)) {\n>\n>\n>  builtin/submodule--helper.c | 28 ++++++++++++++++++----------\n>  t/t7400-submodule-basic.sh  | 10 +++++-----\n>  2 files changed, 23 insertions(+), 15 deletions(-)\n>\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index ef2776a9e45..4730dc141d4 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -1539,16 +1539,24 @@ static void deinit_submodule(const char *path, const char *prefix,\n>  \t\tstruct strbuf sb_rm = STRBUF_INIT;\n>  \t\tconst char *format;\n>\n> -\t\t/*\n> -\t\t * protect submodules containing a .git directory\n> -\t\t * NEEDSWORK: instead of dying, automatically call\n> -\t\t * absorbgitdirs and (possibly) warn.\n> -\t\t */\n> -\t\tif (is_directory(sub_git_dir))\n> -\t\t\tdie(_(\"Submodule work tree '%s' contains a .git \"\n> -\t\t\t      \"directory (use 'rm -rf' if you really want \"\n> -\t\t\t      \"to remove it including all of its history)\"),\n> -\t\t\t    displaypath);\n> +\t\tif (is_directory(sub_git_dir)) {\n> +\t\t\tif (!(flags & OPT_FORCE))\n> +\t\t\t\tdie(_(\"Submodule work tree '%s' contains a \"\n> +\t\t\t\t\t  \".git directory.\\nUse --force if you want \"\n> +\t\t\t\t\t  \"to move its contents to superproject's \"\n> +\t\t\t\t\t  \"module folder and convert .git to a file \"\n> +\t\t\t\t\t  \"and then proceed with deinit.\"),\n> +\t\t\t\t\tdisplaypath);\n> +\n> +\t\t\tif (!(flags & OPT_QUIET))\n> +\t\t\t\twarning(_(\"Submodule work tree '%s' contains a .git \"\n> +\t\t\t\t\t\t  \"directory. This will be replaced with a \"\n> +\t\t\t\t\t\t  \".git file by using absorbgitdirs.\"),\n> +\t\t\t\t\t\tdisplaypath);\n> +\n> +\t\t\tabsorb_git_dir_into_superproject(displaypath, flags);\n> +\n> +\t\t}\n\nLooks okay to me. The quiet flag suppresses the warning, which is nice.\n\n>  \t\tif (!(flags & OPT_FORCE)) {\n>  \t\t\tstruct child_process cp_rm = CHILD_PROCESS_INIT;\n> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\n> index cb1b8e35dbf..3df71478d06 100755\n> --- a/t/t7400-submodule-basic.sh\n> +++ b/t/t7400-submodule-basic.sh\n> @@ -1182,18 +1182,18 @@ test_expect_success 'submodule deinit is silent when used on an uninitialized su\n>  \trmdir init example2\n>  '\n>\n> -test_expect_success 'submodule deinit fails when submodule has a .git directory even when forced' '\n> +test_expect_success 'submodule deinit fails when submodule has a .git directory unless forced' '\n>  \tgit submodule update --init &&\n>  \t(\n>  \t\tcd init &&\n>  \t\trm .git &&\n> -\t\tcp -R ../.git/modules/example .git &&\n> +\t\tmv ../.git/modules/example .git &&\n\nThe original test case was written with the expectation that the deinit\nwon't happen. But now that it will, I suppose an mv was necessary so\nthat absorbgitdirs can write the gitdir back to the original place\nsuccessfully.\n\n>  \t\tGIT_WORK_TREE=. git config --unset core.worktree\n>  \t) &&\n>  \ttest_must_fail git submodule deinit init &&\n> -\ttest_must_fail git submodule deinit -f init &&\n> -\ttest -d init/.git &&\n> -\ttest -n \"$(git config --get-regexp \"submodule\\.example\\.\")\"\n> +\tgit submodule deinit -f init &&\n> +\t! test -d init/.git &&\n> +\ttest -z \"$(git config --get-regexp \"submodule\\.example\\.\")\"\n>  '\n\nLikewise here. The test case has been flipped to reflect the new\nbehaviour.\n\n>  test_expect_success 'submodule with UTF-8 name' '\n>\n> base-commit: c4203212e360b25a1c69467b5a8437d45a373cac\n\nBTW, I see you linked downthread the documentation[1] which says:\n\n  When [old-form submodules] deinitialized or deleted (see below), the\n  submodule’s Git directory is automatically moved to\n  $GIT_DIR/modules/<name>/ of the superproject.\n\nThis was not what Git was doing before this patch, it used to die() out\ninstead. So I think this actually is a bug fix (although I am not sure\nwhy the test suite also did not agree with the documentation).\n\nEither way, I think this is a welcome change. Thanks for the patch!\n\n[1] https://git-scm.com/docs/gitsubmodules#_forms\n"},{"id":"433939","messageId":"xmqqsfyuajak.fsf@gitster.g","threadId":"56384","inReplyTo":"m28s0nc8gv.fsf@gmail.com","subject":"Re: [PATCH v2] submodule: absorb git dir instead of dying on deinit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-27T17:28:51Z","receivedAt":"2021-08-27T17:28:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Atharva Raykar <raykar.ath@gmail.com> writes:\n\n> \"Mugdha Pattnaik via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>\n>> From: mugdha <mugdhapattnaik@gmail.com>\n>>\n>> Currently, running 'git submodule deinit' on repos where the\n>> submodule's '.git' is a folder aborts with a message that is not\n>> exactly user friendly. Let's change this to instead warn the user\n>> to rerun the command with '--force'.\n>\n> This makes sense, a lot of commands seem to follow this pattern of\n> requiring a user to '--force' when the behaviour might not be different\n> from what is normally expected.\n>\n>> This internally calls 'absorb_git_dir_into_superproject()', which\n>> moves the '.git' folder into the superproject and replaces it with\n>> a '.git' file. The rest of the deinit function can operate as it\n>> already does with new-style submodules.\n>\n> Nice. Just like what the NEEDSWORK comment suggested.\n\nIndeed.  Many use of the word \"folder\", especially when the thing is\ncalled \"git dir\" in the title, irritates me, though.  We use files\nand directories, not folders.\n\n> BTW, I see you linked downthread the documentation[1] which says:\n>\n>   When [old-form submodules] deinitialized or deleted (see below), the\n>   submodule’s Git directory is automatically moved to\n>   $GIT_DIR/modules/<name>/ of the superproject.\n>\n> This was not what Git was doing before this patch, it used to die() out\n> instead. So I think this actually is a bug fix (although I am not sure\n> why the test suite also did not agree with the documentation).\n\nCurious.\n\nI see the description was added by d4803455 (submodules: overhaul\ndocumentation, 2017-06-22), which claimed to \"detangle\" by \"move\"\nvarious existing pieces of documentation into a new file.\n\nI guess we added a wish without marking it as such ;-)\n"},{"id":"433942","messageId":"pull.1078.v3.git.git.1630087821429.gitgitgadget@gmail.com","threadId":"56384","inReplyTo":"pull.1078.v2.git.git.1630044219145.gitgitgadget@gmail.com","subject":"[PATCH v3] submodule: absorb git dir instead of dying on deinit","fromName":"Mugdha Pattnaik via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-08-27T18:10:21Z","receivedAt":"2021-08-27T18:10:26Z","isPatch":true,"sender":{"key":"mugdhapattnaik@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53373985?v=4"},"body":"From: mugdha <mugdhapattnaik@gmail.com>\n\nCurrently, running 'git submodule deinit' on repos where the\nsubmodule's '.git' is a directory, aborts with a message that is not\nexactly user friendly. Let's change this to instead warn the user\nto rerun the command with '--force'.\n\nThis internally calls 'absorb_git_dir_into_superproject()', which\nmoves the git dir into the superproject and replaces it with\na '.git' file. The rest of the deinit function can operate as it\nalready does with new-style submodules.\n\nWe also edit a test case such that it matches the new behaviour of\ndeinit.\n\nSuggested-by: Atharva Raykar <raykar.ath@gmail.com>\nSigned-off-by: Mugdha Pattnaik <mugdhapattnaik@gmail.com>\n---\n    submodule: absorb git dir instead of dying on deinit\n    \n    Changes since v2:\n    \n     * Replaced all instances of the word \"folder\" with either \"directory\"\n       or \"git dir\"\n    \n    Thank you, Mugdha\n    \n    cc: Bagas Sanjaya bagasdotme@gmail.com\n    \n    cc: Atharva Raykar raykar.ath@gmail.com\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1078%2Fmugdhapattnaik%2Fsubmodule-deinit-absorbgitdirs-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1078/mugdhapattnaik/submodule-deinit-absorbgitdirs-v3\nPull-Request: https://github.com/git/git/pull/1078\n\nRange-diff vs v2:\n\n 1:  37c9b598fde ! 1:  1ac65b2458b submodule: absorb git dir instead of dying on deinit\n     @@ Commit message\n          submodule: absorb git dir instead of dying on deinit\n      \n          Currently, running 'git submodule deinit' on repos where the\n     -    submodule's '.git' is a folder aborts with a message that is not\n     +    submodule's '.git' is a directory, aborts with a message that is not\n          exactly user friendly. Let's change this to instead warn the user\n          to rerun the command with '--force'.\n      \n          This internally calls 'absorb_git_dir_into_superproject()', which\n     -    moves the '.git' folder into the superproject and replaces it with\n     +    moves the git dir into the superproject and replaces it with\n          a '.git' file. The rest of the deinit function can operate as it\n          already does with new-style submodules.\n      \n\n\n builtin/submodule--helper.c | 28 ++++++++++++++++++----------\n t/t7400-submodule-basic.sh  | 10 +++++-----\n 2 files changed, 23 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex ef2776a9e45..4730dc141d4 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1539,16 +1539,24 @@ static void deinit_submodule(const char *path, const char *prefix,\n \t\tstruct strbuf sb_rm = STRBUF_INIT;\n \t\tconst char *format;\n \n-\t\t/*\n-\t\t * protect submodules containing a .git directory\n-\t\t * NEEDSWORK: instead of dying, automatically call\n-\t\t * absorbgitdirs and (possibly) warn.\n-\t\t */\n-\t\tif (is_directory(sub_git_dir))\n-\t\t\tdie(_(\"Submodule work tree '%s' contains a .git \"\n-\t\t\t      \"directory (use 'rm -rf' if you really want \"\n-\t\t\t      \"to remove it including all of its history)\"),\n-\t\t\t    displaypath);\n+\t\tif (is_directory(sub_git_dir)) {\n+\t\t\tif (!(flags & OPT_FORCE))\n+\t\t\t\tdie(_(\"Submodule work tree '%s' contains a \"\n+\t\t\t\t\t  \".git directory.\\nUse --force if you want \"\n+\t\t\t\t\t  \"to move its contents to superproject's \"\n+\t\t\t\t\t  \"module folder and convert .git to a file \"\n+\t\t\t\t\t  \"and then proceed with deinit.\"),\n+\t\t\t\t\tdisplaypath);\n+\n+\t\t\tif (!(flags & OPT_QUIET))\n+\t\t\t\twarning(_(\"Submodule work tree '%s' contains a .git \"\n+\t\t\t\t\t\t  \"directory. This will be replaced with a \"\n+\t\t\t\t\t\t  \".git file by using absorbgitdirs.\"),\n+\t\t\t\t\t\tdisplaypath);\n+\n+\t\t\tabsorb_git_dir_into_superproject(displaypath, flags);\n+\n+\t\t}\n \n \t\tif (!(flags & OPT_FORCE)) {\n \t\t\tstruct child_process cp_rm = CHILD_PROCESS_INIT;\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex cb1b8e35dbf..3df71478d06 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -1182,18 +1182,18 @@ test_expect_success 'submodule deinit is silent when used on an uninitialized su\n \trmdir init example2\n '\n \n-test_expect_success 'submodule deinit fails when submodule has a .git directory even when forced' '\n+test_expect_success 'submodule deinit fails when submodule has a .git directory unless forced' '\n \tgit submodule update --init &&\n \t(\n \t\tcd init &&\n \t\trm .git &&\n-\t\tcp -R ../.git/modules/example .git &&\n+\t\tmv ../.git/modules/example .git &&\n \t\tGIT_WORK_TREE=. git config --unset core.worktree\n \t) &&\n \ttest_must_fail git submodule deinit init &&\n-\ttest_must_fail git submodule deinit -f init &&\n-\ttest -d init/.git &&\n-\ttest -n \"$(git config --get-regexp \"submodule\\.example\\.\")\"\n+\tgit submodule deinit -f init &&\n+\t! test -d init/.git &&\n+\ttest -z \"$(git config --get-regexp \"submodule\\.example\\.\")\"\n '\n \n test_expect_success 'submodule with UTF-8 name' '\n\nbase-commit: c4203212e360b25a1c69467b5a8437d45a373cac\n-- \ngitgitgadget\n"},{"id":"433948","messageId":"pull.1078.v4.git.git.1630090303445.gitgitgadget@gmail.com","threadId":"56384","inReplyTo":"pull.1078.v3.git.git.1630087821429.gitgitgadget@gmail.com","subject":"[PATCH v4] submodule: absorb git dir instead of dying on deinit","fromName":"Mugdha Pattnaik via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-08-27T18:51:43Z","receivedAt":"2021-08-27T18:51:48Z","isPatch":true,"sender":{"key":"mugdhapattnaik@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53373985?v=4"},"body":"From: mugdha <mugdhapattnaik@gmail.com>\n\nCurrently, running 'git submodule deinit' on repos where the\nsubmodule's '.git' is a directory, aborts with a message that is not\nexactly user friendly. Let's change this to instead warn the user\nto rerun the command with '--force'.\n\nThis internally calls 'absorb_git_dir_into_superproject()', which\nmoves the git dir into the superproject and replaces it with\na '.git' file. The rest of the deinit function can operate as it\nalready does with new-style submodules.\n\nWe also edit a test case such that it matches the new behaviour of\ndeinit.\n\nSuggested-by: Atharva Raykar <raykar.ath@gmail.com>\nSigned-off-by: Mugdha Pattnaik <mugdhapattnaik@gmail.com>\n---\n    submodule: absorb git dir instead of dying on deinit\n    \n    Changes since v3:\n    \n     * Replaced 1 instance of the word \"folder\" with \"directory\"\n     * Fixed tab spacing\n    \n    Thank you, Mugdha\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1078%2Fmugdhapattnaik%2Fsubmodule-deinit-absorbgitdirs-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1078/mugdhapattnaik/submodule-deinit-absorbgitdirs-v4\nPull-Request: https://github.com/git/git/pull/1078\n\nRange-diff vs v3:\n\n 1:  1ac65b2458b ! 1:  7460fc0e12a submodule: absorb git dir instead of dying on deinit\n     @@ builtin/submodule--helper.c: static void deinit_submodule(const char *path, cons\n      +\t\tif (is_directory(sub_git_dir)) {\n      +\t\t\tif (!(flags & OPT_FORCE))\n      +\t\t\t\tdie(_(\"Submodule work tree '%s' contains a \"\n     -+\t\t\t\t\t  \".git directory.\\nUse --force if you want \"\n     -+\t\t\t\t\t  \"to move its contents to superproject's \"\n     -+\t\t\t\t\t  \"module folder and convert .git to a file \"\n     -+\t\t\t\t\t  \"and then proceed with deinit.\"),\n     -+\t\t\t\t\tdisplaypath);\n     ++\t\t\t\t      \".git directory.\\nUse --force if you want \"\n     ++\t\t\t\t      \"to move its contents to superproject's \"\n     ++\t\t\t\t      \"module directory and convert .git to a file \"\n     ++\t\t\t\t      \"and then proceed with deinit.\"),\n     ++\t\t\t\t    displaypath);\n      +\n      +\t\t\tif (!(flags & OPT_QUIET))\n      +\t\t\t\twarning(_(\"Submodule work tree '%s' contains a .git \"\n     -+\t\t\t\t\t\t  \"directory. This will be replaced with a \"\n     -+\t\t\t\t\t\t  \".git file by using absorbgitdirs.\"),\n     -+\t\t\t\t\t\tdisplaypath);\n     ++\t\t\t\t\t  \"directory. This will be replaced with a \"\n     ++\t\t\t\t\t  \".git file by using absorbgitdirs.\"),\n     ++\t\t\t\t\tdisplaypath);\n      +\n      +\t\t\tabsorb_git_dir_into_superproject(displaypath, flags);\n      +\n\n\n builtin/submodule--helper.c | 28 ++++++++++++++++++----------\n t/t7400-submodule-basic.sh  | 10 +++++-----\n 2 files changed, 23 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex ef2776a9e45..040b26f149d 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1539,16 +1539,24 @@ static void deinit_submodule(const char *path, const char *prefix,\n \t\tstruct strbuf sb_rm = STRBUF_INIT;\n \t\tconst char *format;\n \n-\t\t/*\n-\t\t * protect submodules containing a .git directory\n-\t\t * NEEDSWORK: instead of dying, automatically call\n-\t\t * absorbgitdirs and (possibly) warn.\n-\t\t */\n-\t\tif (is_directory(sub_git_dir))\n-\t\t\tdie(_(\"Submodule work tree '%s' contains a .git \"\n-\t\t\t      \"directory (use 'rm -rf' if you really want \"\n-\t\t\t      \"to remove it including all of its history)\"),\n-\t\t\t    displaypath);\n+\t\tif (is_directory(sub_git_dir)) {\n+\t\t\tif (!(flags & OPT_FORCE))\n+\t\t\t\tdie(_(\"Submodule work tree '%s' contains a \"\n+\t\t\t\t      \".git directory.\\nUse --force if you want \"\n+\t\t\t\t      \"to move its contents to superproject's \"\n+\t\t\t\t      \"module directory and convert .git to a file \"\n+\t\t\t\t      \"and then proceed with deinit.\"),\n+\t\t\t\t    displaypath);\n+\n+\t\t\tif (!(flags & OPT_QUIET))\n+\t\t\t\twarning(_(\"Submodule work tree '%s' contains a .git \"\n+\t\t\t\t\t  \"directory. This will be replaced with a \"\n+\t\t\t\t\t  \".git file by using absorbgitdirs.\"),\n+\t\t\t\t\tdisplaypath);\n+\n+\t\t\tabsorb_git_dir_into_superproject(displaypath, flags);\n+\n+\t\t}\n \n \t\tif (!(flags & OPT_FORCE)) {\n \t\t\tstruct child_process cp_rm = CHILD_PROCESS_INIT;\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex cb1b8e35dbf..3df71478d06 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -1182,18 +1182,18 @@ test_expect_success 'submodule deinit is silent when used on an uninitialized su\n \trmdir init example2\n '\n \n-test_expect_success 'submodule deinit fails when submodule has a .git directory even when forced' '\n+test_expect_success 'submodule deinit fails when submodule has a .git directory unless forced' '\n \tgit submodule update --init &&\n \t(\n \t\tcd init &&\n \t\trm .git &&\n-\t\tcp -R ../.git/modules/example .git &&\n+\t\tmv ../.git/modules/example .git &&\n \t\tGIT_WORK_TREE=. git config --unset core.worktree\n \t) &&\n \ttest_must_fail git submodule deinit init &&\n-\ttest_must_fail git submodule deinit -f init &&\n-\ttest -d init/.git &&\n-\ttest -n \"$(git config --get-regexp \"submodule\\.example\\.\")\"\n+\tgit submodule deinit -f init &&\n+\t! test -d init/.git &&\n+\ttest -z \"$(git config --get-regexp \"submodule\\.example\\.\")\"\n '\n \n test_expect_success 'submodule with UTF-8 name' '\n\nbase-commit: c4203212e360b25a1c69467b5a8437d45a373cac\n-- \ngitgitgadget\n"},{"id":"437257","messageId":"CAP8UFD3da8MPgN5FfqKOX+1noGXtX1g=oU8+PaHT_Bcxa=R0RA@mail.gmail.com","threadId":"56384","inReplyTo":"pull.1078.v4.git.git.1630090303445.gitgitgadget@gmail.com","subject":"Re: [PATCH v4] submodule: absorb git dir instead of dying on deinit","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2021-09-28T07:30:32Z","receivedAt":"2021-09-28T07:30:46Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Aug 27, 2021 at 8:53 PM Mugdha Pattnaik via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n>\n> From: mugdha <mugdhapattnaik@gmail.com>\n>\n> Currently, running 'git submodule deinit' on repos where the\n> submodule's '.git' is a directory, aborts with a message that is not\n> exactly user friendly. Let's change this to instead warn the user\n> to rerun the command with '--force'.\n\nIt seems to me that this patch fell into the cracks. Or did I miss something?\n\n> This internally calls 'absorb_git_dir_into_superproject()', which\n> moves the git dir into the superproject and replaces it with\n> a '.git' file. The rest of the deinit function can operate as it\n> already does with new-style submodules.\n>\n> We also edit a test case such that it matches the new behaviour of\n> deinit.\n>\n> Suggested-by: Atharva Raykar <raykar.ath@gmail.com>\n> Signed-off-by: Mugdha Pattnaik <mugdhapattnaik@gmail.com>\n"},{"id":"437274","messageId":"87sfxpvxcs.fsf@evledraar.gmail.com","threadId":"56384","inReplyTo":"pull.1078.v4.git.git.1630090303445.gitgitgadget@gmail.com","subject":"Re: [PATCH v4] submodule: absorb git dir instead of dying on deinit","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-28T09:53:45Z","receivedAt":"2021-09-28T09:54:55Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Aug 27 2021, Mugdha Pattnaik via GitGitGadget wrote:\n\n> From: mugdha <mugdhapattnaik@gmail.com>\n>\n> Currently, running 'git submodule deinit' on repos where the\n> submodule's '.git' is a directory, aborts with a message that is not\n> exactly user friendly. Let's change this to instead warn the user\n> to rerun the command with '--force'.\n>\n> This internally calls 'absorb_git_dir_into_superproject()', which\n> moves the git dir into the superproject and replaces it with\n> a '.git' file. The rest of the deinit function can operate as it\n> already does with new-style submodules.\n>\n> We also edit a test case such that it matches the new behaviour of\n> deinit.\n>\n> Suggested-by: Atharva Raykar <raykar.ath@gmail.com>\n> Signed-off-by: Mugdha Pattnaik <mugdhapattnaik@gmail.com>\n> ---\n>     submodule: absorb git dir instead of dying on deinit\n>     \n>     Changes since v3:\n>     \n>      * Replaced 1 instance of the word \"folder\" with \"directory\"\n>      * Fixed tab spacing\n>     \n>     Thank you, Mugdha\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1078%2Fmugdhapattnaik%2Fsubmodule-deinit-absorbgitdirs-v4\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1078/mugdhapattnaik/submodule-deinit-absorbgitdirs-v4\n> Pull-Request: https://github.com/git/git/pull/1078\n>\n> Range-diff vs v3:\n>\n>  1:  1ac65b2458b ! 1:  7460fc0e12a submodule: absorb git dir instead of dying on deinit\n>      @@ builtin/submodule--helper.c: static void deinit_submodule(const char *path, cons\n>       +\t\tif (is_directory(sub_git_dir)) {\n>       +\t\t\tif (!(flags & OPT_FORCE))\n>       +\t\t\t\tdie(_(\"Submodule work tree '%s' contains a \"\n>      -+\t\t\t\t\t  \".git directory.\\nUse --force if you want \"\n>      -+\t\t\t\t\t  \"to move its contents to superproject's \"\n>      -+\t\t\t\t\t  \"module folder and convert .git to a file \"\n>      -+\t\t\t\t\t  \"and then proceed with deinit.\"),\n>      -+\t\t\t\t\tdisplaypath);\n>      ++\t\t\t\t      \".git directory.\\nUse --force if you want \"\n>      ++\t\t\t\t      \"to move its contents to superproject's \"\n>      ++\t\t\t\t      \"module directory and convert .git to a file \"\n>      ++\t\t\t\t      \"and then proceed with deinit.\"),\n>      ++\t\t\t\t    displaypath);\n>       +\n>       +\t\t\tif (!(flags & OPT_QUIET))\n>       +\t\t\t\twarning(_(\"Submodule work tree '%s' contains a .git \"\n>      -+\t\t\t\t\t\t  \"directory. This will be replaced with a \"\n>      -+\t\t\t\t\t\t  \".git file by using absorbgitdirs.\"),\n>      -+\t\t\t\t\t\tdisplaypath);\n>      ++\t\t\t\t\t  \"directory. This will be replaced with a \"\n>      ++\t\t\t\t\t  \".git file by using absorbgitdirs.\"),\n>      ++\t\t\t\t\tdisplaypath);\n>       +\n>       +\t\t\tabsorb_git_dir_into_superproject(displaypath, flags);\n>       +\n>\n>\n>  builtin/submodule--helper.c | 28 ++++++++++++++++++----------\n>  t/t7400-submodule-basic.sh  | 10 +++++-----\n>  2 files changed, 23 insertions(+), 15 deletions(-)\n>\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index ef2776a9e45..040b26f149d 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -1539,16 +1539,24 @@ static void deinit_submodule(const char *path, const char *prefix,\n>  \t\tstruct strbuf sb_rm = STRBUF_INIT;\n>  \t\tconst char *format;\n>  \n> -\t\t/*\n> -\t\t * protect submodules containing a .git directory\n> -\t\t * NEEDSWORK: instead of dying, automatically call\n> -\t\t * absorbgitdirs and (possibly) warn.\n> -\t\t */\n> -\t\tif (is_directory(sub_git_dir))\n> -\t\t\tdie(_(\"Submodule work tree '%s' contains a .git \"\n> -\t\t\t      \"directory (use 'rm -rf' if you really want \"\n> -\t\t\t      \"to remove it including all of its history)\"),\n> -\t\t\t    displaypath);\n> +\t\tif (is_directory(sub_git_dir)) {\n> +\t\t\tif (!(flags & OPT_FORCE))\n> +\t\t\t\tdie(_(\"Submodule work tree '%s' contains a \"\n> +\t\t\t\t      \".git directory.\\nUse --force if you want \"\n> +\t\t\t\t      \"to move its contents to superproject's \"\n> +\t\t\t\t      \"module directory and convert .git to a file \"\n> +\t\t\t\t      \"and then proceed with deinit.\"),\n> +\t\t\t\t    displaypath);\n> +\n> +\t\t\tif (!(flags & OPT_QUIET))\n> +\t\t\t\twarning(_(\"Submodule work tree '%s' contains a .git \"\n> +\t\t\t\t\t  \"directory. This will be replaced with a \"\n> +\t\t\t\t\t  \".git file by using absorbgitdirs.\"),\n> +\t\t\t\t\tdisplaypath);\n> +\n> +\t\t\tabsorb_git_dir_into_superproject(displaypath, flags);\n> +\n> +\t\t}\n>  \n>  \t\tif (!(flags & OPT_FORCE)) {\n>  \t\t\tstruct child_process cp_rm = CHILD_PROCESS_INIT;\n> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\n> index cb1b8e35dbf..3df71478d06 100755\n> --- a/t/t7400-submodule-basic.sh\n> +++ b/t/t7400-submodule-basic.sh\n> @@ -1182,18 +1182,18 @@ test_expect_success 'submodule deinit is silent when used on an uninitialized su\n>  \trmdir init example2\n>  '\n>  \n> -test_expect_success 'submodule deinit fails when submodule has a .git directory even when forced' '\n> +test_expect_success 'submodule deinit fails when submodule has a .git directory unless forced' '\n>  \tgit submodule update --init &&\n>  \t(\n>  \t\tcd init &&\n>  \t\trm .git &&\n> -\t\tcp -R ../.git/modules/example .git &&\n> +\t\tmv ../.git/modules/example .git &&\n>  \t\tGIT_WORK_TREE=. git config --unset core.worktree\n>  \t) &&\n>  \ttest_must_fail git submodule deinit init &&\n> -\ttest_must_fail git submodule deinit -f init &&\n> -\ttest -d init/.git &&\n> -\ttest -n \"$(git config --get-regexp \"submodule\\.example\\.\")\"\n> +\tgit submodule deinit -f init &&\n> +\t! test -d init/.git &&\n> +\ttest -z \"$(git config --get-regexp \"submodule\\.example\\.\")\"\n\nI see we don't have a \"test_dir_is_missing\" for the pre-image, but\nshouldn't the post-image \"! test -d\" just be a test_path_is_missing?\nI.e. should this pass if .git is a file? Probably not...\n"},{"id":"438061","messageId":"CAA4dvxgXNPDRB6okD8bdS_Tu=EEgr5pphxK1u4QCJjJYb=TaMw@mail.gmail.com","threadId":"56384","inReplyTo":"87sfxpvxcs.fsf@evledraar.gmail.com","subject":"Re: [PATCH v4] submodule: absorb git dir instead of dying on deinit","fromName":"Mugdha Pattnaik","fromEmail":"mugdhapattnaik@gmail.com","sentAt":"2021-10-06T11:45:26Z","receivedAt":"2021-10-06T11:45:46Z","isPatch":true,"sender":{"key":"mugdhapattnaik@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53373985?v=4"},"body":"On Tue, Sep 28, 2021 Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:\n> shouldn't the post-image \"! test -d\" just be a test_path_is_missing?\n> I.e. should this pass if .git is a file? Probably not...\n\nI agree, this test should not pass if .git is a file. I'll change it to\n\"test_path_is_missing\" and send an update to the patch.\n"},{"id":"438062","messageId":"pull.1078.v5.git.git.1633521772731.gitgitgadget@gmail.com","threadId":"56384","inReplyTo":"pull.1078.v4.git.git.1630090303445.gitgitgadget@gmail.com","subject":"[PATCH v5] submodule: absorb git dir instead of dying on deinit","fromName":"Mugdha Pattnaik via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-10-06T12:02:52Z","receivedAt":"2021-10-06T12:02:57Z","isPatch":true,"sender":{"key":"mugdhapattnaik@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53373985?v=4"},"body":"From: mugdha <mugdhapattnaik@gmail.com>\n\nCurrently, running 'git submodule deinit' on repos where the\nsubmodule's '.git' is a directory, aborts with a message that is not\nexactly user friendly. Let's change this to instead warn the user\nto rerun the command with '--force'.\n\nThis internally calls 'absorb_git_dir_into_superproject()', which\nmoves the git dir into the superproject and replaces it with\na '.git' file. The rest of the deinit function can operate as it\nalready does with new-style submodules.\n\nWe also edit a test case such that it matches the new behaviour of\ndeinit.\n\nSuggested-by: Atharva Raykar <raykar.ath@gmail.com>\nSigned-off-by: Mugdha Pattnaik <mugdhapattnaik@gmail.com>\n---\n    submodule: absorb git dir instead of dying on deinit\n    \n    Changes since v4:\n    \n     * Changed test case from \"! test -d\" to \"test_path_is_missing\"\n    \n    Thanks\n    Mugdha\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1078%2Fmugdhapattnaik%2Fsubmodule-deinit-absorbgitdirs-v5\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1078/mugdhapattnaik/submodule-deinit-absorbgitdirs-v5\nPull-Request: https://github.com/git/git/pull/1078\n\nRange-diff vs v4:\n\n 1:  7460fc0e12a = 1:  c39cd681e71 submodule: absorb git dir instead of dying on deinit\n\n\n builtin/submodule--helper.c | 28 ++++++++++++++++++----------\n t/t7400-submodule-basic.sh  | 10 +++++-----\n 2 files changed, 23 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex ef2776a9e45..040b26f149d 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1539,16 +1539,24 @@ static void deinit_submodule(const char *path, const char *prefix,\n \t\tstruct strbuf sb_rm = STRBUF_INIT;\n \t\tconst char *format;\n \n-\t\t/*\n-\t\t * protect submodules containing a .git directory\n-\t\t * NEEDSWORK: instead of dying, automatically call\n-\t\t * absorbgitdirs and (possibly) warn.\n-\t\t */\n-\t\tif (is_directory(sub_git_dir))\n-\t\t\tdie(_(\"Submodule work tree '%s' contains a .git \"\n-\t\t\t      \"directory (use 'rm -rf' if you really want \"\n-\t\t\t      \"to remove it including all of its history)\"),\n-\t\t\t    displaypath);\n+\t\tif (is_directory(sub_git_dir)) {\n+\t\t\tif (!(flags & OPT_FORCE))\n+\t\t\t\tdie(_(\"Submodule work tree '%s' contains a \"\n+\t\t\t\t      \".git directory.\\nUse --force if you want \"\n+\t\t\t\t      \"to move its contents to superproject's \"\n+\t\t\t\t      \"module directory and convert .git to a file \"\n+\t\t\t\t      \"and then proceed with deinit.\"),\n+\t\t\t\t    displaypath);\n+\n+\t\t\tif (!(flags & OPT_QUIET))\n+\t\t\t\twarning(_(\"Submodule work tree '%s' contains a .git \"\n+\t\t\t\t\t  \"directory. This will be replaced with a \"\n+\t\t\t\t\t  \".git file by using absorbgitdirs.\"),\n+\t\t\t\t\tdisplaypath);\n+\n+\t\t\tabsorb_git_dir_into_superproject(displaypath, flags);\n+\n+\t\t}\n \n \t\tif (!(flags & OPT_FORCE)) {\n \t\t\tstruct child_process cp_rm = CHILD_PROCESS_INIT;\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex cb1b8e35dbf..3df71478d06 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -1182,18 +1182,18 @@ test_expect_success 'submodule deinit is silent when used on an uninitialized su\n \trmdir init example2\n '\n \n-test_expect_success 'submodule deinit fails when submodule has a .git directory even when forced' '\n+test_expect_success 'submodule deinit fails when submodule has a .git directory unless forced' '\n \tgit submodule update --init &&\n \t(\n \t\tcd init &&\n \t\trm .git &&\n-\t\tcp -R ../.git/modules/example .git &&\n+\t\tmv ../.git/modules/example .git &&\n \t\tGIT_WORK_TREE=. git config --unset core.worktree\n \t) &&\n \ttest_must_fail git submodule deinit init &&\n-\ttest_must_fail git submodule deinit -f init &&\n-\ttest -d init/.git &&\n-\ttest -n \"$(git config --get-regexp \"submodule\\.example\\.\")\"\n+\tgit submodule deinit -f init &&\n+\t! test -d init/.git &&\n+\ttest -z \"$(git config --get-regexp \"submodule\\.example\\.\")\"\n '\n \n test_expect_success 'submodule with UTF-8 name' '\n\nbase-commit: c4203212e360b25a1c69467b5a8437d45a373cac\n-- \ngitgitgadget\n"},{"id":"438063","messageId":"pull.1078.v6.git.git.1633523057369.gitgitgadget@gmail.com","threadId":"56384","inReplyTo":"pull.1078.v5.git.git.1633521772731.gitgitgadget@gmail.com","subject":"[PATCH v6] submodule: absorb git dir instead of dying on deinit","fromName":"Mugdha Pattnaik via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-10-06T12:24:17Z","receivedAt":"2021-10-06T12:24:26Z","isPatch":true,"sender":{"key":"mugdhapattnaik@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53373985?v=4"},"body":"From: mugdha <mugdhapattnaik@gmail.com>\n\nCurrently, running 'git submodule deinit' on repos where the\nsubmodule's '.git' is a directory, aborts with a message that is not\nexactly user friendly. Let's change this to instead warn the user\nto rerun the command with '--force'.\n\nThis internally calls 'absorb_git_dir_into_superproject()', which\nmoves the git dir into the superproject and replaces it with\na '.git' file. The rest of the deinit function can operate as it\nalready does with new-style submodules.\n\nWe also edit a test case such that it matches the new behaviour of\ndeinit.\n\nSuggested-by: Atharva Raykar <raykar.ath@gmail.com>\nSigned-off-by: Mugdha Pattnaik <mugdhapattnaik@gmail.com>\n---\n    submodule: absorb git dir instead of dying on deinit\n    \n    Changes since v5:\n    \n     * Fixed accidental submission of old version\n    \n    Changes since v4:\n    \n     * Changed test case from \"! test -d\" to \"test_path_is_missing\"\n    \n    Thank you,\n    Mugdha\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1078%2Fmugdhapattnaik%2Fsubmodule-deinit-absorbgitdirs-v6\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1078/mugdhapattnaik/submodule-deinit-absorbgitdirs-v6\nPull-Request: https://github.com/git/git/pull/1078\n\nRange-diff vs v5:\n\n 1:  c39cd681e71 ! 1:  384a6742388 submodule: absorb git dir instead of dying on deinit\n     @@ t/t7400-submodule-basic.sh: test_expect_success 'submodule deinit is silent when\n      -\ttest -d init/.git &&\n      -\ttest -n \"$(git config --get-regexp \"submodule\\.example\\.\")\"\n      +\tgit submodule deinit -f init &&\n     -+\t! test -d init/.git &&\n     ++\ttest_path_is_missing init/.git &&\n      +\ttest -z \"$(git config --get-regexp \"submodule\\.example\\.\")\"\n       '\n       \n\n\n builtin/submodule--helper.c | 28 ++++++++++++++++++----------\n t/t7400-submodule-basic.sh  | 10 +++++-----\n 2 files changed, 23 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex ef2776a9e45..040b26f149d 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1539,16 +1539,24 @@ static void deinit_submodule(const char *path, const char *prefix,\n \t\tstruct strbuf sb_rm = STRBUF_INIT;\n \t\tconst char *format;\n \n-\t\t/*\n-\t\t * protect submodules containing a .git directory\n-\t\t * NEEDSWORK: instead of dying, automatically call\n-\t\t * absorbgitdirs and (possibly) warn.\n-\t\t */\n-\t\tif (is_directory(sub_git_dir))\n-\t\t\tdie(_(\"Submodule work tree '%s' contains a .git \"\n-\t\t\t      \"directory (use 'rm -rf' if you really want \"\n-\t\t\t      \"to remove it including all of its history)\"),\n-\t\t\t    displaypath);\n+\t\tif (is_directory(sub_git_dir)) {\n+\t\t\tif (!(flags & OPT_FORCE))\n+\t\t\t\tdie(_(\"Submodule work tree '%s' contains a \"\n+\t\t\t\t      \".git directory.\\nUse --force if you want \"\n+\t\t\t\t      \"to move its contents to superproject's \"\n+\t\t\t\t      \"module directory and convert .git to a file \"\n+\t\t\t\t      \"and then proceed with deinit.\"),\n+\t\t\t\t    displaypath);\n+\n+\t\t\tif (!(flags & OPT_QUIET))\n+\t\t\t\twarning(_(\"Submodule work tree '%s' contains a .git \"\n+\t\t\t\t\t  \"directory. This will be replaced with a \"\n+\t\t\t\t\t  \".git file by using absorbgitdirs.\"),\n+\t\t\t\t\tdisplaypath);\n+\n+\t\t\tabsorb_git_dir_into_superproject(displaypath, flags);\n+\n+\t\t}\n \n \t\tif (!(flags & OPT_FORCE)) {\n \t\t\tstruct child_process cp_rm = CHILD_PROCESS_INIT;\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex cb1b8e35dbf..f35479ea634 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -1182,18 +1182,18 @@ test_expect_success 'submodule deinit is silent when used on an uninitialized su\n \trmdir init example2\n '\n \n-test_expect_success 'submodule deinit fails when submodule has a .git directory even when forced' '\n+test_expect_success 'submodule deinit fails when submodule has a .git directory unless forced' '\n \tgit submodule update --init &&\n \t(\n \t\tcd init &&\n \t\trm .git &&\n-\t\tcp -R ../.git/modules/example .git &&\n+\t\tmv ../.git/modules/example .git &&\n \t\tGIT_WORK_TREE=. git config --unset core.worktree\n \t) &&\n \ttest_must_fail git submodule deinit init &&\n-\ttest_must_fail git submodule deinit -f init &&\n-\ttest -d init/.git &&\n-\ttest -n \"$(git config --get-regexp \"submodule\\.example\\.\")\"\n+\tgit submodule deinit -f init &&\n+\ttest_path_is_missing init/.git &&\n+\ttest -z \"$(git config --get-regexp \"submodule\\.example\\.\")\"\n '\n \n test_expect_success 'submodule with UTF-8 name' '\n\nbase-commit: c4203212e360b25a1c69467b5a8437d45a373cac\n-- \ngitgitgadget\n"},{"id":"438198","messageId":"xmqqwnmopqqk.fsf@gitster.g","threadId":"56384","inReplyTo":"pull.1078.v6.git.git.1633523057369.gitgitgadget@gmail.com","subject":"Re: [PATCH v6] submodule: absorb git dir instead of dying on deinit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-07T19:41:23Z","receivedAt":"2021-10-07T19:41:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Mugdha Pattnaik via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: mugdha <mugdhapattnaik@gmail.com>\n\nThis line is called \"in-body header\" that is used to override the\nauthor name that is automatically obtained from the e-mail's \"From:\"\nheader (which is set to \"Mugdha Pattnaik via GitGitGadget\" by GGG,\nwhich is obviously not your name, hence we use an in-body header to\noverride it).  It should match what you use to sign off your patches,\nthe one we see below.\n\nI'll hand-edit so that \"git show\" will say that the author is\n\"Mugdha Pattnaik\", not \"mugdha\", while applying, but please make\nsure your further submissions will not have this problem.\n\n> Currently, running 'git submodule deinit' on repos where the\n> submodule's '.git' is a directory, aborts with a message that is not\n> exactly user friendly. Let's change this to instead warn the user\n> to rerun the command with '--force'.\n\nOK.  That sounds like an improvement, albeit possibly an overly\ncautious one, as a casual \"deinit\" user will get an error as before\nwithout \"--force\", which may or may not be a good thing.  Requiring\n\"--force\" means it is safer by default by not changing the on-disk\ndata.  But requiring \"--force\" also means we end up training users\nto say \"--force\" when it shouldn't have to be.\n\nA plausible alternative is to always absorb but with a warning \"we\nabsorbed it for you\", without requiring \"--force\".  If we didn't\nhave \"git submodule deinit\" command now and were designing it from\nscratch, would we design it to fail because the submodule's git\ndirectory is not absorbed?  I doubt it, as I do not think of a good\nreason to do so offhand.\n\nDoes \"git submodule\" currently reject a \"deinit\" request due to some\n_other_ conditions or safety concerns and require the \"--force\"\noption to continue?  Requiring the \"--force\" option to resolve \".git\nis a directory, and the user wants to make it absorbed\" means that\nthe user will be _forced_ to bypass these _other_ safety valves only\nto save the submodule repository from destruction when running\n\"deinit\", which may not be a good trade-off between the safety\nrequirements of these _other_ conditions, if exists, and the one we\nare dealing with.\n\n> This internally calls 'absorb_git_dir_into_superproject()', which\n> moves the git dir into the superproject and replaces it with\n> a '.git' file. The rest of the deinit function can operate as it\n> already does with new-style submodules.\n\nThis is not wrong per-se, but such an implementation detail is\nsomething best left for the patch.  \n\n> We also edit a test case such that it matches the new behaviour of\n> deinit.\n\n\"match the new behaviour\" in what way?\n\n    In one test, we used to require \"git submodule deinit\" to fail\n    even with the \"--force\" option when the submodule's .git/\n    directory is not absorbed.  Adjust it to expect the operation to\n    pass.\n\nwould be a description at the right level of detail, I think.\n\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index ef2776a9e45..040b26f149d 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -1539,16 +1539,24 @@ static void deinit_submodule(const char *path, const char *prefix,\n>  \t\tstruct strbuf sb_rm = STRBUF_INIT;\n>  \t\tconst char *format;\n>  \n> -\t\t/*\n> -\t\t * protect submodules containing a .git directory\n> -\t\t * NEEDSWORK: instead of dying, automatically call\n> -\t\t * absorbgitdirs and (possibly) warn.\n> -\t\t */\n> -\t\tif (is_directory(sub_git_dir))\n> -\t\t\tdie(_(\"Submodule work tree '%s' contains a .git \"\n> -\t\t\t      \"directory (use 'rm -rf' if you really want \"\n> -\t\t\t      \"to remove it including all of its history)\"),\n> -\t\t\t    displaypath);\n> +\t\tif (is_directory(sub_git_dir)) {\n> +\t\t\tif (!(flags & OPT_FORCE))\n> +\t\t\t\tdie(_(\"Submodule work tree '%s' contains a \"\n> +\t\t\t\t      \".git directory.\\nUse --force if you want \"\n> +\t\t\t\t      \"to move its contents to superproject's \"\n> +\t\t\t\t      \"module directory and convert .git to a file \"\n> +\t\t\t\t      \"and then proceed with deinit.\"),\n> +\t\t\t\t    displaypath);\n> +\n> +\t\t\tif (!(flags & OPT_QUIET))\n> +\t\t\t\twarning(_(\"Submodule work tree '%s' contains a .git \"\n> +\t\t\t\t\t  \"directory. This will be replaced with a \"\n> +\t\t\t\t\t  \".git file by using absorbgitdirs.\"),\n> +\t\t\t\t\tdisplaypath);\n> +\n> +\t\t\tabsorb_git_dir_into_superproject(displaypath, flags);\n\nShouldn't the first argument to this call be \"path\" not\n\"displaypath\"?  The paths in messages may want to have the path from\nthe top to the submodule location prefixed for human consumption,\nbut the called function only cares about the path to the submodule\nfrom the current directory, no?\n\nThe second parameter of this call seems totally bogus to me.  What\nis the vocabulary of bits the called function takes?  Is that from\nthe same set the flags this function takes?  Does the called\nfunction even understand OPT_QUIET, or does the bitpattern that\nhappens to correspond to OPT_QUIET have a totally different meaning\nto the called function and we will trigger a behaviour that this\ncaller does not expect at all?\n\nThanks.\n"},{"id":"441338","messageId":"CAA4dvxgNJ8eyuc5B6_GnSLHMWjdbJN5k_rTCLjWndEyjv_vOnw@mail.gmail.com","threadId":"56384","inReplyTo":"xmqqwnmopqqk.fsf@gitster.g","subject":"Re: [PATCH v6] submodule: absorb git dir instead of dying on deinit","fromName":"Mugdha Pattnaik","fromEmail":"mugdhapattnaik@gmail.com","sentAt":"2021-11-16T18:20:45Z","receivedAt":"2021-11-16T18:21:07Z","isPatch":true,"sender":{"key":"mugdhapattnaik@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53373985?v=4"},"body":"Apologies for the late reply, I got caught up with other commitments.\n\n> On Fri, Oct 8, 2021 at 1:11 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> \"Mugdha Pattnaik via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>\n> > From: mugdha <mugdhapattnaik@gmail.com>\n>\n> This line is called \"in-body header\" that is used to override the\n> author name that is automatically obtained from the e-mail's \"From:\"\n> header (which is set to \"Mugdha Pattnaik via GitGitGadget\" by GGG,\n> which is obviously not your name, hence we use an in-body header to\n> override it).  It should match what you use to sign off your patches,\n> the one we see below.\n>\n> I'll hand-edit so that \"git show\" will say that the author is\n> \"Mugdha Pattnaik\", not \"mugdha\", while applying, but please make\n> sure your further submissions will not have this problem.\n\nOkay, I will keep this in mind for future patches.\n\n\n> > Currently, running 'git submodule deinit' on repos where the\n> > submodule's '.git' is a directory, aborts with a message that is not\n> > exactly user friendly. Let's change this to instead warn the user\n> > to rerun the command with '--force'.\n>\n> OK.  That sounds like an improvement, albeit possibly an overly\n> cautious one, as a casual \"deinit\" user will get an error as before\n> without \"--force\", which may or may not be a good thing.  Requiring\n> \"--force\" means it is safer by default by not changing the on-disk\n> data.  But requiring \"--force\" also means we end up training users\n> to say \"--force\" when it shouldn't have to be.\n>\n> A plausible alternative is to always absorb but with a warning \"we\n> absorbed it for you\", without requiring \"--force\".  If we didn't\n> have \"git submodule deinit\" command now and were designing it from\n> scratch, would we design it to fail because the submodule's git\n> directory is not absorbed?  I doubt it, as I do not think of a good\n> reason to do so offhand.\n\nOkay, I understand now. I'll remove the condition that checks for \"--force\"\nand will go ahead with absorbing the gitdir after displaying the suggested\nwarning message. Currently I suppress the warnings when the \"--quiet\"\nflag is set; I think I will continue to do that, even after implementing the\nabove change. Do let me know if I should be doing otherwise.\n\n\n> Does \"git submodule\" currently reject a \"deinit\" request due to some\n> _other_ conditions or safety concerns and require the \"--force\"\n> option to continue?  Requiring the \"--force\" option to resolve \".git\n> is a directory, and the user wants to make it absorbed\" means that\n> the user will be _forced_ to bypass these _other_ safety valves only\n> to save the submodule repository from destruction when running\n> \"deinit\", which may not be a good trade-off between the safety\n> requirements of these _other_ conditions, if exists, and the one we\n> are dealing with.\n\nThis is definitely a situation we want to avoid. How about we try to run\na check for uncommitted local modifications first? If these are present,\nthen deinit can die with a warning. In case there are no uncommitted\nlocal modifications, deinit can go ahead and absorb the gitdir and do the\nrest.\n\nThe existing submodule--helper.c file already has a check for this, but it\nseems to be doing it below the check for absorption. I can try to switch it\naround to see how that works.\n\n\n> > This internally calls 'absorb_git_dir_into_superproject()', which\n> > moves the git dir into the superproject and replaces it with\n> > a '.git' file. The rest of the deinit function can operate as it\n> > already does with new-style submodules.\n>\n> This is not wrong per-se, but such an implementation detail is\n> something best left for the patch.\n>\n> > We also edit a test case such that it matches the new behaviour of\n> > deinit.\n>\n> \"match the new behaviour\" in what way?\n>\n>     In one test, we used to require \"git submodule deinit\" to fail\n>     even with the \"--force\" option when the submodule's .git/\n>     directory is not absorbed.  Adjust it to expect the operation to\n>     pass.\n>\n> would be a description at the right level of detail, I think.\n\nNoted. I'll apply the above changes.\n\n\n> > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> > index ef2776a9e45..040b26f149d 100644\n> > --- a/builtin/submodule--helper.c\n> > +++ b/builtin/submodule--helper.c\n> > @@ -1539,16 +1539,24 @@ static void deinit_submodule(const char *path, const char *prefix,\n> >               struct strbuf sb_rm = STRBUF_INIT;\n> >               const char *format;\n> >\n> > -             /*\n> > -              * protect submodules containing a .git directory\n> > -              * NEEDSWORK: instead of dying, automatically call\n> > -              * absorbgitdirs and (possibly) warn.\n> > -              */\n> > -             if (is_directory(sub_git_dir))\n> > -                     die(_(\"Submodule work tree '%s' contains a .git \"\n> > -                           \"directory (use 'rm -rf' if you really want \"\n> > -                           \"to remove it including all of its history)\"),\n> > -                         displaypath);\n> > +             if (is_directory(sub_git_dir)) {\n> > +                     if (!(flags & OPT_FORCE))\n> > +                             die(_(\"Submodule work tree '%s' contains a \"\n> > +                                   \".git directory.\\nUse --force if you want \"\n> > +                                   \"to move its contents to superproject's \"\n> > +                                   \"module directory and convert .git to a file \"\n> > +                                   \"and then proceed with deinit.\"),\n> > +                                 displaypath);\n> > +\n> > +                     if (!(flags & OPT_QUIET))\n> > +                             warning(_(\"Submodule work tree '%s' contains a .git \"\n> > +                                       \"directory. This will be replaced with a \"\n> > +                                       \".git file by using absorbgitdirs.\"),\n> > +                                     displaypath);\n> > +\n> > +                     absorb_git_dir_into_superproject(displaypath, flags);\n>\n> Shouldn't the first argument to this call be \"path\" not\n> \"displaypath\"?  The paths in messages may want to have the path from\n> the top to the submodule location prefixed for human consumption,\n> but the called function only cares about the path to the submodule\n> from the current directory, no?\n\nYes that makes sense, path is the better argument to pass.\n\n\n> The second parameter of this call seems totally bogus to me.  What\n> is the vocabulary of bits the called function takes?  Is that from\n> the same set the flags this function takes?  Does the called\n> function even understand OPT_QUIET, or does the bitpattern that\n> happens to correspond to OPT_QUIET have a totally different meaning\n> to the called function and we will trigger a behaviour that this\n> caller does not expect at all?\n\nI realised I was passing the wrong flags. On investigating further, the\nflags in submodule.c do have different semantics. I also noticed that it\nchecks for recursive submodule absorption. I believe I should just be\npassing the recursive flag to the function that absorbs gitdir. This way,\nnested old-style gitdirs will also be handled.\n\nI intend to incorporate these changes by the end of the week.\n\nThanks\n\n--\nMugdha\n"},{"id":"441347","messageId":"xmqqy25o9bie.fsf@gitster.g","threadId":"56384","inReplyTo":"CAA4dvxgNJ8eyuc5B6_GnSLHMWjdbJN5k_rTCLjWndEyjv_vOnw@mail.gmail.com","subject":"Re: [PATCH v6] submodule: absorb git dir instead of dying on deinit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-16T18:54:49Z","receivedAt":"2021-11-16T18:54:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mugdha Pattnaik <mugdhapattnaik@gmail.com> writes:\n\n>> OK.  That sounds like an improvement, albeit possibly an overly\n>> cautious one, as a casual \"deinit\" user will get an error as before\n>> without \"--force\", which may or may not be a good thing.  Requiring\n>> \"--force\" means it is safer by default by not changing the on-disk\n>> data.  But requiring \"--force\" also means we end up training users\n>> to say \"--force\" when it shouldn't have to be.\n>> ...\n>> Does \"git submodule\" currently reject a \"deinit\" request due to some\n>> _other_ conditions or safety concerns and require the \"--force\"\n>> option to continue?  Requiring the \"--force\" option to resolve \".git\n>> is a directory, and the user wants to make it absorbed\" means that\n>> the user will be _forced_ to bypass these _other_ safety valves only\n>> to save the submodule repository from destruction when running\n>> \"deinit\", which may not be a good trade-off between the safety\n>> requirements of these _other_ conditions, if exists, and the one we\n>> are dealing with.\n>\n> This is definitely a situation we want to avoid. How about we try to run\n> a check for uncommitted local modifications first?\n\nI am not sure if I follow.  If we stop (ab)using \"--force\" for the\nsituation (i.e. where today's \"deinit\" would die because .git needs\nto be absorbed first), then the user would not have to say \"--force\"\nwhich may override other safety valve.  You'd check if .git needs to\nbe absorbed, make it absorbed as needed while reporting the fact\nthat you did so, and then let the existing \"deinit\" code to take\nover.  If there are other safety checks that needs \"--force\" to be\noverridden, that is handled (presumably) correctly by the existing\ncode, no?  So other than \"do we need absorbing, and if so do it for\nthe user\" check, I do not think you'd need to add any new \"we try to\nrun a check for ...\" at all.\n\n\n"},{"id":"441404","messageId":"CAA4dvxixLKAQeLTUnLFvnwrAzQg2w0zqH7gCUqYLXjf=SAP-wA@mail.gmail.com","threadId":"56384","inReplyTo":"xmqqy25o9bie.fsf@gitster.g","subject":"Re: [PATCH v6] submodule: absorb git dir instead of dying on deinit","fromName":"Mugdha Pattnaik","fromEmail":"mugdhapattnaik@gmail.com","sentAt":"2021-11-17T05:55:06Z","receivedAt":"2021-11-17T05:55:26Z","isPatch":true,"sender":{"key":"mugdhapattnaik@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53373985?v=4"},"body":"On Wed, Nov 17, 2021 at 12:24 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Mugdha Pattnaik <mugdhapattnaik@gmail.com> writes:\n>\n> >> OK.  That sounds like an improvement, albeit possibly an overly\n> >> cautious one, as a casual \"deinit\" user will get an error as before\n> >> without \"--force\", which may or may not be a good thing.  Requiring\n> >> \"--force\" means it is safer by default by not changing the on-disk\n> >> data.  But requiring \"--force\" also means we end up training users\n> >> to say \"--force\" when it shouldn't have to be.\n> >> ...\n> >> Does \"git submodule\" currently reject a \"deinit\" request due to some\n> >> _other_ conditions or safety concerns and require the \"--force\"\n> >> option to continue?  Requiring the \"--force\" option to resolve \".git\n> >> is a directory, and the user wants to make it absorbed\" means that\n> >> the user will be _forced_ to bypass these _other_ safety valves only\n> >> to save the submodule repository from destruction when running\n> >> \"deinit\", which may not be a good trade-off between the safety\n> >> requirements of these _other_ conditions, if exists, and the one we\n> >> are dealing with.\n> >\n> > This is definitely a situation we want to avoid. How about we try to run\n> > a check for uncommitted local modifications first?\n>\n> I am not sure if I follow.  If we stop (ab)using \"--force\" for the\n> situation (i.e. where today's \"deinit\" would die because .git needs\n> to be absorbed first), then the user would not have to say \"--force\"\n> which may override other safety valve.  You'd check if .git needs to\n> be absorbed, make it absorbed as needed while reporting the fact\n> that you did so, and then let the existing \"deinit\" code to take\n> over.  If there are other safety checks that needs \"--force\" to be\n> overridden, that is handled (presumably) correctly by the existing\n> code, no?  So other than \"do we need absorbing, and if so do it for\n> the user\" check, I do not think you'd need to add any new \"we try to\n> run a check for ...\" at all.\n\nYes, I understand why \"--force\" should not be used. The reason why I\nsuggested the check for local modifications is because I thought we\nshould warn users that they have local modifications before we absorb\nthe gitdir. But I see now that this is okay, considering deinit would\ndie in such a situation anyway, and users would not lose their work.\nThe only side-effect of running deinit despite users having local\nmodifications, would be that the gitdir of the submodule has been\nabsorbed and that should be okay.\n\n-- \nMugdha\n"},{"id":"441713","messageId":"pull.1078.v7.git.git.1637319387717.gitgitgadget@gmail.com","threadId":"56384","inReplyTo":"pull.1078.v6.git.git.1633523057369.gitgitgadget@gmail.com","subject":"[PATCH v7] submodule: absorb git dir instead of dying on deinit","fromName":"Mugdha Pattnaik via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-11-19T10:56:27Z","receivedAt":"2021-11-19T10:56:33Z","isPatch":true,"sender":{"key":"mugdhapattnaik@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53373985?v=4"},"body":"From: Mugdha Pattnaik <mugdhapattnaik@gmail.com>\n\nCurrently, running 'git submodule deinit' on repos where the\nsubmodule's '.git' is a directory, aborts with a message that is not\nexactly user friendly.\n\nLet's change this to instead warn the user that the .git/ directory\nhas been absorbed into the superproject.\nThe rest of the deinit function can operate as it already does with\nnew-style submodules.\n\nIn one test, we used to require \"git submodule deinit\" to fail even\nwith the \"--force\" option when the submodule's .git/ directory is not\nabsorbed. Adjust it to expect the operation to pass.\n\nSuggested-by: Atharva Raykar <raykar.ath@gmail.com>\nSigned-off-by: Mugdha Pattnaik <mugdhapattnaik@gmail.com>\n---\n    submodule: absorb git dir instead of dying on deinit\n    \n    Changes since v6:\n    \n     * Edited commit message based on suggestions given.\n     * Passed correct arguments to absorb gitdir function; path and recurse\n       flag\n     * Modified behaviour of deinit such that it absorbs the gitdir even\n       when not forced.\n    \n    Changes since v5:\n    \n     * Fixed accidental submission of old version\n    \n    Changes since v4:\n    \n     * Changed test case from \"! test -d\" to \"test_path_is_missing\"\n    \n    Changes since v3:\n    \n     * Replaced 1 instance of the word \"folder\" with \"directory\"\n     * Fixed tab spacing\n    \n    Changes since v2:\n    \n     * Replaced all instances of the word \"folder\" with either \"directory\"\n       or \"git dir\"\n    \n    Changes since v1:\n    \n     * Removed extra indent within the if statements\n     * Moved absorb_git_dir_into_superproject() call outside the if\n       condition checking for --quiet flag\n    \n    ------------------------------------------------------------------------\n    \n    Currently, running 'git submodule deinit' on repos where the submodule's\n    '.git' is a directory, aborts with a message that is not exactly user\n    friendly.\n    \n    Let's change this to instead warn the user that the .git/ directory has\n    been absorbed into the superproject. The rest of the deinit function can\n    operate as it already does with new-style submodules.\n    \n    In one test, we used to require \"git submodule deinit\" to fail even with\n    the \"--force\" option when the submodule's .git/ directory is not\n    absorbed. Adjust it to expect the operation to pass.\n    \n    I have changed the 'cp -R ../.git/modules/example .git' to 'mv\n    ../.git/modules/example .git' since, at the time of testing, the test\n    would fail - deinit now would be moving the '.git' directory into the\n    superproject's '.git/modules/' directory, and since this same directory\n    already existed before, it was causing errors. So, before running\n    deinit, instead of copying the '.git' directory into the submodule, if\n    we move it there instead, this functionality can be appropriately\n    tested.\n    \n    Thank you, Mugdha\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1078%2Fmugdhapattnaik%2Fsubmodule-deinit-absorbgitdirs-v7\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1078/mugdhapattnaik/submodule-deinit-absorbgitdirs-v7\nPull-Request: https://github.com/git/git/pull/1078\n\nRange-diff vs v6:\n\n 1:  384a6742388 ! 1:  f37724016f0 submodule: absorb git dir instead of dying on deinit\n     @@\n       ## Metadata ##\n     -Author: mugdha <mugdhapattnaik@gmail.com>\n     +Author: Mugdha Pattnaik <mugdhapattnaik@gmail.com>\n      \n       ## Commit message ##\n          submodule: absorb git dir instead of dying on deinit\n      \n          Currently, running 'git submodule deinit' on repos where the\n          submodule's '.git' is a directory, aborts with a message that is not\n     -    exactly user friendly. Let's change this to instead warn the user\n     -    to rerun the command with '--force'.\n     +    exactly user friendly.\n      \n     -    This internally calls 'absorb_git_dir_into_superproject()', which\n     -    moves the git dir into the superproject and replaces it with\n     -    a '.git' file. The rest of the deinit function can operate as it\n     -    already does with new-style submodules.\n     +    Let's change this to instead warn the user that the .git/ directory\n     +    has been absorbed into the superproject.\n     +    The rest of the deinit function can operate as it already does with\n     +    new-style submodules.\n      \n     -    We also edit a test case such that it matches the new behaviour of\n     -    deinit.\n     +    In one test, we used to require \"git submodule deinit\" to fail even\n     +    with the \"--force\" option when the submodule's .git/ directory is not\n     +    absorbed. Adjust it to expect the operation to pass.\n      \n          Suggested-by: Atharva Raykar <raykar.ath@gmail.com>\n          Signed-off-by: Mugdha Pattnaik <mugdhapattnaik@gmail.com>\n     @@ builtin/submodule--helper.c: static void deinit_submodule(const char *path, cons\n      -\t\t\t      \"to remove it including all of its history)\"),\n      -\t\t\t    displaypath);\n      +\t\tif (is_directory(sub_git_dir)) {\n     -+\t\t\tif (!(flags & OPT_FORCE))\n     -+\t\t\t\tdie(_(\"Submodule work tree '%s' contains a \"\n     -+\t\t\t\t      \".git directory.\\nUse --force if you want \"\n     -+\t\t\t\t      \"to move its contents to superproject's \"\n     -+\t\t\t\t      \"module directory and convert .git to a file \"\n     -+\t\t\t\t      \"and then proceed with deinit.\"),\n     -+\t\t\t\t    displaypath);\n     -+\n      +\t\t\tif (!(flags & OPT_QUIET))\n      +\t\t\t\twarning(_(\"Submodule work tree '%s' contains a .git \"\n      +\t\t\t\t\t  \"directory. This will be replaced with a \"\n      +\t\t\t\t\t  \".git file by using absorbgitdirs.\"),\n      +\t\t\t\t\tdisplaypath);\n      +\n     -+\t\t\tabsorb_git_dir_into_superproject(displaypath, flags);\n     ++\t\t\tabsorb_git_dir_into_superproject(path,\n     ++\t\t\t\t\t\t\t ABSORB_GITDIR_RECURSE_SUBMODULES);\n      +\n      +\t\t}\n       \n     @@ t/t7400-submodule-basic.sh: test_expect_success 'submodule deinit is silent when\n       '\n       \n      -test_expect_success 'submodule deinit fails when submodule has a .git directory even when forced' '\n     -+test_expect_success 'submodule deinit fails when submodule has a .git directory unless forced' '\n     ++test_expect_success 'submodule deinit absorbs .git directory if .git is a directory' '\n       \tgit submodule update --init &&\n       \t(\n       \t\tcd init &&\n     @@ t/t7400-submodule-basic.sh: test_expect_success 'submodule deinit is silent when\n      +\t\tmv ../.git/modules/example .git &&\n       \t\tGIT_WORK_TREE=. git config --unset core.worktree\n       \t) &&\n     - \ttest_must_fail git submodule deinit init &&\n     +-\ttest_must_fail git submodule deinit init &&\n      -\ttest_must_fail git submodule deinit -f init &&\n      -\ttest -d init/.git &&\n      -\ttest -n \"$(git config --get-regexp \"submodule\\.example\\.\")\"\n     -+\tgit submodule deinit -f init &&\n     ++\tgit submodule deinit init &&\n      +\ttest_path_is_missing init/.git &&\n      +\ttest -z \"$(git config --get-regexp \"submodule\\.example\\.\")\"\n       '\n\n\n builtin/submodule--helper.c | 21 +++++++++++----------\n t/t7400-submodule-basic.sh  | 11 +++++------\n 2 files changed, 16 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex ef2776a9e45..bbab562dec6 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1539,16 +1539,17 @@ static void deinit_submodule(const char *path, const char *prefix,\n \t\tstruct strbuf sb_rm = STRBUF_INIT;\n \t\tconst char *format;\n \n-\t\t/*\n-\t\t * protect submodules containing a .git directory\n-\t\t * NEEDSWORK: instead of dying, automatically call\n-\t\t * absorbgitdirs and (possibly) warn.\n-\t\t */\n-\t\tif (is_directory(sub_git_dir))\n-\t\t\tdie(_(\"Submodule work tree '%s' contains a .git \"\n-\t\t\t      \"directory (use 'rm -rf' if you really want \"\n-\t\t\t      \"to remove it including all of its history)\"),\n-\t\t\t    displaypath);\n+\t\tif (is_directory(sub_git_dir)) {\n+\t\t\tif (!(flags & OPT_QUIET))\n+\t\t\t\twarning(_(\"Submodule work tree '%s' contains a .git \"\n+\t\t\t\t\t  \"directory. This will be replaced with a \"\n+\t\t\t\t\t  \".git file by using absorbgitdirs.\"),\n+\t\t\t\t\tdisplaypath);\n+\n+\t\t\tabsorb_git_dir_into_superproject(path,\n+\t\t\t\t\t\t\t ABSORB_GITDIR_RECURSE_SUBMODULES);\n+\n+\t\t}\n \n \t\tif (!(flags & OPT_FORCE)) {\n \t\t\tstruct child_process cp_rm = CHILD_PROCESS_INIT;\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex cb1b8e35dbf..e7cec2e457a 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -1182,18 +1182,17 @@ test_expect_success 'submodule deinit is silent when used on an uninitialized su\n \trmdir init example2\n '\n \n-test_expect_success 'submodule deinit fails when submodule has a .git directory even when forced' '\n+test_expect_success 'submodule deinit absorbs .git directory if .git is a directory' '\n \tgit submodule update --init &&\n \t(\n \t\tcd init &&\n \t\trm .git &&\n-\t\tcp -R ../.git/modules/example .git &&\n+\t\tmv ../.git/modules/example .git &&\n \t\tGIT_WORK_TREE=. git config --unset core.worktree\n \t) &&\n-\ttest_must_fail git submodule deinit init &&\n-\ttest_must_fail git submodule deinit -f init &&\n-\ttest -d init/.git &&\n-\ttest -n \"$(git config --get-regexp \"submodule\\.example\\.\")\"\n+\tgit submodule deinit init &&\n+\ttest_path_is_missing init/.git &&\n+\ttest -z \"$(git config --get-regexp \"submodule\\.example\\.\")\"\n '\n \n test_expect_success 'submodule with UTF-8 name' '\n\nbase-commit: c4203212e360b25a1c69467b5a8437d45a373cac\n-- \ngitgitgadget\n"}]}