{"thread":{"id":"44739","subject":"[PATCHv4 0/5] git-rm absorbs submodule git directory before deletion","startedAt":"2016-12-19T23:29:45Z","lastAt":"2016-12-20T22:08:08Z","messageCount":16,"participants":["Stefan Beller","Brandon Williams","David Turner","Johannes Sixt"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"308114","messageId":"20161219232828.5075-1-sbeller@google.com","threadId":"44739","inReplyTo":null,"subject":"[PATCHv4 0/5] git-rm absorbs submodule git directory before deletion","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-19T23:28:23Z","receivedAt":"2016-12-19T23:29:45Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"v4:\n* reworded commit messages of the last 2 patches\n* introduced a new patch introducing {run,start,finish}_command_or_die\n* found an existing function in dir.h to use to remove a directory\n  which deals gracefully with the cornercases, that Junio pointed out.\n\nv3:\n* removed the patch to enhance ok_to_remove_submodule to absorb the submodule\n  if needed\n* Removed all the error reporting from git-rm that was related to submodule\n  git directories not absorbed.\n* instead just absorb the git repositories or let the absorb function die\n  with an appropriate error message.\n\nv2:\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  run-command: add {run,start,finish}_command_or_die\n  submodule: add flags to ok_to_remove_submodule\n  rm: absorb a submodules git dir before deletion\n\n builtin/rm.c  | 82 +++++++++++++++--------------------------------------------\n run-command.c | 28 ++++++++++++++++++++\n run-command.h |  4 +++\n submodule.c   | 27 ++++++++++----------\n submodule.h   | 58 ++++++++++++++++++++++++------------------\n t/t3600-rm.sh | 39 +++++++++++-----------------\n 6 files changed, 114 insertions(+), 124 deletions(-)\n\n-- \n2.11.0.rc2.53.gb7b3fba.dirty\n\n"},{"id":"308115","messageId":"20161219232828.5075-2-sbeller@google.com","threadId":"44739","inReplyTo":"20161219232828.5075-1-sbeller@google.com","subject":"[PATCHv4 1/5] submodule.h: add extern keyword to functions","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-19T23:28:24Z","receivedAt":"2016-12-19T23:29:48Z","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 <sbeller@google.com>\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.53.gb7b3fba.dirty\n\n"},{"id":"308116","messageId":"20161219232828.5075-4-sbeller@google.com","threadId":"44739","inReplyTo":"20161219232828.5075-1-sbeller@google.com","subject":"[PATCHv4 3/5] run-command: add {run,start,finish}_command_or_die","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-19T23:28:26Z","receivedAt":"2016-12-19T23:30:45Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In a later patch we want to report the exact command that we run in the\nerror message. Add a convenient way to the run command API that runs the\ngiven command or reports the exact command as failure.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n run-command.c | 28 ++++++++++++++++++++++++++++\n run-command.h |  4 ++++\n 2 files changed, 32 insertions(+)\n\ndiff --git a/run-command.c b/run-command.c\nindex ca905a9e80..a0587db334 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -279,6 +279,17 @@ static int wait_or_whine(pid_t pid, const char *argv0, int in_signal)\n \treturn code;\n }\n \n+static void report_and_die(struct child_process *cmd, const char *action)\n+{\n+\tint i;\n+\tstruct strbuf err = STRBUF_INIT;\n+\tif (cmd->git_cmd)\n+\t\tstrbuf_addstr(&err, \"git \");\n+\tfor (i = 0; cmd->argv[i]; )\n+\t\tstrbuf_addf(&err, \"'%s'\", cmd->argv[i]);\n+\tdie(_(\"could not %s %s\"), action, err.buf);\n+}\n+\n int start_command(struct child_process *cmd)\n {\n \tint need_in, need_out, need_err;\n@@ -546,6 +557,12 @@ int start_command(struct child_process *cmd)\n \treturn 0;\n }\n \n+void start_command_or_die(struct child_process *cmd)\n+{\n+\tif (start_command(cmd))\n+\t\treport_and_die(cmd, \"start\");\n+}\n+\n int finish_command(struct child_process *cmd)\n {\n \tint ret = wait_or_whine(cmd->pid, cmd->argv[0], 0);\n@@ -558,6 +575,11 @@ int finish_command_in_signal(struct child_process *cmd)\n \treturn wait_or_whine(cmd->pid, cmd->argv[0], 1);\n }\n \n+void finish_command_or_die(struct child_process *cmd)\n+{\n+\tif (finish_command(cmd))\n+\t\treport_and_die(cmd, \"finish\");\n+}\n \n int run_command(struct child_process *cmd)\n {\n@@ -592,6 +614,12 @@ int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir, const\n \treturn run_command(&cmd);\n }\n \n+void run_command_or_die(struct child_process *cmd)\n+{\n+\tif (finish_command(cmd))\n+\t\treport_and_die(cmd, \"run\");\n+}\n+\n #ifndef NO_PTHREADS\n static pthread_t main_thread;\n static int main_thread_set;\ndiff --git a/run-command.h b/run-command.h\nindex dd1c78c28d..e4585885c5 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -56,6 +56,10 @@ int finish_command(struct child_process *);\n int finish_command_in_signal(struct child_process *);\n int run_command(struct child_process *);\n \n+void start_command_or_die(struct child_process *);\n+void finish_command_or_die(struct child_process *);\n+void run_command_or_die(struct child_process *);\n+\n /*\n  * Returns the path to the hook file, or NULL if the hook is missing\n  * or disabled. Note that this points to static storage that will be\n-- \n2.11.0.rc2.53.gb7b3fba.dirty\n\n"},{"id":"308117","messageId":"20161219232828.5075-6-sbeller@google.com","threadId":"44739","inReplyTo":"20161219232828.5075-1-sbeller@google.com","subject":"[PATCHv4 5/5] rm: absorb a submodules git dir before deletion","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-19T23:28:28Z","receivedAt":"2016-12-19T23:30:49Z","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 builtin/rm.c  | 79 ++++++++++++++---------------------------------------------\n t/t3600-rm.sh | 39 ++++++++++++-----------------\n 2 files changed, 33 insertions(+), 85 deletions(-)\n\ndiff --git a/builtin/rm.c b/builtin/rm.c\nindex fdd7183f61..ed758f7144 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@@ -218,13 +196,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@@ -246,8 +219,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@@ -341,6 +312,8 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n \t\t\texit(0);\n \t}\n \n+\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@@ -357,9 +330,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@@ -388,32 +358,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@@ -422,7 +380,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.53.gb7b3fba.dirty\n\n"},{"id":"308118","messageId":"20161219232828.5075-3-sbeller@google.com","threadId":"44739","inReplyTo":"20161219232828.5075-1-sbeller@google.com","subject":"[PATCHv4 2/5] submodule: modernize ok_to_remove_submodule to use argv_array","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-19T23:28:25Z","receivedAt":"2016-12-19T23:32:03Z","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 <sbeller@google.com>\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.53.gb7b3fba.dirty\n\n"},{"id":"308119","messageId":"20161219232828.5075-5-sbeller@google.com","threadId":"44739","inReplyTo":"20161219232828.5075-1-sbeller@google.com","subject":"[PATCHv4 4/5] submodule: add flags to ok_to_remove_submodule","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-19T23:28:27Z","receivedAt":"2016-12-19T23:32:08Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In different contexts the question \"Is it ok to delete a submodule?\"\nmay 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 <sbeller@google.com>\n---\n builtin/rm.c |  3 ++-\n submodule.c  | 19 +++++++++++++------\n submodule.h  |  5 ++++-\n 3 files changed, 19 insertions(+), 8 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..9aecb8930c 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,23 +1032,30 @@ 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+\tstart_command_or_die(&cp);\n \n \tlen = strbuf_read(&buf, cp.out, 1024);\n \tif (len > 2)\n \t\tok_to_remove = 0;\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+\tfinish_command_or_die(&cp);\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.53.gb7b3fba.dirty\n\n"},{"id":"308120","messageId":"20161219233256.GB41080@google.com","threadId":"44739","inReplyTo":"20161219232828.5075-4-sbeller@google.com","subject":"Re: [PATCHv4 3/5] run-command: add {run,start,finish}_command_or_die","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-12-19T23:32:56Z","receivedAt":"2016-12-19T23:34:05Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 12/19, Stefan Beller wrote:\n> In a later patch we want to report the exact command that we run in the\n> error message. Add a convenient way to the run command API that runs the\n> given command or reports the exact command as failure.\n> \n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>  run-command.c | 28 ++++++++++++++++++++++++++++\n>  run-command.h |  4 ++++\n>  2 files changed, 32 insertions(+)\n> \n> diff --git a/run-command.c b/run-command.c\n> index ca905a9e80..a0587db334 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -279,6 +279,17 @@ static int wait_or_whine(pid_t pid, const char *argv0, int in_signal)\n>  \treturn code;\n>  }\n>  \n> +static void report_and_die(struct child_process *cmd, const char *action)\n> +{\n> +\tint i;\n> +\tstruct strbuf err = STRBUF_INIT;\n> +\tif (cmd->git_cmd)\n> +\t\tstrbuf_addstr(&err, \"git \");\n> +\tfor (i = 0; cmd->argv[i]; )\n\nMissing the increment of i here.\n\n> +\t\tstrbuf_addf(&err, \"'%s'\", cmd->argv[i]);\n> +\tdie(_(\"could not %s %s\"), action, err.buf);\n> +}\n> +\n>  int start_command(struct child_process *cmd)\n>  {\n>  \tint need_in, need_out, need_err;\n> @@ -546,6 +557,12 @@ int start_command(struct child_process *cmd)\n>  \treturn 0;\n>  }\n>  \n> +void start_command_or_die(struct child_process *cmd)\n> +{\n> +\tif (start_command(cmd))\n> +\t\treport_and_die(cmd, \"start\");\n> +}\n> +\n>  int finish_command(struct child_process *cmd)\n>  {\n>  \tint ret = wait_or_whine(cmd->pid, cmd->argv[0], 0);\n> @@ -558,6 +575,11 @@ int finish_command_in_signal(struct child_process *cmd)\n>  \treturn wait_or_whine(cmd->pid, cmd->argv[0], 1);\n>  }\n>  \n> +void finish_command_or_die(struct child_process *cmd)\n> +{\n> +\tif (finish_command(cmd))\n> +\t\treport_and_die(cmd, \"finish\");\n> +}\n>  \n>  int run_command(struct child_process *cmd)\n>  {\n> @@ -592,6 +614,12 @@ int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir, const\n>  \treturn run_command(&cmd);\n>  }\n>  \n> +void run_command_or_die(struct child_process *cmd)\n> +{\n> +\tif (finish_command(cmd))\n> +\t\treport_and_die(cmd, \"run\");\n> +}\n> +\n>  #ifndef NO_PTHREADS\n>  static pthread_t main_thread;\n>  static int main_thread_set;\n> diff --git a/run-command.h b/run-command.h\n> index dd1c78c28d..e4585885c5 100644\n> --- a/run-command.h\n> +++ b/run-command.h\n> @@ -56,6 +56,10 @@ int finish_command(struct child_process *);\n>  int finish_command_in_signal(struct child_process *);\n>  int run_command(struct child_process *);\n>  \n> +void start_command_or_die(struct child_process *);\n> +void finish_command_or_die(struct child_process *);\n> +void run_command_or_die(struct child_process *);\n> +\n>  /*\n>   * Returns the path to the hook file, or NULL if the hook is missing\n>   * or disabled. Note that this points to static storage that will be\n> -- \n> 2.11.0.rc2.53.gb7b3fba.dirty\n> \n\n-- \nBrandon Williams\n"},{"id":"308122","messageId":"CAGZ79kY7XpM2zi8CWjZ5d=yBJi1JGz7XkY=d=v+3rVPkF-N4mA@mail.gmail.com","threadId":"44739","inReplyTo":"20161219233256.GB41080@google.com","subject":"Re: [PATCHv4 3/5] run-command: add {run,start,finish}_command_or_die","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-19T23:35:43Z","receivedAt":"2016-12-19T23:37:33Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Dec 19, 2016 at 3:32 PM, Brandon Williams <bmwill@google.com> wrote:\n\n>> +             strbuf_addstr(&err, \"git \");\n>> +     for (i = 0; cmd->argv[i]; )\n>\n> Missing the increment of i here.\n\nDoh :(\n\nWhen writing the code I was undecided between using\nan incremental loop \"for (i = 0; cmd->argv[i]; i++)\" and\na pointer arithmetic thing as \"for(;;) {... cmd->argv++;\",\nand ended up with a broken middle ground.\n"},{"id":"308123","messageId":"a756fb33dbdb4a81a46e40c5e0aaef2f@exmbdft7.ad.twosigma.com","threadId":"44739","inReplyTo":"20161219232828.5075-1-sbeller@google.com","subject":"RE: [PATCHv4 0/5] git-rm absorbs submodule git directory before deletion","fromName":"David Turner","fromEmail":"david.turner@twosigma.com","sentAt":"2016-12-19T23:35:06Z","receivedAt":"2016-12-19T23:43:29Z","isPatch":false,"sender":{"key":"david.turner@twosigma.com","avatar":null},"body":"Other than Brandon's issue on 3/5 (which I noticed myself but wanted to double-check that my mailer wasn't playing tricks on me and so lost the race to report), this version looks good.\n\n> -----Original Message-----\n> From: Stefan Beller [mailto:sbeller@google.com]\n> Sent: Monday, December 19, 2016 6:28 PM\n> To: gitster@pobox.com\n> Cc: git@vger.kernel.org; bmwill@google.com; sandals@crustytoothpaste.net;\n> David Turner; Stefan Beller\n> Subject: [PATCHv4 0/5] git-rm absorbs submodule git directory before\n> deletion\n> \n> v4:\n> * reworded commit messages of the last 2 patches\n> * introduced a new patch introducing {run,start,finish}_command_or_die\n> * found an existing function in dir.h to use to remove a directory\n>   which deals gracefully with the cornercases, that Junio pointed out.\n> \n> v3:\n> * removed the patch to enhance ok_to_remove_submodule to absorb the\n> submodule\n>   if needed\n> * Removed all the error reporting from git-rm that was related to\n> submodule\n>   git directories not absorbed.\n> * instead just absorb the git repositories or let the absorb function die\n>   with an appropriate error message.\n> \n> 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\"\n> >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\n> git dir\"\n>   (David Turner offered me some advice on how to write better English off\n> 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> \n> v1:\n> The \"checkout --recurse-submodules\" series got too large to comfortably\n> send it out for review, so I had to break it up into smaller series'; this\n> is the first subseries, but it makes sense on its own.\n> \n> This series teaches git-rm to absorb the git directory of a submodule\n> instead of failing and complaining about the git directory preventing\n> deletion.\n> \n> It applies on origin/sb/submodule-embed-gitdir.\n> \n> Any feedback welcome!\n> \n> Thanks,\n> Stefan\n> \n> Stefan Beller (5):\n>   submodule.h: add extern keyword to functions\n>   submodule: modernize ok_to_remove_submodule to use argv_array\n>   run-command: add {run,start,finish}_command_or_die\n>   submodule: add flags to ok_to_remove_submodule\n>   rm: absorb a submodules git dir before deletion\n> \n>  builtin/rm.c  | 82 +++++++++++++++---------------------------------------\n> -----\n>  run-command.c | 28 ++++++++++++++++++++  run-command.h |  4 +++\n>  submodule.c   | 27 ++++++++++----------\n>  submodule.h   | 58 ++++++++++++++++++++++++------------------\n>  t/t3600-rm.sh | 39 +++++++++++-----------------\n>  6 files changed, 114 insertions(+), 124 deletions(-)\n> \n> --\n> 2.11.0.rc2.53.gb7b3fba.dirty\n\n"},{"id":"308158","messageId":"aad0af97-7588-632d-a113-5d8372b8b7a8@kdbg.org","threadId":"44739","inReplyTo":"20161219232828.5075-4-sbeller@google.com","subject":"Re: [PATCHv4 3/5] run-command: add {run,start,finish}_command_or_die","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-12-20T18:33:59Z","receivedAt":"2016-12-20T18:34:26Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 20.12.2016 um 00:28 schrieb Stefan Beller:\n> +static void report_and_die(struct child_process *cmd, const char *action)\n> +{\n> +\tint i;\n> +\tstruct strbuf err = STRBUF_INIT;\n> +\tif (cmd->git_cmd)\n> +\t\tstrbuf_addstr(&err, \"git \");\n> +\tfor (i = 0; cmd->argv[i]; )\n> +\t\tstrbuf_addf(&err, \"'%s'\", cmd->argv[i]);\n\nTake note that cmd is accessed here.\n\n> +\tdie(_(\"could not %s %s\"), action, err.buf);\n\nShould lego sentences not be avoided? They are not exactly translator \nfriendly.\n\nGiven that a lot of effort is spent elsewhere to actually *avoid* dying \nin library code, this new die() is not very welcome, I must say. \nGranted, you just add convenience functions here, and callers have \nalternatives that do not die, but still...\n\n> +}\n> +\n>  int start_command(struct child_process *cmd)\n>  {\n>  \tint need_in, need_out, need_err;\n> @@ -546,6 +557,12 @@ int start_command(struct child_process *cmd)\n>  \treturn 0;\n>  }\n>\n> +void start_command_or_die(struct child_process *cmd)\n> +{\n> +\tif (start_command(cmd))\n> +\t\treport_and_die(cmd, \"start\");\n\nBut cmd has been cleaned up at this point of call of report_and_die. The \naccess noted above goes to freed memory, I think.\n\n> +}\n> ...\n> +void finish_command_or_die(struct child_process *cmd)\n> +{\n> +\tif (finish_command(cmd))\n> +\t\treport_and_die(cmd, \"finish\");\n\nSame here.\n\n> +}\n> ...\n> +void run_command_or_die(struct child_process *cmd)\n> +{\n> +\tif (finish_command(cmd))\n> +\t\treport_and_die(cmd, \"run\");\n\nAnd here as well.\n\n> +}\n\n-- Hannes\n\n"},{"id":"308161","messageId":"8a75de4b-76aa-c111-6946-91a709d890f7@kdbg.org","threadId":"44739","inReplyTo":"aad0af97-7588-632d-a113-5d8372b8b7a8@kdbg.org","subject":"Re: [PATCHv4 3/5] run-command: add {run,start,finish}_command_or_die","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-12-20T18:54:57Z","receivedAt":"2016-12-20T18:55:03Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 20.12.2016 um 19:33 schrieb Johannes Sixt:\n> Am 20.12.2016 um 00:28 schrieb Stefan Beller:\n>> +void run_command_or_die(struct child_process *cmd)\n>> +{\n>> +    if (finish_command(cmd))\n>> +        report_and_die(cmd, \"run\");\n>\n> And here as well.\n\nOh, and BTW, this one wraps only finish_command, not run_command.\n\n-- Hannes\n\n"},{"id":"308163","messageId":"CAGZ79kYNKWfnEXWJfyRUutFyaQiRD9qW--LkK4Nbwdf7FtdPQA@mail.gmail.com","threadId":"44739","inReplyTo":"aad0af97-7588-632d-a113-5d8372b8b7a8@kdbg.org","subject":"Re: [PATCHv4 3/5] run-command: add {run,start,finish}_command_or_die","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-20T19:23:31Z","receivedAt":"2016-12-20T19:24:23Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Dec 20, 2016 at 10:33 AM, Johannes Sixt <j6t@kdbg.org> wrote:\n> Am 20.12.2016 um 00:28 schrieb Stefan Beller:\n>>\n>> +static void report_and_die(struct child_process *cmd, const char *action)\n>> +{\n>> +       int i;\n>> +       struct strbuf err = STRBUF_INIT;\n>> +       if (cmd->git_cmd)\n>> +               strbuf_addstr(&err, \"git \");\n>> +       for (i = 0; cmd->argv[i]; )\n>> +               strbuf_addf(&err, \"'%s'\", cmd->argv[i]);\n>\n>\n> Take note that cmd is accessed here.\n>\n>> +       die(_(\"could not %s %s\"), action, err.buf);\n>\n>\n> Should lego sentences not be avoided? They are not exactly translator\n> friendly.\n>\n> Given that a lot of effort is spent elsewhere to actually *avoid* dying in\n> library code, this new die() is not very welcome, I must say.\n\nI agree on the sentiment. In a reroll I'll drop this patch and instead introduce\na function `char *get_child_command_line(struct child_process*);`, which\na caller can call before calling finish_command and then use the\nresulting string\nto assemble an error message without lego.\n\nThanks for the thorough review!\nStefan\n"},{"id":"308167","messageId":"f14ee492-8297-c8ec-f80f-f8f24caf91e1@kdbg.org","threadId":"44739","inReplyTo":"CAGZ79kYNKWfnEXWJfyRUutFyaQiRD9qW--LkK4Nbwdf7FtdPQA@mail.gmail.com","subject":"Re: [PATCHv4 3/5] run-command: add {run,start,finish}_command_or_die","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-12-20T20:12:27Z","receivedAt":"2016-12-20T20:13:02Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 20.12.2016 um 20:23 schrieb Stefan Beller:\n> In a reroll I'll drop this patch and instead introduce\n> a function `char *get_child_command_line(struct child_process*);`, which\n> a caller can call before calling finish_command and then use the\n> resulting string\n> to assemble an error message without lego.\n\nThat sounds a lot better, thank you. Note though, that the function \nwould have to be called before start_command(), because when it fails, \nit would be too late.\n\nI have to ask, though: Is it so much necessary to report the exact \ncommand line in an error message? If someone is interested in which \ncommand failed, it would be \"sufficient\" to run the command under \nGIT_TRACE=2.\n\n-- Hannes\n\n"},{"id":"308169","messageId":"CAGZ79kYG-veuWNFh6G1g5MiQHBGk2i1qbH_NBtnMS6jFcoWocg@mail.gmail.com","threadId":"44739","inReplyTo":"f14ee492-8297-c8ec-f80f-f8f24caf91e1@kdbg.org","subject":"Re: [PATCHv4 3/5] run-command: add {run,start,finish}_command_or_die","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-20T20:49:13Z","receivedAt":"2016-12-20T20:49:19Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Dec 20, 2016 at 12:12 PM, Johannes Sixt <j6t@kdbg.org> wrote:\n> Am 20.12.2016 um 20:23 schrieb Stefan Beller:\n>>\n>> In a reroll I'll drop this patch and instead introduce\n>> a function `char *get_child_command_line(struct child_process*);`, which\n>> a caller can call before calling finish_command and then use the\n>> resulting string\n>> to assemble an error message without lego.\n>\n>\n> That sounds a lot better, thank you. Note though, that the function would\n> have to be called before start_command(), because when it fails, it would be\n> too late.\n\nYes, the pattern to use it would be\n\n    // first assemble the child process struct with conditions\n\n    char *cmdline = get_child_command_line(&child)\n\n    if (start_command(&child))\n        // use cmdline here for error reporting.\n\n\n>\n> I have to ask, though: Is it so much necessary to report the exact command\n> line in an error message?\n\nHave a look at https://github.com/git/git/blob/master/submodule.c#L1122\n\n    die(\"Could not run 'git status --porcelain -uall \\\n        --ignore-submodules=none' in submodule %s\", path);\n\nThere are 2 things:\n1) The error message is not very informative, as your question hints at.\n    I consider changing it to add more meaning. I think the end user\n    would also be interested in \"Why did we run this command?\".\n2) We really want to report the correct command line here.\n    Currently that is the case, but if you look at the prior patch that extends\n    the arguments conditionally (and then also uses the same condition\n    to format the error message... that hints at hard to maintain error\n    messages.)\n\nSo the new proposed function really only addresses 2) here.\n\n> If someone is interested in which command failed,\n> it would be \"sufficient\" to run the command under GIT_TRACE=2.\n>\n\nYes, while at it, I may just move up the error reporting to the builtin\ncommand giving a higher level message.\n\nThanks,\nStefan\n"},{"id":"308173","messageId":"48970168-ba4b-e125-6624-e4c37b8a7180@kdbg.org","threadId":"44739","inReplyTo":"CAGZ79kYG-veuWNFh6G1g5MiQHBGk2i1qbH_NBtnMS6jFcoWocg@mail.gmail.com","subject":"Re: [PATCHv4 3/5] run-command: add {run,start,finish}_command_or_die","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-12-20T21:47:11Z","receivedAt":"2016-12-20T21:47:21Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 20.12.2016 um 21:49 schrieb Stefan Beller:\n> On Tue, Dec 20, 2016 at 12:12 PM, Johannes Sixt <j6t@kdbg.org> wrote:\n>> I have to ask, though: Is it so much necessary to report the exact command\n>> line in an error message?\n>\n> Have a look at https://github.com/git/git/blob/master/submodule.c#L1122\n>\n>     die(\"Could not run 'git status --porcelain -uall \\\n>         --ignore-submodules=none' in submodule %s\", path);\n>\n> There are 2 things:\n> 1) The error message is not very informative, as your question hints at.\n>     I consider changing it to add more meaning. I think the end user\n>     would also be interested in \"Why did we run this command?\".\n\nYou don't have to. start_command() and finish_command() report any \nlow-level errors (exec failed, signal received...). If the exit code of \nthe program is non-zero, finish_command() reports nothing because the \ncommand *itself* will have written some error message (\"working \ndirectory dirty\", \"object database corrupt\", \"xyz: no such file or \ndirectory\"...). At this level, it only makes sense to leave a trace in \nwhich submodule an error occured. So I think it would be sufficient to  just\n\n     die(\"could not run 'git status' in submodule %s\", path);\nor\n     die(\"'git status' in submodule %s failed\", path);\n\n> 2) We really want to report the correct command line here.\n\nWhy? We do not do this anywhere else.\n\n>     Currently that is the case, but if you look at the prior patch that extends\n>     the arguments conditionally (and then also uses the same condition\n>     to format the error message... that hints at hard to maintain error\n>     messages.)\n\nThis does not explain why the *complete and detailed* invocation must be \nreported. I haven't followed this topic at all, so I may be missing some \ncruical detail. (If you say \"it must happen\" one more time, then I will \nbelieve you, because for me that's simpler than to plough through a \nflock of submodule topics. ;-)\n\n-- Hannes\n\n"},{"id":"308175","messageId":"CAGZ79kYgK4e75G9QWUUGKdTUNm+F1GagJ83uXfWY+1ptQQm2dw@mail.gmail.com","threadId":"44739","inReplyTo":"48970168-ba4b-e125-6624-e4c37b8a7180@kdbg.org","subject":"Re: [PATCHv4 3/5] run-command: add {run,start,finish}_command_or_die","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-20T22:07:23Z","receivedAt":"2016-12-20T22:08:08Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Dec 20, 2016 at 1:47 PM, Johannes Sixt <j6t@kdbg.org> wrote:\n\n> This does not explain why the *complete and detailed* invocation must be\n> reported.\n\neh. I tried to say that a report that looks like a *complete and\ndetailed* invocation\nshould be such, and not be misleading (and e.g. miss one out of 5\narguments printed).\n\n I haven't followed this topic at all, so I may be missing some\n> cruical detail. (If you say \"it must happen\" one more time, then I will\n> believe you, because for me that's simpler than to plough through a flock of\n> submodule topics. ;-)\n\nIt doesn't have to happen; I am trying to come up with a better message.\n\n>\n> -- Hannes\n>\n"}]}