{"thread":{"id":"56147","subject":"[GSoC] [PATCH] submodule--helper: run update procedures from C","startedAt":"2021-07-22T13:42:16Z","lastAt":"2021-09-08T00:14:36Z","messageCount":13,"participants":["Atharva Raykar","Ævar Arnfjörð Bjarmason","Shourya Shukla","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"430932","messageId":"20210722134012.99457-1-raykar.ath@gmail.com","threadId":"56147","inReplyTo":null,"subject":"[GSoC] [PATCH] submodule--helper: run update procedures from C","fromName":"Atharva Raykar","fromEmail":"raykar.ath@gmail.com","sentAt":"2021-07-22T13:40:12Z","receivedAt":"2021-07-22T13:42:16Z","isPatch":true,"sender":{"key":"raykar.ath@gmail.com","avatar":"https://avatars.githubusercontent.com/u/24277692?v=4"},"body":"Add a new submodule--helper subcommand `run-update-procedure` that runs\nthe update procedure if the SHA1 of the submodule does not match what\nthe superproject expects.\n\nThis is an intermediate change that works towards total conversion of\n`submodule update` from shell to C.\n\nSpecific error codes are returned so that the shell script calling the\nsubcommand can take a decision on the control flow, and preserve the\nerror messages across subsequent recursive calls of `cmd_update`.\n\nThis patch could have been approached differently, by first changing the\n`is_tip_reachable` and `fetch_in_submodule` shell functions to be\n`submodule--helper` subcommands, and then following up with a patch that\nintroduces the `run-update-procedure` subcommand. We have not done it\nlike that because those functions are trivial enough to convert directly\nalong with these other changes. This lets us avoid the boilerplate and\nthe cleanup patches that will need to be introduced in following that\napproach.\n\nSigned-off-by: Atharva Raykar \nMentored-by: Christian Couder \nMentored-by: Shourya Shukla \n---\n\nThis patch depends on changes introduced in 559e49fe5c (submodule: prefix die\nmessages with 'fatal', 2021-07-08), which belongs to the ar/submodule-add\n(2021-07-12) series[1]. Other than that commit, it is independent of my other\n'submodule add' conversion series.\n\nOpinions on the following would be appreciated:\n\n* Currently there is a lot of special meaning for the exit code, of this\n subcommand, which was needed to handle the various failures of running the\n update mode in the shell code that follows (note the extra handling of exit\n code 128, because a die() in C returns that value). I felt this was okay to do\n because in a later series that converts whatever is left, the handling of exit\n codes will be simplified.\n\n* Is there a way to check if a sha1 is unreachable from all the refs?\n Currently 'is_tip_reachable()' spawns a subprocess with an incantation of:\n 'git rev-list -n 1 $sha1 --not --all'\n I suppose I could do this with the revision-walk API [2], but it felt like\n a lot of boilerplate for something that was really succinct in the original\n shell implementation. Maybe worth looking into it for a later patch?\n\n* I added a 'NOTE' comment for `case SM_UPDATE_COMMAND` in\n submodule--helper.c:run_update_command(). I wonder if that comment is\n unnecessary noise or worth mentioning. Is there an edge case where the\n !command in the 'submodule.update' configuration can be improperly handled by\n strvec_split()?\n\n[1] https://lore.kernel.org/git/20210710074801.19917-1-raykar.ath@gmail.com/\n\nFetch-it-via:\ngit fetch https://github.com/tfidfwastaken/git.git submodule-run-update-proc-list-1\n\n builtin/submodule--helper.c | 247 ++++++++++++++++++++++++++++++++++++\n git-submodule.sh | 95 ++++----------\n 2 files changed, 269 insertions(+), 73 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex d55f6262e9..4e16561bf1 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2029,6 +2029,20 @@ struct submodule_update_clone {\n \t.max_jobs = 1, \\\n }\n \n+struct update_data {\n+\tconst char *recursive_prefix;\n+\tconst char *sm_path;\n+\tconst char *displaypath;\n+\tstruct object_id sha1;\n+\tstruct object_id subsha1;\n+\tstruct submodule_update_strategy update_strategy;\n+\tint depth;\n+\tunsigned int force: 1;\n+\tunsigned int quiet: 1;\n+\tunsigned int nofetch: 1;\n+\tunsigned int just_cloned: 1;\n+};\n+#define UPDATE_DATA_INIT { .update_strategy = SUBMODULE_UPDATE_STRATEGY_INIT }\n \n static void next_submodule_warn_missing(struct submodule_update_clone *suc,\n \t\tstruct strbuf *out, const char *displaypath)\n@@ -2282,6 +2296,165 @@ static int git_update_clone_config(const char *var, const char *value,\n \treturn 0;\n }\n \n+/* NEEDSWORK: try to do this without creating a new process */\n+static int is_tip_reachable(const char *path, struct object_id *sha1)\n+{\n+\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\tstruct strbuf rev = STRBUF_INIT;\n+\tchar *sha1_hex = oid_to_hex(sha1);\n+\n+\tcp.git_cmd = 1;\n+\tcp.dir = xstrdup(path);\n+\tcp.no_stderr = 1;\n+\tstrvec_pushl(&cp.args, \"rev-list\", \"-n\", \"1\", sha1_hex, \"--not\", \"--all\", NULL);\n+\n+\tprepare_submodule_repo_env(&cp.env_array);\n+\n+\tif (capture_command(&cp, &rev, GIT_MAX_HEXSZ + 1) || rev.len)\n+\t\treturn 0;\n+\n+\treturn 1;\n+}\n+\n+static int fetch_in_submodule(const char *module_path, int depth, int quiet, struct object_id *sha1)\n+{\n+\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\n+\tprepare_submodule_repo_env(&cp.env_array);\n+\tcp.git_cmd = 1;\n+\tcp.dir = xstrdup(module_path);\n+\n+\tstrvec_push(&cp.args, \"fetch\");\n+\tif (quiet)\n+\t\tstrvec_push(&cp.args, \"--quiet\");\n+\tif (depth)\n+\t\tstrvec_pushf(&cp.args, \"--depth=%d\", depth);\n+\tif (sha1) {\n+\t\tchar *sha1_hex = oid_to_hex(sha1);\n+\t\tchar *remote = get_default_remote();\n+\t\tstrvec_pushl(&cp.args, remote, sha1_hex, NULL);\n+\t}\n+\n+\treturn run_command(&cp);\n+}\n+\n+static int run_update_command(struct update_data *ud, int subforce)\n+{\n+\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\tstruct strbuf die_msg = STRBUF_INIT;\n+\tstruct strbuf say_msg = STRBUF_INIT;\n+\tchar *sha1 = oid_to_hex(&ud->sha1);\n+\tint retval, must_die_on_failure = 0;\n+\n+\tcp.dir = xstrdup(ud->sm_path);\n+\tswitch (ud->update_strategy.type) {\n+\tcase SM_UPDATE_CHECKOUT:\n+\t\tcp.git_cmd = 1;\n+\t\tstrvec_pushl(&cp.args, \"checkout\", \"-q\", NULL);\n+\t\tif (subforce)\n+\t\t\tstrvec_push(&cp.args, \"-f\");\n+\t\tstrbuf_addf(&die_msg, \"fatal: Unable to checkout '%s' in submodule path '%s'\\n\",\n+\t\t\t sha1, ud->displaypath);\n+\t\tstrbuf_addf(&say_msg, \"Submodule path '%s': checked out '%s'\\n\",\n+\t\t\t ud->displaypath, sha1);\n+\t\tbreak;\n+\tcase SM_UPDATE_REBASE:\n+\t\tcp.git_cmd = 1;\n+\t\tstrvec_push(&cp.args, \"rebase\");\n+\t\tif (ud->quiet)\n+\t\t\tstrvec_push(&cp.args, \"--quiet\");\n+\t\tstrbuf_addf(&die_msg, \"fatal: Unable to rebase '%s' in submodule path '%s'\\n\",\n+\t\t\t sha1, ud->displaypath);\n+\t\tstrbuf_addf(&say_msg, \"Submodule path '%s': rebased into '%s'\\n\",\n+\t\t\t ud->displaypath, sha1);\n+\t\tmust_die_on_failure = 1;\n+\t\tbreak;\n+\tcase SM_UPDATE_MERGE:\n+\t\tcp.git_cmd = 1;\n+\t\tstrvec_push(&cp.args, \"merge\");\n+\t\tif (ud->quiet)\n+\t\t\tstrvec_push(&cp.args, \"--quiet\");\n+\t\tstrbuf_addf(&die_msg, \"fatal: Unable to merge '%s' in submodule path '%s'\\n\",\n+\t\t\t sha1, ud->displaypath);\n+\t\tstrbuf_addf(&say_msg, \"Submodule path '%s': merged in '%s'\\n\",\n+\t\t\t ud->displaypath, sha1);\n+\t\tmust_die_on_failure = 1;\n+\t\tbreak;\n+\tcase SM_UPDATE_COMMAND:\n+\t\t/* NOTE: this does not handle quoted arguments */\n+\t\tstrvec_split(&cp.args, ud->update_strategy.command);\n+\t\tstrbuf_addf(&die_msg, \"fatal: Execution of '%s %s' failed in submodule path '%s'\\n\",\n+\t\t\t ud->update_strategy.command, sha1, ud->displaypath);\n+\t\tstrbuf_addf(&say_msg, \"Submodule path '%s': '%s %s'\\n\",\n+\t\t\t ud->displaypath, ud->update_strategy.command, sha1);\n+\t\tmust_die_on_failure = 1;\n+\t\tbreak;\n+\tcase SM_UPDATE_UNSPECIFIED:\n+\tcase SM_UPDATE_NONE:\n+\t\tBUG(\"update strategy should have been specified\");\n+\t}\n+\n+\tstrvec_push(&cp.args, sha1);\n+\n+\tprepare_submodule_repo_env(&cp.env_array);\n+\n+\tif (run_command(&cp)) {\n+\t\tif (must_die_on_failure) {\n+\t\t\tretval = 2;\n+\t\t\tfputs(_(die_msg.buf), stderr);\n+\t\t\tgoto cleanup;\n+\t\t}\n+\t\t/*\n+\t\t * This signifies to the caller in shell that\n+\t\t * the command failed without dying\n+\t\t */\n+\t\tretval = 1;\n+\t\tgoto cleanup;\n+\t}\n+\tretval = 0;\n+\tputs(_(say_msg.buf));\n+\n+cleanup:\n+\tstrbuf_release(&die_msg);\n+\tstrbuf_release(&say_msg);\n+\treturn retval;\n+}\n+\n+static int do_run_update_procedure(struct update_data *ud)\n+{\n+\tif ((!is_null_oid(&ud->sha1) && !is_null_oid(&ud->subsha1) && !oideq(&ud->sha1, &ud->subsha1)) ||\n+\t is_null_oid(&ud->subsha1) || ud->force) {\n+\t\tint subforce = is_null_oid(&ud->subsha1) || ud->force;\n+\n+\t\tif (!ud->nofetch) {\n+\t\t\t/*\n+\t\t\t * Run fetch only if `sha1` isn't present or it\n+\t\t\t * is not reachable from a ref.\n+\t\t\t */\n+\t\t\tif (!is_tip_reachable(ud->sm_path, &ud->sha1))\n+\t\t\t\tif (fetch_in_submodule(ud->sm_path, ud->depth, ud->quiet, NULL) &&\n+\t\t\t\t !ud->quiet)\n+\t\t\t\t\tfprintf_ln(stderr,\n+\t\t\t\t\t\t _(\"Unable to fetch in submodule path '%s'; \"\n+\t\t\t\t\t\t \"trying to directly fetch %s:\"),\n+\t\t\t\t\t\t ud->displaypath, oid_to_hex(&ud->sha1));\n+\t\t\t/*\n+\t\t\t * Now we tried the usual fetch, but `sha1` may\n+\t\t\t * not be reachable from any of the refs.\n+\t\t\t */\n+\t\t\tif (!is_tip_reachable(ud->sm_path, &ud->sha1))\n+\t\t\t\tif (fetch_in_submodule(ud->sm_path, ud->depth, ud->quiet, &ud->sha1))\n+\t\t\t\t\tdie(_(\"Fetched in submodule path '%s', but it did not \"\n+\t\t\t\t\t \"contain %s. Direct fetching of that commit failed.\"),\n+\t\t\t\t\t ud->displaypath, oid_to_hex(&ud->sha1));\n+\t\t}\n+\n+\t\treturn run_update_command(ud, subforce);\n+\t}\n+\n+\treturn 3;\n+}\n+\n static void update_submodule(struct update_clone_data *ucd)\n {\n \tfprintf(stdout, \"dummy %s %d\\t%s\\n\",\n@@ -2379,6 +2552,79 @@ static int update_clone(int argc, const char **argv, const char *prefix)\n \treturn update_submodules(&suc);\n }\n \n+static int run_update_procedure(int argc, const char **argv, const char *prefix)\n+{\n+\tint force = 0, quiet = 0, nofetch = 0, just_cloned = 0;\n+\tchar *prefixed_path, *update = NULL;\n+\tchar *sha1 = NULL, *subsha1 = NULL;\n+\tstruct update_data update_data = UPDATE_DATA_INIT;\n+\n+\tstruct option options[] = {\n+\t\tOPT__QUIET(&quiet, N_(\"suppress output for update by rebase or merge\")),\n+\t\tOPT__FORCE(&force, N_(\"force checkout updates\"), 0),\n+\t\tOPT_BOOL('N', \"no-fetch\", &nofetch,\n+\t\t\t N_(\"don't fetch new objects from the remote site\")),\n+\t\tOPT_BOOL(0, \"just-cloned\", &just_cloned,\n+\t\t\t N_(\"overrides update mode in case the repository is a fresh clone\")),\n+\t\tOPT_INTEGER(0, \"depth\", &update_data.depth, N_(\"depth for shallow fetch\")),\n+\t\tOPT_STRING(0, \"prefix\", &prefix,\n+\t\t\t N_(\"path\"),\n+\t\t\t N_(\"path into the working tree\")),\n+\t\tOPT_STRING(0, \"update\", &update,\n+\t\t\t N_(\"string\"),\n+\t\t\t N_(\"rebase, merge, checkout or none\")),\n+\t\tOPT_STRING(0, \"recursive-prefix\", &update_data.recursive_prefix, N_(\"path\"),\n+\t\t\t N_(\"path into the working tree, across nested \"\n+\t\t\t \"submodule boundaries\")),\n+\t\tOPT_STRING(0, \"sha1\", &sha1, N_(\"string\"),\n+\t\t\t N_(\"SHA1 expected by superproject\")),\n+\t\tOPT_STRING(0, \"subsha1\", &subsha1, N_(\"string\"),\n+\t\t\t N_(\"SHA1 of submodule's HEAD\")),\n+\t\tOPT_END()\n+\t};\n+\n+\tconst char *const usage[] = {\n+\t\tN_(\"git submodule--helper run-update-procedure [] \"),\n+\t\tNULL\n+\t};\n+\n+\targc = parse_options(argc, argv, prefix, options, usage, 0);\n+\n+\tif (argc != 1)\n+\t\tusage_with_options(usage, options);\n+\n+\tupdate_data.force = !!force;\n+\tupdate_data.quiet = !!quiet;\n+\tupdate_data.nofetch = !!nofetch;\n+\tupdate_data.just_cloned = !!just_cloned;\n+\tupdate_data.sm_path = argv[0];\n+\n+\tif (sha1)\n+\t\tget_oid_hex(sha1, &update_data.sha1);\n+\telse\n+\t\toidcpy(&update_data.sha1, null_oid());\n+\n+\tif (subsha1)\n+\t\tget_oid_hex(subsha1, &update_data.subsha1);\n+\telse\n+\t\toidcpy(&update_data.subsha1, null_oid());\n+\n+\tif (update_data.recursive_prefix)\n+\t\tprefixed_path = xstrfmt(\"%s%s\", update_data.recursive_prefix, update_data.sm_path);\n+\telse\n+\t\tprefixed_path = xstrdup(update_data.sm_path);\n+\n+\tupdate_data.displaypath = get_submodule_displaypath(prefixed_path, prefix);\n+\n+\tdetermine_submodule_update_strategy(the_repository, update_data.just_cloned,\n+\t\t\t\t\t update_data.sm_path, update,\n+\t\t\t\t\t &update_data.update_strategy);\n+\n+\tfree(prefixed_path);\n+\n+\treturn do_run_update_procedure(&update_data);\n+}\n+\n static int resolve_relative_path(int argc, const char **argv, const char *prefix)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n@@ -2759,6 +3005,7 @@ static struct cmd_struct commands[] = {\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{\"run-update-procedure\", run_update_procedure, 0},\n \t{\"ensure-core-worktree\", ensure_core_worktree, 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 1187c21260..4d5437f5c2 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -405,13 +405,6 @@ cmd_deinit()\n \tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper deinit ${GIT_QUIET:+--quiet} ${prefix:+--prefix \"$prefix\"} ${force:+--force} ${deinit_all:+--all} -- \"$@\"\n }\n \n-is_tip_reachable () (\n-\tsanitize_submodule_env &&\n-\tcd \"$1\" &&\n-\trev=$(git rev-list -n 1 \"$2\" --not --all 2>/dev/null) &&\n-\ttest -z \"$rev\"\n-)\n-\n # usage: fetch_in_submodule [] []\n # Because arguments are positional, use an empty string to omit \n # but include .\n@@ -555,14 +548,13 @@ cmd_update()\n \n \t\tgit submodule--helper ensure-core-worktree \"$sm_path\" || exit 1\n \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\telse\n+\t\t\tjust_cloned=\n \t\t\tsubsha1=$(sanitize_submodule_env; cd \"$sm_path\" &&\n \t\t\t\tgit rev-parse --verify HEAD) ||\n \t\t\tdie \"$(eval_gettext \"fatal: Unable to find current revision in submodule path '\\$displaypath'\")\"\n@@ -583,70 +575,27 @@ cmd_update()\n \t\t\tdie \"$(eval_gettext \"fatal: Unable to find current \\${remote_name}/\\${branch} revision in submodule path '\\$sm_path'\")\"\n \t\tfi\n \n-\t\tif test \"$subsha1\" != \"$sha1\" || test -n \"$force\"\n-\t\tthen\n-\t\t\tsubforce=$force\n-\t\t\t# If we don't already have a -f flag and the submodule has never been checked out\n-\t\t\tif test -z \"$subsha1\" && test -z \"$force\"\n-\t\t\tthen\n-\t\t\t\tsubforce=\"-f\"\n-\t\t\tfi\n+\t\tout=$(git submodule--helper run-update-procedure ${wt_prefix:+--prefix \"$wt_prefix\"} ${GIT_QUIET:+--quiet} ${force:+--force} ${just_cloned:+--just-cloned} ${nofetch:+--no-fetch} ${depth:+\"$depth\"} ${update:+--update \"$update\"} ${prefix:+--recursive-prefix \"$prefix\"} ${sha1:+--sha1 \"$sha1\"} ${subsha1:+--subsha1 \"$subsha1\"} \"$sm_path\")\n \n-\t\t\tif test -z \"$nofetch\"\n-\t\t\tthen\n-\t\t\t\t# Run fetch only if $sha1 isn't present or it\n-\t\t\t\t# is not reachable from a ref.\n-\t\t\t\tis_tip_reachable \"$sm_path\" \"$sha1\" ||\n-\t\t\t\tfetch_in_submodule \"$sm_path\" $depth ||\n-\t\t\t\tsay \"$(eval_gettext \"Unable to fetch in submodule path '\\$displaypath'; trying to directly fetch \\$sha1:\")\"\n-\n-\t\t\t\t# Now we tried the usual fetch, but $sha1 may\n-\t\t\t\t# not be reachable from any of the refs\n-\t\t\t\tis_tip_reachable \"$sm_path\" \"$sha1\" ||\n-\t\t\t\tfetch_in_submodule \"$sm_path\" \"$depth\" \"$sha1\" ||\n-\t\t\t\tdie \"$(eval_gettext \"Fetched in submodule path '\\$displaypath', but it did not contain \\$sha1. Direct fetching of that commit failed.\")\"\n-\t\t\tfi\n-\n-\t\t\tmust_die_on_failure=\n-\t\t\tcase \"$update_module\" in\n-\t\t\tcheckout)\n-\t\t\t\tcommand=\"git checkout $subforce -q\"\n-\t\t\t\tdie_msg=\"$(eval_gettext \"Unable to checkout '\\$sha1' in submodule path '\\$displaypath'\")\"\n-\t\t\t\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': checked out '\\$sha1'\")\"\n-\t\t\t\t;;\n-\t\t\trebase)\n-\t\t\t\tcommand=\"git rebase ${GIT_QUIET:+--quiet}\"\n-\t\t\t\tdie_msg=\"$(eval_gettext \"Unable to rebase '\\$sha1' in submodule path '\\$displaypath'\")\"\n-\t\t\t\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': rebased into '\\$sha1'\")\"\n-\t\t\t\tmust_die_on_failure=yes\n-\t\t\t\t;;\n-\t\t\tmerge)\n-\t\t\t\tcommand=\"git merge ${GIT_QUIET:+--quiet}\"\n-\t\t\t\tdie_msg=\"$(eval_gettext \"Unable to merge '\\$sha1' in submodule path '\\$displaypath'\")\"\n-\t\t\t\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': merged in '\\$sha1'\")\"\n-\t\t\t\tmust_die_on_failure=yes\n-\t\t\t\t;;\n-\t\t\t!*)\n-\t\t\t\tcommand=\"${update_module#!}\"\n-\t\t\t\tdie_msg=\"$(eval_gettext \"Execution of '\\$command \\$sha1' failed in submodule path '\\$displaypath'\")\"\n-\t\t\t\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': '\\$command \\$sha1'\")\"\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 path '$path'\")\"\n-\t\t\tesac\n-\n-\t\t\tif (sanitize_submodule_env; cd \"$sm_path\" && $command \"$sha1\")\n-\t\t\tthen\n-\t\t\t\tsay \"$say_msg\"\n-\t\t\telif test -n \"$must_die_on_failure\"\n-\t\t\tthen\n-\t\t\t\tdie_with_status 2 \"$die_msg\"\n-\t\t\telse\n-\t\t\t\terr=\"${err};$die_msg\"\n-\t\t\t\tcontinue\n-\t\t\tfi\n-\t\tfi\n+\t\t# exit codes for run-update-procedure:\n+\t\t# 0: update was successful, say command output\n+\t\t# 128: subcommand died during execution\n+\t\t# 1: update procedure failed and must die\n+\t\t# 2: update procedure failed, but should not die\n+\t\t# 3: no update procedure was run\n+\t\tres=\"$?\"\n+\t\tcase $res in\n+\t\t0)\n+\t\t\tsay \"$out\"\n+\t\t\t;;\n+\t\t2|128)\n+\t\t\texit $res\n+\t\t\t;;\n+\t\t1)\n+\t\t\terr=\"${err};$out\"\n+\t\t\tcontinue\n+\t\t\t;;\n+\t\tesac\n \n \t\tif test -n \"$recursive\"\n \t\tthen\n@@ -661,7 +610,7 @@ cmd_update()\n \t\t\tif test $res -gt 0\n \t\t\tthen\n \t\t\t\tdie_msg=\"$(eval_gettext \"fatal: Failed to recurse into submodule path '\\$displaypath'\")\"\n-\t\t\t\tif test $res -ne 2\n+\t\t\t\tif test $res -ne 2 && test $res -ne 128\n \t\t\t\tthen\n \t\t\t\t\terr=\"${err};$die_msg\"\n \t\t\t\t\tcontinue\n-- \n2.32.0\n\n"},{"id":"431015","messageId":"87r1fps63r.fsf@evledraar.gmail.com","threadId":"56147","inReplyTo":"20210722134012.99457-1-raykar.ath@gmail.com","subject":"Re: [GSoC] [PATCH] submodule--helper: run update procedures from C","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-23T09:37:46Z","receivedAt":"2021-07-23T09:59:35Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Jul 22 2021, Atharva Raykar wrote:\n\n> +/* NEEDSWORK: try to do this without creating a new process */\n> +static int is_tip_reachable(const char *path, struct object_id *sha1)\n> +{\n> +\tstruct child_process cp = CHILD_PROCESS_INIT;\n> +\tstruct strbuf rev = STRBUF_INIT;\n> +\tchar *sha1_hex = oid_to_hex(sha1);\n> +\n> +\tcp.git_cmd = 1;\n> +\tcp.dir = xstrdup(path);\n> +\tcp.no_stderr = 1;\n> +\tstrvec_pushl(&cp.args, \"rev-list\", \"-n\", \"1\", sha1_hex, \"--not\", \"--all\", NULL);\n> +\n> +\tprepare_submodule_repo_env(&cp.env_array);\n> +\n> +\tif (capture_command(&cp, &rev, GIT_MAX_HEXSZ + 1) || rev.len)\n> +\t\treturn 0;\n> +\n> +\treturn 1;\n> +}\n\nI think it's fine to do this & leave out the NEEDSWORK commit, just\nbriefly noting in the commit-message that we're not bothering with\ntrying to reduce sub-command invocations. It can be done later if anyone\ncares.\n\n> [...]\n> +\t\tstrbuf_addf(&die_msg, \"fatal: Unable to checkout '%s' in submodule path '%s'\\n\",\n> +\t\t\t sha1, ud->displaypath);\n> +\t\tstrbuf_addf(&say_msg, \"Submodule path '%s': checked out '%s'\\n\",\n> +\t\t\t ud->displaypath, sha1);\n\nFor all of these you're removing the translation from a message like:\n\n die_msg=\"$(eval_gettext \"Unable to checkout '\\$sha1' in submodule path '\\$displaypath'\")\"\n\nWhich is easy enough to fix, just use _(), i.e.:\n\n strbuf_addf(&die_msg, _(\"Unable to checkout '%s' in submodule path '%s'\\n\"), [...]\n\nI removed the \"fatal: \" per a comment below...\n\n> +\t\tbreak;\n> +\tcase SM_UPDATE_REBASE:\n> +\t\tcp.git_cmd = 1;\n> +\t\tstrvec_push(&cp.args, \"rebase\");\n> +\t\tif (ud->quiet)\n> +\t\t\tstrvec_push(&cp.args, \"--quiet\");\n> +\t\tstrbuf_addf(&die_msg, \"fatal: Unable to rebase '%s' in submodule path '%s'\\n\",\n> +\t\t\t sha1, ud->displaypath);\n> +\t\tstrbuf_addf(&say_msg, \"Submodule path '%s': rebased into '%s'\\n\",\n> +\t\t\t ud->displaypath, sha1);\n> +\t\tmust_die_on_failure = 1;\n> +\t\tbreak;\n> +\tcase SM_UPDATE_MERGE:\n> +\t\tcp.git_cmd = 1;\n> +\t\tstrvec_push(&cp.args, \"merge\");\n> +\t\tif (ud->quiet)\n> +\t\t\tstrvec_push(&cp.args, \"--quiet\");\n> +\t\tstrbuf_addf(&die_msg, \"fatal: Unable to merge '%s' in submodule path '%s'\\n\",\n> +\t\t\t sha1, ud->displaypath);\n> +\t\tstrbuf_addf(&say_msg, \"Submodule path '%s': merged in '%s'\\n\",\n> +\t\t\t ud->displaypath, sha1);\n> +\t\tmust_die_on_failure = 1;\n> +\t\tbreak;\n> +\tcase SM_UPDATE_COMMAND:\n> +\t\t/* NOTE: this does not handle quoted arguments */\n> +\t\tstrvec_split(&cp.args, ud->update_strategy.command);\n> +\t\tstrbuf_addf(&die_msg, \"fatal: Execution of '%s %s' failed in submodule path '%s'\\n\",\n> +\t\t\t ud->update_strategy.command, sha1, ud->displaypath);\n> +\t\tstrbuf_addf(&say_msg, \"Submodule path '%s': '%s %s'\\n\",\n> +\t\t\t ud->displaypath, ud->update_strategy.command, sha1);\n> +\t\tmust_die_on_failure = 1;\n> +\t\tbreak;\n> +\tcase SM_UPDATE_UNSPECIFIED:\n> +\tcase SM_UPDATE_NONE:\n> +\t\tBUG(\"update strategy should have been specified\");\n> +\t}\n> +\n> +\tstrvec_push(&cp.args, sha1);\n> +\n> +\tprepare_submodule_repo_env(&cp.env_array);\n> +\n> +\tif (run_command(&cp)) {\n> +\t\tif (must_die_on_failure) {\n> +\t\t\tretval = 2;\n> +\t\t\tfputs(_(die_msg.buf), stderr);\n> +\t\t\tgoto cleanup;\n\nFWIW I'd find this clearer if we just kept track of what operation we\nran above, and just in this run_command() && must_die_on_failure case\nstarted populating these die messages.\n\nBut even if not the reason I dropped the \"fatal: \" is shouldn't we just\ncall die() here directly? Why clean up when we're dying anyway?\n\nAlso since I see you used _() here that won't work, i.e. with gettet if\nyou happen to need to declare things earlier, you need to use N_() to\nmark the message for translation.\n\nThe _() here won't find any message translated (unless the string\nhappened to exactly match a thing in the *.po file for other reasons,\nnot the case here).\n\nBut in this case we can just die(msg) here and have used the _() above,\nor just call die() directly here not having made a die_msg we usually\nwon't use...\n\n> +static int do_run_update_procedure(struct update_data *ud)\n> +{\n> +\tif ((!is_null_oid(&ud->sha1) && !is_null_oid(&ud->subsha1) && !oideq(&ud->sha1, &ud->subsha1)) ||\n> +\t is_null_oid(&ud->subsha1) || ud->force) {\n> +\t\tint subforce = is_null_oid(&ud->subsha1) || ud->force;\n> +\n> +\t\tif (!ud->nofetch) {\n> +\t\t\t/*\n> +\t\t\t * Run fetch only if `sha1` isn't present or it\n> +\t\t\t * is not reachable from a ref.\n> +\t\t\t */\n> +\t\t\tif (!is_tip_reachable(ud->sm_path, &ud->sha1))\n> +\t\t\t\tif (fetch_in_submodule(ud->sm_path, ud->depth, ud->quiet, NULL) &&\n> +\t\t\t\t !ud->quiet)\n> +\t\t\t\t\tfprintf_ln(stderr,\n> +\t\t\t\t\t\t _(\"Unable to fetch in submodule path '%s'; \"\n> +\t\t\t\t\t\t \"trying to directly fetch %s:\"),\n> +\t\t\t\t\t\t ud->displaypath, oid_to_hex(&ud->sha1));\n> +\t\t\t/*\n> +\t\t\t * Now we tried the usual fetch, but `sha1` may\n> +\t\t\t * not be reachable from any of the refs.\n> +\t\t\t */\n> +\t\t\tif (!is_tip_reachable(ud->sm_path, &ud->sha1))\n> +\t\t\t\tif (fetch_in_submodule(ud->sm_path, ud->depth, ud->quiet, &ud->sha1))\n> +\t\t\t\t\tdie(_(\"Fetched in submodule path '%s', but it did not \"\n> +\t\t\t\t\t \"contain %s. Direct fetching of that commit failed.\"),\n> +\t\t\t\t\t ud->displaypath, oid_to_hex(&ud->sha1));\n> +\t\t}\n> +\n> +\t\treturn run_update_command(ud, subforce);\n> +\t}\n> +\n> +\treturn 3;\n> +}\n\nSince this has excatly one caller I think it's better for readability\n(less indentation) and flow to just remove that \"return 3\" condition and\ndo the big \"if\" you have at the end, i.e. have this function start with\n\"int subforce =\" and...\n\n> static void update_submodule(struct update_clone_data *ucd)\n> {\n> \tfprintf(stdout, \"dummy %s %d\\t%s\\n\",\n> @@ -2379,6 +2552,79 @@ static int update_clone(int argc, const char **argv, const char *prefix)\n> \treturn update_submodules(&suc);\n> }\n> \n> +static int run_update_procedure(int argc, const char **argv, const char *prefix)\n> +{\n> +\tint force = 0, quiet = 0, nofetch = 0, just_cloned = 0;\n> +\tchar *prefixed_path, *update = NULL;\n> +\tchar *sha1 = NULL, *subsha1 = NULL;\n> +\tstruct update_data update_data = UPDATE_DATA_INIT;\n> +\n> +\tstruct option options[] = {\n> +\t\tOPT__QUIET(&quiet, N_(\"suppress output for update by rebase or merge\")),\n> +\t\tOPT__FORCE(&force, N_(\"force checkout updates\"), 0),\n> +\t\tOPT_BOOL('N', \"no-fetch\", &nofetch,\n> +\t\t\t N_(\"don't fetch new objects from the remote site\")),\n> +\t\tOPT_BOOL(0, \"just-cloned\", &just_cloned,\n> +\t\t\t N_(\"overrides update mode in case the repository is a fresh clone\")),\n> +\t\tOPT_INTEGER(0, \"depth\", &update_data.depth, N_(\"depth for shallow fetch\")),\n> +\t\tOPT_STRING(0, \"prefix\", &prefix,\n> +\t\t\t N_(\"path\"),\n> +\t\t\t N_(\"path into the working tree\")),\n> +\t\tOPT_STRING(0, \"update\", &update,\n> +\t\t\t N_(\"string\"),\n> +\t\t\t N_(\"rebase, merge, checkout or none\")),\n> +\t\tOPT_STRING(0, \"recursive-prefix\", &update_data.recursive_prefix, N_(\"path\"),\n> +\t\t\t N_(\"path into the working tree, across nested \"\n> +\t\t\t \"submodule boundaries\")),\n> +\t\tOPT_STRING(0, \"sha1\", &sha1, N_(\"string\"),\n> +\t\t\t N_(\"SHA1 expected by superproject\")),\n> +\t\tOPT_STRING(0, \"subsha1\", &subsha1, N_(\"string\"),\n> +\t\t\t N_(\"SHA1 of submodule's HEAD\")),\n> +\t\tOPT_END()\n> +\t};\n> +\n> +\tconst char *const usage[] = {\n> +\t\tN_(\"git submodule--helper run-update-procedure [] \"),\n> +\t\tNULL\n> +\t};\n> +\n> +\targc = parse_options(argc, argv, prefix, options, usage, 0);\n> +\n> +\tif (argc != 1)\n> +\t\tusage_with_options(usage, options);\n> +\tupdate_data.force = !!force;\n> +\tupdate_data.quiet = !!quiet;\n> +\tupdate_data.nofetch = !!nofetch;\n> +\tupdate_data.just_cloned = !!just_cloned;\n\nFor all of these just pass the reference to the update_data variable\ndirectly in the OPT_*(). No need to set an \"int force\", only to copy it\nover to update_data.force. Let's just use the latter only.\n\n> +\n> +\tif (sha1)\n> +\t\tget_oid_hex(sha1, &update_data.sha1);\n> +\telse\n> +\t\toidcpy(&update_data.sha1, null_oid());\n\nNit: Even if a historical option forces us to support --sha1, let's use\n\"oid\" for the variable etc. But in this case the --sha1 is new, no?\nLet's use --object-id or --oid (whatever is more common, I didn't\ncheck)>\n\n> +\n> +\tif (subsha1)\n> +\t\tget_oid_hex(subsha1, &update_data.subsha1);\n> +\telse\n> +\t\toidcpy(&update_data.subsha1, null_oid());\n\nDitto. Also I think for both of these you can re-use\nparse_opt_object_id. See \"squash-onto\" and \"upstream\" in\nbuiltin/rebase.c.\n\nThen you just supply an oid variable directly and let that helper do all\nthe get_oid etc.\n\n> +\tif (update_data.recursive_prefix)\n> +\t\tprefixed_path = xstrfmt(\"%s%s\", update_data.recursive_prefix, update_data.sm_path);\n> +\telse\n> +\t\tprefixed_path = xstrdup(update_data.sm_path);\n> +\n> +\tupdate_data.displaypath = get_submodule_displaypath(prefixed_path, prefix);\n> +\n> +\tdetermine_submodule_update_strategy(the_repository, update_data.just_cloned,\n> +\t\t\t\t\t update_data.sm_path, update,\n> +\t\t\t\t\t &update_data.update_strategy);\n> +\n> +\tfree(prefixed_path);\n> +\n> +\treturn do_run_update_procedure(&update_data);\n\n....(continued from above) ...here just do:\n\n if (that big if condition)\n return do_run_update_procedure(&update_data);\n else\n return 3;\n"},{"id":"431056","messageId":"9532C3EF-257E-4898-8C75-C49EA4B66A99@gmail.com","threadId":"56147","inReplyTo":"87r1fps63r.fsf@evledraar.gmail.com","subject":"Re: [GSoC] [PATCH] submodule--helper: run update procedures from C","fromName":"Atharva Raykar","fromEmail":"raykar.ath@gmail.com","sentAt":"2021-07-23T16:59:27Z","receivedAt":"2021-07-23T16:59:35Z","isPatch":true,"sender":{"key":"raykar.ath@gmail.com","avatar":"https://avatars.githubusercontent.com/u/24277692?v=4"},"body":"On 23-Jul-2021, at 15:07, Ævar Arnfjörð Bjarmason wrote:\n> \n> \n> On Thu, Jul 22 2021, Atharva Raykar wrote:\n> \n>> +/* NEEDSWORK: try to do this without creating a new process */\n>> +static int is_tip_reachable(const char *path, struct object_id *sha1)\n>> +{\n>> +\tstruct child_process cp = CHILD_PROCESS_INIT;\n>> +\tstruct strbuf rev = STRBUF_INIT;\n>> +\tchar *sha1_hex = oid_to_hex(sha1);\n>> +\n>> +\tcp.git_cmd = 1;\n>> +\tcp.dir = xstrdup(path);\n>> +\tcp.no_stderr = 1;\n>> +\tstrvec_pushl(&cp.args, \"rev-list\", \"-n\", \"1\", sha1_hex, \"--not\", \"--all\", NULL);\n>> +\n>> +\tprepare_submodule_repo_env(&cp.env_array);\n>> +\n>> +\tif (capture_command(&cp, &rev, GIT_MAX_HEXSZ + 1) || rev.len)\n>> +\t\treturn 0;\n>> +\n>> +\treturn 1;\n>> +}\n> \n> I think it's fine to do this & leave out the NEEDSWORK commit, just\n> briefly noting in the commit-message that we're not bothering with\n> trying to reduce sub-command invocations. It can be done later if anyone\n> cares.\n\nOkay, fair enough. I realise I have not been consistent about leaving such\ncomments in all my conversion efforts.\n\n>> [...]\n>> +\t\tstrbuf_addf(&die_msg, \"fatal: Unable to checkout '%s' in submodule path '%s'\\n\",\n>> +\t\t\t sha1, ud->displaypath);\n>> +\t\tstrbuf_addf(&say_msg, \"Submodule path '%s': checked out '%s'\\n\",\n>> +\t\t\t ud->displaypath, sha1);\n> \n> For all of these you're removing the translation from a message like:\n> \n> die_msg=\"$(eval_gettext \"Unable to checkout '\\$sha1' in submodule path '\\$displaypath'\")\"\n> \n> Which is easy enough to fix, just use _(), i.e.:\n> \n> strbuf_addf(&die_msg, _(\"Unable to checkout '%s' in submodule path '%s'\\n\"), [...]\n\nThanks for catching this.\n\n> I removed the \"fatal: \" per a comment below...\n> \n>> +\t\tbreak;\n>> +\tcase SM_UPDATE_REBASE:\n>> +\t\tcp.git_cmd = 1;\n>> +\t\tstrvec_push(&cp.args, \"rebase\");\n>> +\t\tif (ud->quiet)\n>> +\t\t\tstrvec_push(&cp.args, \"--quiet\");\n>> +\t\tstrbuf_addf(&die_msg, \"fatal: Unable to rebase '%s' in submodule path '%s'\\n\",\n>> +\t\t\t sha1, ud->displaypath);\n>> +\t\tstrbuf_addf(&say_msg, \"Submodule path '%s': rebased into '%s'\\n\",\n>> +\t\t\t ud->displaypath, sha1);\n>> +\t\tmust_die_on_failure = 1;\n>> +\t\tbreak;\n>> +\tcase SM_UPDATE_MERGE:\n>> +\t\tcp.git_cmd = 1;\n>> +\t\tstrvec_push(&cp.args, \"merge\");\n>> +\t\tif (ud->quiet)\n>> +\t\t\tstrvec_push(&cp.args, \"--quiet\");\n>> +\t\tstrbuf_addf(&die_msg, \"fatal: Unable to merge '%s' in submodule path '%s'\\n\",\n>> +\t\t\t sha1, ud->displaypath);\n>> +\t\tstrbuf_addf(&say_msg, \"Submodule path '%s': merged in '%s'\\n\",\n>> +\t\t\t ud->displaypath, sha1);\n>> +\t\tmust_die_on_failure = 1;\n>> +\t\tbreak;\n>> +\tcase SM_UPDATE_COMMAND:\n>> +\t\t/* NOTE: this does not handle quoted arguments */\n>> +\t\tstrvec_split(&cp.args, ud->update_strategy.command);\n>> +\t\tstrbuf_addf(&die_msg, \"fatal: Execution of '%s %s' failed in submodule path '%s'\\n\",\n>> +\t\t\t ud->update_strategy.command, sha1, ud->displaypath);\n>> +\t\tstrbuf_addf(&say_msg, \"Submodule path '%s': '%s %s'\\n\",\n>> +\t\t\t ud->displaypath, ud->update_strategy.command, sha1);\n>> +\t\tmust_die_on_failure = 1;\n>> +\t\tbreak;\n>> +\tcase SM_UPDATE_UNSPECIFIED:\n>> +\tcase SM_UPDATE_NONE:\n>> +\t\tBUG(\"update strategy should have been specified\");\n>> +\t}\n>> +\n>> +\tstrvec_push(&cp.args, sha1);\n>> +\n>> +\tprepare_submodule_repo_env(&cp.env_array);\n>> +\n>> +\tif (run_command(&cp)) {\n>> +\t\tif (must_die_on_failure) {\n>> +\t\t\tretval = 2;\n>> +\t\t\tfputs(_(die_msg.buf), stderr);\n>> +\t\t\tgoto cleanup;\n> \n> FWIW I'd find this clearer if we just kept track of what operation we\n> ran above, and just in this run_command() && must_die_on_failure case\n> started populating these die messages.\n\nCould you clarify what you meant here?\n\nI can't think of a way to do this at the moment without introducing one\nor more redundant 'switch ()' statements. This is what I interpreted your\nstatement as:\n\n...\n\t/* we added the arguments to 'cp' already in the switch () above... */\n\n\tif (run_command(&cp)) {\n\t\tif (must_die_on_failure) {\n\t\t\tswitch (ud->update_strategy.type) {\n\t\t\tcase SM_UPDATE_CHECKOUT:\n\t\t\t\tdie(_(\"Execution of...\"));\n\t\t\t\tbreak;\n\t\t\tcase ...\n\t\t\tcase ...\n\t\t}\n\t\t/*\n\t\t * This signifies to the caller in shell that\n\t\t * the command failed without dying\n\t\t */\n\t\tretval = 1;\n\t\tgoto cleanup;\n\t}\n\t\n\t/* ...another switch to figure out which say_msg() to use? */\n...\n\nOr did you mean that instead of storing the die_msg in entirety at the\nfirst switch, I instead store just the action, and in the conditional\nfinally have something like...?\n\n\tdie(_(\"Unable to %s '%s' in submodule path '%s'\"),\n\t action, sha1, ud->displaypath);\n\n...where 'action' is a value like \"merge\", \"checkout\" etc, set in the\nfirst 'switch'.\n\nI am guessing the motivation for this is to make more clear which error\nmessage will be shown for each case?\n\n> But even if not the reason I dropped the \"fatal: \" is shouldn't we just\n> call die() here directly? Why clean up when we're dying anyway?\n\nThe reason I did not call die() directly is because the original shell\nversion originally specifically exited with 2 in the 'must_die_on_error'\ncase while die() in C exits with 128. I think it would be more semantically\ncorrect for me to have done something like an 'exit(2)' instead of cleaning\nup and returning.\n\nThat said, it occured to me that the receiving end does not do anything\nspecial with the different exit code, other than just pass it on to another\nexit, ie, this bit:\n\n+\t\t2|128)\n+\t\t\texit $res\n+\t\t\t;;\n\nThis code currently sits in a weird middle position where it is neither\nfully matching the exit codes as before the conversion (where a failure\nunrelated to the command execution should have exited with 1, not 128),\nnor is it having a complete disregard for their exact value, which would\nsomewhat simplify the failure handling code.\n\nI wonder if any script in a machine somewhere cares about the exact exit value\nof 'submodule add'. I suspect it would be relatively harmless if I just follow\nyour suggestion and just die() on command execution failure...\n\nThoughts?\n\n> Also since I see you used _() here that won't work, i.e. with gettet if\n> you happen to need to declare things earlier, you need to use N_() to\n> mark the message for translation.\n> \n> The _() here won't find any message translated (unless the string\n> happened to exactly match a thing in the *.po file for other reasons,\n> not the case here).\n> \n> But in this case we can just die(msg) here and have used the _() above,\n> or just call die() directly here not having made a die_msg we usually\n> won't use...\n\nOkay, I'll ensure that the translations are marked properly when I reroll.\n\n>> +static int do_run_update_procedure(struct update_data *ud)\n>> +{\n>> +\tif ((!is_null_oid(&ud->sha1) && !is_null_oid(&ud->subsha1) && !oideq(&ud->sha1, &ud->subsha1)) ||\n>> +\t is_null_oid(&ud->subsha1) || ud->force) {\n>> +\t\tint subforce = is_null_oid(&ud->subsha1) || ud->force;\n>> +\n>> +\t\tif (!ud->nofetch) {\n>> +\t\t\t/*\n>> +\t\t\t * Run fetch only if `sha1` isn't present or it\n>> +\t\t\t * is not reachable from a ref.\n>> +\t\t\t */\n>> +\t\t\tif (!is_tip_reachable(ud->sm_path, &ud->sha1))\n>> +\t\t\t\tif (fetch_in_submodule(ud->sm_path, ud->depth, ud->quiet, NULL) &&\n>> +\t\t\t\t !ud->quiet)\n>> +\t\t\t\t\tfprintf_ln(stderr,\n>> +\t\t\t\t\t\t _(\"Unable to fetch in submodule path '%s'; \"\n>> +\t\t\t\t\t\t \"trying to directly fetch %s:\"),\n>> +\t\t\t\t\t\t ud->displaypath, oid_to_hex(&ud->sha1));\n>> +\t\t\t/*\n>> +\t\t\t * Now we tried the usual fetch, but `sha1` may\n>> +\t\t\t * not be reachable from any of the refs.\n>> +\t\t\t */\n>> +\t\t\tif (!is_tip_reachable(ud->sm_path, &ud->sha1))\n>> +\t\t\t\tif (fetch_in_submodule(ud->sm_path, ud->depth, ud->quiet, &ud->sha1))\n>> +\t\t\t\t\tdie(_(\"Fetched in submodule path '%s', but it did not \"\n>> +\t\t\t\t\t \"contain %s. Direct fetching of that commit failed.\"),\n>> +\t\t\t\t\t ud->displaypath, oid_to_hex(&ud->sha1));\n>> +\t\t}\n>> +\n>> +\t\treturn run_update_command(ud, subforce);\n>> +\t}\n>> +\n>> +\treturn 3;\n>> +}\n> \n> Since this has excatly one caller I think it's better for readability\n> (less indentation) and flow to just remove that \"return 3\" condition and\n> do the big \"if\" you have at the end, i.e. have this function start with\n> \"int subforce =\" and...\n\nYeah, that would be better. I'll change that.\n\n>> static void update_submodule(struct update_clone_data *ucd)\n>> {\n>> \tfprintf(stdout, \"dummy %s %d\\t%s\\n\",\n>> @@ -2379,6 +2552,79 @@ static int update_clone(int argc, const char **argv, const char *prefix)\n>> \treturn update_submodules(&suc);\n>> }\n>> \n>> +static int run_update_procedure(int argc, const char **argv, const char *prefix)\n>> +{\n>> +\tint force = 0, quiet = 0, nofetch = 0, just_cloned = 0;\n>> +\tchar *prefixed_path, *update = NULL;\n>> +\tchar *sha1 = NULL, *subsha1 = NULL;\n>> +\tstruct update_data update_data = UPDATE_DATA_INIT;\n>> +\n>> +\tstruct option options[] = {\n>> +\t\tOPT__QUIET(&quiet, N_(\"suppress output for update by rebase or merge\")),\n>> +\t\tOPT__FORCE(&force, N_(\"force checkout updates\"), 0),\n>> +\t\tOPT_BOOL('N', \"no-fetch\", &nofetch,\n>> +\t\t\t N_(\"don't fetch new objects from the remote site\")),\n>> +\t\tOPT_BOOL(0, \"just-cloned\", &just_cloned,\n>> +\t\t\t N_(\"overrides update mode in case the repository is a fresh clone\")),\n>> +\t\tOPT_INTEGER(0, \"depth\", &update_data.depth, N_(\"depth for shallow fetch\")),\n>> +\t\tOPT_STRING(0, \"prefix\", &prefix,\n>> +\t\t\t N_(\"path\"),\n>> +\t\t\t N_(\"path into the working tree\")),\n>> +\t\tOPT_STRING(0, \"update\", &update,\n>> +\t\t\t N_(\"string\"),\n>> +\t\t\t N_(\"rebase, merge, checkout or none\")),\n>> +\t\tOPT_STRING(0, \"recursive-prefix\", &update_data.recursive_prefix, N_(\"path\"),\n>> +\t\t\t N_(\"path into the working tree, across nested \"\n>> +\t\t\t \"submodule boundaries\")),\n>> +\t\tOPT_STRING(0, \"sha1\", &sha1, N_(\"string\"),\n>> +\t\t\t N_(\"SHA1 expected by superproject\")),\n>> +\t\tOPT_STRING(0, \"subsha1\", &subsha1, N_(\"string\"),\n>> +\t\t\t N_(\"SHA1 of submodule's HEAD\")),\n>> +\t\tOPT_END()\n>> +\t};\n>> +\n>> +\tconst char *const usage[] = {\n>> +\t\tN_(\"git submodule--helper run-update-procedure [] \"),\n>> +\t\tNULL\n>> +\t};\n>> +\n>> +\targc = parse_options(argc, argv, prefix, options, usage, 0);\n>> +\n>> +\tif (argc != 1)\n>> +\t\tusage_with_options(usage, options);\n>> +\tupdate_data.force = !!force;\n>> +\tupdate_data.quiet = !!quiet;\n>> +\tupdate_data.nofetch = !!nofetch;\n>> +\tupdate_data.just_cloned = !!just_cloned;\n> \n> For all of these just pass the reference to the update_data variable\n> directly in the OPT_*(). No need to set an \"int force\", only to copy it\n> over to update_data.force. Let's just use the latter only.\n\nHmm, I'm trying to remember why the single bit values are treated this way\nin this whole file...\n\n...there seems to be no good reason for it. The API docs for parse options\nstate that OPT_BOOL() is guaranteed to return either zero or one, so that\ndouble negation does look unnecessary.\n\n>> +\n>> +\tif (sha1)\n>> +\t\tget_oid_hex(sha1, &update_data.sha1);\n>> +\telse\n>> +\t\toidcpy(&update_data.sha1, null_oid());\n> \n> Nit: Even if a historical option forces us to support --sha1, let's use\n> \"oid\" for the variable etc. But in this case the --sha1 is new, no?\n> Let's use --object-id or --oid (whatever is more common, I didn't\n> check)>\n\nOkay. I can see the confusion this may cause.\n\n>> +\n>> +\tif (subsha1)\n>> +\t\tget_oid_hex(subsha1, &update_data.subsha1);\n>> +\telse\n>> +\t\toidcpy(&update_data.subsha1, null_oid());\n> \n> Ditto. Also I think for both of these you can re-use\n> parse_opt_object_id. See \"squash-onto\" and \"upstream\" in\n> builtin/rebase.c.\n> \n> Then you just supply an oid variable directly and let that helper do all\n> the get_oid etc.\n\nThanks for pointing me to this!\n\n>> +\tif (update_data.recursive_prefix)\n>> +\t\tprefixed_path = xstrfmt(\"%s%s\", update_data.recursive_prefix, update_data.sm_path);\n>> +\telse\n>> +\t\tprefixed_path = xstrdup(update_data.sm_path);\n>> +\n>> +\tupdate_data.displaypath = get_submodule_displaypath(prefixed_path, prefix);\n>> +\n>> +\tdetermine_submodule_update_strategy(the_repository, update_data.just_cloned,\n>> +\t\t\t\t\t update_data.sm_path, update,\n>> +\t\t\t\t\t &update_data.update_strategy);\n>> +\n>> +\tfree(prefixed_path);\n>> +\n>> +\treturn do_run_update_procedure(&update_data);\n> \n> ....(continued from above) ...here just do:\n> \n> if (that big if condition)\n> return do_run_update_procedure(&update_data);\n> else\n> return 3;\n\nOkay.\n\nThanks for the review!\n\n"},{"id":"431683","messageId":"20210802130627.36170-1-raykar.ath@gmail.com","threadId":"56147","inReplyTo":"20210722134012.99457-1-raykar.ath@gmail.com","subject":"[GSoC] [PATCH v2] submodule--helper: run update procedures from C","fromName":"Atharva Raykar","fromEmail":"raykar.ath@gmail.com","sentAt":"2021-08-02T13:06:27Z","receivedAt":"2021-08-02T13:06:46Z","isPatch":true,"sender":{"key":"raykar.ath@gmail.com","avatar":"https://avatars.githubusercontent.com/u/24277692?v=4"},"body":"Add a new submodule--helper subcommand `run-update-procedure` that runs\nthe update procedure if the SHA1 of the submodule does not match what\nthe superproject expects.\n\nThis is an intermediate change that works towards total conversion of\n`submodule update` from shell to C.\n\nSpecific error codes are returned so that the shell script calling the\nsubcommand can take a decision on the control flow, and preserve the\nerror messages across subsequent recursive calls of `cmd_update`.\n\nThis patch could have been approached differently, by first changing the\n`is_tip_reachable` and `fetch_in_submodule` shell functions to be\n`submodule--helper` subcommands, and then following up with a patch that\nintroduces the `run-update-procedure` subcommand. We have not done it\nlike that because those functions are trivial enough to convert directly\nalong with these other changes. This lets us avoid the boilerplate and\nthe cleanup patches that will need to be introduced in following that\napproach.\n\nThis change is more focused on doing a faithful conversion, so for now we\nare not too concerned with trying to reduce subprocess spawns.\n\nSigned-off-by: Atharva Raykar \nMentored-by: Christian Couder \nMentored-by: Shourya Shukla \n---\n\nNotable changes since v1:\n\n* Modified the code structure in\n submodule--helper.c:run_update_command(), while fixing problems with\n the translation marks.\n\n* Renamed '--sha1' and '--subsha1' options to '--oid' and '--suboid' to\n since the argument is parsed into an object_id struct, not plain sha1\n data.\n\n* Used option callbacks to parse the SHA1 arguments directly.\n\n* Moved the conditional out of 'do_run_update_procedure()'.\n\nFeedback required:\n\nÆvar felt that it would be clearer to populate the 'fatal' messages\nafter the run_command() operation in 'run_update_command()', to make it\nmore readable [1]. I have attempted something like that here, and it has led\nto a lot more duplicated 'switch' statements, which feels suboptimal.\nI'd appreciate suggestions to make it more legible.\n\n[1] https://lore.kernel.org/git/87r1fps63r.fsf@evledraar.gmail.com/\n\nFetch-it-Via:\ngit fetch https://github.com/tfidfwastaken/git submodule-run-update-proc-list-2\n\n builtin/submodule--helper.c | 253 ++++++++++++++++++++++++++++++++++++\n git-submodule.sh | 106 +++++----------\n 2 files changed, 286 insertions(+), 73 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex d55f6262e9..b9c40324d0 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2029,6 +2029,20 @@ struct submodule_update_clone {\n \t.max_jobs = 1, \\\n }\n \n+struct update_data {\n+\tconst char *recursive_prefix;\n+\tconst char *sm_path;\n+\tconst char *displaypath;\n+\tstruct object_id oid;\n+\tstruct object_id suboid;\n+\tstruct submodule_update_strategy update_strategy;\n+\tint depth;\n+\tunsigned int force: 1;\n+\tunsigned int quiet: 1;\n+\tunsigned int nofetch: 1;\n+\tunsigned int just_cloned: 1;\n+};\n+#define UPDATE_DATA_INIT { .update_strategy = SUBMODULE_UPDATE_STRATEGY_INIT }\n \n static void next_submodule_warn_missing(struct submodule_update_clone *suc,\n \t\tstruct strbuf *out, const char *displaypath)\n@@ -2282,6 +2296,175 @@ static int git_update_clone_config(const char *var, const char *value,\n \treturn 0;\n }\n \n+static int is_tip_reachable(const char *path, struct object_id *oid)\n+{\n+\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\tstruct strbuf rev = STRBUF_INIT;\n+\tchar *hex = oid_to_hex(oid);\n+\n+\tcp.git_cmd = 1;\n+\tcp.dir = xstrdup(path);\n+\tcp.no_stderr = 1;\n+\tstrvec_pushl(&cp.args, \"rev-list\", \"-n\", \"1\", hex, \"--not\", \"--all\", NULL);\n+\n+\tprepare_submodule_repo_env(&cp.env_array);\n+\n+\tif (capture_command(&cp, &rev, GIT_MAX_HEXSZ + 1) || rev.len)\n+\t\treturn 0;\n+\n+\treturn 1;\n+}\n+\n+static int fetch_in_submodule(const char *module_path, int depth, int quiet, struct object_id *oid)\n+{\n+\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\n+\tprepare_submodule_repo_env(&cp.env_array);\n+\tcp.git_cmd = 1;\n+\tcp.dir = xstrdup(module_path);\n+\n+\tstrvec_push(&cp.args, \"fetch\");\n+\tif (quiet)\n+\t\tstrvec_push(&cp.args, \"--quiet\");\n+\tif (depth)\n+\t\tstrvec_pushf(&cp.args, \"--depth=%d\", depth);\n+\tif (oid) {\n+\t\tchar *hex = oid_to_hex(oid);\n+\t\tchar *remote = get_default_remote();\n+\t\tstrvec_pushl(&cp.args, remote, hex, NULL);\n+\t}\n+\n+\treturn run_command(&cp);\n+}\n+\n+static int run_update_command(struct update_data *ud, int subforce)\n+{\n+\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\tchar *oid = oid_to_hex(&ud->oid);\n+\tint must_die_on_failure = 0;\n+\n+\tcp.dir = xstrdup(ud->sm_path);\n+\tswitch (ud->update_strategy.type) {\n+\tcase SM_UPDATE_CHECKOUT:\n+\t\tcp.git_cmd = 1;\n+\t\tstrvec_pushl(&cp.args, \"checkout\", \"-q\", NULL);\n+\t\tif (subforce)\n+\t\t\tstrvec_push(&cp.args, \"-f\");\n+\t\tbreak;\n+\tcase SM_UPDATE_REBASE:\n+\t\tcp.git_cmd = 1;\n+\t\tstrvec_push(&cp.args, \"rebase\");\n+\t\tif (ud->quiet)\n+\t\t\tstrvec_push(&cp.args, \"--quiet\");\n+\t\tmust_die_on_failure = 1;\n+\t\tbreak;\n+\tcase SM_UPDATE_MERGE:\n+\t\tcp.git_cmd = 1;\n+\t\tstrvec_push(&cp.args, \"merge\");\n+\t\tif (ud->quiet)\n+\t\t\tstrvec_push(&cp.args, \"--quiet\");\n+\t\tmust_die_on_failure = 1;\n+\t\tbreak;\n+\tcase SM_UPDATE_COMMAND:\n+\t\t/* NOTE: this does not handle quoted arguments */\n+\t\tstrvec_split(&cp.args, ud->update_strategy.command);\n+\t\tmust_die_on_failure = 1;\n+\t\tbreak;\n+\tcase SM_UPDATE_UNSPECIFIED:\n+\tcase SM_UPDATE_NONE:\n+\t\tBUG(\"update strategy should have been specified\");\n+\t}\n+\n+\tstrvec_push(&cp.args, oid);\n+\n+\tprepare_submodule_repo_env(&cp.env_array);\n+\n+\tif (run_command(&cp)) {\n+\t\tif (must_die_on_failure) {\n+\t\t\tswitch (ud->update_strategy.type) {\n+\t\t\tcase SM_UPDATE_CHECKOUT:\n+\t\t\t\tdie(_(\"Unable to checkout '%s' in submodule path '%s'\"),\n+\t\t\t\t oid, ud->displaypath);\n+\t\t\t\tbreak;\n+\t\t\tcase SM_UPDATE_REBASE:\n+\t\t\t\tdie(_(\"Unable to rebase '%s' in submodule path '%s'\"),\n+\t\t\t\t oid, ud->displaypath);\n+\t\t\t\tbreak;\n+\t\t\tcase SM_UPDATE_MERGE:\n+\t\t\t\tdie(_(\"Unable to merge '%s' in submodule path '%s'\"),\n+\t\t\t\t oid, ud->displaypath);\n+\t\t\t\tbreak;\n+\t\t\tcase SM_UPDATE_COMMAND:\n+\t\t\t\tdie(_(\"Execution of '%s %s' failed in submodule path '%s'\"),\n+\t\t\t\t ud->update_strategy.command, oid, ud->displaypath);\n+\t\t\t\tbreak;\n+\t\t\tcase SM_UPDATE_UNSPECIFIED:\n+\t\t\tcase SM_UPDATE_NONE:\n+\t\t\t\tBUG(\"update strategy should have been specified\");\n+\t\t\t}\n+\t\t}\n+\t\t/*\n+\t\t * This signifies to the caller in shell that\n+\t\t * the command failed without dying\n+\t\t */\n+\t\treturn 1;\n+\t}\n+\n+\tswitch (ud->update_strategy.type) {\n+\tcase SM_UPDATE_CHECKOUT:\n+\t\tprintf(_(\"Submodule path '%s': checked out '%s'\\n\"),\n+\t\t ud->displaypath, oid);\n+\t\tbreak;\n+\tcase SM_UPDATE_REBASE:\n+\t\tprintf(_(\"Submodule path '%s': rebased into '%s'\\n\"),\n+\t\t ud->displaypath, oid);\n+\t\tbreak;\n+\tcase SM_UPDATE_MERGE:\n+\t\tprintf(_(\"Submodule path '%s': merged in '%s'\\n\"),\n+\t\t ud->displaypath, oid);\n+\t\tbreak;\n+\tcase SM_UPDATE_COMMAND:\n+\t\tprintf(_(\"Submodule path '%s': '%s %s'\\n\"),\n+\t\t ud->displaypath, ud->update_strategy.command, oid);\n+\t\tbreak;\n+\tcase SM_UPDATE_UNSPECIFIED:\n+\tcase SM_UPDATE_NONE:\n+\t\tBUG(\"update strategy should have been specified\");\n+\t}\n+\n+\treturn 0;\n+}\n+\n+static int do_run_update_procedure(struct update_data *ud)\n+{\n+\tint subforce = is_null_oid(&ud->suboid) || ud->force;\n+\n+\tif (!ud->nofetch) {\n+\t\t/*\n+\t\t * Run fetch only if `oid` isn't present or it\n+\t\t * is not reachable from a ref.\n+\t\t */\n+\t\tif (!is_tip_reachable(ud->sm_path, &ud->oid))\n+\t\t\tif (fetch_in_submodule(ud->sm_path, ud->depth, ud->quiet, NULL) &&\n+\t\t\t !ud->quiet)\n+\t\t\t\tfprintf_ln(stderr,\n+\t\t\t\t\t _(\"Unable to fetch in submodule path '%s'; \"\n+\t\t\t\t\t \"trying to directly fetch %s:\"),\n+\t\t\t\t\t ud->displaypath, oid_to_hex(&ud->oid));\n+\t\t/*\n+\t\t * Now we tried the usual fetch, but `oid` may\n+\t\t * not be reachable from any of the refs.\n+\t\t */\n+\t\tif (!is_tip_reachable(ud->sm_path, &ud->oid))\n+\t\t\tif (fetch_in_submodule(ud->sm_path, ud->depth, ud->quiet, &ud->oid))\n+\t\t\t\tdie(_(\"Fetched in submodule path '%s', but it did not \"\n+\t\t\t\t \"contain %s. Direct fetching of that commit failed.\"),\n+\t\t\t\t ud->displaypath, oid_to_hex(&ud->oid));\n+\t}\n+\n+\treturn run_update_command(ud, subforce);\n+}\n+\n static void update_submodule(struct update_clone_data *ucd)\n {\n \tfprintf(stdout, \"dummy %s %d\\t%s\\n\",\n@@ -2379,6 +2562,75 @@ static int update_clone(int argc, const char **argv, const char *prefix)\n \treturn update_submodules(&suc);\n }\n \n+static int run_update_procedure(int argc, const char **argv, const char *prefix)\n+{\n+\tint force = 0, quiet = 0, nofetch = 0, just_cloned = 0;\n+\tchar *prefixed_path, *update = NULL;\n+\tstruct update_data update_data = UPDATE_DATA_INIT;\n+\n+\tstruct option options[] = {\n+\t\tOPT__QUIET(&quiet, N_(\"suppress output for update by rebase or merge\")),\n+\t\tOPT__FORCE(&force, N_(\"force checkout updates\"), 0),\n+\t\tOPT_BOOL('N', \"no-fetch\", &nofetch,\n+\t\t\t N_(\"don't fetch new objects from the remote site\")),\n+\t\tOPT_BOOL(0, \"just-cloned\", &just_cloned,\n+\t\t\t N_(\"overrides update mode in case the repository is a fresh clone\")),\n+\t\tOPT_INTEGER(0, \"depth\", &update_data.depth, N_(\"depth for shallow fetch\")),\n+\t\tOPT_STRING(0, \"prefix\", &prefix,\n+\t\t\t N_(\"path\"),\n+\t\t\t N_(\"path into the working tree\")),\n+\t\tOPT_STRING(0, \"update\", &update,\n+\t\t\t N_(\"string\"),\n+\t\t\t N_(\"rebase, merge, checkout or none\")),\n+\t\tOPT_STRING(0, \"recursive-prefix\", &update_data.recursive_prefix, N_(\"path\"),\n+\t\t\t N_(\"path into the working tree, across nested \"\n+\t\t\t \"submodule boundaries\")),\n+\t\tOPT_CALLBACK_F(0, \"oid\", &update_data.oid, N_(\"sha1\"),\n+\t\t\t N_(\"SHA1 expected by superproject\"), PARSE_OPT_NONEG,\n+\t\t\t parse_opt_object_id),\n+\t\tOPT_CALLBACK_F(0, \"suboid\", &update_data.suboid, N_(\"subsha1\"),\n+\t\t\t N_(\"SHA1 of submodule's HEAD\"), PARSE_OPT_NONEG,\n+\t\t\t parse_opt_object_id),\n+\t\tOPT_END()\n+\t};\n+\n+\tconst char *const usage[] = {\n+\t\tN_(\"git submodule--helper run-update-procedure [] \"),\n+\t\tNULL\n+\t};\n+\n+\targc = parse_options(argc, argv, prefix, options, usage, 0);\n+\n+\tif (argc != 1)\n+\t\tusage_with_options(usage, options);\n+\n+\tupdate_data.force = !!force;\n+\tupdate_data.quiet = !!quiet;\n+\tupdate_data.nofetch = !!nofetch;\n+\tupdate_data.just_cloned = !!just_cloned;\n+\tupdate_data.sm_path = argv[0];\n+\n+\tif (update_data.recursive_prefix)\n+\t\tprefixed_path = xstrfmt(\"%s%s\", update_data.recursive_prefix, update_data.sm_path);\n+\telse\n+\t\tprefixed_path = xstrdup(update_data.sm_path);\n+\n+\tupdate_data.displaypath = get_submodule_displaypath(prefixed_path, prefix);\n+\n+\tdetermine_submodule_update_strategy(the_repository, update_data.just_cloned,\n+\t\t\t\t\t update_data.sm_path, update,\n+\t\t\t\t\t &update_data.update_strategy);\n+\n+\tfree(prefixed_path);\n+\n+\tif ((!is_null_oid(&update_data.oid) && !is_null_oid(&update_data.suboid) &&\n+\t !oideq(&update_data.oid, &update_data.suboid)) ||\n+\t is_null_oid(&update_data.suboid) || update_data.force)\n+\t\treturn do_run_update_procedure(&update_data);\n+\n+\treturn 3;\n+}\n+\n static int resolve_relative_path(int argc, const char **argv, const char *prefix)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n@@ -2759,6 +3011,7 @@ static struct cmd_struct commands[] = {\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{\"run-update-procedure\", run_update_procedure, 0},\n \t{\"ensure-core-worktree\", ensure_core_worktree, 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 1187c21260..0bb4514859 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -405,13 +405,6 @@ cmd_deinit()\n \tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper deinit ${GIT_QUIET:+--quiet} ${prefix:+--prefix \"$prefix\"} ${force:+--force} ${deinit_all:+--all} -- \"$@\"\n }\n \n-is_tip_reachable () (\n-\tsanitize_submodule_env &&\n-\tcd \"$1\" &&\n-\trev=$(git rev-list -n 1 \"$2\" --not --all 2>/dev/null) &&\n-\ttest -z \"$rev\"\n-)\n-\n # usage: fetch_in_submodule [] []\n # Because arguments are positional, use an empty string to omit \n # but include .\n@@ -555,14 +548,13 @@ cmd_update()\n \n \t\tgit submodule--helper ensure-core-worktree \"$sm_path\" || exit 1\n \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\telse\n+\t\t\tjust_cloned=\n \t\t\tsubsha1=$(sanitize_submodule_env; cd \"$sm_path\" &&\n \t\t\t\tgit rev-parse --verify HEAD) ||\n \t\t\tdie \"$(eval_gettext \"fatal: Unable to find current revision in submodule path '\\$displaypath'\")\"\n@@ -583,70 +575,38 @@ cmd_update()\n \t\t\tdie \"$(eval_gettext \"fatal: Unable to find current \\${remote_name}/\\${branch} revision in submodule path '\\$sm_path'\")\"\n \t\tfi\n \n-\t\tif test \"$subsha1\" != \"$sha1\" || test -n \"$force\"\n-\t\tthen\n-\t\t\tsubforce=$force\n-\t\t\t# If we don't already have a -f flag and the submodule has never been checked out\n-\t\t\tif test -z \"$subsha1\" && test -z \"$force\"\n-\t\t\tthen\n-\t\t\t\tsubforce=\"-f\"\n-\t\t\tfi\n+\t\tout=$(git submodule--helper run-update-procedure \\\n+\t\t\t ${wt_prefix:+--prefix \"$wt_prefix\"} \\\n+\t\t\t ${GIT_QUIET:+--quiet} \\\n+\t\t\t ${force:+--force} \\\n+\t\t\t ${just_cloned:+--just-cloned} \\\n+\t\t\t ${nofetch:+--no-fetch} \\\n+\t\t\t ${depth:+\"$depth\"} \\\n+\t\t\t ${update:+--update \"$update\"} \\\n+\t\t\t ${prefix:+--recursive-prefix \"$prefix\"} \\\n+\t\t\t ${sha1:+--oid \"$sha1\"} \\\n+\t\t\t ${subsha1:+--suboid \"$subsha1\"} \\\n+\t\t\t \"--\" \\\n+\t\t\t \"$sm_path\")\n \n-\t\t\tif test -z \"$nofetch\"\n-\t\t\tthen\n-\t\t\t\t# Run fetch only if $sha1 isn't present or it\n-\t\t\t\t# is not reachable from a ref.\n-\t\t\t\tis_tip_reachable \"$sm_path\" \"$sha1\" ||\n-\t\t\t\tfetch_in_submodule \"$sm_path\" $depth ||\n-\t\t\t\tsay \"$(eval_gettext \"Unable to fetch in submodule path '\\$displaypath'; trying to directly fetch \\$sha1:\")\"\n-\n-\t\t\t\t# Now we tried the usual fetch, but $sha1 may\n-\t\t\t\t# not be reachable from any of the refs\n-\t\t\t\tis_tip_reachable \"$sm_path\" \"$sha1\" ||\n-\t\t\t\tfetch_in_submodule \"$sm_path\" \"$depth\" \"$sha1\" ||\n-\t\t\t\tdie \"$(eval_gettext \"Fetched in submodule path '\\$displaypath', but it did not contain \\$sha1. Direct fetching of that commit failed.\")\"\n-\t\t\tfi\n-\n-\t\t\tmust_die_on_failure=\n-\t\t\tcase \"$update_module\" in\n-\t\t\tcheckout)\n-\t\t\t\tcommand=\"git checkout $subforce -q\"\n-\t\t\t\tdie_msg=\"$(eval_gettext \"Unable to checkout '\\$sha1' in submodule path '\\$displaypath'\")\"\n-\t\t\t\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': checked out '\\$sha1'\")\"\n-\t\t\t\t;;\n-\t\t\trebase)\n-\t\t\t\tcommand=\"git rebase ${GIT_QUIET:+--quiet}\"\n-\t\t\t\tdie_msg=\"$(eval_gettext \"Unable to rebase '\\$sha1' in submodule path '\\$displaypath'\")\"\n-\t\t\t\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': rebased into '\\$sha1'\")\"\n-\t\t\t\tmust_die_on_failure=yes\n-\t\t\t\t;;\n-\t\t\tmerge)\n-\t\t\t\tcommand=\"git merge ${GIT_QUIET:+--quiet}\"\n-\t\t\t\tdie_msg=\"$(eval_gettext \"Unable to merge '\\$sha1' in submodule path '\\$displaypath'\")\"\n-\t\t\t\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': merged in '\\$sha1'\")\"\n-\t\t\t\tmust_die_on_failure=yes\n-\t\t\t\t;;\n-\t\t\t!*)\n-\t\t\t\tcommand=\"${update_module#!}\"\n-\t\t\t\tdie_msg=\"$(eval_gettext \"Execution of '\\$command \\$sha1' failed in submodule path '\\$displaypath'\")\"\n-\t\t\t\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': '\\$command \\$sha1'\")\"\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 path '$path'\")\"\n-\t\t\tesac\n-\n-\t\t\tif (sanitize_submodule_env; cd \"$sm_path\" && $command \"$sha1\")\n-\t\t\tthen\n-\t\t\t\tsay \"$say_msg\"\n-\t\t\telif test -n \"$must_die_on_failure\"\n-\t\t\tthen\n-\t\t\t\tdie_with_status 2 \"$die_msg\"\n-\t\t\telse\n-\t\t\t\terr=\"${err};$die_msg\"\n-\t\t\t\tcontinue\n-\t\t\tfi\n-\t\tfi\n+\t\t# exit codes for run-update-procedure:\n+\t\t# 0: update was successful, say command output\n+\t\t# 128: subcommand died during execution\n+\t\t# 1: update procedure failed, but should not die\n+\t\t# 3: no update procedure was run\n+\t\tres=\"$?\"\n+\t\tcase $res in\n+\t\t0)\n+\t\t\tsay \"$out\"\n+\t\t\t;;\n+\t\t128)\n+\t\t\texit $res\n+\t\t\t;;\n+\t\t1)\n+\t\t\terr=\"${err};$out\"\n+\t\t\tcontinue\n+\t\t\t;;\n+\t\tesac\n \n \t\tif test -n \"$recursive\"\n \t\tthen\n@@ -661,7 +621,7 @@ cmd_update()\n \t\t\tif test $res -gt 0\n \t\t\tthen\n \t\t\t\tdie_msg=\"$(eval_gettext \"fatal: Failed to recurse into submodule path '\\$displaypath'\")\"\n-\t\t\t\tif test $res -ne 2\n+\t\t\t\tif test $res -ne 2 && test $res -ne 128\n \t\t\t\tthen\n \t\t\t\t\terr=\"${err};$die_msg\"\n \t\t\t\t\tcontinue\n-- \n2.32.0\n\n"},{"id":"431743","messageId":"CACdWUYXhckBkHLPnRDxxb=raAD0=7236jAzvneBLhw8fXvGTMw@mail.gmail.com","threadId":"56147","inReplyTo":"20210802130627.36170-1-raykar.ath@gmail.com","subject":"Re: [GSoC] [PATCH v2] submodule--helper: run update procedures from C","fromName":"Shourya Shukla","fromEmail":"periperidip@gmail.com","sentAt":"2021-08-02T18:50:39Z","receivedAt":"2021-08-02T18:50:55Z","isPatch":true,"sender":{"key":"periperidip@gmail.com","avatar":null},"body":"Le lun. 2 août 2021 à 18:36, Atharva Raykar a écrit :\n>\n> Add a new submodule--helper subcommand `run-update-procedure` that runs\n> the update procedure if the SHA1 of the submodule does not match what\n> the superproject expects.\n>\n> This is an intermediate change that works towards total conversion of\n> `submodule update` from shell to C.\n>\n> Specific error codes are returned so that the shell script calling the\n> subcommand can take a decision on the control flow, and preserve the\n> error messages across subsequent recursive calls of `cmd_update`.\n>\n> This patch could have been approached differently, by first changing the\n> `is_tip_reachable` and `fetch_in_submodule` shell functions to be\n> `submodule--helper` subcommands, and then following up with a patch that\n> introduces the `run-update-procedure` subcommand. We have not done it\n> like that because those functions are trivial enough to convert directly\n> along with these other changes. This lets us avoid the boilerplate and\n> the cleanup patches that will need to be introduced in following that\n> approach.\n\nI feel that this part is more suitable for a cover letter rather than the commit\nmessage itself. It is a useful piece of info though.\n\n> This change is more focused on doing a faithful conversion, so for now we\n> are not too concerned with trying to reduce subprocess spawns.\n>\n> Signed-off-by: Atharva Raykar \n> Mentored-by: Christian Couder \n> Mentored-by: Shourya Shukla \n> ---\n>\n> Notable changes since v1:\n>\n> * Modified the code structure in\n> submodule--helper.c:run_update_command(), while fixing problems with\n> the translation marks.\n>\n> * Renamed '--sha1' and '--subsha1' options to '--oid' and '--suboid' to\n> since the argument is parsed into an object_id struct, not plain sha1\n> data.\n>\n> * Used option callbacks to parse the SHA1 arguments directly.\n>\n> * Moved the conditional out of 'do_run_update_procedure()'.\n>\n> Feedback required:\n>\n> Ævar felt that it would be clearer to populate the 'fatal' messages\n> after the run_command() operation in 'run_update_command()', to make it\n> more readable [1]. I have attempted something like that here, and it has led\n> to a lot more duplicated 'switch' statements, which feels suboptimal.\n> I'd appreciate suggestions to make it more legible.\n>\n> [1] https://lore.kernel.org/git/87r1fps63r.fsf@evledraar.gmail.com/\n>\n> Fetch-it-Via:\n> git fetch https://github.com/tfidfwastaken/git submodule-run-update-proc-list-2\n>\n> builtin/submodule--helper.c | 253 ++++++++++++++++++++++++++++++++++++\n> git-submodule.sh | 106 +++++----------\n> 2 files changed, 286 insertions(+), 73 deletions(-)\n>\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index d55f6262e9..b9c40324d0 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -2029,6 +2029,20 @@ struct submodule_update_clone {\n> .max_jobs = 1, \\\n> }\n>\n> +struct update_data {\n> + const char *recursive_prefix;\n> + const char *sm_path;\n> + const char *displaypath;\n> + struct object_id oid;\n> + struct object_id suboid;\n> + struct submodule_update_strategy update_strategy;\n> + int depth;\n> + unsigned int force: 1;\n> + unsigned int quiet: 1;\n> + unsigned int nofetch: 1;\n> + unsigned int just_cloned: 1;\n> +};\n> +#define UPDATE_DATA_INIT { .update_strategy = SUBMODULE_UPDATE_STRATEGY_INIT }\n>\n> static void next_submodule_warn_missing(struct submodule_update_clone *suc,\n> struct strbuf *out, const char *displaypath)\n> @@ -2282,6 +2296,175 @@ static int git_update_clone_config(const char *var, const char *value,\n> return 0;\n> }\n> +\n> +static int run_update_command(struct update_data *ud, int subforce)\n> +{\n> + struct child_process cp = CHILD_PROCESS_INIT;\n> + char *oid = oid_to_hex(&ud->oid);\n> + int must_die_on_failure = 0;\n> +\n> + cp.dir = xstrdup(ud->sm_path);\n> + switch (ud->update_strategy.type) {\n> + case SM_UPDATE_CHECKOUT:\n> + cp.git_cmd = 1;\n> + strvec_pushl(&cp.args, \"checkout\", \"-q\", NULL);\n\nWould it be possible to add the 'if' statement above just before the\n'switch' (or after,\nwhichever seems okay) since this is common amongst (almost) all the cases?\n\n> + if (subforce)\n> + strvec_push(&cp.args, \"-f\");\n> + break;\n> + case SM_UPDATE_REBASE:\n> + cp.git_cmd = 1;\n> + strvec_push(&cp.args, \"rebase\");\n> + if (ud->quiet)\n> + strvec_push(&cp.args, \"--quiet\");\n> + must_die_on_failure = 1;\n> + break;\n> + case SM_UPDATE_MERGE:\n> + cp.git_cmd = 1;\n> + strvec_push(&cp.args, \"merge\");\n> + if (ud->quiet)\n> + strvec_push(&cp.args, \"--quiet\");\n> + must_die_on_failure = 1;\n> + break;\n> + case SM_UPDATE_COMMAND:\n> + /* NOTE: this does not handle quoted arguments */\n> + strvec_split(&cp.args, ud->update_strategy.command);\n> + must_die_on_failure = 1;\n> + break;\n> + case SM_UPDATE_UNSPECIFIED:\n> + case SM_UPDATE_NONE:\n> + BUG(\"update strategy should have been specified\");\n> + }\n\nIf the original did not bug out, do we need to? The documentation does\nnot mention\nthis as well:\nhttps://git-scm.com/docs/git-submodule#Documentation/git-submodule.txt-none\n\n> +\n> + strvec_push(&cp.args, oid);\n> +\n> + prepare_submodule_repo_env(&cp.env_array);\n> +\n> + if (run_command(&cp)) {\n> + if (must_die_on_failure) {\n> + switch (ud->update_strategy.type) {\n> + case SM_UPDATE_CHECKOUT:\n> + die(_(\"Unable to checkout '%s' in submodule path '%s'\"),\n> + oid, ud->displaypath);\n> + break;\n> + case SM_UPDATE_REBASE:\n> + die(_(\"Unable to rebase '%s' in submodule path '%s'\"),\n> + oid, ud->displaypath);\n> + break;\n> + case SM_UPDATE_MERGE:\n> + die(_(\"Unable to merge '%s' in submodule path '%s'\"),\n> + oid, ud->displaypath);\n> + break;\n> + case SM_UPDATE_COMMAND:\n> + die(_(\"Execution of '%s %s' failed in submodule path '%s'\"),\n> + ud->update_strategy.command, oid, ud->displaypath);\n> + break;\n> + case SM_UPDATE_UNSPECIFIED:\n> + case SM_UPDATE_NONE:\n> + BUG(\"update strategy should have been specified\");\n> + }\n> + }\n> + /*\n> + * This signifies to the caller in shell that\n> + * the command failed without dying\n> + */\n> + return 1;\n> + }\n> +\n> + switch (ud->update_strategy.type) {\n> + case SM_UPDATE_CHECKOUT:\n> + printf(_(\"Submodule path '%s': checked out '%s'\\n\"),\n> + ud->displaypath, oid);\n> + break;\n> + case SM_UPDATE_REBASE:\n> + printf(_(\"Submodule path '%s': rebased into '%s'\\n\"),\n> + ud->displaypath, oid);\n> + break;\n> + case SM_UPDATE_MERGE:\n> + printf(_(\"Submodule path '%s': merged in '%s'\\n\"),\n> + ud->displaypath, oid);\n> + break;\n> + case SM_UPDATE_COMMAND:\n> + printf(_(\"Submodule path '%s': '%s %s'\\n\"),\n> + ud->displaypath, ud->update_strategy.command, oid);\n> + break;\n> + case SM_UPDATE_UNSPECIFIED:\n> + case SM_UPDATE_NONE:\n> + BUG(\"update strategy should have been specified\");\n> + }\n> +\n> + return 0;\n> +}\n> +\n> +static int do_run_update_procedure(struct update_data *ud)\n> +{\n> + int subforce = is_null_oid(&ud->suboid) || ud->force;\n> +\n> + if (!ud->nofetch) {\n> + /*\n> + * Run fetch only if `oid` isn't present or it\n> + * is not reachable from a ref.\n> + */\n> + if (!is_tip_reachable(ud->sm_path, &ud->oid))\n> + if (fetch_in_submodule(ud->sm_path, ud->depth, ud->quiet, NULL) &&\n> + !ud->quiet)\n> + fprintf_ln(stderr,\n> + _(\"Unable to fetch in submodule path '%s'; \"\n> + \"trying to directly fetch %s:\"),\n> + ud->displaypath, oid_to_hex(&ud->oid));\n\nI was wondering if an OID is invalid, will it be counted as\nunreachable and vice-versa?\nIf that is the case then that would simplify the work.\n"},{"id":"431803","messageId":"m2czquc3v0.fsf@gmail.com","threadId":"56147","inReplyTo":"CACdWUYXhckBkHLPnRDxxb=raAD0=7236jAzvneBLhw8fXvGTMw@mail.gmail.com","subject":"Re: [GSoC] [PATCH v2] submodule--helper: run update procedures from C","fromName":"Atharva Raykar","fromEmail":"raykar.ath@gmail.com","sentAt":"2021-08-03T08:46:43Z","receivedAt":"2021-08-03T08:46:52Z","isPatch":true,"sender":{"key":"raykar.ath@gmail.com","avatar":"https://avatars.githubusercontent.com/u/24277692?v=4"},"body":"\nShourya Shukla writes:\n\n> Le lun. 2 août 2021 à 18:36, Atharva Raykar \n> a écrit :\n>>\n>> Add a new submodule--helper subcommand `run-update-procedure` \n>> that runs\n>> the update procedure if the SHA1 of the submodule does not \n>> match what\n>> the superproject expects.\n>>\n>> This is an intermediate change that works towards total \n>> conversion of\n>> `submodule update` from shell to C.\n>>\n>> Specific error codes are returned so that the shell script \n>> calling the\n>> subcommand can take a decision on the control flow, and \n>> preserve the\n>> error messages across subsequent recursive calls of \n>> `cmd_update`.\n>>\n>> This patch could have been approached differently, by first \n>> changing the\n>> `is_tip_reachable` and `fetch_in_submodule` shell functions to \n>> be\n>> `submodule--helper` subcommands, and then following up with a \n>> patch that\n>> introduces the `run-update-procedure` subcommand. We have not \n>> done it\n>> like that because those functions are trivial enough to convert \n>> directly\n>> along with these other changes. This lets us avoid the \n>> boilerplate and\n>> the cleanup patches that will need to be introduced in \n>> following that\n>> approach.\n>\n> I feel that this part is more suitable for a cover letter rather \n> than the commit\n> message itself. It is a useful piece of info though.\n\nOkay, that seems right, the message does seem a bit too\ncontext-sensitive.\n\n>> This change is more focused on doing a faithful conversion, so \n>> for now we\n>> are not too concerned with trying to reduce subprocess spawns.\n>>\n>> Signed-off-by: Atharva Raykar \n>> Mentored-by: Christian Couder \n>> Mentored-by: Shourya Shukla \n>> ---\n>>\n>> Notable changes since v1:\n>>\n>> * Modified the code structure in\n>> submodule--helper.c:run_update_command(), while fixing \n>> problems with\n>> the translation marks.\n>>\n>> * Renamed '--sha1' and '--subsha1' options to '--oid' and \n>> '--suboid' to\n>> since the argument is parsed into an object_id struct, not \n>> plain sha1\n>> data.\n>>\n>> * Used option callbacks to parse the SHA1 arguments directly.\n>>\n>> * Moved the conditional out of 'do_run_update_procedure()'.\n>>\n>> Feedback required:\n>>\n>> Ævar felt that it would be clearer to populate the 'fatal' \n>> messages\n>> after the run_command() operation in 'run_update_command()', to \n>> make it\n>> more readable [1]. I have attempted something like that here, \n>> and it has led\n>> to a lot more duplicated 'switch' statements, which feels \n>> suboptimal.\n>> I'd appreciate suggestions to make it more legible.\n>>\n>> [1] \n>> https://lore.kernel.org/git/87r1fps63r.fsf@evledraar.gmail.com/\n>>\n>> Fetch-it-Via:\n>> git fetch https://github.com/tfidfwastaken/git \n>> submodule-run-update-proc-list-2\n>>\n>> builtin/submodule--helper.c | 253 \n>> ++++++++++++++++++++++++++++++++++++\n>> git-submodule.sh | 106 +++++----------\n>> 2 files changed, 286 insertions(+), 73 deletions(-)\n>>\n>> diff --git a/builtin/submodule--helper.c \n>> b/builtin/submodule--helper.c\n>> index d55f6262e9..b9c40324d0 100644\n>> --- a/builtin/submodule--helper.c\n>> +++ b/builtin/submodule--helper.c\n>> @@ -2029,6 +2029,20 @@ struct submodule_update_clone {\n>> .max_jobs = 1, \\\n>> }\n>>\n>> +struct update_data {\n>> + const char *recursive_prefix;\n>> + const char *sm_path;\n>> + const char *displaypath;\n>> + struct object_id oid;\n>> + struct object_id suboid;\n>> + struct submodule_update_strategy update_strategy;\n>> + int depth;\n>> + unsigned int force: 1;\n>> + unsigned int quiet: 1;\n>> + unsigned int nofetch: 1;\n>> + unsigned int just_cloned: 1;\n>> +};\n>> +#define UPDATE_DATA_INIT { .update_strategy = \n>> SUBMODULE_UPDATE_STRATEGY_INIT }\n>>\n>> static void next_submodule_warn_missing(struct \n>> submodule_update_clone *suc,\n>> struct strbuf *out, const char *displaypath)\n>> @@ -2282,6 +2296,175 @@ static int \n>> git_update_clone_config(const char *var, const char *value,\n>> return 0;\n>> }\n>> +\n>> +static int run_update_command(struct update_data *ud, int \n>> subforce)\n>> +{\n>> + struct child_process cp = CHILD_PROCESS_INIT;\n>> + char *oid = oid_to_hex(&ud->oid);\n>> + int must_die_on_failure = 0;\n>> +\n>> + cp.dir = xstrdup(ud->sm_path);\n>> + switch (ud->update_strategy.type) {\n>> + case SM_UPDATE_CHECKOUT:\n>> + cp.git_cmd = 1;\n>> + strvec_pushl(&cp.args, \"checkout\", \"-q\", NULL);\n>\n> Would it be possible to add the 'if' statement above just before \n> the\n> 'switch' (or after,\n> whichever seems okay) since this is common amongst (almost) all \n> the cases?\n\nI'll try it on once, if it makes the code more readable, I'll \ninclude it\nin the reroll.\n\n>> + if (subforce)\n>> + strvec_push(&cp.args, \"-f\");\n>> + break;\n>> + case SM_UPDATE_REBASE:\n>> + cp.git_cmd = 1;\n>> + strvec_push(&cp.args, \"rebase\");\n>> + if (ud->quiet)\n>> + strvec_push(&cp.args, \"--quiet\");\n>> + must_die_on_failure = 1;\n>> + break;\n>> + case SM_UPDATE_MERGE:\n>> + cp.git_cmd = 1;\n>> + strvec_push(&cp.args, \"merge\");\n>> + if (ud->quiet)\n>> + strvec_push(&cp.args, \"--quiet\");\n>> + must_die_on_failure = 1;\n>> + break;\n>> + case SM_UPDATE_COMMAND:\n>> + /* NOTE: this does not handle quoted arguments \n>> */\n>> + strvec_split(&cp.args, \n>> ud->update_strategy.command);\n>> + must_die_on_failure = 1;\n>> + break;\n>> + case SM_UPDATE_UNSPECIFIED:\n>> + case SM_UPDATE_NONE:\n>> + BUG(\"update strategy should have been \n>> specified\");\n>> + }\n>\n> If the original did not bug out, do we need to? The \n> documentation does\n> not mention\n> this as well:\n> https://git-scm.com/docs/git-submodule#Documentation/git-submodule.txt-none\n\nThis was how the original shell porcelain did it:\ncase \"$update_module\" in\ncheckout)\n\tcommand=\"git checkout $subforce -q\"\n\tdie_msg=\"$(eval_gettext \"Unable to checkout '\\$sha1' in \n\tsubmodule path '\\$displaypath'\")\"\n\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': \n\tchecked out '\\$sha1'\")\"\n\t;;\nrebase)\n\tcommand=\"git rebase ${GIT_QUIET:+--quiet}\"\n\tdie_msg=\"$(eval_gettext \"Unable to rebase '\\$sha1' in \n\tsubmodule path '\\$displaypath'\")\"\n\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': \n\trebased into '\\$sha1'\")\"\n\tmust_die_on_failure=yes\n\t;;\nmerge)\n\tcommand=\"git merge ${GIT_QUIET:+--quiet}\"\n\tdie_msg=\"$(eval_gettext \"Unable to merge '\\$sha1' in submodule \n\tpath '\\$displaypath'\")\"\n\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': \n\tmerged in '\\$sha1'\")\"\n\tmust_die_on_failure=yes\n\t;;\n!*)\n\tcommand=\"${update_module#!}\"\n\tdie_msg=\"$(eval_gettext \"Execution of '\\$command \\$sha1' \n\tfailed in submodule path '\\$displaypath'\")\"\n\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': \n\t'\\$command \\$sha1'\")\"\n\tmust_die_on_failure=yes\n\t;;\n*)\n\tdie \"$(eval_gettext \"Invalid update mode '$update_module' for \n\tsubmodule path '$path'\")\"\nesac\n\nThe fallthrough case used to die, but I noticed that this branch \nwill\nnever get activated. This is because the 'update-clone' helper \nwill not\noutput any entry that has the update mode set to 'none', and thus \nthe\n`while` loop that contains this code would never run.\n\nWhich is why I decided to BUG out on that case, because that state\nshould never be reached. But I see the source of confusion, and \nmaybe I\nshould have different BUG messages for SM_UPDATE_UNSPECIFIED and\nSM_UPDATE_NONE. The latter should probably say \"should have been \nhandled\nby update-clone\".\n\n>> +\n>> + strvec_push(&cp.args, oid);\n>> +\n>> + prepare_submodule_repo_env(&cp.env_array);\n>> +\n>> + if (run_command(&cp)) {\n>> + if (must_die_on_failure) {\n>> + switch (ud->update_strategy.type) {\n>> + case SM_UPDATE_CHECKOUT:\n>> + die(_(\"Unable to checkout '%s' \n>> in submodule path '%s'\"),\n>> + oid, ud->displaypath);\n>> + break;\n>> + case SM_UPDATE_REBASE:\n>> + die(_(\"Unable to rebase '%s' in \n>> submodule path '%s'\"),\n>> + oid, ud->displaypath);\n>> + break;\n>> + case SM_UPDATE_MERGE:\n>> + die(_(\"Unable to merge '%s' in \n>> submodule path '%s'\"),\n>> + oid, ud->displaypath);\n>> + break;\n>> + case SM_UPDATE_COMMAND:\n>> + die(_(\"Execution of '%s %s' \n>> failed in submodule path '%s'\"),\n>> + \n>> ud->update_strategy.command, oid, ud->displaypath);\n>> + break;\n>> + case SM_UPDATE_UNSPECIFIED:\n>> + case SM_UPDATE_NONE:\n>> + BUG(\"update strategy should \n>> have been specified\");\n>> + }\n>> + }\n>> + /*\n>> + * This signifies to the caller in shell that\n>> + * the command failed without dying\n>> + */\n>> + return 1;\n>> + }\n>> +\n>> + switch (ud->update_strategy.type) {\n>> + case SM_UPDATE_CHECKOUT:\n>> + printf(_(\"Submodule path '%s': checked out \n>> '%s'\\n\"),\n>> + ud->displaypath, oid);\n>> + break;\n>> + case SM_UPDATE_REBASE:\n>> + printf(_(\"Submodule path '%s': rebased into \n>> '%s'\\n\"),\n>> + ud->displaypath, oid);\n>> + break;\n>> + case SM_UPDATE_MERGE:\n>> + printf(_(\"Submodule path '%s': merged in \n>> '%s'\\n\"),\n>> + ud->displaypath, oid);\n>> + break;\n>> + case SM_UPDATE_COMMAND:\n>> + printf(_(\"Submodule path '%s': '%s %s'\\n\"),\n>> + ud->displaypath, \n>> ud->update_strategy.command, oid);\n>> + break;\n>> + case SM_UPDATE_UNSPECIFIED:\n>> + case SM_UPDATE_NONE:\n>> + BUG(\"update strategy should have been \n>> specified\");\n>> + }\n>> +\n>> + return 0;\n>> +}\n>> +\n>> +static int do_run_update_procedure(struct update_data *ud)\n>> +{\n>> + int subforce = is_null_oid(&ud->suboid) || ud->force;\n>> +\n>> + if (!ud->nofetch) {\n>> + /*\n>> + * Run fetch only if `oid` isn't present or it\n>> + * is not reachable from a ref.\n>> + */\n>> + if (!is_tip_reachable(ud->sm_path, &ud->oid))\n>> + if (fetch_in_submodule(ud->sm_path, \n>> ud->depth, ud->quiet, NULL) &&\n>> + !ud->quiet)\n>> + fprintf_ln(stderr,\n>> + _(\"Unable to fetch \n>> in submodule path '%s'; \"\n>> + \"trying to \n>> directly fetch %s:\"),\n>> + ud->displaypath, \n>> oid_to_hex(&ud->oid));\n>\n> I was wondering if an OID is invalid, will it be counted as\n> unreachable and vice-versa?\n> If that is the case then that would simplify the work.\n\nCould you elaborate? I'm not sure what you mean by 'invalid' in \nthis\ncontext. I don't think this code will receive any kind of \nmalformed\noid--they come from 'update-clone' which handles it correctly.\n\nAs far as I can tell, the only way to check if a particular OID is\nunreachable is when we check if all the refs cannot find it.\n"},{"id":"431810","messageId":"m2zgtyaljh.fsf@gmail.com","threadId":"56147","inReplyTo":"CACdWUYXhckBkHLPnRDxxb=raAD0=7236jAzvneBLhw8fXvGTMw@mail.gmail.com","subject":"Re: [GSoC] [PATCH v2] submodule--helper: run update procedures from C","fromName":"Atharva Raykar","fromEmail":"raykar.ath@gmail.com","sentAt":"2021-08-03T10:07:46Z","receivedAt":"2021-08-03T10:08:22Z","isPatch":true,"sender":{"key":"raykar.ath@gmail.com","avatar":"https://avatars.githubusercontent.com/u/24277692?v=4"},"body":"\n(I am resending this email, because my client mangled the whitespaces\ndue to a misconfiguration. Please ignore the my previous message.)\n\nShourya Shukla writes:\n\n> Le lun. 2 août 2021 à 18:36, Atharva Raykar a écrit :\n>>\n>> Add a new submodule--helper subcommand `run-update-procedure` that runs\n>> the update procedure if the SHA1 of the submodule does not match what\n>> the superproject expects.\n>>\n>> This is an intermediate change that works towards total conversion of\n>> `submodule update` from shell to C.\n>>\n>> Specific error codes are returned so that the shell script calling the\n>> subcommand can take a decision on the control flow, and preserve the\n>> error messages across subsequent recursive calls of `cmd_update`.\n>>\n>> This patch could have been approached differently, by first changing the\n>> `is_tip_reachable` and `fetch_in_submodule` shell functions to be\n>> `submodule--helper` subcommands, and then following up with a patch that\n>> introduces the `run-update-procedure` subcommand. We have not done it\n>> like that because those functions are trivial enough to convert directly\n>> along with these other changes. This lets us avoid the boilerplate and\n>> the cleanup patches that will need to be introduced in following that\n>> approach.\n>\n> I feel that this part is more suitable for a cover letter rather than the commit\n> message itself. It is a useful piece of info though.\n\nOkay, that seems right, the message does seem a bit too context-sensitive.\n\n>> This change is more focused on doing a faithful conversion, so for now we\n>> are not too concerned with trying to reduce subprocess spawns.\n>>\n>> Signed-off-by: Atharva Raykar \n>> Mentored-by: Christian Couder \n>> Mentored-by: Shourya Shukla \n>> ---\n>>\n>> Notable changes since v1:\n>>\n>> * Modified the code structure in\n>> submodule--helper.c:run_update_command(), while fixing problems with\n>> the translation marks.\n>>\n>> * Renamed '--sha1' and '--subsha1' options to '--oid' and '--suboid' to\n>> since the argument is parsed into an object_id struct, not plain sha1\n>> data.\n>>\n>> * Used option callbacks to parse the SHA1 arguments directly.\n>>\n>> * Moved the conditional out of 'do_run_update_procedure()'.\n>>\n>> Feedback required:\n>>\n>> Ævar felt that it would be clearer to populate the 'fatal' messages\n>> after the run_command() operation in 'run_update_command()', to make it\n>> more readable [1]. I have attempted something like that here, and it has led\n>> to a lot more duplicated 'switch' statements, which feels suboptimal.\n>> I'd appreciate suggestions to make it more legible.\n>>\n>> [1] https://lore.kernel.org/git/87r1fps63r.fsf@evledraar.gmail.com/\n>>\n>> Fetch-it-Via:\n>> git fetch https://github.com/tfidfwastaken/git submodule-run-update-proc-list-2\n>>\n>> builtin/submodule--helper.c | 253 ++++++++++++++++++++++++++++++++++++\n>> git-submodule.sh | 106 +++++----------\n>> 2 files changed, 286 insertions(+), 73 deletions(-)\n>>\n>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n>> index d55f6262e9..b9c40324d0 100644\n>> --- a/builtin/submodule--helper.c\n>> +++ b/builtin/submodule--helper.c\n>> @@ -2029,6 +2029,20 @@ struct submodule_update_clone {\n>> .max_jobs = 1, \\\n>> }\n>>\n>> +struct update_data {\n>> + const char *recursive_prefix;\n>> + const char *sm_path;\n>> + const char *displaypath;\n>> + struct object_id oid;\n>> + struct object_id suboid;\n>> + struct submodule_update_strategy update_strategy;\n>> + int depth;\n>> + unsigned int force: 1;\n>> + unsigned int quiet: 1;\n>> + unsigned int nofetch: 1;\n>> + unsigned int just_cloned: 1;\n>> +};\n>> +#define UPDATE_DATA_INIT { .update_strategy = SUBMODULE_UPDATE_STRATEGY_INIT }\n>>\n>> static void next_submodule_warn_missing(struct submodule_update_clone *suc,\n>> struct strbuf *out, const char *displaypath)\n>> @@ -2282,6 +2296,175 @@ static int git_update_clone_config(const char *var, const char *value,\n>> return 0;\n>> }\n>> +\n>> +static int run_update_command(struct update_data *ud, int subforce)\n>> +{\n>> + struct child_process cp = CHILD_PROCESS_INIT;\n>> + char *oid = oid_to_hex(&ud->oid);\n>> + int must_die_on_failure = 0;\n>> +\n>> + cp.dir = xstrdup(ud->sm_path);\n>> + switch (ud->update_strategy.type) {\n>> + case SM_UPDATE_CHECKOUT:\n>> + cp.git_cmd = 1;\n>> + strvec_pushl(&cp.args, \"checkout\", \"-q\", NULL);\n>\n> Would it be possible to add the 'if' statement above just before the\n> 'switch' (or after,\n> whichever seems okay) since this is common amongst (almost) all the cases?\n\nI'll try it on once, if it makes the code more readable, I'll include it in the\nreroll.\n\n>> + if (subforce)\n>> + strvec_push(&cp.args, \"-f\");\n>> + break;\n>> + case SM_UPDATE_REBASE:\n>> + cp.git_cmd = 1;\n>> + strvec_push(&cp.args, \"rebase\");\n>> + if (ud->quiet)\n>> + strvec_push(&cp.args, \"--quiet\");\n>> + must_die_on_failure = 1;\n>> + break;\n>> + case SM_UPDATE_MERGE:\n>> + cp.git_cmd = 1;\n>> + strvec_push(&cp.args, \"merge\");\n>> + if (ud->quiet)\n>> + strvec_push(&cp.args, \"--quiet\");\n>> + must_die_on_failure = 1;\n>> + break;\n>> + case SM_UPDATE_COMMAND:\n>> + /* NOTE: this does not handle quoted arguments */\n>> + strvec_split(&cp.args, ud->update_strategy.command);\n>> + must_die_on_failure = 1;\n>> + break;\n>> + case SM_UPDATE_UNSPECIFIED:\n>> + case SM_UPDATE_NONE:\n>> + BUG(\"update strategy should have been specified\");\n>> + }\n>\n> If the original did not bug out, do we need to? The documentation does\n> not mention\n> this as well:\n> https://git-scm.com/docs/git-submodule#Documentation/git-submodule.txt-none\n\nThis was how the original shell porcelain did it:\n\ncase \"$update_module\" in\ncheckout)\n\tcommand=\"git checkout $subforce -q\"\n\tdie_msg=\"$(eval_gettext \"Unable to checkout '\\$sha1' in submodule path '\\$displaypath'\")\"\n\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': checked out '\\$sha1'\")\"\n\t;;\nrebase)\n\tcommand=\"git rebase ${GIT_QUIET:+--quiet}\"\n\tdie_msg=\"$(eval_gettext \"Unable to rebase '\\$sha1' in submodule path '\\$displaypath'\")\"\n\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': rebased into '\\$sha1'\")\"\n\tmust_die_on_failure=yes\n\t;;\nmerge)\n\tcommand=\"git merge ${GIT_QUIET:+--quiet}\"\n\tdie_msg=\"$(eval_gettext \"Unable to merge '\\$sha1' in submodule path '\\$displaypath'\")\"\n\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': merged in '\\$sha1'\")\"\n\tmust_die_on_failure=yes\n\t;;\n!*)\n\tcommand=\"${update_module#!}\"\n\tdie_msg=\"$(eval_gettext \"Execution of '\\$command \\$sha1' failed in submodule path '\\$displaypath'\")\"\n\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': '\\$command \\$sha1'\")\"\n\tmust_die_on_failure=yes\n\t;;\n*)\n\tdie \"$(eval_gettext \"Invalid update mode '$update_module' for submodule path '$path'\")\"\nesac\n\nThe fallthrough case used to die, but I noticed that this branch will never get\nactivated. This is because the 'update-clone' helper will not output any entry\nthat has the update mode set to 'none', and thus the `while` loop that contains\nthis code would never run.\n\nWhich is why I decided to BUG out on that case, because that state should never\nbe reached. But I see the source of confusion, and maybe I should have different\nBUG messages for SM_UPDATE_UNSPECIFIED and SM_UPDATE_NONE. The latter should\nprobably say \"should have been handled by update-clone\".\n\n>> +\n>> + strvec_push(&cp.args, oid);\n>> +\n>> + prepare_submodule_repo_env(&cp.env_array);\n>> +\n>> + if (run_command(&cp)) {\n>> + if (must_die_on_failure) {\n>> + switch (ud->update_strategy.type) {\n>> + case SM_UPDATE_CHECKOUT:\n>> + die(_(\"Unable to checkout '%s' in submodule path '%s'\"),\n>> + oid, ud->displaypath);\n>> + break;\n>> + case SM_UPDATE_REBASE:\n>> + die(_(\"Unable to rebase '%s' in submodule path '%s'\"),\n>> + oid, ud->displaypath);\n>> + break;\n>> + case SM_UPDATE_MERGE:\n>> + die(_(\"Unable to merge '%s' in submodule path '%s'\"),\n>> + oid, ud->displaypath);\n>> + break;\n>> + case SM_UPDATE_COMMAND:\n>> + die(_(\"Execution of '%s %s' failed in submodule path '%s'\"),\n>> + ud->update_strategy.command, oid, ud->displaypath);\n>> + break;\n>> + case SM_UPDATE_UNSPECIFIED:\n>> + case SM_UPDATE_NONE:\n>> + BUG(\"update strategy should have been specified\");\n>> + }\n>> + }\n>> + /*\n>> + * This signifies to the caller in shell that\n>> + * the command failed without dying\n>> + */\n>> + return 1;\n>> + }\n>> +\n>> + switch (ud->update_strategy.type) {\n>> + case SM_UPDATE_CHECKOUT:\n>> + printf(_(\"Submodule path '%s': checked out '%s'\\n\"),\n>> + ud->displaypath, oid);\n>> + break;\n>> + case SM_UPDATE_REBASE:\n>> + printf(_(\"Submodule path '%s': rebased into '%s'\\n\"),\n>> + ud->displaypath, oid);\n>> + break;\n>> + case SM_UPDATE_MERGE:\n>> + printf(_(\"Submodule path '%s': merged in '%s'\\n\"),\n>> + ud->displaypath, oid);\n>> + break;\n>> + case SM_UPDATE_COMMAND:\n>> + printf(_(\"Submodule path '%s': '%s %s'\\n\"),\n>> + ud->displaypath, ud->update_strategy.command, oid);\n>> + break;\n>> + case SM_UPDATE_UNSPECIFIED:\n>> + case SM_UPDATE_NONE:\n>> + BUG(\"update strategy should have been specified\");\n>> + }\n>> +\n>> + return 0;\n>> +}\n>> +\n>> +static int do_run_update_procedure(struct update_data *ud)\n>> +{\n>> + int subforce = is_null_oid(&ud->suboid) || ud->force;\n>> +\n>> + if (!ud->nofetch) {\n>> + /*\n>> + * Run fetch only if `oid` isn't present or it\n>> + * is not reachable from a ref.\n>> + */\n>> + if (!is_tip_reachable(ud->sm_path, &ud->oid))\n>> + if (fetch_in_submodule(ud->sm_path, ud->depth, ud->quiet, NULL) &&\n>> + !ud->quiet)\n>> + fprintf_ln(stderr,\n>> + _(\"Unable to fetch in submodule path '%s'; \"\n>> + \"trying to directly fetch %s:\"),\n>> + ud->displaypath, oid_to_hex(&ud->oid));\n>\n> I was wondering if an OID is invalid, will it be counted as\n> unreachable and vice-versa?\n> If that is the case then that would simplify the work.\n\nCould you elaborate? I'm not sure what you mean by 'invalid' in this context. I\ndon't think this code will receive any kind of malformed oid--they come from\n'update-clone' which handles it correctly.\n\nAs far as I can tell, the only way to check if a particular OID is unreachable\nis when we check if all the refs cannot find it.\n"},{"id":"431943","messageId":"m2wnp1a9q7.fsf@gmail.com","threadId":"56147","inReplyTo":"9532C3EF-257E-4898-8C75-C49EA4B66A99@gmail.com","subject":"Re: [GSoC] [PATCH] submodule--helper: run update procedures from C","fromName":"Atharva Raykar","fromEmail":"raykar.ath@gmail.com","sentAt":"2021-08-04T08:35:12Z","receivedAt":"2021-08-04T08:35:19Z","isPatch":true,"sender":{"key":"raykar.ath@gmail.com","avatar":"https://avatars.githubusercontent.com/u/24277692?v=4"},"body":"\nAtharva Raykar writes:\n>> Ævar Arnfjörð Bjarmason wrote:\n>>> static void update_submodule(struct update_clone_data *ucd)\n>>> {\n>>> \tfprintf(stdout, \"dummy %s %d\\t%s\\n\",\n>>> @@ -2379,6 +2552,79 @@ static int update_clone(int argc, const char **argv, const char *prefix)\n>>> \treturn update_submodules(&suc);\n>>> }\n>>>\n>>> +static int run_update_procedure(int argc, const char **argv, const char *prefix)\n>>> +{\n>>> +\tint force = 0, quiet = 0, nofetch = 0, just_cloned = 0;\n>>> +\tchar *prefixed_path, *update = NULL;\n>>> +\tchar *sha1 = NULL, *subsha1 = NULL;\n>>> +\tstruct update_data update_data = UPDATE_DATA_INIT;\n>>> +\n>>> +\tstruct option options[] = {\n>>> +\t\tOPT__QUIET(&quiet, N_(\"suppress output for update by rebase or merge\")),\n>>> +\t\tOPT__FORCE(&force, N_(\"force checkout updates\"), 0),\n>>> +\t\tOPT_BOOL('N', \"no-fetch\", &nofetch,\n>>> +\t\t\t N_(\"don't fetch new objects from the remote site\")),\n>>> +\t\tOPT_BOOL(0, \"just-cloned\", &just_cloned,\n>>> +\t\t\t N_(\"overrides update mode in case the repository is a fresh clone\")),\n>>> +\t\tOPT_INTEGER(0, \"depth\", &update_data.depth, N_(\"depth for shallow fetch\")),\n>>> +\t\tOPT_STRING(0, \"prefix\", &prefix,\n>>> +\t\t\t N_(\"path\"),\n>>> +\t\t\t N_(\"path into the working tree\")),\n>>> +\t\tOPT_STRING(0, \"update\", &update,\n>>> +\t\t\t N_(\"string\"),\n>>> +\t\t\t N_(\"rebase, merge, checkout or none\")),\n>>> +\t\tOPT_STRING(0, \"recursive-prefix\", &update_data.recursive_prefix, N_(\"path\"),\n>>> +\t\t\t N_(\"path into the working tree, across nested \"\n>>> +\t\t\t \"submodule boundaries\")),\n>>> +\t\tOPT_STRING(0, \"sha1\", &sha1, N_(\"string\"),\n>>> +\t\t\t N_(\"SHA1 expected by superproject\")),\n>>> +\t\tOPT_STRING(0, \"subsha1\", &subsha1, N_(\"string\"),\n>>> +\t\t\t N_(\"SHA1 of submodule's HEAD\")),\n>>> +\t\tOPT_END()\n>>> +\t};\n>>> +\n>>> +\tconst char *const usage[] = {\n>>> +\t\tN_(\"git submodule--helper run-update-procedure [] \"),\n>>> +\t\tNULL\n>>> +\t};\n>>> +\n>>> +\targc = parse_options(argc, argv, prefix, options, usage, 0);\n>>> +\n>>> +\tif (argc != 1)\n>>> +\t\tusage_with_options(usage, options);\n>>> +\tupdate_data.force = !!force;\n>>> +\tupdate_data.quiet = !!quiet;\n>>> +\tupdate_data.nofetch = !!nofetch;\n>>> +\tupdate_data.just_cloned = !!just_cloned;\n>>\n>> For all of these just pass the reference to the update_data variable\n>> directly in the OPT_*(). No need to set an \"int force\", only to copy it\n>> over to update_data.force. Let's just use the latter only.\n>\n> Hmm, I'm trying to remember why the single bit values are treated this way\n> in this whole file...\n>\n> ...there seems to be no good reason for it. The API docs for parse options\n> state that OPT_BOOL() is guaranteed to return either zero or one, so that\n> double negation does look unnecessary.\n\nI forgot to mention why I did not address this change in my v3 patch.\nThe reason why we are handling boolean values this way is because they\nare declared as bitfields in the 'update_data' struct. Since we cannot\ntake the address of bitfields, we have to use a different variable to\nstore when using 'parse_options()'.\n"},{"id":"432630","messageId":"20210813075653.56817-1-raykar.ath@gmail.com","threadId":"56147","inReplyTo":"20210802130627.36170-1-raykar.ath@gmail.com","subject":"[GSoC] [PATCH v3] submodule--helper: run update procedures from C","fromName":"Atharva Raykar","fromEmail":"raykar.ath@gmail.com","sentAt":"2021-08-13T07:56:53Z","receivedAt":"2021-08-13T07:57:11Z","isPatch":true,"sender":{"key":"raykar.ath@gmail.com","avatar":"https://avatars.githubusercontent.com/u/24277692?v=4"},"body":"Add a new submodule--helper subcommand `run-update-procedure` that runs\nthe update procedure if the SHA1 of the submodule does not match what\nthe superproject expects.\n\nThis is an intermediate change that works towards total conversion of\n`submodule update` from shell to C.\n\nSpecific error codes are returned so that the shell script calling the\nsubcommand can take a decision on the control flow, and preserve the\nerror messages across subsequent recursive calls of `cmd_update`.\n\nThis change is more focused on doing a faithful conversion, so for now we\nare not too concerned with trying to reduce subprocess spawns.\n\nSigned-off-by: Atharva Raykar \nMentored-by: Christian Couder \nMentored-by: Shourya Shukla \n---\n\nThis patch could have been approached differently, by first changing the\n`is_tip_reachable` and `fetch_in_submodule` shell functions to be\n`submodule--helper` subcommands, and then following up with a patch that\nintroduces the `run-update-procedure` subcommand. We have not done it\nlike that because those functions are trivial enough to convert directly\nalong with these other changes. This lets us avoid the boilerplate and\nthe cleanup patches that will need to be introduced in following that\napproach.\n\nSince v2:\n* Different BUG messages in run_update_command() for the \"Unspecified\" and\n \"None\" update modes.\n* Move the information about how the patch was approached out of the commit\n message.\n* Rebase this patch on top of master (the previous one was based on a stale,\n unmerged topic branch). This patch no longer depends on a topic branch.\n\nFetch-it-Via:\ngit fetch https://github.com/tfidfwastaken/git submodule-run-update-proc-list-4\n\n builtin/submodule--helper.c | 259 ++++++++++++++++++++++++++++++++++++\n git-submodule.sh | 104 +++++----------\n 2 files changed, 291 insertions(+), 72 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex ef2776a9e4..9b34b29ce2 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2045,6 +2045,20 @@ struct submodule_update_clone {\n \t.max_jobs = 1, \\\n }\n \n+struct update_data {\n+\tconst char *recursive_prefix;\n+\tconst char *sm_path;\n+\tconst char *displaypath;\n+\tstruct object_id oid;\n+\tstruct object_id suboid;\n+\tstruct submodule_update_strategy update_strategy;\n+\tint depth;\n+\tunsigned int force: 1;\n+\tunsigned int quiet: 1;\n+\tunsigned int nofetch: 1;\n+\tunsigned int just_cloned: 1;\n+};\n+#define UPDATE_DATA_INIT { .update_strategy = SUBMODULE_UPDATE_STRATEGY_INIT }\n \n static void next_submodule_warn_missing(struct submodule_update_clone *suc,\n \t\tstruct strbuf *out, const char *displaypath)\n@@ -2298,6 +2312,181 @@ static int git_update_clone_config(const char *var, const char *value,\n \treturn 0;\n }\n \n+static int is_tip_reachable(const char *path, struct object_id *oid)\n+{\n+\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\tstruct strbuf rev = STRBUF_INIT;\n+\tchar *hex = oid_to_hex(oid);\n+\n+\tcp.git_cmd = 1;\n+\tcp.dir = xstrdup(path);\n+\tcp.no_stderr = 1;\n+\tstrvec_pushl(&cp.args, \"rev-list\", \"-n\", \"1\", hex, \"--not\", \"--all\", NULL);\n+\n+\tprepare_submodule_repo_env(&cp.env_array);\n+\n+\tif (capture_command(&cp, &rev, GIT_MAX_HEXSZ + 1) || rev.len)\n+\t\treturn 0;\n+\n+\treturn 1;\n+}\n+\n+static int fetch_in_submodule(const char *module_path, int depth, int quiet, struct object_id *oid)\n+{\n+\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\n+\tprepare_submodule_repo_env(&cp.env_array);\n+\tcp.git_cmd = 1;\n+\tcp.dir = xstrdup(module_path);\n+\n+\tstrvec_push(&cp.args, \"fetch\");\n+\tif (quiet)\n+\t\tstrvec_push(&cp.args, \"--quiet\");\n+\tif (depth)\n+\t\tstrvec_pushf(&cp.args, \"--depth=%d\", depth);\n+\tif (oid) {\n+\t\tchar *hex = oid_to_hex(oid);\n+\t\tchar *remote = get_default_remote();\n+\t\tstrvec_pushl(&cp.args, remote, hex, NULL);\n+\t}\n+\n+\treturn run_command(&cp);\n+}\n+\n+static int run_update_command(struct update_data *ud, int subforce)\n+{\n+\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\tchar *oid = oid_to_hex(&ud->oid);\n+\tint must_die_on_failure = 0;\n+\n+\tcp.dir = xstrdup(ud->sm_path);\n+\tswitch (ud->update_strategy.type) {\n+\tcase SM_UPDATE_CHECKOUT:\n+\t\tcp.git_cmd = 1;\n+\t\tstrvec_pushl(&cp.args, \"checkout\", \"-q\", NULL);\n+\t\tif (subforce)\n+\t\t\tstrvec_push(&cp.args, \"-f\");\n+\t\tbreak;\n+\tcase SM_UPDATE_REBASE:\n+\t\tcp.git_cmd = 1;\n+\t\tstrvec_push(&cp.args, \"rebase\");\n+\t\tif (ud->quiet)\n+\t\t\tstrvec_push(&cp.args, \"--quiet\");\n+\t\tmust_die_on_failure = 1;\n+\t\tbreak;\n+\tcase SM_UPDATE_MERGE:\n+\t\tcp.git_cmd = 1;\n+\t\tstrvec_push(&cp.args, \"merge\");\n+\t\tif (ud->quiet)\n+\t\t\tstrvec_push(&cp.args, \"--quiet\");\n+\t\tmust_die_on_failure = 1;\n+\t\tbreak;\n+\tcase SM_UPDATE_COMMAND:\n+\t\t/* NOTE: this does not handle quoted arguments */\n+\t\tstrvec_split(&cp.args, ud->update_strategy.command);\n+\t\tmust_die_on_failure = 1;\n+\t\tbreak;\n+\tcase SM_UPDATE_NONE:\n+\t\tBUG(\"this should have been handled before. How did we reach here?\");\n+\t\tbreak;\n+\tcase SM_UPDATE_UNSPECIFIED:\n+\t\tBUG(\"update strategy should have been specified\");\n+\t}\n+\n+\tstrvec_push(&cp.args, oid);\n+\n+\tprepare_submodule_repo_env(&cp.env_array);\n+\n+\tif (run_command(&cp)) {\n+\t\tif (must_die_on_failure) {\n+\t\t\tswitch (ud->update_strategy.type) {\n+\t\t\tcase SM_UPDATE_CHECKOUT:\n+\t\t\t\tdie(_(\"Unable to checkout '%s' in submodule path '%s'\"),\n+\t\t\t\t oid, ud->displaypath);\n+\t\t\t\tbreak;\n+\t\t\tcase SM_UPDATE_REBASE:\n+\t\t\t\tdie(_(\"Unable to rebase '%s' in submodule path '%s'\"),\n+\t\t\t\t oid, ud->displaypath);\n+\t\t\t\tbreak;\n+\t\t\tcase SM_UPDATE_MERGE:\n+\t\t\t\tdie(_(\"Unable to merge '%s' in submodule path '%s'\"),\n+\t\t\t\t oid, ud->displaypath);\n+\t\t\t\tbreak;\n+\t\t\tcase SM_UPDATE_COMMAND:\n+\t\t\t\tdie(_(\"Execution of '%s %s' failed in submodule path '%s'\"),\n+\t\t\t\t ud->update_strategy.command, oid, ud->displaypath);\n+\t\t\t\tbreak;\n+\t\t\tcase SM_UPDATE_NONE:\n+\t\t\t\tBUG(\"this should have been handled before. How did we reach here?\");\n+\t\t\t\tbreak;\n+\t\t\tcase SM_UPDATE_UNSPECIFIED:\n+\t\t\t\tBUG(\"update strategy should have been specified\");\n+\t\t\t}\n+\t\t}\n+\t\t/*\n+\t\t * This signifies to the caller in shell that\n+\t\t * the command failed without dying\n+\t\t */\n+\t\treturn 1;\n+\t}\n+\n+\tswitch (ud->update_strategy.type) {\n+\tcase SM_UPDATE_CHECKOUT:\n+\t\tprintf(_(\"Submodule path '%s': checked out '%s'\\n\"),\n+\t\t ud->displaypath, oid);\n+\t\tbreak;\n+\tcase SM_UPDATE_REBASE:\n+\t\tprintf(_(\"Submodule path '%s': rebased into '%s'\\n\"),\n+\t\t ud->displaypath, oid);\n+\t\tbreak;\n+\tcase SM_UPDATE_MERGE:\n+\t\tprintf(_(\"Submodule path '%s': merged in '%s'\\n\"),\n+\t\t ud->displaypath, oid);\n+\t\tbreak;\n+\tcase SM_UPDATE_COMMAND:\n+\t\tprintf(_(\"Submodule path '%s': '%s %s'\\n\"),\n+\t\t ud->displaypath, ud->update_strategy.command, oid);\n+\t\tbreak;\n+\tcase SM_UPDATE_NONE:\n+\t\tBUG(\"this should have been handled before. How did we reach here?\");\n+\t\tbreak;\n+\tcase SM_UPDATE_UNSPECIFIED:\n+\t\tBUG(\"update strategy should have been specified\");\n+\t}\n+\n+\treturn 0;\n+}\n+\n+static int do_run_update_procedure(struct update_data *ud)\n+{\n+\tint subforce = is_null_oid(&ud->suboid) || ud->force;\n+\n+\tif (!ud->nofetch) {\n+\t\t/*\n+\t\t * Run fetch only if `oid` isn't present or it\n+\t\t * is not reachable from a ref.\n+\t\t */\n+\t\tif (!is_tip_reachable(ud->sm_path, &ud->oid))\n+\t\t\tif (fetch_in_submodule(ud->sm_path, ud->depth, ud->quiet, NULL) &&\n+\t\t\t !ud->quiet)\n+\t\t\t\tfprintf_ln(stderr,\n+\t\t\t\t\t _(\"Unable to fetch in submodule path '%s'; \"\n+\t\t\t\t\t \"trying to directly fetch %s:\"),\n+\t\t\t\t\t ud->displaypath, oid_to_hex(&ud->oid));\n+\t\t/*\n+\t\t * Now we tried the usual fetch, but `oid` may\n+\t\t * not be reachable from any of the refs.\n+\t\t */\n+\t\tif (!is_tip_reachable(ud->sm_path, &ud->oid))\n+\t\t\tif (fetch_in_submodule(ud->sm_path, ud->depth, ud->quiet, &ud->oid))\n+\t\t\t\tdie(_(\"Fetched in submodule path '%s', but it did not \"\n+\t\t\t\t \"contain %s. Direct fetching of that commit failed.\"),\n+\t\t\t\t ud->displaypath, oid_to_hex(&ud->oid));\n+\t}\n+\n+\treturn run_update_command(ud, subforce);\n+}\n+\n static void update_submodule(struct update_clone_data *ucd)\n {\n \tfprintf(stdout, \"dummy %s %d\\t%s\\n\",\n@@ -2395,6 +2584,75 @@ static int update_clone(int argc, const char **argv, const char *prefix)\n \treturn update_submodules(&suc);\n }\n \n+static int run_update_procedure(int argc, const char **argv, const char *prefix)\n+{\n+\tint force = 0, quiet = 0, nofetch = 0, just_cloned = 0;\n+\tchar *prefixed_path, *update = NULL;\n+\tstruct update_data update_data = UPDATE_DATA_INIT;\n+\n+\tstruct option options[] = {\n+\t\tOPT__QUIET(&quiet, N_(\"suppress output for update by rebase or merge\")),\n+\t\tOPT__FORCE(&force, N_(\"force checkout updates\"), 0),\n+\t\tOPT_BOOL('N', \"no-fetch\", &nofetch,\n+\t\t\t N_(\"don't fetch new objects from the remote site\")),\n+\t\tOPT_BOOL(0, \"just-cloned\", &just_cloned,\n+\t\t\t N_(\"overrides update mode in case the repository is a fresh clone\")),\n+\t\tOPT_INTEGER(0, \"depth\", &update_data.depth, N_(\"depth for shallow fetch\")),\n+\t\tOPT_STRING(0, \"prefix\", &prefix,\n+\t\t\t N_(\"path\"),\n+\t\t\t N_(\"path into the working tree\")),\n+\t\tOPT_STRING(0, \"update\", &update,\n+\t\t\t N_(\"string\"),\n+\t\t\t N_(\"rebase, merge, checkout or none\")),\n+\t\tOPT_STRING(0, \"recursive-prefix\", &update_data.recursive_prefix, N_(\"path\"),\n+\t\t\t N_(\"path into the working tree, across nested \"\n+\t\t\t \"submodule boundaries\")),\n+\t\tOPT_CALLBACK_F(0, \"oid\", &update_data.oid, N_(\"sha1\"),\n+\t\t\t N_(\"SHA1 expected by superproject\"), PARSE_OPT_NONEG,\n+\t\t\t parse_opt_object_id),\n+\t\tOPT_CALLBACK_F(0, \"suboid\", &update_data.suboid, N_(\"subsha1\"),\n+\t\t\t N_(\"SHA1 of submodule's HEAD\"), PARSE_OPT_NONEG,\n+\t\t\t parse_opt_object_id),\n+\t\tOPT_END()\n+\t};\n+\n+\tconst char *const usage[] = {\n+\t\tN_(\"git submodule--helper run-update-procedure [] \"),\n+\t\tNULL\n+\t};\n+\n+\targc = parse_options(argc, argv, prefix, options, usage, 0);\n+\n+\tif (argc != 1)\n+\t\tusage_with_options(usage, options);\n+\n+\tupdate_data.force = !!force;\n+\tupdate_data.quiet = !!quiet;\n+\tupdate_data.nofetch = !!nofetch;\n+\tupdate_data.just_cloned = !!just_cloned;\n+\tupdate_data.sm_path = argv[0];\n+\n+\tif (update_data.recursive_prefix)\n+\t\tprefixed_path = xstrfmt(\"%s%s\", update_data.recursive_prefix, update_data.sm_path);\n+\telse\n+\t\tprefixed_path = xstrdup(update_data.sm_path);\n+\n+\tupdate_data.displaypath = get_submodule_displaypath(prefixed_path, prefix);\n+\n+\tdetermine_submodule_update_strategy(the_repository, update_data.just_cloned,\n+\t\t\t\t\t update_data.sm_path, update,\n+\t\t\t\t\t &update_data.update_strategy);\n+\n+\tfree(prefixed_path);\n+\n+\tif ((!is_null_oid(&update_data.oid) && !is_null_oid(&update_data.suboid) &&\n+\t !oideq(&update_data.oid, &update_data.suboid)) ||\n+\t is_null_oid(&update_data.suboid) || update_data.force)\n+\t\treturn do_run_update_procedure(&update_data);\n+\n+\treturn 3;\n+}\n+\n static int resolve_relative_path(int argc, const char **argv, const char *prefix)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n@@ -2951,6 +3209,7 @@ static struct cmd_struct commands[] = {\n \t{\"add-clone\", add_clone, 0},\n \t{\"update-module-mode\", module_update_module_mode, 0},\n \t{\"update-clone\", update_clone, 0},\n+\t{\"run-update-procedure\", run_update_procedure, 0},\n \t{\"ensure-core-worktree\", ensure_core_worktree, 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 dbd2ec2050..d8e30d1afa 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -369,13 +369,6 @@ cmd_deinit()\n \tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper deinit ${GIT_QUIET:+--quiet} ${force:+--force} ${deinit_all:+--all} -- \"$@\"\n }\n \n-is_tip_reachable () (\n-\tsanitize_submodule_env &&\n-\tcd \"$1\" &&\n-\trev=$(git rev-list -n 1 \"$2\" --not --all 2>/dev/null) &&\n-\ttest -z \"$rev\"\n-)\n-\n # usage: fetch_in_submodule [] []\n # Because arguments are positional, use an empty string to omit \n # but include .\n@@ -519,14 +512,13 @@ cmd_update()\n \n \t\tgit submodule--helper ensure-core-worktree \"$sm_path\" || exit 1\n \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\telse\n+\t\t\tjust_cloned=\n \t\t\tsubsha1=$(sanitize_submodule_env; cd \"$sm_path\" &&\n \t\t\t\tgit rev-parse --verify HEAD) ||\n \t\t\tdie \"fatal: $(eval_gettext \"Unable to find current revision in submodule path '\\$displaypath'\")\"\n@@ -547,70 +539,38 @@ cmd_update()\n \t\t\tdie \"fatal: $(eval_gettext \"Unable to find current \\${remote_name}/\\${branch} revision in submodule path '\\$sm_path'\")\"\n \t\tfi\n \n-\t\tif test \"$subsha1\" != \"$sha1\" || test -n \"$force\"\n-\t\tthen\n-\t\t\tsubforce=$force\n-\t\t\t# If we don't already have a -f flag and the submodule has never been checked out\n-\t\t\tif test -z \"$subsha1\" && test -z \"$force\"\n-\t\t\tthen\n-\t\t\t\tsubforce=\"-f\"\n-\t\t\tfi\n+\t\tout=$(git submodule--helper run-update-procedure \\\n+\t\t\t ${wt_prefix:+--prefix \"$wt_prefix\"} \\\n+\t\t\t ${GIT_QUIET:+--quiet} \\\n+\t\t\t ${force:+--force} \\\n+\t\t\t ${just_cloned:+--just-cloned} \\\n+\t\t\t ${nofetch:+--no-fetch} \\\n+\t\t\t ${depth:+\"$depth\"} \\\n+\t\t\t ${update:+--update \"$update\"} \\\n+\t\t\t ${prefix:+--recursive-prefix \"$prefix\"} \\\n+\t\t\t ${sha1:+--oid \"$sha1\"} \\\n+\t\t\t ${subsha1:+--suboid \"$subsha1\"} \\\n+\t\t\t \"--\" \\\n+\t\t\t \"$sm_path\")\n \n-\t\t\tif test -z \"$nofetch\"\n-\t\t\tthen\n-\t\t\t\t# Run fetch only if $sha1 isn't present or it\n-\t\t\t\t# is not reachable from a ref.\n-\t\t\t\tis_tip_reachable \"$sm_path\" \"$sha1\" ||\n-\t\t\t\tfetch_in_submodule \"$sm_path\" $depth ||\n-\t\t\t\tsay \"$(eval_gettext \"Unable to fetch in submodule path '\\$displaypath'; trying to directly fetch \\$sha1:\")\"\n-\n-\t\t\t\t# Now we tried the usual fetch, but $sha1 may\n-\t\t\t\t# not be reachable from any of the refs\n-\t\t\t\tis_tip_reachable \"$sm_path\" \"$sha1\" ||\n-\t\t\t\tfetch_in_submodule \"$sm_path\" \"$depth\" \"$sha1\" ||\n-\t\t\t\tdie \"fatal: $(eval_gettext \"Fetched in submodule path '\\$displaypath', but it did not contain \\$sha1. Direct fetching of that commit failed.\")\"\n-\t\t\tfi\n-\n-\t\t\tmust_die_on_failure=\n-\t\t\tcase \"$update_module\" in\n-\t\t\tcheckout)\n-\t\t\t\tcommand=\"git checkout $subforce -q\"\n-\t\t\t\tdie_msg=\"fatal: $(eval_gettext \"Unable to checkout '\\$sha1' in submodule path '\\$displaypath'\")\"\n-\t\t\t\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': checked out '\\$sha1'\")\"\n-\t\t\t\t;;\n-\t\t\trebase)\n-\t\t\t\tcommand=\"git rebase ${GIT_QUIET:+--quiet}\"\n-\t\t\t\tdie_msg=\"fatal: $(eval_gettext \"Unable to rebase '\\$sha1' in submodule path '\\$displaypath'\")\"\n-\t\t\t\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': rebased into '\\$sha1'\")\"\n-\t\t\t\tmust_die_on_failure=yes\n-\t\t\t\t;;\n-\t\t\tmerge)\n-\t\t\t\tcommand=\"git merge ${GIT_QUIET:+--quiet}\"\n-\t\t\t\tdie_msg=\"fatal: $(eval_gettext \"Unable to merge '\\$sha1' in submodule path '\\$displaypath'\")\"\n-\t\t\t\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': merged in '\\$sha1'\")\"\n-\t\t\t\tmust_die_on_failure=yes\n-\t\t\t\t;;\n-\t\t\t!*)\n-\t\t\t\tcommand=\"${update_module#!}\"\n-\t\t\t\tdie_msg=\"fatal: $(eval_gettext \"Execution of '\\$command \\$sha1' failed in submodule path '\\$displaypath'\")\"\n-\t\t\t\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': '\\$command \\$sha1'\")\"\n-\t\t\t\tmust_die_on_failure=yes\n-\t\t\t\t;;\n-\t\t\t*)\n-\t\t\t\tdie \"fatal: $(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-\t\t\tthen\n-\t\t\t\tsay \"$say_msg\"\n-\t\t\telif test -n \"$must_die_on_failure\"\n-\t\t\tthen\n-\t\t\t\tdie_with_status 2 \"$die_msg\"\n-\t\t\telse\n-\t\t\t\terr=\"${err};$die_msg\"\n-\t\t\t\tcontinue\n-\t\t\tfi\n-\t\tfi\n+\t\t# exit codes for run-update-procedure:\n+\t\t# 0: update was successful, say command output\n+\t\t# 128: subcommand died during execution\n+\t\t# 1: update procedure failed, but should not die\n+\t\t# 3: no update procedure was run\n+\t\tres=\"$?\"\n+\t\tcase $res in\n+\t\t0)\n+\t\t\tsay \"$out\"\n+\t\t\t;;\n+\t\t128)\n+\t\t\texit $res\n+\t\t\t;;\n+\t\t1)\n+\t\t\terr=\"${err};$out\"\n+\t\t\tcontinue\n+\t\t\t;;\n+\t\tesac\n \n \t\tif test -n \"$recursive\"\n \t\tthen\n-- \n2.32.0\n\n"},{"id":"432698","messageId":"xmqqim09w5yc.fsf@gitster.g","threadId":"56147","inReplyTo":"20210813075653.56817-1-raykar.ath@gmail.com","subject":"Re: [GSoC] [PATCH v3] submodule--helper: run update procedures from C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-13T18:32:59Z","receivedAt":"2021-08-13T18:33:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Atharva Raykar writes:\n\n> Add a new submodule--helper subcommand `run-update-procedure` that runs\n> the update procedure if the SHA1 of the submodule does not match what\n> the superproject expects.\n>\n> This is an intermediate change that works towards total conversion of\n> `submodule update` from shell to C.\n\nOK. Various things can happen depending on the setting during the\nupdate procedure, and it is complex enough to split it out to a\nsingle step, I guess.\n\n> Specific error codes are returned so that the shell script calling the\n> subcommand can take a decision on the control flow, and preserve the\n> error messages across subsequent recursive calls of `cmd_update`.\n>\n> This change is more focused on doing a faithful conversion, so for now we\n> are not too concerned with trying to reduce subprocess spawns.\n>\n> Signed-off-by: Atharva Raykar \n> Mentored-by: Christian Couder \n> Mentored-by: Shourya Shukla \n\nKeep trailer lines in chronological order. The mentors mentored,\nthe patch was written and finally you signed it off.\n\n> +static int run_update_command(struct update_data *ud, int subforce)\n> +{\n> +\tstruct child_process cp = CHILD_PROCESS_INIT;\n> +\tchar *oid = oid_to_hex(&ud->oid);\n> +\tint must_die_on_failure = 0;\n> +\n> +\tcp.dir = xstrdup(ud->sm_path);\n> +\tswitch (ud->update_strategy.type) {\n> +\tcase SM_UPDATE_CHECKOUT:\n> +\t\tcp.git_cmd = 1;\n> +\t\tstrvec_pushl(&cp.args, \"checkout\", \"-q\", NULL);\n> +\t\tif (subforce)\n> +\t\t\tstrvec_push(&cp.args, \"-f\");\n> +\t\tbreak;\n> +\tcase SM_UPDATE_REBASE:\n> +\t\tcp.git_cmd = 1;\n> +\t\tstrvec_push(&cp.args, \"rebase\");\n> +\t\tif (ud->quiet)\n> +\t\t\tstrvec_push(&cp.args, \"--quiet\");\n> +\t\tmust_die_on_failure = 1;\n> +\t\tbreak;\n> +\tcase SM_UPDATE_MERGE:\n> +\t\tcp.git_cmd = 1;\n> +\t\tstrvec_push(&cp.args, \"merge\");\n> +\t\tif (ud->quiet)\n> +\t\t\tstrvec_push(&cp.args, \"--quiet\");\n> +\t\tmust_die_on_failure = 1;\n> +\t\tbreak;\n> +\tcase SM_UPDATE_COMMAND:\n> +\t\t/* NOTE: this does not handle quoted arguments */\n> +\t\tstrvec_split(&cp.args, ud->update_strategy.command);\n\nIndeed this doesn't. I think cp.use_shell would be the right way to\nrun this.\n\nStudy what happens before run_command_v_opt_cd_env() with\nRUN_USING_SHELL calls run_command() and make something similar\nhappen here, instead of doing a manual command line splitting.\n\n\tSide note: run_command_v_opt_cd_env() with RUN_USING_SHELL\n\tis used in places like diff.c::run_external_diff() to invoke\n\tan external diff command, ll-merge.c::ll_ext_merge() to\n\tinvoke a user-defined low level merge driver,\n\tsequencer.c::do_exec() to invoke 'x cmd' you write in the\n\ttodo list during an \"rebase -i\" session.\n\n> +\t\tmust_die_on_failure = 1;\n> +\t\tbreak;\n> +\tcase SM_UPDATE_NONE:\n> +\t\tBUG(\"this should have been handled before. How did we reach here?\");\n> +\t\tbreak;\n> +\tcase SM_UPDATE_UNSPECIFIED:\n> +\t\tBUG(\"update strategy should have been specified\");\n\nThese two case arms are not a faithful conversion from the original,\nbut because you do not carry around a random string from the caller\nand instead have parsed enums, it cannot be ;-) But it makes me\nwonder why we want these two cases separate. Isn't it a BUG() if\nanything other than what we handled (i.e. prepared cp.args for)\nalready is in ud->update_strategy.type? IOW, wouldn't it be more\nforward looking to do\n\n\tdefault:\n\t\tBUG(\"unexpected ud->update_strategy.type (%d)\",\n\t\t ud->update_strategy.type (%d)\");\n\nor something? That way, if we ever come up with a new update\nstrategy and forget to update this part, we will catch such a bug\nfairly quickly.\n\n> +\t}\n> +\n> +\tstrvec_push(&cp.args, oid);\n> +\n> +\tprepare_submodule_repo_env(&cp.env_array);\n> +\n> +\tif (run_command(&cp)) {\n> +\t\tif (must_die_on_failure) {\n> +\t\t\tswitch (ud->update_strategy.type) {\n> +\t\t\tcase SM_UPDATE_CHECKOUT:\n> +\t\t\t\tdie(_(\"Unable to checkout '%s' in submodule path '%s'\"),\n> +\t\t\t\t oid, ud->displaypath);\n> +\t\t\t\tbreak;\n> +\t\t\tcase SM_UPDATE_REBASE:\n> +\t\t\t\tdie(_(\"Unable to rebase '%s' in submodule path '%s'\"),\n> +\t\t\t\t oid, ud->displaypath);\n> +\t\t\t\tbreak;\n> +\t\t\tcase SM_UPDATE_MERGE:\n> +\t\t\t\tdie(_(\"Unable to merge '%s' in submodule path '%s'\"),\n> +\t\t\t\t oid, ud->displaypath);\n> +\t\t\t\tbreak;\n> +\t\t\tcase SM_UPDATE_COMMAND:\n> +\t\t\t\tdie(_(\"Execution of '%s %s' failed in submodule path '%s'\"),\n> +\t\t\t\t ud->update_strategy.command, oid, ud->displaypath);\n> +\t\t\t\tbreak;\n\nThe messages here correspond to what is assigned to $die_message in\nthe original. Note that they are emitted to the standard error\nstream.\n\nI suspect that these should be \"printf()\" followed by a call to\nexit() with some non-zero value (see below).\n\n> +\t\t\tcase SM_UPDATE_NONE:\n> +\t\t\t\tBUG(\"this should have been handled before. How did we reach here?\");\n> +\t\t\t\tbreak;\n> +\t\t\tcase SM_UPDATE_UNSPECIFIED:\n> +\t\t\t\tBUG(\"update strategy should have been specified\");\n> +\t\t\t}\n\nThe same comment applies to the last two case arms of this switch\nstatement and the next one, too. I think we just should catch\n\"everything else\" with a simple \"default:\" label.\n\nAlso, don't omit the \"break;\" from the last case arm in a switch\nstatement. It harms the long-term help of the code---the last case\narm may not forever stay to be the last one.\n\n> +\t\t}\n> +\t\t/*\n> +\t\t * This signifies to the caller in shell that\n> +\t\t * the command failed without dying\n> +\t\t */\n> +\t\treturn 1;\n> +\t}\n> +\n> +\tswitch (ud->update_strategy.type) {\n> +\tcase SM_UPDATE_CHECKOUT:\n> +\t\tprintf(_(\"Submodule path '%s': checked out '%s'\\n\"),\n> +\t\t ud->displaypath, oid);\n> +\t\tbreak;\n> +\tcase SM_UPDATE_REBASE:\n> +\t\tprintf(_(\"Submodule path '%s': rebased into '%s'\\n\"),\n> +\t\t ud->displaypath, oid);\n> +\t\tbreak;\n> +\tcase SM_UPDATE_MERGE:\n> +\t\tprintf(_(\"Submodule path '%s': merged in '%s'\\n\"),\n> +\t\t ud->displaypath, oid);\n> +\t\tbreak;\n> +\tcase SM_UPDATE_COMMAND:\n> +\t\tprintf(_(\"Submodule path '%s': '%s %s'\\n\"),\n> +\t\t ud->displaypath, ud->update_strategy.command, oid);\n> +\t\tbreak;\n> +\tcase SM_UPDATE_NONE:\n> +\t\tBUG(\"this should have been handled before. How did we reach here?\");\n> +\t\tbreak;\n> +\tcase SM_UPDATE_UNSPECIFIED:\n> +\t\tBUG(\"update strategy should have been specified\");\n\nLikewise here.\n\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +static int do_run_update_procedure(struct update_data *ud)\n> +{\n> +\tint subforce = is_null_oid(&ud->suboid) || ud->force;\n> +\n> +\tif (!ud->nofetch) {\n> +\t\t/*\n> +\t\t * Run fetch only if `oid` isn't present or it\n> +\t\t * is not reachable from a ref.\n> +\t\t */\n> +\t\tif (!is_tip_reachable(ud->sm_path, &ud->oid))\n> +\t\t\tif (fetch_in_submodule(ud->sm_path, ud->depth, ud->quiet, NULL) &&\n> +\t\t\t !ud->quiet)\n> +\t\t\t\tfprintf_ln(stderr,\n\nOK. Combining these into a single statement like\n\n\t\tif (!is_tip_reachable(...) &&\n\t\t fetch_in_submodule(...) &&\n\t\t !ud->quiet)\n\t\t\tfprintf_ln(...\n\nwould reduce the indentation level, but the way the conditional is\nstructured may convey the flow of the thought better, i.e.\n\n\tif we need to fetch, \n\t try to fetch and if that fails,\n\t\treport failure.\n\nOn the other hand, if we take that line of thought to the extreme,\nthe check for !ud->quiet should belong to another level of if\nstatement so perhaps the more concise version I showed above might\nbe an overall win. I dunno.\n\n> +\t\t\t\t\t _(\"Unable to fetch in submodule path '%s'; \"\n> +\t\t\t\t\t \"trying to directly fetch %s:\"),\n> +\t\t\t\t\t ud->displaypath, oid_to_hex(&ud->oid));\n> +\t\t/*\n> +\t\t * Now we tried the usual fetch, but `oid` may\n> +\t\t * not be reachable from any of the refs.\n> +\t\t */\n> +\t\tif (!is_tip_reachable(ud->sm_path, &ud->oid))\n> +\t\t\tif (fetch_in_submodule(ud->sm_path, ud->depth, ud->quiet, &ud->oid))\n> +\t\t\t\tdie(_(\"Fetched in submodule path '%s', but it did not \"\n> +\t\t\t\t \"contain %s. Direct fetching of that commit failed.\"),\n> +\t\t\t\t ud->displaypath, oid_to_hex(&ud->oid));\n\nLikewise.\n\n> +\t}\n> +\n> +\treturn run_update_command(ud, subforce);\n> +}\n> +\n> static void update_submodule(struct update_clone_data *ucd)\n> {\n> \tfprintf(stdout, \"dummy %s %d\\t%s\\n\",\n> @@ -2395,6 +2584,75 @@ static int update_clone(int argc, const char **argv, const char *prefix)\n> \treturn update_submodules(&suc);\n> }\n> \n> +static int run_update_procedure(int argc, const char **argv, const char *prefix)\n> +{\n> +\tint force = 0, quiet = 0, nofetch = 0, just_cloned = 0;\n> +\tchar *prefixed_path, *update = NULL;\n> +\tstruct update_data update_data = UPDATE_DATA_INIT;\n> +\n> +\tstruct option options[] = {\n> ...\n> +\t\tOPT_END()\n> +\t};\n> ...\n> +\tupdate_data.displaypath = get_submodule_displaypath(prefixed_path, prefix);\n> +\n> +\tdetermine_submodule_update_strategy(the_repository, update_data.just_cloned,\n> +\t\t\t\t\t update_data.sm_path, update,\n> +\t\t\t\t\t &update_data.update_strategy);\n> +\n> +\tfree(prefixed_path);\n> +\n> +\tif ((!is_null_oid(&update_data.oid) && !is_null_oid(&update_data.suboid) &&\n> +\t !oideq(&update_data.oid, &update_data.suboid)) ||\n> +\t is_null_oid(&update_data.suboid) || update_data.force)\n> +\t\treturn do_run_update_procedure(&update_data);\n\nThe original does the update procedure if $sha1 and $subsha1 are\ndifferent or if $force option is given. The rewritten seems to skip\nthe update when .oid is NULL and .suboid is not NULL; intended?\n\nI understand that the division of labour between this function and\ndo_run_update_procedure() is for the former to only exist to\ninterface with the script side by populating the update_data\nstructure, and the latter implements the logic to run update\nprocedure. I was a bit surprised that this conditional is\nhere, not at the very beginning of the callee.\n\n> +\treturn 3;\n> +}\n> +\n> static int resolve_relative_path(int argc, const char **argv, const char *prefix)\n> {\n> \tstruct strbuf sb = STRBUF_INIT;\n> @@ -2951,6 +3209,7 @@ static struct cmd_struct commands[] = {\n> \t{\"add-clone\", add_clone, 0},\n> \t{\"update-module-mode\", module_update_module_mode, 0},\n> \t{\"update-clone\", update_clone, 0},\n> +\t{\"run-update-procedure\", run_update_procedure, 0},\n> \t{\"ensure-core-worktree\", ensure_core_worktree, 0},\n> \t{\"relative-path\", resolve_relative_path, 0},\n> \t{\"resolve-relative-url\", resolve_relative_url, 0},\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index dbd2ec2050..d8e30d1afa 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -519,14 +512,13 @@ cmd_update()\n> \n> \t\tgit submodule--helper ensure-core-worktree \"$sm_path\" || exit 1\n> \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\nOn the other side of the API boundary, update_data.displaypath is\npopulated by value computed in C. It is a bit unfortunate that we\nstill need to compute it here and risk the two to drift apart.\n\n> \t\tif test $just_cloned -eq 1\n> \t\tthen\n> \t\t\tsubsha1=\n> \t\telse\n> +\t\t\tjust_cloned=\n> \t\t\tsubsha1=$(sanitize_submodule_env; cd \"$sm_path\" &&\n> \t\t\t\tgit rev-parse --verify HEAD) ||\n> \t\t\tdie \"fatal: $(eval_gettext \"Unable to find current revision in submodule path '\\$displaypath'\")\"\n> @@ -547,70 +539,38 @@ cmd_update()\n> \t\t\tdie \"fatal: $(eval_gettext \"Unable to find current \\${remote_name}/\\${branch} revision in submodule path '\\$sm_path'\")\"\n> \t\tfi\n> \n> -\t\tif test \"$subsha1\" != \"$sha1\" || test -n \"$force\"\n> -\t\tthen\n> -\t\t\tsubforce=$force\n> -\t\t\t# If we don't already have a -f flag and the submodule has never been checked out\n> -\t\t\tif test -z \"$subsha1\" && test -z \"$force\"\n> -\t\t\tthen\n> -\t\t\t\tsubforce=\"-f\"\n> -\t\t\tfi\n> +\t\tout=$(git submodule--helper run-update-procedure \\\n> +\t\t\t ${wt_prefix:+--prefix \"$wt_prefix\"} \\\n> +\t\t\t ${GIT_QUIET:+--quiet} \\\n> +\t\t\t ${force:+--force} \\\n> +\t\t\t ${just_cloned:+--just-cloned} \\\n> +\t\t\t ${nofetch:+--no-fetch} \\\n> +\t\t\t ${depth:+\"$depth\"} \\\n> +\t\t\t ${update:+--update \"$update\"} \\\n> +\t\t\t ${prefix:+--recursive-prefix \"$prefix\"} \\\n> +\t\t\t ${sha1:+--oid \"$sha1\"} \\\n> +\t\t\t ${subsha1:+--suboid \"$subsha1\"} \\\n> +\t\t\t \"--\" \\\n> +\t\t\t \"$sm_path\")\n\nWe'd just show errors directly to the standard error stream from\nsubmodule--helper, but what comes from the printf in the switch\nstatement at the end of run_update_command() is captured in $out\nvariable. Notably, the messages from die()s in the second switch\nstatement in run_update_command() are not captured in $out here.\n\n> -\t\t\tif test -z \"$nofetch\"\n> -\t\t\tthen\n> -\t\t\t\t# Run fetch only if $sha1 isn't present or it\n> -\t\t\t\t# is not reachable from a ref.\n> -\t\t\t\tis_tip_reachable \"$sm_path\" \"$sha1\" ||\n> -\t\t\t\tfetch_in_submodule \"$sm_path\" $depth ||\n> -\t\t\t\tsay \"$(eval_gettext \"Unable to fetch in submodule path '\\$displaypath'; trying to directly fetch \\$sha1:\")\"\n> -\n> -\t\t\t\t# Now we tried the usual fetch, but $sha1 may\n> -\t\t\t\t# not be reachable from any of the refs\n> -\t\t\t\tis_tip_reachable \"$sm_path\" \"$sha1\" ||\n> -\t\t\t\tfetch_in_submodule \"$sm_path\" \"$depth\" \"$sha1\" ||\n> -\t\t\t\tdie \"fatal: $(eval_gettext \"Fetched in submodule path '\\$displaypath', but it did not contain \\$sha1. Direct fetching of that commit failed.\")\"\n> -\t\t\tfi\n> -\n> -\t\t\tmust_die_on_failure=\n> -\t\t\tcase \"$update_module\" in\n> -\t\t\tcheckout)\n> -\t\t\t\tcommand=\"git checkout $subforce -q\"\n> -\t\t\t\tdie_msg=\"fatal: $(eval_gettext \"Unable to checkout '\\$sha1' in submodule path '\\$displaypath'\")\"\n> -\t\t\t\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': checked out '\\$sha1'\")\"\n> -\t\t\t\t;;\n> -\t\t\trebase)\n> -\t\t\t\tcommand=\"git rebase ${GIT_QUIET:+--quiet}\"\n> -\t\t\t\tdie_msg=\"fatal: $(eval_gettext \"Unable to rebase '\\$sha1' in submodule path '\\$displaypath'\")\"\n> -\t\t\t\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': rebased into '\\$sha1'\")\"\n> -\t\t\t\tmust_die_on_failure=yes\n> -\t\t\t\t;;\n> -\t\t\tmerge)\n> -\t\t\t\tcommand=\"git merge ${GIT_QUIET:+--quiet}\"\n> -\t\t\t\tdie_msg=\"fatal: $(eval_gettext \"Unable to merge '\\$sha1' in submodule path '\\$displaypath'\")\"\n> -\t\t\t\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': merged in '\\$sha1'\")\"\n> -\t\t\t\tmust_die_on_failure=yes\n> -\t\t\t\t;;\n> -\t\t\t!*)\n> -\t\t\t\tcommand=\"${update_module#!}\"\n> -\t\t\t\tdie_msg=\"fatal: $(eval_gettext \"Execution of '\\$command \\$sha1' failed in submodule path '\\$displaypath'\")\"\n> -\t\t\t\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': '\\$command \\$sha1'\")\"\n> -\t\t\t\tmust_die_on_failure=yes\n> -\t\t\t\t;;\n> -\t\t\t*)\n> -\t\t\t\tdie \"fatal: $(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> -\t\t\tthen\n> -\t\t\t\tsay \"$say_msg\"\n> -\t\t\telif test -n \"$must_die_on_failure\"\n> -\t\t\tthen\n> -\t\t\t\tdie_with_status 2 \"$die_msg\"\n> -\t\t\telse\n> -\t\t\t\terr=\"${err};$die_msg\"\n> -\t\t\t\tcontinue\n> -\t\t\tfi\n> -\t\tfi\n> +\t\t# exit codes for run-update-procedure:\n> +\t\t# 0: update was successful, say command output\n> +\t\t# 128: subcommand died during execution\n> +\t\t# 1: update procedure failed, but should not die\n> +\t\t# 3: no update procedure was run\n> +\t\tres=\"$?\"\n> +\t\tcase $res in\n> +\t\t0)\n> +\t\t\tsay \"$out\"\n> +\t\t\t;;\n\nAnd the case where there is no error is quite straight-forward. We\njust emit what we saw in the standard output stream of the helper.\n\n> +\t\t128)\n> +\t\t\texit $res\n> +\t\t\t;;\n> +\t\t1)\n> +\t\t\terr=\"${err};$out\"\n\nThis part is dubious. In the original, $err accumulates what is in\n$die_msg, which are things like \"fatal: Unable to rebase ...\", but\nwith this patch, what used to be the contents of $die_msg are given\nto die() after we see run_command() fail, and would have sent to the\nstandard error stream, not captured in $out here, no?\n\n> +\t\t\tcontinue\n> +\t\t\t;;\n> +\t\tesac\n> \n> \t\tif test -n \"$recursive\"\n> \t\tthen\n\n\nThanks.\n"},{"id":"433522","messageId":"m2r1ejdxb0.fsf@gmail.com","threadId":"56147","inReplyTo":"xmqqim09w5yc.fsf@gitster.g","subject":"Re: [GSoC] [PATCH v3] submodule--helper: run update procedures from C","fromName":"Atharva Raykar","fromEmail":"raykar.ath@gmail.com","sentAt":"2021-08-24T08:58:22Z","receivedAt":"2021-08-24T09:42:50Z","isPatch":true,"sender":{"key":"raykar.ath@gmail.com","avatar":"https://avatars.githubusercontent.com/u/24277692?v=4"},"body":"Pardon my late response, I had been occupied with other things.\n\nJunio C Hamano writes:\n\n> Atharva Raykar writes:\n> [...]\n>> Signed-off-by: Atharva Raykar \n>> Mentored-by: Christian Couder \n>> Mentored-by: Shourya Shukla \n>\n> Keep trailer lines in chronological order. The mentors mentored,\n> the patch was written and finally you signed it off.\n\nOkay.\n\n>> +static int run_update_command(struct update_data *ud, int subforce)\n>> +{\n>> +\tstruct child_process cp = CHILD_PROCESS_INIT;\n>> +\tchar *oid = oid_to_hex(&ud->oid);\n>> +\tint must_die_on_failure = 0;\n>> +\n>> +\tcp.dir = xstrdup(ud->sm_path);\n>> +\tswitch (ud->update_strategy.type) {\n>> +\tcase SM_UPDATE_CHECKOUT:\n>> +\t\tcp.git_cmd = 1;\n>> +\t\tstrvec_pushl(&cp.args, \"checkout\", \"-q\", NULL);\n>> +\t\tif (subforce)\n>> +\t\t\tstrvec_push(&cp.args, \"-f\");\n>> +\t\tbreak;\n>> +\tcase SM_UPDATE_REBASE:\n>> +\t\tcp.git_cmd = 1;\n>> +\t\tstrvec_push(&cp.args, \"rebase\");\n>> +\t\tif (ud->quiet)\n>> +\t\t\tstrvec_push(&cp.args, \"--quiet\");\n>> +\t\tmust_die_on_failure = 1;\n>> +\t\tbreak;\n>> +\tcase SM_UPDATE_MERGE:\n>> +\t\tcp.git_cmd = 1;\n>> +\t\tstrvec_push(&cp.args, \"merge\");\n>> +\t\tif (ud->quiet)\n>> +\t\t\tstrvec_push(&cp.args, \"--quiet\");\n>> +\t\tmust_die_on_failure = 1;\n>> +\t\tbreak;\n>> +\tcase SM_UPDATE_COMMAND:\n>> +\t\t/* NOTE: this does not handle quoted arguments */\n>> +\t\tstrvec_split(&cp.args, ud->update_strategy.command);\n>\n> Indeed this doesn't. I think cp.use_shell would be the right way to\n> run this.\n>\n> Study what happens before run_command_v_opt_cd_env() with\n> RUN_USING_SHELL calls run_command() and make something similar\n> happen here, instead of doing a manual command line splitting.\n>\n> \tSide note: run_command_v_opt_cd_env() with RUN_USING_SHELL\n> \tis used in places like diff.c::run_external_diff() to invoke\n> \tan external diff command, ll-merge.c::ll_ext_merge() to\n> \tinvoke a user-defined low level merge driver,\n> \tsequencer.c::do_exec() to invoke 'x cmd' you write in the\n> \ttodo list during an \"rebase -i\" session.\n\nThanks for the pointers, the details helped. I'll handle this more\ncorrectly in the next version.\n\n>> +\t\tmust_die_on_failure = 1;\n>> +\t\tbreak;\n>> +\tcase SM_UPDATE_NONE:\n>> +\t\tBUG(\"this should have been handled before. How did we reach here?\");\n>> +\t\tbreak;\n>> +\tcase SM_UPDATE_UNSPECIFIED:\n>> +\t\tBUG(\"update strategy should have been specified\");\n>\n> These two case arms are not a faithful conversion from the original,\n> but because you do not carry around a random string from the caller\n> and instead have parsed enums, it cannot be ;-) But it makes me\n> wonder why we want these two cases separate. Isn't it a BUG() if\n> anything other than what we handled (i.e. prepared cp.args for)\n> already is in ud->update_strategy.type? IOW, wouldn't it be more\n> forward looking to do\n>\n> \tdefault:\n> \t\tBUG(\"unexpected ud->update_strategy.type (%d)\",\n> \t\t ud->update_strategy.type (%d)\");\n>\n> or something? That way, if we ever come up with a new update\n> strategy and forget to update this part, we will catch such a bug\n> fairly quickly.\n\nThe original intention for separating the cases was to differentiate the\ncause for the invalid state, but your proposed suggestion is a lot\nbetter. I'll address this.\n\n>> +\t}\n>> +\n>> +\tstrvec_push(&cp.args, oid);\n>> +\n>> +\tprepare_submodule_repo_env(&cp.env_array);\n>> +\n>> +\tif (run_command(&cp)) {\n>> +\t\tif (must_die_on_failure) {\n>> +\t\t\tswitch (ud->update_strategy.type) {\n>> +\t\t\tcase SM_UPDATE_CHECKOUT:\n>> +\t\t\t\tdie(_(\"Unable to checkout '%s' in submodule path '%s'\"),\n>> +\t\t\t\t oid, ud->displaypath);\n>> +\t\t\t\tbreak;\n>> +\t\t\tcase SM_UPDATE_REBASE:\n>> +\t\t\t\tdie(_(\"Unable to rebase '%s' in submodule path '%s'\"),\n>> +\t\t\t\t oid, ud->displaypath);\n>> +\t\t\t\tbreak;\n>> +\t\t\tcase SM_UPDATE_MERGE:\n>> +\t\t\t\tdie(_(\"Unable to merge '%s' in submodule path '%s'\"),\n>> +\t\t\t\t oid, ud->displaypath);\n>> +\t\t\t\tbreak;\n>> +\t\t\tcase SM_UPDATE_COMMAND:\n>> +\t\t\t\tdie(_(\"Execution of '%s %s' failed in submodule path '%s'\"),\n>> +\t\t\t\t ud->update_strategy.command, oid, ud->displaypath);\n>> +\t\t\t\tbreak;\n>\n> The messages here correspond to what is assigned to $die_message in\n> the original. Note that they are emitted to the standard error\n> stream.\n>\n> I suspect that these should be \"printf()\" followed by a call to\n> exit() with some non-zero value (see below).\n\nI also notice another major lapse in conversion. In the shell porcelain,\nthe \"checkout\" mode should not die out at all, instead it should print\nout the error message.\n\nMy code tries to die() on the checkout mode (in a case arm that will\nnever be activated), and does not ever print the checkout failure\nmessage at all. I will fix this in the re-roll.\n\n(Will address more of this below...)\n\n>> +\t\t\tcase SM_UPDATE_NONE:\n>> +\t\t\t\tBUG(\"this should have been handled before. How did we reach here?\");\n>> +\t\t\t\tbreak;\n>> +\t\t\tcase SM_UPDATE_UNSPECIFIED:\n>> +\t\t\t\tBUG(\"update strategy should have been specified\");\n>> +\t\t\t}\n>\n> The same comment applies to the last two case arms of this switch\n> statement and the next one, too. I think we just should catch\n> \"everything else\" with a simple \"default:\" label.\n>\n> Also, don't omit the \"break;\" from the last case arm in a switch\n> statement. It harms the long-term help of the code---the last case\n> arm may not forever stay to be the last one.\n>\n>> +\t\t}\n>> +\t\t/*\n>> +\t\t * This signifies to the caller in shell that\n>> +\t\t * the command failed without dying\n>> +\t\t */\n>> +\t\treturn 1;\n>> +\t}\n>> +\n>> +\tswitch (ud->update_strategy.type) {\n>> +\tcase SM_UPDATE_CHECKOUT:\n>> +\t\tprintf(_(\"Submodule path '%s': checked out '%s'\\n\"),\n>> +\t\t ud->displaypath, oid);\n>> +\t\tbreak;\n>> +\tcase SM_UPDATE_REBASE:\n>> +\t\tprintf(_(\"Submodule path '%s': rebased into '%s'\\n\"),\n>> +\t\t ud->displaypath, oid);\n>> +\t\tbreak;\n>> +\tcase SM_UPDATE_MERGE:\n>> +\t\tprintf(_(\"Submodule path '%s': merged in '%s'\\n\"),\n>> +\t\t ud->displaypath, oid);\n>> +\t\tbreak;\n>> +\tcase SM_UPDATE_COMMAND:\n>> +\t\tprintf(_(\"Submodule path '%s': '%s %s'\\n\"),\n>> +\t\t ud->displaypath, ud->update_strategy.command, oid);\n>> +\t\tbreak;\n>> +\tcase SM_UPDATE_NONE:\n>> +\t\tBUG(\"this should have been handled before. How did we reach here?\");\n>> +\t\tbreak;\n>> +\tcase SM_UPDATE_UNSPECIFIED:\n>> +\t\tBUG(\"update strategy should have been specified\");\n>\n> Likewise here.\n\nOkay.\n\n>> +\t}\n>> +\n>> +\treturn 0;\n>> +}\n>> +\n>> +static int do_run_update_procedure(struct update_data *ud)\n>> +{\n>> +\tint subforce = is_null_oid(&ud->suboid) || ud->force;\n>> +\n>> +\tif (!ud->nofetch) {\n>> +\t\t/*\n>> +\t\t * Run fetch only if `oid` isn't present or it\n>> +\t\t * is not reachable from a ref.\n>> +\t\t */\n>> +\t\tif (!is_tip_reachable(ud->sm_path, &ud->oid))\n>> +\t\t\tif (fetch_in_submodule(ud->sm_path, ud->depth, ud->quiet, NULL) &&\n>> +\t\t\t !ud->quiet)\n>> +\t\t\t\tfprintf_ln(stderr,\n>\n> OK. Combining these into a single statement like\n>\n> \t\tif (!is_tip_reachable(...) &&\n> \t\t fetch_in_submodule(...) &&\n> \t\t !ud->quiet)\n> \t\t\tfprintf_ln(...\n>\n> would reduce the indentation level, but the way the conditional is\n> structured may convey the flow of the thought better, i.e.\n>\n> \tif we need to fetch,\n> \t try to fetch and if that fails,\n> \t\treport failure.\n>\n> On the other hand, if we take that line of thought to the extreme,\n> the check for !ud->quiet should belong to another level of if\n> statement so perhaps the more concise version I showed above might\n> be an overall win. I dunno.\n\nRight. I agree with you, mainly because there are many other predicates\nin my previous conversions that were done with short-circuited &&'s, so\nmight as well stick to what has been my convention.\n\n>> +\t\t\t\t\t _(\"Unable to fetch in submodule path '%s'; \"\n>> +\t\t\t\t\t \"trying to directly fetch %s:\"),\n>> +\t\t\t\t\t ud->displaypath, oid_to_hex(&ud->oid));\n>> +\t\t/*\n>> +\t\t * Now we tried the usual fetch, but `oid` may\n>> +\t\t * not be reachable from any of the refs.\n>> +\t\t */\n>> +\t\tif (!is_tip_reachable(ud->sm_path, &ud->oid))\n>> +\t\t\tif (fetch_in_submodule(ud->sm_path, ud->depth, ud->quiet, &ud->oid))\n>> +\t\t\t\tdie(_(\"Fetched in submodule path '%s', but it did not \"\n>> +\t\t\t\t \"contain %s. Direct fetching of that commit failed.\"),\n>> +\t\t\t\t ud->displaypath, oid_to_hex(&ud->oid));\n>\n> Likewise.\n>\n>> +\t}\n>> [...]\n>> +\n>> +\tif ((!is_null_oid(&update_data.oid) && !is_null_oid(&update_data.suboid) &&\n>> +\t !oideq(&update_data.oid, &update_data.suboid)) ||\n>> +\t is_null_oid(&update_data.suboid) || update_data.force)\n>> +\t\treturn do_run_update_procedure(&update_data);\n>\n> The original does the update procedure if $sha1 and $subsha1 are\n> different or if $force option is given. The rewritten seems to skip\n> the update when .oid is NULL and .suboid is not NULL; intended?\n\nUnintended. I initially implemented this with raw chars until I\ndiscovered the object_id API. So this was the result of me\nindiscriminately substituting NULL checks with the OID equivalents. I\nrealise this is not needed anymore, and we can simplify that to:\n\n if (!oideq(&update_data.oid, &update_data.suboid) || update_data.force)\n\n> I understand that the division of labour between this function and\n> do_run_update_procedure() is for the former to only exist to\n> interface with the script side by populating the update_data\n> structure, and the latter implements the logic to run update\n> procedure. I was a bit surprised that this conditional is\n> here, not at the very beginning of the callee.\n\nÆvar pointed out that since this function just had one caller, I could\nmove the whole 'if' outside it for now, which would save me one level of\nindentation within that function, and make it easier to parse. With the\nconditional being simplified to what I showed above, I think it can\nstill be justified?\n\nEven in the series that will follow this, we would still have only one\ncaller for this function.\n\n>> +\treturn 3;\n>> +}\n>> +\n>> static int resolve_relative_path(int argc, const char **argv, const char *prefix)\n>> {\n>> \tstruct strbuf sb = STRBUF_INIT;\n>> @@ -2951,6 +3209,7 @@ static struct cmd_struct commands[] = {\n>> \t{\"add-clone\", add_clone, 0},\n>> \t{\"update-module-mode\", module_update_module_mode, 0},\n>> \t{\"update-clone\", update_clone, 0},\n>> +\t{\"run-update-procedure\", run_update_procedure, 0},\n>> \t{\"ensure-core-worktree\", ensure_core_worktree, 0},\n>> \t{\"relative-path\", resolve_relative_path, 0},\n>> \t{\"resolve-relative-url\", resolve_relative_url, 0},\n>> diff --git a/git-submodule.sh b/git-submodule.sh\n>> index dbd2ec2050..d8e30d1afa 100755\n>> --- a/git-submodule.sh\n>> +++ b/git-submodule.sh\n>> @@ -519,14 +512,13 @@ cmd_update()\n>>\n>> \t\tgit submodule--helper ensure-core-worktree \"$sm_path\" || exit 1\n>>\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> On the other side of the API boundary, update_data.displaypath is\n> populated by value computed in C. It is a bit unfortunate that we\n> still need to compute it here and risk the two to drift apart.\n\nYes, and I had some trouble figuring out a clean separation boundary,\nand this compromise felt like the best one. The best I can do is assure\nyou that the patch series following this change solves this issue\nentirely, as it moves all of the shell code you see here into the C\nhelper, and thus we only compute this value once.\n\nSo there should be no worry about drift, as I will not give any chance\nto introduce it at all :)\n\n>> \t\tif test $just_cloned -eq 1\n>> \t\tthen\n>> \t\t\tsubsha1=\n>> \t\telse\n>> +\t\t\tjust_cloned=\n>> \t\t\tsubsha1=$(sanitize_submodule_env; cd \"$sm_path\" &&\n>> \t\t\t\tgit rev-parse --verify HEAD) ||\n>> \t\t\tdie \"fatal: $(eval_gettext \"Unable to find current revision in submodule path '\\$displaypath'\")\"\n>> @@ -547,70 +539,38 @@ cmd_update()\n>> \t\t\tdie \"fatal: $(eval_gettext \"Unable to find current \\${remote_name}/\\${branch} revision in submodule path '\\$sm_path'\")\"\n>> \t\tfi\n>>\n>> -\t\tif test \"$subsha1\" != \"$sha1\" || test -n \"$force\"\n>> -\t\tthen\n>> -\t\t\tsubforce=$force\n>> -\t\t\t# If we don't already have a -f flag and the submodule has never been checked out\n>> -\t\t\tif test -z \"$subsha1\" && test -z \"$force\"\n>> -\t\t\tthen\n>> -\t\t\t\tsubforce=\"-f\"\n>> -\t\t\tfi\n>> +\t\tout=$(git submodule--helper run-update-procedure \\\n>> +\t\t\t ${wt_prefix:+--prefix \"$wt_prefix\"} \\\n>> +\t\t\t ${GIT_QUIET:+--quiet} \\\n>> +\t\t\t ${force:+--force} \\\n>> +\t\t\t ${just_cloned:+--just-cloned} \\\n>> +\t\t\t ${nofetch:+--no-fetch} \\\n>> +\t\t\t ${depth:+\"$depth\"} \\\n>> +\t\t\t ${update:+--update \"$update\"} \\\n>> +\t\t\t ${prefix:+--recursive-prefix \"$prefix\"} \\\n>> +\t\t\t ${sha1:+--oid \"$sha1\"} \\\n>> +\t\t\t ${subsha1:+--suboid \"$subsha1\"} \\\n>> +\t\t\t \"--\" \\\n>> +\t\t\t \"$sm_path\")\n>\n> We'd just show errors directly to the standard error stream from\n> submodule--helper, but what comes from the printf in the switch\n> statement at the end of run_update_command() is captured in $out\n> variable. Notably, the messages from die()s in the second switch\n> statement in run_update_command() are not captured in $out here.\n>\n>> -\t\t\tif test -z \"$nofetch\"\n>> -\t\t\tthen\n>> -\t\t\t\t# Run fetch only if $sha1 isn't present or it\n>> -\t\t\t\t# is not reachable from a ref.\n>> -\t\t\t\tis_tip_reachable \"$sm_path\" \"$sha1\" ||\n>> -\t\t\t\tfetch_in_submodule \"$sm_path\" $depth ||\n>> -\t\t\t\tsay \"$(eval_gettext \"Unable to fetch in submodule path '\\$displaypath'; trying to directly fetch \\$sha1:\")\"\n>> -\n>> -\t\t\t\t# Now we tried the usual fetch, but $sha1 may\n>> -\t\t\t\t# not be reachable from any of the refs\n>> -\t\t\t\tis_tip_reachable \"$sm_path\" \"$sha1\" ||\n>> -\t\t\t\tfetch_in_submodule \"$sm_path\" \"$depth\" \"$sha1\" ||\n>> -\t\t\t\tdie \"fatal: $(eval_gettext \"Fetched in submodule path '\\$displaypath', but it did not contain \\$sha1. Direct fetching of that commit failed.\")\"\n>> -\t\t\tfi\n>> -\n>> -\t\t\tmust_die_on_failure=\n>> -\t\t\tcase \"$update_module\" in\n>> -\t\t\tcheckout)\n>> -\t\t\t\tcommand=\"git checkout $subforce -q\"\n>> -\t\t\t\tdie_msg=\"fatal: $(eval_gettext \"Unable to checkout '\\$sha1' in submodule path '\\$displaypath'\")\"\n>> -\t\t\t\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': checked out '\\$sha1'\")\"\n>> -\t\t\t\t;;\n>> -\t\t\trebase)\n>> -\t\t\t\tcommand=\"git rebase ${GIT_QUIET:+--quiet}\"\n>> -\t\t\t\tdie_msg=\"fatal: $(eval_gettext \"Unable to rebase '\\$sha1' in submodule path '\\$displaypath'\")\"\n>> -\t\t\t\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': rebased into '\\$sha1'\")\"\n>> -\t\t\t\tmust_die_on_failure=yes\n>> -\t\t\t\t;;\n>> -\t\t\tmerge)\n>> -\t\t\t\tcommand=\"git merge ${GIT_QUIET:+--quiet}\"\n>> -\t\t\t\tdie_msg=\"fatal: $(eval_gettext \"Unable to merge '\\$sha1' in submodule path '\\$displaypath'\")\"\n>> -\t\t\t\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': merged in '\\$sha1'\")\"\n>> -\t\t\t\tmust_die_on_failure=yes\n>> -\t\t\t\t;;\n>> -\t\t\t!*)\n>> -\t\t\t\tcommand=\"${update_module#!}\"\n>> -\t\t\t\tdie_msg=\"fatal: $(eval_gettext \"Execution of '\\$command \\$sha1' failed in submodule path '\\$displaypath'\")\"\n>> -\t\t\t\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': '\\$command \\$sha1'\")\"\n>> -\t\t\t\tmust_die_on_failure=yes\n>> -\t\t\t\t;;\n>> -\t\t\t*)\n>> -\t\t\t\tdie \"fatal: $(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>> -\t\t\tthen\n>> -\t\t\t\tsay \"$say_msg\"\n>> -\t\t\telif test -n \"$must_die_on_failure\"\n>> -\t\t\tthen\n>> -\t\t\t\tdie_with_status 2 \"$die_msg\"\n>> -\t\t\telse\n>> -\t\t\t\terr=\"${err};$die_msg\"\n>> -\t\t\t\tcontinue\n>> -\t\t\tfi\n>> -\t\tfi\n>> +\t\t# exit codes for run-update-procedure:\n>> +\t\t# 0: update was successful, say command output\n>> +\t\t# 128: subcommand died during execution\n>> +\t\t# 1: update procedure failed, but should not die\n>> +\t\t# 3: no update procedure was run\n>> +\t\tres=\"$?\"\n>> +\t\tcase $res in\n>> +\t\t0)\n>> +\t\t\tsay \"$out\"\n>> +\t\t\t;;\n>\n> And the case where there is no error is quite straight-forward. We\n> just emit what we saw in the standard output stream of the helper.\n>\n>> +\t\t128)\n>> +\t\t\texit $res\n>> +\t\t\t;;\n>> +\t\t1)\n>> +\t\t\terr=\"${err};$out\"\n>\n> This part is dubious. In the original, $err accumulates what is in\n> $die_msg, which are things like \"fatal: Unable to rebase ...\", but\n> with this patch, what used to be the contents of $die_msg are given\n> to die() after we see run_command() fail, and would have sent to the\n> standard error stream, not captured in $out here, no?\n\nYes, this is bad. This error slipped past the test suite that I was too\nreliant on. Even if it did work, it would not have printed the error\nmessages for the \"checkout\" mode. I will fix all of these issues.\nPrinting to stdout with error return ought to fix it, as you suggested.\n\n>> +\t\t\tcontinue\n>> +\t\t\t;;\n>> +\t\tesac\n>>\n>> \t\tif test -n \"$recursive\"\n>> \t\tthen\n>\n>\n> Thanks.\n\nThanks for the thorough review.\n"},{"id":"433554","messageId":"20210824140609.1496-1-raykar.ath@gmail.com","threadId":"56147","inReplyTo":"20210813075653.56817-1-raykar.ath@gmail.com","subject":"[PATCH v4] submodule--helper: run update procedures from C","fromName":"Atharva Raykar","fromEmail":"raykar.ath@gmail.com","sentAt":"2021-08-24T14:06:09Z","receivedAt":"2021-08-24T14:06:26Z","isPatch":true,"sender":{"key":"raykar.ath@gmail.com","avatar":"https://avatars.githubusercontent.com/u/24277692?v=4"},"body":"Add a new submodule--helper subcommand `run-update-procedure` that runs\nthe update procedure if the SHA1 of the submodule does not match what\nthe superproject expects.\n\nThis is an intermediate change that works towards total conversion of\n`submodule update` from shell to C.\n\nSpecific error codes are returned so that the shell script calling the\nsubcommand can take a decision on the control flow, and preserve the\nerror messages across subsequent recursive calls of `cmd_update`.\n\nThis change is more focused on doing a faithful conversion, so for now we\nare not too concerned with trying to reduce subprocess spawns.\n\nMentored-by: Christian Couder \nMentored-by: Shourya Shukla \nSigned-off-by: Atharva Raykar \n---\n\nThis patch addresses the concerns raised by Junio. The important changes are:\n\n* Fix error message handling of the output of 'run-update-procedure'. While at\n it, ensure the \"checkout\" mode error message is stored and printed\n appropriately.\n\n* In 'run_update_command()' switch from 'run_command()' to\n 'run_command_v_opt_cd_env()' to ensure quoted command update modes are handled\n correctly.\n\n* Code style and hygiene changes.\n\n* Introduce a NEEDSWORK comment, because the printf() and error return is\n correct only because the shell caller in the other end redirects it to the\n correct output stream. Once we switch this completely to C (ie, in the\n follow-up series), I need to remember to die() instead (or print to stderr) to\n reproduce the original behaviour.\n\nRange-diff against v3:\n1: 2ff48f8790 ! 1: 2729834e43 submodule--helper: run update procedures from C\n @@ Commit message\n This change is more focused on doing a faithful conversion, so for now we\n are not too concerned with trying to reduce subprocess spawns.\n \n - Signed-off-by: Atharva Raykar \n Mentored-by: Christian Couder \n Mentored-by: Shourya Shukla \n + Signed-off-by: Atharva Raykar \n \n ## builtin/submodule--helper.c ##\n @@ builtin/submodule--helper.c: struct submodule_update_clone {\n @@ builtin/submodule--helper.c: static int git_update_clone_config(const char *var,\n +\n +static int run_update_command(struct update_data *ud, int subforce)\n +{\n -+\tstruct child_process cp = CHILD_PROCESS_INIT;\n ++\tstruct strvec args = STRVEC_INIT;\n ++\tstruct strvec child_env = STRVEC_INIT;\n +\tchar *oid = oid_to_hex(&ud->oid);\n +\tint must_die_on_failure = 0;\n ++\tint git_cmd;\n +\n -+\tcp.dir = xstrdup(ud->sm_path);\n +\tswitch (ud->update_strategy.type) {\n +\tcase SM_UPDATE_CHECKOUT:\n -+\t\tcp.git_cmd = 1;\n -+\t\tstrvec_pushl(&cp.args, \"checkout\", \"-q\", NULL);\n ++\t\tgit_cmd = 1;\n ++\t\tstrvec_pushl(&args, \"checkout\", \"-q\", NULL);\n +\t\tif (subforce)\n -+\t\t\tstrvec_push(&cp.args, \"-f\");\n ++\t\t\tstrvec_push(&args, \"-f\");\n +\t\tbreak;\n +\tcase SM_UPDATE_REBASE:\n -+\t\tcp.git_cmd = 1;\n -+\t\tstrvec_push(&cp.args, \"rebase\");\n ++\t\tgit_cmd = 1;\n ++\t\tstrvec_push(&args, \"rebase\");\n +\t\tif (ud->quiet)\n -+\t\t\tstrvec_push(&cp.args, \"--quiet\");\n ++\t\t\tstrvec_push(&args, \"--quiet\");\n +\t\tmust_die_on_failure = 1;\n +\t\tbreak;\n +\tcase SM_UPDATE_MERGE:\n -+\t\tcp.git_cmd = 1;\n -+\t\tstrvec_push(&cp.args, \"merge\");\n ++\t\tgit_cmd = 1;\n ++\t\tstrvec_push(&args, \"merge\");\n +\t\tif (ud->quiet)\n -+\t\t\tstrvec_push(&cp.args, \"--quiet\");\n ++\t\t\tstrvec_push(&args, \"--quiet\");\n +\t\tmust_die_on_failure = 1;\n +\t\tbreak;\n +\tcase SM_UPDATE_COMMAND:\n -+\t\t/* NOTE: this does not handle quoted arguments */\n -+\t\tstrvec_split(&cp.args, ud->update_strategy.command);\n ++\t\tgit_cmd = 0;\n ++\t\tstrvec_push(&args, ud->update_strategy.command);\n +\t\tmust_die_on_failure = 1;\n +\t\tbreak;\n -+\tcase SM_UPDATE_NONE:\n -+\t\tBUG(\"this should have been handled before. How did we reach here?\");\n -+\t\tbreak;\n -+\tcase SM_UPDATE_UNSPECIFIED:\n -+\t\tBUG(\"update strategy should have been specified\");\n ++\tdefault:\n ++\t\tBUG(\"unexpected update strategy type: %s\",\n ++\t\t submodule_strategy_to_string(&ud->update_strategy));\n +\t}\n -+\n -+\tstrvec_push(&cp.args, oid);\n -+\n -+\tprepare_submodule_repo_env(&cp.env_array);\n -+\n -+\tif (run_command(&cp)) {\n -+\t\tif (must_die_on_failure) {\n -+\t\t\tswitch (ud->update_strategy.type) {\n -+\t\t\tcase SM_UPDATE_CHECKOUT:\n -+\t\t\t\tdie(_(\"Unable to checkout '%s' in submodule path '%s'\"),\n -+\t\t\t\t oid, ud->displaypath);\n -+\t\t\t\tbreak;\n -+\t\t\tcase SM_UPDATE_REBASE:\n -+\t\t\t\tdie(_(\"Unable to rebase '%s' in submodule path '%s'\"),\n -+\t\t\t\t oid, ud->displaypath);\n -+\t\t\t\tbreak;\n -+\t\t\tcase SM_UPDATE_MERGE:\n -+\t\t\t\tdie(_(\"Unable to merge '%s' in submodule path '%s'\"),\n -+\t\t\t\t oid, ud->displaypath);\n -+\t\t\t\tbreak;\n -+\t\t\tcase SM_UPDATE_COMMAND:\n -+\t\t\t\tdie(_(\"Execution of '%s %s' failed in submodule path '%s'\"),\n -+\t\t\t\t ud->update_strategy.command, oid, ud->displaypath);\n -+\t\t\t\tbreak;\n -+\t\t\tcase SM_UPDATE_NONE:\n -+\t\t\t\tBUG(\"this should have been handled before. How did we reach here?\");\n -+\t\t\t\tbreak;\n -+\t\t\tcase SM_UPDATE_UNSPECIFIED:\n -+\t\t\t\tBUG(\"update strategy should have been specified\");\n -+\t\t\t}\n ++\tstrvec_push(&args, oid);\n ++\n ++\tprepare_submodule_repo_env(&child_env);\n ++\tif (run_command_v_opt_cd_env(args.v, git_cmd ? RUN_GIT_CMD : RUN_USING_SHELL,\n ++\t\t\t\t ud->sm_path, child_env.v)) {\n ++\t\tswitch (ud->update_strategy.type) {\n ++\t\tcase SM_UPDATE_CHECKOUT:\n ++\t\t\tprintf(_(\"Unable to checkout '%s' in submodule path '%s'\"),\n ++\t\t\t oid, ud->displaypath);\n ++\t\t\tbreak;\n ++\t\tcase SM_UPDATE_REBASE:\n ++\t\t\tprintf(_(\"Unable to rebase '%s' in submodule path '%s'\"),\n ++\t\t\t oid, ud->displaypath);\n ++\t\t\tbreak;\n ++\t\tcase SM_UPDATE_MERGE:\n ++\t\t\tprintf(_(\"Unable to merge '%s' in submodule path '%s'\"),\n ++\t\t\t oid, ud->displaypath);\n ++\t\t\tbreak;\n ++\t\tcase SM_UPDATE_COMMAND:\n ++\t\t\tprintf(_(\"Execution of '%s %s' failed in submodule path '%s'\"),\n ++\t\t\t ud->update_strategy.command, oid, ud->displaypath);\n ++\t\t\tbreak;\n ++\t\tdefault:\n ++\t\t\tBUG(\"unexpected update strategy type: %s\",\n ++\t\t\t submodule_strategy_to_string(&ud->update_strategy));\n +\t\t}\n +\t\t/*\n -+\t\t * This signifies to the caller in shell that\n -+\t\t * the command failed without dying\n ++\t\t * NEEDSWORK: We are currently printing to stdout with error\n ++\t\t * return so that the shell caller handles the error output\n ++\t\t * properly. Once we start handling the error messages within\n ++\t\t * C, we should use die() instead.\n ++\t\t */\n ++\t\tif (must_die_on_failure)\n ++\t\t\treturn 2;\n ++\t\t/*\n ++\t\t * This signifies to the caller in shell that the command\n ++\t\t * failed without dying\n +\t\t */\n +\t\treturn 1;\n +\t}\n @@ builtin/submodule--helper.c: static int git_update_clone_config(const char *var,\n +\t\tprintf(_(\"Submodule path '%s': '%s %s'\\n\"),\n +\t\t ud->displaypath, ud->update_strategy.command, oid);\n +\t\tbreak;\n -+\tcase SM_UPDATE_NONE:\n -+\t\tBUG(\"this should have been handled before. How did we reach here?\");\n -+\t\tbreak;\n -+\tcase SM_UPDATE_UNSPECIFIED:\n -+\t\tBUG(\"update strategy should have been specified\");\n ++\tdefault:\n ++\t\tBUG(\"unexpected update strategy type: %s\",\n ++\t\t submodule_strategy_to_string(&ud->update_strategy));\n +\t}\n +\n +\treturn 0;\n @@ builtin/submodule--helper.c: static int git_update_clone_config(const char *var,\n +\t\t * Run fetch only if `oid` isn't present or it\n +\t\t * is not reachable from a ref.\n +\t\t */\n -+\t\tif (!is_tip_reachable(ud->sm_path, &ud->oid))\n -+\t\t\tif (fetch_in_submodule(ud->sm_path, ud->depth, ud->quiet, NULL) &&\n -+\t\t\t !ud->quiet)\n -+\t\t\t\tfprintf_ln(stderr,\n -+\t\t\t\t\t _(\"Unable to fetch in submodule path '%s'; \"\n -+\t\t\t\t\t \"trying to directly fetch %s:\"),\n -+\t\t\t\t\t ud->displaypath, oid_to_hex(&ud->oid));\n ++\t\tif (!is_tip_reachable(ud->sm_path, &ud->oid) &&\n ++\t\t fetch_in_submodule(ud->sm_path, ud->depth, ud->quiet, NULL) &&\n ++\t\t !ud->quiet)\n ++\t\t\tfprintf_ln(stderr,\n ++\t\t\t\t _(\"Unable to fetch in submodule path '%s'; \"\n ++\t\t\t\t \"trying to directly fetch %s:\"),\n ++\t\t\t\t ud->displaypath, oid_to_hex(&ud->oid));\n +\t\t/*\n +\t\t * Now we tried the usual fetch, but `oid` may\n +\t\t * not be reachable from any of the refs.\n +\t\t */\n -+\t\tif (!is_tip_reachable(ud->sm_path, &ud->oid))\n -+\t\t\tif (fetch_in_submodule(ud->sm_path, ud->depth, ud->quiet, &ud->oid))\n -+\t\t\t\tdie(_(\"Fetched in submodule path '%s', but it did not \"\n -+\t\t\t\t \"contain %s. Direct fetching of that commit failed.\"),\n -+\t\t\t\t ud->displaypath, oid_to_hex(&ud->oid));\n ++\t\tif (!is_tip_reachable(ud->sm_path, &ud->oid) &&\n ++\t\t fetch_in_submodule(ud->sm_path, ud->depth, ud->quiet, &ud->oid))\n ++\t\t\tdie(_(\"Fetched in submodule path '%s', but it did not \"\n ++\t\t\t \"contain %s. Direct fetching of that commit failed.\"),\n ++\t\t\t ud->displaypath, oid_to_hex(&ud->oid));\n +\t}\n +\n +\treturn run_update_command(ud, subforce);\n @@ builtin/submodule--helper.c: static int update_clone(int argc, const char **argv\n +\n +\tfree(prefixed_path);\n +\n -+\tif ((!is_null_oid(&update_data.oid) && !is_null_oid(&update_data.suboid) &&\n -+\t !oideq(&update_data.oid, &update_data.suboid)) ||\n -+\t is_null_oid(&update_data.suboid) || update_data.force)\n ++\tif (!oideq(&update_data.oid, &update_data.suboid) || update_data.force)\n +\t\treturn do_run_update_procedure(&update_data);\n +\n +\treturn 3;\n @@ git-submodule.sh: cmd_update()\n +\n +\t\t# exit codes for run-update-procedure:\n +\t\t# 0: update was successful, say command output\n -+\t\t# 128: subcommand died during execution\n +\t\t# 1: update procedure failed, but should not die\n ++\t\t# 2 or 128: subcommand died during execution\n +\t\t# 3: no update procedure was run\n +\t\tres=\"$?\"\n +\t\tcase $res in\n +\t\t0)\n +\t\t\tsay \"$out\"\n +\t\t\t;;\n -+\t\t128)\n -+\t\t\texit $res\n -+\t\t\t;;\n +\t\t1)\n -+\t\t\terr=\"${err};$out\"\n ++\t\t\terr=\"${err};fatal: $out\"\n +\t\t\tcontinue\n +\t\t\t;;\n ++\t\t2|128)\n ++\t\t\tdie_with_status $res \"fatal: $out\"\n ++\t\t\t;;\n +\t\tesac\n \n \t\tif test -n \"$recursive\"\n\n builtin/submodule--helper.c | 257 ++++++++++++++++++++++++++++++++++++\n git-submodule.sh | 104 +++++----------\n 2 files changed, 289 insertions(+), 72 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex ef2776a9e4..80619361fc 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2045,6 +2045,20 @@ struct submodule_update_clone {\n \t.max_jobs = 1, \\\n }\n \n+struct update_data {\n+\tconst char *recursive_prefix;\n+\tconst char *sm_path;\n+\tconst char *displaypath;\n+\tstruct object_id oid;\n+\tstruct object_id suboid;\n+\tstruct submodule_update_strategy update_strategy;\n+\tint depth;\n+\tunsigned int force: 1;\n+\tunsigned int quiet: 1;\n+\tunsigned int nofetch: 1;\n+\tunsigned int just_cloned: 1;\n+};\n+#define UPDATE_DATA_INIT { .update_strategy = SUBMODULE_UPDATE_STRATEGY_INIT }\n \n static void next_submodule_warn_missing(struct submodule_update_clone *suc,\n \t\tstruct strbuf *out, const char *displaypath)\n@@ -2298,6 +2312,181 @@ static int git_update_clone_config(const char *var, const char *value,\n \treturn 0;\n }\n \n+static int is_tip_reachable(const char *path, struct object_id *oid)\n+{\n+\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\tstruct strbuf rev = STRBUF_INIT;\n+\tchar *hex = oid_to_hex(oid);\n+\n+\tcp.git_cmd = 1;\n+\tcp.dir = xstrdup(path);\n+\tcp.no_stderr = 1;\n+\tstrvec_pushl(&cp.args, \"rev-list\", \"-n\", \"1\", hex, \"--not\", \"--all\", NULL);\n+\n+\tprepare_submodule_repo_env(&cp.env_array);\n+\n+\tif (capture_command(&cp, &rev, GIT_MAX_HEXSZ + 1) || rev.len)\n+\t\treturn 0;\n+\n+\treturn 1;\n+}\n+\n+static int fetch_in_submodule(const char *module_path, int depth, int quiet, struct object_id *oid)\n+{\n+\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\n+\tprepare_submodule_repo_env(&cp.env_array);\n+\tcp.git_cmd = 1;\n+\tcp.dir = xstrdup(module_path);\n+\n+\tstrvec_push(&cp.args, \"fetch\");\n+\tif (quiet)\n+\t\tstrvec_push(&cp.args, \"--quiet\");\n+\tif (depth)\n+\t\tstrvec_pushf(&cp.args, \"--depth=%d\", depth);\n+\tif (oid) {\n+\t\tchar *hex = oid_to_hex(oid);\n+\t\tchar *remote = get_default_remote();\n+\t\tstrvec_pushl(&cp.args, remote, hex, NULL);\n+\t}\n+\n+\treturn run_command(&cp);\n+}\n+\n+static int run_update_command(struct update_data *ud, int subforce)\n+{\n+\tstruct strvec args = STRVEC_INIT;\n+\tstruct strvec child_env = STRVEC_INIT;\n+\tchar *oid = oid_to_hex(&ud->oid);\n+\tint must_die_on_failure = 0;\n+\tint git_cmd;\n+\n+\tswitch (ud->update_strategy.type) {\n+\tcase SM_UPDATE_CHECKOUT:\n+\t\tgit_cmd = 1;\n+\t\tstrvec_pushl(&args, \"checkout\", \"-q\", NULL);\n+\t\tif (subforce)\n+\t\t\tstrvec_push(&args, \"-f\");\n+\t\tbreak;\n+\tcase SM_UPDATE_REBASE:\n+\t\tgit_cmd = 1;\n+\t\tstrvec_push(&args, \"rebase\");\n+\t\tif (ud->quiet)\n+\t\t\tstrvec_push(&args, \"--quiet\");\n+\t\tmust_die_on_failure = 1;\n+\t\tbreak;\n+\tcase SM_UPDATE_MERGE:\n+\t\tgit_cmd = 1;\n+\t\tstrvec_push(&args, \"merge\");\n+\t\tif (ud->quiet)\n+\t\t\tstrvec_push(&args, \"--quiet\");\n+\t\tmust_die_on_failure = 1;\n+\t\tbreak;\n+\tcase SM_UPDATE_COMMAND:\n+\t\tgit_cmd = 0;\n+\t\tstrvec_push(&args, ud->update_strategy.command);\n+\t\tmust_die_on_failure = 1;\n+\t\tbreak;\n+\tdefault:\n+\t\tBUG(\"unexpected update strategy type: %s\",\n+\t\t submodule_strategy_to_string(&ud->update_strategy));\n+\t}\n+\tstrvec_push(&args, oid);\n+\n+\tprepare_submodule_repo_env(&child_env);\n+\tif (run_command_v_opt_cd_env(args.v, git_cmd ? RUN_GIT_CMD : RUN_USING_SHELL,\n+\t\t\t\t ud->sm_path, child_env.v)) {\n+\t\tswitch (ud->update_strategy.type) {\n+\t\tcase SM_UPDATE_CHECKOUT:\n+\t\t\tprintf(_(\"Unable to checkout '%s' in submodule path '%s'\"),\n+\t\t\t oid, ud->displaypath);\n+\t\t\tbreak;\n+\t\tcase SM_UPDATE_REBASE:\n+\t\t\tprintf(_(\"Unable to rebase '%s' in submodule path '%s'\"),\n+\t\t\t oid, ud->displaypath);\n+\t\t\tbreak;\n+\t\tcase SM_UPDATE_MERGE:\n+\t\t\tprintf(_(\"Unable to merge '%s' in submodule path '%s'\"),\n+\t\t\t oid, ud->displaypath);\n+\t\t\tbreak;\n+\t\tcase SM_UPDATE_COMMAND:\n+\t\t\tprintf(_(\"Execution of '%s %s' failed in submodule path '%s'\"),\n+\t\t\t ud->update_strategy.command, oid, ud->displaypath);\n+\t\t\tbreak;\n+\t\tdefault:\n+\t\t\tBUG(\"unexpected update strategy type: %s\",\n+\t\t\t submodule_strategy_to_string(&ud->update_strategy));\n+\t\t}\n+\t\t/*\n+\t\t * NEEDSWORK: We are currently printing to stdout with error\n+\t\t * return so that the shell caller handles the error output\n+\t\t * properly. Once we start handling the error messages within\n+\t\t * C, we should use die() instead.\n+\t\t */\n+\t\tif (must_die_on_failure)\n+\t\t\treturn 2;\n+\t\t/*\n+\t\t * This signifies to the caller in shell that the command\n+\t\t * failed without dying\n+\t\t */\n+\t\treturn 1;\n+\t}\n+\n+\tswitch (ud->update_strategy.type) {\n+\tcase SM_UPDATE_CHECKOUT:\n+\t\tprintf(_(\"Submodule path '%s': checked out '%s'\\n\"),\n+\t\t ud->displaypath, oid);\n+\t\tbreak;\n+\tcase SM_UPDATE_REBASE:\n+\t\tprintf(_(\"Submodule path '%s': rebased into '%s'\\n\"),\n+\t\t ud->displaypath, oid);\n+\t\tbreak;\n+\tcase SM_UPDATE_MERGE:\n+\t\tprintf(_(\"Submodule path '%s': merged in '%s'\\n\"),\n+\t\t ud->displaypath, oid);\n+\t\tbreak;\n+\tcase SM_UPDATE_COMMAND:\n+\t\tprintf(_(\"Submodule path '%s': '%s %s'\\n\"),\n+\t\t ud->displaypath, ud->update_strategy.command, oid);\n+\t\tbreak;\n+\tdefault:\n+\t\tBUG(\"unexpected update strategy type: %s\",\n+\t\t submodule_strategy_to_string(&ud->update_strategy));\n+\t}\n+\n+\treturn 0;\n+}\n+\n+static int do_run_update_procedure(struct update_data *ud)\n+{\n+\tint subforce = is_null_oid(&ud->suboid) || ud->force;\n+\n+\tif (!ud->nofetch) {\n+\t\t/*\n+\t\t * Run fetch only if `oid` isn't present or it\n+\t\t * is not reachable from a ref.\n+\t\t */\n+\t\tif (!is_tip_reachable(ud->sm_path, &ud->oid) &&\n+\t\t fetch_in_submodule(ud->sm_path, ud->depth, ud->quiet, NULL) &&\n+\t\t !ud->quiet)\n+\t\t\tfprintf_ln(stderr,\n+\t\t\t\t _(\"Unable to fetch in submodule path '%s'; \"\n+\t\t\t\t \"trying to directly fetch %s:\"),\n+\t\t\t\t ud->displaypath, oid_to_hex(&ud->oid));\n+\t\t/*\n+\t\t * Now we tried the usual fetch, but `oid` may\n+\t\t * not be reachable from any of the refs.\n+\t\t */\n+\t\tif (!is_tip_reachable(ud->sm_path, &ud->oid) &&\n+\t\t fetch_in_submodule(ud->sm_path, ud->depth, ud->quiet, &ud->oid))\n+\t\t\tdie(_(\"Fetched in submodule path '%s', but it did not \"\n+\t\t\t \"contain %s. Direct fetching of that commit failed.\"),\n+\t\t\t ud->displaypath, oid_to_hex(&ud->oid));\n+\t}\n+\n+\treturn run_update_command(ud, subforce);\n+}\n+\n static void update_submodule(struct update_clone_data *ucd)\n {\n \tfprintf(stdout, \"dummy %s %d\\t%s\\n\",\n@@ -2395,6 +2584,73 @@ static int update_clone(int argc, const char **argv, const char *prefix)\n \treturn update_submodules(&suc);\n }\n \n+static int run_update_procedure(int argc, const char **argv, const char *prefix)\n+{\n+\tint force = 0, quiet = 0, nofetch = 0, just_cloned = 0;\n+\tchar *prefixed_path, *update = NULL;\n+\tstruct update_data update_data = UPDATE_DATA_INIT;\n+\n+\tstruct option options[] = {\n+\t\tOPT__QUIET(&quiet, N_(\"suppress output for update by rebase or merge\")),\n+\t\tOPT__FORCE(&force, N_(\"force checkout updates\"), 0),\n+\t\tOPT_BOOL('N', \"no-fetch\", &nofetch,\n+\t\t\t N_(\"don't fetch new objects from the remote site\")),\n+\t\tOPT_BOOL(0, \"just-cloned\", &just_cloned,\n+\t\t\t N_(\"overrides update mode in case the repository is a fresh clone\")),\n+\t\tOPT_INTEGER(0, \"depth\", &update_data.depth, N_(\"depth for shallow fetch\")),\n+\t\tOPT_STRING(0, \"prefix\", &prefix,\n+\t\t\t N_(\"path\"),\n+\t\t\t N_(\"path into the working tree\")),\n+\t\tOPT_STRING(0, \"update\", &update,\n+\t\t\t N_(\"string\"),\n+\t\t\t N_(\"rebase, merge, checkout or none\")),\n+\t\tOPT_STRING(0, \"recursive-prefix\", &update_data.recursive_prefix, N_(\"path\"),\n+\t\t\t N_(\"path into the working tree, across nested \"\n+\t\t\t \"submodule boundaries\")),\n+\t\tOPT_CALLBACK_F(0, \"oid\", &update_data.oid, N_(\"sha1\"),\n+\t\t\t N_(\"SHA1 expected by superproject\"), PARSE_OPT_NONEG,\n+\t\t\t parse_opt_object_id),\n+\t\tOPT_CALLBACK_F(0, \"suboid\", &update_data.suboid, N_(\"subsha1\"),\n+\t\t\t N_(\"SHA1 of submodule's HEAD\"), PARSE_OPT_NONEG,\n+\t\t\t parse_opt_object_id),\n+\t\tOPT_END()\n+\t};\n+\n+\tconst char *const usage[] = {\n+\t\tN_(\"git submodule--helper run-update-procedure [] \"),\n+\t\tNULL\n+\t};\n+\n+\targc = parse_options(argc, argv, prefix, options, usage, 0);\n+\n+\tif (argc != 1)\n+\t\tusage_with_options(usage, options);\n+\n+\tupdate_data.force = !!force;\n+\tupdate_data.quiet = !!quiet;\n+\tupdate_data.nofetch = !!nofetch;\n+\tupdate_data.just_cloned = !!just_cloned;\n+\tupdate_data.sm_path = argv[0];\n+\n+\tif (update_data.recursive_prefix)\n+\t\tprefixed_path = xstrfmt(\"%s%s\", update_data.recursive_prefix, update_data.sm_path);\n+\telse\n+\t\tprefixed_path = xstrdup(update_data.sm_path);\n+\n+\tupdate_data.displaypath = get_submodule_displaypath(prefixed_path, prefix);\n+\n+\tdetermine_submodule_update_strategy(the_repository, update_data.just_cloned,\n+\t\t\t\t\t update_data.sm_path, update,\n+\t\t\t\t\t &update_data.update_strategy);\n+\n+\tfree(prefixed_path);\n+\n+\tif (!oideq(&update_data.oid, &update_data.suboid) || update_data.force)\n+\t\treturn do_run_update_procedure(&update_data);\n+\n+\treturn 3;\n+}\n+\n static int resolve_relative_path(int argc, const char **argv, const char *prefix)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n@@ -2951,6 +3207,7 @@ static struct cmd_struct commands[] = {\n \t{\"add-clone\", add_clone, 0},\n \t{\"update-module-mode\", module_update_module_mode, 0},\n \t{\"update-clone\", update_clone, 0},\n+\t{\"run-update-procedure\", run_update_procedure, 0},\n \t{\"ensure-core-worktree\", ensure_core_worktree, 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 dbd2ec2050..f703cddce8 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -369,13 +369,6 @@ cmd_deinit()\n \tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper deinit ${GIT_QUIET:+--quiet} ${force:+--force} ${deinit_all:+--all} -- \"$@\"\n }\n \n-is_tip_reachable () (\n-\tsanitize_submodule_env &&\n-\tcd \"$1\" &&\n-\trev=$(git rev-list -n 1 \"$2\" --not --all 2>/dev/null) &&\n-\ttest -z \"$rev\"\n-)\n-\n # usage: fetch_in_submodule [] []\n # Because arguments are positional, use an empty string to omit \n # but include .\n@@ -519,14 +512,13 @@ cmd_update()\n \n \t\tgit submodule--helper ensure-core-worktree \"$sm_path\" || exit 1\n \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\telse\n+\t\t\tjust_cloned=\n \t\t\tsubsha1=$(sanitize_submodule_env; cd \"$sm_path\" &&\n \t\t\t\tgit rev-parse --verify HEAD) ||\n \t\t\tdie \"fatal: $(eval_gettext \"Unable to find current revision in submodule path '\\$displaypath'\")\"\n@@ -547,70 +539,38 @@ cmd_update()\n \t\t\tdie \"fatal: $(eval_gettext \"Unable to find current \\${remote_name}/\\${branch} revision in submodule path '\\$sm_path'\")\"\n \t\tfi\n \n-\t\tif test \"$subsha1\" != \"$sha1\" || test -n \"$force\"\n-\t\tthen\n-\t\t\tsubforce=$force\n-\t\t\t# If we don't already have a -f flag and the submodule has never been checked out\n-\t\t\tif test -z \"$subsha1\" && test -z \"$force\"\n-\t\t\tthen\n-\t\t\t\tsubforce=\"-f\"\n-\t\t\tfi\n+\t\tout=$(git submodule--helper run-update-procedure \\\n+\t\t\t ${wt_prefix:+--prefix \"$wt_prefix\"} \\\n+\t\t\t ${GIT_QUIET:+--quiet} \\\n+\t\t\t ${force:+--force} \\\n+\t\t\t ${just_cloned:+--just-cloned} \\\n+\t\t\t ${nofetch:+--no-fetch} \\\n+\t\t\t ${depth:+\"$depth\"} \\\n+\t\t\t ${update:+--update \"$update\"} \\\n+\t\t\t ${prefix:+--recursive-prefix \"$prefix\"} \\\n+\t\t\t ${sha1:+--oid \"$sha1\"} \\\n+\t\t\t ${subsha1:+--suboid \"$subsha1\"} \\\n+\t\t\t \"--\" \\\n+\t\t\t \"$sm_path\")\n \n-\t\t\tif test -z \"$nofetch\"\n-\t\t\tthen\n-\t\t\t\t# Run fetch only if $sha1 isn't present or it\n-\t\t\t\t# is not reachable from a ref.\n-\t\t\t\tis_tip_reachable \"$sm_path\" \"$sha1\" ||\n-\t\t\t\tfetch_in_submodule \"$sm_path\" $depth ||\n-\t\t\t\tsay \"$(eval_gettext \"Unable to fetch in submodule path '\\$displaypath'; trying to directly fetch \\$sha1:\")\"\n-\n-\t\t\t\t# Now we tried the usual fetch, but $sha1 may\n-\t\t\t\t# not be reachable from any of the refs\n-\t\t\t\tis_tip_reachable \"$sm_path\" \"$sha1\" ||\n-\t\t\t\tfetch_in_submodule \"$sm_path\" \"$depth\" \"$sha1\" ||\n-\t\t\t\tdie \"fatal: $(eval_gettext \"Fetched in submodule path '\\$displaypath', but it did not contain \\$sha1. Direct fetching of that commit failed.\")\"\n-\t\t\tfi\n-\n-\t\t\tmust_die_on_failure=\n-\t\t\tcase \"$update_module\" in\n-\t\t\tcheckout)\n-\t\t\t\tcommand=\"git checkout $subforce -q\"\n-\t\t\t\tdie_msg=\"fatal: $(eval_gettext \"Unable to checkout '\\$sha1' in submodule path '\\$displaypath'\")\"\n-\t\t\t\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': checked out '\\$sha1'\")\"\n-\t\t\t\t;;\n-\t\t\trebase)\n-\t\t\t\tcommand=\"git rebase ${GIT_QUIET:+--quiet}\"\n-\t\t\t\tdie_msg=\"fatal: $(eval_gettext \"Unable to rebase '\\$sha1' in submodule path '\\$displaypath'\")\"\n-\t\t\t\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': rebased into '\\$sha1'\")\"\n-\t\t\t\tmust_die_on_failure=yes\n-\t\t\t\t;;\n-\t\t\tmerge)\n-\t\t\t\tcommand=\"git merge ${GIT_QUIET:+--quiet}\"\n-\t\t\t\tdie_msg=\"fatal: $(eval_gettext \"Unable to merge '\\$sha1' in submodule path '\\$displaypath'\")\"\n-\t\t\t\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': merged in '\\$sha1'\")\"\n-\t\t\t\tmust_die_on_failure=yes\n-\t\t\t\t;;\n-\t\t\t!*)\n-\t\t\t\tcommand=\"${update_module#!}\"\n-\t\t\t\tdie_msg=\"fatal: $(eval_gettext \"Execution of '\\$command \\$sha1' failed in submodule path '\\$displaypath'\")\"\n-\t\t\t\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': '\\$command \\$sha1'\")\"\n-\t\t\t\tmust_die_on_failure=yes\n-\t\t\t\t;;\n-\t\t\t*)\n-\t\t\t\tdie \"fatal: $(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-\t\t\tthen\n-\t\t\t\tsay \"$say_msg\"\n-\t\t\telif test -n \"$must_die_on_failure\"\n-\t\t\tthen\n-\t\t\t\tdie_with_status 2 \"$die_msg\"\n-\t\t\telse\n-\t\t\t\terr=\"${err};$die_msg\"\n-\t\t\t\tcontinue\n-\t\t\tfi\n-\t\tfi\n+\t\t# exit codes for run-update-procedure:\n+\t\t# 0: update was successful, say command output\n+\t\t# 1: update procedure failed, but should not die\n+\t\t# 2 or 128: subcommand died during execution\n+\t\t# 3: no update procedure was run\n+\t\tres=\"$?\"\n+\t\tcase $res in\n+\t\t0)\n+\t\t\tsay \"$out\"\n+\t\t\t;;\n+\t\t1)\n+\t\t\terr=\"${err};fatal: $out\"\n+\t\t\tcontinue\n+\t\t\t;;\n+\t\t2|128)\n+\t\t\tdie_with_status $res \"fatal: $out\"\n+\t\t\t;;\n+\t\tesac\n \n \t\tif test -n \"$recursive\"\n \t\tthen\n-- \n2.32.0\n\n"},{"id":"434970","messageId":"xmqqk0jr9b4p.fsf@gitster.g","threadId":"56147","inReplyTo":"20210824140609.1496-1-raykar.ath@gmail.com","subject":"Re: [PATCH v4] submodule--helper: run update procedures from C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-08T00:14:30Z","receivedAt":"2021-09-08T00:14:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Atharva Raykar writes:\n\n> * Fix error message handling of the output of 'run-update-procedure'. While at\n> it, ensure the \"checkout\" mode error message is stored and printed\n> appropriately.\n>\n> * In 'run_update_command()' switch from 'run_command()' to\n> 'run_command_v_opt_cd_env()' to ensure quoted command update modes are handled\n> correctly.\n>\n> * Code style and hygiene changes.\n>\n> * Introduce a NEEDSWORK comment, because the printf() and error return is\n> correct only because the shell caller in the other end redirects it to the\n> correct output stream. Once we switch this completely to C (ie, in the\n> follow-up series), I need to remember to die() instead (or print to stderr) to\n> reproduce the original behaviour.\n\nI didn't see anybody comment on this round (and do not think I saw\nanything glaringly wrong).\n\nIs everybody happy with this version? I am about to mark it for\n'next' in the next issue of \"What's cooking\" report, so please\nholler if I should wait.\n\nThanks.\n\n"}]}