{"thread":{"id":"44772","subject":"[PATCHv6] rm: absorb a submodules git dir before deletion","startedAt":"2016-12-27T19:10:20Z","lastAt":"2016-12-27T19:10:20Z","messageCount":1,"participants":["Stefan Beller"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"308402","messageId":"20161227190314.22655-1-sbeller@google.com","threadId":"44772","inReplyTo":null,"subject":"[PATCHv6] rm: absorb a submodules git dir before deletion","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-27T19:03:14Z","receivedAt":"2016-12-27T19:10:20Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"When deleting a submodule, we need to keep the actual git directory around,\nsuch that we do not lose local changes in there and at a later checkout\nof the submodule we don't need to clone it again.\n\nNow that the functionality is available to absorb the git directory of a\nsubmodule, rewrite the checking in git-rm to not complain, but rather\nrelocate the git directories inside the superproject.\n\nAn alternative solution was discussed to have a function\n`depopulate_submodule`. That would couple the check for its git directory\nand possible relocation before the the removal, such that it is less\nlikely to miss the check in the future.  But the indirection with such\na function added seemed also complex. The reason for that was that this\npossible move of the git directory was also implemented in\n`ok_to_remove_submodule`, such that this function could truthfully\nanswer whether it is ok to remove the submodule.\n\nThe solution proposed here defers all these checks to the caller.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\nThis replaces the last patch of the series \n\"git-rm absorbs submodule git directory before deletion\".\nnamed \"rm: absorb a submodules git dir before deletion\".\n\nThe only change is a guard (!index_only) for\nsubmodules_absorb_gitdir_if_needed(prefix);\n\nJunio, the other concern you raised about not checking\nfor local modifications in the submodule has been answered\nas a reply there; we do check for local modifications there,\nthough it is not obvious.\n\nThanks,\nStefan\n\n builtin/rm.c  | 80 ++++++++++++++---------------------------------------------\n t/t3600-rm.sh | 39 +++++++++++------------------\n 2 files changed, 34 insertions(+), 85 deletions(-)\n\ndiff --git a/builtin/rm.c b/builtin/rm.c\nindex 5a5a66272b..20635dca94 100644\n--- a/builtin/rm.c\n+++ b/builtin/rm.c\n@@ -59,27 +59,9 @@ static void print_error_files(struct string_list *files_list,\n \t}\n }\n \n-static void error_removing_concrete_submodules(struct string_list *files, int *errs)\n-{\n-\tprint_error_files(files,\n-\t\t\t  Q_(\"the following submodule (or one of its nested \"\n-\t\t\t     \"submodules)\\n\"\n-\t\t\t     \"uses a .git directory:\",\n-\t\t\t     \"the following submodules (or one of their nested \"\n-\t\t\t     \"submodules)\\n\"\n-\t\t\t     \"use a .git directory:\", files->nr),\n-\t\t\t  _(\"\\n(use 'rm -rf' if you really want to remove \"\n-\t\t\t    \"it including all of its history)\"),\n-\t\t\t  errs);\n-\tstring_list_clear(files, 0);\n-}\n-\n-static int check_submodules_use_gitfiles(void)\n+static void submodules_absorb_gitdir_if_needed(const char *prefix)\n {\n \tint i;\n-\tint errs = 0;\n-\tstruct string_list files = STRING_LIST_INIT_NODUP;\n-\n \tfor (i = 0; i < list.nr; i++) {\n \t\tconst char *name = list.entry[i].name;\n \t\tint pos;\n@@ -99,12 +81,9 @@ static int check_submodules_use_gitfiles(void)\n \t\t\tcontinue;\n \n \t\tif (!submodule_uses_gitfile(name))\n-\t\t\tstring_list_append(&files, name);\n+\t\t\tabsorb_git_dir_into_superproject(prefix, name,\n+\t\t\t\tABSORB_GITDIR_RECURSE_SUBMODULES);\n \t}\n-\n-\terror_removing_concrete_submodules(&files, &errs);\n-\n-\treturn errs;\n }\n \n static int check_local_mod(struct object_id *head, int index_only)\n@@ -120,7 +99,6 @@ static int check_local_mod(struct object_id *head, int index_only)\n \tint errs = 0;\n \tstruct string_list files_staged = STRING_LIST_INIT_NODUP;\n \tstruct string_list files_cached = STRING_LIST_INIT_NODUP;\n-\tstruct string_list files_submodule = STRING_LIST_INIT_NODUP;\n \tstruct string_list files_local = STRING_LIST_INIT_NODUP;\n \n \tno_head = is_null_oid(head);\n@@ -219,13 +197,8 @@ static int check_local_mod(struct object_id *head, int index_only)\n \t\telse if (!index_only) {\n \t\t\tif (staged_changes)\n \t\t\t\tstring_list_append(&files_cached, name);\n-\t\t\tif (local_changes) {\n-\t\t\t\tif (S_ISGITLINK(ce->ce_mode) &&\n-\t\t\t\t    !submodule_uses_gitfile(name))\n-\t\t\t\t\tstring_list_append(&files_submodule, name);\n-\t\t\t\telse\n-\t\t\t\t\tstring_list_append(&files_local, name);\n-\t\t\t}\n+\t\t\tif (local_changes)\n+\t\t\t\tstring_list_append(&files_local, name);\n \t\t}\n \t}\n \tprint_error_files(&files_staged,\n@@ -247,8 +220,6 @@ static int check_local_mod(struct object_id *head, int index_only)\n \t\t\t  &errs);\n \tstring_list_clear(&files_cached, 0);\n \n-\terror_removing_concrete_submodules(&files_submodule, &errs);\n-\n \tprint_error_files(&files_local,\n \t\t\t  Q_(\"the following file has local modifications:\",\n \t\t\t     \"the following files have local modifications:\",\n@@ -342,6 +313,9 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n \t\t\texit(0);\n \t}\n \n+\tif (!index_only)\n+\t\tsubmodules_absorb_gitdir_if_needed(prefix);\n+\n \t/*\n \t * If not forced, the file, the index and the HEAD (if exists)\n \t * must match; but the file can already been removed, since\n@@ -358,9 +332,6 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n \t\t\toidclr(&oid);\n \t\tif (check_local_mod(&oid, index_only))\n \t\t\texit(1);\n-\t} else if (!index_only) {\n-\t\tif (check_submodules_use_gitfiles())\n-\t\t\texit(1);\n \t}\n \n \t/*\n@@ -389,32 +360,20 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n \t */\n \tif (!index_only) {\n \t\tint removed = 0, gitmodules_modified = 0;\n-\t\tstruct strbuf buf = STRBUF_INIT;\n \t\tfor (i = 0; i < list.nr; i++) {\n \t\t\tconst char *path = list.entry[i].name;\n \t\t\tif (list.entry[i].is_submodule) {\n-\t\t\t\tif (is_empty_dir(path)) {\n-\t\t\t\t\tif (!rmdir(path)) {\n-\t\t\t\t\t\tremoved = 1;\n-\t\t\t\t\t\tif (!remove_path_from_gitmodules(path))\n-\t\t\t\t\t\t\tgitmodules_modified = 1;\n-\t\t\t\t\t\tcontinue;\n-\t\t\t\t\t}\n-\t\t\t\t} else {\n-\t\t\t\t\tstrbuf_reset(&buf);\n-\t\t\t\t\tstrbuf_addstr(&buf, path);\n-\t\t\t\t\tif (!remove_dir_recursively(&buf, 0)) {\n-\t\t\t\t\t\tremoved = 1;\n-\t\t\t\t\t\tif (!remove_path_from_gitmodules(path))\n-\t\t\t\t\t\t\tgitmodules_modified = 1;\n-\t\t\t\t\t\tstrbuf_release(&buf);\n-\t\t\t\t\t\tcontinue;\n-\t\t\t\t\t} else if (!file_exists(path))\n-\t\t\t\t\t\t/* Submodule was removed by user */\n-\t\t\t\t\t\tif (!remove_path_from_gitmodules(path))\n-\t\t\t\t\t\t\tgitmodules_modified = 1;\n-\t\t\t\t\t/* Fallthrough and let remove_path() fail. */\n-\t\t\t\t}\n+\t\t\t\tstruct strbuf buf = STRBUF_INIT;\n+\n+\t\t\t\tstrbuf_addstr(&buf, path);\n+\t\t\t\tif (remove_dir_recursively(&buf, 0))\n+\t\t\t\t\tdie(_(\"could not remove '%s'\"), path);\n+\t\t\t\tstrbuf_release(&buf);\n+\n+\t\t\t\tremoved = 1;\n+\t\t\t\tif (!remove_path_from_gitmodules(path))\n+\t\t\t\t\tgitmodules_modified = 1;\n+\t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (!remove_path(path)) {\n \t\t\t\tremoved = 1;\n@@ -423,7 +382,6 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n \t\t\tif (!removed)\n \t\t\t\tdie_errno(\"git rm: '%s'\", path);\n \t\t}\n-\t\tstrbuf_release(&buf);\n \t\tif (gitmodules_modified)\n \t\t\tstage_updated_gitmodules();\n \t}\ndiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\nindex bcbb680651..5aa6db584c 100755\n--- a/t/t3600-rm.sh\n+++ b/t/t3600-rm.sh\n@@ -569,26 +569,22 @@ test_expect_success 'rm of a conflicted unpopulated submodule succeeds' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'rm of a populated submodule with a .git directory fails even when forced' '\n+test_expect_success 'rm of a populated submodule with a .git directory migrates git dir' '\n \tgit checkout -f master &&\n \tgit reset --hard &&\n \tgit submodule update &&\n \t(cd submod &&\n \t\trm .git &&\n \t\tcp -R ../.git/modules/sub .git &&\n-\t\tGIT_WORK_TREE=. git config --unset core.worktree\n+\t\tGIT_WORK_TREE=. git config --unset core.worktree &&\n+\t\trm -r ../.git/modules/sub\n \t) &&\n-\ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -d submod/.git &&\n-\tgit status -s -uno --ignore-submodules=none >actual &&\n-\t! test -s actual &&\n-\ttest_must_fail git rm -f submod &&\n-\ttest -d submod &&\n-\ttest -d submod/.git &&\n+\tgit rm submod 2>output.err &&\n+\t! test -d submod &&\n+\t! test -d submod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n-\t! test -s actual &&\n-\trm -rf submod\n+\ttest -s actual &&\n+\ttest_i18ngrep Migrating output.err\n '\n \n cat >expect.deepmodified <<EOF\n@@ -667,24 +663,19 @@ test_expect_success 'rm of a populated nested submodule with a nested .git direc\n \tgit submodule update --recursive &&\n \t(cd submod/subsubmod &&\n \t\trm .git &&\n-\t\tcp -R ../../.git/modules/sub/modules/sub .git &&\n+\t\tmv ../../.git/modules/sub/modules/sub .git &&\n \t\tGIT_WORK_TREE=. git config --unset core.worktree\n \t) &&\n-\ttest_must_fail git rm submod &&\n-\ttest -d submod &&\n-\ttest -d submod/subsubmod/.git &&\n-\tgit status -s -uno --ignore-submodules=none >actual &&\n-\t! test -s actual &&\n-\ttest_must_fail git rm -f submod &&\n-\ttest -d submod &&\n-\ttest -d submod/subsubmod/.git &&\n+\tgit rm submod 2>output.err &&\n+\t! test -d submod &&\n+\t! test -d submod/subsubmod/.git &&\n \tgit status -s -uno --ignore-submodules=none >actual &&\n-\t! test -s actual &&\n-\trm -rf submod\n+\ttest -s actual &&\n+\ttest_i18ngrep Migrating output.err\n '\n \n test_expect_success 'checking out a commit after submodule removal needs manual updates' '\n-\tgit commit -m \"submodule removal\" submod &&\n+\tgit commit -m \"submodule removal\" submod .gitmodules &&\n \tgit checkout HEAD^ &&\n \tgit submodule update &&\n \tgit checkout -q HEAD^ &&\n-- \n2.11.0.rc2.50.g8bda6b2.dirty\n\n"}]}