{"thread":{"id":"44491","subject":"[RFC PATCH 00/16] Checkout aware of Submodules!","startedAt":"2016-11-15T23:07:01Z","lastAt":"2016-12-03T06:14:00Z","messageCount":50,"participants":["Stefan Beller","David Turner","Brandon Williams","Junio C Hamano","Xiaodong Qi"],"isPatch":true,"patchVersion":1,"patchTotal":16},"messages":[{"id":"305993","messageId":"20161115230651.23953-1-sbeller@google.com","threadId":"44491","inReplyTo":null,"subject":"[RFC PATCH 00/16] Checkout aware of Submodules!","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-11-15T23:06:35Z","receivedAt":"2016-11-15T23:07:01Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"When working with submodules, nearly anytime after checking out \na different state of the projects, that has submodules changed\nyou'd run \"git submodule update\" with a current version of Git.\n\nThere are two problems with this approach:\n\n* The \"submodule update\" command is dangerous as it\n doesn't check for work that may be lost in the submodule\n (e.g. a dangling commit).\n* you may forget to run the command as checkout is supposed\n to do all the work for you.\n\nIntegrate updating the submodules into git checkout, with the same\nsafety promises that git-checkout has, i.e. not throw away data unless\nasked to. This is done by first checking if the submodule is at the same\nsha1 as it is recorded in the superproject. If there are changes we stop\nproceeding the checkout just like it is when checking out a file that\nhas local changes.\n\nThe integration happens in the code that is also used in other commands\nsuch that it will be easier in the future to make other commands aware\nof submodule.\n\nThis also solves d/f conflicts in case you replace a file/directory\nwith a submodule or vice versa.\n\nThe patches are still a bit rough, but the overall series seems\npromising enough to me that I want to put it out here.\n\nAny review, specifically on the design level welcome!\n\nThanks,\nStefan\n\nStefan Beller (16):\n submodule.h: add extern keyword to functions, break line before 80\n submodule: modernize ok_to_remove_submodule to use argv_array\n submodule: use absolute path for computing relative path connecting\n update submodules: add is_submodule_populated\n update submodules: add submodule config parsing\n update submodules: add a config option to determine if submodules are\n updated\n update submodules: introduce submodule_is_interesting\n update submodules: add depopulate_submodule\n update submodules: add scheduling to update submodules\n update submodules: is_submodule_checkout_safe\n teach unpack_trees() to remove submodule contents\n entry: write_entry to write populate submodules\n submodule: teach unpack_trees() to update submodules\n checkout: recurse into submodules if asked to\n completion: add '--recurse-submodules' to checkout\n checkout: add config option to recurse into submodules by default\n\n Documentation/config.txt | 6 +\n Documentation/git-checkout.txt | 8 +\n builtin/checkout.c | 31 ++-\n cache.h | 2 +\n contrib/completion/git-completion.bash | 2 +-\n entry.c | 17 +-\n submodule-config.c | 22 +++\n submodule-config.h | 17 +-\n submodule.c | 246 +++++++++++++++++++++--\n submodule.h | 77 +++++---\n t/lib-submodule-update.sh | 10 +-\n t/t2013-checkout-submodule.sh | 344 ++++++++++++++++++++++++++++++++-\n t/t9902-completion.sh | 1 +\n unpack-trees.c | 103 ++++++++--\n unpack-trees.h | 1 +\n wrapper.c | 4 +\n 16 files changed, 806 insertions(+), 85 deletions(-)\n\n-- \n2.10.1.469.g00a8914\n\n"},{"id":"305994","messageId":"20161115230651.23953-2-sbeller@google.com","threadId":"44491","inReplyTo":"20161115230651.23953-1-sbeller@google.com","subject":"[PATCH 01/16] submodule.h: add extern keyword to functions, break line before 80","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-11-15T23:06:36Z","receivedAt":"2016-11-15T23:07:04Z","isPatch":true,"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, it's good to start of a sane base. So format the header to\nnot exceed 80 characters a line and mark all functions to be extern.\n\nSigned-off-by: Stefan Beller \n---\n submodule.h | 60 ++++++++++++++++++++++++++++++++++--------------------------\n 1 file changed, 34 insertions(+), 26 deletions(-)\n\ndiff --git a/submodule.h b/submodule.h\nindex d9e197a..afc58d0 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -29,50 +29,58 @@ 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(\n+\t\tstruct diff_options *diffopt,\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(\n+\t\tconst struct submodule_update_strategy *s);\n+extern void handle_ignore_submodules_arg(struct diff_options *diffopt,\n+\t\t\t\t\t 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-void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);\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,\n+\t\t\t\t\t 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 #endif\n-- \n2.10.1.469.g00a8914\n\n"},{"id":"305995","messageId":"20161115230651.23953-7-sbeller@google.com","threadId":"44491","inReplyTo":"20161115230651.23953-1-sbeller@google.com","subject":"[PATCH 06/16] update submodules: add a config option to determine if submodules are updated","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-11-15T23:06:41Z","receivedAt":"2016-11-15T23:07:08Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In later patches we introduce the options and flag for commands\nthat modify the working directory, e.g. git-checkout.\n\nHave a central place to store such settings whether we want to update\na submodule.\n\nSigned-off-by: Stefan Beller \n---\n submodule.c | 6 ++++++\n submodule.h | 1 +\n 2 files changed, 7 insertions(+)\n\ndiff --git a/submodule.c b/submodule.c\nindex 97eaf7c..38b0573 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -16,6 +16,7 @@\n #include \"quote.h\"\n \n static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;\n+static int config_update_recurse_submodules = RECURSE_SUBMODULES_DEFAULT;\n static int parallel_jobs = 1;\n static struct string_list changed_submodule_paths = STRING_LIST_INIT_NODUP;\n static int initialized_fetch_ref_tips;\n@@ -494,6 +495,11 @@ void set_config_fetch_recurse_submodules(int value)\n \tconfig_fetch_recurse_submodules = value;\n }\n \n+void set_config_update_recurse_submodules(int value)\n+{\n+\tconfig_update_recurse_submodules = value;\n+}\n+\n static int has_remote(const char *refname, const struct object_id *oid,\n \t\t int flags, void *cb_data)\n {\ndiff --git a/submodule.h b/submodule.h\nindex d44b4f1..185ad18 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -56,6 +56,7 @@ 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+extern void set_config_update_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-- \n2.10.1.469.g00a8914\n\n"},{"id":"305996","messageId":"20161115230651.23953-9-sbeller@google.com","threadId":"44491","inReplyTo":"20161115230651.23953-1-sbeller@google.com","subject":"[PATCH 08/16] update submodules: add depopulate_submodule","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-11-15T23:06:43Z","receivedAt":"2016-11-15T23:07:11Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Implement the functionality needed to enable work tree manipulating\ncommands to that a deleted submodule should not only affect the index\n(leaving all the files of the submodule in the work tree) but also to\nremove the work tree of the superproject (including any untracked\nfiles).\n\nTo do so, we need an equivalent of \"rm -rf\", which is already found in\nentry.c, so expose that and for clarity add a suffix \"_or_dir\" to it.\n\nThat will only work properly when the submodule uses a gitfile instead of\na .git directory and no untracked files are present. Otherwise the removal\nwill fail with a warning (which is just what happened until now).\n\nSigned-off-by: Stefan Beller \n---\n cache.h | 2 ++\n entry.c | 8 ++++++++\n submodule.c | 25 +++++++++++++++++++++++++\n submodule.h | 1 +\n 4 files changed, 36 insertions(+)\n\ndiff --git a/cache.h b/cache.h\nindex a50a61a..65c47e4 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+void remove_subtree_or_die(const char *path);\n+\n #endif /* CACHE_H */\ndiff --git a/entry.c b/entry.c\nindex c6eea24..019826b 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -73,6 +73,14 @@ static void remove_subtree(struct strbuf *path)\n \t\tdie_errno(\"cannot rmdir '%s'\", path->buf);\n }\n \n+void remove_subtree_or_die(const char *path)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tstrbuf_addstr(&sb, path);\n+\tremove_subtree(&sb);\n+\tstrbuf_release(&sb);\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 d34b721..0fa4613 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -308,6 +308,31 @@ static void print_submodule_summary(struct rev_info *rev, FILE *f,\n \tstrbuf_release(&sb);\n }\n \n+int depopulate_submodule(const char *path)\n+{\n+\tint ret = 0;\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\twarning(_(\"cannot remove submodule '%s' because it (or one of \"\n+\t\t\t \"its nested submodules) uses a .git directory\"),\n+\t\t\t path);\n+\t\tret = -1;\n+\t\tgoto out;\n+\t}\n+\n+\tremove_subtree_or_die(path);\n+\n+out:\n+\tfree(dot_git);\n+\treturn ret;\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 3df6881..8518cf3 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -65,6 +65,7 @@ extern void set_config_update_recurse_submodules(int value);\n */\n extern int submodule_is_interesting(const char *path, const unsigned char *sha1);\n extern int submodules_interesting_for_update(void);\n+extern int 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,\n-- \n2.10.1.469.g00a8914\n\n"},{"id":"305997","messageId":"20161115230651.23953-12-sbeller@google.com","threadId":"44491","inReplyTo":"20161115230651.23953-1-sbeller@google.com","subject":"[PATCH 11/16] teach unpack_trees() to remove submodule contents","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-11-15T23:06:46Z","receivedAt":"2016-11-15T23:07:19Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Extend rmdir_or_warn() to remove the directories of those submodules which\nare scheduled for removal. Also teach verify_clean_submodule() to check\nthat a submodule configured to be removed is not modified before scheduling\nit for removal.\n\nSigned-off-by: Stefan Beller \n---\n unpack-trees.c | 6 ++----\n wrapper.c | 4 ++++\n 2 files changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex ea6bdd2..576e1d5 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -9,6 +9,7 @@\n #include \"refs.h\"\n #include \"attr.h\"\n #include \"split-index.h\"\n+#include \"submodule.h\"\n #include \"dir.h\"\n \n /*\n@@ -1361,15 +1362,12 @@ static void invalidate_ce_path(const struct cache_entry *ce,\n /*\n * Check that checking out ce->sha1 in subdir ce->name is not\n * going to overwrite any working files.\n- *\n- * Currently, git does not checkout subprojects during a superproject\n- * checkout, so it is not going to overwrite anything.\n */\n static int verify_clean_submodule(const struct cache_entry *ce,\n \t\t\t\t enum unpack_trees_error_types error_type,\n \t\t\t\t struct unpack_trees_options *o)\n {\n-\treturn 0;\n+\treturn submodule_is_interesting(ce->name, null_sha1) && is_submodule_modified(ce->name, 0);\n }\n \n static int verify_clean_subdirectory(const struct cache_entry *ce,\ndiff --git a/wrapper.c b/wrapper.c\nindex e7f1979..17c08de 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -2,6 +2,7 @@\n * Various trivial helper wrappers around standard functions\n */\n #include \"cache.h\"\n+#include \"submodule.h\"\n \n static void do_nothing(size_t size)\n {\n@@ -592,6 +593,9 @@ int unlink_or_warn(const char *file)\n \n int rmdir_or_warn(const char *file)\n {\n+\tif (submodule_is_interesting(file, null_sha1)\n+\t && depopulate_submodule(file))\n+\t\treturn -1;\n \treturn warn_if_unremovable(\"rmdir\", file, rmdir(file));\n }\n \n-- \n2.10.1.469.g00a8914\n\n"},{"id":"305998","messageId":"20161115230651.23953-10-sbeller@google.com","threadId":"44491","inReplyTo":"20161115230651.23953-1-sbeller@google.com","subject":"[PATCH 09/16] update submodules: add scheduling to update submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-11-15T23:06:44Z","receivedAt":"2016-11-15T23:07:23Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The walker of a tree is only expected to call `schedule_submodule_for_update`\nand once done, to run `update_submodules`. This avoids directory/file\nconflicts and later we can parallelize all submodule actions if needed.\n\nSigned-off-by: Stefan Beller \n---\n submodule.c | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n submodule.h | 4 +++\n 2 files changed, 121 insertions(+)\n\ndiff --git a/submodule.c b/submodule.c\nindex 0fa4613..2773151 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -308,6 +308,75 @@ static void print_submodule_summary(struct rev_info *rev, FILE *f,\n \tstrbuf_release(&sb);\n }\n \n+static int update_submodule(const char *path, const struct object_id *oid,\n+\t\t\t int force, int is_new)\n+{\n+\tconst char *git_dir;\n+\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\tconst struct submodule *sub = submodule_from_path(null_sha1, path);\n+\n+\tif (!sub || !sub->name)\n+\t\treturn -1;\n+\n+\tgit_dir = resolve_gitdir(git_common_path(\"modules/%s\", sub->name));\n+\n+\tif (!git_dir)\n+\t\treturn -1;\n+\n+\tif (is_new)\n+\t\tconnect_work_tree_and_git_dir(path, git_dir);\n+\n+\t/* update index via `read-tree --reset sha1` */\n+\targv_array_pushl(&cp.args, \"read-tree\",\n+\t\t\t\t force ? \"--reset\" : \"-m\",\n+\t\t\t\t \"-u\", sha1_to_hex(oid->hash), NULL);\n+\tprepare_submodule_repo_env(&cp.env_array);\n+\tcp.git_cmd = 1;\n+\tcp.no_stdin = 1;\n+\tcp.dir = path;\n+\tif (run_command(&cp)) {\n+\t\twarning(_(\"reading the index in submodule '%s' failed\"), path);\n+\t\tchild_process_clear(&cp);\n+\t\treturn -1;\n+\t}\n+\n+\t/* write index to working dir */\n+\tchild_process_clear(&cp);\n+\tchild_process_init(&cp);\n+\targv_array_pushl(&cp.args, \"checkout-index\", \"-a\", NULL);\n+\tcp.git_cmd = 1;\n+\tcp.no_stdin = 1;\n+\tcp.dir = path;\n+\tif (force)\n+\t\targv_array_push(&cp.args, \"-f\");\n+\n+\tif (run_command(&cp)) {\n+\t\twarning(_(\"populating the working directory in submodule '%s' failed\"), path);\n+\t\tchild_process_clear(&cp);\n+\t\treturn -1;\n+\t}\n+\n+\t/* get the HEAD right */\n+\tchild_process_clear(&cp);\n+\tchild_process_init(&cp);\n+\targv_array_pushl(&cp.args, \"checkout\", \"--recurse-submodules\", NULL);\n+\tcp.git_cmd = 1;\n+\tcp.no_stdin = 1;\n+\tcp.dir = path;\n+\tif (force)\n+\t\targv_array_push(&cp.args, \"-f\");\n+\targv_array_push(&cp.args, sha1_to_hex(oid->hash));\n+\n+\tif (run_command(&cp)) {\n+\t\twarning(_(\"setting the HEAD in submodule '%s' failed\"), path);\n+\t\tchild_process_clear(&cp);\n+\t\treturn -1;\n+\t}\n+\n+\tchild_process_clear(&cp);\n+\treturn 0;\n+}\n+\n int depopulate_submodule(const char *path)\n {\n \tint ret = 0;\n@@ -1336,3 +1405,51 @@ void prepare_submodule_repo_env(struct argv_array *out)\n \t}\n \targv_array_push(out, \"GIT_DIR=.git\");\n }\n+\n+struct scheduled_submodules_update_type {\n+\tconst char *path;\n+\tconst struct object_id *oid;\n+\t/*\n+\t * Do we need to perform a complete checkout or just incremental\n+\t * update?\n+\t */\n+\tunsigned is_new:1;\n+} *scheduled_submodules;\n+#define SCHEDULED_SUBMODULES_INIT {NULL, NULL}\n+\n+int scheduled_submodules_nr, scheduled_submodules_alloc;\n+\n+void schedule_submodule_for_update(const struct cache_entry *ce, int is_new)\n+{\n+\tstruct scheduled_submodules_update_type *ssu;\n+\tALLOC_GROW(scheduled_submodules,\n+\t\t scheduled_submodules_nr + 1,\n+\t\t scheduled_submodules_alloc);\n+\tssu = &scheduled_submodules[scheduled_submodules_nr++];\n+\tssu->path = ce->name;\n+\tssu->oid = &ce->oid;\n+\tssu->is_new = !!is_new;\n+}\n+\n+int update_submodules(int force)\n+{\n+\tint i;\n+\tgitmodules_config();\n+\n+\t/*\n+\t * NEEDSWORK: As submodule updates can potentially take some\n+\t * time each and they do not overlap (i.e. no d/f conflicts;\n+\t * this can be parallelized using the run_commands.h API.\n+\t */\n+\tfor (i = 0; i < scheduled_submodules_nr; i++) {\n+\t\tstruct scheduled_submodules_update_type *ssu =\n+\t\t\t&scheduled_submodules[i];\n+\n+\t\tif (submodule_is_interesting(ssu->path, null_sha1))\n+\t\t\tupdate_submodule(ssu->path, ssu->oid,\n+\t\t\t\t\t force, ssu->is_new);\n+\t}\n+\n+\tscheduled_submodules_nr = 0;\n+\treturn 0;\n+}\ndiff --git a/submodule.h b/submodule.h\nindex 8518cf3..f01f87c 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -94,4 +94,8 @@ extern int parallel_submodules(void);\n */\n extern void prepare_submodule_repo_env(struct argv_array *out);\n \n+extern void schedule_submodule_for_update(const struct cache_entry *ce,\n+\t\t\t\t\t int new);\n+extern int update_submodules(int force);\n+\n #endif\n-- \n2.10.1.469.g00a8914\n\n"},{"id":"305999","messageId":"20161115230651.23953-13-sbeller@google.com","threadId":"44491","inReplyTo":"20161115230651.23953-1-sbeller@google.com","subject":"[PATCH 12/16] entry: write_entry to write populate submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-11-15T23:06:47Z","receivedAt":"2016-11-15T23:07:24Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \n---\n entry.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/entry.c b/entry.c\nindex 019826b..2330b6e 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -2,6 +2,7 @@\n #include \"blob.h\"\n #include \"dir.h\"\n #include \"streaming.h\"\n+#include \"submodule.h\"\n \n static void create_directories(const char *path, int path_len,\n \t\t\t const struct checkout *state)\n@@ -211,6 +212,7 @@ static int write_entry(struct cache_entry *ce,\n \t\t\treturn error(\"cannot create temporary submodule %s\", path);\n \t\tif (mkdir(path, 0777) < 0)\n \t\t\treturn error(\"cannot create submodule directory %s\", path);\n+\t\tschedule_submodule_for_update(ce, 1);\n \t\tbreak;\n \tdefault:\n \t\treturn error(\"unknown file mode for %s in index\", path);\n-- \n2.10.1.469.g00a8914\n\n"},{"id":"306000","messageId":"20161115230651.23953-15-sbeller@google.com","threadId":"44491","inReplyTo":"20161115230651.23953-1-sbeller@google.com","subject":"[PATCH 14/16] checkout: recurse into submodules if asked to","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-11-15T23:06:49Z","receivedAt":"2016-11-15T23:07:28Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Allow checkout to recurse into submodules via\nthe command line option --[no-]recurse-submodules.\n\nThe flag for recurse-submodules in its current form\ncould be an OPT_BOOL, but eventually we may want to have\nit as:\n\n git checkout --recurse-submodules=rebase|merge| \\\n\t\t\tcherry-pick|checkout|none\n\nwhich resembles the submodule..update options,\nso naturally a value such as\n\"as-configured-or-X-as-fallback\" would also come in handy.\n\nSigned-off-by: Stefan Beller \n---\n Documentation/git-checkout.txt | 7 +\n builtin/checkout.c | 31 +++-\n t/lib-submodule-update.sh | 10 +-\n t/t2013-checkout-submodule.sh | 325 ++++++++++++++++++++++++++++++++++++++++-\n 4 files changed, 362 insertions(+), 11 deletions(-)\n\ndiff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt\nindex 8e2c066..a0ea2c5 100644\n--- a/Documentation/git-checkout.txt\n+++ b/Documentation/git-checkout.txt\n@@ -256,6 +256,13 @@ section of linkgit:git-add[1] to learn how to operate the `--patch` mode.\n \tout anyway. In other words, the ref can be held by more than one\n \tworktree.\n \n+--[no-]recurse-submodules::\n+\tUsing --recurse-submodules will update the content of all initialized\n+\tsubmodules according to the commit recorded in the superproject. If\n+\tlocal modifications in a submodule would be overwritten the checkout\n+\twill fail until `-f` is used. If nothing (or --no-recurse-submodules)\n+\tis used, the work trees of submodules will not be updated.\n+\n ::\n \tBranch to checkout; if it refers to a branch (i.e., a name that,\n \twhen prepended with \"refs/heads/\", is a valid ref), then that\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 9b2a5b3..2a626a3 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -21,12 +21,31 @@\n #include \"submodule-config.h\"\n #include \"submodule.h\"\n \n+static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;\n+\n static const char * const checkout_usage[] = {\n \tN_(\"git checkout [] \"),\n \tN_(\"git checkout [] [] -- ...\"),\n \tNULL,\n };\n \n+int option_parse_recurse_submodules(const struct option *opt,\n+\t\t\t\t const char *arg, int unset)\n+{\n+\tif (unset) {\n+\t\trecurse_submodules = RECURSE_SUBMODULES_OFF;\n+\t\treturn 0;\n+\t}\n+\tif (arg)\n+\t\trecurse_submodules =\n+\t\t\tparse_update_recurse_submodules_arg(opt->long_name,\n+\t\t\t\t\t\t\t arg);\n+\telse\n+\t\trecurse_submodules = RECURSE_SUBMODULES_ON;\n+\n+\treturn 0;\n+}\n+\n struct checkout_opts {\n \tint patch_mode;\n \tint quiet;\n@@ -826,7 +845,8 @@ static int switch_branches(const struct checkout_opts *opts,\n \t\tparse_commit_or_die(new->commit);\n \t}\n \n-\tret = merge_working_tree(opts, &old, new, &writeout_error);\n+\tret = merge_working_tree(opts, &old, new, &writeout_error) ||\n+\t update_submodules(opts->force);\n \tif (ret) {\n \t\tfree(path_to_free);\n \t\treturn ret;\n@@ -1160,6 +1180,9 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \t\t\t\tN_(\"second guess 'git checkout '\")),\n \t\tOPT_BOOL(0, \"ignore-other-worktrees\", &opts.ignore_other_worktrees,\n \t\t\t N_(\"do not check if another worktree is holding the given ref\")),\n+\t\t{ OPTION_CALLBACK, 0, \"recurse-submodules\", &recurse_submodules,\n+\t\t\t \"checkout\", \"control recursive updating of submodules\",\n+\t\t\t PARSE_OPT_OPTARG, option_parse_recurse_submodules },\n \t\tOPT_BOOL(0, \"progress\", &opts.show_progress, N_(\"force progress reporting\")),\n \t\tOPT_END(),\n \t};\n@@ -1190,6 +1213,12 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \t\tgit_xmerge_config(\"merge.conflictstyle\", conflict_style, NULL);\n \t}\n \n+\tif (recurse_submodules != RECURSE_SUBMODULES_OFF) {\n+\t\tgit_config(submodule_config, NULL);\n+\t\tif (recurse_submodules != RECURSE_SUBMODULES_DEFAULT)\n+\t\t\tset_config_update_recurse_submodules(recurse_submodules);\n+\t}\n+\n \tif ((!!opts.new_branch + !!opts.new_branch_force + !!opts.new_orphan_branch) > 1)\n \t\tdie(_(\"-b, -B and --orphan are mutually exclusive\"));\n \ndiff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh\nindex 79cdd34..e0773c6 100755\n--- a/t/lib-submodule-update.sh\n+++ b/t/lib-submodule-update.sh\n@@ -634,7 +634,13 @@ test_submodule_forced_switch () {\n \n \t########################## Modified submodule #########################\n \t# Updating a submodule sha1 doesn't update the submodule's work tree\n-\ttest_expect_success \"$command: modified submodule does not update submodule work tree\" '\n+\tif test \"$KNOWN_FAILURE_RECURSE_SUBMODULE_SERIES_BREAKS_REPLACE_SUBMODULE_TEST\" = 1\n+\tthen\n+\t\tRESULT=\"failure\"\n+\telse\n+\t\tRESULT=\"success\"\n+\tfi\n+\ttest_expect_$RESULT \"$command: modified submodule does not update submodule work tree\" '\n \t\tprolog &&\n \t\treset_work_tree_to add_sub1 &&\n \t\t(\n@@ -649,7 +655,7 @@ test_submodule_forced_switch () {\n \t'\n \t# Updating a submodule to an invalid sha1 doesn't update the\n \t# submodule's work tree, subsequent update will fail\n-\ttest_expect_success \"$command: modified submodule does not update submodule work tree to invalid commit\" '\n+\ttest_expect_$RESULT \"$command: modified submodule does not update submodule work tree to invalid commit\" '\n \t\tprolog &&\n \t\treset_work_tree_to add_sub1 &&\n \t\t(\ndiff --git a/t/t2013-checkout-submodule.sh b/t/t2013-checkout-submodule.sh\nindex 6847f75..60f6987 100755\n--- a/t/t2013-checkout-submodule.sh\n+++ b/t/t2013-checkout-submodule.sh\n@@ -7,15 +7,21 @@ test_description='checkout can handle submodules'\n \n test_expect_success 'setup' '\n \tmkdir submodule &&\n-\t(cd submodule &&\n-\t git init &&\n-\t test_commit first) &&\n-\tgit add submodule &&\n+\t(\n+\t\tcd submodule &&\n+\t\tgit init &&\n+\t\ttest_commit first\n+\t) &&\n+\techo first >file &&\n+\tgit add file submodule &&\n \ttest_tick &&\n \tgit commit -m superproject &&\n-\t(cd submodule &&\n-\t test_commit second) &&\n-\tgit add submodule &&\n+\t(\n+\t\tcd submodule &&\n+\t\ttest_commit second\n+\t) &&\n+\techo second > file &&\n+\tgit add file submodule &&\n \ttest_tick &&\n \tgit commit -m updated.superproject\n '\n@@ -37,7 +43,8 @@ test_expect_success '\"checkout \" updates the index only' '\n \tgit checkout HEAD^ submodule &&\n \ttest_must_fail git diff-files --quiet &&\n \tgit checkout HEAD submodule &&\n-\tgit diff-files --quiet\n+\tgit diff-files --quiet &&\n+\tgit diff-index --quiet --cached HEAD\n '\n \n test_expect_success '\"checkout \" honors diff.ignoreSubmodules' '\n@@ -63,8 +70,310 @@ test_expect_success '\"checkout \" honors submodule.*.ignore from .git/\n \t! test -s actual\n '\n \n+# TODO: hardcode $2\n+# use flag instead of checking for directory.\n+submodule_creation_must_succeed() {\n+\t# checkout base ($1)\n+\tgit checkout -f --recurse-submodules $1 &&\n+\tgit diff-files --quiet &&\n+\tgit diff-index --quiet --cached $1 &&\n+\n+\t# checkout target ($2)\n+\tif test -d submodule; then\n+\t\techo change >>submodule/first.t &&\n+\t\ttest_must_fail git checkout --recurse-submodules $2 &&\n+\t\tgit checkout -f --recurse-submodules $2\n+\telse\n+\t\tgit checkout --recurse-submodules $2\n+\tfi &&\n+\ttest -e submodule/.git &&\n+\ttest -f submodule/first.t &&\n+\ttest -f submodule/second.t &&\n+\tgit diff-files --quiet &&\n+\tgit diff-index --quiet --cached $2\n+}\n+\n+# TODO: inline this:\n+submodule_removal_must_succeed() {\n+\n+\t# checkout base ($1)\n+\tgit checkout -f --recurse-submodules $1 &&\n+\tgit submodule update -f . &&\n+\ttest -e submodule/.git &&\n+\tgit diff-files --quiet &&\n+\tgit diff-index --quiet --cached $1 &&\n+\n+\t# checkout target ($2)\n+\techo change >>submodule/first.t &&\n+\ttest_must_fail git checkout --recurse-submodules $2 &&\n+\tgit checkout -f --recurse-submodules $2 &&\n+\tgit diff-files --quiet &&\n+\tgit diff-index --quiet --cached $2 &&\n+\t! test -d submodule\n+}\n+\n+test_expect_success '\"check --recurse-submodules\" removes deleted submodule' '\n+\t# first some setup:\n+\tgit config -f .gitmodules submodule.submodule.path submodule &&\n+\tgit config -f .gitmodules submodule.submodule.url ./submodule.bare &&\n+\tgit -C submodule clone --bare . ../submodule.bare &&\n+\techo submodule.bare >>.gitignore &&\n+\tgit add .gitignore .gitmodules submodule &&\n+\tgit commit -m \"submodule registered with a gitmodules file\" &&\n+\n+\t# for testing this case, do it in a fresh clone with the submodules\n+\t# git dir inside the superprojects .git/modules dir.\n+\tgit clone --recurse-submodules . super &&\n+\t(\n+\t\tcd super &&\n+\t\tgit checkout -b base &&\n+\t\tgit checkout -b delete_submodule &&\n+\t\trm -rf submodule &&\n+\t\tgit rm submodule &&\n+\t\tgit commit -m \"submodule deleted\" &&\n+\t\tsubmodule_removal_must_succeed base delete_submodule\n+\t)\n+'\n+\n+test_expect_success '\"checkout --recurse-submodules\" does not delete submodule with .git dir inside' '\n+\tgit fetch super delete_submodule &&\n+\tgit checkout --recurse-submodules FETCH_HEAD 2>output.err &&\n+\ttest_i18ngrep \"cannot remove submodule\" output.err &&\n+\ttest -d submodule/.git\n+'\n+\n+test_expect_success '\"checkout --recurse-submodules\" repopulates submodule' '\n+\t(\n+\t\tcd super &&\n+\t\tsubmodule_creation_must_succeed delete_submodule base\n+\t)\n+'\n+\n+test_expect_success '\"checkout --recurse-submodules\" repopulates submodule in existing directory' '\n+\t(\n+\t\tcd super &&\n+\t\tgit checkout --recurse-submodules delete_submodule &&\n+\t\tmkdir submodule &&\n+\t\tsubmodule_creation_must_succeed delete_submodule base\n+\t)\n+'\n+\n+test_expect_success '\"checkout --recurse-submodules\" replaces submodule with files' '\n+\t(\n+\t\tcd super\n+\t\tgit checkout -f base &&\n+\t\tgit checkout -b replace_submodule_with_file &&\n+\t\tgit rm -f submodule &&\n+\t\techo \"file instead\" >submodule &&\n+\t\tgit add submodule &&\n+\t\tgit commit -m \"submodule replaced\" &&\n+\t\tgit checkout -f base &&\n+\t\tgit submodule update -f . &&\n+\t\tgit checkout --recurse-submodules replace_submodule_with_file &&\n+\t\ttest -f submodule\n+\t)\n+'\n+\n+test_expect_success '\"checkout --recurse-submodules\" removes files and repopulates submodule' '\n+\t(\n+\t\tcd super &&\n+\t\tsubmodule_creation_must_succeed replace_submodule_with_file base\n+\t)\n+'\n+\n+test_expect_success '\"checkout --recurse-submodules\" replaces submodule with a directory' '\n+\t(\n+\t\tcd super\n+\t\tgit checkout -f base &&\n+\t\tgit checkout -b replace_submodule_with_dir &&\n+\t\tgit rm -f submodule &&\n+\t\tmkdir -p submodule/dir &&\n+\t\techo content >submodule/dir/file &&\n+\t\tgit add submodule &&\n+\t\tgit commit -m \"submodule replaced with a directory (file inside)\" &&\n+\t\tgit checkout -f base &&\n+\t\tgit submodule update -f . &&\n+\t\tgit checkout --recurse-submodules replace_submodule_with_dir &&\n+\t\ttest -d submodule &&\n+\t\t! test -e submodule/.git &&\n+\t\t! test -f submodule/first.t &&\n+\t\t! test -f submodule/second.t &&\n+\t\ttest -d submodule/dir\n+\t)\n+'\n+\n+test_expect_success '\"checkout --recurse-submodules\" removes the directory and repopulates submodule' '\n+\t(\n+\t\tcd super\n+\t\tsubmodule_creation_must_succeed replace_submodule_with_dir base\n+\t)\n+'\n+\n+test_expect_success SYMLINKS '\"checkout --recurse-submodules\" replaces submodule with a link' '\n+\t(\n+\t\tcd super\n+\t\tgit checkout -f base &&\n+\t\tgit checkout -b replace_submodule_with_link &&\n+\t\tgit rm -f submodule &&\n+\t\tln -s submodule &&\n+\t\tgit add submodule &&\n+\t\tgit commit -m \"submodule replaced with a link\" &&\n+\t\tgit checkout -f base &&\n+\t\tgit submodule update -f . &&\n+\t\tgit checkout --recurse-submodules replace_submodule_with_link &&\n+\t\ttest -L submodule\n+\t)\n+'\n+\n+test_expect_success SYMLINKS '\"checkout --recurse-submodules\" removes the link and repopulates submodule' '\n+\t(\n+\t\tcd super\n+\t\tsubmodule_creation_must_succeed replace_submodule_with_link base\n+\t)\n+'\n+\n+test_expect_success '\"checkout --recurse-submodules\" updates the submodule' '\n+\t(\n+\t\tcd super\n+\t\tgit checkout --recurse-submodules base &&\n+\t\tgit diff-files --quiet &&\n+\t\tgit diff-index --quiet --cached HEAD &&\n+\t\tgit checkout -b updated_submodule &&\n+\t\t(\n+\t\t\tcd submodule &&\n+\t\t\techo x >>first.t &&\n+\t\t\tgit add first.t &&\n+\t\t\ttest_commit third\n+\t\t) &&\n+\t\tgit add submodule &&\n+\t\ttest_tick &&\n+\t\tgit commit -m updated.superproject &&\n+\t\tgit checkout --recurse-submodules base &&\n+\t\tgit diff-files --quiet &&\n+\t\tgit diff-index --quiet --cached HEAD\n+\t)\n+'\n+\n+test_expect_failure 'untracked file is not deleted' '\n+\t(\n+\t\tcd super &&\n+\t\tgit checkout --recurse-submodules base &&\n+\t\techo important >submodule/untracked &&\n+\t\ttest_must_fail git checkout --recurse-submodules delete_submodule &&\n+\t\tgit checkout -f --recurse-submodules delete_submodule\n+\t)\n+'\n+\n+test_expect_success 'ignored file works just fine' '\n+\t(\n+\t\tcd super &&\n+\t\tgit checkout --recurse-submodules base &&\n+\t\techo important >submodule/ignored &&\n+\t\techo ignored >.git/modules/submodule/info/exclude &&\n+\t\tgit checkout --recurse-submodules delete_submodule\n+\t)\n+'\n+\n+test_expect_success 'dirty file file is not deleted' '\n+\t(\n+\t\tcd super &&\n+\t\tgit checkout --recurse-submodules base &&\n+\t\techo important >submodule/first.t &&\n+\t\ttest_must_fail git checkout --recurse-submodules delete_submodule &&\n+\t\tgit checkout -f --recurse-submodules delete_submodule\n+\t)\n+'\n+\n+test_expect_success 'added to index is not deleted' '\n+\t(\n+\t\tcd super &&\n+\t\tgit checkout --recurse-submodules base &&\n+\t\techo important >submodule/to_index &&\n+\t\tgit -C submodule add to_index &&\n+\t\ttest_must_fail git checkout --recurse-submodules delete_submodule &&\n+\t\tgit checkout -f --recurse-submodules delete_submodule\n+\t)\n+'\n+\n+# This is ok in theory, we just need to make sure\n+# the garbage collection doesn't eat the commit.\n+test_expect_success 'different commit prevents from deleting' '\n+\t(\n+\t\tcd super &&\n+\t\tgit checkout --recurse-submodules base &&\n+\t\techo important >submodule/to_index &&\n+\t\tgit -C submodule add to_index &&\n+\t\ttest_must_fail git checkout --recurse-submodules delete_submodule &&\n+\t\tgit checkout -f --recurse-submodules delete_submodule\n+\t)\n+'\n+\n+test_expect_failure '\"checkout --recurse-submodules\" needs -f to update a modifed submodule commit' '\n+\t(\n+\t\tcd submodule &&\n+\t\tgit checkout --recurse-submodules HEAD^\n+\t) &&\n+\ttest_must_fail git checkout --recurse-submodules master &&\n+\ttest_must_fail git diff-files --quiet submodule &&\n+\tgit diff-files --quiet file &&\n+\tgit checkout --recurse-submodules -f master &&\n+\tgit diff-files --quiet &&\n+\tgit diff-index --quiet --cached HEAD\n+'\n+\n+test_expect_failure '\"checkout --recurse-submodules\" needs -f to update modifed submodule content' '\n+\techo modified >submodule/second.t &&\n+\ttest_must_fail git checkout --recurse-submodules HEAD^ &&\n+\ttest_must_fail git diff-files --quiet submodule &&\n+\tgit diff-files --quiet file &&\n+\tgit checkout --recurse-submodules -f HEAD^ &&\n+\tgit diff-files --quiet &&\n+\tgit diff-index --quiet --cached HEAD &&\n+\tgit checkout --recurse-submodules -f master &&\n+\tgit diff-files --quiet &&\n+\tgit diff-index --quiet --cached HEAD\n+'\n+\n+test_expect_failure '\"checkout --recurse-submodules\" ignores modified submodule content that would not be changed' '\n+\techo modified >expected &&\n+\tcp expected submodule/first.t &&\n+\tgit checkout --recurse-submodules HEAD^ &&\n+\ttest_cmp expected submodule/first.t &&\n+\ttest_must_fail git diff-files --quiet submodule &&\n+\tgit diff-index --quiet --cached HEAD &&\n+\tgit checkout --recurse-submodules -f master &&\n+\tgit diff-files --quiet &&\n+\tgit diff-index --quiet --cached HEAD\n+'\n+\n+test_expect_failure '\"checkout --recurse-submodules\" does not care about untracked submodule content' '\n+\techo untracked >submodule/untracked &&\n+\tgit checkout --recurse-submodules master &&\n+\tgit diff-files --quiet --ignore-submodules=untracked &&\n+\tgit diff-index --quiet --cached HEAD &&\n+\trm submodule/untracked\n+'\n+\n+test_expect_failure '\"checkout --recurse-submodules\" needs -f when submodule commit is not present (but does fail anyway)' '\n+\tgit checkout --recurse-submodules -b bogus_commit master &&\n+\tgit update-index --cacheinfo 160000 0123456789012345678901234567890123456789 submodule &&\n+\tBOGUS_TREE=$(git write-tree) &&\n+\tBOGUS_COMMIT=$(echo \"bogus submodule commit\" | git commit-tree $BOGUS_TREE) &&\n+\tgit commit -m \"bogus submodule commit\" &&\n+\tgit checkout --recurse-submodules -f master &&\n+\ttest_must_fail git checkout --recurse-submodules bogus_commit &&\n+\tgit diff-files --quiet &&\n+\ttest_must_fail git checkout --recurse-submodules -f bogus_commit &&\n+\ttest_must_fail git diff-files --quiet submodule &&\n+\tgit diff-files --quiet file &&\n+\tgit diff-index --quiet --cached HEAD &&\n+\tgit checkout --recurse-submodules -f master\n+'\n+\n+KNOWN_FAILURE_RECURSE_SUBMODULE_SERIES_BREAKS_REPLACE_SUBMODULE_TEST=1\n test_submodule_switch \"git checkout\"\n \n+KNOWN_FAILURE_RECURSE_SUBMODULE_SERIES_BREAKS_REPLACE_SUBMODULE_TEST=\n test_submodule_forced_switch \"git checkout -f\"\n \n test_done\n-- \n2.10.1.469.g00a8914\n\n"},{"id":"306001","messageId":"20161115230651.23953-14-sbeller@google.com","threadId":"44491","inReplyTo":"20161115230651.23953-1-sbeller@google.com","subject":"[PATCH 13/16] submodule: teach unpack_trees() to update submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-11-15T23:06:48Z","receivedAt":"2016-11-15T23:07:31Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \n---\n entry.c | 7 +++--\n unpack-trees.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++++----------\n unpack-trees.h | 1 +\n 3 files changed, 86 insertions(+), 19 deletions(-)\n\ndiff --git a/entry.c b/entry.c\nindex 2330b6e..dd829ec 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -270,7 +270,7 @@ int checkout_entry(struct cache_entry *ce,\n \n \tif (!check_path(path.buf, path.len, &st, state->base_dir_len)) {\n \t\tunsigned changed = ce_match_stat(ce, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE);\n-\t\tif (!changed)\n+\t\tif (!changed && (!S_ISDIR(st.st_mode) || !S_ISGITLINK(ce->ce_mode)))\n \t\t\treturn 0;\n \t\tif (!state->force) {\n \t\t\tif (!state->quiet)\n@@ -287,9 +287,10 @@ int checkout_entry(struct cache_entry *ce,\n \t\t * just do the right thing)\n \t\t */\n \t\tif (S_ISDIR(st.st_mode)) {\n-\t\t\t/* If it is a gitlink, leave it alone! */\n-\t\t\tif (S_ISGITLINK(ce->ce_mode))\n+\t\t\tif (S_ISGITLINK(ce->ce_mode)) {\n+\t\t\t\tschedule_submodule_for_update(ce, 1);\n \t\t\t\treturn 0;\n+\t\t\t}\n \t\t\tif (!state->force)\n \t\t\t\treturn error(\"%s is a directory\", path.buf);\n \t\t\tremove_subtree(&path);\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 576e1d5..c5c22ed 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -29,6 +29,9 @@ static const char *unpack_plumbing_errors[NB_UNPACK_TREES_ERROR_TYPES] = {\n \t/* ERROR_NOT_UPTODATE_DIR */\n \t\"Updating '%s' would lose untracked files in it\",\n \n+\t/* ERROR_NOT_UPTODATE_SUBMODULE */\n+\t\"Updating submodule '%s' would lose modifications in it\",\n+\n \t/* ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN */\n \t\"Untracked working tree file '%s' would be overwritten by merge.\",\n \n@@ -80,6 +83,8 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,\n \n \tmsgs[ERROR_NOT_UPTODATE_DIR] =\n \t\t_(\"Updating the following directories would lose untracked files in it:\\n%s\");\n+\tmsgs[ERROR_NOT_UPTODATE_SUBMODULE] =\n+\t\t_(\"Updating the following submodules would lose modifications in it:\\n%s\");\n \n \tif (!strcmp(cmd, \"checkout\"))\n \t\tmsg = advice_commit_before_merge\n@@ -1315,19 +1320,18 @@ static int verify_uptodate_1(const struct cache_entry *ce,\n \t\treturn 0;\n \n \tif (!lstat(ce->name, &st)) {\n-\t\tint flags = CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE;\n-\t\tunsigned changed = ie_match_stat(o->src_index, ce, &st, flags);\n-\t\tif (!changed)\n-\t\t\treturn 0;\n-\t\t/*\n-\t\t * NEEDSWORK: the current default policy is to allow\n-\t\t * submodule to be out of sync wrt the superproject\n-\t\t * index. This needs to be tightened later for\n-\t\t * submodules that are marked to be automatically\n-\t\t * checked out.\n-\t\t */\n-\t\tif (S_ISGITLINK(ce->ce_mode))\n-\t\t\treturn 0;\n+\t\tif (!S_ISGITLINK(ce->ce_mode)) {\n+\t\t\tint flags = CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE;\n+\t\t\tunsigned changed = ie_match_stat(o->src_index, ce, &st, flags);\n+\t\t\tif (!changed)\n+\t\t\t\treturn 0;\n+\t\t} else {\n+\t\t\tif (!submodule_is_interesting(ce->name, null_sha1))\n+\t\t\t\treturn 0;\n+\t\t\tif (ce_stage(ce) ? is_submodule_checkout_safe(ce->name, &ce->oid)\n+\t\t\t : !is_submodule_modified(ce->name, 1))\n+\t\t\t\treturn 0;\n+\t\t}\n \t\terrno = 0;\n \t}\n \tif (errno == ENOENT)\n@@ -1350,6 +1354,59 @@ static int verify_uptodate_sparse(const struct cache_entry *ce,\n \treturn verify_uptodate_1(ce, o, ERROR_SPARSE_NOT_UPTODATE_FILE);\n }\n \n+/*\n+ * When a submodule gets turned into an unmerged entry, we want it to be\n+ * up-to-date regarding the merge changes.\n+ */\n+static int verify_uptodate_submodule(const struct cache_entry *old,\n+\t\t\t\t const struct cache_entry *new,\n+\t\t\t\t struct unpack_trees_options *o)\n+{\n+\tstruct stat st;\n+\n+\tif (o->index_only\n+\t || (!((old->ce_flags & CE_VALID) || ce_skip_worktree(old))\n+\t\t&& (o->reset || ce_uptodate(old))))\n+\t\treturn 0;\n+\tif (!submodule_is_interesting(new->name, null_sha1))\n+\t\treturn 0;\n+\tif (!lstat(old->name, &st)) {\n+\t\tunsigned changed = ie_match_stat(o->src_index, old, &st,\n+\t\t\t\t\t\t CE_MATCH_IGNORE_VALID|\n+\t\t\t\t\t\t CE_MATCH_IGNORE_SKIP_WORKTREE);\n+\t\tif (!changed) {\n+\t\t\t/* old is always a submodule */\n+\t\t\tif (S_ISGITLINK(new->ce_mode)) {\n+\t\t\t\t/*\n+\t\t\t\t * new is also a submodule, so check if we care\n+\t\t\t\t * and then if can checkout the new sha1 safely\n+\t\t\t\t */\n+\t\t\t\tif (submodule_is_interesting(old->name, null_sha1)\n+\t\t\t\t && is_submodule_checkout_safe(new->name, &new->oid))\n+\t\t\t\t\treturn 0;\n+\t\t\t} else {\n+\t\t\t\t/*\n+\t\t\t\t * new is not a submodule any more, so only\n+\t\t\t\t * care if we care:\n+\t\t\t\t */\n+\t\t\t\tif (submodule_is_interesting(old->name, null_sha1)\n+\t\t\t\t && ok_to_remove_submodule(old->name))\n+\t\t\t\t\treturn 0;\n+\t\t\t}\n+\t\t} else {\n+\t\t\tif (S_ISGITLINK(new->ce_mode))\n+\t\t\t\treturn !is_submodule_checkout_safe(new->name, &new->oid);\n+\t\t\telse\n+\t\t\t\treturn !ok_to_remove_submodule(old->name);\n+\t\t}\n+\t\terrno = 0;\n+\t}\n+\tif (errno == ENOENT)\n+\t\treturn 0;\n+\treturn o->gently ? -1 :\n+\t\tadd_rejected_path(o, ERROR_NOT_UPTODATE_SUBMODULE, old->name);\n+}\n+\n static void invalidate_ce_path(const struct cache_entry *ce,\n \t\t\t struct unpack_trees_options *o)\n {\n@@ -1608,9 +1665,17 @@ static int merged_entry(const struct cache_entry *ce,\n \t\t\tcopy_cache_entry(merge, old);\n \t\t\tupdate = 0;\n \t\t} else {\n-\t\t\tif (verify_uptodate(old, o)) {\n-\t\t\t\tfree(merge);\n-\t\t\t\treturn -1;\n+\t\t\tif (S_ISGITLINK(old->ce_mode) ||\n+\t\t\t S_ISGITLINK(merge->ce_mode)) {\n+\t\t\t\tif (verify_uptodate_submodule(old, merge, o)) {\n+\t\t\t\t\tfree(merge);\n+\t\t\t\t\treturn -1;\n+\t\t\t\t}\n+\t\t\t} else {\n+\t\t\t\tif (verify_uptodate(old, o)) {\n+\t\t\t\t\tfree(merge);\n+\t\t\t\t\treturn -1;\n+\t\t\t\t}\n \t\t\t}\n \t\t\t/* Migrate old flags over */\n \t\t\tupdate |= old->ce_flags & (CE_SKIP_WORKTREE | CE_NEW_SKIP_WORKTREE);\ndiff --git a/unpack-trees.h b/unpack-trees.h\nindex 36a73a6..bee8740 100644\n--- a/unpack-trees.h\n+++ b/unpack-trees.h\n@@ -15,6 +15,7 @@ enum unpack_trees_error_types {\n \tERROR_WOULD_OVERWRITE = 0,\n \tERROR_NOT_UPTODATE_FILE,\n \tERROR_NOT_UPTODATE_DIR,\n+\tERROR_NOT_UPTODATE_SUBMODULE,\n \tERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN,\n \tERROR_WOULD_LOSE_UNTRACKED_REMOVED,\n \tERROR_BIND_OVERLAP,\n-- \n2.10.1.469.g00a8914\n\n"},{"id":"306002","messageId":"e19fbbca91d5446e8fe308e847f53ae3@exmbdft7.ad.twosigma.com","threadId":"44491","inReplyTo":"20161115230651.23953-3-sbeller@google.com","subject":"RE: [PATCH 02/16] submodule: modernize ok_to_remove_submodule to use argv_array","fromName":"David Turner","fromEmail":"david.turner@twosigma.com","sentAt":"2016-11-15T23:11:15Z","receivedAt":"2016-11-15T23:11:22Z","isPatch":true,"sender":{"key":"david.turner@twosigma.com","avatar":null},"body":"> -\t\t\"-u\",\n...\n> +\targv_array_pushl(&cp.args, \"status\", \"--porcelain\", \"-uall\",\n\nThis also changes -u to -uall, which is not mentioned in the commit message. That should probably be called out.\n"},{"id":"306003","messageId":"20161115230651.23953-17-sbeller@google.com","threadId":"44491","inReplyTo":"20161115230651.23953-1-sbeller@google.com","subject":"[PATCH 16/16] checkout: add config option to recurse into submodules by default","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-11-15T23:06:51Z","receivedAt":"2016-11-15T23:12:23Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"To make it easier for the user, who doesn't want to give the\n`--recurse-submodules` option whenever they run checkout, have an\noption for to set the default behavior for checkout to recurse into\nsubmodules.\n\nSigned-off-by: Stefan Beller \n---\n Documentation/config.txt | 6 ++++++\n Documentation/git-checkout.txt | 5 +++--\n submodule.c | 6 +++---\n t/t2013-checkout-submodule.sh | 19 +++++++++++++++++++\n 4 files changed, 31 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex a0ab66a..67e0714 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -949,6 +949,12 @@ clean.requireForce::\n \tA boolean to make git-clean do nothing unless given -f,\n \t-i or -n. Defaults to true.\n \n+checkout.recurseSubmodules::\n+\tThis option can be set to a boolean value.\n+\tWhen set to true checkout will recurse into submodules and\n+\tupdate them. When set to false, which is the default, checkout\n+\twill leave submodules untouched.\n+\n color.branch::\n \tA boolean to enable/disable color in the output of\n \tlinkgit:git-branch[1]. May be set to `always`,\ndiff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt\nindex a0ea2c5..819c430 100644\n--- a/Documentation/git-checkout.txt\n+++ b/Documentation/git-checkout.txt\n@@ -260,8 +260,9 @@ section of linkgit:git-add[1] to learn how to operate the `--patch` mode.\n \tUsing --recurse-submodules will update the content of all initialized\n \tsubmodules according to the commit recorded in the superproject. If\n \tlocal modifications in a submodule would be overwritten the checkout\n-\twill fail until `-f` is used. If nothing (or --no-recurse-submodules)\n-\tis used, the work trees of submodules will not be updated.\n+\twill fail until `-f` is used. If `--no-recurse-submodules` is used,\n+\tthe work trees of submodules will not be updated. If no command line\n+\targument is given, `checkout.recurseSubmodules` is used as a default.\n \n ::\n \tBranch to checkout; if it refers to a branch (i.e., a name that,\ndiff --git a/submodule.c b/submodule.c\nindex 2149ef7..0c807d9 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -160,10 +160,10 @@ int submodule_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t} else if (starts_with(var, \"submodule.\"))\n \t\treturn parse_submodule_config_option(var, value);\n-\telse if (!strcmp(var, \"fetch.recursesubmodules\")) {\n+\telse if (!strcmp(var, \"fetch.recursesubmodules\"))\n \t\tconfig_fetch_recurse_submodules = parse_fetch_recurse_submodules_arg(var, value);\n-\t\treturn 0;\n-\t}\n+\telse if (!strcmp(var, \"checkout.recursesubmodules\"))\n+\t\tconfig_update_recurse_submodules = parse_update_recurse_submodules_arg(var, value);\n \treturn 0;\n }\n \ndiff --git a/t/t2013-checkout-submodule.sh b/t/t2013-checkout-submodule.sh\nindex 60f6987..788c59d 100755\n--- a/t/t2013-checkout-submodule.sh\n+++ b/t/t2013-checkout-submodule.sh\n@@ -149,6 +149,25 @@ test_expect_success '\"checkout --recurse-submodules\" repopulates submodule' '\n \t)\n '\n \n+test_expect_success 'option checkout.recurseSubmodules updates submodule' '\n+\ttest_config -C super checkout.recurseSubmodules 1 &&\n+\t(\n+\t\tcd super &&\n+\t\tgit checkout base &&\n+\t\tgit checkout -b advanced-base &&\n+\t\tgit -C submodule commit --allow-empty -m \"empty commit\" &&\n+\t\tgit add submodule &&\n+\t\tgit commit -m \"advance submodule\" &&\n+\t\tgit checkout base &&\n+\t\tgit diff-files --quiet &&\n+\t\tgit diff-index --quiet --cached base &&\n+\t\tgit checkout advanced-base &&\n+\t\tgit diff-files --quiet &&\n+\t\tgit diff-index --quiet --cached advanced-base &&\n+\t\tgit checkout --recurse-submodules base\n+\t)\n+'\n+\n test_expect_success '\"checkout --recurse-submodules\" repopulates submodule in existing directory' '\n \t(\n \t\tcd super &&\n-- \n2.10.1.469.g00a8914\n\n"},{"id":"306004","messageId":"20161115230651.23953-8-sbeller@google.com","threadId":"44491","inReplyTo":"20161115230651.23953-1-sbeller@google.com","subject":"[PATCH 07/16] update submodules: introduce submodule_is_interesting","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-11-15T23:06:42Z","receivedAt":"2016-11-15T23:13:01Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In later patches we introduce the --recurse-submodule flag for commands\nthat modify the working directory, e.g. git-checkout.\n\nIt is potentially expensive to check if a submodule needs an update,\nbecause a common theme to interact with submodules is to spawn a child\nprocess for each interaction.\n\nSo let's introduce a function that pre checks if a submodule needs\nto be checked for an update.\n\nI am not particular happy with the name `submodule_is_interesting`,\nin internal iterations I had `submodule_requires_check_for_update`\nand `submodule_needs_update`, but I was even less happy with those\nnames. Maybe `submodule_interesting_for_update`?\n\nGenerally this is to answer \"Am I allowed to touch the submodule\nat all?\" or: \"Does the user expect me to touch it?\"\nwhich includes all of creation/deletion/update.\n\nThis patch is based off a prior attempt by Jens Lehmann to add\nsubmodules to checkout.\n\nSigned-off-by: Stefan Beller \n---\n submodule.c | 37 +++++++++++++++++++++++++++++++++++++\n submodule.h | 8 ++++++++\n 2 files changed, 45 insertions(+)\n\ndiff --git a/submodule.c b/submodule.c\nindex 38b0573..d34b721 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -500,6 +500,43 @@ void set_config_update_recurse_submodules(int value)\n \tconfig_update_recurse_submodules = value;\n }\n \n+int submodules_interesting_for_update(void)\n+{\n+\t/*\n+\t * Update can't be \"none\", \"merge\" or \"rebase\",\n+\t * treat any value as OFF, except an explicit ON.\n+\t */\n+\treturn config_update_recurse_submodules == RECURSE_SUBMODULES_ON;\n+}\n+\n+int submodule_is_interesting(const char *path, const unsigned char *sha1)\n+{\n+\t/*\n+\t * If we cannot load a submodule config, we cannot get the name\n+\t * of the submodule, so we'd need to follow the gitlink file\n+\t */\n+\tconst struct submodule *sub;\n+\n+\tif (!submodules_interesting_for_update())\n+\t\treturn 0;\n+\n+\tsub = submodule_from_path(sha1, path);\n+\tif (!sub)\n+\t\treturn 0;\n+\n+\tswitch (sub->update_strategy.type) {\n+\tcase SM_UPDATE_UNSPECIFIED:\n+\tcase SM_UPDATE_CHECKOUT:\n+\t\treturn 1;\n+\tcase SM_UPDATE_REBASE:\n+\tcase SM_UPDATE_MERGE:\n+\tcase SM_UPDATE_NONE:\n+\tcase SM_UPDATE_COMMAND:\n+\t\treturn 0;\n+\t}\n+\treturn 0;\n+}\n+\n static int has_remote(const char *refname, const struct object_id *oid,\n \t\t int flags, void *cb_data)\n {\ndiff --git a/submodule.h b/submodule.h\nindex 185ad18..3df6881 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -57,6 +57,14 @@ extern void show_submodule_inline_diff(FILE *f, const char *path,\n \t\tconst struct diff_options *opt);\n extern void set_config_fetch_recurse_submodules(int value);\n extern void set_config_update_recurse_submodules(int value);\n+/**\n+ * When updating the working tree, do we need to check if the submodule needs\n+ * updating. We do not require a check if we are already sure that the\n+ * submodule doesn't need updating, e.g. when we are not interested in submodules\n+ * or the submodule is marked uninteresting by being not initialized.\n+ */\n+extern int submodule_is_interesting(const char *path, const unsigned char *sha1);\n+extern int submodules_interesting_for_update(void);\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-- \n2.10.1.469.g00a8914\n\n"},{"id":"306005","messageId":"20161115230651.23953-4-sbeller@google.com","threadId":"44491","inReplyTo":"20161115230651.23953-1-sbeller@google.com","subject":"[PATCH 03/16] submodule: use absolute path for computing relative path connecting","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-11-15T23:06:38Z","receivedAt":"2016-11-15T23:13:14Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This addresses a similar concern as in f8eaa0ba98b (submodule--helper,\nmodule_clone: always operate on absolute paths, 2016-03-31)\n\nWhen computing the relative path from one to another location, we\nneed to provide both locations as absolute paths to make sure the\ncomputation of the relative path is correct.\n\nWhile at it, change `real_work_tree` to be non const as we own\nthe memory.\n\nSigned-off-by: Stefan Beller \n---\n submodule.c | 12 +++++++-----\n 1 file changed, 7 insertions(+), 5 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 53a6dbb..c9d22e5 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1221,23 +1221,25 @@ void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir)\n {\n \tstruct strbuf file_name = STRBUF_INIT;\n \tstruct strbuf rel_path = STRBUF_INIT;\n-\tconst char *real_work_tree = xstrdup(real_path(work_tree));\n+\tchar *real_git_dir = xstrdup(real_path(git_dir));\n+\tchar *real_work_tree = xstrdup(real_path(work_tree));\n \n \t/* Update gitfile */\n \tstrbuf_addf(&file_name, \"%s/.git\", work_tree);\n \twrite_file(file_name.buf, \"gitdir: %s\",\n-\t\t relative_path(git_dir, real_work_tree, &rel_path));\n+\t\t relative_path(real_git_dir, real_work_tree, &rel_path));\n \n \t/* Update core.worktree setting */\n \tstrbuf_reset(&file_name);\n-\tstrbuf_addf(&file_name, \"%s/config\", git_dir);\n+\tstrbuf_addf(&file_name, \"%s/config\", real_git_dir);\n \tgit_config_set_in_file(file_name.buf, \"core.worktree\",\n-\t\t\t relative_path(real_work_tree, git_dir,\n+\t\t\t relative_path(real_work_tree, real_git_dir,\n \t\t\t\t\t &rel_path));\n \n \tstrbuf_release(&file_name);\n \tstrbuf_release(&rel_path);\n-\tfree((void *)real_work_tree);\n+\tfree(real_work_tree);\n+\tfree(real_git_dir);\n }\n \n int parallel_submodules(void)\n-- \n2.10.1.469.g00a8914\n\n"},{"id":"306006","messageId":"20161115230651.23953-5-sbeller@google.com","threadId":"44491","inReplyTo":"20161115230651.23953-1-sbeller@google.com","subject":"[PATCH 04/16] update submodules: add is_submodule_populated","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-11-15T23:06:39Z","receivedAt":"2016-11-15T23:13:35Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This is nearly same as Brandon sent out.\n(First patch of origin/bw/grep-recurse-submodules,\nwill drop this patch once Brandons series is stable\nenough to build on).\n\nSigned-off-by: Stefan Beller \n---\n submodule.c | 11 +++++++++++\n submodule.h | 1 +\n 2 files changed, 12 insertions(+)\n\ndiff --git a/submodule.c b/submodule.c\nindex c9d22e5..97eaf7c 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -914,6 +914,17 @@ int fetch_populated_submodules(const struct argv_array *options,\n \treturn spf.result;\n }\n \n+int is_submodule_populated(const char *path)\n+{\n+\tint retval = 0;\n+\tstruct strbuf gitdir = STRBUF_INIT;\n+\tstrbuf_addf(&gitdir, \"%s/.git\", path);\n+\tif (resolve_gitdir(gitdir.buf))\n+\t\tretval = 1;\n+\tstrbuf_release(&gitdir);\n+\treturn retval;\n+}\n+\n unsigned is_submodule_modified(const char *path, int ignore_untracked)\n {\n \tssize_t len;\ndiff --git a/submodule.h b/submodule.h\nindex afc58d0..d44b4f1 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -61,6 +61,7 @@ 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 extern unsigned is_submodule_modified(const char *path, int ignore_untracked);\n+extern int is_submodule_populated(const char *path);\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-- \n2.10.1.469.g00a8914\n\n"},{"id":"306007","messageId":"20161115230651.23953-3-sbeller@google.com","threadId":"44491","inReplyTo":"20161115230651.23953-1-sbeller@google.com","subject":"[PATCH 02/16] submodule: modernize ok_to_remove_submodule to use argv_array","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-11-15T23:06:37Z","receivedAt":"2016-11-15T23:14:19Z","isPatch":true,"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\nSigned-off-by: Stefan Beller \n---\n submodule.c | 10 ++--------\n 1 file changed, 2 insertions(+), 8 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 6f7d883..53a6dbb 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1022,13 +1022,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@@ -1038,7 +1031,8 @@ 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\", \"-uall\",\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-- \n2.10.1.469.g00a8914\n\n"},{"id":"306008","messageId":"20161115230651.23953-11-sbeller@google.com","threadId":"44491","inReplyTo":"20161115230651.23953-1-sbeller@google.com","subject":"[PATCH 10/16] update submodules: is_submodule_checkout_safe","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-11-15T23:06:45Z","receivedAt":"2016-11-15T23:14:27Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In later patches we introduce the options and flag for commands\nthat modify the working directory, e.g. git-checkout.\n\nThis piece of code will answer the question:\n\"Is it safe to change the submodule to this new state?\"\ne.g. is it overwriting untracked files or are there local\nchanges that would be overwritten?\n\nSigned-off-by: Stefan Beller \n---\n submodule.c | 22 ++++++++++++++++++++++\n submodule.h | 2 ++\n 2 files changed, 24 insertions(+)\n\ndiff --git a/submodule.c b/submodule.c\nindex 2773151..2149ef7 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1201,6 +1201,28 @@ int ok_to_remove_submodule(const char *path)\n \treturn ok_to_remove;\n }\n \n+int is_submodule_checkout_safe(const char *path, const struct object_id *oid)\n+{\n+\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\n+\tif (!is_submodule_populated(path))\n+\t\t/* The submodule is not populated, it's safe to check it out */\n+\t\t/*\n+\t\t * TODO: When git learns to re-populate submodules, a check must be\n+\t\t * added here to assert that no local files will be overwritten.\n+\t\t */\n+\t\treturn 1;\n+\n+\targv_array_pushl(&cp.args, \"read-tree\", \"-n\", \"-m\", \"HEAD\",\n+\t\t\t sha1_to_hex(oid->hash), NULL);\n+\n+\tprepare_submodule_repo_env(&cp.env_array);\n+\tcp.git_cmd = 1;\n+\tcp.no_stdin = 1;\n+\tcp.dir = path;\n+\treturn run_command(&cp) == 0;\n+}\n+\n static int find_first_merges(struct object_array *result, const char *path,\n \t\tstruct commit *a, struct commit *b)\n {\ndiff --git a/submodule.h b/submodule.h\nindex f01f87c..a7fa634 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -74,6 +74,8 @@ extern unsigned is_submodule_modified(const char *path, int ignore_untracked);\n extern int is_submodule_populated(const char *path);\n extern int submodule_uses_gitfile(const char *path);\n extern int ok_to_remove_submodule(const char *path);\n+extern int is_submodule_checkout_safe(const char *path,\n+\t\t\t\t const struct object_id *oid);\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.10.1.469.g00a8914\n\n"},{"id":"306009","messageId":"20161115230651.23953-6-sbeller@google.com","threadId":"44491","inReplyTo":"20161115230651.23953-1-sbeller@google.com","subject":"[PATCH 05/16] update submodules: add submodule config parsing","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-11-15T23:06:40Z","receivedAt":"2016-11-15T23:14:37Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Similar as in b33a15b08 (push: add recurseSubmodules config option,\n2015-11-17) and 027771fcb1 (submodule: allow erroneous values for the\nfetchRecurseSubmodules option, 2015-08-17), we add submodule-config code\nthat is later used to parse whether we are interested in updating\nsubmodules.\n\nWe need the `die_on_error` parameter to be able to call this parsing\nfunction for the config file as well, which if incorrect let's Git die.\n\nAs we're just touching the header file, also mark all functions extern.\n\nSigned-off-by: Stefan Beller \n---\n submodule-config.c | 22 ++++++++++++++++++++++\n submodule-config.h | 17 +++++++++--------\n 2 files changed, 31 insertions(+), 8 deletions(-)\n\ndiff --git a/submodule-config.c b/submodule-config.c\nindex 098085b..4b5297e 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -234,6 +234,28 @@ int parse_fetch_recurse_submodules_arg(const char *opt, const char *arg)\n \treturn parse_fetch_recurse(opt, arg, 1);\n }\n \n+static int parse_update_recurse(const char *opt, const char *arg,\n+\t\t\t\tint die_on_error)\n+{\n+\tswitch (git_config_maybe_bool(opt, arg)) {\n+\tcase 1:\n+\t\treturn RECURSE_SUBMODULES_ON;\n+\tcase 0:\n+\t\treturn RECURSE_SUBMODULES_OFF;\n+\tdefault:\n+\t\tif (!strcmp(arg, \"checkout\"))\n+\t\t\treturn RECURSE_SUBMODULES_ON;\n+\t\tif (die_on_error)\n+\t\t\tdie(\"bad %s argument: %s\", opt, arg);\n+\t\treturn RECURSE_SUBMODULES_ERROR;\n+\t}\n+}\n+\n+int parse_update_recurse_submodules_arg(const char *opt, const char *arg)\n+{\n+\treturn parse_update_recurse(opt, arg, 1);\n+}\n+\n static int parse_push_recurse(const char *opt, const char *arg,\n \t\t\t int die_on_error)\n {\ndiff --git a/submodule-config.h b/submodule-config.h\nindex d05c542..992bfbe 100644\n--- a/submodule-config.h\n+++ b/submodule-config.h\n@@ -22,13 +22,14 @@ struct submodule {\n \tint recommend_shallow;\n };\n \n-int parse_fetch_recurse_submodules_arg(const char *opt, const char *arg);\n-int parse_push_recurse_submodules_arg(const char *opt, const char *arg);\n-int parse_submodule_config_option(const char *var, const char *value);\n-const struct submodule *submodule_from_name(const unsigned char *commit_sha1,\n-\t\tconst char *name);\n-const struct submodule *submodule_from_path(const unsigned char *commit_sha1,\n-\t\tconst char *path);\n-void submodule_free(void);\n+extern int parse_fetch_recurse_submodules_arg(const char *opt, const char *arg);\n+extern int parse_update_recurse_submodules_arg(const char *opt, const char *arg);\n+extern int parse_push_recurse_submodules_arg(const char *opt, const char *arg);\n+extern int parse_submodule_config_option(const char *var, const char *value);\n+extern const struct submodule *submodule_from_name(\n+\t\tconst unsigned char *commit_sha1, const char *name);\n+extern const struct submodule *submodule_from_path(\n+\t\tconst unsigned char *commit_sha1, const char *path);\n+extern void submodule_free(void);\n \n #endif /* SUBMODULE_CONFIG_H */\n-- \n2.10.1.469.g00a8914\n\n"},{"id":"306010","messageId":"20161115230651.23953-16-sbeller@google.com","threadId":"44491","inReplyTo":"20161115230651.23953-1-sbeller@google.com","subject":"[PATCH 15/16] completion: add '--recurse-submodules' to checkout","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-11-15T23:06:50Z","receivedAt":"2016-11-15T23:14:42Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \n---\n contrib/completion/git-completion.bash | 2 +-\n t/t9902-completion.sh | 1 +\n 2 files changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 21016bf..28acfdb 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -1068,7 +1068,7 @@ _git_checkout ()\n \t--*)\n \t\t__gitcomp \"\n \t\t\t--quiet --ours --theirs --track --no-track --merge\n-\t\t\t--conflict= --orphan --patch\n+\t\t\t--conflict= --orphan --patch --recurse-submodules\n \t\t\t\"\n \t\t;;\n \t*)\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex 2ba62fb..d2d1102 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -447,6 +447,7 @@ test_expect_success 'double dash \"git checkout\"' '\n \t--conflict=\n \t--orphan Z\n \t--patch Z\n+\t--recurse-submodules Z\n \tEOF\n '\n \n-- \n2.10.1.469.g00a8914\n\n"},{"id":"306012","messageId":"20161115232049.GC66382@google.com","threadId":"44491","inReplyTo":"20161115230651.23953-5-sbeller@google.com","subject":"Re: [PATCH 04/16] update submodules: add is_submodule_populated","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-11-15T23:20:49Z","receivedAt":"2016-11-15T23:20:56Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 11/15, Stefan Beller wrote:\n> This is nearly same as Brandon sent out.\n> (First patch of origin/bw/grep-recurse-submodules,\n> will drop this patch once Brandons series is stable\n> enough to build on).\n\nI would be thrilled to see more reviews on that series :D\n\n-- \nBrandon Williams\n"},{"id":"306013","messageId":"20161115233404.GD66382@google.com","threadId":"44491","inReplyTo":"20161115230651.23953-8-sbeller@google.com","subject":"Re: [PATCH 07/16] update submodules: introduce submodule_is_interesting","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-11-15T23:34:04Z","receivedAt":"2016-11-15T23:34:14Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 11/15, Stefan Beller wrote:\n> +/**\n> + * When updating the working tree, do we need to check if the submodule needs\n> + * updating. We do not require a check if we are already sure that the\n> + * submodule doesn't need updating, e.g. when we are not interested in submodules\n> + * or the submodule is marked uninteresting by being not initialized.\n> + */\n\nThe first sentence seems a bit awkward. It seems like its worded as a\nquestion, maybe drop the 'do'?\n\n-- \nBrandon Williams\n"},{"id":"306014","messageId":"20161115234403.GE66382@google.com","threadId":"44491","inReplyTo":"20161115230651.23953-9-sbeller@google.com","subject":"Re: [PATCH 08/16] update submodules: add depopulate_submodule","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-11-15T23:44:03Z","receivedAt":"2016-11-15T23:44:10Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 11/15, Stefan Beller wrote:\n> Implement the functionality needed to enable work tree manipulating\n> commands to that a deleted submodule should not only affect the index\n\n\"to that a deleted\" did you mean \"so that a deleted\"\n\n> (leaving all the files of the submodule in the work tree) but also to\n> remove the work tree of the superproject (including any untracked\n> files).\n> \n> To do so, we need an equivalent of \"rm -rf\", which is already found in\n> entry.c, so expose that and for clarity add a suffix \"_or_dir\" to it.\n> \n> That will only work properly when the submodule uses a gitfile instead of\n> a .git directory and no untracked files are present. Otherwise the removal\n> will fail with a warning (which is just what happened until now).\n\nSo if a submodule uses a .git directory then it will be ignored during\nthe checkout? All other submodules will actually be removed? Couldn't\nyou end up in an undesirable state with a checkout effecting one\nsubmodule but not another?\n\n> diff --git a/cache.h b/cache.h\n> index a50a61a..65c47e4 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> +void remove_subtree_or_die(const char *path);\n> +\n> #endif /* CACHE_H */\n\nShould probably place an explicit 'extern' in the function prototype.\n\n> +int depopulate_submodule(const char *path)\n> +{\n> +\tint ret = 0;\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\twarning(_(\"cannot remove submodule '%s' because it (or one of \"\n> +\t\t\t \"its nested submodules) uses a .git directory\"),\n> +\t\t\t path);\n> +\t\tret = -1;\n> +\t\tgoto out;\n> +\t}\n> +\n> +\tremove_subtree_or_die(path);\n> +\n> +out:\n> +\tfree(dot_git);\n> +\treturn ret;\n> +}\n\n-- \nBrandon Williams\n"},{"id":"306016","messageId":"20161116000236.GF66382@google.com","threadId":"44491","inReplyTo":"20161115230651.23953-10-sbeller@google.com","subject":"Re: [PATCH 09/16] update submodules: add scheduling to update submodules","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-11-16T00:02:36Z","receivedAt":"2016-11-16T00:02:45Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 11/15, Stefan Beller wrote:\n> +static int update_submodule(const char *path, const struct object_id *oid,\n> +\t\t\t int force, int is_new)\n> +{\n> +\tconst char *git_dir;\n> +\tstruct child_process cp = CHILD_PROCESS_INIT;\n> +\tconst struct submodule *sub = submodule_from_path(null_sha1, path);\n> +\n> +\tif (!sub || !sub->name)\n> +\t\treturn -1;\n> +\n> +\tgit_dir = resolve_gitdir(git_common_path(\"modules/%s\", sub->name));\n> +\n> +\tif (!git_dir)\n> +\t\treturn -1;\n> +\n> +\tif (is_new)\n> +\t\tconnect_work_tree_and_git_dir(path, git_dir);\n\n> +\n> +\t/* update index via `read-tree --reset sha1` */\n> +\targv_array_pushl(&cp.args, \"read-tree\",\n> +\t\t\t\t force ? \"--reset\" : \"-m\",\n> +\t\t\t\t \"-u\", sha1_to_hex(oid->hash), NULL);\n> +\tprepare_submodule_repo_env(&cp.env_array);\n> +\tcp.git_cmd = 1;\n> +\tcp.no_stdin = 1;\n> +\tcp.dir = path;\n> +\tif (run_command(&cp)) {\n> +\t\twarning(_(\"reading the index in submodule '%s' failed\"), path);\n> +\t\tchild_process_clear(&cp);\n> +\t\treturn -1;\n> +\t}\n> +\n> +\t/* write index to working dir */\n> +\tchild_process_clear(&cp);\n> +\tchild_process_init(&cp);\n> +\targv_array_pushl(&cp.args, \"checkout-index\", \"-a\", NULL);\n> +\tcp.git_cmd = 1;\n> +\tcp.no_stdin = 1;\n> +\tcp.dir = path;\n> +\tif (force)\n> +\t\targv_array_push(&cp.args, \"-f\");\n> +\n> +\tif (run_command(&cp)) {\n> +\t\twarning(_(\"populating the working directory in submodule '%s' failed\"), path);\n> +\t\tchild_process_clear(&cp);\n> +\t\treturn -1;\n> +\t}\n> +\n> +\t/* get the HEAD right */\n> +\tchild_process_clear(&cp);\n> +\tchild_process_init(&cp);\n> +\targv_array_pushl(&cp.args, \"checkout\", \"--recurse-submodules\", NULL);\n> +\tcp.git_cmd = 1;\n> +\tcp.no_stdin = 1;\n> +\tcp.dir = path;\n> +\tif (force)\n> +\t\targv_array_push(&cp.args, \"-f\");\n> +\targv_array_push(&cp.args, sha1_to_hex(oid->hash));\n> +\n> +\tif (run_command(&cp)) {\n> +\t\twarning(_(\"setting the HEAD in submodule '%s' failed\"), path);\n> +\t\tchild_process_clear(&cp);\n> +\t\treturn -1;\n> +\t}\n> +\n> +\tchild_process_clear(&cp);\n> +\treturn 0;\n> +}\n\nIf run command is successful then it handles the clearing of the child\nprocess struct, correct? Is there a negative to having all the explicit\nclears when the child was successful?\n\n> +\n> int depopulate_submodule(const char *path)\n> {\n> \tint ret = 0;\n> @@ -1336,3 +1405,51 @@ void prepare_submodule_repo_env(struct argv_array *out)\n> \t}\n> \targv_array_push(out, \"GIT_DIR=.git\");\n> }\n> +\n> +struct scheduled_submodules_update_type {\n> +\tconst char *path;\n> +\tconst struct object_id *oid;\n> +\t/*\n> +\t * Do we need to perform a complete checkout or just incremental\n> +\t * update?\n> +\t */\n> +\tunsigned is_new:1;\n> +} *scheduled_submodules;\n> +#define SCHEDULED_SUBMODULES_INIT {NULL, NULL}\n\nI may not know enough about these types of initializors but that Init\nmacro only has 2 entries while there are three entries in the struct\nitself.\n\n> +\n> +int scheduled_submodules_nr, scheduled_submodules_alloc;\n\nShould these globals be static since they should be scoped to only this\nfile?\n\n> +\n> +void schedule_submodule_for_update(const struct cache_entry *ce, int is_new)\n> +{\n> +\tstruct scheduled_submodules_update_type *ssu;\n> +\tALLOC_GROW(scheduled_submodules,\n> +\t\t scheduled_submodules_nr + 1,\n> +\t\t scheduled_submodules_alloc);\n> +\tssu = &scheduled_submodules[scheduled_submodules_nr++];\n> +\tssu->path = ce->name;\n> +\tssu->oid = &ce->oid;\n> +\tssu->is_new = !!is_new;\n> +}\n> +\n> +int update_submodules(int force)\n> +{\n> +\tint i;\n> +\tgitmodules_config();\n> +\n> +\t/*\n> +\t * NEEDSWORK: As submodule updates can potentially take some\n> +\t * time each and they do not overlap (i.e. no d/f conflicts;\n> +\t * this can be parallelized using the run_commands.h API.\n> +\t */\n> +\tfor (i = 0; i < scheduled_submodules_nr; i++) {\n> +\t\tstruct scheduled_submodules_update_type *ssu =\n> +\t\t\t&scheduled_submodules[i];\n> +\n> +\t\tif (submodule_is_interesting(ssu->path, null_sha1))\n> +\t\t\tupdate_submodule(ssu->path, ssu->oid,\n> +\t\t\t\t\t force, ssu->is_new);\n> +\t}\n> +\n> +\tscheduled_submodules_nr = 0;\n> +\treturn 0;\n> +}\n\nnit: organization wise it makes more sense to me to have the\n'update_submodule' helper function be located more closely to the\n'update_submodules' function.\n\n-- \nBrandon Williams\n"},{"id":"306018","messageId":"20161116000621.GG66382@google.com","threadId":"44491","inReplyTo":"20161115230651.23953-11-sbeller@google.com","subject":"Re: [PATCH 10/16] update submodules: is_submodule_checkout_safe","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-11-16T00:06:21Z","receivedAt":"2016-11-16T00:06:28Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 11/15, Stefan Beller wrote:\n> In later patches we introduce the options and flag for commands\n> that modify the working directory, e.g. git-checkout.\n> \n> This piece of code will answer the question:\n> \"Is it safe to change the submodule to this new state?\"\n> e.g. is it overwriting untracked files or are there local\n> changes that would be overwritten?\n> \n> Signed-off-by: Stefan Beller \n> ---\n> submodule.c | 22 ++++++++++++++++++++++\n> submodule.h | 2 ++\n> 2 files changed, 24 insertions(+)\n> \n> diff --git a/submodule.c b/submodule.c\n> index 2773151..2149ef7 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -1201,6 +1201,28 @@ int ok_to_remove_submodule(const char *path)\n> \treturn ok_to_remove;\n> }\n> \n> +int is_submodule_checkout_safe(const char *path, const struct object_id *oid)\n> +{\n> +\tstruct child_process cp = CHILD_PROCESS_INIT;\n> +\n> +\tif (!is_submodule_populated(path))\n> +\t\t/* The submodule is not populated, it's safe to check it out */\n> +\t\t/*\n> +\t\t * TODO: When git learns to re-populate submodules, a check must be\n> +\t\t * added here to assert that no local files will be overwritten.\n> +\t\t */\n\nWhen you mean local files do you mean in the situation where we want to\ncheckout a submodule at path 'foo' but there already exists a file at\npath 'foo'?\n\n> +\t\treturn 1;\n> +\n> +\targv_array_pushl(&cp.args, \"read-tree\", \"-n\", \"-m\", \"HEAD\",\n> +\t\t\t sha1_to_hex(oid->hash), NULL);\n> +\n> +\tprepare_submodule_repo_env(&cp.env_array);\n> +\tcp.git_cmd = 1;\n> +\tcp.no_stdin = 1;\n> +\tcp.dir = path;\n> +\treturn run_command(&cp) == 0;\n> +}\n> +\n> static int find_first_merges(struct object_array *result, const char *path,\n> \t\tstruct commit *a, struct commit *b)\n> {\n> diff --git a/submodule.h b/submodule.h\n> index f01f87c..a7fa634 100644\n> --- a/submodule.h\n> +++ b/submodule.h\n> @@ -74,6 +74,8 @@ extern unsigned is_submodule_modified(const char *path, int ignore_untracked);\n> extern int is_submodule_populated(const char *path);\n> extern int submodule_uses_gitfile(const char *path);\n> extern int ok_to_remove_submodule(const char *path);\n> +extern int is_submodule_checkout_safe(const char *path,\n> +\t\t\t\t const struct object_id *oid);\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> -- \n> 2.10.1.469.g00a8914\n> \n\n-- \nBrandon Williams\n"},{"id":"306020","messageId":"7d6c85faf2f94a65947c65bad350532a@exmbdft7.ad.twosigma.com","threadId":"44491","inReplyTo":"20161116000236.GF66382@google.com","subject":"RE: [PATCH 09/16] update submodules: add scheduling to update submodules","fromName":"David Turner","fromEmail":"david.turner@twosigma.com","sentAt":"2016-11-16T00:07:34Z","receivedAt":"2016-11-16T00:07:41Z","isPatch":true,"sender":{"key":"david.turner@twosigma.com","avatar":null},"body":"> -----Original Message-----\n> From: Brandon Williams [mailto:bmwill@google.com]\n> > +struct scheduled_submodules_update_type {\n> > +\tconst char *path;\n> > +\tconst struct object_id *oid;\n> > +\t/*\n> > +\t * Do we need to perform a complete checkout or just incremental\n> > +\t * update?\n> > +\t */\n> > +\tunsigned is_new:1;\n> > +} *scheduled_submodules;\n> > +#define SCHEDULED_SUBMODULES_INIT {NULL, NULL}\n> \n> I may not know enough about these types of initializors but that Init\n> macro only has 2 entries while there are three entries in the struct\n> itself.\n\nIn fact, only the first NULL is necessary; unspecified initializer entries in C default to zero.\n\n"},{"id":"306022","messageId":"20161116001402.GH66382@google.com","threadId":"44491","inReplyTo":"20161115230651.23953-12-sbeller@google.com","subject":"Re: [PATCH 11/16] teach unpack_trees() to remove submodule contents","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-11-16T00:14:02Z","receivedAt":"2016-11-16T00:14:23Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 11/15, Stefan Beller wrote:\n> Extend rmdir_or_warn() to remove the directories of those submodules which\n> are scheduled for removal. Also teach verify_clean_submodule() to check\n> that a submodule configured to be removed is not modified before scheduling\n> it for removal.\n> \n> Signed-off-by: Stefan Beller \n> ---\n> unpack-trees.c | 6 ++----\n> wrapper.c | 4 ++++\n> 2 files changed, 6 insertions(+), 4 deletions(-)\n> \n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index ea6bdd2..576e1d5 100644\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -9,6 +9,7 @@\n> #include \"refs.h\"\n> #include \"attr.h\"\n> #include \"split-index.h\"\n> +#include \"submodule.h\"\n> #include \"dir.h\"\n> \n> /*\n> @@ -1361,15 +1362,12 @@ static void invalidate_ce_path(const struct cache_entry *ce,\n> /*\n> * Check that checking out ce->sha1 in subdir ce->name is not\n> * going to overwrite any working files.\n> - *\n> - * Currently, git does not checkout subprojects during a superproject\n> - * checkout, so it is not going to overwrite anything.\n> */\n> static int verify_clean_submodule(const struct cache_entry *ce,\n> \t\t\t\t enum unpack_trees_error_types error_type,\n> \t\t\t\t struct unpack_trees_options *o)\n> {\n> -\treturn 0;\n> +\treturn submodule_is_interesting(ce->name, null_sha1) && is_submodule_modified(ce->name, 0);\n> }\n\nSo what does the return value from this function meant to mean? Is '1'\nmean the submodule is clean while '0' indicates it is dirty or is it the\nreverse of that? Reading this it seems to me a value of '1' means \"yes\nthe submodule is clean!\" but the way the return value is calculated\ntells a different story. Either I'm understanding it incorrectly or I\nthink the return should be something like this:\n\n return submodule_is_interesting(ce->name, null_sha1) && !is_submodule_modified(ce->name, 0);\n\nWhere we return '1' if the submodule is interesting and it hasn't been\nmodified.\n\n> \n> static int verify_clean_subdirectory(const struct cache_entry *ce,\n> diff --git a/wrapper.c b/wrapper.c\n> index e7f1979..17c08de 100644\n> --- a/wrapper.c\n> +++ b/wrapper.c\n> @@ -2,6 +2,7 @@\n> * Various trivial helper wrappers around standard functions\n> */\n> #include \"cache.h\"\n> +#include \"submodule.h\"\n> \n> static void do_nothing(size_t size)\n> {\n> @@ -592,6 +593,9 @@ int unlink_or_warn(const char *file)\n> \n> int rmdir_or_warn(const char *file)\n> {\n> +\tif (submodule_is_interesting(file, null_sha1)\n> +\t && depopulate_submodule(file))\n> +\t\treturn -1;\n> \treturn warn_if_unremovable(\"rmdir\", file, rmdir(file));\n> }\n\nIt seems weird to me that rmdir is doing checks to see if the file being\nremoved is a submodule. Shouldn't those checks have occurred before\ncalling rmdir?\n\n-- \nBrandon Williams\n"},{"id":"306023","messageId":"e748abfad8b04a8eaaa10797d9324891@exmbdft7.ad.twosigma.com","threadId":"44491","inReplyTo":"20161115230651.23953-8-sbeller@google.com","subject":"RE: [PATCH 07/16] update submodules: introduce submodule_is_interesting","fromName":"David Turner","fromEmail":"david.turner@twosigma.com","sentAt":"2016-11-16T00:14:05Z","receivedAt":"2016-11-16T00:14:24Z","isPatch":true,"sender":{"key":"david.turner@twosigma.com","avatar":null},"body":"> -----Original Message-----\n> From: Stefan Beller [mailto:sbeller@google.com]\n> Sent: Tuesday, November 15, 2016 6:07 PM\n> Cc: git@vger.kernel.org; bmwill@google.com; gitster@pobox.com;\n> jrnieder@gmail.com; mogulguy10@gmail.com; David Turner; Stefan Beller\n> Subject: [PATCH 07/16] update submodules: introduce\n> submodule_is_interesting\n> +int submodule_is_interesting(const char *path, const unsigned char\n> +*sha1) {\n\nThis is apparently only ever (in this series) called with null_sha1. So either this arg is unnecessary, or there are bugs elsewhere in the code.\n"},{"id":"306026","messageId":"f54d446aa7734cb4aec4b51c7b81a2b6@exmbdft7.ad.twosigma.com","threadId":"44491","inReplyTo":"20161115230651.23953-14-sbeller@google.com","subject":"RE: [PATCH 13/16] submodule: teach unpack_trees() to update submodules","fromName":"David Turner","fromEmail":"david.turner@twosigma.com","sentAt":"2016-11-16T00:22:21Z","receivedAt":"2016-11-16T00:22:45Z","isPatch":true,"sender":{"key":"david.turner@twosigma.com","avatar":null},"body":"[I've reviewed up-to and including 13; I'll look at 14-16 tomorrow-ish]\n\n> -----Original Message-----\n> From: Stefan Beller [mailto:sbeller@google.com]\n> Sent: Tuesday, November 15, 2016 6:07 PM\n> Cc: git@vger.kernel.org; bmwill@google.com; gitster@pobox.com;\n> jrnieder@gmail.com; mogulguy10@gmail.com; David Turner; Stefan Beller\n> Subject: [PATCH 13/16] submodule: teach unpack_trees() to update\n> submodules\n...\n> \tmsgs[ERROR_NOT_UPTODATE_DIR] =\n> \t\t_(\"Updating the following directories would lose untracked\n> files in it:\\n%s\");\n> +\tmsgs[ERROR_NOT_UPTODATE_SUBMODULE] =\n> +\t\t_(\"Updating the following submodules would lose modifications\n> in\n> +it:\\n%s\");\n\ns/it/them/\n\n> \tif (!strcmp(cmd, \"checkout\"))\n> \t\tmsg = advice_commit_before_merge\n> @@ -1315,19 +1320,18 @@ static int verify_uptodate_1(const struct\n> cache_entry *ce,\n> \t\treturn 0;\n> \n> \tif (!lstat(ce->name, &st)) {\n> -\t\tint flags =\n> CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE;\n> -\t\tunsigned changed = ie_match_stat(o->src_index, ce, &st,\n> flags);\n> -\t\tif (!changed)\n> -\t\t\treturn 0;\n> -\t\t/*\n> -\t\t * NEEDSWORK: the current default policy is to allow\n> -\t\t * submodule to be out of sync wrt the superproject\n> -\t\t * index. This needs to be tightened later for\n> -\t\t * submodules that are marked to be automatically\n> -\t\t * checked out.\n> -\t\t */\n> -\t\tif (S_ISGITLINK(ce->ce_mode))\n> -\t\t\treturn 0;\n> +\t\tif (!S_ISGITLINK(ce->ce_mode)) {\n\nI generally prefer to avoid if (!x) { A } else { B } -- I would rather just see if (x) { B } else { A }.\n\n> +\t\tif (!changed) {\n> +\t\t\t/* old is always a submodule */\n> +\t\t\tif (S_ISGITLINK(new->ce_mode)) {\n> +\t\t\t\t/*\n> +\t\t\t\t * new is also a submodule, so check if we care\n> +\t\t\t\t * and then if can checkout the new sha1 safely\n> +\t\t\t\t */\n> +\t\t\t\tif (submodule_is_interesting(old->name, null_sha1)\n> +\t\t\t\t && is_submodule_checkout_safe(new->name, &new-\n> >oid))\n> +\t\t\t\t\treturn 0;\n> +\t\t\t} else {\n> +\t\t\t\t/*\n> +\t\t\t\t * new is not a submodule any more, so only\n> +\t\t\t\t * care if we care:\n> +\t\t\t\t */\n> +\t\t\t\tif (submodule_is_interesting(old->name, null_sha1)\n> +\t\t\t\t && ok_to_remove_submodule(old->name))\n> +\t\t\t\t\treturn 0;\n> +\t\t\t}\n\nDo we need a return 1 in here somewhere? Because otherwise, we fall through and return 0 later.\n\n"},{"id":"306027","messageId":"20161116002520.GI66382@google.com","threadId":"44491","inReplyTo":"20161115230651.23953-14-sbeller@google.com","subject":"Re: [PATCH 13/16] submodule: teach unpack_trees() to update submodules","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-11-16T00:25:20Z","receivedAt":"2016-11-16T00:25:26Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 11/15, Stefan Beller wrote:\n> +\t\tif (!S_ISGITLINK(ce->ce_mode)) {\n> +\t\t\tint flags = CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE;\n\nFor readability you may want to have spaces between the two flags\n\n> +\tif (o->index_only\n> +\t || (!((old->ce_flags & CE_VALID) || ce_skip_worktree(old))\n> +\t\t&& (o->reset || ce_uptodate(old))))\n> +\t\treturn 0;\n\nThe coding guidelines say that git prefers to have the logical operators\non the right side like this:\n\nif (o->index_only ||\n (!((old->ce_flags & CE_VALID) || ce_skip_worktree(old))\t&&\n (o->reset || ce_uptodate(old))))\n\treturn 0;\n\nIt also says the other way is ok too, just a thought :)\n\n> +\t\t\t\tif (submodule_is_interesting(old->name, null_sha1)\n> +\t\t\t\t && is_submodule_checkout_safe(new->name, &new->oid))\n> +\t\t\t\t\treturn 0;\n\nhere too.\n\n> +\t\t\t} else {\n> +\t\t\t\t/*\n> +\t\t\t\t * new is not a submodule any more, so only\n> +\t\t\t\t * care if we care:\n> +\t\t\t\t */\n> +\t\t\t\tif (submodule_is_interesting(old->name, null_sha1)\n> +\t\t\t\t && ok_to_remove_submodule(old->name))\n> +\t\t\t\t\treturn 0;\n\nand here\n\n-- \nBrandon Williams\n"},{"id":"306028","messageId":"20161116003347.GJ66382@google.com","threadId":"44491","inReplyTo":"20161115230651.23953-15-sbeller@google.com","subject":"Re: [PATCH 14/16] checkout: recurse into submodules if asked to","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-11-16T00:33:47Z","receivedAt":"2016-11-16T00:33:55Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 11/15, Stefan Beller wrote:\n> +int option_parse_recurse_submodules(const struct option *opt,\n> +\t\t\t\t const char *arg, int unset)\n> +{\n> +\tif (unset) {\n> +\t\trecurse_submodules = RECURSE_SUBMODULES_OFF;\n> +\t\treturn 0;\n> +\t}\n> +\tif (arg)\n> +\t\trecurse_submodules =\n> +\t\t\tparse_update_recurse_submodules_arg(opt->long_name,\n> +\t\t\t\t\t\t\t arg);\n> +\telse\n> +\t\trecurse_submodules = RECURSE_SUBMODULES_ON;\n> +\n> +\treturn 0;\n> +}\n\nI assume it is ok to always return 0 from this function? Also, I know\nwe don't like having braces on single statement if's but it is a bit\nhard to read that multi-line statement without braces. But that's just\nmy opinion :)\n\n\n-- \nBrandon Williams\n"},{"id":"306060","messageId":"a9c85966bf174509a246106d782c36a3@exmbdft7.ad.twosigma.com","threadId":"44491","inReplyTo":"20161115230651.23953-15-sbeller@google.com","subject":"RE: [PATCH 14/16] checkout: recurse into submodules if asked to","fromName":"David Turner","fromEmail":"david.turner@twosigma.com","sentAt":"2016-11-16T17:03:30Z","receivedAt":"2016-11-16T17:03:37Z","isPatch":true,"sender":{"key":"david.turner@twosigma.com","avatar":null},"body":"\n\n> -----Original Message-----\n> From: Stefan Beller [mailto:sbeller@google.com]\n> \n> diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh index\n> 79cdd34..e0773c6 100755\n> --- a/t/lib-submodule-update.sh\n> +++ b/t/lib-submodule-update.sh\n> @@ -634,7 +634,13 @@ test_submodule_forced_switch () {\n> \n> \t########################## Modified submodule\n> #########################\n> \t# Updating a submodule sha1 doesn't update the submodule's work tree\n> -\ttest_expect_success \"$command: modified submodule does not update\n> submodule work tree\" '\n> +\tif test\n> \"$KNOWN_FAILURE_RECURSE_SUBMODULE_SERIES_BREAKS_REPLACE_SUBMODULE_TEST\" =\n> 1\n> +\tthen\n> +\t\tRESULT=\"failure\"\n> +\telse\n> +\t\tRESULT=\"success\"\n> +\tfi\n> +\ttest_expect_$RESULT \"$command: modified submodule does not update\n> submodule work tree\" '\n\nWhy does this break? I thought it was only if checkout is run with --recurse-submodules that anything should change?\n\n> +test_expect_success 'dirty file file is not deleted' '\n\nDuplicate 'file' in this test name.\n> +# This is ok in theory, we just need to make sure # the garbage\n> +collection doesn't eat the commit.\n> +test_expect_success 'different commit prevents from deleting' '\n\nThis isn't a different commit -- it's a dirty index, right? \n\n> +test_expect_failure '\"checkout --recurse-submodules\" does not care about\n> untracked submodule content' '\n> +\techo untracked >submodule/untracked &&\n> +\tgit checkout --recurse-submodules master &&\n> +\tgit diff-files --quiet --ignore-submodules=untracked &&\n> +\tgit diff-index --quiet --cached HEAD &&\n> +\trm submodule/untracked\n> +'\n\nUse test_when_finished for cleanup.\n\n> +test_expect_failure '\"checkout --recurse-submodules\" needs -f when\n> submodule commit is not present (but does fail anyway)' '\n> +\tgit checkout --recurse-submodules -b bogus_commit master &&\n> +\tgit update-index --cacheinfo 160000\n> 0123456789012345678901234567890123456789 submodule &&\n> +\tBOGUS_TREE=$(git write-tree) &&\n> +\tBOGUS_COMMIT=$(echo \"bogus submodule commit\" | git commit-tree\n> $BOGUS_TREE) &&\n> +\tgit commit -m \"bogus submodule commit\" &&\n> +\tgit checkout --recurse-submodules -f master &&\n> +\ttest_must_fail git checkout --recurse-submodules bogus_commit &&\n> +\tgit diff-files --quiet &&\n> +\ttest_must_fail git checkout --recurse-submodules -f bogus_commit &&\n> +\ttest_must_fail git diff-files --quiet submodule &&\n> +\tgit diff-files --quiet file &&\n> +\tgit diff-index --quiet --cached HEAD &&\n> +\tgit checkout --recurse-submodules -f master '\n> +KNOWN_FAILURE_RECURSE_SUBMODULE_SERIES_BREAKS_REPLACE_SUBMODULE_TEST=1\n> test_submodule_switch \"git checkout\"\n> \n> +KNOWN_FAILURE_RECURSE_SUBMODULE_SERIES_BREAKS_REPLACE_SUBMODULE_TEST=\n> test_submodule_forced_switch \"git checkout -f\"\n> \n> test_done\n> --\n> 2.10.1.469.g00a8914\n\n"},{"id":"306061","messageId":"d9b50f01144f4c9a99b42230d4216222@exmbdft7.ad.twosigma.com","threadId":"44491","inReplyTo":"20161115230651.23953-15-sbeller@google.com","subject":"RE: [PATCH 14/16] checkout: recurse into submodules if asked to","fromName":"David Turner","fromEmail":"david.turner@twosigma.com","sentAt":"2016-11-16T17:05:55Z","receivedAt":"2016-11-16T17:06:01Z","isPatch":true,"sender":{"key":"david.turner@twosigma.com","avatar":null},"body":"Sorry, my previous message accidentally sent before I was done. One more comment:\n\n> -----Original Message-----\n> From: Stefan Beller [mailto:sbeller@google.com]\n\n> +test_expect_failure '\"checkout --recurse-submodules\" needs -f to update\n> modifed submodule content' '\n> +\techo modified >submodule/second.t &&\n> +\ttest_must_fail git checkout --recurse-submodules HEAD^ &&\n> +\ttest_must_fail git diff-files --quiet submodule &&\n> +\tgit diff-files --quiet file &&\n> +\tgit checkout --recurse-submodules -f HEAD^ &&\n> +\tgit diff-files --quiet &&\n> +\tgit diff-index --quiet --cached HEAD &&\n> +\tgit checkout --recurse-submodules -f master &&\n> +\tgit diff-files --quiet &&\n> +\tgit diff-index --quiet --cached HEAD\n> +'\n\nIt might be worth adding some comments explaining why you expect these to fail. \n\n"},{"id":"306074","messageId":"xmqqwpg3p7rm.fsf@gitster.mtv.corp.google.com","threadId":"44491","inReplyTo":"e19fbbca91d5446e8fe308e847f53ae3@exmbdft7.ad.twosigma.com","subject":"Re: [PATCH 02/16] submodule: modernize ok_to_remove_submodule to use argv_array","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-16T19:03:09Z","receivedAt":"2016-11-16T19:03:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Turner writes:\n\n>> -\t\t\"-u\",\n> ...\n>> +\targv_array_pushl(&cp.args, \"status\", \"--porcelain\", \"-uall\",\n>\n> This also changes -u to -uall, which is not mentioned in the\n> commit message. That should probably be called out.\n\nOr not making that change at all. Isn't \"-u\" the same as \"-uall\"?\n"},{"id":"306075","messageId":"xmqqshqrp7i8.fsf@gitster.mtv.corp.google.com","threadId":"44491","inReplyTo":"20161115230651.23953-2-sbeller@google.com","subject":"Re: [PATCH 01/16] submodule.h: add extern keyword to functions, break line before 80","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-16T19:08:47Z","receivedAt":"2016-11-16T19:08:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> submodule.h: add extern keyword to functions, break line before 80\n\nThe former is probably a good change for consistency. As the latter\nchange breaks a workflow around quickly checking the output from\n\"git grep funcname \\*.h\", I am not sure if it is a good idea.\n\nEspecially things like this look like a usability regression:\n\n-void handle_ignore_submodules_arg(struct diff_options *diffopt, const char *);\n+extern void handle_ignore_submodules_arg(struct diff_options *diffopt,\n+\t\t\t\t\t const char *);\n\nPerhaps the name \"diffopt\" can be dropped from there if we want it\nto be shorter. Names in prototypes are valuable for parameters of\nmore generic types like \"char *\", especially when there are more\nthan one parameters of the same type, but in this case the typename\nis specific enough for readers to tell what it is.\n\n"},{"id":"306107","messageId":"CAGZ79kbVq0SrD8u+GrGAVYpvNeirDqnAU9jJCbSLoEvqSgB4yg@mail.gmail.com","threadId":"44491","inReplyTo":"xmqqshqrp7i8.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 01/16] submodule.h: add extern keyword to functions, break line before 80","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-11-17T18:29:39Z","receivedAt":"2016-11-17T18:29:46Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Nov 16, 2016 at 11:08 AM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> submodule.h: add extern keyword to functions, break line before 80\n>\n> The former is probably a good change for consistency. As the latter\n> change breaks a workflow around quickly checking the output from\n> \"git grep funcname \\*.h\", I am not sure if it is a good idea.\n>\n> Especially things like this look like a usability regression:\n>\n> -void handle_ignore_submodules_arg(struct diff_options *diffopt, const char *);\n> +extern void handle_ignore_submodules_arg(struct diff_options *diffopt,\n> + const char *);\n>\n> Perhaps the name \"diffopt\" can be dropped from there if we want it\n> to be shorter.\n\nDoes the Git community have an opinion on dropping the name?\nUsually the meaning can be inferred from the type such as in this example.\n\"diffopt\" doesn't add information to my understanding when looking at\nthe header file.\n\nOne (cherry-picked) counter example is:\n\n /**\n * Like `strbuf_getwholeline`, but operates on a file descriptor.\n * It reads one character at a time, so it is very slow. Do not\n * use it unless you need the correct position in the file\n * descriptor.\n */\n extern int strbuf_getwholeline_fd(struct strbuf *, int, int);\n\nwhere I'd need a bit of time to figure out which of the 2 ints is the\nfd and which is the line termination character.\n\nSo I'd rather have a broken line than dropping names,\nas when grepping for names I'd look them up most of the time\nanyway (to e.g. read additional documentation or just reading the\nsource)\n\n\n> Names in prototypes are valuable for parameters of\n> more generic types like \"char *\", especially when there are more\n> than one parameters of the same type, but in this case the typename\n> is specific enough for readers to tell what it is.\n\nOk, that seems a reasonable to me.\n\nI'll squash the changes below, with an updated commit message:\n\nsubmodule.h: add extern keyword to functions\n\nAs 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\n(diff on top of the patch under discussion)\ndiff --git a/submodule.h b/submodule.h\nindex afc58d0..2082847 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -33,17 +33,14 @@ 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(\n- struct diff_options *diffopt,\n+extern void set_diffopt_flags_from_submodule_config(struct diff_options *,\n const char *path);\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 struct submodule_update_strategy *dst);\n-extern const char *submodule_strategy_to_string(\n- const struct submodule_update_strategy *s);\n-extern void handle_ignore_submodules_arg(struct diff_options *diffopt,\n- const char *);\n+extern const char *submodule_strategy_to_string(const struct\nsubmodule_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 const char *line_prefix,\n struct object_id *one, struct object_id *two,\n@@ -72,8 +69,7 @@ extern int find_unpushed_submodules(unsigned char\nnew_sha1[20],\n struct string_list *needs_pushing);\n extern int push_unpushed_submodules(unsigned char new_sha1[20],\n const char *remotes_name);\n-extern void connect_work_tree_and_git_dir(const char *work_tree,\n- const char *git_dir);\n+extern void connect_work_tree_and_git_dir(const char *work_tree,\nconst char *git_dir);\n extern int parallel_submodules(void);\n\n /*\n"},{"id":"306109","messageId":"CAGZ79kbStcBguwuoTD4bXch4NmAaSLuEPTLW_hGM=PLe2g9upQ@mail.gmail.com","threadId":"44491","inReplyTo":"xmqqwpg3p7rm.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 02/16] submodule: modernize ok_to_remove_submodule to use argv_array","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-11-17T18:36:19Z","receivedAt":"2016-11-17T18:41:28Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Nov 16, 2016 at 11:03 AM, Junio C Hamano wrote:\n> David Turner writes:\n>\n>>> - \"-u\",\n>> ...\n>>> + argv_array_pushl(&cp.args, \"status\", \"--porcelain\", \"-uall\",\n>>\n>> This also changes -u to -uall, which is not mentioned in the\n>> commit message. That should probably be called out.\n>\n> Or not making that change at all. Isn't \"-u\" the same as \"-uall\"?\n\nYes it is. My original line of thinking was to have it spelled out\nclearly in case\nit changes in the future, but then we could argue that the --porcelain parameter\nought to keep the default of -u to \"all\".\n\nI'll undo that change then.\n\nThanks,\nStefan\n"},{"id":"306120","messageId":"CAGZ79kbs1ymw7mE6ctGt2_wgHoFSYMbuhcFK_3vGR9tYZzJxOg@mail.gmail.com","threadId":"44491","inReplyTo":"e748abfad8b04a8eaaa10797d9324891@exmbdft7.ad.twosigma.com","subject":"Re: [PATCH 07/16] update submodules: introduce submodule_is_interesting","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-11-17T20:03:41Z","receivedAt":"2016-11-17T20:03:48Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Nov 15, 2016 at 4:14 PM, David Turner wrote:\n\n>> +int submodule_is_interesting(const char *path, const unsigned char\n>> +*sha1) {\n>\n> This is apparently only ever (in this series) called with null_sha1. So either this arg is unnecessary, or there are bugs elsewhere in the code.\n\nI was torn when writing the series, as I initially had submodule_is_interesting\nwith no sha1 argument and it turned out to be buggy in my first\ninitial implementation,\nwhich lead me to thinking the sha1 actually matters.\n\nThe line of thinking was similar to loading the submodules from the\nsubmodule-config cache as that also has different values for different sha1s,\ne.g. a submodule is only interesting if submodule..update != none,\nwhich can have changed with different sha1s.\n\nI refactored the series since then to call the _is_initeresting method\nat different times\n(before and after the actual checkout), such that we implicitly have\nthe correct sha1\nwhile calling it.\n\nSo I would argue the sha1 argument is not needed. I'll remove it.\n"},{"id":"306121","messageId":"CAGZ79kYEdA6crWTf3ZW2EthenZA4=PeWd4HfKnCAKxgZoz54qg@mail.gmail.com","threadId":"44491","inReplyTo":"20161117105715.GC39230@book.hvoigt.net","subject":"Re: [PATCH 07/16] update submodules: introduce submodule_is_interesting","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-11-17T20:08:12Z","receivedAt":"2016-11-17T20:08:19Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Nov 17, 2016 at 2:57 AM, Heiko Voigt wrote:\n\n> It seems that you are only looking at the submodule config from a\n> commit. Should a user be able to override this with local configuration?\n> Haven't looked further in the patchseries so maybe that is somewhere\n> else?\n\nIt turns out that in later patches we pass in null_sha1 only, which is\nlooking at the config and possible overrides.\n\nI'll refactor to take no sha1 argument and use null_sha1 here directly.\n"},{"id":"306129","messageId":"CAGZ79kbCqLsRzrsX29uM7pobs_11UZtFOQWP9RO8ptS5PyDfmw@mail.gmail.com","threadId":"44491","inReplyTo":"20161115234403.GE66382@google.com","subject":"Re: [PATCH 08/16] update submodules: add depopulate_submodule","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-11-17T22:23:29Z","receivedAt":"2016-11-17T22:23:41Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Nov 15, 2016 at 3:44 PM, Brandon Williams wrote:\n> \"to that a deleted\" did you mean \"so that a deleted\"\n\ndone\n\n>> That will only work properly when the submodule uses a gitfile instead of\n>> a .git directory and no untracked files are present. Otherwise the removal\n>> will fail with a warning (which is just what happened until now).\n>\n> So if a submodule uses a .git directory then it will be ignored during\n> the checkout?\n\nWell first you get the warning:\n\n \"cannot remove submodule '%s' because it (or one of \"\n \"its nested submodules) uses a .git directory\"),\n\nand in case a d/f/ conflict arises in a later stage (e.g. when the submodule\nis replaced by a file or symlink), you get another related error with\nless helpful description how to debug it.\n\n> All other submodules will actually be removed? Couldn't\n> you end up in an undesirable state with a checkout effecting one\n> submodule but not another?\n\nYes you could. Maybe it's time to add\n\"git submodule intern-git-dir\", which can be given as a helpful hint\nor even run here first.\n\n>\n> Should probably place an explicit 'extern' in the function prototype.\n\ndone\n"},{"id":"306131","messageId":"CAGZ79kZLqVg=FTiF+tOCSrqca_5zQoZHqGswJ0u+9u_kdmMaig@mail.gmail.com","threadId":"44491","inReplyTo":"20161117111337.GD39230@book.hvoigt.net","subject":"Re: [PATCH 08/16] update submodules: add depopulate_submodule","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-11-17T22:28:32Z","receivedAt":"2016-11-17T22:28:38Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Nov 17, 2016 at 3:13 AM, Heiko Voigt wrote:\n> On Tue, Nov 15, 2016 at 03:06:43PM -0800, Stefan Beller wrote:\n>> diff --git a/cache.h b/cache.h\n>> index a50a61a..65c47e4 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>> +void remove_subtree_or_die(const char *path);\n>\n> It seems that it is called remove_subtree already internally but can we\n> maybe change that name? The term 'subtree' refers to something else[1] for\n> me.\n\nDoh, right.\n\n> Maybe just: remove_directory() would make it also clear that there\n> is no special internal git datatype meant by that but just a directory\n> in the filesystem.\n\nI'll go with `remove_directory_or_die` for now.\n\n>\n>> +\n>> #endif /* CACHE_H */\n>> diff --git a/entry.c b/entry.c\n>> index c6eea24..019826b 100644\n>> --- a/entry.c\n>> +++ b/entry.c\n>> @@ -73,6 +73,14 @@ static void remove_subtree(struct strbuf *path)\n>> die_errno(\"cannot rmdir '%s'\", path->buf);\n>> }\n>>\n>> +void remove_subtree_or_die(const char *path)\n>> +{\n>> + struct strbuf sb = STRBUF_INIT;\n>> + strbuf_addstr(&sb, path);\n>> + remove_subtree(&sb);\n>> + strbuf_release(&sb);\n>> +}\n>\n> Why are you exposing it with const char * instead of strbuf? We get\n> unnecessary conversions in case a caller already has a strbuf ready.\n> Just in case later code also wants to use it.\n\nFor the cleanliness of API design. (I thought `void remove(char *dir)` is\nthe closest approximation of `rm -rf $1`)\n\nI'll use a strbuf instead.\n\nThanks!\nStefan\n"},{"id":"306132","messageId":"20161117222926.GN66382@google.com","threadId":"44491","inReplyTo":"CAGZ79kbCqLsRzrsX29uM7pobs_11UZtFOQWP9RO8ptS5PyDfmw@mail.gmail.com","subject":"Re: [PATCH 08/16] update submodules: add depopulate_submodule","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-11-17T22:29:26Z","receivedAt":"2016-11-17T22:29:34Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 11/17, Stefan Beller wrote:\n> Well first you get the warning:\n> \n> \"cannot remove submodule '%s' because it (or one of \"\n> \"its nested submodules) uses a .git directory\"),\n> \n> and in case a d/f/ conflict arises in a later stage (e.g. when the submodule\n> is replaced by a file or symlink), you get another related error with\n> less helpful description how to debug it.\n\nMaybe a warning isn't the right thing? Shouldn't the checkout fail if\nthere are any issues? This would force the user to stash/commit their\nchanges and then retry.\n\n> > All other submodules will actually be removed? Couldn't\n> > you end up in an undesirable state with a checkout effecting one\n> > submodule but not another?\n> \n> Yes you could. Maybe it's time to add\n> \"git submodule intern-git-dir\", which can be given as a helpful hint\n> or even run here first.\n\nThat would be a good idea, does that functionality already exist in one\nform or another? I'm assuming it must since git update does just that\nwhen cloning a submodule.\n\n-- \nBrandon Williams\n"},{"id":"306133","messageId":"CAGZ79ka0-JFvogHRoTA4ioMK86zD=zkgEfBb-gpU8tbOjwEoFA@mail.gmail.com","threadId":"44491","inReplyTo":"20161117222926.GN66382@google.com","subject":"Re: [PATCH 08/16] update submodules: add depopulate_submodule","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-11-17T22:42:11Z","receivedAt":"2016-11-17T22:42:18Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Nov 17, 2016 at 2:29 PM, Brandon Williams wrote:\n> On 11/17, Stefan Beller wrote:\n>> Well first you get the warning:\n>>\n>> \"cannot remove submodule '%s' because it (or one of \"\n>> \"its nested submodules) uses a .git directory\"),\n>>\n>> and in case a d/f/ conflict arises in a later stage (e.g. when the submodule\n>> is replaced by a file or symlink), you get another related error with\n>> less helpful description how to debug it.\n>\n> Maybe a warning isn't the right thing? Shouldn't the checkout fail if\n> there are any issues? This would force the user to stash/commit their\n> changes and then retry.\n\nWell if the path is not reused, e.g. you just delete a submodule in a commit\nwithout anything else, you could proceed and have the submodule laying\naround dirty?\n\n>\n>> > All other submodules will actually be removed? Couldn't\n>> > you end up in an undesirable state with a checkout effecting one\n>> > submodule but not another?\n>>\n>> Yes you could. Maybe it's time to add\n>> \"git submodule intern-git-dir\", which can be given as a helpful hint\n>> or even run here first.\n>\n> That would be a good idea, does that functionality already exist in one\n> form or another? I'm assuming it must since git update does just that\n> when cloning a submodule.\n\nNo it doesn't (it is roughly these three steps):\n\n mv ${SUBMODULE_PATH}/.git ${GIT_DIR}/modules/${SUBMODULE_NAME}\n git config -f ${GIT_DIR}/modules/${SUBMODULE_NAME}/config\ncore.worktree ${SUBMODULE_PATH}\n echo \"gitdir: ${GIT_DIR}/modules/${SUBMODULE_NAME}\" >\n${SUBMODULE_PATH}/.git\n\nThe last 2 steps are done via\n\n void connect_work_tree_and_git_dir(const char *work_tree, const\nchar *git_dir);\n\nin submodule.{c,h}\n\nHowever we'd need to make sure the first step is performed correctly. (and make\ndamn sure we don't loose that git dir), so I think rename(2) does the\ncorrect thing\nfor directories, except when these two locations are on a different mount point.\n\nI think I'll just write this functionality in C and optionally expose\nit via the submodule--helper,\nsuch that the user facing git-submodule.sh only has to call that helper.\n"},{"id":"306135","messageId":"CAGZ79kYE1JooyKMDsEM5=6OWxbCOL3q2=Et3nL7mMcayxtLZxA@mail.gmail.com","threadId":"44491","inReplyTo":"CAGZ79ka0-JFvogHRoTA4ioMK86zD=zkgEfBb-gpU8tbOjwEoFA@mail.gmail.com","subject":"Re: [PATCH 08/16] update submodules: add depopulate_submodule","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-11-18T00:16:36Z","receivedAt":"2016-11-18T00:16:44Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Nov 17, 2016 at 2:42 PM, Stefan Beller wrote:\n>\n> I think I'll just write this functionality in C and optionally expose\n> it via the submodule--helper,\n> such that the user facing git-submodule.sh only has to call that helper.\n\nI think it will roughly look like this:\n(white space mangled)\n\n\ncommit e72ef244c667920c874247aa32aa55845500aac8\nAuthor: Stefan Beller \nDate: Thu Nov 17 16:14:46 2016 -0800\n\n submodule--helper: add intern-git-dir function\n\n When a submodule has its git dir inside the working dir, the submodule\n support for checkout that we plan to add in a later patch will fail.\n\n Add functionality to migrate the git directory to be embedded\n into the superprojects git directory.\n\n Signed-off-by: Stefan Beller \n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 4beeda5..4f31100 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1076,6 +1076,21 @@ static int resolve_remote_submodule_branch(int\nargc, const char **argv,\n return 0;\n }\n\n+static int intern_git_dir(int argc, const char **argv, const char *prefix)\n+{\n+ int i;\n+ struct pathspec pathspec;\n+ struct module_list list = MODULE_LIST_INIT;\n+\n+ if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)\n+ return 1;\n+\n+ for (i = 0; i < list.nr; i++)\n+ migrate_submodule_gitdir(list.entries[i]->name);\n+\n+ return 0;\n+}\n+\n struct cmd_struct {\n const char *cmd;\n int (*fn)(int, const char **, const char *);\n@@ -1090,7 +1105,8 @@ static struct cmd_struct commands[] = {\n {\"resolve-relative-url\", resolve_relative_url},\n {\"resolve-relative-url-test\", resolve_relative_url_test},\n {\"init\", module_init},\n- {\"remote-branch\", resolve_remote_submodule_branch}\n+ {\"remote-branch\", resolve_remote_submodule_branch},\n+ {\"intern-git-dir\", intern_git_dir}\n };\n\n int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\ndiff --git a/submodule.c b/submodule.c\nindex 45b9060..e513bba 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1335,3 +1335,42 @@ void prepare_submodule_repo_env(struct argv_array *out)\n }\n argv_array_push(out, \"GIT_DIR=.git\");\n }\n+\n+/*\n+ * Migrate the given submodule (and all its submodules recursively) from\n+ * having its git directory within the working tree to the git dir nested\n+ * in its superprojects git dir under modules/.\n+ */\n+void migrate_submodule_gitdir(const char *path)\n+{\n+ char *old_git_dir;\n+ const char *new_git_dir;\n+ const struct submodule *sub;\n+\n+ struct child_process cp = CHILD_PROCESS_INIT;\n+ cp.git_cmd = 1;\n+ cp.no_stdin = 1;\n+ cp.dir = path;\n+ argv_array_pushl(&cp.args, \"submodule\", \"foreach\", \"--recursive\",\n+ \"git\", \"submodule--helper\" \"intern-git-dir\", NULL);\n+\n+ if (run_command(&cp))\n+ die(_(\"Could not migrate git directory in submodule '%s'\"),\n+ path);\n+\n+ old_git_dir = xstrfmt(\"%s/.git\", path);\n+ if (read_gitfile(old_git_dir))\n+ /* If it is an actual gitfile, it doesn't need migration. */\n+ goto out;\n+\n+ sub = submodule_from_path(null_sha1, path);\n+ new_git_dir = git_common_path(\"modules/%s\", sub->name);\n+\n+ if (rename(old_git_dir, new_git_dir) < 0)\n+ die_errno(_(\"Could not migrate git directory from %s to %s\"),\n+ old_git_dir, new_git_dir);\n+\n+ connect_work_tree_and_git_dir(path, new_git_dir);\n+out:\n+ free(old_git_dir);\n+}\ndiff --git a/submodule.h b/submodule.h\nindex aac202c..143ec18 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -90,5 +90,6 @@ extern int parallel_submodules(void);\n * retaining any config in the environment.\n */\n extern void prepare_submodule_repo_env(struct argv_array *out);\n+extern void migrate_submodule_gitdir(const char *path);\n\n #endif\n"},{"id":"306136","messageId":"CAGZ79kY1umjAF2jsvTHoG=M21a3VqcvjM5NZYiMr+qAkv5_APw@mail.gmail.com","threadId":"44491","inReplyTo":"20161116000236.GF66382@google.com","subject":"Re: [PATCH 09/16] update submodules: add scheduling to update submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-11-18T00:28:25Z","receivedAt":"2016-11-18T00:28:31Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Nov 15, 2016 at 4:02 PM, Brandon Williams wrote:\n> On 11/15, Stefan Beller wrote:\n>> +\n>> + child_process_clear(&cp);\n>> + return 0;\n>> +}\n>\n> If run command is successful then it handles the clearing of the child\n> process struct, correct? Is there a negative to having all the explicit\n> clears when the child was successful?\n\nvoid child_process_clear(struct child_process *child)\n{\n argv_array_clear(&child->args);\n argv_array_clear(&child->env_array);\n}\n\nI don't think so, as clearing empty arg arrays is a no op.\n\n>> +#define SCHEDULED_SUBMODULES_INIT {NULL, NULL}\n>\n> I may not know enough about these types of initializors but that Init\n> macro only has 2 entries while there are three entries in the struct\n> itself.\n\nFilled up to 3 to be explicit.\n\n>\n>> +\n>> +int scheduled_submodules_nr, scheduled_submodules_alloc;\n>\n> Should these globals be static since they should be scoped to only this\n> file?\n\nOf course, done.\n\n>\n> nit: organization wise it makes more sense to me to have the\n> 'update_submodule' helper function be located more closely to the\n> 'update_submodules' function.\n>\n\ndone\n\n\nDavid wrote:\n> In fact, only the first NULL is necessary; unspecified initializer entries in C default to zero.\n\nas said above, I explicitly init all of them now.\n"},{"id":"306146","messageId":"20161118174607.GP66382@google.com","threadId":"44491","inReplyTo":"CAGZ79kYE1JooyKMDsEM5=6OWxbCOL3q2=Et3nL7mMcayxtLZxA@mail.gmail.com","subject":"Re: [PATCH 08/16] update submodules: add depopulate_submodule","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-11-18T17:46:07Z","receivedAt":"2016-11-18T17:46:20Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 11/17, Stefan Beller wrote:\n> On Thu, Nov 17, 2016 at 2:42 PM, Stefan Beller wrote:\n> >\n> > I think I'll just write this functionality in C and optionally expose\n> > it via the submodule--helper,\n> > such that the user facing git-submodule.sh only has to call that helper.\n> \n> I think it will roughly look like this:\n> (white space mangled)\n> \n> \n> commit e72ef244c667920c874247aa32aa55845500aac8\n> Author: Stefan Beller \n> Date: Thu Nov 17 16:14:46 2016 -0800\n> \n> submodule--helper: add intern-git-dir function\n> \n> When a submodule has its git dir inside the working dir, the submodule\n> support for checkout that we plan to add in a later patch will fail.\n> \n> Add functionality to migrate the git directory to be embedded\n> into the superprojects git directory.\n> \n> Signed-off-by: Stefan Beller \n> \n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index 4beeda5..4f31100 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -1076,6 +1076,21 @@ static int resolve_remote_submodule_branch(int\n> argc, const char **argv,\n> return 0;\n> }\n> \n> +static int intern_git_dir(int argc, const char **argv, const char *prefix)\n> +{\n> + int i;\n> + struct pathspec pathspec;\n> + struct module_list list = MODULE_LIST_INIT;\n> +\n> + if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)\n> + return 1;\n> +\n> + for (i = 0; i < list.nr; i++)\n> + migrate_submodule_gitdir(list.entries[i]->name);\n> +\n> + return 0;\n> +}\n> +\n> struct cmd_struct {\n> const char *cmd;\n> int (*fn)(int, const char **, const char *);\n> @@ -1090,7 +1105,8 @@ static struct cmd_struct commands[] = {\n> {\"resolve-relative-url\", resolve_relative_url},\n> {\"resolve-relative-url-test\", resolve_relative_url_test},\n> {\"init\", module_init},\n> - {\"remote-branch\", resolve_remote_submodule_branch}\n> + {\"remote-branch\", resolve_remote_submodule_branch},\n> + {\"intern-git-dir\", intern_git_dir}\n> };\n> \n> int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\n> diff --git a/submodule.c b/submodule.c\n> index 45b9060..e513bba 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -1335,3 +1335,42 @@ void prepare_submodule_repo_env(struct argv_array *out)\n> }\n> argv_array_push(out, \"GIT_DIR=.git\");\n> }\n> +\n> +/*\n> + * Migrate the given submodule (and all its submodules recursively) from\n> + * having its git directory within the working tree to the git dir nested\n> + * in its superprojects git dir under modules/.\n> + */\n> +void migrate_submodule_gitdir(const char *path)\n> +{\n> + char *old_git_dir;\n> + const char *new_git_dir;\n> + const struct submodule *sub;\n> +\n> + struct child_process cp = CHILD_PROCESS_INIT;\n> + cp.git_cmd = 1;\n> + cp.no_stdin = 1;\n> + cp.dir = path;\n> + argv_array_pushl(&cp.args, \"submodule\", \"foreach\", \"--recursive\",\n> + \"git\", \"submodule--helper\" \"intern-git-dir\", NULL);\n> +\n> + if (run_command(&cp))\n> + die(_(\"Could not migrate git directory in submodule '%s'\"),\n> + path);\n> +\n> + old_git_dir = xstrfmt(\"%s/.git\", path);\n> + if (read_gitfile(old_git_dir))\n> + /* If it is an actual gitfile, it doesn't need migration. */\n> + goto out;\n> +\n> + sub = submodule_from_path(null_sha1, path);\n\nThis should probably be checked to see if sub not NULL before\ndereferencing it right?\n\n> + new_git_dir = git_common_path(\"modules/%s\", sub->name);\n> +\n> + if (rename(old_git_dir, new_git_dir) < 0)\n> + die_errno(_(\"Could not migrate git directory from %s to %s\"),\n> + old_git_dir, new_git_dir);\n> +\n> + connect_work_tree_and_git_dir(path, new_git_dir);\n> +out:\n> + free(old_git_dir);\n> +}\n> diff --git a/submodule.h b/submodule.h\n> index aac202c..143ec18 100644\n> --- a/submodule.h\n> +++ b/submodule.h\n> @@ -90,5 +90,6 @@ extern int parallel_submodules(void);\n> * retaining any config in the environment.\n> */\n> extern void prepare_submodule_repo_env(struct argv_array *out);\n> +extern void migrate_submodule_gitdir(const char *path);\n> \n> #endif\n\n-- \nBrandon Williams\n"},{"id":"306150","messageId":"CAGZ79kYD0ER4k6+o=spnCS3ymOnTcZ5i7Lu5-G_GLXu6XDqS3Q@mail.gmail.com","threadId":"44491","inReplyTo":"20161118174607.GP66382@google.com","subject":"Re: [PATCH 08/16] update submodules: add depopulate_submodule","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-11-18T18:25:31Z","receivedAt":"2016-11-18T18:25:37Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Nov 18, 2016 at 9:46 AM, Brandon Williams wrote:\n>> +\n>> + sub = submodule_from_path(null_sha1, path);\n>\n> This should probably be checked to see if sub not NULL before\n> dereferencing it right?\n\nyeah, will fix.\n"},{"id":"306152","messageId":"CAGZ79kYBWa1yjdRK4GzFo8BtsGJVexxxh0vVW4K5-xYhG9ucEA@mail.gmail.com","threadId":"44491","inReplyTo":"20161117133538.GF39230@book.hvoigt.net","subject":"Re: [PATCH 11/16] teach unpack_trees() to remove submodule contents","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-11-18T19:25:28Z","receivedAt":"2016-11-18T19:25:34Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Nov 17, 2016 at 5:35 AM, Heiko Voigt wrote:\n> On Tue, Nov 15, 2016 at 03:06:46PM -0800, Stefan Beller wrote:\n>> Extend rmdir_or_warn() to remove the directories of those submodules which\n>> are scheduled for removal. Also teach verify_clean_submodule() to check\n>> that a submodule configured to be removed is not modified before scheduling\n>> it for removal.\n>>\n>> Signed-off-by: Stefan Beller \n>> ---\n> [...]\n>> --- a/wrapper.c\n>> +++ b/wrapper.c\n>> @@ -2,6 +2,7 @@\n>> * Various trivial helper wrappers around standard functions\n>> */\n>> #include \"cache.h\"\n>> +#include \"submodule.h\"\n>>\n>> static void do_nothing(size_t size)\n>> {\n>> @@ -592,6 +593,9 @@ int unlink_or_warn(const char *file)\n>>\n>> int rmdir_or_warn(const char *file)\n>> {\n>> + if (submodule_is_interesting(file, null_sha1)\n>> + && depopulate_submodule(file))\n>> + return -1;\n>\n> How about untracked files inside submodules? Should we not care about\n> them? With this code the entire directory will be removed. In general\n> git would not remove a directory with untracked files in it even though\n> it is removed in the database. I wonder if we should imitate that here\n> and just remove files that are tracked by git so that users do not loose\n> files that might be precious to them.\n\nWell from the superprojects perspective a submodule looks like a file,\nso if the checkout involved removing a file it had to either be clean or the\ncheckout failed. So I though we'd go with a similar way here?\n\nWe need to stop when untracked files are present, I am not sure if we need\nto care if the files are ignored, though.\n\nIn this version of the series, the test added in patch 14, testing for\ntreating untracked files properly is a failing test. So I agree that we should\nnot just delete them; I'll fix that in the next version of the series.\n\nThanks,\nStefan\n"},{"id":"306182","messageId":"CAGZ79kb-TKgQm1_p3NyJS7mso2FFWMNHa7F8vwzocjSJfyjXJQ@mail.gmail.com","threadId":"44491","inReplyTo":"f54d446aa7734cb4aec4b51c7b81a2b6@exmbdft7.ad.twosigma.com","subject":"Re: [PATCH 13/16] submodule: teach unpack_trees() to update submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-11-18T23:33:22Z","receivedAt":"2016-11-18T23:33:28Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Nov 15, 2016 at 4:22 PM, David Turner wrote:\n>> msgs[ERROR_NOT_UPTODATE_DIR] =\n>> _(\"Updating the following directories would lose untracked\n>> files in it:\\n%s\");\n>> + msgs[ERROR_NOT_UPTODATE_SUBMODULE] =\n>> + _(\"Updating the following submodules would lose modifications\n>> in\n>> +it:\\n%s\");\n>\n> s/it/them/\n\ndone, also fixed the existing ERROR_NOT_UPTODATE_DIR.\n\n>> + if (!S_ISGITLINK(ce->ce_mode)) {\n>\n> I generally prefer to avoid if (!x) { A } else { B } -- I would rather just see if (x) { B } else { A }.\n\ndone.\n\n>> + if (submodule_is_interesting(old->name, null_sha1)\n>> + && ok_to_remove_submodule(old->name))\n>> + return 0;\n>> + }\n>\n> Do we need a return 1 in here somewhere? Because otherwise, we fall through and return 0 later.\n\nOtherwise we would fall through and run\n\n if (errno == ENOENT)\n return 0;\n return o->gently ? -1 :\n add_rejected_path(o, error_type, ce->name);\n\nwhich produces different results than 0?\n"},{"id":"306184","messageId":"CAGZ79kbxd6PxkBkdyGJOOV4f_LwroStXBDwNZ7RAEX+cL1m51w@mail.gmail.com","threadId":"44491","inReplyTo":"20161116002520.GI66382@google.com","subject":"Re: [PATCH 13/16] submodule: teach unpack_trees() to update submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-11-18T23:39:02Z","receivedAt":"2016-11-18T23:39:08Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Nov 15, 2016 at 4:25 PM, Brandon Williams wrote:\n> On 11/15, Stefan Beller wrote:\n>> + int flags = CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE;\n>\n> For readability you may want to have spaces between the two flags\n\ndone\n\n>\n>> + if (o->index_only\n>> + || (!((old->ce_flags & CE_VALID) || ce_skip_worktree(old))\n>> + && (o->reset || ce_uptodate(old))))\n>> + return 0;\n>\n> The coding guidelines say that git prefers to have the logical operators\n> on the right side like this:\n\nfixed all the coding style issues.\n"},{"id":"306224","messageId":"9fa4c24dc02941ff8a43c4b97dad8dc0@EXMBNJE7.ad.twosigma.com","threadId":"44491","inReplyTo":"CAGZ79kb-TKgQm1_p3NyJS7mso2FFWMNHa7F8vwzocjSJfyjXJQ@mail.gmail.com","subject":"RE: [PATCH 13/16] submodule: teach unpack_trees() to update submodules","fromName":"David Turner","fromEmail":"david.turner@twosigma.com","sentAt":"2016-11-21T18:12:32Z","receivedAt":"2016-11-21T18:12:39Z","isPatch":true,"sender":{"key":"david.turner@twosigma.com","avatar":null},"body":"\n\n> -----Original Message-----\n> From: Stefan Beller [mailto:sbeller@google.com]\n\n> >> + if (submodule_is_interesting(old->name,\n> null_sha1)\n> >> + && ok_to_remove_submodule(old->name))\n> >> + return 0;\n> >> + }\n> >\n> > Do we need a return 1 in here somewhere? Because otherwise, we fall\n> through and return 0 later.\n> \n> Otherwise we would fall through and run\n> \n> if (errno == ENOENT)\n> return 0;\n> return o->gently ? -1 :\n> add_rejected_path(o, error_type, ce->name);\n> \n> which produces different results than 0?\n\nOh, I see. I was misreading that errno check.\n"},{"id":"306861","messageId":"1df79f7b-83c5-91f3-7bd4-92eef200a682@gmail.com","threadId":"44491","inReplyTo":"20161115230651.23953-1-sbeller@google.com","subject":"Re: [RFC PATCH 00/16] Checkout aware of Submodules!","fromName":"Xiaodong Qi","fromEmail":"e2000s@gmail.com","sentAt":"2016-12-03T06:13:52Z","receivedAt":"2016-12-03T06:14:00Z","isPatch":true,"sender":{"key":"e2000s@gmail.com","avatar":null},"body":"I found this patch on Reddit and personally support this idea to\nsimplify the submodule update and checkout process. I don't know how\nother users handle the submodule update process, I do sometimes forget\nto checkout in superprojects with submodules and get a lot of trouble in\nusing the submodule function. The patch seems aims to making the process\neasier than before, although I am not qualified to review the code in\ndetail. I suggest experts in this area to review the code promptly and\nwork out the nuts and bolts toward the goal of this patch. Thank you for\nlistening.\n\nRegards,\nXiaodong Qi\n\nOn 11/15/2016 04:06 PM, Stefan Beller wrote:\n> When working with submodules, nearly anytime after checking out \n> a different state of the projects, that has submodules changed\n> you'd run \"git submodule update\" with a current version of Git.\n> \n> There are two problems with this approach:\n> \n> * The \"submodule update\" command is dangerous as it\n> doesn't check for work that may be lost in the submodule\n> (e.g. a dangling commit).\n> * you may forget to run the command as checkout is supposed\n> to do all the work for you.\n> \n> Integrate updating the submodules into git checkout, with the same\n> safety promises that git-checkout has, i.e. not throw away data unless\n> asked to. This is done by first checking if the submodule is at the same\n> sha1 as it is recorded in the superproject. If there are changes we stop\n> proceeding the checkout just like it is when checking out a file that\n> has local changes.\n> \n> The integration happens in the code that is also used in other commands\n> such that it will be easier in the future to make other commands aware\n> of submodule.\n> \n> This also solves d/f conflicts in case you replace a file/directory\n> with a submodule or vice versa.\n> \n> The patches are still a bit rough, but the overall series seems\n> promising enough to me that I want to put it out here.\n> \n> Any review, specifically on the design level welcome!\n> \n> Thanks,\n> Stefan\n> \n> Stefan Beller (16):\n> submodule.h: add extern keyword to functions, break line before 80\n> submodule: modernize ok_to_remove_submodule to use argv_array\n> submodule: use absolute path for computing relative path connecting\n> update submodules: add is_submodule_populated\n> update submodules: add submodule config parsing\n> update submodules: add a config option to determine if submodules are\n> updated\n> update submodules: introduce submodule_is_interesting\n> update submodules: add depopulate_submodule\n> update submodules: add scheduling to update submodules\n> update submodules: is_submodule_checkout_safe\n> teach unpack_trees() to remove submodule contents\n> entry: write_entry to write populate submodules\n> submodule: teach unpack_trees() to update submodules\n> checkout: recurse into submodules if asked to\n> completion: add '--recurse-submodules' to checkout\n> checkout: add config option to recurse into submodules by default\n> \n> Documentation/config.txt | 6 +\n> Documentation/git-checkout.txt | 8 +\n> builtin/checkout.c | 31 ++-\n> cache.h | 2 +\n> contrib/completion/git-completion.bash | 2 +-\n> entry.c | 17 +-\n> submodule-config.c | 22 +++\n> submodule-config.h | 17 +-\n> submodule.c | 246 +++++++++++++++++++++--\n> submodule.h | 77 +++++---\n> t/lib-submodule-update.sh | 10 +-\n> t/t2013-checkout-submodule.sh | 344 ++++++++++++++++++++++++++++++++-\n> t/t9902-completion.sh | 1 +\n> unpack-trees.c | 103 ++++++++--\n> unpack-trees.h | 1 +\n> wrapper.c | 4 +\n> 16 files changed, 806 insertions(+), 85 deletions(-)\n> \n"}]}