{"thread":{"id":"48872","subject":"[PATCH 0/6] git-submodule.sh: convert part of cmd_update to C","startedAt":"2018-07-12T19:48:00Z","lastAt":"2018-07-18T19:41:51Z","messageCount":13,"participants":["Stefan Beller","Junio C Hamano","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"352416","messageId":"20180712194754.71979-1-sbeller@google.com","threadId":"48872","inReplyTo":null,"subject":"[PATCH 0/6] git-submodule.sh: convert part of cmd_update to C","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-07-12T19:47:48Z","receivedAt":"2018-07-12T19:48:00Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"I thought about writing it all in one go, but the series got too large,\nso let's chew one bite at a time.\n\nThanks,\nStefan\n\nStefan Beller (6):\n  git-submodule.sh: align error reporting for update mode to use path\n  git-submodule.sh: rename unused variables\n  builtin/submodule--helper: factor out submodule updating\n  builtin/submodule--helper: store update_clone information in a struct\n  builtin/submodule--helper: factor out method to update a single\n    submodule\n  submodule--helper: introduce new update-module-mode helper\n\n builtin/submodule--helper.c | 152 ++++++++++++++++++++++++++++--------\n git-submodule.sh            |  22 +-----\n 2 files changed, 122 insertions(+), 52 deletions(-)\n\n-- \n2.18.0.203.gfac676dfb9-goog\n\n"},{"id":"352417","messageId":"20180712194754.71979-2-sbeller@google.com","threadId":"48872","inReplyTo":"20180712194754.71979-1-sbeller@google.com","subject":"[PATCH 1/6] git-submodule.sh: align error reporting for update mode to use path","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-07-12T19:47:49Z","receivedAt":"2018-07-12T19:48:02Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"All other error messages in cmd_update are reporting the submodule based\non its path, so let's do that for invalid update modes, too.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n git-submodule.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 5f9d9f6ea37..8a611865397 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -627,7 +627,7 @@ cmd_update()\n \t\t\t\tmust_die_on_failure=yes\n \t\t\t\t;;\n \t\t\t*)\n-\t\t\t\tdie \"$(eval_gettext \"Invalid update mode '$update_module' for submodule '$name'\")\"\n+\t\t\t\tdie \"$(eval_gettext \"Invalid update mode '$update_module' for submodule path '$path'\")\"\n \t\t\tesac\n \n \t\t\tif (sanitize_submodule_env; cd \"$sm_path\" && $command \"$sha1\")\n-- \n2.18.0.203.gfac676dfb9-goog\n\n"},{"id":"352418","messageId":"20180712194754.71979-3-sbeller@google.com","threadId":"48872","inReplyTo":"20180712194754.71979-1-sbeller@google.com","subject":"[PATCH 2/6] git-submodule.sh: rename unused variables","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-07-12T19:47:50Z","receivedAt":"2018-07-12T19:48:04Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The 'mode' variable is not used in cmd_update for its original purpose,\nrename it to 'dummy' as it only serves the purpose to abort quickly\ndocumenting this knowledge.\n\nThe variable 'stage' is also not used any more in cmd_update, so remove it.\n\nThis went unnoticed as first each function used the commonly used\nsubmodule listing, which was converted in 74703a1e4df (submodule: rewrite\n`module_list` shell function in C, 2015-09-02). When cmd_update was\nusing its own function starting in 48308681b07 (git submodule update:\nhave a dedicated helper for cloning, 2016-02-29), its removal was missed.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n builtin/submodule--helper.c | 5 ++---\n git-submodule.sh            | 4 ++--\n 2 files changed, 4 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 20ae9191ca3..ebcfe85bfa9 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1571,9 +1571,8 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n \tneeds_cloning = !file_exists(sb.buf);\n \n \tstrbuf_reset(&sb);\n-\tstrbuf_addf(&sb, \"%06o %s %d %d\\t%s\\n\", ce->ce_mode,\n-\t\t\toid_to_hex(&ce->oid), ce_stage(ce),\n-\t\t\tneeds_cloning, ce->name);\n+\tstrbuf_addf(&sb, \"dummy %s %d\\t%s\\n\",\n+\t\t    oid_to_hex(&ce->oid), needs_cloning, ce->name);\n \tstring_list_append(&suc->projectlines, sb.buf);\n \n \tif (!needs_cloning)\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 8a611865397..56588aa304d 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -531,9 +531,9 @@ cmd_update()\n \t\t\"$@\" || echo \"#unmatched\" $?\n \t} | {\n \terr=\n-\twhile read -r mode sha1 stage just_cloned sm_path\n+\twhile read -r quickabort sha1 just_cloned sm_path\n \tdo\n-\t\tdie_if_unmatched \"$mode\" \"$sha1\"\n+\t\tdie_if_unmatched \"$quickabort\" \"$sha1\"\n \n \t\tname=$(git submodule--helper name \"$sm_path\") || exit\n \t\tif ! test -z \"$update\"\n-- \n2.18.0.203.gfac676dfb9-goog\n\n"},{"id":"352419","messageId":"20180712194754.71979-4-sbeller@google.com","threadId":"48872","inReplyTo":"20180712194754.71979-1-sbeller@google.com","subject":"[PATCH 3/6] builtin/submodule--helper: factor out submodule updating","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-07-12T19:47:51Z","receivedAt":"2018-07-12T19:48:07Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Separate the command line parsing from the actual execution of the command\nwithin the repository. For now there is not a lot of execution as\nmost of it is still in git-submodule.sh.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n builtin/submodule--helper.c | 59 +++++++++++++++++++++----------------\n 1 file changed, 33 insertions(+), 26 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex ebcfe85bfa9..96929ba1096 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1472,6 +1472,8 @@ struct submodule_update_clone {\n \t/* failed clones to be retried again */\n \tconst struct cache_entry **failed_clones;\n \tint failed_clones_nr, failed_clones_alloc;\n+\n+\tint max_jobs;\n };\n #define SUBMODULE_UPDATE_CLONE_INIT {0, MODULE_LIST_INIT, 0, \\\n \tSUBMODULE_UPDATE_STRATEGY_INIT, 0, 0, -1, STRING_LIST_INIT_DUP, 0, \\\n@@ -1714,11 +1716,36 @@ static int gitmodules_update_clone_config(const char *var, const char *value,\n \treturn 0;\n }\n \n+static int update_submodules(struct submodule_update_clone *suc)\n+{\n+\tstruct string_list_item *item;\n+\n+\trun_processes_parallel(suc->max_jobs,\n+\t\t\t       update_clone_get_next_task,\n+\t\t\t       update_clone_start_failure,\n+\t\t\t       update_clone_task_finished,\n+\t\t\t       suc);\n+\n+\t/*\n+\t * We saved the output and put it out all at once now.\n+\t * That means:\n+\t * - the listener does not have to interleave their (checkout)\n+\t *   work with our fetching.  The writes involved in a\n+\t *   checkout involve more straightforward sequential I/O.\n+\t * - the listener can avoid doing any work if fetching failed.\n+\t */\n+\tif (suc->quickstop)\n+\t\treturn 1;\n+\n+\tfor_each_string_list_item(item, &suc->projectlines)\n+\t\tfprintf(stdout, \"%s\", item->string);\n+\n+\treturn 0;\n+}\n+\n static int update_clone(int argc, const char **argv, const char *prefix)\n {\n \tconst char *update = NULL;\n-\tint max_jobs = 1;\n-\tstruct string_list_item *item;\n \tstruct pathspec pathspec;\n \tstruct submodule_update_clone suc = SUBMODULE_UPDATE_CLONE_INIT;\n \n@@ -1740,7 +1767,7 @@ static int update_clone(int argc, const char **argv, const char *prefix)\n \t\tOPT_STRING(0, \"depth\", &suc.depth, \"<depth>\",\n \t\t\t   N_(\"Create a shallow clone truncated to the \"\n \t\t\t      \"specified number of revisions\")),\n-\t\tOPT_INTEGER('j', \"jobs\", &max_jobs,\n+\t\tOPT_INTEGER('j', \"jobs\", &suc.max_jobs,\n \t\t\t    N_(\"parallel jobs\")),\n \t\tOPT_BOOL(0, \"recommend-shallow\", &suc.recommend_shallow,\n \t\t\t    N_(\"whether the initial clone should follow the shallow recommendation\")),\n@@ -1756,8 +1783,8 @@ static int update_clone(int argc, const char **argv, const char *prefix)\n \t};\n \tsuc.prefix = prefix;\n \n-\tconfig_from_gitmodules(gitmodules_update_clone_config, &max_jobs);\n-\tgit_config(gitmodules_update_clone_config, &max_jobs);\n+\tconfig_from_gitmodules(gitmodules_update_clone_config, &suc.max_jobs);\n+\tgit_config(gitmodules_update_clone_config, &suc.max_jobs);\n \n \targc = parse_options(argc, argv, prefix, module_update_clone_options,\n \t\t\t     git_submodule_helper_usage, 0);\n@@ -1772,27 +1799,7 @@ static int update_clone(int argc, const char **argv, const char *prefix)\n \tif (pathspec.nr)\n \t\tsuc.warn_if_uninitialized = 1;\n \n-\trun_processes_parallel(max_jobs,\n-\t\t\t       update_clone_get_next_task,\n-\t\t\t       update_clone_start_failure,\n-\t\t\t       update_clone_task_finished,\n-\t\t\t       &suc);\n-\n-\t/*\n-\t * We saved the output and put it out all at once now.\n-\t * That means:\n-\t * - the listener does not have to interleave their (checkout)\n-\t *   work with our fetching.  The writes involved in a\n-\t *   checkout involve more straightforward sequential I/O.\n-\t * - the listener can avoid doing any work if fetching failed.\n-\t */\n-\tif (suc.quickstop)\n-\t\treturn 1;\n-\n-\tfor_each_string_list_item(item, &suc.projectlines)\n-\t\tfprintf(stdout, \"%s\", item->string);\n-\n-\treturn 0;\n+\treturn update_submodules(&suc);\n }\n \n static int resolve_relative_path(int argc, const char **argv, const char *prefix)\n-- \n2.18.0.203.gfac676dfb9-goog\n\n"},{"id":"352420","messageId":"20180712194754.71979-5-sbeller@google.com","threadId":"48872","inReplyTo":"20180712194754.71979-1-sbeller@google.com","subject":"[PATCH 4/6] builtin/submodule--helper: store update_clone information in a struct","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-07-12T19:47:52Z","receivedAt":"2018-07-12T19:48:09Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The information that is printed for update_submodules in\n'submodule--helper update-clone' and consumed by 'git submodule update'\nis stored as a string per submodule. This made sense at the time of\n48308681b07 (git submodule update: have a dedicated helper for cloning,\n2016-02-29), but as we want to migrate the rest of the submodule update\ninto C, we're better off having access to the raw information in a helper\nstruct.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n builtin/submodule--helper.c | 37 +++++++++++++++++++++++++++----------\n 1 file changed, 27 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 96929ba1096..c9c3fe2bf28 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1444,6 +1444,12 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n+struct submodule_update_clone_information {\n+\tconst struct submodule *sub;\n+\tstruct object_id oid;\n+\tunsigned just_cloned;\n+};\n+\n struct submodule_update_clone {\n \t/* index into 'list', the list of submodules to look into for cloning */\n \tint current;\n@@ -1463,8 +1469,9 @@ struct submodule_update_clone {\n \tconst char *recursive_prefix;\n \tconst char *prefix;\n \n-\t/* Machine-readable status lines to be consumed by git-submodule.sh */\n-\tstruct string_list projectlines;\n+\t/* to be consumed by git-submodule.sh */\n+\tstruct submodule_update_clone_information *submodule_lines;\n+\tint submodule_lines_nr; int submodule_lines_alloc;\n \n \t/* If we want to stop as fast as possible and return an error */\n \tunsigned quickstop : 1;\n@@ -1478,7 +1485,7 @@ struct submodule_update_clone {\n #define SUBMODULE_UPDATE_CLONE_INIT {0, MODULE_LIST_INIT, 0, \\\n \tSUBMODULE_UPDATE_STRATEGY_INIT, 0, 0, -1, STRING_LIST_INIT_DUP, 0, \\\n \tNULL, NULL, NULL, \\\n-\tSTRING_LIST_INIT_DUP, 0, NULL, 0, 0}\n+\tNULL, 0, 0, 0, NULL, 0, 0, 0}\n \n \n static void next_submodule_warn_missing(struct submodule_update_clone *suc,\n@@ -1572,10 +1579,12 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n \tstrbuf_addf(&sb, \"%s/.git\", ce->name);\n \tneeds_cloning = !file_exists(sb.buf);\n \n-\tstrbuf_reset(&sb);\n-\tstrbuf_addf(&sb, \"dummy %s %d\\t%s\\n\",\n-\t\t    oid_to_hex(&ce->oid), needs_cloning, ce->name);\n-\tstring_list_append(&suc->projectlines, sb.buf);\n+\tALLOC_GROW(suc->submodule_lines, suc->submodule_lines_nr + 1,\n+\t\t\t\t\t suc->submodule_lines_alloc);\n+\toidcpy(&suc->submodule_lines[suc->submodule_lines_nr].oid, &ce->oid);\n+\tsuc->submodule_lines[suc->submodule_lines_nr].just_cloned = needs_cloning;\n+\tsuc->submodule_lines[suc->submodule_lines_nr].sub = sub;\n+\tsuc->submodule_lines_nr++;\n \n \tif (!needs_cloning)\n \t\tgoto cleanup;\n@@ -1718,7 +1727,8 @@ static int gitmodules_update_clone_config(const char *var, const char *value,\n \n static int update_submodules(struct submodule_update_clone *suc)\n {\n-\tstruct string_list_item *item;\n+\tint i;\n+\tstruct strbuf sb = STRBUF_INIT;\n \n \trun_processes_parallel(suc->max_jobs,\n \t\t\t       update_clone_get_next_task,\n@@ -1737,9 +1747,16 @@ static int update_submodules(struct submodule_update_clone *suc)\n \tif (suc->quickstop)\n \t\treturn 1;\n \n-\tfor_each_string_list_item(item, &suc->projectlines)\n-\t\tfprintf(stdout, \"%s\", item->string);\n+\tfor (i = 0; i < suc->submodule_lines_nr; i++) {\n+\t\tstrbuf_addf(&sb, \"dummy %s %d\\t%s\\n\",\n+\t\t\toid_to_hex(&suc->submodule_lines[i].oid),\n+\t\t\tsuc->submodule_lines[i].just_cloned,\n+\t\t\tsuc->submodule_lines[i].sub->path);\n+\t\tfprintf(stdout, \"%s\", sb.buf);\n+\t\tstrbuf_reset(&sb);\n+\t}\n \n+\tstrbuf_release(&sb);\n \treturn 0;\n }\n \n-- \n2.18.0.203.gfac676dfb9-goog\n\n"},{"id":"352421","messageId":"20180712194754.71979-6-sbeller@google.com","threadId":"48872","inReplyTo":"20180712194754.71979-1-sbeller@google.com","subject":"[PATCH 5/6] builtin/submodule--helper: factor out method to update a single submodule","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-07-12T19:47:53Z","receivedAt":"2018-07-12T19:48:11Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In a later patch we'll find this method handy.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n builtin/submodule--helper.c | 20 ++++++++++----------\n 1 file changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex c9c3fe2bf28..4bbf580df79 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1725,10 +1725,17 @@ static int gitmodules_update_clone_config(const char *var, const char *value,\n \treturn 0;\n }\n \n+static void update_submodule(struct submodule_update_clone_information *suci)\n+{\n+\tfprintf(stdout, \"dummy %s %d\\t%s\\n\",\n+\t\toid_to_hex(&suci->oid),\n+\t\tsuci->just_cloned,\n+\t\tsuci->sub->path);\n+}\n+\n static int update_submodules(struct submodule_update_clone *suc)\n {\n \tint i;\n-\tstruct strbuf sb = STRBUF_INIT;\n \n \trun_processes_parallel(suc->max_jobs,\n \t\t\t       update_clone_get_next_task,\n@@ -1747,16 +1754,9 @@ static int update_submodules(struct submodule_update_clone *suc)\n \tif (suc->quickstop)\n \t\treturn 1;\n \n-\tfor (i = 0; i < suc->submodule_lines_nr; i++) {\n-\t\tstrbuf_addf(&sb, \"dummy %s %d\\t%s\\n\",\n-\t\t\toid_to_hex(&suc->submodule_lines[i].oid),\n-\t\t\tsuc->submodule_lines[i].just_cloned,\n-\t\t\tsuc->submodule_lines[i].sub->path);\n-\t\tfprintf(stdout, \"%s\", sb.buf);\n-\t\tstrbuf_reset(&sb);\n-\t}\n+\tfor (i = 0; i < suc->submodule_lines_nr; i++)\n+\t\tupdate_submodule(&suc->submodule_lines[i]);\n \n-\tstrbuf_release(&sb);\n \treturn 0;\n }\n \n-- \n2.18.0.203.gfac676dfb9-goog\n\n"},{"id":"352422","messageId":"20180712194754.71979-7-sbeller@google.com","threadId":"48872","inReplyTo":"20180712194754.71979-1-sbeller@google.com","subject":"[PATCH 6/6] submodule--helper: introduce new update-module-mode helper","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-07-12T19:47:54Z","receivedAt":"2018-07-12T19:48:13Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This chews off a bit of the shell part of the update command in\ngit-submodule.sh. When writing the C code, keep in mind that the\nsubmodule--helper part will go away eventually and we want to have\na C function that is able to determine the submodule update strategy,\nit as a nicety, make determine_submodule_update_strategy accessible\nfor arbitrary repositories.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n builtin/submodule--helper.c | 61 +++++++++++++++++++++++++++++++++++++\n git-submodule.sh            | 16 +---------\n 2 files changed, 62 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 4bbf580df79..e53231cf286 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1444,6 +1444,66 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n+static void determine_submodule_update_strategy(struct repository *r,\n+\t\t\t\t\t\tint just_cloned,\n+\t\t\t\t\t\tconst char *path,\n+\t\t\t\t\t\tconst char *update,\n+\t\t\t\t\t\tstruct submodule_update_strategy *out)\n+{\n+\tconst struct submodule *sub = submodule_from_path(r, &null_oid, path);\n+\tchar *key;\n+\tconst char *val;\n+\n+\tkey = xstrfmt(\"submodule.%s.update\", sub->name);\n+\n+\tif (update) {\n+\t\ttrace_printf(\"parsing update\");\n+\t\tif (parse_submodule_update_strategy(update, out) < 0)\n+\t\t\tdie(_(\"Invalid update mode '%s' for submodule path '%s'\"),\n+\t\t\t\tupdate, path);\n+\t} else if (!repo_config_get_string_const(r, key, &val)) {\n+\t\tif (parse_submodule_update_strategy(val, out) < 0)\n+\t\t\tdie(_(\"Invalid update mode '%s' configured for submodule path '%s'\"),\n+\t\t\t\tval, path);\n+\t} else if (sub->update_strategy.type != SM_UPDATE_UNSPECIFIED) {\n+\t\ttrace_printf(\"loaded thing\");\n+\t\tout->type = sub->update_strategy.type;\n+\t\tout->command = sub->update_strategy.command;\n+\t} else\n+\t\tout->type = SM_UPDATE_CHECKOUT;\n+\n+\tif (just_cloned &&\n+\t    (out->type == SM_UPDATE_MERGE ||\n+\t     out->type == SM_UPDATE_REBASE ||\n+\t     out->type == SM_UPDATE_NONE))\n+\t\tout->type = SM_UPDATE_CHECKOUT;\n+\n+\tfree(key);\n+}\n+\n+static int module_update_module_mode(int argc, const char **argv, const char *prefix)\n+{\n+\tconst char *path, *update = NULL;\n+\tint just_cloned;\n+\tstruct submodule_update_strategy update_strategy = { .type = SM_UPDATE_CHECKOUT };\n+\n+\tif (argc < 3 || argc > 4)\n+\t\tdie(\"submodule--helper update-module-clone expects <just-cloned> <path> [<update>]\");\n+\n+\tjust_cloned = git_config_int(\"just_cloned\", argv[1]);\n+\tpath = argv[2];\n+\n+\tif (argc == 4)\n+\t\tupdate = argv[3];\n+\n+\tdetermine_submodule_update_strategy(the_repository,\n+\t\t\t\t\t    just_cloned, path, update,\n+\t\t\t\t\t    &update_strategy);\n+\tfprintf(stdout, submodule_strategy_to_string(&update_strategy));\n+\n+\treturn 0;\n+}\n+\n struct submodule_update_clone_information {\n \tconst struct submodule *sub;\n \tstruct object_id oid;\n@@ -2039,6 +2099,7 @@ static struct cmd_struct commands[] = {\n \t{\"list\", module_list, 0},\n \t{\"name\", module_name, 0},\n \t{\"clone\", module_clone, 0},\n+\t{\"update-module-mode\", module_update_module_mode, 0},\n \t{\"update-clone\", update_clone, 0},\n \t{\"relative-path\", resolve_relative_path, 0},\n \t{\"resolve-relative-url\", resolve_relative_url, 0},\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 56588aa304d..215760898ce 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -535,27 +535,13 @@ cmd_update()\n \tdo\n \t\tdie_if_unmatched \"$quickabort\" \"$sha1\"\n \n-\t\tname=$(git submodule--helper name \"$sm_path\") || exit\n-\t\tif ! test -z \"$update\"\n-\t\tthen\n-\t\t\tupdate_module=$update\n-\t\telse\n-\t\t\tupdate_module=$(git config submodule.\"$name\".update)\n-\t\t\tif test -z \"$update_module\"\n-\t\t\tthen\n-\t\t\t\tupdate_module=\"checkout\"\n-\t\t\tfi\n-\t\tfi\n+\t\tupdate_module=$(git submodule--helper update-module-mode $just_cloned \"$sm_path\" $update)\n \n \t\tdisplaypath=$(git submodule--helper relative-path \"$prefix$sm_path\" \"$wt_prefix\")\n \n \t\tif test $just_cloned -eq 1\n \t\tthen\n \t\t\tsubsha1=\n-\t\t\tcase \"$update_module\" in\n-\t\t\tmerge | rebase | none)\n-\t\t\t\tupdate_module=checkout ;;\n-\t\t\tesac\n \t\telse\n \t\t\tsubsha1=$(sanitize_submodule_env; cd \"$sm_path\" &&\n \t\t\t\tgit rev-parse --verify HEAD) ||\n-- \n2.18.0.203.gfac676dfb9-goog\n\n"},{"id":"352439","messageId":"xmqqa7qwupje.fsf@gitster-ct.c.googlers.com","threadId":"48872","inReplyTo":"20180712194754.71979-2-sbeller@google.com","subject":"Re: [PATCH 1/6] git-submodule.sh: align error reporting for update mode to use path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-12T21:12:21Z","receivedAt":"2018-07-12T21:12:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> All other error messages in cmd_update are reporting the submodule based\n> on its path, so let's do that for invalid update modes, too.\n>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n\nMakes sense.\n\n>  git-submodule.sh | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index 5f9d9f6ea37..8a611865397 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -627,7 +627,7 @@ cmd_update()\n>  \t\t\t\tmust_die_on_failure=yes\n>  \t\t\t\t;;\n>  \t\t\t*)\n> -\t\t\t\tdie \"$(eval_gettext \"Invalid update mode '$update_module' for submodule '$name'\")\"\n> +\t\t\t\tdie \"$(eval_gettext \"Invalid update mode '$update_module' for submodule path '$path'\")\"\n>  \t\t\tesac\n>  \n>  \t\t\tif (sanitize_submodule_env; cd \"$sm_path\" && $command \"$sha1\")\n"},{"id":"352440","messageId":"xmqq601kuozm.fsf@gitster-ct.c.googlers.com","threadId":"48872","inReplyTo":"20180712194754.71979-3-sbeller@google.com","subject":"Re: [PATCH 2/6] git-submodule.sh: rename unused variables","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-12T21:24:13Z","receivedAt":"2018-07-12T21:24:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> The 'mode' variable is not used in cmd_update for its original purpose,\n> rename it to 'dummy' as it only serves the purpose to abort quickly\n> documenting this knowledge.\n\nIt seems that\n\n (1) the while loop in git-submodule.sh we see in this patch is the\n     only thing that read from submodule--helper update-clone; and\n\n (2) the mode/sha1/stage output from prepare_to_clone_next_submodule()\n     is shown only when update_clone_get_next_task which in turn is\n     run only during update-clone task\n\nso this conversion will not have unintended bad effect on other\ncodepaths that use similarly formatted (but already different)\noutput used by the users of module_list.  \n\nWhich tells us that this step is safe.\n\nI am not sure how much it buys us not having to format mode into hex\nand not having to call ce_stage(), when we have cache entry anyway\nin the codeflow, though.  IOW, it is unclear at this second step in\nthe six-patch series if we get to the end of the tunnel sooner by\nhaving this step here.  Let's keep reading ;-).\n\n\n\n\n> The variable 'stage' is also not used any more in cmd_update, so remove it.\n>\n> This went unnoticed as first each function used the commonly used\n> submodule listing, which was converted in 74703a1e4df (submodule: rewrite\n> `module_list` shell function in C, 2015-09-02). When cmd_update was\n> using its own function starting in 48308681b07 (git submodule update:\n> have a dedicated helper for cloning, 2016-02-29), its removal was missed.\n>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>  builtin/submodule--helper.c | 5 ++---\n>  git-submodule.sh            | 4 ++--\n>  2 files changed, 4 insertions(+), 5 deletions(-)\n>\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index 20ae9191ca3..ebcfe85bfa9 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -1571,9 +1571,8 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n>  \tneeds_cloning = !file_exists(sb.buf);\n>  \n>  \tstrbuf_reset(&sb);\n> -\tstrbuf_addf(&sb, \"%06o %s %d %d\\t%s\\n\", ce->ce_mode,\n> -\t\t\toid_to_hex(&ce->oid), ce_stage(ce),\n> -\t\t\tneeds_cloning, ce->name);\n> +\tstrbuf_addf(&sb, \"dummy %s %d\\t%s\\n\",\n> +\t\t    oid_to_hex(&ce->oid), needs_cloning, ce->name);\n>  \tstring_list_append(&suc->projectlines, sb.buf);\n>  \n>  \tif (!needs_cloning)\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index 8a611865397..56588aa304d 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -531,9 +531,9 @@ cmd_update()\n>  \t\t\"$@\" || echo \"#unmatched\" $?\n>  \t} | {\n>  \terr=\n> -\twhile read -r mode sha1 stage just_cloned sm_path\n> +\twhile read -r quickabort sha1 just_cloned sm_path\n>  \tdo\n> -\t\tdie_if_unmatched \"$mode\" \"$sha1\"\n> +\t\tdie_if_unmatched \"$quickabort\" \"$sha1\"\n>  \n>  \t\tname=$(git submodule--helper name \"$sm_path\") || exit\n>  \t\tif ! test -z \"$update\"\n"},{"id":"352660","messageId":"xmqqo9f7t1jy.fsf@gitster-ct.c.googlers.com","threadId":"48872","inReplyTo":"20180712194754.71979-5-sbeller@google.com","subject":"Re: [PATCH 4/6] builtin/submodule--helper: store update_clone information in a struct","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-16T19:37:05Z","receivedAt":"2018-07-16T19:37:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> The information that is printed for update_submodules in\n> 'submodule--helper update-clone' and consumed by 'git submodule update'\n> is stored as a string per submodule. This made sense at the time of\n> 48308681b07 (git submodule update: have a dedicated helper for cloning,\n> 2016-02-29), but as we want to migrate the rest of the submodule update\n> into C, we're better off having access to the raw information in a helper\n> struct.\n\nThe reasoning above makes sense, but I have a few niggles on the\nnaming.\n\n * Anything you'd place to keep track of the state is \"information\",\n   so a whole \"_information\" suffix to the structure name does not\n   add much value.  We've seen our fair share of (meaningless)\n   \"_data\" suffix used in many places but I think the overly long\n   \"_information\" that is equally meaningless trumps them.  I think\n   we also have \"_info\", but if we are not going to have a beter\n   name, let's not be original and stick to \"_data\" like other\n   existing codepath.  An alternative with more meaningful name is\n   of course better, though, but it is not all that much worth to\n   spend too many braincycles on it.\n\n * Is the fact that these parameters necessary to help populating\n   the submodule repository are sent on a line to external process\n   the most important aspect of the submodule_lines[] array?  As\n   this step is a preparation to migrate out of that line/text\n   oriented IPC, I think line-ness is the least important and\n   interesting thing to name the variable with.\n\n\nIf I were writing this patch, perhaps I'd do\n\n\tstruct update_clone_data *update_clone;\n\tint update_clone_nr, update_clone_alloc;\n\nfollowing my gut, but since you've been thinking about submodule\nlonger than I have, perhaps you can come up with a better name.\n\n> @@ -1463,8 +1469,9 @@ struct submodule_update_clone {\n>  \tconst char *recursive_prefix;\n>  \tconst char *prefix;\n>  \n> -\t/* Machine-readable status lines to be consumed by git-submodule.sh */\n> -\tstruct string_list projectlines;\n> +\t/* to be consumed by git-submodule.sh */\n> +\tstruct submodule_update_clone_information *submodule_lines;\n> +\tint submodule_lines_nr; int submodule_lines_alloc;\n>  \n>  \t/* If we want to stop as fast as possible and return an error */\n>  \tunsigned quickstop : 1;\n> @@ -1478,7 +1485,7 @@ struct submodule_update_clone {\n>  #define SUBMODULE_UPDATE_CLONE_INIT {0, MODULE_LIST_INIT, 0, \\\n>  \tSUBMODULE_UPDATE_STRATEGY_INIT, 0, 0, -1, STRING_LIST_INIT_DUP, 0, \\\n>  \tNULL, NULL, NULL, \\\n> -\tSTRING_LIST_INIT_DUP, 0, NULL, 0, 0}\n> +\tNULL, 0, 0, 0, NULL, 0, 0, 0}\n\nThe structure definition and this macro definition are nearby, so it\nis not crucial, but its probably not a bad idea to switch to C99\ninitializers for a thing like this to make it more readable, once\nthe code around this area stabilizes back again sufficiently (IOW,\nlet's not distract ourselves in the middle of adding a new feature\nlike this one).\n"},{"id":"352727","messageId":"CAGZ79kaOy0pZJCob4d=66FQLaOu_q_BAJeFc94xkFa0SSqYigw@mail.gmail.com","threadId":"48872","inReplyTo":"xmqqo9f7t1jy.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 4/6] builtin/submodule--helper: store update_clone information in a struct","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-07-16T23:55:59Z","receivedAt":"2018-07-16T23:56:13Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Jul 16, 2018 at 12:37 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Stefan Beller <sbeller@google.com> writes:\n>\n> > The information that is printed for update_submodules in\n> > 'submodule--helper update-clone' and consumed by 'git submodule update'\n> > is stored as a string per submodule. This made sense at the time of\n> > 48308681b07 (git submodule update: have a dedicated helper for cloning,\n> > 2016-02-29), but as we want to migrate the rest of the submodule update\n> > into C, we're better off having access to the raw information in a helper\n> > struct.\n>\n> The reasoning above makes sense, but I have a few niggles on the\n> naming.\n>\n>  * Anything you'd place to keep track of the state is \"information\",\n>    so a whole \"_information\" suffix to the structure name does not\n>    add much value.  We've seen our fair share of (meaningless)\n>    \"_data\" suffix used in many places but I think the overly long\n>    \"_information\" that is equally meaningless trumps them.  I think\n>    we also have \"_info\", but if we are not going to have a beter\n>    name, let's not be original and stick to \"_data\" like other\n>    existing codepath.  An alternative with more meaningful name is\n>    of course better, though, but it is not all that much worth to\n>    spend too many braincycles on it.\n\nagreed.\n\n>  * Is the fact that these parameters necessary to help populating\n>    the submodule repository are sent on a line to external process\n>    the most important aspect of the submodule_lines[] array?  As\n\nIn terms of what the submodule--helper does, it is.\nIt is not the most important aspect in the big picture, or once\nwe finish the migration to not have shell interpret its output.\n\n>    this step is a preparation to migrate out of that line/text\n>    oriented IPC, I think line-ness is the least important and\n>    interesting thing to name the variable with.\n\nok.\n\n> If I were writing this patch, perhaps I'd do\n>\n>         struct update_clone_data *update_clone;\n>         int update_clone_nr, update_clone_alloc;\n>\n> following my gut, but since you've been thinking about submodule\n> longer than I have, perhaps you can come up with a better name.\n\nThat makes sense. We do not need to mention 'submodule' as that\nought to be obvious from the file name already and 'update_clone'\nis just enough to describe what we are doing.\nAlthough there is already struct submodule_update_clone, which\nwould be the better candidate for 'update_clone' and this new\nstruct would be used as a helper in that struct, so update_clone_data\n\nI'll think of adding a patch to rename that already existing struct\n(submodule_update_clone -> update_clone) and renaming\nthis to update_clone_data.\n\n\n\n>\n> > @@ -1463,8 +1469,9 @@ struct submodule_update_clone {\n> >       const char *recursive_prefix;\n> >       const char *prefix;\n> >\n> > -     /* Machine-readable status lines to be consumed by git-submodule.sh */\n> > -     struct string_list projectlines;\n> > +     /* to be consumed by git-submodule.sh */\n> > +     struct submodule_update_clone_information *submodule_lines;\n> > +     int submodule_lines_nr; int submodule_lines_alloc;\n> >\n> >       /* If we want to stop as fast as possible and return an error */\n> >       unsigned quickstop : 1;\n> > @@ -1478,7 +1485,7 @@ struct submodule_update_clone {\n> >  #define SUBMODULE_UPDATE_CLONE_INIT {0, MODULE_LIST_INIT, 0, \\\n> >       SUBMODULE_UPDATE_STRATEGY_INIT, 0, 0, -1, STRING_LIST_INIT_DUP, 0, \\\n> >       NULL, NULL, NULL, \\\n> > -     STRING_LIST_INIT_DUP, 0, NULL, 0, 0}\n> > +     NULL, 0, 0, 0, NULL, 0, 0, 0}\n>\n> The structure definition and this macro definition are nearby, so it\n> is not crucial, but its probably not a bad idea to switch to C99\n> initializers for a thing like this to make it more readable, once\n> the code around this area stabilizes back again sufficiently (IOW,\n> let's not distract ourselves in the middle of adding a new feature\n> like this one).\n\nAre we still in the phase of \"test balloon\" or do we now accept\nC99 initializers all over the code base?\n\nBut I agree to defer the conversion for now.\n\nThanks,\nStefan\n"},{"id":"352870","messageId":"e5366f75-3669-bfbb-2561-ee06751b923d@gmail.com","threadId":"48872","inReplyTo":"20180712194754.71979-7-sbeller@google.com","subject":"Re: [PATCH 6/6] submodule--helper: introduce new update-module-mode helper","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-07-18T15:00:53Z","receivedAt":"2018-07-18T15:00:59Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 7/12/2018 3:47 PM, Stefan Beller wrote:\n> +\tfprintf(stdout, submodule_strategy_to_string(&update_strategy));\n\nThis line is causing build failures on 'pu' for certain setups:\n\n2018-07-18T14:53:03.6857987Z builtin/submodule--helper.c: In function \n‘module_update_module_mode’:\n2018-07-18T14:53:03.6874886Z builtin/submodule--helper.c:1504:2: error: \nformat not a string literal and no format arguments \n[-Werror=format-security]\n2018-07-18T14:53:03.6890814Z   fprintf(stdout, \nsubmodule_strategy_to_string(&update_strategy));\n2018-07-18T14:53:03.6905905Z   ^\n\nThanks,\n\n-Stolee\n\n"},{"id":"352961","messageId":"CAGZ79ka1Y4ocnES8=iKv+GuCBpuprrjrasU3UQ04CmUQu8QxyQ@mail.gmail.com","threadId":"48872","inReplyTo":"e5366f75-3669-bfbb-2561-ee06751b923d@gmail.com","subject":"Re: [PATCH 6/6] submodule--helper: introduce new update-module-mode helper","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-07-18T19:41:38Z","receivedAt":"2018-07-18T19:41:51Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Jul 18, 2018 at 8:00 AM Derrick Stolee <stolee@gmail.com> wrote:\n>\n> On 7/12/2018 3:47 PM, Stefan Beller wrote:\n> > +     fprintf(stdout, submodule_strategy_to_string(&update_strategy));\n>\n> This line is causing build failures on 'pu' for certain setups:\n>\n\nyeah, will fix in a resend.\noriginally reported at\nhttps://public-inbox.org/git/20180717075959.30594-1-szeder.dev@gmail.com/\n"}]}