{"thread":{"id":"44694","subject":"[PATCHv2 1/5] submodule.h: add extern keyword to functions","startedAt":"2016-12-13T20:56:39Z","lastAt":"2016-12-14T19:01:53Z","messageCount":10,"participants":["Stefan Beller","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"307680","messageId":"20161213205622.841-2-sbeller@google.com","threadId":"44694","inReplyTo":"20161213205622.841-1-sbeller@google.com","subject":"[PATCHv2 1/5] submodule.h: add extern keyword to functions","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-13T20:56:18Z","receivedAt":"2016-12-13T20:56:39Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"As the upcoming series will add a lot of functions to the submodule\nheader, let's first make the header consistent to the rest of the project\nby adding the extern keyword to functions.\n\nAs per the CodingGuidelines we try to stay below 80 characters per line,\nso adapt all those functions to stay below 80 characters that are already\nusing more than one line. Those function using just one line are better\nkept in one line than breaking them up into multiple lines just for the\ngoal of staying below the character limit as it makes grepping\nfor functions easier if they are one liners.\n\nSigned-off-by: Stefan Beller \n---\n submodule.h | 55 ++++++++++++++++++++++++++++++-------------------------\n 1 file changed, 30 insertions(+), 25 deletions(-)\n\ndiff --git a/submodule.h b/submodule.h\nindex 6229054b99..61fb610749 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -29,50 +29,55 @@ struct submodule_update_strategy {\n };\n #define SUBMODULE_UPDATE_STRATEGY_INIT {SM_UPDATE_UNSPECIFIED, NULL}\n \n-int is_staging_gitmodules_ok(void);\n-int update_path_in_gitmodules(const char *oldpath, const char *newpath);\n-int remove_path_from_gitmodules(const char *path);\n-void stage_updated_gitmodules(void);\n-void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,\n+extern int is_staging_gitmodules_ok(void);\n+extern int update_path_in_gitmodules(const char *oldpath, const char *newpath);\n+extern int remove_path_from_gitmodules(const char *path);\n+extern void stage_updated_gitmodules(void);\n+extern void set_diffopt_flags_from_submodule_config(struct diff_options *,\n \t\tconst char *path);\n-int submodule_config(const char *var, const char *value, void *cb);\n-void gitmodules_config(void);\n-int parse_submodule_update_strategy(const char *value,\n+extern int submodule_config(const char *var, const char *value, void *cb);\n+extern void gitmodules_config(void);\n+extern int parse_submodule_update_strategy(const char *value,\n \t\tstruct submodule_update_strategy *dst);\n-const char *submodule_strategy_to_string(const struct submodule_update_strategy *s);\n-void handle_ignore_submodules_arg(struct diff_options *diffopt, const char *);\n-void show_submodule_summary(FILE *f, const char *path,\n+extern const char *submodule_strategy_to_string(const struct submodule_update_strategy *s);\n+extern void handle_ignore_submodules_arg(struct diff_options *, const char *);\n+extern void show_submodule_summary(FILE *f, const char *path,\n \t\tconst char *line_prefix,\n \t\tstruct object_id *one, struct object_id *two,\n \t\tunsigned dirty_submodule, const char *meta,\n \t\tconst char *del, const char *add, const char *reset);\n-void show_submodule_inline_diff(FILE *f, const char *path,\n+extern void show_submodule_inline_diff(FILE *f, const char *path,\n \t\tconst char *line_prefix,\n \t\tstruct object_id *one, struct object_id *two,\n \t\tunsigned dirty_submodule, const char *meta,\n \t\tconst char *del, const char *add, const char *reset,\n \t\tconst struct diff_options *opt);\n-void set_config_fetch_recurse_submodules(int value);\n-void check_for_new_submodule_commits(unsigned char new_sha1[20]);\n-int fetch_populated_submodules(const struct argv_array *options,\n+extern void set_config_fetch_recurse_submodules(int value);\n+extern void check_for_new_submodule_commits(unsigned char new_sha1[20]);\n+extern int fetch_populated_submodules(const struct argv_array *options,\n \t\t\t const char *prefix, int command_line_option,\n \t\t\t int quiet, int max_parallel_jobs);\n-unsigned is_submodule_modified(const char *path, int ignore_untracked);\n-int submodule_uses_gitfile(const char *path);\n-int ok_to_remove_submodule(const char *path);\n-int merge_submodule(unsigned char result[20], const char *path, const unsigned char base[20],\n-\t\t const unsigned char a[20], const unsigned char b[20], int search);\n-int find_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name,\n-\t\tstruct string_list *needs_pushing);\n-int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name);\n-int parallel_submodules(void);\n+extern unsigned is_submodule_modified(const char *path, int ignore_untracked);\n+extern int submodule_uses_gitfile(const char *path);\n+extern int ok_to_remove_submodule(const char *path);\n+extern int merge_submodule(unsigned char result[20], const char *path,\n+\t\t\t const unsigned char base[20],\n+\t\t\t const unsigned char a[20],\n+\t\t\t const unsigned char b[20], int search);\n+extern int find_unpushed_submodules(unsigned char new_sha1[20],\n+\t\t\t\t const char *remotes_name,\n+\t\t\t\t struct string_list *needs_pushing);\n+extern int push_unpushed_submodules(unsigned char new_sha1[20],\n+\t\t\t\t const char *remotes_name);\n+extern void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);\n+extern int parallel_submodules(void);\n \n /*\n * Prepare the \"env_array\" parameter of a \"struct child_process\" for executing\n * a submodule by clearing any repo-specific envirionment variables, but\n * retaining any config in the environment.\n */\n-void prepare_submodule_repo_env(struct argv_array *out);\n+extern void prepare_submodule_repo_env(struct argv_array *out);\n \n #define ABSORB_GITDIR_RECURSE_SUBMODULES (1<<0)\n extern void absorb_git_dir_into_superproject(const char *prefix,\n-- \n2.11.0.rc2.35.g26e18c9\n\n"},{"id":"307681","messageId":"20161213205622.841-1-sbeller@google.com","threadId":"44694","inReplyTo":null,"subject":"[PATCHv2 0/5] git-rm absorbs submodule git directory before deletion","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-13T20:56:17Z","receivedAt":"2016-12-13T20:56:49Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"v2:\n* new base where to apply the patch:\n sb/submodule-embed-gitdir merged with sb/t3600-cleanup.\n I got merge conflicts and resolved them this way:\n#@@@ -709,9 -687,10 +687,9 @@@ test_expect_success 'checking out a com\n# git commit -m \"submodule removal\" submod &&\n# git checkout HEAD^ &&\n# git submodule update &&\n#- git checkout -q HEAD^ 2>actual &&\n#+ git checkout -q HEAD^ &&\n# git checkout -q master 2>actual &&\n# - echo \"warning: unable to rmdir submod: Directory not empty\" >expected &&\n# - test_i18ncmp expected actual &&\n# + test_i18ngrep \"^warning: unable to rmdir submod:\" actual &&\n# git status -s submod >actual &&\n# echo \"?? submod/\" >expected &&\n# test_cmp expected actual &&\n#\n\n* improved commit message in \"ok_to_remove_submodule: absorb the submodule git dir\"\n (David Turner offered me some advice on how to write better English off list)\n* simplified code in last patch:\n -> dropped wrong comment for fallthrough\n -> moved redundant code out of both bodies of an if-clause.\n* Fixed last patchs commit message to have \"or_die\" instead of or_dir.\n\nv1:\nThe \"checkout --recurse-submodules\" series got too large to comfortably send\nit out for review, so I had to break it up into smaller series'; this is the\nfirst subseries, but it makes sense on its own.\n\nThis series teaches git-rm to absorb the git directory of a submodule instead\nof failing and complaining about the git directory preventing deletion.\n\nIt applies on origin/sb/submodule-embed-gitdir.\n\nAny feedback welcome!\n\nThanks,\nStefan\n\nStefan Beller (5):\n submodule.h: add extern keyword to functions\n submodule: modernize ok_to_remove_submodule to use argv_array\n submodule: add flags to ok_to_remove_submodule\n ok_to_remove_submodule: absorb the submodule git dir\n rm: add absorb a submodules git dir before deletion\n\n builtin/rm.c | 33 ++++++++-----------------\n cache.h | 2 ++\n entry.c | 5 ++++\n submodule.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++----------\n submodule.h | 64 ++++++++++++++++++++++++++++++-------------------\n t/t3600-rm.sh | 39 ++++++++++++------------------\n 6 files changed, 135 insertions(+), 85 deletions(-)\n\n-- \n2.11.0.rc2.35.g26e18c9\n\n"},{"id":"307682","messageId":"20161213205622.841-3-sbeller@google.com","threadId":"44694","inReplyTo":"20161213205622.841-1-sbeller@google.com","subject":"[PATCHv2 2/5] submodule: modernize ok_to_remove_submodule to use argv_array","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-13T20:56:19Z","receivedAt":"2016-12-13T20:56:53Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Instead of constructing the NULL terminated array ourselves, we\nshould make use of the argv_array infrastructure.\n\nWhile at it, adapt the error messages to reflect the actual invocation.\n\nSigned-off-by: Stefan Beller \n---\n submodule.c | 14 ++++----------\n 1 file changed, 4 insertions(+), 10 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 45ccfb7ab4..9f0b544ebe 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1023,13 +1023,6 @@ int ok_to_remove_submodule(const char *path)\n {\n \tssize_t len;\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n-\tconst char *argv[] = {\n-\t\t\"status\",\n-\t\t\"--porcelain\",\n-\t\t\"-u\",\n-\t\t\"--ignore-submodules=none\",\n-\t\tNULL,\n-\t};\n \tstruct strbuf buf = STRBUF_INIT;\n \tint ok_to_remove = 1;\n \n@@ -1039,14 +1032,15 @@ int ok_to_remove_submodule(const char *path)\n \tif (!submodule_uses_gitfile(path))\n \t\treturn 0;\n \n-\tcp.argv = argv;\n+\targv_array_pushl(&cp.args, \"status\", \"--porcelain\", \"-u\",\n+\t\t\t\t \"--ignore-submodules=none\", NULL);\n \tprepare_submodule_repo_env(&cp.env_array);\n \tcp.git_cmd = 1;\n \tcp.no_stdin = 1;\n \tcp.out = -1;\n \tcp.dir = path;\n \tif (start_command(&cp))\n-\t\tdie(\"Could not run 'git status --porcelain -uall --ignore-submodules=none' in submodule %s\", path);\n+\t\tdie(_(\"could not run 'git status --porcelain -u --ignore-submodules=none' in submodule %s\"), path);\n \n \tlen = strbuf_read(&buf, cp.out, 1024);\n \tif (len > 2)\n@@ -1054,7 +1048,7 @@ int ok_to_remove_submodule(const char *path)\n \tclose(cp.out);\n \n \tif (finish_command(&cp))\n-\t\tdie(\"'git status --porcelain -uall --ignore-submodules=none' failed in submodule %s\", path);\n+\t\tdie(_(\"'git status --porcelain -u --ignore-submodules=none' failed in submodule %s\"), path);\n \n \tstrbuf_release(&buf);\n \treturn ok_to_remove;\n-- \n2.11.0.rc2.35.g26e18c9\n\n"},{"id":"307683","messageId":"20161213205622.841-5-sbeller@google.com","threadId":"44694","inReplyTo":"20161213205622.841-1-sbeller@google.com","subject":"[PATCHv2 4/5] ok_to_remove_submodule: absorb the submodule git dir","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-13T20:56:21Z","receivedAt":"2016-12-13T20:57:42Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"It is a major reason to say no, when deciding if a submodule can be\ndeleted, if the git directory of the submodule being contained in the\nsubmodule's working directory.\n\nMigrate the git directory into the superproject instead of failing,\nand proceed with the other checks.\n\nSigned-off-by: Stefan Beller \n---\n submodule.c | 15 +++++++++++++--\n 1 file changed, 13 insertions(+), 2 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 2d13744b06..e42efa2337 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1026,11 +1026,22 @@ int ok_to_remove_submodule(const char *path, unsigned flags)\n \tstruct strbuf buf = STRBUF_INIT;\n \tint ok_to_remove = 1;\n \n+\t/* Is it there? */\n \tif (!file_exists(path) || is_empty_dir(path))\n \t\treturn 1;\n \n-\tif (!submodule_uses_gitfile(path))\n-\t\treturn 0;\n+\t/* Does it have a .git directory? */\n+\tif (!submodule_uses_gitfile(path)) {\n+\t\tabsorb_git_dir_into_superproject(\"\", path,\n+\t\t\tABSORB_GITDIR_RECURSE_SUBMODULES);\n+\n+\t\t/*\n+\t\t * We should be using a gitfile by now. Let's double\n+\t\t * check as losing the git dir would be fatal.\n+\t\t */\n+\t\tif (!submodule_uses_gitfile(path))\n+\t\t\treturn 0;\n+\t}\n \n \targv_array_pushl(&cp.args, \"status\", \"--porcelain\",\n \t\t\t\t \"--ignore-submodules=none\", NULL);\n-- \n2.11.0.rc2.35.g26e18c9\n\n"},{"id":"307684","messageId":"20161213205622.841-6-sbeller@google.com","threadId":"44694","inReplyTo":"20161213205622.841-1-sbeller@google.com","subject":"[PATCHv2 5/5] rm: add absorb a submodules git dir before deletion","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-13T20:56:22Z","receivedAt":"2016-12-13T20:57:50Z","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\nImplement `depopulate_submodule`, that migrates the git directory before\ndeletion of a submodule and afterwards the equivalent of \"rm -rf\", which\nis already found in entry.c, so expose that and for clarity add a suffix\n\"_or_die\" to it.\n\nSigned-off-by: Stefan Beller \n---\n builtin/rm.c | 30 ++++++++----------------------\n cache.h | 2 ++\n entry.c | 5 +++++\n submodule.c | 31 +++++++++++++++++++++++++++++++\n submodule.h | 6 ++++++\n t/t3600-rm.sh | 39 +++++++++++++++------------------------\n 6 files changed, 67 insertions(+), 46 deletions(-)\n\ndiff --git a/builtin/rm.c b/builtin/rm.c\nindex fdd7183f61..8c9c535a88 100644\n--- a/builtin/rm.c\n+++ b/builtin/rm.c\n@@ -392,28 +392,14 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\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\tif (is_empty_dir(path) && rmdir(path))\n+\t\t\t\t\tdie_errno(\"git rm: '%s'\", path);\n+\t\t\t\tif (file_exists(path))\n+\t\t\t\t\tdepopulate_submodule(path);\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;\ndiff --git a/cache.h b/cache.h\nindex a50a61a197..b645ca2f9a 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -2018,4 +2018,6 @@ void sleep_millisec(int millisec);\n */\n void safe_create_dir(const char *dir, int share);\n \n+extern void remove_directory_or_die(struct strbuf *path);\n+\n #endif /* CACHE_H */\ndiff --git a/entry.c b/entry.c\nindex c6eea240b6..02c4ac9f22 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -73,6 +73,11 @@ static void remove_subtree(struct strbuf *path)\n \t\tdie_errno(\"cannot rmdir '%s'\", path->buf);\n }\n \n+void remove_directory_or_die(struct strbuf *path)\n+{\n+\tremove_subtree(path);\n+}\n+\n static int create_file(const char *path, unsigned int mode)\n {\n \tmode = (mode & 0100) ? 0777 : 0666;\ndiff --git a/submodule.c b/submodule.c\nindex e42efa2337..3770ecb7b9 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -308,6 +308,37 @@ static void print_submodule_summary(struct rev_info *rev, FILE *f,\n \tstrbuf_release(&sb);\n }\n \n+void depopulate_submodule(const char *path)\n+{\n+\tstruct strbuf pathbuf = STRBUF_INIT;\n+\tchar *dot_git = xstrfmt(\"%s/.git\", path);\n+\n+\t/* Is it populated? */\n+\tif (!resolve_gitdir(dot_git))\n+\t\tgoto out;\n+\n+\t/* Does it have a .git directory? */\n+\tif (!submodule_uses_gitfile(path)) {\n+\t\tabsorb_git_dir_into_superproject(\"\", path,\n+\t\t\tABSORB_GITDIR_RECURSE_SUBMODULES);\n+\n+\t\tif (!submodule_uses_gitfile(path)) {\n+\t\t\t/*\n+\t\t\t * We should be using a gitfile by now. Let's double\n+\t\t\t * check as losing the git dir would be fatal.\n+\t\t\t */\n+\t\t\tdie(\"BUG: could not absorb git directory for '%s'\", path);\n+\t\t}\n+\t}\n+\n+\tstrbuf_addstr(&pathbuf, path);\n+\tremove_directory_or_die(&pathbuf);\n+\n+out:\n+\tstrbuf_release(&pathbuf);\n+\tfree(dot_git);\n+}\n+\n /* Helper function to display the submodule header line prior to the full\n * summary output. If it can locate the submodule objects directory it will\n * attempt to lookup both the left and right commits and put them into the\ndiff --git a/submodule.h b/submodule.h\nindex 3ed3aa479a..516e377a12 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -53,6 +53,12 @@ extern void show_submodule_inline_diff(FILE *f, const char *path,\n \t\tconst char *del, const char *add, const char *reset,\n \t\tconst struct diff_options *opt);\n extern void set_config_fetch_recurse_submodules(int value);\n+\n+/*\n+ * Removes a submodule from a given path. When the submodule contains its\n+ * git directory instead of a gitlink, migrate that first into the superproject.\n+ */\n+extern void depopulate_submodule(const char *path);\n extern void check_for_new_submodule_commits(unsigned char new_sha1[20]);\n extern int fetch_populated_submodules(const struct argv_array *options,\n \t\t\t const char *prefix, int command_line_option,\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 <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.35.g26e18c9\n\n"},{"id":"307685","messageId":"20161213205622.841-4-sbeller@google.com","threadId":"44694","inReplyTo":"20161213205622.841-1-sbeller@google.com","subject":"[PATCHv2 3/5] submodule: add flags to ok_to_remove_submodule","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-13T20:56:20Z","receivedAt":"2016-12-13T20:57:52Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In different contexts the question whether deleting a submodule is ok\nto remove may be answered differently.\n\nIn 293ab15eea (submodule: teach rm to remove submodules unless they\ncontain a git directory, 2012-09-26) a case was made that we can safely\nignore ignored untracked files for removal as we explicitely ask for the\nremoval of the submodule.\n\nIn a later patch we want to remove submodules even when the user doesn't\nexplicitly ask for it (e.g. checking out a tree-ish in which the submodule\ndoesn't exist). In that case we want to be more careful when it comes\nto deletion of untracked files. As of this patch it is unclear how this\nwill be implemented exactly, so we'll offer flags in which the caller\ncan specify how the different untracked files ought to be handled.\n\nSigned-off-by: Stefan Beller \n---\n builtin/rm.c | 3 ++-\n submodule.c | 23 +++++++++++++++++++----\n submodule.h | 5 ++++-\n 3 files changed, 25 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/rm.c b/builtin/rm.c\nindex 3f3e24eb36..fdd7183f61 100644\n--- a/builtin/rm.c\n+++ b/builtin/rm.c\n@@ -187,7 +187,8 @@ static int check_local_mod(struct object_id *head, int index_only)\n \t\t */\n \t\tif (ce_match_stat(ce, &st, 0) ||\n \t\t (S_ISGITLINK(ce->ce_mode) &&\n-\t\t !ok_to_remove_submodule(ce->name)))\n+\t\t !ok_to_remove_submodule(ce->name,\n+\t\t\t\tSUBMODULE_REMOVAL_IGNORE_IGNORED_UNTRACKED)))\n \t\t\tlocal_changes = 1;\n \n \t\t/*\ndiff --git a/submodule.c b/submodule.c\nindex 9f0b544ebe..2d13744b06 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1019,7 +1019,7 @@ int submodule_uses_gitfile(const char *path)\n \treturn 1;\n }\n \n-int ok_to_remove_submodule(const char *path)\n+int ok_to_remove_submodule(const char *path, unsigned flags)\n {\n \tssize_t len;\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n@@ -1032,15 +1032,27 @@ int ok_to_remove_submodule(const char *path)\n \tif (!submodule_uses_gitfile(path))\n \t\treturn 0;\n \n-\targv_array_pushl(&cp.args, \"status\", \"--porcelain\", \"-u\",\n+\targv_array_pushl(&cp.args, \"status\", \"--porcelain\",\n \t\t\t\t \"--ignore-submodules=none\", NULL);\n+\n+\tif (flags & SUBMODULE_REMOVAL_IGNORE_UNTRACKED)\n+\t\targv_array_push(&cp.args, \"-uno\");\n+\telse\n+\t\targv_array_push(&cp.args, \"-uall\");\n+\n+\tif (!(flags & SUBMODULE_REMOVAL_IGNORE_IGNORED_UNTRACKED))\n+\t\targv_array_push(&cp.args, \"--ignored\");\n+\n \tprepare_submodule_repo_env(&cp.env_array);\n \tcp.git_cmd = 1;\n \tcp.no_stdin = 1;\n \tcp.out = -1;\n \tcp.dir = path;\n \tif (start_command(&cp))\n-\t\tdie(_(\"could not run 'git status --porcelain -u --ignore-submodules=none' in submodule %s\"), path);\n+\t\tdie(_(\"could not run 'git status --porcelain --ignore-submodules=none %s %s' in submodule '%s'\"),\n+\t\t\t(flags & SUBMODULE_REMOVAL_IGNORE_UNTRACKED) ? \"-uno\" : \"-uall\",\n+\t\t\t(!(flags & SUBMODULE_REMOVAL_IGNORE_IGNORED_UNTRACKED)) ? \"--ignored\" : \"\",\n+\t\t\tpath);\n \n \tlen = strbuf_read(&buf, cp.out, 1024);\n \tif (len > 2)\n@@ -1048,7 +1060,10 @@ int ok_to_remove_submodule(const char *path)\n \tclose(cp.out);\n \n \tif (finish_command(&cp))\n-\t\tdie(_(\"'git status --porcelain -u --ignore-submodules=none' failed in submodule %s\"), path);\n+\t\tdie(_(\"'git status --porcelain --ignore-submodules=none %s %s' failed in submodule '%s'\"),\n+\t\t\t(flags & SUBMODULE_REMOVAL_IGNORE_UNTRACKED) ? \"-uno\" : \"-uall\",\n+\t\t\t(!(flags & SUBMODULE_REMOVAL_IGNORE_IGNORED_UNTRACKED)) ? \"--ignored\" : \"\",\n+\t\t\tpath);\n \n \tstrbuf_release(&buf);\n \treturn ok_to_remove;\ndiff --git a/submodule.h b/submodule.h\nindex 61fb610749..3ed3aa479a 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -59,7 +59,10 @@ extern int fetch_populated_submodules(const struct argv_array *options,\n \t\t\t int quiet, int max_parallel_jobs);\n extern unsigned is_submodule_modified(const char *path, int ignore_untracked);\n extern int submodule_uses_gitfile(const char *path);\n-extern int ok_to_remove_submodule(const char *path);\n+\n+#define SUBMODULE_REMOVAL_IGNORE_UNTRACKED (1<<0)\n+#define SUBMODULE_REMOVAL_IGNORE_IGNORED_UNTRACKED (1<<1)\n+extern int ok_to_remove_submodule(const char *path, unsigned flags);\n extern int merge_submodule(unsigned char result[20], const char *path,\n \t\t\t const unsigned char base[20],\n \t\t\t const unsigned char a[20],\n-- \n2.11.0.rc2.35.g26e18c9\n\n"},{"id":"307796","messageId":"xmqqfulqxsml.fsf@gitster.mtv.corp.google.com","threadId":"44694","inReplyTo":"20161213205622.841-3-sbeller@google.com","subject":"Re: [PATCHv2 2/5] submodule: modernize ok_to_remove_submodule to use argv_array","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-14T18:39:14Z","receivedAt":"2016-12-14T18:40:50Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Instead of constructing the NULL terminated array ourselves, we\n> should make use of the argv_array infrastructure.\n>\n> While at it, adapt the error messages to reflect the actual invocation.\n>\n> Signed-off-by: Stefan Beller \n> ---\n> submodule.c | 14 ++++----------\n> 1 file changed, 4 insertions(+), 10 deletions(-)\n\nLooks good.\n\n>\n> diff --git a/submodule.c b/submodule.c\n> index 45ccfb7ab4..9f0b544ebe 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -1023,13 +1023,6 @@ int ok_to_remove_submodule(const char *path)\n> {\n> \tssize_t len;\n> \tstruct child_process cp = CHILD_PROCESS_INIT;\n> -\tconst char *argv[] = {\n> -\t\t\"status\",\n> -\t\t\"--porcelain\",\n> -\t\t\"-u\",\n> -\t\t\"--ignore-submodules=none\",\n> -\t\tNULL,\n> -\t};\n> \tstruct strbuf buf = STRBUF_INIT;\n> \tint ok_to_remove = 1;\n> \n> @@ -1039,14 +1032,15 @@ int ok_to_remove_submodule(const char *path)\n> \tif (!submodule_uses_gitfile(path))\n> \t\treturn 0;\n> \n> -\tcp.argv = argv;\n> +\targv_array_pushl(&cp.args, \"status\", \"--porcelain\", \"-u\",\n> +\t\t\t\t \"--ignore-submodules=none\", NULL);\n> \tprepare_submodule_repo_env(&cp.env_array);\n> \tcp.git_cmd = 1;\n> \tcp.no_stdin = 1;\n> \tcp.out = -1;\n> \tcp.dir = path;\n> \tif (start_command(&cp))\n> -\t\tdie(\"Could not run 'git status --porcelain -uall --ignore-submodules=none' in submodule %s\", path);\n> +\t\tdie(_(\"could not run 'git status --porcelain -u --ignore-submodules=none' in submodule %s\"), path);\n> \n> \tlen = strbuf_read(&buf, cp.out, 1024);\n> \tif (len > 2)\n> @@ -1054,7 +1048,7 @@ int ok_to_remove_submodule(const char *path)\n> \tclose(cp.out);\n> \n> \tif (finish_command(&cp))\n> -\t\tdie(\"'git status --porcelain -uall --ignore-submodules=none' failed in submodule %s\", path);\n> +\t\tdie(_(\"'git status --porcelain -u --ignore-submodules=none' failed in submodule %s\"), path);\n> \n> \tstrbuf_release(&buf);\n> \treturn ok_to_remove;\n"},{"id":"307797","messageId":"xmqqbmwexs9x.fsf@gitster.mtv.corp.google.com","threadId":"44694","inReplyTo":"20161213205622.841-5-sbeller@google.com","subject":"Re: [PATCHv2 4/5] ok_to_remove_submodule: absorb the submodule git dir","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-14T18:46:50Z","receivedAt":"2016-12-14T18:48:27Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> It is a major reason to say no, when deciding if a submodule can be\n> deleted, if the git directory of the submodule being contained in the\n> submodule's working directory.\n>\n> Migrate the git directory into the superproject instead of failing,\n> and proceed with the other checks.\n>\n> Signed-off-by: Stefan Beller \n> ---\n> submodule.c | 15 +++++++++++++--\n> 1 file changed, 13 insertions(+), 2 deletions(-)\n>\n> diff --git a/submodule.c b/submodule.c\n> index 2d13744b06..e42efa2337 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -1026,11 +1026,22 @@ int ok_to_remove_submodule(const char *path, unsigned flags)\n> \tstruct strbuf buf = STRBUF_INIT;\n> \tint ok_to_remove = 1;\n> \n> +\t/* Is it there? */\n> \tif (!file_exists(path) || is_empty_dir(path))\n> \t\treturn 1;\n> \n> -\tif (!submodule_uses_gitfile(path))\n> -\t\treturn 0;\n> +\t/* Does it have a .git directory? */\n> +\tif (!submodule_uses_gitfile(path)) {\n> +\t\tabsorb_git_dir_into_superproject(\"\", path,\n> +\t\t\tABSORB_GITDIR_RECURSE_SUBMODULES);\n> +\n> +\t\t/*\n> +\t\t * We should be using a gitfile by now. Let's double\n> +\t\t * check as losing the git dir would be fatal.\n> +\t\t */\n> +\t\tif (!submodule_uses_gitfile(path))\n> +\t\t\treturn 0;\n> +\t}\n\nIt feels a bit funny for a function that answers yes/no question to\nactually have a side effect, but probably it is OK. It feels dirty,\nbut I dunno.\n\nA brief reading of the above makes us wonder what should happen if\nthe absorb_git_dir_into_superproject() call fails, but a separate\n\"submodule_uses_gitfile()\" is needed to see if it failed? Perhaps\nthe function needs to tell the caller if it succeeded?\n\n> \n> \targv_array_pushl(&cp.args, \"status\", \"--porcelain\",\n> \t\t\t\t \"--ignore-submodules=none\", NULL);\n"},{"id":"307798","messageId":"CAGZ79kZ=PqTKt5xDobnR5oqwz84i10ZobmQb6QqeA3aSkbrd8Q@mail.gmail.com","threadId":"44694","inReplyTo":"xmqqbmwexs9x.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv2 4/5] ok_to_remove_submodule: absorb the submodule git dir","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-14T18:52:07Z","receivedAt":"2016-12-14T18:52:14Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Dec 14, 2016 at 10:46 AM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> It is a major reason to say no, when deciding if a submodule can be\n>> deleted, if the git directory of the submodule being contained in the\n>> submodule's working directory.\n>>\n>> Migrate the git directory into the superproject instead of failing,\n>> and proceed with the other checks.\n>>\n>> Signed-off-by: Stefan Beller \n>> ---\n>> submodule.c | 15 +++++++++++++--\n>> 1 file changed, 13 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/submodule.c b/submodule.c\n>> index 2d13744b06..e42efa2337 100644\n>> --- a/submodule.c\n>> +++ b/submodule.c\n>> @@ -1026,11 +1026,22 @@ int ok_to_remove_submodule(const char *path, unsigned flags)\n>> struct strbuf buf = STRBUF_INIT;\n>> int ok_to_remove = 1;\n>>\n>> + /* Is it there? */\n>> if (!file_exists(path) || is_empty_dir(path))\n>> return 1;\n>>\n>> - if (!submodule_uses_gitfile(path))\n>> - return 0;\n>> + /* Does it have a .git directory? */\n>> + if (!submodule_uses_gitfile(path)) {\n>> + absorb_git_dir_into_superproject(\"\", path,\n>> + ABSORB_GITDIR_RECURSE_SUBMODULES);\n>> +\n>> + /*\n>> + * We should be using a gitfile by now. Let's double\n>> + * check as losing the git dir would be fatal.\n>> + */\n>> + if (!submodule_uses_gitfile(path))\n>> + return 0;\n>> + }\n>\n> It feels a bit funny for a function that answers yes/no question to\n> actually have a side effect, but probably it is OK. It feels dirty,\n> but I dunno.\n\nAnother approach would be to return more than yes/no but the reason\nwhy it is a no. And then the caller would call absorb_git_dir_into_superproject.\n\n> A brief reading of the above makes us wonder what should happen if\n> the absorb_git_dir_into_superproject() call fails, but a separate\n> \"submodule_uses_gitfile()\" is needed to see if it failed? Perhaps\n> the function needs to tell the caller if it succeeded?\n\nI don't know about the double check as I think we should really be sure before\ndeleting that repo, but absorb_git_dir_into_superproject would fail/die\nif it would not actually absorb it, so maybe it is too much caution.\n\nSo I'd omit the double check in a reroll.\n\n>\n>>\n>> argv_array_pushl(&cp.args, \"status\", \"--porcelain\",\n>> \"--ignore-submodules=none\", NULL);\n"},{"id":"307799","messageId":"xmqq7f72xrvw.fsf@gitster.mtv.corp.google.com","threadId":"44694","inReplyTo":"20161213205622.841-6-sbeller@google.com","subject":"Re: [PATCHv2 5/5] rm: add absorb a submodules git dir before deletion","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-14T18:55:15Z","receivedAt":"2016-12-14T19:01:53Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> diff --git a/builtin/rm.c b/builtin/rm.c\n> index fdd7183f61..8c9c535a88 100644\n> --- a/builtin/rm.c\n> +++ b/builtin/rm.c\n> @@ -392,28 +392,14 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\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\tif (is_empty_dir(path) && rmdir(path))\n> +\t\t\t\t\tdie_errno(\"git rm: '%s'\", path);\n> +\t\t\t\tif (file_exists(path))\n> +\t\t\t\t\tdepopulate_submodule(path);\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\nThe updated code structure is somewhat nicer for understanding the\nflow than the original, but it can further be improved.\n\nA step \"If empty directory, we try to rmdir and we check its return\nstatus and die ourselves if we couldn't remove it\" reads very\nsensible, but coming immediately after that, \"if it still exists,\ncall depop()\" looks a bit strange. I would have expected a similar\nconstruct, i.e.\n\n\tif (directory_exists(path) && depop_submodlue(path))\n\t\tdie(\"git rm: '%s' submodule still populated\", path);\n\nthere. Also,\n\n\tif (is_empty_dir(path)) {\n\t\tif (rmdir(path))\n\t\t\tdie_errno(...);\n\t} else if (is_nonempty_dir(path)) {\n\t\tif (depop_subm(path))\n \tdie(...);\n\t}\n\nwould have clarified the structure even further.\n\nYes, I know that you made depop_subm() to die on error, and the\nabove is questioning if that is a sensible design choice.\n"}]}