{"thread":{"id":"58074","subject":"[PATCH 0/5] submodule: remove \"--recursive-prefix\"","startedAt":"2022-06-27T23:20:22Z","lastAt":"2022-07-01T02:12:43Z","messageCount":48,"participants":["Glen Choo via GitGitGadget","Ævar Arnfjörð Bjarmason","Glen Choo","Ævar Arnfjörð Bjarmason via GitGitGadget"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"457975","messageId":"pull.1282.git.git.1656372017.gitgitgadget@gmail.com","threadId":"58074","inReplyTo":null,"subject":"[PATCH 0/5] submodule: remove \"--recursive-prefix\"","fromName":"Glen Choo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-27T23:20:12Z","receivedAt":"2022-06-27T23:20:22Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"= Description\n\nThis series is a refactor of \"git submodule--helper update\" that replaces\n\"--recursive-prefix\" with \"--super-prefix\" (see Background). This was\ninitially motivated by:\n\n * Junio's suggestion to simplify the code [1] (in response to a memory leak\n found by Johannes Schindelin [2]).\n * A desire to use the module_list API + get_submodule_displaypath() outside\n of builtin/submodule--helper.c (I expect to use this to check out\n branches in each submodule).\n\nBut it also happens to remove some overly complicated/duplicated code that\nwas literally converted from shell :)\n\n= Background\n\nWhen recursing into nested submodules, Git commands keep track of the path\nfrom the superproject to the submodule in order to print a \"display path\" to\nthe user, e.g.\n\nSubmodule path '../super/sub/nested-sub': checked out 'abcdef'\n\nFor historical reasons, \"git submodule--helper update\" uses\n\"--recursive-prefix\" for this purpose, but it should use \"--super-prefix\"\ninstead because:\n\n * That's what every other command uses (not just submodule--helper\n subcommands).\n * Using the \"--super-prefix\" helper functions makes the \"git\n submodule--helper update\" code simpler\n\n= Patch organization\n\n * Patch 1/5 makes sure that display paths are only computed using display\n path helper functions ([do_]get_submodule_displaypath()) and fixes some\n display paths that we never realized were broken.\n * Patches 2-3/5 simplify and deduplicate some display path computations.\n * Patch 4/5 replaces \"--recursive-prefix\" with \"--super-prefix\", making\n do_get_submodule_displaypath() obsolete.\n * Patch 5/5 removes do_get_submodule_displaypath().\n\n= Interactions with other series\n\nThis would have been rebased onto ab/submodule-cleanup, but it's not yet\nclear to me if that series will be merged first. That series is almost done,\nbut Ævar is still doing some digging on SUPPORT_SUPER_PREFIX [3] and may\ncome back with findings that affect this series.\n\nFortunately, this series is only tangentially related to\nab/submodule-cleanup, and the conflicts are quite simple:\n\n| this | ab/submodule-cleanup | resolution |\n|-----------------------------+-------------------------------+---------------------------|\n| push --super-prefix arg | add new \"ud\" var | keep both |\n|-----------------------------+-------------------------------+---------------------------|\n| remove \"--recursive-prefix\" | add \"--checkout\", \"--rebase\", | keep both |\n| | \"--merge\" | |\n|-----------------------------+-------------------------------+---------------------------|\n| add SUPPORT_SUPER_PREFIX | remove SUPPORT_SUPER_PREFIX | keep\nab/submodule-cleanup | | to \"git submodule--helper | from \"git\nsubmodule--helper\" | | | update\" | | |\n\n= Future work\n\nAt the end of this series, get_submodule_displaypath() can be moved to\nsubmodule.h, which would make submodule.c:get_super_prefix_or_empty()\nobsolete. I have a patch that demonstrates this (CI run: [4]), but I decided\nto keep this series more focused on \"git submodule--helper update\" so that\nit's easier to review.\n\n[1] https://lore.kernel.org/git/xmqq35g5xmmv.fsf@gitster.g [2]\nhttps://lore.kernel.org/git/877a45867ae368bf9e053caedcb6cf421e02344d.1655336146.git.gitgitgadget@gmail.com\n[3]\nhttps://lore.kernel.org/git/patch-v3-02.12-082e015781e-20220622T142012Z-avarab@gmail.com\n[4] https://github.com/chooglen/git/actions/runs/2572557584\n\nGlen Choo (5):\n submodule--helper update: use display path helper\n submodule--helper: don't recreate recursive prefix\n submodule--helper: use correct display path helper\n submodule--helper update: use --super-prefix\n submodule--helper: remove display path helper\n\n builtin/submodule--helper.c | 82 +++++++++----------------------------\n t/t7406-submodule-update.sh | 59 ++++++++++++++++++++++++++\n 2 files changed, 79 insertions(+), 62 deletions(-)\n\n\nbase-commit: 39c15e485575089eb77c769f6da02f98a55905e0\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1282%2Fchooglen%2Fsubmodule%2Fremove-recursive-prefix-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1282/chooglen/submodule/remove-recursive-prefix-v1\nPull-Request: https://github.com/git/git/pull/1282\n-- \ngitgitgadget\n"},{"id":"457977","messageId":"473548f2fa473b9b94fcc099a81613c622a32022.1656372017.git.gitgitgadget@gmail.com","threadId":"58074","inReplyTo":"pull.1282.git.git.1656372017.gitgitgadget@gmail.com","subject":"[PATCH 1/5] submodule--helper update: use display path helper","fromName":"Glen Choo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-27T23:20:13Z","receivedAt":"2022-06-27T23:20:24Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"From: Glen Choo \n\nThere are two locations in prepare_to_clone_next_submodule() that\nmanually calculate the submodule display path, but should just use\ndo_get_submodule_displaypath() for consistency.\n\nDo this replacement and reorder the code slightly to avoid computing\nthe display path twice.\n\nThis code was never tested, and adding tests shows that both these sites\nhave been computing the display path incorrectly ever since they were\nintroduced in 48308681b0 (git submodule update: have a dedicated helper\nfor cloning, 2016-02-29) [1]:\n\n- The first hunk puts a \"/\" between recursive_prefix and ce->name, but\n recursive_prefix already ends with \"/\".\n- The second hunk calls relative_path() on recursive_prefix and\n ce->name, but relative_path() only makes sense when both paths share\n the same base directory. This is never the case here:\n - recursive_prefix is the path from the topmost superproject to the\n current submodule\n - ce->name is the path from the root of the current submodule to its\n submodule.\n so, e.g. recursive_prefix=\"super\" and ce->name=\"submodule\" produces\n displayname=\"../super\" instead of \"super/submodule\".\n\nWhile we're fixing the display names, also fix inconsistent quoting of\nthe submodule name.\n\n[1] I verified this by applying the tests to 48308681b0.\n\nSigned-off-by: Glen Choo \n---\n builtin/submodule--helper.c | 22 +++++---------\n t/t7406-submodule-update.sh | 59 +++++++++++++++++++++++++++++++++++++\n 2 files changed, 66 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex c597df7528e..63c661b26a6 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1949,30 +1949,22 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n \tconst char *update_string;\n \tenum submodule_update_type update_type;\n \tchar *key;\n-\tstruct strbuf displaypath_sb = STRBUF_INIT;\n \tstruct strbuf sb = STRBUF_INIT;\n-\tconst char *displaypath = NULL;\n+\tchar *displaypath = NULL;\n \tint needs_cloning = 0;\n \tint need_free_url = 0;\n \n+\tdisplaypath = do_get_submodule_displaypath(ce->name,\n+\t\t\t\t\t\t suc->update_data->prefix,\n+\t\t\t\t\t\t suc->update_data->recursive_prefix);\n+\n \tif (ce_stage(ce)) {\n-\t\tif (suc->update_data->recursive_prefix)\n-\t\t\tstrbuf_addf(&sb, \"%s/%s\", suc->update_data->recursive_prefix, ce->name);\n-\t\telse\n-\t\t\tstrbuf_addstr(&sb, ce->name);\n-\t\tstrbuf_addf(out, _(\"Skipping unmerged submodule %s\"), sb.buf);\n-\t\tstrbuf_addch(out, '\\n');\n+\t\tstrbuf_addf(out, _(\"Skipping unmerged submodule '%s'\\n\"), displaypath);\n \t\tgoto cleanup;\n \t}\n \n \tsub = submodule_from_path(the_repository, null_oid(), ce->name);\n \n-\tif (suc->update_data->recursive_prefix)\n-\t\tdisplaypath = relative_path(suc->update_data->recursive_prefix,\n-\t\t\t\t\t ce->name, &displaypath_sb);\n-\telse\n-\t\tdisplaypath = ce->name;\n-\n \tif (!sub) {\n \t\tnext_submodule_warn_missing(suc, out, displaypath);\n \t\tgoto cleanup;\n@@ -2062,7 +2054,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n \t\t\t\t\t \"--no-single-branch\");\n \n cleanup:\n-\tstrbuf_release(&displaypath_sb);\n+\tfree(displaypath);\n \tstrbuf_release(&sb);\n \tif (need_free_url)\n \t\tfree((void*)url);\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex 43f779d751c..e1dc3b1041b 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -1116,4 +1116,63 @@ test_expect_success 'submodule update --filter sets partial clone settings' '\n \ttest_cmp_config -C super-filter/submodule blob:none remote.origin.partialclonefilter\n '\n \n+# NEEDSWORK: Clean up the tests so that we can reuse the test setup.\n+# Don't reuse the existing repos because the earlier tests have\n+# intentionally disruptive configurations.\n+test_expect_success 'setup clean recursive superproject' '\n+\tgit init bottom &&\n+\ttest_commit -C bottom \"bottom\" &&\n+\tgit init middle &&\n+\tgit -C middle submodule add ../bottom bottom &&\n+\tgit -C middle commit -m \"middle\" &&\n+\tgit init top &&\n+\tgit -C top submodule add ../middle middle &&\n+\tgit -C top commit -m \"top\" &&\n+\tgit clone --recurse-submodules top top-clean\n+'\n+\n+test_expect_success 'submodule update should skip unmerged submodules' '\n+\ttest_when_finished \"rm -fr top-cloned\" &&\n+\tcp -r top-clean top-cloned &&\n+\n+\t# Create an upstream commit in each repo\n+\ttest_commit -C bottom upstream_commit &&\n+\t(cd middle &&\n+\t git -C bottom fetch &&\n+\t git -C bottom checkout -f FETCH_HEAD &&\n+\t git add bottom &&\n+\t git commit -m \"upstream_commit\"\n+\t) &&\n+\t(cd top &&\n+\t git -C middle fetch &&\n+\t git -C middle checkout -f FETCH_HEAD &&\n+\t git add middle &&\n+\t git commit -m \"upstream_commit\"\n+\t) &&\n+\n+\t# Create a downstream conflict\n+\t(cd top-cloned/middle &&\n+\t test_commit -C bottom downstream_commit &&\n+\t git add bottom &&\n+\t git commit -m \"downstream_commit\" &&\n+\t git fetch --recurse-submodules origin &&\n+\t test_must_fail git merge origin/main\n+\t) &&\n+\t# Make the update of \"middle\" a no-op, otherwise we error out\n+\t# because of its unmerged state\n+\ttest_config -C top-cloned submodule.middle.update !true &&\n+\tgit -C top-cloned submodule update --recursive 2>actual.err &&\n+\tgrep \"Skipping unmerged submodule .middle/bottom.\" actual.err\n+'\n+\n+test_expect_success 'submodule update --recursive skip submodules with strategy=none' '\n+\ttest_when_finished \"rm -fr top-cloned\" &&\n+\tcp -r top-clean top-cloned &&\n+\n+\ttest_commit -C top-cloned/middle/bottom downstream_commit &&\n+\tgit -C top-cloned/middle config submodule.bottom.update none &&\n+\tgit -C top-cloned submodule update --recursive 2>actual.err &&\n+\tgrep \"Skipping submodule .middle/bottom.\" actual.err\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"457976","messageId":"618053730e1f0732fa5a56ffbaa123129eaa17ca.1656372017.git.gitgitgadget@gmail.com","threadId":"58074","inReplyTo":"pull.1282.git.git.1656372017.gitgitgadget@gmail.com","subject":"[PATCH 2/5] submodule--helper: don't recreate recursive prefix","fromName":"Glen Choo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-27T23:20:14Z","receivedAt":"2022-06-27T23:20:26Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"From: Glen Choo \n\nupdate_submodule() uses duplicated code to compute\nupdate_data->displaypath and next.recursive_prefix. The latter is just\nthe former with \"/\" appended to it, and since update_data->displaypath\nnot changed outside of this statement, we can just reuse the already\ncomputed result.\n\nWe can go one step further and remove the reference to\nnext.recursive_prefix altogether. Since it is only used in\nupdate_data_to_args() (to compute the \"--recursive-prefix\" flag for the\nrecursive update child process) we can just use the already computed\n.displaypath value of there.\n\nDelete the duplicated code, and remove the unnecessary reference to\nnext.recursive_prefix. As a bonus, this fixes a memory leak where\nprefixed_path was never freed (this leak was first reported in [1]).\n\n[1] https://lore.kernel.org/git/877a45867ae368bf9e053caedcb6cf421e02344d.1655336146.git.gitgitgadget@gmail.com\n\nSigned-off-by: Glen Choo \n---\n builtin/submodule--helper.c | 14 +++-----------\n 1 file changed, 3 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 63c661b26a6..7a963a967b8 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2401,9 +2401,9 @@ static void update_data_to_args(struct update_data *update_data, struct strvec *\n {\n \tstrvec_pushl(args, \"submodule--helper\", \"update\", \"--recursive\", NULL);\n \tstrvec_pushf(args, \"--jobs=%d\", update_data->max_jobs);\n-\tif (update_data->recursive_prefix)\n-\t\tstrvec_pushl(args, \"--recursive-prefix\",\n-\t\t\t update_data->recursive_prefix, NULL);\n+\tif (update_data->displaypath)\n+\t\tstrvec_pushf(args, \"--recursive-prefix=%s/\",\n+\t\t\t update_data->displaypath);\n \tif (update_data->quiet)\n \t\tstrvec_push(args, \"--quiet\");\n \tif (update_data->force)\n@@ -2497,14 +2497,6 @@ static int update_submodule(struct update_data *update_data)\n \t\tstruct update_data next = *update_data;\n \t\tint res;\n \n-\t\tif (update_data->recursive_prefix)\n-\t\t\tprefixed_path = xstrfmt(\"%s%s/\", update_data->recursive_prefix,\n-\t\t\t\t\t\tupdate_data->sm_path);\n-\t\telse\n-\t\t\tprefixed_path = xstrfmt(\"%s/\", update_data->sm_path);\n-\n-\t\tnext.recursive_prefix = get_submodule_displaypath(prefixed_path,\n-\t\t\t\t\t\t\t\t update_data->prefix);\n \t\tnext.prefix = NULL;\n \t\toidcpy(&next.oid, null_oid());\n \t\toidcpy(&next.suboid, null_oid());\n-- \ngitgitgadget\n\n"},{"id":"457978","messageId":"7cd1c46f350bf26216814dcf9dd4b8002a91467a.1656372017.git.gitgitgadget@gmail.com","threadId":"58074","inReplyTo":"pull.1282.git.git.1656372017.gitgitgadget@gmail.com","subject":"[PATCH 3/5] submodule--helper: use correct display path helper","fromName":"Glen Choo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-27T23:20:15Z","receivedAt":"2022-06-27T23:20:30Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"From: Glen Choo \n\nReplace a chunk of code in update_submodule() with an equivalent\ndo_get_submodule_displaypath() invocation. This is already tested by\nt/t7406-submodule-update.sh:'submodule update --init --recursive from\nsubdirectory', so no tests are added.\n\nThe two are equivalent because:\n\n- Exactly one of recursive_prefix|prefix is non-NULL at a time; prefix\n is set at the superproject level, and recursive_prefix is set when\n recursing into submodules. There is also a BUG() statement in\n get_submodule_displaypath() that asserts that both cannot be non-NULL.\n\n- In get_submodule_displaypath(), get_super_prefix() always returns NULL\n because \"--super-prefix\" is never passed. Thus calling it is\n equivalent to calling do_get_submodule_displaypath() with super_prefix\n = NULL.\n\nTherefore:\n\n- When recursive_prefix is non-NULL, prefix is NULL, and thus\n get_submodule_displaypath() just returns prefixed_path. This is\n identical to calling do_get_submodule_displaypath() with super_prefix\n = recursive_prefix because the return value is still the concatenation\n of recursive_prefix + update_data->sm_path.\n\n- When prefix is non-NULL, prefixed_path = update_data->sm_path. Thus\n calling get_submodule_displaypath() with prefixed_path is equivalent\n to calling do_get_submodule_displaypath() with update_data->sm_path\n\nSigned-off-by: Glen Choo \n---\n builtin/submodule--helper.c | 14 +++-----------\n 1 file changed, 3 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 7a963a967b8..fa8256526e9 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2445,19 +2445,11 @@ static void update_data_to_args(struct update_data *update_data, struct strvec *\n \n static int update_submodule(struct update_data *update_data)\n {\n-\tchar *prefixed_path;\n-\n \tensure_core_worktree(update_data->sm_path);\n \n-\tif (update_data->recursive_prefix)\n-\t\tprefixed_path = xstrfmt(\"%s%s\", update_data->recursive_prefix,\n-\t\t\t\t\tupdate_data->sm_path);\n-\telse\n-\t\tprefixed_path = xstrdup(update_data->sm_path);\n-\n-\tupdate_data->displaypath = get_submodule_displaypath(prefixed_path,\n-\t\t\t\t\t\t\t update_data->prefix);\n-\tfree(prefixed_path);\n+\tupdate_data->displaypath = do_get_submodule_displaypath(update_data->sm_path,\n+\t\t\t\t\t\t\t\tupdate_data->prefix,\n+\t\t\t\t\t\t\t\tupdate_data->recursive_prefix);\n \n \tdetermine_submodule_update_strategy(the_repository, update_data->just_cloned,\n \t\t\t\t\t update_data->sm_path, update_data->update_default,\n-- \ngitgitgadget\n\n"},{"id":"457979","messageId":"57988287fc01a8baf5c4fd7326772c80bc015f3c.1656372017.git.gitgitgadget@gmail.com","threadId":"58074","inReplyTo":"pull.1282.git.git.1656372017.gitgitgadget@gmail.com","subject":"[PATCH 4/5] submodule--helper update: use --super-prefix","fromName":"Glen Choo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-27T23:20:16Z","receivedAt":"2022-06-27T23:20:35Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"From: Glen Choo \n\nUnlike the other subcommands, \"git submodule--helper update\" uses the\n\"--recursive-prefix\" flag instead of \"--super-prefix\". The two flags are\notherwise identical (they only serve to compute the 'display path' of a\nsubmodule), except that there is a dedicated helper function to get the\nvalue of \"--super-prefix\".\n\nThis inconsistency exists because \"git submodule update\" used to pass\n\"--recursive-prefix\" between shell and C (introduced in [1]) before\n\"--super-prefix\" was introduced (in [2]), and for simplicity, we kept\nthis name when \"git submodule--helper update\" was created.\n\nRemove \"--recursive-prefix\" and its associated code from \"git\nsubmodule--helper update\", replacing it with \"--super-prefix\".\n\nTo use \"--super-prefix\", module_update is marked with\nSUPPORT_SUPER_PREFIX. Note that module_clone must also be marked with\nSUPPORT_SUPER_PREFIX, otherwise the \"git submodule--helper clone\"\nsubprocess will fail check because \"--super-prefix\" is propagated via\nthe environment.\n\n[1] 48308681b0 (git submodule update: have a dedicated helper for\ncloning, 2016-02-29)\n[2] 74866d7579 (git: make super-prefix option, 2016-10-07)\n\nSigned-off-by: Glen Choo \n---\n builtin/submodule--helper.c | 30 ++++++++++--------------------\n 1 file changed, 10 insertions(+), 20 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex fa8256526e9..81ea4669aab 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -477,22 +477,18 @@ static int starts_with_dot_dot_slash(const char *const path)\n \n struct init_cb {\n \tconst char *prefix;\n-\tconst char *superprefix;\n \tunsigned int flags;\n };\n #define INIT_CB_INIT { 0 }\n \n static void init_submodule(const char *path, const char *prefix,\n-\t\t\t const char *superprefix, unsigned int flags)\n+\t\t\t unsigned int flags)\n {\n \tconst struct submodule *sub;\n \tstruct strbuf sb = STRBUF_INIT;\n \tchar *upd = NULL, *url = NULL, *displaypath;\n \n-\t/* try superprefix from the environment, if it is not passed explicitly */\n-\tif (!superprefix)\n-\t\tsuperprefix = get_super_prefix();\n-\tdisplaypath = do_get_submodule_displaypath(path, prefix, superprefix);\n+\tdisplaypath = do_get_submodule_displaypath(path, prefix, get_super_prefix());\n \n \tsub = submodule_from_path(the_repository, null_oid(), path);\n \n@@ -566,7 +562,7 @@ static void init_submodule(const char *path, const char *prefix,\n static void init_submodule_cb(const struct cache_entry *list_item, void *cb_data)\n {\n \tstruct init_cb *info = cb_data;\n-\tinit_submodule(list_item->name, info->prefix, info->superprefix, info->flags);\n+\tinit_submodule(list_item->name, info->prefix, info->flags);\n }\n \n static int module_init(int argc, const char **argv, const char *prefix)\n@@ -1880,7 +1876,6 @@ struct submodule_update_clone {\n \n struct update_data {\n \tconst char *prefix;\n-\tconst char *recursive_prefix;\n \tconst char *displaypath;\n \tconst char *update_default;\n \tstruct object_id suboid;\n@@ -1956,7 +1951,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n \n \tdisplaypath = do_get_submodule_displaypath(ce->name,\n \t\t\t\t\t\t suc->update_data->prefix,\n-\t\t\t\t\t\t suc->update_data->recursive_prefix);\n+\t\t\t\t\t\t get_super_prefix());\n \n \tif (ce_stage(ce)) {\n \t\tstrbuf_addf(out, _(\"Skipping unmerged submodule '%s'\\n\"), displaypath);\n@@ -2399,11 +2394,11 @@ static void ensure_core_worktree(const char *path)\n \n static void update_data_to_args(struct update_data *update_data, struct strvec *args)\n {\n-\tstrvec_pushl(args, \"submodule--helper\", \"update\", \"--recursive\", NULL);\n-\tstrvec_pushf(args, \"--jobs=%d\", update_data->max_jobs);\n \tif (update_data->displaypath)\n-\t\tstrvec_pushf(args, \"--recursive-prefix=%s/\",\n+\t\tstrvec_pushf(args, \"--super-prefix=%s/\",\n \t\t\t update_data->displaypath);\n+\tstrvec_pushl(args, \"submodule--helper\", \"update\", \"--recursive\", NULL);\n+\tstrvec_pushf(args, \"--jobs=%d\", update_data->max_jobs);\n \tif (update_data->quiet)\n \t\tstrvec_push(args, \"--quiet\");\n \tif (update_data->force)\n@@ -2449,7 +2444,7 @@ static int update_submodule(struct update_data *update_data)\n \n \tupdate_data->displaypath = do_get_submodule_displaypath(update_data->sm_path,\n \t\t\t\t\t\t\t\tupdate_data->prefix,\n-\t\t\t\t\t\t\t\tupdate_data->recursive_prefix);\n+\t\t\t\t\t\t\t\tget_super_prefix());\n \n \tdetermine_submodule_update_strategy(the_repository, update_data->just_cloned,\n \t\t\t\t\t update_data->sm_path, update_data->update_default,\n@@ -2573,10 +2568,6 @@ static int module_update(int argc, const char **argv, const char *prefix)\n \t\tOPT_STRING(0, \"prefix\", &opt.prefix,\n \t\t\t N_(\"path\"),\n \t\t\t N_(\"path into the working tree\")),\n-\t\tOPT_STRING(0, \"recursive-prefix\", &opt.recursive_prefix,\n-\t\t\t 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, \"update\", &opt.update_default,\n \t\t\t N_(\"string\"),\n \t\t\t N_(\"rebase, merge, checkout or none\")),\n@@ -2655,7 +2646,6 @@ static int module_update(int argc, const char **argv, const char *prefix)\n \t\t\tmodule_list_active(&list);\n \n \t\tinfo.prefix = opt.prefix;\n-\t\tinfo.superprefix = opt.recursive_prefix;\n \t\tif (opt.quiet)\n \t\t\tinfo.flags |= OPT_QUIET;\n \n@@ -3352,9 +3342,9 @@ struct cmd_struct {\n static struct cmd_struct commands[] = {\n \t{\"list\", module_list, 0},\n \t{\"name\", module_name, 0},\n-\t{\"clone\", module_clone, 0},\n+\t{\"clone\", module_clone, SUPPORT_SUPER_PREFIX},\n \t{\"add\", module_add, SUPPORT_SUPER_PREFIX},\n-\t{\"update\", module_update, 0},\n+\t{\"update\", module_update, SUPPORT_SUPER_PREFIX},\n \t{\"resolve-relative-url-test\", resolve_relative_url_test, 0},\n \t{\"foreach\", module_foreach, SUPPORT_SUPER_PREFIX},\n \t{\"init\", module_init, SUPPORT_SUPER_PREFIX},\n-- \ngitgitgadget\n\n"},{"id":"457980","messageId":"9fa13380b02174a4ff9cbe153166689561d23535.1656372017.git.gitgitgadget@gmail.com","threadId":"58074","inReplyTo":"pull.1282.git.git.1656372017.gitgitgadget@gmail.com","subject":"[PATCH 5/5] submodule--helper: remove display path helper","fromName":"Glen Choo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-27T23:20:17Z","receivedAt":"2022-06-27T23:20:46Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"From: Glen Choo \n\nAll invocations of do_get_submodule_displaypath() pass\nget_super_prefix() as the super_prefix arg, which is exactly the same\nas get_submodule_displaypath().\n\nReplace all calls to do_get_submodule_displaypath() with\nget_submodule_displaypath(), and since it has no more callers, remove\nit.\n\nSigned-off-by: Glen Choo \n---\n builtin/submodule--helper.c | 26 +++++++++-----------------\n 1 file changed, 9 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 81ea4669aab..73b36f47600 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -118,10 +118,11 @@ static int resolve_relative_url_test(int argc, const char **argv, const char *pr\n \treturn 0;\n }\n \n-static char *do_get_submodule_displaypath(const char *path,\n-\t\t\t\t\t const char *prefix,\n-\t\t\t\t\t const char *super_prefix)\n+/* the result should be freed by the caller. */\n+static char *get_submodule_displaypath(const char *path, const char *prefix)\n {\n+\tconst char *super_prefix = get_super_prefix();\n+\n \tif (prefix && super_prefix) {\n \t\tBUG(\"cannot have prefix '%s' and superprefix '%s'\",\n \t\t prefix, super_prefix);\n@@ -137,13 +138,6 @@ static char *do_get_submodule_displaypath(const char *path,\n \t}\n }\n \n-/* the result should be freed by the caller. */\n-static char *get_submodule_displaypath(const char *path, const char *prefix)\n-{\n-\tconst char *super_prefix = get_super_prefix();\n-\treturn do_get_submodule_displaypath(path, prefix, super_prefix);\n-}\n-\n static char *compute_rev_name(const char *sub_path, const char* object_id)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n@@ -488,7 +482,7 @@ static void init_submodule(const char *path, const char *prefix,\n \tstruct strbuf sb = STRBUF_INIT;\n \tchar *upd = NULL, *url = NULL, *displaypath;\n \n-\tdisplaypath = do_get_submodule_displaypath(path, prefix, get_super_prefix());\n+\tdisplaypath = get_submodule_displaypath(path, prefix);\n \n \tsub = submodule_from_path(the_repository, null_oid(), path);\n \n@@ -1949,9 +1943,8 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n \tint needs_cloning = 0;\n \tint need_free_url = 0;\n \n-\tdisplaypath = do_get_submodule_displaypath(ce->name,\n-\t\t\t\t\t\t suc->update_data->prefix,\n-\t\t\t\t\t\t get_super_prefix());\n+\tdisplaypath =\n+\t\tget_submodule_displaypath(ce->name, suc->update_data->prefix);\n \n \tif (ce_stage(ce)) {\n \t\tstrbuf_addf(out, _(\"Skipping unmerged submodule '%s'\\n\"), displaypath);\n@@ -2442,9 +2435,8 @@ static int update_submodule(struct update_data *update_data)\n {\n \tensure_core_worktree(update_data->sm_path);\n \n-\tupdate_data->displaypath = do_get_submodule_displaypath(update_data->sm_path,\n-\t\t\t\t\t\t\t\tupdate_data->prefix,\n-\t\t\t\t\t\t\t\tget_super_prefix());\n+\tupdate_data->displaypath = get_submodule_displaypath(\n+\t\tupdate_data->sm_path, update_data->prefix);\n \n \tdetermine_submodule_update_strategy(the_repository, update_data->just_cloned,\n \t\t\t\t\t update_data->sm_path, update_data->update_default,\n-- \ngitgitgadget\n"},{"id":"457998","messageId":"220628.861qv9gpg5.gmgdl@evledraar.gmail.com","threadId":"58074","inReplyTo":"473548f2fa473b9b94fcc099a81613c622a32022.1656372017.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/5] submodule--helper update: use display path helper","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-28T08:23:22Z","receivedAt":"2022-06-28T08:30:48Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Jun 27 2022, Glen Choo via GitGitGadget wrote:\n\n> From: Glen Choo \n>\n> There are two locations in prepare_to_clone_next_submodule() that\n> manually calculate the submodule display path, but should just use\n> do_get_submodule_displaypath() for consistency.\n>\n> Do this replacement and reorder the code slightly to avoid computing\n> the display path twice.\n>\n> This code was never tested, and adding tests shows that both these sites\n> have been computing the display path incorrectly ever since they were\n> introduced in 48308681b0 (git submodule update: have a dedicated helper\n> for cloning, 2016-02-29) [1]:\n\nI think the tests should really be split up as their own preceding\ncommit, so we're assured that this \"git rebase -x 'make test'\"'s\ncleanly.\n\n> [...]\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index c597df7528e..63c661b26a6 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -1949,30 +1949,22 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n> \tconst char *update_string;\n> \tenum submodule_update_type update_type;\n> \tchar *key;\n> -\tstruct strbuf displaypath_sb = STRBUF_INIT;\n> \tstruct strbuf sb = STRBUF_INIT;\n> -\tconst char *displaypath = NULL;\n> +\tchar *displaypath = NULL;\n\nDon't init to NULL?...\n\n> \tint needs_cloning = 0;\n> \tint need_free_url = 0;\n> \n> +\tdisplaypath = do_get_submodule_displaypath(ce->name,\n\n...because this will either die or return a valid string, and the init\nis just suppressing compiler flow analysis, no?\n\n> +\t\t\t\t\t\t suc->update_data->prefix,\n> +\t\t\t\t\t\t suc->update_data->recursive_prefix);\n> +\n> \tif (ce_stage(ce)) {\n> -\t\tif (suc->update_data->recursive_prefix)\n> -\t\t\tstrbuf_addf(&sb, \"%s/%s\", suc->update_data->recursive_prefix, ce->name);\n> -\t\telse\n> -\t\t\tstrbuf_addstr(&sb, ce->name);\n> -\t\tstrbuf_addf(out, _(\"Skipping unmerged submodule %s\"), sb.buf);\n> -\t\tstrbuf_addch(out, '\\n');\n> +\t\tstrbuf_addf(out, _(\"Skipping unmerged submodule '%s'\\n\"), displaypath);\n\nNit: The removal of strbuf_addch() here is bad, we try not to expose\ntranslators to such formatting concerns, so let's leave the \\n out of\nthe message still.\n\nBut more imporantly let's also split up the %s to '%s' change up, or\nperhaps just drop it for now? Isn't it entirely unrelated?\n\n> \t\tgoto cleanup;\n> \t}\n> \n> \tsub = submodule_from_path(the_repository, null_oid(), ce->name);\n> \n> -\tif (suc->update_data->recursive_prefix)\n> -\t\tdisplaypath = relative_path(suc->update_data->recursive_prefix,\n> -\t\t\t\t\t ce->name, &displaypath_sb);\n> -\telse\n> -\t\tdisplaypath = ce->name;\n> -\n> \tif (!sub) {\n> \t\tnext_submodule_warn_missing(suc, out, displaypath);\n> \t\tgoto cleanup;\n> @@ -2062,7 +2054,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n> \t\t\t\t\t \"--no-single-branch\");\n> \n> cleanup:\n> -\tstrbuf_release(&displaypath_sb);\n> +\tfree(displaypath);\n> \tstrbuf_release(&sb);\n> \tif (need_free_url)\n> \t\tfree((void*)url);\n\nI must admit I didn't spend too much time on the *actual* logic change\nhere, but nothing seems obviously incorrect...\n\n> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\n> index 43f779d751c..e1dc3b1041b 100755\n> --- a/t/t7406-submodule-update.sh\n> +++ b/t/t7406-submodule-update.sh\n> @@ -1116,4 +1116,63 @@ test_expect_success 'submodule update --filter sets partial clone settings' '\n> \ttest_cmp_config -C super-filter/submodule blob:none remote.origin.partialclonefilter\n> '\n> \n> +# NEEDSWORK: Clean up the tests so that we can reuse the test setup.\n> +# Don't reuse the existing repos because the earlier tests have\n> +# intentionally disruptive configurations.\n\nYeah this does seem a bit repetative...\n\n> +test_expect_success 'setup clean recursive superproject' '\n> +\tgit init bottom &&\n> +\ttest_commit -C bottom \"bottom\" &&\n> +\tgit init middle &&\n> +\tgit -C middle submodule add ../bottom bottom &&\n> +\tgit -C middle commit -m \"middle\" &&\n> +\tgit init top &&\n> +\tgit -C top submodule add ../middle middle &&\n> +\tgit -C top commit -m \"top\" &&\n> +\tgit clone --recurse-submodules top top-clean\n> +'\n> +\n> +test_expect_success 'submodule update should skip unmerged submodules' '\n> +\ttest_when_finished \"rm -fr top-cloned\" &&\n> +\tcp -r top-clean top-cloned &&\n> +\n> +\t# Create an upstream commit in each repo\n> +\ttest_commit -C bottom upstream_commit &&\n> +\t(cd middle &&\n> +\t git -C bottom fetch &&\n> +\t git -C bottom checkout -f FETCH_HEAD &&\n> +\t git add bottom &&\n> +\t git commit -m \"upstream_commit\"\n> +\t) &&\n> +\t(cd top &&\n> +\t git -C middle fetch &&\n> +\t git -C middle checkout -f FETCH_HEAD &&\n> +\t git add middle &&\n> +\t git commit -m \"upstream_commit\"\n> +\t) &&\n\nE.g. here the mixture of \"cd\" and \"-C\" is a bit odd, can't we just use:\n\n git -C middle/bottom ...\n ...\n git -C middle add \n\nI also wonder if this can't be a for-loop with the right params\n\n> +\n> +\t# Create a downstream conflict\n> +\t(cd top-cloned/middle &&\n> +\t test_commit -C bottom downstream_commit &&\n> +\t git add bottom &&\n> +\t git commit -m \"downstream_commit\" &&\n> +\t git fetch --recurse-submodules origin &&\n> +\t test_must_fail git merge origin/main\n> +\t) &&\n> +\t# Make the update of \"middle\" a no-op, otherwise we error out\n> +\t# because of its unmerged state\n> +\ttest_config -C top-cloned submodule.middle.update !true &&\n> +\tgit -C top-cloned submodule update --recursive 2>actual.err &&\n> +\tgrep \"Skipping unmerged submodule .middle/bottom.\" actual.err\n\nMaybe use grep -F with ${SQ} instead?\n> +'\n> +\n> +test_expect_success 'submodule update --recursive skip submodules with strategy=none' '\n> +\ttest_when_finished \"rm -fr top-cloned\" &&\n> +\tcp -r top-clean top-cloned &&\n> +\n> +\ttest_commit -C top-cloned/middle/bottom downstream_commit &&\n> +\tgit -C top-cloned/middle config submodule.bottom.update none &&\n> +\tgit -C top-cloned submodule update --recursive 2>actual.err &&\n> +\tgrep \"Skipping submodule .middle/bottom.\" actual.err\n> +'\n> +\n> test_done\n\n"},{"id":"457999","messageId":"220628.86wnd1f9t1.gmgdl@evledraar.gmail.com","threadId":"58074","inReplyTo":"57988287fc01a8baf5c4fd7326772c80bc015f3c.1656372017.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 4/5] submodule--helper update: use --super-prefix","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-28T08:47:21Z","receivedAt":"2022-06-28T08:53:54Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Jun 27 2022, Glen Choo via GitGitGadget wrote:\n\n> From: Glen Choo \n>\n> Unlike the other subcommands, \"git submodule--helper update\" uses the\n> \"--recursive-prefix\" flag instead of \"--super-prefix\". The two flags are\n> otherwise identical (they only serve to compute the 'display path' of a\n> submodule), except that there is a dedicated helper function to get the\n> value of \"--super-prefix\".\n\nThis is a good change, it was slightly confusing that --recursive-prefix\nis left in git-submodule.sh after this, but then I remembered that I\nremoved it in my ab/submodule-cleanup, and you were presumably trying to\navoid the conflict.\n\nStill, I think it's probably better to either base this on my series\n(re-roll incoming), or take make this truly stand-alone, and have Junio\nsort out the minor conflict.\n\n> static void update_data_to_args(struct update_data *update_data, struct strvec *args)\n> {\n> -\tstrvec_pushl(args, \"submodule--helper\", \"update\", \"--recursive\", NULL);\n> -\tstrvec_pushf(args, \"--jobs=%d\", update_data->max_jobs);\n> \tif (update_data->displaypath)\n> -\t\tstrvec_pushf(args, \"--recursive-prefix=%s/\",\n> +\t\tstrvec_pushf(args, \"--super-prefix=%s/\",\n> \t\t\t update_data->displaypath);\n> +\tstrvec_pushl(args, \"submodule--helper\", \"update\", \"--recursive\", NULL);\n> +\tstrvec_pushf(args, \"--jobs=%d\", update_data->max_jobs);\n\nI did a double-take at this, but it's just one of these cases where\n\"diff\" is being overly helpful in trying to find us the most minimal\ndiff possible :)\n\n> @@ -3352,9 +3342,9 @@ struct cmd_struct {\n> static struct cmd_struct commands[] = {\n> \t{\"list\", module_list, 0},\n> \t{\"name\", module_name, 0},\n> -\t{\"clone\", module_clone, 0},\n> +\t{\"clone\", module_clone, SUPPORT_SUPER_PREFIX},\n> \t{\"add\", module_add, SUPPORT_SUPER_PREFIX},\n> -\t{\"update\", module_update, 0},\n> +\t{\"update\", module_update, SUPPORT_SUPER_PREFIX},\n> \t{\"resolve-relative-url-test\", resolve_relative_url_test, 0},\n> \t{\"foreach\", module_foreach, SUPPORT_SUPER_PREFIX},\n> \t{\"init\", module_init, SUPPORT_SUPER_PREFIX},\n\nI did my own spelunking into --super-prefix recently, and went a bit\noverboard, I don't think I'll ever submit all of these, but they're in\nmy avar/git github fork:\n\n\tf445c57490d (submodule--helper: remove unused SUPPORT_SUPER_PREFIX flags, 2022-06-27)\n\tbac3def78e9 (submodule--helper.c: remove unnecessary \", 0\" in init, 2022-06-27)\n\taf03aa2ad40 (submodule--helper.c: create a command dispatch helper, 2022-06-27)\n\t952fdec4cc0 (submodule--helper.c: make \"support super prefix\" a bitfield, not a flag, 2022-06-09)\n\t2d30186e633 (cocci: don't use strvec_pushl() if strvec_push() will do, 2022-06-27)\n\t8aa7e049360 (git.c: die earlier on bad \"--super-prefix\" combined with \"-h\", 2022-06-27)\n\tb0d324e9ad2 (git: make --super-prefix truly internal-only, BUG() on misuse, 2022-06-27)\n\nSo, this is a digressio, but after doing those I figured we could\neventually get rid of --super-prefix, but it'll require some more\nmake-things-a-built-in, or make-things-a-library.\n\nBut I think out of those perhaps you'd be interested in cherry-picking\nf445c57490d (submodule--helper: remove unused SUPPORT_SUPER_PREFIX\nflags, 2022-06-27) before this 4/5? I.e. before adding a new\nSUPPORT_SUPER_PREFIX flag we can remove it from those commands that\ndon't use it, which clears things up a bit.\n\nThe others are all mostly unrelated cleanup, and I'm only noting them in\ncase you're overly curious. A web view for f445c57490d is at:\nhttps://github.com/avar/git/commit/f445c57490d\n"},{"id":"458048","messageId":"kl6l4k04iw5y.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"58074","inReplyTo":"pull.1282.git.git.1656372017.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/5] submodule: remove \"--recursive-prefix\"","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-06-28T16:34:49Z","receivedAt":"2022-06-28T16:40:42Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"\"Glen Choo via GitGitGadget\" writes:\n\n> = Interactions with other series\n>\n> This would have been rebased onto ab/submodule-cleanup, but it's not yet\n> clear to me if that series will be merged first. That series is almost done,\n> but Ævar is still doing some digging on SUPPORT_SUPER_PREFIX [3] and may\n> come back with findings that affect this series.\n>\n> Fortunately, this series is only tangentially related to\n> ab/submodule-cleanup, and the conflicts are quite simple:\n>\n> | this | ab/submodule-cleanup | resolution |\n> |-----------------------------+-------------------------------+---------------------------|\n> | push --super-prefix arg | add new \"ud\" var | keep both |\n> |-----------------------------+-------------------------------+---------------------------|\n> | remove \"--recursive-prefix\" | add \"--checkout\", \"--rebase\", | keep both |\n> | | \"--merge\" | |\n> |-----------------------------+-------------------------------+---------------------------|\n> | add SUPPORT_SUPER_PREFIX | remove SUPPORT_SUPER_PREFIX | keep\n> ab/submodule-cleanup | | to \"git submodule--helper | from \"git\n> submodule--helper\" | | | update\" | | |\n\nHm, maybe I'm not using GGG correctly, but these whitespace issues seem\npretty common (Markdown collapsing the whitespace maybe?). Maybe I need\nto blockquote the monospaced stuff.\n\nHere's the original table:\n\n| this | ab/submodule-cleanup | resolution |\n|-----------------------------+-------------------------------+---------------------------|\n| push --super-prefix arg | add new \"ud\" var | keep both |\n|-----------------------------+-------------------------------+---------------------------|\n| remove \"--recursive-prefix\" | add \"--checkout\", \"--rebase\", | keep both |\n| | \"--merge\" | |\n|-----------------------------+-------------------------------+---------------------------|\n| add SUPPORT_SUPER_PREFIX | remove SUPPORT_SUPER_PREFIX | keep ab/submodule-cleanup |\n| to \"git submodule--helper | from \"git submodule--helper\" | |\n| update\" | | |\n\n(For the org-mode users: org-table-align fixes everything except the\nlast row, presumably because of the rogue line breaks.)\n"},{"id":"458054","messageId":"kl6ly1xghetp.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"58074","inReplyTo":"220628.861qv9gpg5.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 1/5] submodule--helper update: use display path helper","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-06-28T17:34:42Z","receivedAt":"2022-06-28T17:34:51Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"\nThanks! Everything not mentioned here seems like an obviously good\nsuggestion. I especially appreciate the translation/compiler flow\nanalysis pieces (I hadn't considered either).\n\nÆvar Arnfjörð Bjarmason writes:\n\n> On Mon, Jun 27 2022, Glen Choo via GitGitGadget wrote:\n>\n>> From: Glen Choo \n>>\n>> There are two locations in prepare_to_clone_next_submodule() that\n>> manually calculate the submodule display path, but should just use\n>> do_get_submodule_displaypath() for consistency.\n>>\n>> Do this replacement and reorder the code slightly to avoid computing\n>> the display path twice.\n>>\n>> This code was never tested, and adding tests shows that both these sites\n>> have been computing the display path incorrectly ever since they were\n>> introduced in 48308681b0 (git submodule update: have a dedicated helper\n>> for cloning, 2016-02-29) [1]:\n>\n> I think the tests should really be split up as their own preceding\n> commit, so we're assured that this \"git rebase -x 'make test'\"'s\n> cleanly.\n\nHm, I'm not sure how this changes the result of \"git rebase -x 'make\ntest'\", since these tests will fail prior to this patch. I could modify\nthe tests to demonstrate the broken behavior though, which would save me\nsome effort in describing it in the commit message.\n\n>> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\n>> index 43f779d751c..e1dc3b1041b 100755\n>> --- a/t/t7406-submodule-update.sh\n>> +++ b/t/t7406-submodule-update.sh\n>> @@ -1116,4 +1116,63 @@ test_expect_success 'submodule update --filter sets partial clone settings' '\n>> \ttest_cmp_config -C super-filter/submodule blob:none remote.origin.partialclonefilter\n>> '\n>> \n>> +# NEEDSWORK: Clean up the tests so that we can reuse the test setup.\n>> +# Don't reuse the existing repos because the earlier tests have\n>> +# intentionally disruptive configurations.\n>\n> Yeah this does seem a bit repetative...\n>\n>> +test_expect_success 'setup clean recursive superproject' '\n>> +\tgit init bottom &&\n>> +\ttest_commit -C bottom \"bottom\" &&\n>> +\tgit init middle &&\n>> +\tgit -C middle submodule add ../bottom bottom &&\n>> +\tgit -C middle commit -m \"middle\" &&\n>> +\tgit init top &&\n>> +\tgit -C top submodule add ../middle middle &&\n>> +\tgit -C top commit -m \"top\" &&\n>> +\tgit clone --recurse-submodules top top-clean\n>> +'\n>> +\n>> +test_expect_success 'submodule update should skip unmerged submodules' '\n>> +\ttest_when_finished \"rm -fr top-cloned\" &&\n>> +\tcp -r top-clean top-cloned &&\n>> +\n>> +\t# Create an upstream commit in each repo\n>> +\ttest_commit -C bottom upstream_commit &&\n>> +\t(cd middle &&\n>> +\t git -C bottom fetch &&\n>> +\t git -C bottom checkout -f FETCH_HEAD &&\n>> +\t git add bottom &&\n>> +\t git commit -m \"upstream_commit\"\n>> +\t) &&\n>> +\t(cd top &&\n>> +\t git -C middle fetch &&\n>> +\t git -C middle checkout -f FETCH_HEAD &&\n>> +\t git add middle &&\n>> +\t git commit -m \"upstream_commit\"\n>> +\t) &&\n>\n> E.g. here the mixture of \"cd\" and \"-C\" is a bit odd, can't we just use:\n>\n> git -C middle/bottom ...\n> ...\n> git -C middle add \n\n\nYeah admittedly this is quite odd. I was hoping that this would be\nmore readable than big chunks of \"-C\", since I personally find those\nquite difficult to follow when the directory changes from line to line.\nBut, maybe it's just better to be consistent in just using \"-C\", and\nI'll find some way of organizing the lines that keeps them readable.\n\n> I also wonder if this can't be a for-loop with the right params\n\nI suppose so, though I think there isn't enough repetition here to\njustify the readability/correctness trade-off.\n"},{"id":"458059","messageId":"kl6lv8skhcy8.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"58074","inReplyTo":"220628.86wnd1f9t1.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 4/5] submodule--helper update: use --super-prefix","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-06-28T18:15:11Z","receivedAt":"2022-06-28T18:15:22Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> On Mon, Jun 27 2022, Glen Choo via GitGitGadget wrote:\n>\n>> From: Glen Choo \n>>\n>> Unlike the other subcommands, \"git submodule--helper update\" uses the\n>> \"--recursive-prefix\" flag instead of \"--super-prefix\". The two flags are\n>> otherwise identical (they only serve to compute the 'display path' of a\n>> submodule), except that there is a dedicated helper function to get the\n>> value of \"--super-prefix\".\n>\n> This is a good change, it was slightly confusing that --recursive-prefix\n> is left in git-submodule.sh after this, but then I remembered that I\n> removed it in my ab/submodule-cleanup, and you were presumably trying to\n> avoid the conflict.\n>\n> Still, I think it's probably better to either base this on my series\n> (re-roll incoming), or take make this truly stand-alone, and have Junio\n> sort out the minor conflict.\n\nAh good point. This was an oversight actually; I initially based this\noff ab/submodule-cleanup, but decided to make it standalone. Thanks for\nspotting the mistake.\n\nAt any rate, I'm quite sure that ab/submodule-cleanup v5 is ready for\n'next', so I'll rebase this.\n\n>> static void update_data_to_args(struct update_data *update_data, struct strvec *args)\n>> {\n>> -\tstrvec_pushl(args, \"submodule--helper\", \"update\", \"--recursive\", NULL);\n>> -\tstrvec_pushf(args, \"--jobs=%d\", update_data->max_jobs);\n>> \tif (update_data->displaypath)\n>> -\t\tstrvec_pushf(args, \"--recursive-prefix=%s/\",\n>> +\t\tstrvec_pushf(args, \"--super-prefix=%s/\",\n>> \t\t\t update_data->displaypath);\n>> +\tstrvec_pushl(args, \"submodule--helper\", \"update\", \"--recursive\", NULL);\n>> +\tstrvec_pushf(args, \"--jobs=%d\", update_data->max_jobs);\n>\n> I did a double-take at this, but it's just one of these cases where\n> \"diff\" is being overly helpful in trying to find us the most minimal\n> diff possible :)\n\nYes. Sigh..\n\nIt looks like \"--histogram\" produces the diff I'd want:\n\n enum submodule_update_type update_type = update_data->update_default;\n\n + if (update_data->displaypath)\n + strvec_pushf(args, \"--super-prefix=%s/\",\n + update_data->displaypath);\n strvec_pushl(args, \"submodule--helper\", \"update\", \"--recursive\", NULL);\n strvec_pushf(args, \"--jobs=%d\", update_data->max_jobs);\n - if (update_data->displaypath)\n - strvec_pushf(args, \"--recursive-prefix=%s/\",\n - update_data->displaypath);\n\nbut that probably means I'd need to give up on GGG :/ (which I've grown\nto like despite its shortcomings).\n\n>> @@ -3352,9 +3342,9 @@ struct cmd_struct {\n>> static struct cmd_struct commands[] = {\n>> \t{\"list\", module_list, 0},\n>> \t{\"name\", module_name, 0},\n>> -\t{\"clone\", module_clone, 0},\n>> +\t{\"clone\", module_clone, SUPPORT_SUPER_PREFIX},\n>> \t{\"add\", module_add, SUPPORT_SUPER_PREFIX},\n>> -\t{\"update\", module_update, 0},\n>> +\t{\"update\", module_update, SUPPORT_SUPER_PREFIX},\n>> \t{\"resolve-relative-url-test\", resolve_relative_url_test, 0},\n>> \t{\"foreach\", module_foreach, SUPPORT_SUPER_PREFIX},\n>> \t{\"init\", module_init, SUPPORT_SUPER_PREFIX},\n>\n> I did my own spelunking into --super-prefix recently, and went a bit\n> overboard, I don't think I'll ever submit all of these, but they're in\n> my avar/git github fork:\n>\n> \tf445c57490d (submodule--helper: remove unused SUPPORT_SUPER_PREFIX flags, 2022-06-27)\n> \tbac3def78e9 (submodule--helper.c: remove unnecessary \", 0\" in init, 2022-06-27)\n> \taf03aa2ad40 (submodule--helper.c: create a command dispatch helper, 2022-06-27)\n> \t952fdec4cc0 (submodule--helper.c: make \"support super prefix\" a bitfield, not a flag, 2022-06-09)\n> \t2d30186e633 (cocci: don't use strvec_pushl() if strvec_push() will do, 2022-06-27)\n> \t8aa7e049360 (git.c: die earlier on bad \"--super-prefix\" combined with \"-h\", 2022-06-27)\n> \tb0d324e9ad2 (git: make --super-prefix truly internal-only, BUG() on misuse, 2022-06-27)\n>\n> So, this is a digressio, but after doing those I figured we could\n> eventually get rid of --super-prefix, but it'll require some more\n> make-things-a-built-in, or make-things-a-library.\n>\n> But I think out of those perhaps you'd be interested in cherry-picking\n> f445c57490d (submodule--helper: remove unused SUPPORT_SUPER_PREFIX\n> flags, 2022-06-27) before this 4/5? I.e. before adding a new\n> SUPPORT_SUPER_PREFIX flag we can remove it from those commands that\n> don't use it, which clears things up a bit.\n>\n> The others are all mostly unrelated cleanup, and I'm only noting them in\n> case you're overly curious. A web view for f445c57490d is at:\n> https://github.com/avar/git/commit/f445c57490d\n\nVery interesting, thanks for sharing :) I'll take your suggestion to\ncherry-pick f445c57490d.\n"},{"id":"458254","messageId":"pull.1282.v2.git.git.1656623978.gitgitgadget@gmail.com","threadId":"58074","inReplyTo":"pull.1282.git.git.1656372017.gitgitgadget@gmail.com","subject":"[PATCH v2 00/18] submodule: remove \"--recursive-prefix\"","fromName":"Glen Choo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-30T21:19:20Z","receivedAt":"2022-06-30T21:19:45Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Thanks Ævar for the feedback! I've incorporated all of the suggestions,\nexcept breaking up the tests in 1/6 into their own patch - I wasn't\nconvinced of the value of having prescriptive tests (with\ntest_expect_failure), and I found it difficult to discuss descriptive tests\n(with test_expect_success) without also having the code in the same diff.\nFWIW, this version (and the previous one) \"git rebase -x 'make test'\"\ncleanly :)\n\nNote for Junio: this version is based on ab/submodule-cleanup (and so will\nfuture versions).\n\n= Description\n\nThis series is a refactor of \"git submodule--helper update\" that replaces\n\"--recursive-prefix\" with \"--super-prefix\" (see Background). This was\ninitially motivated by:\n\n * Junio's suggestion to simplify the code [1] (in response to a memory leak\n found by Johannes Schindelin [2]).\n * A desire to use the module_list API + get_submodule_displaypath() outside\n of builtin/submodule--helper.c (I expect to use this to check out\n branches in each submodule).\n\nBut it also happens to remove some overly complicated/duplicated code that\nwas literally converted from shell :)\n\n= Background\n\nWhen recursing into nested submodules, Git commands keep track of the path\nfrom the superproject to the submodule in order to print a \"display path\" to\nthe user, e.g.\n\nSubmodule path '../super/sub/nested-sub': checked out 'abcdef'\n\nFor historical reasons, \"git submodule--helper update\" uses\n\"--recursive-prefix\" for this purpose, but it should use \"--super-prefix\"\ninstead because:\n\n * That's what every other command uses (not just submodule--helper\n subcommands).\n * Using the \"--super-prefix\" helper functions makes the \"git\n submodule--helper update\" code simpler\n\n= Patch organization\n\n * Patch 1/6 makes sure that display paths are only computed using display\n path helper functions ([do_]get_submodule_displaypath()) and fixes some\n display paths that we never realized were broken.\n * Patches 2-3/6 simplify and deduplicate some display path computations.\n * Patch 4/6 (authored by Ævar) removes SUPPORT_SUPER_PREFIX where it's not\n needed.\n * This doesn't affect correctness, but we want to do this eventually, so\n do it now to make 5/6 a bit cleaner.\n * Patch 5/6 replaces \"--recursive-prefix\" with \"--super-prefix\", making\n do_get_submodule_displaypath() obsolete.\n * GGG outputs an odd diff; I recommend viewing it with \"--histogram\"\n * Patch 6/6 removes do_get_submodule_displaypath().\n\n= Series history\n\nChanges in v2:\n\n * Rebase onto ab/submodule-cleanup (previously master)\n * Cherry pick https://github.com/avar/git/commit/f445c57490d into 4/6\n * Style fixes in .c and tests\n\n= Future work\n\nAt the end of this series, get_submodule_displaypath() can be moved to\nsubmodule.h, which would make submodule.c:get_super_prefix_or_empty()\nobsolete. I have a patch that demonstrates this (CI run: [4]), but I decided\nto keep this series more focused on \"git submodule--helper update\" so that\nit's easier to review.\n\n[1] https://lore.kernel.org/git/xmqq35g5xmmv.fsf@gitster.g [2]\nhttps://lore.kernel.org/git/877a45867ae368bf9e053caedcb6cf421e02344d.1655336146.git.gitgitgadget@gmail.com\n[3]\nhttps://lore.kernel.org/git/patch-v3-02.12-082e015781e-20220622T142012Z-avarab@gmail.com\n[4] https://github.com/chooglen/git/actions/runs/2572557584\n\nGlen Choo (6):\n submodule--helper: eliminate internal \"--update\" option\n submodule--helper update: use display path helper\n submodule--helper: don't recreate recursive prefix\n submodule--helper: use correct display path helper\n submodule--helper update: use --super-prefix\n submodule--helper: remove display path helper\n\nÆvar Arnfjörð Bjarmason (12):\n git-submodule.sh: remove unused sanitize_submodule_env()\n git-submodule.sh: remove unused $prefix variable\n git-submodule.sh: make the \"$cached\" variable a boolean\n git-submodule.sh: remove unused top-level \"--branch\" argument\n submodule--helper: have --require-init imply --init\n submodule update: remove \"-v\" option\n submodule--helper: rename \"absorb-git-dirs\" to \"absorbgitdirs\"\n submodule--helper: report \"submodule\" as our name in some \"-h\" output\n submodule--helper: understand --checkout, --merge and --rebase\n synonyms\n git-submodule.sh: use \"$quiet\", not \"$GIT_QUIET\"\n git-sh-setup.sh: remove \"say\" function, change last users\n submodule--helper: remove unused SUPPORT_SUPER_PREFIX flags\n\n builtin/submodule--helper.c | 161 +++++++++++++++------------------\n contrib/subtree/git-subtree.sh | 15 ++-\n git-instaweb.sh | 2 +-\n git-sh-setup.sh | 16 ----\n git-submodule.sh | 88 +++++++-----------\n submodule.c | 2 +-\n t/t7406-submodule-update.sh | 58 +++++++++++-\n 7 files changed, 175 insertions(+), 167 deletions(-)\n\n\nbase-commit: 8168d5e9c23ed44ae3d604f392320d66556453c9\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1282%2Fchooglen%2Fsubmodule%2Fremove-recursive-prefix-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1282/chooglen/submodule/remove-recursive-prefix-v2\nPull-Request: https://github.com/git/git/pull/1282\n\nRange-diff vs v1:\n\n -: ----------- > 1: 85775255f18 git-submodule.sh: remove unused sanitize_submodule_env()\n -: ----------- > 2: 960fad98e8a git-submodule.sh: remove unused $prefix variable\n -: ----------- > 3: 757d0927976 git-submodule.sh: make the \"$cached\" variable a boolean\n -: ----------- > 4: da3aae9e847 git-submodule.sh: remove unused top-level \"--branch\" argument\n -: ----------- > 5: d9c7f69aaa6 submodule--helper: have --require-init imply --init\n -: ----------- > 6: 0d68ee723e5 submodule update: remove \"-v\" option\n -: ----------- > 7: 6e556c412e9 submodule--helper: rename \"absorb-git-dirs\" to \"absorbgitdirs\"\n -: ----------- > 8: 36d45163b6d submodule--helper: report \"submodule\" as our name in some \"-h\" output\n -: ----------- > 9: 8f12108c295 submodule--helper: understand --checkout, --merge and --rebase synonyms\n -: ----------- > 10: b788fc671bf submodule--helper: eliminate internal \"--update\" option\n -: ----------- > 11: 2eec4637397 git-submodule.sh: use \"$quiet\", not \"$GIT_QUIET\"\n -: ----------- > 12: 5b893f7d81e git-sh-setup.sh: remove \"say\" function, change last users\n 1: 473548f2fa4 ! 13: 64c138df196 submodule--helper update: use display path helper\n @@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const st\n -\tstruct strbuf displaypath_sb = STRBUF_INIT;\n \tstruct strbuf sb = STRBUF_INIT;\n -\tconst char *displaypath = NULL;\n -+\tchar *displaypath = NULL;\n ++\tchar *displaypath;\n \tint needs_cloning = 0;\n \tint need_free_url = 0;\n \n @@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const st\n -\t\telse\n -\t\t\tstrbuf_addstr(&sb, ce->name);\n -\t\tstrbuf_addf(out, _(\"Skipping unmerged submodule %s\"), sb.buf);\n --\t\tstrbuf_addch(out, '\\n');\n -+\t\tstrbuf_addf(out, _(\"Skipping unmerged submodule '%s'\\n\"), displaypath);\n ++\t\tstrbuf_addf(out, _(\"Skipping unmerged submodule %s\"), displaypath);\n + \t\tstrbuf_addch(out, '\\n');\n \t\tgoto cleanup;\n \t}\n \n @@ t/t7406-submodule-update.sh: test_expect_success 'submodule update --filter sets\n +\ttest_when_finished \"rm -fr top-cloned\" &&\n +\tcp -r top-clean top-cloned &&\n +\n -+\t# Create an upstream commit in each repo\n ++\t# Create an upstream commit in each repo, starting with bottom\n +\ttest_commit -C bottom upstream_commit &&\n -+\t(cd middle &&\n -+\t git -C bottom fetch &&\n -+\t git -C bottom checkout -f FETCH_HEAD &&\n -+\t git add bottom &&\n -+\t git commit -m \"upstream_commit\"\n -+\t) &&\n -+\t(cd top &&\n -+\t git -C middle fetch &&\n -+\t git -C middle checkout -f FETCH_HEAD &&\n -+\t git add middle &&\n -+\t git commit -m \"upstream_commit\"\n -+\t) &&\n ++\t# Create middle commit\n ++\tgit -C middle/bottom fetch &&\n ++\tgit -C middle/bottom checkout -f FETCH_HEAD &&\n ++\tgit -C middle add bottom &&\n ++\tgit -C middle commit -m \"upstream_commit\" &&\n ++\t# Create top commit\n ++\tgit -C top/middle fetch &&\n ++\tgit -C top/middle checkout -f FETCH_HEAD &&\n ++\tgit -C top add middle &&\n ++\tgit -C top commit -m \"upstream_commit\" &&\n +\n +\t# Create a downstream conflict\n -+\t(cd top-cloned/middle &&\n -+\t test_commit -C bottom downstream_commit &&\n -+\t git add bottom &&\n -+\t git commit -m \"downstream_commit\" &&\n -+\t git fetch --recurse-submodules origin &&\n -+\t test_must_fail git merge origin/main\n -+\t) &&\n ++\ttest_commit -C top-cloned/middle/bottom downstream_commit &&\n ++\tgit -C top-cloned/middle add bottom &&\n ++\tgit -C top-cloned/middle commit -m \"downstream_commit\" &&\n ++\tgit -C top-cloned/middle fetch --recurse-submodules origin &&\n ++\ttest_must_fail git -C top-cloned/middle merge origin/main &&\n ++\n +\t# Make the update of \"middle\" a no-op, otherwise we error out\n +\t# because of its unmerged state\n +\ttest_config -C top-cloned submodule.middle.update !true &&\n +\tgit -C top-cloned submodule update --recursive 2>actual.err &&\n -+\tgrep \"Skipping unmerged submodule .middle/bottom.\" actual.err\n ++\tgrep -F \"Skipping unmerged submodule middle/bottom\" actual.err\n +'\n +\n +test_expect_success 'submodule update --recursive skip submodules with strategy=none' '\n @@ t/t7406-submodule-update.sh: test_expect_success 'submodule update --filter sets\n +\ttest_commit -C top-cloned/middle/bottom downstream_commit &&\n +\tgit -C top-cloned/middle config submodule.bottom.update none &&\n +\tgit -C top-cloned submodule update --recursive 2>actual.err &&\n -+\tgrep \"Skipping submodule .middle/bottom.\" actual.err\n ++\tgrep -F \"Skipping submodule ${SQ}middle/bottom${SQ}\" actual.err\n +'\n +\n test_done\n 2: 618053730e1 ! 14: d3e803a4630 submodule--helper: don't recreate recursive prefix\n @@ Commit message\n \n ## builtin/submodule--helper.c ##\n @@ builtin/submodule--helper.c: static void update_data_to_args(struct update_data *update_data, struct strvec *\n - {\n + \n \tstrvec_pushl(args, \"submodule--helper\", \"update\", \"--recursive\", NULL);\n \tstrvec_pushf(args, \"--jobs=%d\", update_data->max_jobs);\n -\tif (update_data->recursive_prefix)\n 3: 7cd1c46f350 = 15: 1f7cf6ffaf1 submodule--helper: use correct display path helper\n -: ----------- > 16: 85e65f143b6 submodule--helper: remove unused SUPPORT_SUPER_PREFIX flags\n 4: 57988287fc0 ! 17: 1d8b6b244dc submodule--helper update: use --super-prefix\n @@ builtin/submodule--helper.c: struct submodule_update_clone {\n \tconst char *prefix;\n -\tconst char *recursive_prefix;\n \tconst char *displaypath;\n - \tconst char *update_default;\n + \tenum submodule_update_type update_default;\n \tstruct object_id suboid;\n @@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n \n @@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const st\n +\t\t\t\t\t\t get_super_prefix());\n \n \tif (ce_stage(ce)) {\n - \t\tstrbuf_addf(out, _(\"Skipping unmerged submodule '%s'\\n\"), displaypath);\n -@@ builtin/submodule--helper.c: static void ensure_core_worktree(const char *path)\n - \n - static void update_data_to_args(struct update_data *update_data, struct strvec *args)\n + \t\tstrbuf_addf(out, _(\"Skipping unmerged submodule %s\"), displaypath);\n +@@ builtin/submodule--helper.c: static void update_data_to_args(struct update_data *update_data, struct strvec *\n {\n + \tenum submodule_update_type update_type = update_data->update_default;\n + \n -\tstrvec_pushl(args, \"submodule--helper\", \"update\", \"--recursive\", NULL);\n -\tstrvec_pushf(args, \"--jobs=%d\", update_data->max_jobs);\n \tif (update_data->displaypath)\n @@ builtin/submodule--helper.c: static int module_update(int argc, const char **arg\n -\t\t\t 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, \"update\", &opt.update_default,\n - \t\t\t N_(\"string\"),\n - \t\t\t N_(\"rebase, merge, checkout or none\")),\n + \t\tOPT_SET_INT(0, \"checkout\", &opt.update_default,\n + \t\t\tN_(\"use the 'checkout' update strategy (default)\"),\n + \t\t\tSM_UPDATE_CHECKOUT),\n @@ builtin/submodule--helper.c: static int module_update(int argc, const char **argv, const char *prefix)\n \t\t\tmodule_list_active(&list);\n \n @@ builtin/submodule--helper.c: struct cmd_struct {\n \t{\"name\", module_name, 0},\n -\t{\"clone\", module_clone, 0},\n +\t{\"clone\", module_clone, SUPPORT_SUPER_PREFIX},\n - \t{\"add\", module_add, SUPPORT_SUPER_PREFIX},\n + \t{\"add\", module_add, 0},\n -\t{\"update\", module_update, 0},\n +\t{\"update\", module_update, SUPPORT_SUPER_PREFIX},\n \t{\"resolve-relative-url-test\", resolve_relative_url_test, 0},\n \t{\"foreach\", module_foreach, SUPPORT_SUPER_PREFIX},\n - \t{\"init\", module_init, SUPPORT_SUPER_PREFIX},\n + \t{\"init\", module_init, 0},\n 5: 9fa13380b02 ! 18: a21e8cd174d submodule--helper: remove display path helper\n @@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const st\n +\t\tget_submodule_displaypath(ce->name, suc->update_data->prefix);\n \n \tif (ce_stage(ce)) {\n - \t\tstrbuf_addf(out, _(\"Skipping unmerged submodule '%s'\\n\"), displaypath);\n + \t\tstrbuf_addf(out, _(\"Skipping unmerged submodule %s\"), displaypath);\n @@ builtin/submodule--helper.c: static int update_submodule(struct update_data *update_data)\n {\n \tensure_core_worktree(update_data->sm_path);\n\n-- \ngitgitgadget\n"},{"id":"458255","messageId":"85775255f18e0a6a6b2e65394bc18ec440dba99d.1656623978.git.gitgitgadget@gmail.com","threadId":"58074","inReplyTo":"pull.1282.v2.git.git.1656623978.gitgitgadget@gmail.com","subject":"[PATCH v2 01/18] git-submodule.sh: remove unused sanitize_submodule_env()","fromName":"Ævar Arnfjörð Bjarmason via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-30T21:19:21Z","receivedAt":"2022-06-30T21:19:49Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?=\n \n\nThe sanitize_submodule_env() function was last used before\nb3c5f5cb048 (submodule: move core cmd_update() logic to C,\n2022-03-15), let's remove it.\n\nThis also allows us to remove clear_local_git_env() from\ngit-sh-setup.sh. That function hasn't been documented in\nDocumentation/git-sh-setup.sh, and since 14111fc4927 (git: submodule\nhonor -c credential.* from command line, 2016-02-29) it had only been\nused in the sanitize_submodule_env() function being removed here.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \nSigned-off-by: Junio C Hamano \n---\n git-sh-setup.sh | 7 -------\n git-submodule.sh | 11 -----------\n 2 files changed, 18 deletions(-)\n\ndiff --git a/git-sh-setup.sh b/git-sh-setup.sh\nindex d92df37e992..ecb60d9e3cb 100644\n--- a/git-sh-setup.sh\n+++ b/git-sh-setup.sh\n@@ -285,13 +285,6 @@ get_author_ident_from_commit () {\n \tparse_ident_from_commit author AUTHOR\n }\n \n-# Clear repo-local GIT_* environment variables. Useful when switching to\n-# another repository (e.g. when entering a submodule). See also the env\n-# list in git_connect()\n-clear_local_git_env() {\n-\tunset $(git rev-parse --local-env-vars)\n-}\n-\n # Generate a virtual base file for a two-file merge. Uses git apply to\n # remove lines from $1 that are not in $2, leaving only common lines.\n create_virtual_base() {\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex fd0b4a2c947..bc436c4ca47 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -56,17 +56,6 @@ isnumber()\n \tn=$(($1 + 0)) 2>/dev/null && test \"$n\" = \"$1\"\n }\n \n-# Sanitize the local git environment for use within a submodule. We\n-# can't simply use clear_local_git_env since we want to preserve some\n-# of the settings from GIT_CONFIG_PARAMETERS.\n-sanitize_submodule_env()\n-{\n-\tsave_config=$GIT_CONFIG_PARAMETERS\n-\tclear_local_git_env\n-\tGIT_CONFIG_PARAMETERS=$save_config\n-\texport GIT_CONFIG_PARAMETERS\n-}\n-\n #\n # Add a new submodule to the working tree, .gitmodules and the index\n #\n-- \ngitgitgadget\n\n"},{"id":"458256","messageId":"960fad98e8a8d4c123bb5040f616f856cb9925ea.1656623978.git.gitgitgadget@gmail.com","threadId":"58074","inReplyTo":"pull.1282.v2.git.git.1656623978.gitgitgadget@gmail.com","subject":"[PATCH v2 02/18] git-submodule.sh: remove unused $prefix variable","fromName":"Ævar Arnfjörð Bjarmason via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-30T21:19:22Z","receivedAt":"2022-06-30T21:19:53Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?=\n \n\nRemove the $prefix variable which isn't used anymore, and hasn't been\nsince b3c5f5cb048 (submodule: move core cmd_update() logic to C,\n2022-03-15).\n\nBefore that we'd use it to invoke \"git submodule--helper\" with the\n\"--recursive-prefix\" option, but since b3c5f5cb048 that \"git\nsubmodule--helper\" option is only used when it invokes itself.\n\nSo the \"--recursive-prefix\" option is still in use, but at this point\nonly when the helper invokes itself during submodule recursion. See\nthe \"--recursive-prefix\" option added in\nc51f8f94e5b (submodule--helper: run update procedures from C,\n2021-08-24).\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \nSigned-off-by: Junio C Hamano \n---\n git-submodule.sh | 6 ++----\n 1 file changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex bc436c4ca47..53847bbf6e2 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -41,7 +41,6 @@ files=\n remote=\n nofetch=\n update=\n-prefix=\n custom_name=\n depth=\n progress=\n@@ -127,7 +126,7 @@ cmd_add()\n \t\tusage\n \tfi\n \n-\tgit ${wt_prefix:+-C \"$wt_prefix\"} ${prefix:+--super-prefix \"$prefix\"} submodule--helper add ${GIT_QUIET:+--quiet} ${force:+--force} ${progress:+\"--progress\"} ${branch:+--branch \"$branch\"} ${reference_path:+--reference \"$reference_path\"} ${dissociate:+--dissociate} ${custom_name:+--name \"$custom_name\"} ${depth:+\"$depth\"} -- \"$@\"\n+\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper add ${GIT_QUIET:+--quiet} ${force:+--force} ${progress:+\"--progress\"} ${branch:+--branch \"$branch\"} ${reference_path:+--reference \"$reference_path\"} ${dissociate:+--dissociate} ${custom_name:+--name \"$custom_name\"} ${depth:+\"$depth\"} -- \"$@\"\n }\n \n #\n@@ -189,7 +188,7 @@ cmd_init()\n \t\tshift\n \tdone\n \n-\tgit ${wt_prefix:+-C \"$wt_prefix\"} ${prefix:+--super-prefix \"$prefix\"} submodule--helper init ${GIT_QUIET:+--quiet} -- \"$@\"\n+\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper init ${GIT_QUIET:+--quiet} -- \"$@\"\n }\n \n #\n@@ -346,7 +345,6 @@ cmd_update()\n \t\t${init:+--init} \\\n \t\t${nofetch:+--no-fetch} \\\n \t\t${wt_prefix:+--prefix \"$wt_prefix\"} \\\n-\t\t${prefix:+--recursive-prefix \"$prefix\"} \\\n \t\t${update:+--update \"$update\"} \\\n \t\t${reference:+\"$reference\"} \\\n \t\t${dissociate:+\"--dissociate\"} \\\n-- \ngitgitgadget\n\n"},{"id":"458257","messageId":"757d0927976cf9d59a050f8f9ce1fe54d4dff653.1656623978.git.gitgitgadget@gmail.com","threadId":"58074","inReplyTo":"pull.1282.v2.git.git.1656623978.gitgitgadget@gmail.com","subject":"[PATCH v2 03/18] git-submodule.sh: make the \"$cached\" variable a boolean","fromName":"Ævar Arnfjörð Bjarmason via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-30T21:19:23Z","receivedAt":"2022-06-30T21:19:54Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?=\n \n\nRemove the assignment of \"$1\" to the \"$cached\" variable. As seen in\nthe initial implementation in 70c7ac22de6 (Add git-submodule command,\n2007-05-26) we only need to keep track of if we've seen the --cached\noption, not save the \"--cached\" string for later use.\n\nIn 28f9af5d25e (git-submodule summary: code framework, 2008-03-11)\n\"$1\" was assigned to it, but since there was no reason to do so let's\nstop doing it. This trivial change will make it easier to reason about\nan eventual change that'll remove the cmd_summary() function in favor\nof dispatching to \"git submodule--helper summary\" directly.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \nSigned-off-by: Junio C Hamano \n---\n git-submodule.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 53847bbf6e2..b99a00d9f84 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -446,7 +446,7 @@ cmd_summary() {\n \tdo\n \t\tcase \"$1\" in\n \t\t--cached)\n-\t\t\tcached=\"$1\"\n+\t\t\tcached=1\n \t\t\t;;\n \t\t--files)\n \t\t\tfiles=\"$1\"\n@@ -583,7 +583,7 @@ do\n \t\tbranch=\"$2\"; shift\n \t\t;;\n \t--cached)\n-\t\tcached=\"$1\"\n+\t\tcached=1\n \t\t;;\n \t--)\n \t\tbreak\n-- \ngitgitgadget\n\n"},{"id":"458258","messageId":"da3aae9e8476af3b23363d39dba86b679c14a498.1656623978.git.gitgitgadget@gmail.com","threadId":"58074","inReplyTo":"pull.1282.v2.git.git.1656623978.gitgitgadget@gmail.com","subject":"[PATCH v2 04/18] git-submodule.sh: remove unused top-level \"--branch\" argument","fromName":"Ævar Arnfjörð Bjarmason via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-30T21:19:24Z","receivedAt":"2022-06-30T21:19:55Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?=\n \n\nIn 5c08dbbdf1a (git-submodule: fix subcommand parser, 2008-01-15) the\n\"--branch\" option was supported as an option to \"git submodule\"\nitself, i.e. \"git submodule --branch\" as a side-effect of its\nimplementation.\n\nThen in b57e8119e6e (submodule: teach set-branch subcommand,\n2019-02-08) when the \"set-branch\" subcommand was added the assertion\nthat we shouldn't have \"--branch\" anywhere except as an argument to\n\"add\" and \"set-branch\" was copy/pasted from the adjacent check for\n\"--cache\" added (or rather modified) in 496eeeb19b9 (git-submodule.sh:\navoid \"test -a/-o \", 2014-06-10).\n\nBut there's been a logic error in that check, which at a glance looked\nlike it should be supporting:\n\n git submodule --branch (add | set-branch) []\n\nBut due to \"||\" in the condition (as opposed to \"&&\" for \"--cache\") if\nwe have \"--branch\" here already we'll emit usage, even for \"add\" and\n\"set-branch\".\n\nSo in addition to never having documented this form, it hasn't worked\nsince b57e8119e6e was released with v2.22.0.\n\nSo it's safe to remove this code. I.e. we don't want to support the\nform noted above, but only:\n\n git submodule (add | set-branch) --branch []\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \nSigned-off-by: Junio C Hamano \n---\n git-submodule.sh | 14 --------------\n 1 file changed, 14 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex b99a00d9f84..20fc1b620fa 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -574,14 +574,6 @@ do\n \t-q|--quiet)\n \t\tGIT_QUIET=1\n \t\t;;\n-\t-b|--branch)\n-\t\tcase \"$2\" in\n-\t\t'')\n-\t\t\tusage\n-\t\t\t;;\n-\t\tesac\n-\t\tbranch=\"$2\"; shift\n-\t\t;;\n \t--cached)\n \t\tcached=1\n \t\t;;\n@@ -609,12 +601,6 @@ then\n fi\n fi\n \n-# \"-b branch\" is accepted only by \"add\" and \"set-branch\"\n-if test -n \"$branch\" && (test \"$command\" != add || test \"$command\" != set-branch)\n-then\n-\tusage\n-fi\n-\n # \"--cached\" is accepted only by \"status\" and \"summary\"\n if test -n \"$cached\" && test \"$command\" != status && test \"$command\" != summary\n then\n-- \ngitgitgadget\n\n"},{"id":"458259","messageId":"d9c7f69aaa6b001949e9d2b693c22c595cc9d0d6.1656623978.git.gitgitgadget@gmail.com","threadId":"58074","inReplyTo":"pull.1282.v2.git.git.1656623978.gitgitgadget@gmail.com","subject":"[PATCH v2 05/18] submodule--helper: have --require-init imply --init","fromName":"Ævar Arnfjörð Bjarmason via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-30T21:19:25Z","receivedAt":"2022-06-30T21:19:56Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?=\n \n\nAdjust code added in 0060fd1511b (clone --recurse-submodules: prevent\nname squatting on Windows, 2019-09-12) to have the internal\n--require-init option imply --init, rather than having\n\"git-submodule.sh\" add it implicitly.\n\nThis change doesn't make any difference now, but eliminates another\nspecial-case where \"git submodule--helper update\"'s behavior was\ndifferent from \"git submodule update\". This will make it easier to\neventually replace the cmd_update() function in git-submodule.sh.\n\nWe'll still need to keep the distinction between \"--init\" and\n\"--require-init\" in git-submodule.sh. Once cmd_update() gets\nre-implemented in C we'll be able to change variables and other code\nrelated to that, but not yet.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \nSigned-off-by: Junio C Hamano \n---\n builtin/submodule--helper.c | 5 ++++-\n git-submodule.sh | 1 -\n 2 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 5c77dfcffee..f0702d0cfa2 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2618,7 +2618,7 @@ static int module_update(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOL(0, \"progress\", &opt.progress,\n \t\t\t N_(\"force cloning progress\")),\n \t\tOPT_BOOL(0, \"require-init\", &opt.require_init,\n-\t\t\t N_(\"disallow cloning into non-empty directory\")),\n+\t\t\t N_(\"disallow cloning into non-empty directory, implies --init\")),\n \t\tOPT_BOOL(0, \"single-branch\", &opt.single_branch,\n \t\t\t N_(\"clone only one branch, HEAD or --branch\")),\n \t\tOPT_PARSE_LIST_OBJECTS_FILTER(&filter_options),\n@@ -2642,6 +2642,9 @@ static int module_update(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, module_update_options,\n \t\t\t git_submodule_helper_usage, 0);\n \n+\tif (opt.require_init)\n+\t\topt.init = 1;\n+\n \tif (filter_options.choice && !opt.init) {\n \t\tusage_with_options(git_submodule_helper_usage,\n \t\t\t\t module_update_options);\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 20fc1b620fa..5b9683bf766 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -251,7 +251,6 @@ cmd_update()\n \t\t\tinit=1\n \t\t\t;;\n \t\t--require-init)\n-\t\t\tinit=1\n \t\t\trequire_init=1\n \t\t\t;;\n \t\t--remote)\n-- \ngitgitgadget\n\n"},{"id":"458260","messageId":"0d68ee723e54330138450f29e358e5ebe1a47aa0.1656623978.git.gitgitgadget@gmail.com","threadId":"58074","inReplyTo":"pull.1282.v2.git.git.1656623978.gitgitgadget@gmail.com","subject":"[PATCH v2 06/18] submodule update: remove \"-v\" option","fromName":"Ævar Arnfjörð Bjarmason via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-30T21:19:26Z","receivedAt":"2022-06-30T21:19:59Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?=\n \n\nIn e84c3cf3dc3 (git-submodule.sh: accept verbose flag in cmd_update to\nbe non-quiet, 2018-08-14) the \"git submodule update\" sub-command was\nmade to understand \"-v\", but the option was never documented.\n\nThe only in-tree user has been this test added in\n3ad0401e9e6 (submodule update: silence underlying merge/rebase with\n\"--quiet\", 2020-09-30), it wasn't per-se testing --quiet, but fixing a\nbug in e84c3cf3dc3: It used to set \"GIT_QUIET=0\" instead of unsetting\nit on \"-v\", and thus we'd end up passing \"--quiet\" to \"git\nsubmodule--helper\" on \"-v\", since the \"--quiet\" option was passed\nusing the ${parameter:+word} construct.\n\nFurthermore, even if someone had used the \"-v\" option they'd only be\ngetting the default output. Our default in both git-submodule.sh and\n\"git submodule--helper\" has been to be \"verbose\", so the only way this\noption could have matter is if it were used as e.g.:\n\n git submodule --quiet update -v [...]\n\nI.e. to undo the effect of a previous \"--quiet\" on the command-line.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \nSigned-off-by: Junio C Hamano \n---\n git-submodule.sh | 3 ---\n t/t7406-submodule-update.sh | 2 +-\n 2 files changed, 1 insertion(+), 4 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 5b9683bf766..0df6b0fc974 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -241,9 +241,6 @@ cmd_update()\n \t\t-q|--quiet)\n \t\t\tGIT_QUIET=1\n \t\t\t;;\n-\t\t-v)\n-\t\t\tunset GIT_QUIET\n-\t\t\t;;\n \t\t--progress)\n \t\t\tprogress=1\n \t\t\t;;\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex 43f779d751c..06d804e2131 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -1074,7 +1074,7 @@ test_expect_success 'submodule update --quiet passes quietness to merge/rebase'\n \t git submodule update --rebase --quiet >out 2>err &&\n \t test_must_be_empty out &&\n \t test_must_be_empty err &&\n-\t git submodule update --rebase -v >out 2>err &&\n+\t git submodule update --rebase >out 2>err &&\n \t test_file_not_empty out &&\n \t test_must_be_empty err\n \t)\n-- \ngitgitgadget\n\n"},{"id":"458261","messageId":"6e556c412e9283cfa9f9be4bfe4e9c813a53bf52.1656623978.git.gitgitgadget@gmail.com","threadId":"58074","inReplyTo":"pull.1282.v2.git.git.1656623978.gitgitgadget@gmail.com","subject":"[PATCH v2 07/18] submodule--helper: rename \"absorb-git-dirs\" to \"absorbgitdirs\"","fromName":"Ævar Arnfjörð Bjarmason via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-30T21:19:27Z","receivedAt":"2022-06-30T21:20:01Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?=\n \n\nRename the \"absorb-git-dirs\" subcommand to \"absorbgitdirs\", which is\nwhat the \"git submodule\" command itself has called it since the\nsubcommand was implemented in f6f85861400 (submodule: add\nabsorb-git-dir function, 2016-12-12).\n\nHaving these two be different will make it more tedious to dispatch to\neventually dispatch \"git submodule--helper\" directly, as we'd need to\nretain this name mapping. So let's get rid of this needless\ninconsistency.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \nSigned-off-by: Junio C Hamano \n---\n builtin/submodule--helper.c | 4 ++--\n git-submodule.sh | 2 +-\n submodule.c | 2 +-\n 3 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex f0702d0cfa2..1a84ae8efd2 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2787,7 +2787,7 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)\n \t};\n \n \tconst char *const git_submodule_helper_usage[] = {\n-\t\tN_(\"git submodule--helper absorb-git-dirs [] [...]\"),\n+\t\tN_(\"git submodule--helper absorbgitdirs [] [...]\"),\n \t\tNULL\n \t};\n \n@@ -3389,7 +3389,7 @@ static struct cmd_struct commands[] = {\n \t{\"deinit\", module_deinit, 0},\n \t{\"summary\", module_summary, SUPPORT_SUPER_PREFIX},\n \t{\"push-check\", push_check, 0},\n-\t{\"absorb-git-dirs\", absorb_git_dirs, SUPPORT_SUPER_PREFIX},\n+\t{\"absorbgitdirs\", absorb_git_dirs, SUPPORT_SUPER_PREFIX},\n \t{\"is-active\", is_active, 0},\n \t{\"check-name\", check_name, 0},\n \t{\"config\", module_config, 0},\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 0df6b0fc974..1c1dc320922 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -552,7 +552,7 @@ cmd_sync()\n \n cmd_absorbgitdirs()\n {\n-\tgit submodule--helper absorb-git-dirs --prefix \"$wt_prefix\" \"$@\"\n+\tgit submodule--helper absorbgitdirs --prefix \"$wt_prefix\" \"$@\"\n }\n \n # This loop parses the command line arguments to find the\ndiff --git a/submodule.c b/submodule.c\nindex 4e299f578f9..2af16c647d5 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -2374,7 +2374,7 @@ void absorb_git_dir_into_superproject(const char *path,\n \t\tcp.no_stdin = 1;\n \t\tstrvec_pushl(&cp.args, \"--super-prefix\", sb.buf,\n \t\t\t \"submodule--helper\",\n-\t\t\t \"absorb-git-dirs\", NULL);\n+\t\t\t \"absorbgitdirs\", NULL);\n \t\tprepare_submodule_repo_env(&cp.env);\n \t\tif (run_command(&cp))\n \t\t\tdie(_(\"could not recurse into submodule '%s'\"), path);\n-- \ngitgitgadget\n\n"},{"id":"458262","messageId":"36d45163b6de05886c2c6feb2e626ce423b96b3b.1656623978.git.gitgitgadget@gmail.com","threadId":"58074","inReplyTo":"pull.1282.v2.git.git.1656623978.gitgitgadget@gmail.com","subject":"[PATCH v2 08/18] submodule--helper: report \"submodule\" as our name in some \"-h\" output","fromName":"Ævar Arnfjörð Bjarmason via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-30T21:19:28Z","receivedAt":"2022-06-30T21:20:03Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?=\n \n\nChange the user-facing \"git submodule--helper\" commands so that\nthey'll report their name as being \"git submodule\". To a user these\ncommands are internal implementation details, and it doesn't make\nsense to emit usage about an internal helper when \"git submodule\" is\ninvoked with invalid options.\n\nBefore this we'd emit e.g.:\n\n\t$ git submodule absorbgitdirs --blah\n\terror: unknown option `blah'\n\tusage: git submodule--helper absorbgitdirs [] [...]\n\t[...]\nAnd:\n\n\t$ git submodule set-url -- --\n\tusage: git submodule--helper set-url [--quiet] \n\t[...]\n\nNow we'll start with \"usage: git submodule [...]\" in both of those\ncases. This change does not alter the \"list\", \"name\", \"clone\",\n\"config\" and \"create-branch\" commands, those are internal-only (as an\naside; their usage info should probably invoke BUG(...)). This only\nchanges the user-facing commands.\n\nThe \"status\", \"deinit\" and \"update\" commands are not included in this\nchange, because their usage information already used \"submodule\"\nrather than \"submodule--helper\".\n\nI don't think it's currently possible to emit some of this usage\ninformation in practice, as git-submodule.sh will catch unknown\noptions, and e.g. it doesn't seem to be possible to get \"add\" to emit\nits usage information from \"submodule--helper\".\n\nThough that change may be superfluous now, it's also harmless, and\nwill allow us to eventually dispatch further into \"git\nsubmodule--helper\" from git-submodule.sh, while emitting the correct\nusage output.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \nSigned-off-by: Junio C Hamano \n---\n builtin/submodule--helper.c | 18 +++++++++---------\n 1 file changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 1a84ae8efd2..04d2620fce8 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -444,7 +444,7 @@ static int module_foreach(int argc, const char **argv, const char *prefix)\n \t};\n \n \tconst char *const git_submodule_helper_usage[] = {\n-\t\tN_(\"git submodule--helper foreach [--quiet] [--recursive] [--] \"),\n+\t\tN_(\"git submodule foreach [--quiet] [--recursive] [--] \"),\n \t\tNULL\n \t};\n \n@@ -582,7 +582,7 @@ static int module_init(int argc, const char **argv, const char *prefix)\n \t};\n \n \tconst char *const git_submodule_helper_usage[] = {\n-\t\tN_(\"git submodule--helper init [] []\"),\n+\t\tN_(\"git submodule init [] []\"),\n \t\tNULL\n \t};\n \n@@ -1185,7 +1185,7 @@ static int module_summary(int argc, const char **argv, const char *prefix)\n \t};\n \n \tconst char *const git_submodule_helper_usage[] = {\n-\t\tN_(\"git submodule--helper summary [] [] [--] []\"),\n+\t\tN_(\"git submodule summary [] [] [--] []\"),\n \t\tNULL\n \t};\n \n@@ -1349,7 +1349,7 @@ static int module_sync(int argc, const char **argv, const char *prefix)\n \t};\n \n \tconst char *const git_submodule_helper_usage[] = {\n-\t\tN_(\"git submodule--helper sync [--quiet] [--recursive] []\"),\n+\t\tN_(\"git submodule sync [--quiet] [--recursive] []\"),\n \t\tNULL\n \t};\n \n@@ -2787,7 +2787,7 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)\n \t};\n \n \tconst char *const git_submodule_helper_usage[] = {\n-\t\tN_(\"git submodule--helper absorbgitdirs [] [...]\"),\n+\t\tN_(\"git submodule absorbgitdirs [] [...]\"),\n \t\tNULL\n \t};\n \n@@ -2892,7 +2892,7 @@ static int module_set_url(int argc, const char **argv, const char *prefix)\n \t\tOPT_END()\n \t};\n \tconst char *const usage[] = {\n-\t\tN_(\"git submodule--helper set-url [--quiet] \"),\n+\t\tN_(\"git submodule set-url [--quiet] \"),\n \t\tNULL\n \t};\n \n@@ -2931,8 +2931,8 @@ static int module_set_branch(int argc, const char **argv, const char *prefix)\n \t\tOPT_END()\n \t};\n \tconst char *const usage[] = {\n-\t\tN_(\"git submodule--helper set-branch [-q|--quiet] (-d|--default) \"),\n-\t\tN_(\"git submodule--helper set-branch [-q|--quiet] (-b|--branch) \"),\n+\t\tN_(\"git submodule set-branch [-q|--quiet] (-d|--default) \"),\n+\t\tN_(\"git submodule set-branch [-q|--quiet] (-b|--branch) \"),\n \t\tNULL\n \t};\n \n@@ -3276,7 +3276,7 @@ static int module_add(int argc, const char **argv, const char *prefix)\n \t};\n \n \tconst char *const usage[] = {\n-\t\tN_(\"git submodule--helper add [] [--] []\"),\n+\t\tN_(\"git submodule add [] [--] []\"),\n \t\tNULL\n \t};\n \n-- \ngitgitgadget\n\n"},{"id":"458263","messageId":"8f12108c2951cdfa181d6be66b6def28cd007bdd.1656623978.git.gitgitgadget@gmail.com","threadId":"58074","inReplyTo":"pull.1282.v2.git.git.1656623978.gitgitgadget@gmail.com","subject":"[PATCH v2 09/18] submodule--helper: understand --checkout, --merge and --rebase synonyms","fromName":"Ævar Arnfjörð Bjarmason via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-30T21:19:29Z","receivedAt":"2022-06-30T21:20:05Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?=\n \n\nUnderstand --checkout, --merge and --rebase synonyms for\n--update={checkout,merge,rebase}, as well as the short options that\n'git submodule' itself understands.\n\nThis removes a difference between the CLI API of \"git submodule\" and\n\"git submodule--helper\", making it easier to make the latter an alias\nfor the former. See 48308681b07 (git submodule update: have a\ndedicated helper for cloning, 2016-02-29) for the initial addition of\n--update.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \nSigned-off-by: Junio C Hamano \n---\n builtin/submodule--helper.c | 30 ++++++++++++++++++++++++++++++\n git-submodule.sh | 14 +++++++++-----\n 2 files changed, 39 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 04d2620fce8..53179472d85 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2404,6 +2404,23 @@ static void ensure_core_worktree(const char *path)\n \t}\n }\n \n+static const char *submodule_update_type_to_label(enum submodule_update_type type)\n+{\n+\tswitch (type) {\n+\tcase SM_UPDATE_CHECKOUT:\n+\t\treturn \"checkout\";\n+\tcase SM_UPDATE_MERGE:\n+\t\treturn \"merge\";\n+\tcase SM_UPDATE_REBASE:\n+\t\treturn \"rebase\";\n+\tcase SM_UPDATE_UNSPECIFIED:\n+\tcase SM_UPDATE_NONE:\n+\tcase SM_UPDATE_COMMAND:\n+\t\tbreak;\n+\t}\n+\tBUG(\"unreachable with type %d\", type);\n+}\n+\n static void update_data_to_args(struct update_data *update_data, struct strvec *args)\n {\n \tstrvec_pushl(args, \"submodule--helper\", \"update\", \"--recursive\", NULL);\n@@ -2582,6 +2599,7 @@ static int module_update(int argc, const char **argv, const char *prefix)\n \tstruct update_data opt = UPDATE_DATA_INIT;\n \tstruct list_objects_filter_options filter_options;\n \tint ret;\n+\tenum submodule_update_type update_type = SM_UPDATE_UNSPECIFIED;\n \n \tstruct option module_update_options[] = {\n \t\tOPT__FORCE(&opt.force, N_(\"force checkout updates\"), 0),\n@@ -2603,6 +2621,15 @@ static int module_update(int argc, const char **argv, const char *prefix)\n \t\tOPT_STRING(0, \"update\", &opt.update_default,\n \t\t\t N_(\"string\"),\n \t\t\t N_(\"rebase, merge, checkout or none\")),\n+\t\tOPT_SET_INT(0, \"checkout\", &update_type,\n+\t\t\tN_(\"use the 'checkout' update strategy (default)\"),\n+\t\t\tSM_UPDATE_CHECKOUT),\n+\t\tOPT_SET_INT('m', \"merge\", &update_type,\n+\t\t\tN_(\"use the 'merge' update strategy\"),\n+\t\t\tSM_UPDATE_MERGE),\n+\t\tOPT_SET_INT('r', \"rebase\", &update_type,\n+\t\t\tN_(\"use the 'rebase' update strategy\"),\n+\t\t\tSM_UPDATE_REBASE),\n \t\tOPT_STRING_LIST(0, \"reference\", &opt.references, N_(\"repo\"),\n \t\t\t N_(\"reference repository\")),\n \t\tOPT_BOOL(0, \"dissociate\", &opt.dissociate,\n@@ -2652,6 +2679,9 @@ static int module_update(int argc, const char **argv, const char *prefix)\n \n \topt.filter_options = &filter_options;\n \n+\tif (update_type != SM_UPDATE_UNSPECIFIED)\n+\t\topt.update_default = submodule_update_type_to_label(update_type);\n+\n \tif (opt.update_default)\n \t\tif (parse_submodule_update_strategy(opt.update_default,\n \t\t\t\t\t\t &opt.update_strategy) < 0)\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 1c1dc320922..7fc7119fb21 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -40,7 +40,9 @@ require_init=\n files=\n remote=\n nofetch=\n-update=\n+rebase=\n+merge=\n+checkout=\n custom_name=\n depth=\n progress=\n@@ -260,7 +262,7 @@ cmd_update()\n \t\t\tforce=$1\n \t\t\t;;\n \t\t-r|--rebase)\n-\t\t\tupdate=\"rebase\"\n+\t\t\trebase=1\n \t\t\t;;\n \t\t--reference)\n \t\t\tcase \"$2\" in '') usage ;; esac\n@@ -274,13 +276,13 @@ cmd_update()\n \t\t\tdissociate=1\n \t\t\t;;\n \t\t-m|--merge)\n-\t\t\tupdate=\"merge\"\n+\t\t\tmerge=1\n \t\t\t;;\n \t\t--recursive)\n \t\t\trecursive=1\n \t\t\t;;\n \t\t--checkout)\n-\t\t\tupdate=\"checkout\"\n+\t\t\tcheckout=1\n \t\t\t;;\n \t\t--recommend-shallow)\n \t\t\trecommend_shallow=\"--recommend-shallow\"\n@@ -341,7 +343,9 @@ cmd_update()\n \t\t${init:+--init} \\\n \t\t${nofetch:+--no-fetch} \\\n \t\t${wt_prefix:+--prefix \"$wt_prefix\"} \\\n-\t\t${update:+--update \"$update\"} \\\n+\t\t${rebase:+--rebase} \\\n+\t\t${merge:+--merge} \\\n+\t\t${checkout:+--checkout} \\\n \t\t${reference:+\"$reference\"} \\\n \t\t${dissociate:+\"--dissociate\"} \\\n \t\t${depth:+\"$depth\"} \\\n-- \ngitgitgadget\n\n"},{"id":"458264","messageId":"b788fc671bf1862a72f9fe7256affd7d152b8a6f.1656623978.git.gitgitgadget@gmail.com","threadId":"58074","inReplyTo":"pull.1282.v2.git.git.1656623978.gitgitgadget@gmail.com","subject":"[PATCH v2 10/18] submodule--helper: eliminate internal \"--update\" option","fromName":"Glen Choo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-30T21:19:30Z","receivedAt":"2022-06-30T21:20:07Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"From: Glen Choo \n\nFollow-up on the preceding commit which taught \"git submodule--helper\nupdate\" to understand \"--merge\", \"--checkout\" and \"--rebase\" and use\nthose options instead of \"--update=(rebase|merge|checkout|none)\" when\nthe command invokes itself.\n\nUnlike the preceding change this isn't strictly necessary to\neventually change \"git-submodule.sh\" so that it invokes \"git\nsubmodule--helper update\" directly, but let's remove this\ninconsistency in the command-line interface. We shouldn't need to\ncarry special synonyms for existing options in \"git submodule--helper\"\nwhen that command can use the primary documented names instead.\n\nBut, as seen in the post-image this makes the control flow within\n\"builtin/submodule--helper.c\" simpler, we can now write directly to\nthe \"update_default\" member of \"struct update_data\" when parsing the\noptions in \"module_update()\".\n\nSigned-off-by: Glen Choo \nSigned-off-by: Ævar Arnfjörð Bjarmason \nSigned-off-by: Junio C Hamano \n---\n builtin/submodule--helper.c | 33 +++++++++++++--------------------\n 1 file changed, 13 insertions(+), 20 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 53179472d85..389b900602f 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1818,7 +1818,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n static void determine_submodule_update_strategy(struct repository *r,\n \t\t\t\t\t\tint just_cloned,\n \t\t\t\t\t\tconst char *path,\n-\t\t\t\t\t\tconst char *update,\n+\t\t\t\t\t\tenum submodule_update_type update,\n \t\t\t\t\t\tstruct submodule_update_strategy *out)\n {\n \tconst struct submodule *sub = submodule_from_path(r, null_oid(), path);\n@@ -1828,9 +1828,7 @@ static void determine_submodule_update_strategy(struct repository *r,\n \tkey = xstrfmt(\"submodule.%s.update\", sub->name);\n \n \tif (update) {\n-\t\tif (parse_submodule_update_strategy(update, out) < 0)\n-\t\t\tdie(_(\"Invalid update mode '%s' for submodule path '%s'\"),\n-\t\t\t\tupdate, path);\n+\t\tout->type = update;\n \t} else if (!repo_config_get_string_tmp(r, key, &val)) {\n \t\tif (parse_submodule_update_strategy(val, out) < 0)\n \t\t\tdie(_(\"Invalid update mode '%s' configured for submodule path '%s'\"),\n@@ -1882,7 +1880,7 @@ struct update_data {\n \tconst char *prefix;\n \tconst char *recursive_prefix;\n \tconst char *displaypath;\n-\tconst char *update_default;\n+\tenum submodule_update_type update_default;\n \tstruct object_id suboid;\n \tstruct string_list references;\n \tstruct submodule_update_strategy update_strategy;\n@@ -2423,6 +2421,8 @@ static const char *submodule_update_type_to_label(enum submodule_update_type typ\n \n static void update_data_to_args(struct update_data *update_data, struct strvec *args)\n {\n+\tenum submodule_update_type update_type = update_data->update_default;\n+\n \tstrvec_pushl(args, \"submodule--helper\", \"update\", \"--recursive\", NULL);\n \tstrvec_pushf(args, \"--jobs=%d\", update_data->max_jobs);\n \tif (update_data->recursive_prefix)\n@@ -2446,8 +2446,10 @@ static void update_data_to_args(struct update_data *update_data, struct strvec *\n \t\tstrvec_push(args, \"--require-init\");\n \tif (update_data->depth)\n \t\tstrvec_pushf(args, \"--depth=%d\", update_data->depth);\n-\tif (update_data->update_default)\n-\t\tstrvec_pushl(args, \"--update\", update_data->update_default, NULL);\n+\tif (update_type != SM_UPDATE_UNSPECIFIED)\n+\t\tstrvec_pushf(args, \"--%s\",\n+\t\t\t submodule_update_type_to_label(update_type));\n+\n \tif (update_data->references.nr) {\n \t\tstruct string_list_item *item;\n \t\tfor_each_string_list_item(item, &update_data->references)\n@@ -2599,7 +2601,6 @@ static int module_update(int argc, const char **argv, const char *prefix)\n \tstruct update_data opt = UPDATE_DATA_INIT;\n \tstruct list_objects_filter_options filter_options;\n \tint ret;\n-\tenum submodule_update_type update_type = SM_UPDATE_UNSPECIFIED;\n \n \tstruct option module_update_options[] = {\n \t\tOPT__FORCE(&opt.force, N_(\"force checkout updates\"), 0),\n@@ -2618,16 +2619,13 @@ static int module_update(int argc, const char **argv, const char *prefix)\n \t\t\t 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, \"update\", &opt.update_default,\n-\t\t\t N_(\"string\"),\n-\t\t\t N_(\"rebase, merge, checkout or none\")),\n-\t\tOPT_SET_INT(0, \"checkout\", &update_type,\n+\t\tOPT_SET_INT(0, \"checkout\", &opt.update_default,\n \t\t\tN_(\"use the 'checkout' update strategy (default)\"),\n \t\t\tSM_UPDATE_CHECKOUT),\n-\t\tOPT_SET_INT('m', \"merge\", &update_type,\n+\t\tOPT_SET_INT('m', \"merge\", &opt.update_default,\n \t\t\tN_(\"use the 'merge' update strategy\"),\n \t\t\tSM_UPDATE_MERGE),\n-\t\tOPT_SET_INT('r', \"rebase\", &update_type,\n+\t\tOPT_SET_INT('r', \"rebase\", &opt.update_default,\n \t\t\tN_(\"use the 'rebase' update strategy\"),\n \t\t\tSM_UPDATE_REBASE),\n \t\tOPT_STRING_LIST(0, \"reference\", &opt.references, N_(\"repo\"),\n@@ -2679,13 +2677,8 @@ static int module_update(int argc, const char **argv, const char *prefix)\n \n \topt.filter_options = &filter_options;\n \n-\tif (update_type != SM_UPDATE_UNSPECIFIED)\n-\t\topt.update_default = submodule_update_type_to_label(update_type);\n-\n \tif (opt.update_default)\n-\t\tif (parse_submodule_update_strategy(opt.update_default,\n-\t\t\t\t\t\t &opt.update_strategy) < 0)\n-\t\t\tdie(_(\"bad value for update parameter\"));\n+\t\topt.update_strategy.type = opt.update_default;\n \n \tif (module_list_compute(argc, argv, prefix, &pathspec, &opt.list) < 0) {\n \t\tlist_objects_filter_release(&filter_options);\n-- \ngitgitgadget\n\n"},{"id":"458265","messageId":"2eec463739745d2110aa5462ba9547fa8d255ebb.1656623978.git.gitgitgadget@gmail.com","threadId":"58074","inReplyTo":"pull.1282.v2.git.git.1656623978.gitgitgadget@gmail.com","subject":"[PATCH v2 11/18] git-submodule.sh: use \"$quiet\", not \"$GIT_QUIET\"","fromName":"Ævar Arnfjörð Bjarmason via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-30T21:19:31Z","receivedAt":"2022-06-30T21:20:08Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?=\n \n\nRemove the use of the \"$GIT_QUIET\" variable in favor of our own\n\"$quiet\", ever since b3c5f5cb048 (submodule: move core cmd_update()\nlogic to C, 2022-03-15) we have not used the \"say\" function in\ngit-sh-setup.sh, which is the only thing that's affected by using\n\"GIT_QUIET\".\n\nWe still want to support --quiet for our own use though, but let's use\nour own variable for that. Now it's obvious that we only care about\npassing \"--quiet\" to \"git submodule--helper\", and not to change the\noutput of any \"say\" invocation.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \nSigned-off-by: Junio C Hamano \n---\n git-submodule.sh | 37 +++++++++++++++++++------------------\n 1 file changed, 19 insertions(+), 18 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 7fc7119fb21..5e5d21c010f 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -30,6 +30,7 @@ GIT_PROTOCOL_FROM_USER=0\n export GIT_PROTOCOL_FROM_USER\n \n command=\n+quiet=\n branch=\n force=\n reference=\n@@ -80,7 +81,7 @@ cmd_add()\n \t\t\tforce=$1\n \t\t\t;;\n \t\t-q|--quiet)\n-\t\t\tGIT_QUIET=1\n+\t\t\tquiet=1\n \t\t\t;;\n \t\t--progress)\n \t\t\tprogress=1\n@@ -128,7 +129,7 @@ cmd_add()\n \t\tusage\n \tfi\n \n-\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper add ${GIT_QUIET:+--quiet} ${force:+--force} ${progress:+\"--progress\"} ${branch:+--branch \"$branch\"} ${reference_path:+--reference \"$reference_path\"} ${dissociate:+--dissociate} ${custom_name:+--name \"$custom_name\"} ${depth:+\"$depth\"} -- \"$@\"\n+\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper add ${quiet:+--quiet} ${force:+--force} ${progress:+\"--progress\"} ${branch:+--branch \"$branch\"} ${reference_path:+--reference \"$reference_path\"} ${dissociate:+--dissociate} ${custom_name:+--name \"$custom_name\"} ${depth:+\"$depth\"} -- \"$@\"\n }\n \n #\n@@ -144,7 +145,7 @@ cmd_foreach()\n \tdo\n \t\tcase \"$1\" in\n \t\t-q|--quiet)\n-\t\t\tGIT_QUIET=1\n+\t\t\tquiet=1\n \t\t\t;;\n \t\t--recursive)\n \t\t\trecursive=1\n@@ -159,7 +160,7 @@ cmd_foreach()\n \t\tshift\n \tdone\n \n-\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper foreach ${GIT_QUIET:+--quiet} ${recursive:+--recursive} -- \"$@\"\n+\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper foreach ${quiet:+--quiet} ${recursive:+--recursive} -- \"$@\"\n }\n \n #\n@@ -174,7 +175,7 @@ cmd_init()\n \tdo\n \t\tcase \"$1\" in\n \t\t-q|--quiet)\n-\t\t\tGIT_QUIET=1\n+\t\t\tquiet=1\n \t\t\t;;\n \t\t--)\n \t\t\tshift\n@@ -190,7 +191,7 @@ cmd_init()\n \t\tshift\n \tdone\n \n-\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper init ${GIT_QUIET:+--quiet} -- \"$@\"\n+\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper init ${quiet:+--quiet} -- \"$@\"\n }\n \n #\n@@ -207,7 +208,7 @@ cmd_deinit()\n \t\t\tforce=$1\n \t\t\t;;\n \t\t-q|--quiet)\n-\t\t\tGIT_QUIET=1\n+\t\t\tquiet=1\n \t\t\t;;\n \t\t--all)\n \t\t\tdeinit_all=t\n@@ -226,7 +227,7 @@ cmd_deinit()\n \t\tshift\n \tdone\n \n-\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper deinit ${GIT_QUIET:+--quiet} ${force:+--force} ${deinit_all:+--all} -- \"$@\"\n+\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper deinit ${quiet:+--quiet} ${force:+--force} ${deinit_all:+--all} -- \"$@\"\n }\n \n #\n@@ -241,7 +242,7 @@ cmd_update()\n \tdo\n \t\tcase \"$1\" in\n \t\t-q|--quiet)\n-\t\t\tGIT_QUIET=1\n+\t\t\tquiet=1\n \t\t\t;;\n \t\t--progress)\n \t\t\tprogress=1\n@@ -335,7 +336,7 @@ cmd_update()\n \tdone\n \n \tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper update \\\n-\t\t${GIT_QUIET:+--quiet} \\\n+\t\t${quiet:+--quiet} \\\n \t\t${force:+--force} \\\n \t\t${progress:+\"--progress\"} \\\n \t\t${remote:+--remote} \\\n@@ -396,7 +397,7 @@ cmd_set_branch() {\n \t\tshift\n \tdone\n \n-\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper set-branch ${GIT_QUIET:+--quiet} ${branch:+--branch \"$branch\"} ${default:+--default} -- \"$@\"\n+\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper set-branch ${quiet:+--quiet} ${branch:+--branch \"$branch\"} ${default:+--default} -- \"$@\"\n }\n \n #\n@@ -409,7 +410,7 @@ cmd_set_url() {\n \tdo\n \t\tcase \"$1\" in\n \t\t-q|--quiet)\n-\t\t\tGIT_QUIET=1\n+\t\t\tquiet=1\n \t\t\t;;\n \t\t--)\n \t\t\tshift\n@@ -425,7 +426,7 @@ cmd_set_url() {\n \t\tshift\n \tdone\n \n-\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper set-url ${GIT_QUIET:+--quiet} -- \"$@\"\n+\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper set-url ${quiet:+--quiet} -- \"$@\"\n }\n \n #\n@@ -496,7 +497,7 @@ cmd_status()\n \tdo\n \t\tcase \"$1\" in\n \t\t-q|--quiet)\n-\t\t\tGIT_QUIET=1\n+\t\t\tquiet=1\n \t\t\t;;\n \t\t--cached)\n \t\t\tcached=1\n@@ -518,7 +519,7 @@ cmd_status()\n \t\tshift\n \tdone\n \n-\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper status ${GIT_QUIET:+--quiet} ${cached:+--cached} ${recursive:+--recursive} -- \"$@\"\n+\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper status ${quiet:+--quiet} ${cached:+--cached} ${recursive:+--recursive} -- \"$@\"\n }\n #\n # Sync remote urls for submodules\n@@ -531,7 +532,7 @@ cmd_sync()\n \tdo\n \t\tcase \"$1\" in\n \t\t-q|--quiet)\n-\t\t\tGIT_QUIET=1\n+\t\t\tquiet=1\n \t\t\tshift\n \t\t\t;;\n \t\t--recursive)\n@@ -551,7 +552,7 @@ cmd_sync()\n \t\tesac\n \tdone\n \n-\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper sync ${GIT_QUIET:+--quiet} ${recursive:+--recursive} -- \"$@\"\n+\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper sync ${quiet:+--quiet} ${recursive:+--recursive} -- \"$@\"\n }\n \n cmd_absorbgitdirs()\n@@ -572,7 +573,7 @@ do\n \t\tcommand=$1\n \t\t;;\n \t-q|--quiet)\n-\t\tGIT_QUIET=1\n+\t\tquiet=1\n \t\t;;\n \t--cached)\n \t\tcached=1\n-- \ngitgitgadget\n\n"},{"id":"458266","messageId":"5b893f7d81eb7feb43662ed8663e2af76a76b4c8.1656623978.git.gitgitgadget@gmail.com","threadId":"58074","inReplyTo":"pull.1282.v2.git.git.1656623978.gitgitgadget@gmail.com","subject":"[PATCH v2 12/18] git-sh-setup.sh: remove \"say\" function, change last users","fromName":"Ævar Arnfjörð Bjarmason via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-30T21:19:32Z","receivedAt":"2022-06-30T21:20:09Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?=\n \n\nRemove the \"say\" function, with various rewrites of the remaining\ngit-*.sh code to C and the preceding change to have git-submodule.sh\nstop using the GIT_QUIET variable there were only four uses in\ngit-subtree.sh. Let's have it use an \"arg_quiet\" variable instead, and\nmove the \"say\" function over to it.\n\nThe only other use was a trivial message in git-instaweb.sh, since it\nhas never supported the --quiet option (or similar) that code added in\n0b624b4ceee (instaweb: restart server if already running, 2009-11-22)\ncan simply use \"echo\" instead.\n\nThe remaining in-tree hits from \"say\" are all for the sibling function\ndefined in t/test-lib.sh. It's safe to remove this function since it\nhas never been documented in Documentation/git-sh-setup.txt.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \nSigned-off-by: Junio C Hamano \n---\n contrib/subtree/git-subtree.sh | 15 ++++++++++++---\n git-instaweb.sh | 2 +-\n git-sh-setup.sh | 9 ---------\n 3 files changed, 13 insertions(+), 13 deletions(-)\n\ndiff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\nindex 1af1d9653e9..7562a395c24 100755\n--- a/contrib/subtree/git-subtree.sh\n+++ b/contrib/subtree/git-subtree.sh\n@@ -50,6 +50,14 @@ m,message= use the given message as the commit message for the merge commit\n \n indent=0\n \n+# Usage: say [MSG...]\n+say () {\n+\tif test -z \"$arg_quiet\"\n+\tthen\n+\t\tprintf '%s\\n' \"$*\"\n+\tfi\n+}\n+\n # Usage: debug [MSG...]\n debug () {\n \tif test -n \"$arg_debug\"\n@@ -60,7 +68,7 @@ debug () {\n \n # Usage: progress [MSG...]\n progress () {\n-\tif test -z \"$GIT_QUIET\"\n+\tif test -z \"$arg_quiet\"\n \tthen\n \t\tif test -z \"$arg_debug\"\n \t\tthen\n@@ -146,6 +154,7 @@ main () {\n \teval \"$set_args\"\n \n \t# Begin \"real\" flag parsing.\n+\targ_quiet=\n \targ_debug=\n \targ_prefix=\n \targ_split_branch=\n@@ -161,7 +170,7 @@ main () {\n \n \t\tcase \"$opt\" in\n \t\t-q)\n-\t\t\tGIT_QUIET=1\n+\t\t\targ_quiet=1\n \t\t\t;;\n \t\t-d)\n \t\t\targ_debug=1\n@@ -252,7 +261,7 @@ main () {\n \tdir=\"$(dirname \"$arg_prefix/.\")\"\n \n \tdebug \"command: {$arg_command}\"\n-\tdebug \"quiet: {$GIT_QUIET}\"\n+\tdebug \"quiet: {$arg_quiet}\"\n \tdebug \"dir: {$dir}\"\n \tdebug \"opts: {$*}\"\n \tdebug\ndiff --git a/git-instaweb.sh b/git-instaweb.sh\nindex 4349566c891..c68f49454cd 100755\n--- a/git-instaweb.sh\n+++ b/git-instaweb.sh\n@@ -102,7 +102,7 @@ resolve_full_httpd () {\n \n start_httpd () {\n \tif test -f \"$fqgitdir/pid\"; then\n-\t\tsay \"Instance already running. Restarting...\"\n+\t\techo \"Instance already running. Restarting...\"\n \t\tstop_httpd\n \tfi\n \ndiff --git a/git-sh-setup.sh b/git-sh-setup.sh\nindex ecb60d9e3cb..ce273fe0e48 100644\n--- a/git-sh-setup.sh\n+++ b/git-sh-setup.sh\n@@ -57,15 +57,6 @@ die_with_status () {\n \texit \"$status\"\n }\n \n-GIT_QUIET=\n-\n-say () {\n-\tif test -z \"$GIT_QUIET\"\n-\tthen\n-\t\tprintf '%s\\n' \"$*\"\n-\tfi\n-}\n-\n if test -n \"$OPTIONS_SPEC\"; then\n \tusage() {\n \t\t\"$0\" -h\n-- \ngitgitgadget\n\n"},{"id":"458267","messageId":"64c138df19670cc615c7bb6cda33f085c17ca837.1656623978.git.gitgitgadget@gmail.com","threadId":"58074","inReplyTo":"pull.1282.v2.git.git.1656623978.gitgitgadget@gmail.com","subject":"[PATCH v2 13/18] submodule--helper update: use display path helper","fromName":"Glen Choo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-30T21:19:33Z","receivedAt":"2022-06-30T21:20:11Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"From: Glen Choo \n\nThere are two locations in prepare_to_clone_next_submodule() that\nmanually calculate the submodule display path, but should just use\ndo_get_submodule_displaypath() for consistency.\n\nDo this replacement and reorder the code slightly to avoid computing\nthe display path twice.\n\nThis code was never tested, and adding tests shows that both these sites\nhave been computing the display path incorrectly ever since they were\nintroduced in 48308681b0 (git submodule update: have a dedicated helper\nfor cloning, 2016-02-29) [1]:\n\n- The first hunk puts a \"/\" between recursive_prefix and ce->name, but\n recursive_prefix already ends with \"/\".\n- The second hunk calls relative_path() on recursive_prefix and\n ce->name, but relative_path() only makes sense when both paths share\n the same base directory. This is never the case here:\n - recursive_prefix is the path from the topmost superproject to the\n current submodule\n - ce->name is the path from the root of the current submodule to its\n submodule.\n so, e.g. recursive_prefix=\"super\" and ce->name=\"submodule\" produces\n displayname=\"../super\" instead of \"super/submodule\".\n\nWhile we're fixing the display names, also fix inconsistent quoting of\nthe submodule name.\n\n[1] I verified this by applying the tests to 48308681b0.\n\nSigned-off-by: Glen Choo \n---\n builtin/submodule--helper.c | 21 +++++---------\n t/t7406-submodule-update.sh | 56 +++++++++++++++++++++++++++++++++++++\n 2 files changed, 63 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 389b900602f..db2d5ab7998 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1947,30 +1947,23 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n \tconst char *update_string;\n \tenum submodule_update_type update_type;\n \tchar *key;\n-\tstruct strbuf displaypath_sb = STRBUF_INIT;\n \tstruct strbuf sb = STRBUF_INIT;\n-\tconst char *displaypath = NULL;\n+\tchar *displaypath;\n \tint needs_cloning = 0;\n \tint need_free_url = 0;\n \n+\tdisplaypath = do_get_submodule_displaypath(ce->name,\n+\t\t\t\t\t\t suc->update_data->prefix,\n+\t\t\t\t\t\t suc->update_data->recursive_prefix);\n+\n \tif (ce_stage(ce)) {\n-\t\tif (suc->update_data->recursive_prefix)\n-\t\t\tstrbuf_addf(&sb, \"%s/%s\", suc->update_data->recursive_prefix, ce->name);\n-\t\telse\n-\t\t\tstrbuf_addstr(&sb, ce->name);\n-\t\tstrbuf_addf(out, _(\"Skipping unmerged submodule %s\"), sb.buf);\n+\t\tstrbuf_addf(out, _(\"Skipping unmerged submodule %s\"), displaypath);\n \t\tstrbuf_addch(out, '\\n');\n \t\tgoto cleanup;\n \t}\n \n \tsub = submodule_from_path(the_repository, null_oid(), ce->name);\n \n-\tif (suc->update_data->recursive_prefix)\n-\t\tdisplaypath = relative_path(suc->update_data->recursive_prefix,\n-\t\t\t\t\t ce->name, &displaypath_sb);\n-\telse\n-\t\tdisplaypath = ce->name;\n-\n \tif (!sub) {\n \t\tnext_submodule_warn_missing(suc, out, displaypath);\n \t\tgoto cleanup;\n@@ -2060,7 +2053,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n \t\t\t\t\t \"--no-single-branch\");\n \n cleanup:\n-\tstrbuf_release(&displaypath_sb);\n+\tfree(displaypath);\n \tstrbuf_release(&sb);\n \tif (need_free_url)\n \t\tfree((void*)url);\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex 06d804e2131..f0408c5cc4a 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -1116,4 +1116,60 @@ test_expect_success 'submodule update --filter sets partial clone settings' '\n \ttest_cmp_config -C super-filter/submodule blob:none remote.origin.partialclonefilter\n '\n \n+# NEEDSWORK: Clean up the tests so that we can reuse the test setup.\n+# Don't reuse the existing repos because the earlier tests have\n+# intentionally disruptive configurations.\n+test_expect_success 'setup clean recursive superproject' '\n+\tgit init bottom &&\n+\ttest_commit -C bottom \"bottom\" &&\n+\tgit init middle &&\n+\tgit -C middle submodule add ../bottom bottom &&\n+\tgit -C middle commit -m \"middle\" &&\n+\tgit init top &&\n+\tgit -C top submodule add ../middle middle &&\n+\tgit -C top commit -m \"top\" &&\n+\tgit clone --recurse-submodules top top-clean\n+'\n+\n+test_expect_success 'submodule update should skip unmerged submodules' '\n+\ttest_when_finished \"rm -fr top-cloned\" &&\n+\tcp -r top-clean top-cloned &&\n+\n+\t# Create an upstream commit in each repo, starting with bottom\n+\ttest_commit -C bottom upstream_commit &&\n+\t# Create middle commit\n+\tgit -C middle/bottom fetch &&\n+\tgit -C middle/bottom checkout -f FETCH_HEAD &&\n+\tgit -C middle add bottom &&\n+\tgit -C middle commit -m \"upstream_commit\" &&\n+\t# Create top commit\n+\tgit -C top/middle fetch &&\n+\tgit -C top/middle checkout -f FETCH_HEAD &&\n+\tgit -C top add middle &&\n+\tgit -C top commit -m \"upstream_commit\" &&\n+\n+\t# Create a downstream conflict\n+\ttest_commit -C top-cloned/middle/bottom downstream_commit &&\n+\tgit -C top-cloned/middle add bottom &&\n+\tgit -C top-cloned/middle commit -m \"downstream_commit\" &&\n+\tgit -C top-cloned/middle fetch --recurse-submodules origin &&\n+\ttest_must_fail git -C top-cloned/middle merge origin/main &&\n+\n+\t# Make the update of \"middle\" a no-op, otherwise we error out\n+\t# because of its unmerged state\n+\ttest_config -C top-cloned submodule.middle.update !true &&\n+\tgit -C top-cloned submodule update --recursive 2>actual.err &&\n+\tgrep -F \"Skipping unmerged submodule middle/bottom\" actual.err\n+'\n+\n+test_expect_success 'submodule update --recursive skip submodules with strategy=none' '\n+\ttest_when_finished \"rm -fr top-cloned\" &&\n+\tcp -r top-clean top-cloned &&\n+\n+\ttest_commit -C top-cloned/middle/bottom downstream_commit &&\n+\tgit -C top-cloned/middle config submodule.bottom.update none &&\n+\tgit -C top-cloned submodule update --recursive 2>actual.err &&\n+\tgrep -F \"Skipping submodule ${SQ}middle/bottom${SQ}\" actual.err\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"458268","messageId":"d3e803a4630a11bb68f673403eceb890bd573562.1656623978.git.gitgitgadget@gmail.com","threadId":"58074","inReplyTo":"pull.1282.v2.git.git.1656623978.gitgitgadget@gmail.com","subject":"[PATCH v2 14/18] submodule--helper: don't recreate recursive prefix","fromName":"Glen Choo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-30T21:19:34Z","receivedAt":"2022-06-30T21:20:12Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"From: Glen Choo \n\nupdate_submodule() uses duplicated code to compute\nupdate_data->displaypath and next.recursive_prefix. The latter is just\nthe former with \"/\" appended to it, and since update_data->displaypath\nnot changed outside of this statement, we can just reuse the already\ncomputed result.\n\nWe can go one step further and remove the reference to\nnext.recursive_prefix altogether. Since it is only used in\nupdate_data_to_args() (to compute the \"--recursive-prefix\" flag for the\nrecursive update child process) we can just use the already computed\n.displaypath value of there.\n\nDelete the duplicated code, and remove the unnecessary reference to\nnext.recursive_prefix. As a bonus, this fixes a memory leak where\nprefixed_path was never freed (this leak was first reported in [1]).\n\n[1] https://lore.kernel.org/git/877a45867ae368bf9e053caedcb6cf421e02344d.1655336146.git.gitgitgadget@gmail.com\n\nSigned-off-by: Glen Choo \n---\n builtin/submodule--helper.c | 14 +++-----------\n 1 file changed, 3 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex db2d5ab7998..c104b1a0236 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2418,9 +2418,9 @@ static void update_data_to_args(struct update_data *update_data, struct strvec *\n \n \tstrvec_pushl(args, \"submodule--helper\", \"update\", \"--recursive\", NULL);\n \tstrvec_pushf(args, \"--jobs=%d\", update_data->max_jobs);\n-\tif (update_data->recursive_prefix)\n-\t\tstrvec_pushl(args, \"--recursive-prefix\",\n-\t\t\t update_data->recursive_prefix, NULL);\n+\tif (update_data->displaypath)\n+\t\tstrvec_pushf(args, \"--recursive-prefix=%s/\",\n+\t\t\t update_data->displaypath);\n \tif (update_data->quiet)\n \t\tstrvec_push(args, \"--quiet\");\n \tif (update_data->force)\n@@ -2516,14 +2516,6 @@ static int update_submodule(struct update_data *update_data)\n \t\tstruct update_data next = *update_data;\n \t\tint res;\n \n-\t\tif (update_data->recursive_prefix)\n-\t\t\tprefixed_path = xstrfmt(\"%s%s/\", update_data->recursive_prefix,\n-\t\t\t\t\t\tupdate_data->sm_path);\n-\t\telse\n-\t\t\tprefixed_path = xstrfmt(\"%s/\", update_data->sm_path);\n-\n-\t\tnext.recursive_prefix = get_submodule_displaypath(prefixed_path,\n-\t\t\t\t\t\t\t\t update_data->prefix);\n \t\tnext.prefix = NULL;\n \t\toidcpy(&next.oid, null_oid());\n \t\toidcpy(&next.suboid, null_oid());\n-- \ngitgitgadget\n\n"},{"id":"458269","messageId":"1f7cf6ffaf1086321061d896c4270955d8ed136e.1656623978.git.gitgitgadget@gmail.com","threadId":"58074","inReplyTo":"pull.1282.v2.git.git.1656623978.gitgitgadget@gmail.com","subject":"[PATCH v2 15/18] submodule--helper: use correct display path helper","fromName":"Glen Choo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-30T21:19:35Z","receivedAt":"2022-06-30T21:20:14Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"From: Glen Choo \n\nReplace a chunk of code in update_submodule() with an equivalent\ndo_get_submodule_displaypath() invocation. This is already tested by\nt/t7406-submodule-update.sh:'submodule update --init --recursive from\nsubdirectory', so no tests are added.\n\nThe two are equivalent because:\n\n- Exactly one of recursive_prefix|prefix is non-NULL at a time; prefix\n is set at the superproject level, and recursive_prefix is set when\n recursing into submodules. There is also a BUG() statement in\n get_submodule_displaypath() that asserts that both cannot be non-NULL.\n\n- In get_submodule_displaypath(), get_super_prefix() always returns NULL\n because \"--super-prefix\" is never passed. Thus calling it is\n equivalent to calling do_get_submodule_displaypath() with super_prefix\n = NULL.\n\nTherefore:\n\n- When recursive_prefix is non-NULL, prefix is NULL, and thus\n get_submodule_displaypath() just returns prefixed_path. This is\n identical to calling do_get_submodule_displaypath() with super_prefix\n = recursive_prefix because the return value is still the concatenation\n of recursive_prefix + update_data->sm_path.\n\n- When prefix is non-NULL, prefixed_path = update_data->sm_path. Thus\n calling get_submodule_displaypath() with prefixed_path is equivalent\n to calling do_get_submodule_displaypath() with update_data->sm_path\n\nSigned-off-by: Glen Choo \n---\n builtin/submodule--helper.c | 14 +++-----------\n 1 file changed, 3 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex c104b1a0236..aad431f898e 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2464,19 +2464,11 @@ static void update_data_to_args(struct update_data *update_data, struct strvec *\n \n static int update_submodule(struct update_data *update_data)\n {\n-\tchar *prefixed_path;\n-\n \tensure_core_worktree(update_data->sm_path);\n \n-\tif (update_data->recursive_prefix)\n-\t\tprefixed_path = xstrfmt(\"%s%s\", update_data->recursive_prefix,\n-\t\t\t\t\tupdate_data->sm_path);\n-\telse\n-\t\tprefixed_path = xstrdup(update_data->sm_path);\n-\n-\tupdate_data->displaypath = get_submodule_displaypath(prefixed_path,\n-\t\t\t\t\t\t\t update_data->prefix);\n-\tfree(prefixed_path);\n+\tupdate_data->displaypath = do_get_submodule_displaypath(update_data->sm_path,\n+\t\t\t\t\t\t\t\tupdate_data->prefix,\n+\t\t\t\t\t\t\t\tupdate_data->recursive_prefix);\n \n \tdetermine_submodule_update_strategy(the_repository, update_data->just_cloned,\n \t\t\t\t\t update_data->sm_path, update_data->update_default,\n-- \ngitgitgadget\n\n"},{"id":"458270","messageId":"85e65f143b602be28c7551729c5e3357118a755b.1656623978.git.gitgitgadget@gmail.com","threadId":"58074","inReplyTo":"pull.1282.v2.git.git.1656623978.gitgitgadget@gmail.com","subject":"[PATCH v2 16/18] submodule--helper: remove unused SUPPORT_SUPER_PREFIX flags","fromName":"Ævar Arnfjörð Bjarmason via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-30T21:19:36Z","receivedAt":"2022-06-30T21:20:30Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?=\n \n\nRemove the SUPPORT_SUPER_PREFIX flag from \"add\", \"init\" and\n\"summary\". For the \"add\" command it hasn't been used since [1],\nlikewise for \"init\" and \"summary\" since [2] and [3], respectively.\n\nAs implemented in 74866d75793 (git: make super-prefix option,\n2016-10-07) the SUPPORT_SUPER_PREFIX flag in git.c applies for the\nentire command, but as implemented in 89c86265576 (submodule helper:\nsupport super prefix, 2016-12-08) we assert here in\ncmd_submodule__helper() that we're not getting the flag unexpectedly.\n\n1. 8c8195e9c3e (submodule--helper: introduce add-clone subcommand,\n 2021-07-10)\n2. 6e7c14e65c8 (submodule update --init: display correct path from\n submodule, 2017-01-06)\n3. 1cf823d8f00 (submodule: remove unnecessary `prefix` based option\n logic, 2021-06-22)\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \nSigned-off-by: Glen Choo \n---\n builtin/submodule--helper.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex aad431f898e..360309195fb 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -3379,15 +3379,15 @@ static struct cmd_struct commands[] = {\n \t{\"list\", module_list, 0},\n \t{\"name\", module_name, 0},\n \t{\"clone\", module_clone, 0},\n-\t{\"add\", module_add, SUPPORT_SUPER_PREFIX},\n+\t{\"add\", module_add, 0},\n \t{\"update\", module_update, 0},\n \t{\"resolve-relative-url-test\", resolve_relative_url_test, 0},\n \t{\"foreach\", module_foreach, SUPPORT_SUPER_PREFIX},\n-\t{\"init\", module_init, SUPPORT_SUPER_PREFIX},\n+\t{\"init\", module_init, 0},\n \t{\"status\", module_status, SUPPORT_SUPER_PREFIX},\n \t{\"sync\", module_sync, SUPPORT_SUPER_PREFIX},\n \t{\"deinit\", module_deinit, 0},\n-\t{\"summary\", module_summary, SUPPORT_SUPER_PREFIX},\n+\t{\"summary\", module_summary, 0},\n \t{\"push-check\", push_check, 0},\n \t{\"absorbgitdirs\", absorb_git_dirs, SUPPORT_SUPER_PREFIX},\n \t{\"is-active\", is_active, 0},\n-- \ngitgitgadget\n\n"},{"id":"458271","messageId":"a21e8cd174db48ea0a771be67583cef0aa866173.1656623978.git.gitgitgadget@gmail.com","threadId":"58074","inReplyTo":"pull.1282.v2.git.git.1656623978.gitgitgadget@gmail.com","subject":"[PATCH v2 18/18] submodule--helper: remove display path helper","fromName":"Glen Choo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-30T21:19:38Z","receivedAt":"2022-06-30T21:20:43Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"From: Glen Choo \n\nAll invocations of do_get_submodule_displaypath() pass\nget_super_prefix() as the super_prefix arg, which is exactly the same\nas get_submodule_displaypath().\n\nReplace all calls to do_get_submodule_displaypath() with\nget_submodule_displaypath(), and since it has no more callers, remove\nit.\n\nSigned-off-by: Glen Choo \n---\n builtin/submodule--helper.c | 26 +++++++++-----------------\n 1 file changed, 9 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 41177332dd8..3e0ceb52589 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -118,10 +118,11 @@ static int resolve_relative_url_test(int argc, const char **argv, const char *pr\n \treturn 0;\n }\n \n-static char *do_get_submodule_displaypath(const char *path,\n-\t\t\t\t\t const char *prefix,\n-\t\t\t\t\t const char *super_prefix)\n+/* the result should be freed by the caller. */\n+static char *get_submodule_displaypath(const char *path, const char *prefix)\n {\n+\tconst char *super_prefix = get_super_prefix();\n+\n \tif (prefix && super_prefix) {\n \t\tBUG(\"cannot have prefix '%s' and superprefix '%s'\",\n \t\t prefix, super_prefix);\n@@ -137,13 +138,6 @@ static char *do_get_submodule_displaypath(const char *path,\n \t}\n }\n \n-/* the result should be freed by the caller. */\n-static char *get_submodule_displaypath(const char *path, const char *prefix)\n-{\n-\tconst char *super_prefix = get_super_prefix();\n-\treturn do_get_submodule_displaypath(path, prefix, super_prefix);\n-}\n-\n static char *compute_rev_name(const char *sub_path, const char* object_id)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n@@ -488,7 +482,7 @@ static void init_submodule(const char *path, const char *prefix,\n \tstruct strbuf sb = STRBUF_INIT;\n \tchar *upd = NULL, *url = NULL, *displaypath;\n \n-\tdisplaypath = do_get_submodule_displaypath(path, prefix, get_super_prefix());\n+\tdisplaypath = get_submodule_displaypath(path, prefix);\n \n \tsub = submodule_from_path(the_repository, null_oid(), path);\n \n@@ -1947,9 +1941,8 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n \tint needs_cloning = 0;\n \tint need_free_url = 0;\n \n-\tdisplaypath = do_get_submodule_displaypath(ce->name,\n-\t\t\t\t\t\t suc->update_data->prefix,\n-\t\t\t\t\t\t get_super_prefix());\n+\tdisplaypath =\n+\t\tget_submodule_displaypath(ce->name, suc->update_data->prefix);\n \n \tif (ce_stage(ce)) {\n \t\tstrbuf_addf(out, _(\"Skipping unmerged submodule %s\"), displaypath);\n@@ -2461,9 +2454,8 @@ static int update_submodule(struct update_data *update_data)\n {\n \tensure_core_worktree(update_data->sm_path);\n \n-\tupdate_data->displaypath = do_get_submodule_displaypath(update_data->sm_path,\n-\t\t\t\t\t\t\t\tupdate_data->prefix,\n-\t\t\t\t\t\t\t\tget_super_prefix());\n+\tupdate_data->displaypath = get_submodule_displaypath(\n+\t\tupdate_data->sm_path, update_data->prefix);\n \n \tdetermine_submodule_update_strategy(the_repository, update_data->just_cloned,\n \t\t\t\t\t update_data->sm_path, update_data->update_default,\n-- \ngitgitgadget\n"},{"id":"458272","messageId":"1d8b6b244dca5908a10e952a7a1ec4f92103f144.1656623978.git.gitgitgadget@gmail.com","threadId":"58074","inReplyTo":"pull.1282.v2.git.git.1656623978.gitgitgadget@gmail.com","subject":"[PATCH v2 17/18] submodule--helper update: use --super-prefix","fromName":"Glen Choo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-30T21:19:37Z","receivedAt":"2022-06-30T21:20:43Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"From: Glen Choo \n\nUnlike the other subcommands, \"git submodule--helper update\" uses the\n\"--recursive-prefix\" flag instead of \"--super-prefix\". The two flags are\notherwise identical (they only serve to compute the 'display path' of a\nsubmodule), except that there is a dedicated helper function to get the\nvalue of \"--super-prefix\".\n\nThis inconsistency exists because \"git submodule update\" used to pass\n\"--recursive-prefix\" between shell and C (introduced in [1]) before\n\"--super-prefix\" was introduced (in [2]), and for simplicity, we kept\nthis name when \"git submodule--helper update\" was created.\n\nRemove \"--recursive-prefix\" and its associated code from \"git\nsubmodule--helper update\", replacing it with \"--super-prefix\".\n\nTo use \"--super-prefix\", module_update is marked with\nSUPPORT_SUPER_PREFIX. Note that module_clone must also be marked with\nSUPPORT_SUPER_PREFIX, otherwise the \"git submodule--helper clone\"\nsubprocess will fail check because \"--super-prefix\" is propagated via\nthe environment.\n\n[1] 48308681b0 (git submodule update: have a dedicated helper for\ncloning, 2016-02-29)\n[2] 74866d7579 (git: make super-prefix option, 2016-10-07)\n\nSigned-off-by: Glen Choo \n---\n builtin/submodule--helper.c | 30 ++++++++++--------------------\n 1 file changed, 10 insertions(+), 20 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 360309195fb..41177332dd8 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -477,22 +477,18 @@ static int starts_with_dot_dot_slash(const char *const path)\n \n struct init_cb {\n \tconst char *prefix;\n-\tconst char *superprefix;\n \tunsigned int flags;\n };\n #define INIT_CB_INIT { 0 }\n \n static void init_submodule(const char *path, const char *prefix,\n-\t\t\t const char *superprefix, unsigned int flags)\n+\t\t\t unsigned int flags)\n {\n \tconst struct submodule *sub;\n \tstruct strbuf sb = STRBUF_INIT;\n \tchar *upd = NULL, *url = NULL, *displaypath;\n \n-\t/* try superprefix from the environment, if it is not passed explicitly */\n-\tif (!superprefix)\n-\t\tsuperprefix = get_super_prefix();\n-\tdisplaypath = do_get_submodule_displaypath(path, prefix, superprefix);\n+\tdisplaypath = do_get_submodule_displaypath(path, prefix, get_super_prefix());\n \n \tsub = submodule_from_path(the_repository, null_oid(), path);\n \n@@ -566,7 +562,7 @@ static void init_submodule(const char *path, const char *prefix,\n static void init_submodule_cb(const struct cache_entry *list_item, void *cb_data)\n {\n \tstruct init_cb *info = cb_data;\n-\tinit_submodule(list_item->name, info->prefix, info->superprefix, info->flags);\n+\tinit_submodule(list_item->name, info->prefix, info->flags);\n }\n \n static int module_init(int argc, const char **argv, const char *prefix)\n@@ -1878,7 +1874,6 @@ struct submodule_update_clone {\n \n struct update_data {\n \tconst char *prefix;\n-\tconst char *recursive_prefix;\n \tconst char *displaypath;\n \tenum submodule_update_type update_default;\n \tstruct object_id suboid;\n@@ -1954,7 +1949,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n \n \tdisplaypath = do_get_submodule_displaypath(ce->name,\n \t\t\t\t\t\t suc->update_data->prefix,\n-\t\t\t\t\t\t suc->update_data->recursive_prefix);\n+\t\t\t\t\t\t get_super_prefix());\n \n \tif (ce_stage(ce)) {\n \t\tstrbuf_addf(out, _(\"Skipping unmerged submodule %s\"), displaypath);\n@@ -2416,11 +2411,11 @@ static void update_data_to_args(struct update_data *update_data, struct strvec *\n {\n \tenum submodule_update_type update_type = update_data->update_default;\n \n-\tstrvec_pushl(args, \"submodule--helper\", \"update\", \"--recursive\", NULL);\n-\tstrvec_pushf(args, \"--jobs=%d\", update_data->max_jobs);\n \tif (update_data->displaypath)\n-\t\tstrvec_pushf(args, \"--recursive-prefix=%s/\",\n+\t\tstrvec_pushf(args, \"--super-prefix=%s/\",\n \t\t\t update_data->displaypath);\n+\tstrvec_pushl(args, \"submodule--helper\", \"update\", \"--recursive\", NULL);\n+\tstrvec_pushf(args, \"--jobs=%d\", update_data->max_jobs);\n \tif (update_data->quiet)\n \t\tstrvec_push(args, \"--quiet\");\n \tif (update_data->force)\n@@ -2468,7 +2463,7 @@ static int update_submodule(struct update_data *update_data)\n \n \tupdate_data->displaypath = do_get_submodule_displaypath(update_data->sm_path,\n \t\t\t\t\t\t\t\tupdate_data->prefix,\n-\t\t\t\t\t\t\t\tupdate_data->recursive_prefix);\n+\t\t\t\t\t\t\t\tget_super_prefix());\n \n \tdetermine_submodule_update_strategy(the_repository, update_data->just_cloned,\n \t\t\t\t\t update_data->sm_path, update_data->update_default,\n@@ -2592,10 +2587,6 @@ static int module_update(int argc, const char **argv, const char *prefix)\n \t\tOPT_STRING(0, \"prefix\", &opt.prefix,\n \t\t\t N_(\"path\"),\n \t\t\t N_(\"path into the working tree\")),\n-\t\tOPT_STRING(0, \"recursive-prefix\", &opt.recursive_prefix,\n-\t\t\t N_(\"path\"),\n-\t\t\t N_(\"path into the working tree, across nested \"\n-\t\t\t \"submodule boundaries\")),\n \t\tOPT_SET_INT(0, \"checkout\", &opt.update_default,\n \t\t\tN_(\"use the 'checkout' update strategy (default)\"),\n \t\t\tSM_UPDATE_CHECKOUT),\n@@ -2681,7 +2672,6 @@ static int module_update(int argc, const char **argv, const char *prefix)\n \t\t\tmodule_list_active(&list);\n \n \t\tinfo.prefix = opt.prefix;\n-\t\tinfo.superprefix = opt.recursive_prefix;\n \t\tif (opt.quiet)\n \t\t\tinfo.flags |= OPT_QUIET;\n \n@@ -3378,9 +3368,9 @@ struct cmd_struct {\n static struct cmd_struct commands[] = {\n \t{\"list\", module_list, 0},\n \t{\"name\", module_name, 0},\n-\t{\"clone\", module_clone, 0},\n+\t{\"clone\", module_clone, SUPPORT_SUPER_PREFIX},\n \t{\"add\", module_add, 0},\n-\t{\"update\", module_update, 0},\n+\t{\"update\", module_update, SUPPORT_SUPER_PREFIX},\n \t{\"resolve-relative-url-test\", resolve_relative_url_test, 0},\n \t{\"foreach\", module_foreach, SUPPORT_SUPER_PREFIX},\n \t{\"init\", module_init, 0},\n-- \ngitgitgadget\n\n"},{"id":"458276","messageId":"kl6lwncxstk9.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"58074","inReplyTo":"pull.1282.v2.git.git.1656623978.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 00/18] submodule: remove \"--recursive-prefix\"","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-06-30T21:57:42Z","receivedAt":"2022-06-30T21:57:49Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"\n\"Glen Choo via GitGitGadget\" writes:\n\n> Glen Choo (6):\n> submodule--helper: eliminate internal \"--update\" option\n> submodule--helper update: use display path helper\n> submodule--helper: don't recreate recursive prefix\n> submodule--helper: use correct display path helper\n> submodule--helper update: use --super-prefix\n> submodule--helper: remove display path helper\n>\n> Ævar Arnfjörð Bjarmason (12):\n> git-submodule.sh: remove unused sanitize_submodule_env()\n> git-submodule.sh: remove unused $prefix variable\n> git-submodule.sh: make the \"$cached\" variable a boolean\n> git-submodule.sh: remove unused top-level \"--branch\" argument\n> submodule--helper: have --require-init imply --init\n> submodule update: remove \"-v\" option\n> submodule--helper: rename \"absorb-git-dirs\" to \"absorbgitdirs\"\n> submodule--helper: report \"submodule\" as our name in some \"-h\" output\n> submodule--helper: understand --checkout, --merge and --rebase\n> synonyms\n> git-submodule.sh: use \"$quiet\", not \"$GIT_QUIET\"\n> git-sh-setup.sh: remove \"say\" function, change last users\n> submodule--helper: remove unused SUPPORT_SUPER_PREFIX flags\n\nLooks like I accidentally submitted all of ab/submodule-cleanup... Let\nme try this again without GGG.\n"},{"id":"458278","messageId":"20220630221147.45689-1-chooglen@google.com","threadId":"58074","inReplyTo":"pull.1282.v2.git.git.1656623978.gitgitgadget@gmail.com","subject":"[PATCH v3 0/6] submodule: remove \"--recursive-prefix\"","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-06-30T22:11:41Z","receivedAt":"2022-06-30T22:12:01Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"(resending as v3 because I accidentally included ab/submodule-cleanup in\nv2)\n\nThanks Ævar for the feedback! I've incorporated all of the suggestions,\nexcept breaking up the tests in 1/6 into their own patch - I wasn't\nconvinced of the value of having prescriptive tests (with\ntest_expect_failure), and I found it difficult to discuss descriptive tests\n(with test_expect_success) without also having the code in the same diff.\nFWIW, this version (and the previous one) \"git rebase -x 'make test'\"\ncleanly :)\n\nNote for Junio: this version is based on ab/submodule-cleanup (and so will\nfuture versions).\n\n= Description\n\nThis series is a refactor of \"git submodule--helper update\" that replaces\n\"--recursive-prefix\" with \"--super-prefix\" (see Background). This was\ninitially motivated by:\n\n * Junio's suggestion to simplify the code [1] (in response to a memory leak\n found by Johannes Schindelin [2]).\n * A desire to use the module_list API + get_submodule_displaypath() outside\n of builtin/submodule--helper.c (I expect to use this to check out\n branches in each submodule).\n\nBut it also happens to remove some overly complicated/duplicated code that\nwas literally converted from shell :)\n\n= Background\n\nWhen recursing into nested submodules, Git commands keep track of the path\nfrom the superproject to the submodule in order to print a \"display path\" to\nthe user, e.g.\n\n Submodule path '../super/sub/nested-sub': checked out 'abcdef'\n\nFor historical reasons, \"git submodule--helper update\" uses\n\"--recursive-prefix\" for this purpose, but it should use \"--super-prefix\"\ninstead because:\n\n * That's what every other command uses (not just submodule--helper\n subcommands).\n * Using the \"--super-prefix\" helper functions makes the \"git\n submodule--helper update\" code simpler\n\n= Patch organization\n\n * Patch 1/6 makes sure that display paths are only computed using display\n path helper functions ([do_]get_submodule_displaypath()) and fixes some\n display paths that we never realized were broken.\n * Patches 2-3/6 simplify and deduplicate some display path computations.\n * Patch 4/6 (authored by Ævar) removes SUPPORT_SUPER_PREFIX where it's not\n needed.\n * This doesn't affect correctness, but we want to do this eventually, so\n do it now to make 5/6 a bit cleaner.\n * Patch 5/6 replaces \"--recursive-prefix\" with \"--super-prefix\", making\n do_get_submodule_displaypath() obsolete.\n * GGG outputs an odd diff; I recommend viewing it with \"--histogram\"\n * Patch 6/6 removes do_get_submodule_displaypath().\n\n= Series history\n\nChanges in v3:\n* None (resend of v2 because v2 accidentally included\n ab/submodule-cleanup)\n\nChanges in v2:\n * Rebase onto ab/submodule-cleanup (previously master)\n * Cherry pick https://github.com/avar/git/commit/f445c57490d into 4/6\n * Style fixes in .c and tests\n\n= Future work\n\nAt the end of this series, get_submodule_displaypath() can be moved to\nsubmodule.h, which would make submodule.c:get_super_prefix_or_empty()\nobsolete. I have a patch that demonstrates this (CI run: [4]), but I decided\nto keep this series more focused on \"git submodule--helper update\" so that\nit's easier to review.\n\n[1] https://lore.kernel.org/git/xmqq35g5xmmv.fsf@gitster.g\n[2] https://lore.kernel.org/git/877a45867ae368bf9e053caedcb6cf421e02344d.1655336146.git.gitgitgadget@gmail.com\n[3] https://lore.kernel.org/git/patch-v3-02.12-082e015781e-20220622T142012Z-avarab@gmail.com\n[4] https://github.com/chooglen/git/actions/runs/2572557584\n\nGlen Choo (5):\n submodule--helper update: use display path helper\n submodule--helper: don't recreate recursive prefix\n submodule--helper: use correct display path helper\n submodule--helper update: use --super-prefix\n submodule--helper: remove display path helper\n\nÆvar Arnfjörð Bjarmason (1):\n submodule--helper: remove unused SUPPORT_SUPER_PREFIX flags\n\n builtin/submodule--helper.c | 87 ++++++++++---------------------------\n t/t7406-submodule-update.sh | 56 ++++++++++++++++++++++++\n 2 files changed, 79 insertions(+), 64 deletions(-)\n\nRange-diff against v1:\n1: 473548f2fa ! 1: 64c138df19 submodule--helper update: use display path helper\n @@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const st\n -\tstruct strbuf displaypath_sb = STRBUF_INIT;\n \tstruct strbuf sb = STRBUF_INIT;\n -\tconst char *displaypath = NULL;\n -+\tchar *displaypath = NULL;\n ++\tchar *displaypath;\n \tint needs_cloning = 0;\n \tint need_free_url = 0;\n \n @@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const st\n -\t\telse\n -\t\t\tstrbuf_addstr(&sb, ce->name);\n -\t\tstrbuf_addf(out, _(\"Skipping unmerged submodule %s\"), sb.buf);\n --\t\tstrbuf_addch(out, '\\n');\n -+\t\tstrbuf_addf(out, _(\"Skipping unmerged submodule '%s'\\n\"), displaypath);\n ++\t\tstrbuf_addf(out, _(\"Skipping unmerged submodule %s\"), displaypath);\n + \t\tstrbuf_addch(out, '\\n');\n \t\tgoto cleanup;\n \t}\n \n @@ t/t7406-submodule-update.sh: test_expect_success 'submodule update --filter sets\n +\ttest_when_finished \"rm -fr top-cloned\" &&\n +\tcp -r top-clean top-cloned &&\n +\n -+\t# Create an upstream commit in each repo\n ++\t# Create an upstream commit in each repo, starting with bottom\n +\ttest_commit -C bottom upstream_commit &&\n -+\t(cd middle &&\n -+\t git -C bottom fetch &&\n -+\t git -C bottom checkout -f FETCH_HEAD &&\n -+\t git add bottom &&\n -+\t git commit -m \"upstream_commit\"\n -+\t) &&\n -+\t(cd top &&\n -+\t git -C middle fetch &&\n -+\t git -C middle checkout -f FETCH_HEAD &&\n -+\t git add middle &&\n -+\t git commit -m \"upstream_commit\"\n -+\t) &&\n ++\t# Create middle commit\n ++\tgit -C middle/bottom fetch &&\n ++\tgit -C middle/bottom checkout -f FETCH_HEAD &&\n ++\tgit -C middle add bottom &&\n ++\tgit -C middle commit -m \"upstream_commit\" &&\n ++\t# Create top commit\n ++\tgit -C top/middle fetch &&\n ++\tgit -C top/middle checkout -f FETCH_HEAD &&\n ++\tgit -C top add middle &&\n ++\tgit -C top commit -m \"upstream_commit\" &&\n +\n +\t# Create a downstream conflict\n -+\t(cd top-cloned/middle &&\n -+\t test_commit -C bottom downstream_commit &&\n -+\t git add bottom &&\n -+\t git commit -m \"downstream_commit\" &&\n -+\t git fetch --recurse-submodules origin &&\n -+\t test_must_fail git merge origin/main\n -+\t) &&\n ++\ttest_commit -C top-cloned/middle/bottom downstream_commit &&\n ++\tgit -C top-cloned/middle add bottom &&\n ++\tgit -C top-cloned/middle commit -m \"downstream_commit\" &&\n ++\tgit -C top-cloned/middle fetch --recurse-submodules origin &&\n ++\ttest_must_fail git -C top-cloned/middle merge origin/main &&\n ++\n +\t# Make the update of \"middle\" a no-op, otherwise we error out\n +\t# because of its unmerged state\n +\ttest_config -C top-cloned submodule.middle.update !true &&\n +\tgit -C top-cloned submodule update --recursive 2>actual.err &&\n -+\tgrep \"Skipping unmerged submodule .middle/bottom.\" actual.err\n ++\tgrep -F \"Skipping unmerged submodule middle/bottom\" actual.err\n +'\n +\n +test_expect_success 'submodule update --recursive skip submodules with strategy=none' '\n @@ t/t7406-submodule-update.sh: test_expect_success 'submodule update --filter sets\n +\ttest_commit -C top-cloned/middle/bottom downstream_commit &&\n +\tgit -C top-cloned/middle config submodule.bottom.update none &&\n +\tgit -C top-cloned submodule update --recursive 2>actual.err &&\n -+\tgrep \"Skipping submodule .middle/bottom.\" actual.err\n ++\tgrep -F \"Skipping submodule ${SQ}middle/bottom${SQ}\" actual.err\n +'\n +\n test_done\n2: 618053730e ! 2: d3e803a463 submodule--helper: don't recreate recursive prefix\n @@ Commit message\n \n ## builtin/submodule--helper.c ##\n @@ builtin/submodule--helper.c: static void update_data_to_args(struct update_data *update_data, struct strvec *\n - {\n + \n \tstrvec_pushl(args, \"submodule--helper\", \"update\", \"--recursive\", NULL);\n \tstrvec_pushf(args, \"--jobs=%d\", update_data->max_jobs);\n -\tif (update_data->recursive_prefix)\n3: 7cd1c46f35 = 3: 1f7cf6ffaf submodule--helper: use correct display path helper\n-: ---------- > 4: 85e65f143b submodule--helper: remove unused SUPPORT_SUPER_PREFIX flags\n4: 57988287fc ! 5: 1d8b6b244d submodule--helper update: use --super-prefix\n @@ builtin/submodule--helper.c: struct submodule_update_clone {\n \tconst char *prefix;\n -\tconst char *recursive_prefix;\n \tconst char *displaypath;\n - \tconst char *update_default;\n + \tenum submodule_update_type update_default;\n \tstruct object_id suboid;\n @@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n \n @@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const st\n +\t\t\t\t\t\t get_super_prefix());\n \n \tif (ce_stage(ce)) {\n - \t\tstrbuf_addf(out, _(\"Skipping unmerged submodule '%s'\\n\"), displaypath);\n -@@ builtin/submodule--helper.c: static void ensure_core_worktree(const char *path)\n - \n - static void update_data_to_args(struct update_data *update_data, struct strvec *args)\n + \t\tstrbuf_addf(out, _(\"Skipping unmerged submodule %s\"), displaypath);\n +@@ builtin/submodule--helper.c: static void update_data_to_args(struct update_data *update_data, struct strvec *\n {\n + \tenum submodule_update_type update_type = update_data->update_default;\n + \n -\tstrvec_pushl(args, \"submodule--helper\", \"update\", \"--recursive\", NULL);\n -\tstrvec_pushf(args, \"--jobs=%d\", update_data->max_jobs);\n \tif (update_data->displaypath)\n @@ builtin/submodule--helper.c: static int module_update(int argc, const char **arg\n -\t\t\t 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, \"update\", &opt.update_default,\n - \t\t\t N_(\"string\"),\n - \t\t\t N_(\"rebase, merge, checkout or none\")),\n + \t\tOPT_SET_INT(0, \"checkout\", &opt.update_default,\n + \t\t\tN_(\"use the 'checkout' update strategy (default)\"),\n + \t\t\tSM_UPDATE_CHECKOUT),\n @@ builtin/submodule--helper.c: static int module_update(int argc, const char **argv, const char *prefix)\n \t\t\tmodule_list_active(&list);\n \n @@ builtin/submodule--helper.c: struct cmd_struct {\n \t{\"name\", module_name, 0},\n -\t{\"clone\", module_clone, 0},\n +\t{\"clone\", module_clone, SUPPORT_SUPER_PREFIX},\n - \t{\"add\", module_add, SUPPORT_SUPER_PREFIX},\n + \t{\"add\", module_add, 0},\n -\t{\"update\", module_update, 0},\n +\t{\"update\", module_update, SUPPORT_SUPER_PREFIX},\n \t{\"resolve-relative-url-test\", resolve_relative_url_test, 0},\n \t{\"foreach\", module_foreach, SUPPORT_SUPER_PREFIX},\n - \t{\"init\", module_init, SUPPORT_SUPER_PREFIX},\n + \t{\"init\", module_init, 0},\n5: 9fa13380b0 ! 6: a21e8cd174 submodule--helper: remove display path helper\n @@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const st\n +\t\tget_submodule_displaypath(ce->name, suc->update_data->prefix);\n \n \tif (ce_stage(ce)) {\n - \t\tstrbuf_addf(out, _(\"Skipping unmerged submodule '%s'\\n\"), displaypath);\n + \t\tstrbuf_addf(out, _(\"Skipping unmerged submodule %s\"), displaypath);\n @@ builtin/submodule--helper.c: static int update_submodule(struct update_data *update_data)\n {\n \tensure_core_worktree(update_data->sm_path);\n\nbase-commit: 5b893f7d81eb7feb43662ed8663e2af76a76b4c8\n-- \n2.37.0.rc0.161.g10f37bed90-goog\n\n"},{"id":"458279","messageId":"20220630221147.45689-2-chooglen@google.com","threadId":"58074","inReplyTo":"20220630221147.45689-1-chooglen@google.com","subject":"[PATCH v3 1/6] submodule--helper update: use display path helper","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-06-30T22:11:42Z","receivedAt":"2022-06-30T22:12:02Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"There are two locations in prepare_to_clone_next_submodule() that\nmanually calculate the submodule display path, but should just use\ndo_get_submodule_displaypath() for consistency.\n\nDo this replacement and reorder the code slightly to avoid computing\nthe display path twice.\n\nThis code was never tested, and adding tests shows that both these sites\nhave been computing the display path incorrectly ever since they were\nintroduced in 48308681b0 (git submodule update: have a dedicated helper\nfor cloning, 2016-02-29) [1]:\n\n- The first hunk puts a \"/\" between recursive_prefix and ce->name, but\n recursive_prefix already ends with \"/\".\n- The second hunk calls relative_path() on recursive_prefix and\n ce->name, but relative_path() only makes sense when both paths share\n the same base directory. This is never the case here:\n - recursive_prefix is the path from the topmost superproject to the\n current submodule\n - ce->name is the path from the root of the current submodule to its\n submodule.\n so, e.g. recursive_prefix=\"super\" and ce->name=\"submodule\" produces\n displayname=\"../super\" instead of \"super/submodule\".\n\nWhile we're fixing the display names, also fix inconsistent quoting of\nthe submodule name.\n\n[1] I verified this by applying the tests to 48308681b0.\n\nSigned-off-by: Glen Choo \n---\n builtin/submodule--helper.c | 21 +++++---------\n t/t7406-submodule-update.sh | 56 +++++++++++++++++++++++++++++++++++++\n 2 files changed, 63 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 389b900602..db2d5ab799 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1947,30 +1947,23 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n \tconst char *update_string;\n \tenum submodule_update_type update_type;\n \tchar *key;\n-\tstruct strbuf displaypath_sb = STRBUF_INIT;\n \tstruct strbuf sb = STRBUF_INIT;\n-\tconst char *displaypath = NULL;\n+\tchar *displaypath;\n \tint needs_cloning = 0;\n \tint need_free_url = 0;\n \n+\tdisplaypath = do_get_submodule_displaypath(ce->name,\n+\t\t\t\t\t\t suc->update_data->prefix,\n+\t\t\t\t\t\t suc->update_data->recursive_prefix);\n+\n \tif (ce_stage(ce)) {\n-\t\tif (suc->update_data->recursive_prefix)\n-\t\t\tstrbuf_addf(&sb, \"%s/%s\", suc->update_data->recursive_prefix, ce->name);\n-\t\telse\n-\t\t\tstrbuf_addstr(&sb, ce->name);\n-\t\tstrbuf_addf(out, _(\"Skipping unmerged submodule %s\"), sb.buf);\n+\t\tstrbuf_addf(out, _(\"Skipping unmerged submodule %s\"), displaypath);\n \t\tstrbuf_addch(out, '\\n');\n \t\tgoto cleanup;\n \t}\n \n \tsub = submodule_from_path(the_repository, null_oid(), ce->name);\n \n-\tif (suc->update_data->recursive_prefix)\n-\t\tdisplaypath = relative_path(suc->update_data->recursive_prefix,\n-\t\t\t\t\t ce->name, &displaypath_sb);\n-\telse\n-\t\tdisplaypath = ce->name;\n-\n \tif (!sub) {\n \t\tnext_submodule_warn_missing(suc, out, displaypath);\n \t\tgoto cleanup;\n@@ -2060,7 +2053,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n \t\t\t\t\t \"--no-single-branch\");\n \n cleanup:\n-\tstrbuf_release(&displaypath_sb);\n+\tfree(displaypath);\n \tstrbuf_release(&sb);\n \tif (need_free_url)\n \t\tfree((void*)url);\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex 06d804e213..f0408c5cc4 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -1116,4 +1116,60 @@ test_expect_success 'submodule update --filter sets partial clone settings' '\n \ttest_cmp_config -C super-filter/submodule blob:none remote.origin.partialclonefilter\n '\n \n+# NEEDSWORK: Clean up the tests so that we can reuse the test setup.\n+# Don't reuse the existing repos because the earlier tests have\n+# intentionally disruptive configurations.\n+test_expect_success 'setup clean recursive superproject' '\n+\tgit init bottom &&\n+\ttest_commit -C bottom \"bottom\" &&\n+\tgit init middle &&\n+\tgit -C middle submodule add ../bottom bottom &&\n+\tgit -C middle commit -m \"middle\" &&\n+\tgit init top &&\n+\tgit -C top submodule add ../middle middle &&\n+\tgit -C top commit -m \"top\" &&\n+\tgit clone --recurse-submodules top top-clean\n+'\n+\n+test_expect_success 'submodule update should skip unmerged submodules' '\n+\ttest_when_finished \"rm -fr top-cloned\" &&\n+\tcp -r top-clean top-cloned &&\n+\n+\t# Create an upstream commit in each repo, starting with bottom\n+\ttest_commit -C bottom upstream_commit &&\n+\t# Create middle commit\n+\tgit -C middle/bottom fetch &&\n+\tgit -C middle/bottom checkout -f FETCH_HEAD &&\n+\tgit -C middle add bottom &&\n+\tgit -C middle commit -m \"upstream_commit\" &&\n+\t# Create top commit\n+\tgit -C top/middle fetch &&\n+\tgit -C top/middle checkout -f FETCH_HEAD &&\n+\tgit -C top add middle &&\n+\tgit -C top commit -m \"upstream_commit\" &&\n+\n+\t# Create a downstream conflict\n+\ttest_commit -C top-cloned/middle/bottom downstream_commit &&\n+\tgit -C top-cloned/middle add bottom &&\n+\tgit -C top-cloned/middle commit -m \"downstream_commit\" &&\n+\tgit -C top-cloned/middle fetch --recurse-submodules origin &&\n+\ttest_must_fail git -C top-cloned/middle merge origin/main &&\n+\n+\t# Make the update of \"middle\" a no-op, otherwise we error out\n+\t# because of its unmerged state\n+\ttest_config -C top-cloned submodule.middle.update !true &&\n+\tgit -C top-cloned submodule update --recursive 2>actual.err &&\n+\tgrep -F \"Skipping unmerged submodule middle/bottom\" actual.err\n+'\n+\n+test_expect_success 'submodule update --recursive skip submodules with strategy=none' '\n+\ttest_when_finished \"rm -fr top-cloned\" &&\n+\tcp -r top-clean top-cloned &&\n+\n+\ttest_commit -C top-cloned/middle/bottom downstream_commit &&\n+\tgit -C top-cloned/middle config submodule.bottom.update none &&\n+\tgit -C top-cloned submodule update --recursive 2>actual.err &&\n+\tgrep -F \"Skipping submodule ${SQ}middle/bottom${SQ}\" actual.err\n+'\n+\n test_done\n-- \n2.37.0.rc0.161.g10f37bed90-goog\n\n"},{"id":"458280","messageId":"20220630221147.45689-3-chooglen@google.com","threadId":"58074","inReplyTo":"20220630221147.45689-1-chooglen@google.com","subject":"[PATCH v3 2/6] submodule--helper: don't recreate recursive prefix","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-06-30T22:11:43Z","receivedAt":"2022-06-30T22:12:05Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"update_submodule() uses duplicated code to compute\nupdate_data->displaypath and next.recursive_prefix. The latter is just\nthe former with \"/\" appended to it, and since update_data->displaypath\nnot changed outside of this statement, we can just reuse the already\ncomputed result.\n\nWe can go one step further and remove the reference to\nnext.recursive_prefix altogether. Since it is only used in\nupdate_data_to_args() (to compute the \"--recursive-prefix\" flag for the\nrecursive update child process) we can just use the already computed\n.displaypath value of there.\n\nDelete the duplicated code, and remove the unnecessary reference to\nnext.recursive_prefix. As a bonus, this fixes a memory leak where\nprefixed_path was never freed (this leak was first reported in [1]).\n\n[1] https://lore.kernel.org/git/877a45867ae368bf9e053caedcb6cf421e02344d.1655336146.git.gitgitgadget@gmail.com\n\nSigned-off-by: Glen Choo \n---\n builtin/submodule--helper.c | 14 +++-----------\n 1 file changed, 3 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex db2d5ab799..c104b1a023 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2418,9 +2418,9 @@ static void update_data_to_args(struct update_data *update_data, struct strvec *\n \n \tstrvec_pushl(args, \"submodule--helper\", \"update\", \"--recursive\", NULL);\n \tstrvec_pushf(args, \"--jobs=%d\", update_data->max_jobs);\n-\tif (update_data->recursive_prefix)\n-\t\tstrvec_pushl(args, \"--recursive-prefix\",\n-\t\t\t update_data->recursive_prefix, NULL);\n+\tif (update_data->displaypath)\n+\t\tstrvec_pushf(args, \"--recursive-prefix=%s/\",\n+\t\t\t update_data->displaypath);\n \tif (update_data->quiet)\n \t\tstrvec_push(args, \"--quiet\");\n \tif (update_data->force)\n@@ -2516,14 +2516,6 @@ static int update_submodule(struct update_data *update_data)\n \t\tstruct update_data next = *update_data;\n \t\tint res;\n \n-\t\tif (update_data->recursive_prefix)\n-\t\t\tprefixed_path = xstrfmt(\"%s%s/\", update_data->recursive_prefix,\n-\t\t\t\t\t\tupdate_data->sm_path);\n-\t\telse\n-\t\t\tprefixed_path = xstrfmt(\"%s/\", update_data->sm_path);\n-\n-\t\tnext.recursive_prefix = get_submodule_displaypath(prefixed_path,\n-\t\t\t\t\t\t\t\t update_data->prefix);\n \t\tnext.prefix = NULL;\n \t\toidcpy(&next.oid, null_oid());\n \t\toidcpy(&next.suboid, null_oid());\n-- \n2.37.0.rc0.161.g10f37bed90-goog\n\n"},{"id":"458281","messageId":"20220630221147.45689-4-chooglen@google.com","threadId":"58074","inReplyTo":"20220630221147.45689-1-chooglen@google.com","subject":"[PATCH v3 3/6] submodule--helper: use correct display path helper","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-06-30T22:11:44Z","receivedAt":"2022-06-30T22:12:16Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Replace a chunk of code in update_submodule() with an equivalent\ndo_get_submodule_displaypath() invocation. This is already tested by\nt/t7406-submodule-update.sh:'submodule update --init --recursive from\nsubdirectory', so no tests are added.\n\nThe two are equivalent because:\n\n- Exactly one of recursive_prefix|prefix is non-NULL at a time; prefix\n is set at the superproject level, and recursive_prefix is set when\n recursing into submodules. There is also a BUG() statement in\n get_submodule_displaypath() that asserts that both cannot be non-NULL.\n\n- In get_submodule_displaypath(), get_super_prefix() always returns NULL\n because \"--super-prefix\" is never passed. Thus calling it is\n equivalent to calling do_get_submodule_displaypath() with super_prefix\n = NULL.\n\nTherefore:\n\n- When recursive_prefix is non-NULL, prefix is NULL, and thus\n get_submodule_displaypath() just returns prefixed_path. This is\n identical to calling do_get_submodule_displaypath() with super_prefix\n = recursive_prefix because the return value is still the concatenation\n of recursive_prefix + update_data->sm_path.\n\n- When prefix is non-NULL, prefixed_path = update_data->sm_path. Thus\n calling get_submodule_displaypath() with prefixed_path is equivalent\n to calling do_get_submodule_displaypath() with update_data->sm_path\n\nSigned-off-by: Glen Choo \n---\n builtin/submodule--helper.c | 14 +++-----------\n 1 file changed, 3 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex c104b1a023..aad431f898 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2464,19 +2464,11 @@ static void update_data_to_args(struct update_data *update_data, struct strvec *\n \n static int update_submodule(struct update_data *update_data)\n {\n-\tchar *prefixed_path;\n-\n \tensure_core_worktree(update_data->sm_path);\n \n-\tif (update_data->recursive_prefix)\n-\t\tprefixed_path = xstrfmt(\"%s%s\", update_data->recursive_prefix,\n-\t\t\t\t\tupdate_data->sm_path);\n-\telse\n-\t\tprefixed_path = xstrdup(update_data->sm_path);\n-\n-\tupdate_data->displaypath = get_submodule_displaypath(prefixed_path,\n-\t\t\t\t\t\t\t update_data->prefix);\n-\tfree(prefixed_path);\n+\tupdate_data->displaypath = do_get_submodule_displaypath(update_data->sm_path,\n+\t\t\t\t\t\t\t\tupdate_data->prefix,\n+\t\t\t\t\t\t\t\tupdate_data->recursive_prefix);\n \n \tdetermine_submodule_update_strategy(the_repository, update_data->just_cloned,\n \t\t\t\t\t update_data->sm_path, update_data->update_default,\n-- \n2.37.0.rc0.161.g10f37bed90-goog\n\n"},{"id":"458282","messageId":"20220630221147.45689-5-chooglen@google.com","threadId":"58074","inReplyTo":"20220630221147.45689-1-chooglen@google.com","subject":"[PATCH v3 4/6] submodule--helper: remove unused SUPPORT_SUPER_PREFIX flags","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-06-30T22:11:45Z","receivedAt":"2022-06-30T22:12:18Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"From: Ævar Arnfjörð Bjarmason \n\nRemove the SUPPORT_SUPER_PREFIX flag from \"add\", \"init\" and\n\"summary\". For the \"add\" command it hasn't been used since [1],\nlikewise for \"init\" and \"summary\" since [2] and [3], respectively.\n\nAs implemented in 74866d75793 (git: make super-prefix option,\n2016-10-07) the SUPPORT_SUPER_PREFIX flag in git.c applies for the\nentire command, but as implemented in 89c86265576 (submodule helper:\nsupport super prefix, 2016-12-08) we assert here in\ncmd_submodule__helper() that we're not getting the flag unexpectedly.\n\n1. 8c8195e9c3e (submodule--helper: introduce add-clone subcommand,\n 2021-07-10)\n2. 6e7c14e65c8 (submodule update --init: display correct path from\n submodule, 2017-01-06)\n3. 1cf823d8f00 (submodule: remove unnecessary `prefix` based option\n logic, 2021-06-22)\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \nSigned-off-by: Glen Choo \n---\n builtin/submodule--helper.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex aad431f898..360309195f 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -3379,15 +3379,15 @@ static struct cmd_struct commands[] = {\n \t{\"list\", module_list, 0},\n \t{\"name\", module_name, 0},\n \t{\"clone\", module_clone, 0},\n-\t{\"add\", module_add, SUPPORT_SUPER_PREFIX},\n+\t{\"add\", module_add, 0},\n \t{\"update\", module_update, 0},\n \t{\"resolve-relative-url-test\", resolve_relative_url_test, 0},\n \t{\"foreach\", module_foreach, SUPPORT_SUPER_PREFIX},\n-\t{\"init\", module_init, SUPPORT_SUPER_PREFIX},\n+\t{\"init\", module_init, 0},\n \t{\"status\", module_status, SUPPORT_SUPER_PREFIX},\n \t{\"sync\", module_sync, SUPPORT_SUPER_PREFIX},\n \t{\"deinit\", module_deinit, 0},\n-\t{\"summary\", module_summary, SUPPORT_SUPER_PREFIX},\n+\t{\"summary\", module_summary, 0},\n \t{\"push-check\", push_check, 0},\n \t{\"absorbgitdirs\", absorb_git_dirs, SUPPORT_SUPER_PREFIX},\n \t{\"is-active\", is_active, 0},\n-- \n2.37.0.rc0.161.g10f37bed90-goog\n\n"},{"id":"458283","messageId":"20220630221147.45689-6-chooglen@google.com","threadId":"58074","inReplyTo":"20220630221147.45689-1-chooglen@google.com","subject":"[PATCH v3 5/6] submodule--helper update: use --super-prefix","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-06-30T22:11:46Z","receivedAt":"2022-06-30T22:12:20Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Unlike the other subcommands, \"git submodule--helper update\" uses the\n\"--recursive-prefix\" flag instead of \"--super-prefix\". The two flags are\notherwise identical (they only serve to compute the 'display path' of a\nsubmodule), except that there is a dedicated helper function to get the\nvalue of \"--super-prefix\".\n\nThis inconsistency exists because \"git submodule update\" used to pass\n\"--recursive-prefix\" between shell and C (introduced in [1]) before\n\"--super-prefix\" was introduced (in [2]), and for simplicity, we kept\nthis name when \"git submodule--helper update\" was created.\n\nRemove \"--recursive-prefix\" and its associated code from \"git\nsubmodule--helper update\", replacing it with \"--super-prefix\".\n\nTo use \"--super-prefix\", module_update is marked with\nSUPPORT_SUPER_PREFIX. Note that module_clone must also be marked with\nSUPPORT_SUPER_PREFIX, otherwise the \"git submodule--helper clone\"\nsubprocess will fail check because \"--super-prefix\" is propagated via\nthe environment.\n\n[1] 48308681b0 (git submodule update: have a dedicated helper for\ncloning, 2016-02-29)\n[2] 74866d7579 (git: make super-prefix option, 2016-10-07)\n\nSigned-off-by: Glen Choo \n---\n builtin/submodule--helper.c | 30 ++++++++++--------------------\n 1 file changed, 10 insertions(+), 20 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 360309195f..41177332dd 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -477,22 +477,18 @@ static int starts_with_dot_dot_slash(const char *const path)\n \n struct init_cb {\n \tconst char *prefix;\n-\tconst char *superprefix;\n \tunsigned int flags;\n };\n #define INIT_CB_INIT { 0 }\n \n static void init_submodule(const char *path, const char *prefix,\n-\t\t\t const char *superprefix, unsigned int flags)\n+\t\t\t unsigned int flags)\n {\n \tconst struct submodule *sub;\n \tstruct strbuf sb = STRBUF_INIT;\n \tchar *upd = NULL, *url = NULL, *displaypath;\n \n-\t/* try superprefix from the environment, if it is not passed explicitly */\n-\tif (!superprefix)\n-\t\tsuperprefix = get_super_prefix();\n-\tdisplaypath = do_get_submodule_displaypath(path, prefix, superprefix);\n+\tdisplaypath = do_get_submodule_displaypath(path, prefix, get_super_prefix());\n \n \tsub = submodule_from_path(the_repository, null_oid(), path);\n \n@@ -566,7 +562,7 @@ static void init_submodule(const char *path, const char *prefix,\n static void init_submodule_cb(const struct cache_entry *list_item, void *cb_data)\n {\n \tstruct init_cb *info = cb_data;\n-\tinit_submodule(list_item->name, info->prefix, info->superprefix, info->flags);\n+\tinit_submodule(list_item->name, info->prefix, info->flags);\n }\n \n static int module_init(int argc, const char **argv, const char *prefix)\n@@ -1878,7 +1874,6 @@ struct submodule_update_clone {\n \n struct update_data {\n \tconst char *prefix;\n-\tconst char *recursive_prefix;\n \tconst char *displaypath;\n \tenum submodule_update_type update_default;\n \tstruct object_id suboid;\n@@ -1954,7 +1949,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n \n \tdisplaypath = do_get_submodule_displaypath(ce->name,\n \t\t\t\t\t\t suc->update_data->prefix,\n-\t\t\t\t\t\t suc->update_data->recursive_prefix);\n+\t\t\t\t\t\t get_super_prefix());\n \n \tif (ce_stage(ce)) {\n \t\tstrbuf_addf(out, _(\"Skipping unmerged submodule %s\"), displaypath);\n@@ -2416,11 +2411,11 @@ static void update_data_to_args(struct update_data *update_data, struct strvec *\n {\n \tenum submodule_update_type update_type = update_data->update_default;\n \n-\tstrvec_pushl(args, \"submodule--helper\", \"update\", \"--recursive\", NULL);\n-\tstrvec_pushf(args, \"--jobs=%d\", update_data->max_jobs);\n \tif (update_data->displaypath)\n-\t\tstrvec_pushf(args, \"--recursive-prefix=%s/\",\n+\t\tstrvec_pushf(args, \"--super-prefix=%s/\",\n \t\t\t update_data->displaypath);\n+\tstrvec_pushl(args, \"submodule--helper\", \"update\", \"--recursive\", NULL);\n+\tstrvec_pushf(args, \"--jobs=%d\", update_data->max_jobs);\n \tif (update_data->quiet)\n \t\tstrvec_push(args, \"--quiet\");\n \tif (update_data->force)\n@@ -2468,7 +2463,7 @@ static int update_submodule(struct update_data *update_data)\n \n \tupdate_data->displaypath = do_get_submodule_displaypath(update_data->sm_path,\n \t\t\t\t\t\t\t\tupdate_data->prefix,\n-\t\t\t\t\t\t\t\tupdate_data->recursive_prefix);\n+\t\t\t\t\t\t\t\tget_super_prefix());\n \n \tdetermine_submodule_update_strategy(the_repository, update_data->just_cloned,\n \t\t\t\t\t update_data->sm_path, update_data->update_default,\n@@ -2592,10 +2587,6 @@ static int module_update(int argc, const char **argv, const char *prefix)\n \t\tOPT_STRING(0, \"prefix\", &opt.prefix,\n \t\t\t N_(\"path\"),\n \t\t\t N_(\"path into the working tree\")),\n-\t\tOPT_STRING(0, \"recursive-prefix\", &opt.recursive_prefix,\n-\t\t\t N_(\"path\"),\n-\t\t\t N_(\"path into the working tree, across nested \"\n-\t\t\t \"submodule boundaries\")),\n \t\tOPT_SET_INT(0, \"checkout\", &opt.update_default,\n \t\t\tN_(\"use the 'checkout' update strategy (default)\"),\n \t\t\tSM_UPDATE_CHECKOUT),\n@@ -2681,7 +2672,6 @@ static int module_update(int argc, const char **argv, const char *prefix)\n \t\t\tmodule_list_active(&list);\n \n \t\tinfo.prefix = opt.prefix;\n-\t\tinfo.superprefix = opt.recursive_prefix;\n \t\tif (opt.quiet)\n \t\t\tinfo.flags |= OPT_QUIET;\n \n@@ -3378,9 +3368,9 @@ struct cmd_struct {\n static struct cmd_struct commands[] = {\n \t{\"list\", module_list, 0},\n \t{\"name\", module_name, 0},\n-\t{\"clone\", module_clone, 0},\n+\t{\"clone\", module_clone, SUPPORT_SUPER_PREFIX},\n \t{\"add\", module_add, 0},\n-\t{\"update\", module_update, 0},\n+\t{\"update\", module_update, SUPPORT_SUPER_PREFIX},\n \t{\"resolve-relative-url-test\", resolve_relative_url_test, 0},\n \t{\"foreach\", module_foreach, SUPPORT_SUPER_PREFIX},\n \t{\"init\", module_init, 0},\n-- \n2.37.0.rc0.161.g10f37bed90-goog\n\n"},{"id":"458284","messageId":"20220630221147.45689-7-chooglen@google.com","threadId":"58074","inReplyTo":"20220630221147.45689-1-chooglen@google.com","subject":"[PATCH v3 6/6] submodule--helper: remove display path helper","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-06-30T22:11:47Z","receivedAt":"2022-06-30T22:12:22Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"All invocations of do_get_submodule_displaypath() pass\nget_super_prefix() as the super_prefix arg, which is exactly the same\nas get_submodule_displaypath().\n\nReplace all calls to do_get_submodule_displaypath() with\nget_submodule_displaypath(), and since it has no more callers, remove\nit.\n\nSigned-off-by: Glen Choo \n---\n builtin/submodule--helper.c | 26 +++++++++-----------------\n 1 file changed, 9 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 41177332dd..3e0ceb5258 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -118,10 +118,11 @@ static int resolve_relative_url_test(int argc, const char **argv, const char *pr\n \treturn 0;\n }\n \n-static char *do_get_submodule_displaypath(const char *path,\n-\t\t\t\t\t const char *prefix,\n-\t\t\t\t\t const char *super_prefix)\n+/* the result should be freed by the caller. */\n+static char *get_submodule_displaypath(const char *path, const char *prefix)\n {\n+\tconst char *super_prefix = get_super_prefix();\n+\n \tif (prefix && super_prefix) {\n \t\tBUG(\"cannot have prefix '%s' and superprefix '%s'\",\n \t\t prefix, super_prefix);\n@@ -137,13 +138,6 @@ static char *do_get_submodule_displaypath(const char *path,\n \t}\n }\n \n-/* the result should be freed by the caller. */\n-static char *get_submodule_displaypath(const char *path, const char *prefix)\n-{\n-\tconst char *super_prefix = get_super_prefix();\n-\treturn do_get_submodule_displaypath(path, prefix, super_prefix);\n-}\n-\n static char *compute_rev_name(const char *sub_path, const char* object_id)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n@@ -488,7 +482,7 @@ static void init_submodule(const char *path, const char *prefix,\n \tstruct strbuf sb = STRBUF_INIT;\n \tchar *upd = NULL, *url = NULL, *displaypath;\n \n-\tdisplaypath = do_get_submodule_displaypath(path, prefix, get_super_prefix());\n+\tdisplaypath = get_submodule_displaypath(path, prefix);\n \n \tsub = submodule_from_path(the_repository, null_oid(), path);\n \n@@ -1947,9 +1941,8 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n \tint needs_cloning = 0;\n \tint need_free_url = 0;\n \n-\tdisplaypath = do_get_submodule_displaypath(ce->name,\n-\t\t\t\t\t\t suc->update_data->prefix,\n-\t\t\t\t\t\t get_super_prefix());\n+\tdisplaypath =\n+\t\tget_submodule_displaypath(ce->name, suc->update_data->prefix);\n \n \tif (ce_stage(ce)) {\n \t\tstrbuf_addf(out, _(\"Skipping unmerged submodule %s\"), displaypath);\n@@ -2461,9 +2454,8 @@ static int update_submodule(struct update_data *update_data)\n {\n \tensure_core_worktree(update_data->sm_path);\n \n-\tupdate_data->displaypath = do_get_submodule_displaypath(update_data->sm_path,\n-\t\t\t\t\t\t\t\tupdate_data->prefix,\n-\t\t\t\t\t\t\t\tget_super_prefix());\n+\tupdate_data->displaypath = get_submodule_displaypath(\n+\t\tupdate_data->sm_path, update_data->prefix);\n \n \tdetermine_submodule_update_strategy(the_repository, update_data->just_cloned,\n \t\t\t\t\t update_data->sm_path, update_data->update_default,\n-- \n2.37.0.rc0.161.g10f37bed90-goog\n\n"},{"id":"458289","messageId":"220701.865ykhdboc.gmgdl@evledraar.gmail.com","threadId":"58074","inReplyTo":"pull.1282.v2.git.git.1656623978.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 00/18] submodule: remove \"--recursive-prefix\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-30T22:16:47Z","receivedAt":"2022-06-30T22:33:16Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Jun 30 2022, Glen Choo via GitGitGadget wrote:\n\n> Thanks Ævar for the feedback! I've incorporated all of the suggestions,\n> except breaking up the tests in 1/6 into their own patch - I wasn't\n> convinced of the value of having prescriptive tests (with\n> test_expect_failure), and I found it difficult to discuss descriptive tests\n> (with test_expect_success) without also having the code in the same diff.\n> FWIW, this version (and the previous one) \"git rebase -x 'make test'\"\n> cleanly :)\n>\n> Note for Junio: this version is based on ab/submodule-cleanup (and so will\n> future versions).\n>\n> = Description\n>\n> This series is a refactor of \"git submodule--helper update\" that replaces\n> \"--recursive-prefix\" with \"--super-prefix\" (see Background). This was\n> initially motivated by:\n>\n> * Junio's suggestion to simplify the code [1] (in response to a memory leak\n> found by Johannes Schindelin [2]).\n> * A desire to use the module_list API + get_submodule_displaypath() outside\n> of builtin/submodule--helper.c (I expect to use this to check out\n> branches in each submodule).\n>\n> But it also happens to remove some overly complicated/duplicated code that\n> was literally converted from shell :)\n>\n> = Background\n>\n> When recursing into nested submodules, Git commands keep track of the path\n> from the superproject to the submodule in order to print a \"display path\" to\n> the user, e.g.\n>\n> Submodule path '../super/sub/nested-sub': checked out 'abcdef'\n>\n> For historical reasons, \"git submodule--helper update\" uses\n> \"--recursive-prefix\" for this purpose, but it should use \"--super-prefix\"\n> instead because:\n>\n> * That's what every other command uses (not just submodule--helper\n> subcommands).\n> * Using the \"--super-prefix\" helper functions makes the \"git\n> submodule--helper update\" code simpler\n>\n> = Patch organization\n>\n> * Patch 1/6 makes sure that display paths are only computed using display\n> path helper functions ([do_]get_submodule_displaypath()) and fixes some\n> display paths that we never realized were broken.\n> * Patches 2-3/6 simplify and deduplicate some display path computations.\n> * Patch 4/6 (authored by Ævar) removes SUPPORT_SUPER_PREFIX where it's not\n> needed.\n> * This doesn't affect correctness, but we want to do this eventually, so\n> do it now to make 5/6 a bit cleaner.\n> * Patch 5/6 replaces \"--recursive-prefix\" with \"--super-prefix\", making\n> do_get_submodule_displaypath() obsolete.\n> * GGG outputs an odd diff; I recommend viewing it with \"--histogram\"\n> * Patch 6/6 removes do_get_submodule_displaypath().\n>\n> = Series history\n>\n> Changes in v2:\n>\n> * Rebase onto ab/submodule-cleanup (previously master)\n> * Cherry pick https://github.com/avar/git/commit/f445c57490d into 4/6\n> * Style fixes in .c and tests\n\nThanks for the update, Looks like something happened with GGG to send\nthe base series + yours, I confirmed that 1-13/18 are the same as my\nseries in gitster/ab/submodule-cleanup.\n\nI played around with this a bit, and pushed some amends to\nhttps://github.com/avar/git/tree/avar/pr-git-1282/chooglen/submodule/remove-recursive-prefix-v2;\nwhich you should take more as the results of poking around, I think this\nis fine as-is if Junio would like to pick it up, sans the possible bug\nmentioned below.\n\nA range-diff of the changes I made follows below, changes:\n\n * Split up the \"add tests\" to a new commit :) You mentioned\n \"test_expect_failure\", I just assumed we'd use test_expect_succes,\n and then the 2nd commit has this test change:\n\t\n\tdiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\n\tindex 9a076e025f3..6cc07460dd2 100755\n\t--- a/t/t7406-submodule-update.sh\n\t+++ b/t/t7406-submodule-update.sh\n\t@@ -1163 +1163 @@ test_expect_success 'submodule update should skip unmerged submodules' '\n\t- Skipping unmerged submodule middle//bottom\n\t+ Skipping unmerged submodule middle/bottom\n\t@@ -1176 +1176 @@ test_expect_success 'submodule update --recursive skip submodules with strategy=\n\t- Skipping submodule '\\''../middle/'\\''\n\t+ Skipping submodule '\\''middle/bottom'\\''\n\n Which I think makes it much easier to explain the change itself, the\n first commit basically just says \"tests for existing behavior, see\n the next commit for details\".\n\n * Used test_cmp instead of \"grep -F\", makes for easier to grok output :)\n\n * I had to squint a bit at the change from strvec_pushl() to\n strvec_pushf().\n\n I.e. we're changing both that we're passing \"--foo bar\"\n v.s. \"--foo=bar\" *and* \"bar\" v.s. \"bar/\", but as the commit message\n notes it's just the latter that matters.\n\n Just a nit, but I think it's a bit easier to reason about just\n changing the one thing we need to change there, although that means\n giving the \"if\" braces. This also leaves the \"--super-prefix\" code\n consistent with all the other places where we\n \"strvec.*--super-prefix\" at the end.\n\n * We can just declare some of the variables and initialize them at the\n beginning, but couldn't when they were a strbuf.\n\n * Your 18/18 has some odd code formatting of \"var =\\n\\t\\tvalue(...\"\n when we usually do \"var = value(...\\n...\".\n\n * You appear to have (and I haven't \"fixed\" it here) in 13/18 mentioned\n \"While we're fixing the display names, also fix inconsistent quoting\n of the submodule name\", but as the test_cmp shows we appear not to\n have done that part at all? Is this referring to a forgotten change\n where we should be saying (note the added '-quotes):\n\n Skipping unmerged submodule 'middle/bottom'\n\n Either that's what you mean & you forgot, or we're missing a test, or\n I'm misunderstanding it...\n\nI hope this helps, at least some of it...\n\n1: 64c138df196 ! 1: 3d9977006d3 submodule--helper update: use display path helper\n @@ Metadata\n Author: Glen Choo \n \n ## Commit message ##\n - submodule--helper update: use display path helper\n + submodule--helper tests: add missing \"display path\" coverage\n \n There are two locations in prepare_to_clone_next_submodule() that\n - manually calculate the submodule display path, but should just use\n - do_get_submodule_displaypath() for consistency.\n -\n - Do this replacement and reorder the code slightly to avoid computing\n - the display path twice.\n -\n - This code was never tested, and adding tests shows that both these sites\n - have been computing the display path incorrectly ever since they were\n - introduced in 48308681b0 (git submodule update: have a dedicated helper\n - for cloning, 2016-02-29) [1]:\n -\n - - The first hunk puts a \"/\" between recursive_prefix and ce->name, but\n - recursive_prefix already ends with \"/\".\n - - The second hunk calls relative_path() on recursive_prefix and\n - ce->name, but relative_path() only makes sense when both paths share\n - the same base directory. This is never the case here:\n - - recursive_prefix is the path from the topmost superproject to the\n - current submodule\n - - ce->name is the path from the root of the current submodule to its\n - submodule.\n - so, e.g. recursive_prefix=\"super\" and ce->name=\"submodule\" produces\n - displayname=\"../super\" instead of \"super/submodule\".\n -\n - While we're fixing the display names, also fix inconsistent quoting of\n - the submodule name.\n -\n - [1] I verified this by applying the tests to 48308681b0.\n + manually calculate the submodule display path, as discussed in the\n + next commit the \"Skipping\" output isn't exactly what we want, but\n + let's test how we behave now, before changing the existing behavior.\n \n Signed-off-by: Glen Choo \n -\n - ## builtin/submodule--helper.c ##\n -@@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n - \tconst char *update_string;\n - \tenum submodule_update_type update_type;\n - \tchar *key;\n --\tstruct strbuf displaypath_sb = STRBUF_INIT;\n - \tstruct strbuf sb = STRBUF_INIT;\n --\tconst char *displaypath = NULL;\n -+\tchar *displaypath;\n - \tint needs_cloning = 0;\n - \tint need_free_url = 0;\n - \n -+\tdisplaypath = do_get_submodule_displaypath(ce->name,\n -+\t\t\t\t\t\t suc->update_data->prefix,\n -+\t\t\t\t\t\t suc->update_data->recursive_prefix);\n -+\n - \tif (ce_stage(ce)) {\n --\t\tif (suc->update_data->recursive_prefix)\n --\t\t\tstrbuf_addf(&sb, \"%s/%s\", suc->update_data->recursive_prefix, ce->name);\n --\t\telse\n --\t\t\tstrbuf_addstr(&sb, ce->name);\n --\t\tstrbuf_addf(out, _(\"Skipping unmerged submodule %s\"), sb.buf);\n -+\t\tstrbuf_addf(out, _(\"Skipping unmerged submodule %s\"), displaypath);\n - \t\tstrbuf_addch(out, '\\n');\n - \t\tgoto cleanup;\n - \t}\n - \n - \tsub = submodule_from_path(the_repository, null_oid(), ce->name);\n - \n --\tif (suc->update_data->recursive_prefix)\n --\t\tdisplaypath = relative_path(suc->update_data->recursive_prefix,\n --\t\t\t\t\t ce->name, &displaypath_sb);\n --\telse\n --\t\tdisplaypath = ce->name;\n --\n - \tif (!sub) {\n - \t\tnext_submodule_warn_missing(suc, out, displaypath);\n - \t\tgoto cleanup;\n -@@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n - \t\t\t\t\t \"--no-single-branch\");\n - \n - cleanup:\n --\tstrbuf_release(&displaypath_sb);\n -+\tfree(displaypath);\n - \tstrbuf_release(&sb);\n - \tif (need_free_url)\n - \t\tfree((void*)url);\n + Signed-off-by: Ævar Arnfjörð Bjarmason \n \n ## t/t7406-submodule-update.sh ##\n @@ t/t7406-submodule-update.sh: test_expect_success 'submodule update --filter sets partial clone settings' '\n @@ t/t7406-submodule-update.sh: test_expect_success 'submodule update --filter sets\n +\t# because of its unmerged state\n +\ttest_config -C top-cloned submodule.middle.update !true &&\n +\tgit -C top-cloned submodule update --recursive 2>actual.err &&\n -+\tgrep -F \"Skipping unmerged submodule middle/bottom\" actual.err\n ++\tcat >expect.err <<-\\EOF &&\n ++\tSkipping unmerged submodule middle//bottom\n ++\tEOF\n ++\ttest_cmp expect.err actual.err\n +'\n +\n +test_expect_success 'submodule update --recursive skip submodules with strategy=none' '\n @@ t/t7406-submodule-update.sh: test_expect_success 'submodule update --filter sets\n +\ttest_commit -C top-cloned/middle/bottom downstream_commit &&\n +\tgit -C top-cloned/middle config submodule.bottom.update none &&\n +\tgit -C top-cloned submodule update --recursive 2>actual.err &&\n -+\tgrep -F \"Skipping submodule ${SQ}middle/bottom${SQ}\" actual.err\n ++\tcat >expect.err <<-\\EOF &&\n ++\tSkipping submodule '\\''../middle/'\\''\n ++\tEOF\n ++\ttest_cmp expect.err actual.err\n +'\n +\n test_done\n-: ----------- > 2: d1a47d302ee submodule--helper update: use display path helper\n2: d3e803a4630 ! 3: a5b30ac94e6 submodule--helper: don't recreate recursive prefix\n @@ builtin/submodule--helper.c: static void update_data_to_args(struct update_data\n -\tif (update_data->recursive_prefix)\n -\t\tstrvec_pushl(args, \"--recursive-prefix\",\n -\t\t\t update_data->recursive_prefix, NULL);\n -+\tif (update_data->displaypath)\n -+\t\tstrvec_pushf(args, \"--recursive-prefix=%s/\",\n -+\t\t\t update_data->displaypath);\n ++\tif (update_data->displaypath) {\n ++\t\tstrvec_push(args, \"--recursive-prefix\");\n ++\t\tstrvec_pushf(args, \"%s/\", update_data->displaypath);\n ++\t}\n \tif (update_data->quiet)\n \t\tstrvec_push(args, \"--quiet\");\n \tif (update_data->force)\n3: 1f7cf6ffaf1 = 4: 0ea102882a7 submodule--helper: use correct display path helper\n4: 85e65f143b6 = 5: 8085bc83a0c submodule--helper: remove unused SUPPORT_SUPER_PREFIX flags\n5: 1d8b6b244dc ! 6: 4c00a1b496a submodule--helper update: use --super-prefix\n @@ builtin/submodule--helper.c: struct submodule_update_clone {\n \tenum submodule_update_type update_default;\n \tstruct object_id suboid;\n @@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n - \n - \tdisplaypath = do_get_submodule_displaypath(ce->name,\n - \t\t\t\t\t\t suc->update_data->prefix,\n --\t\t\t\t\t\t suc->update_data->recursive_prefix);\n -+\t\t\t\t\t\t get_super_prefix());\n - \n - \tif (ce_stage(ce)) {\n - \t\tstrbuf_addf(out, _(\"Skipping unmerged submodule %s\"), displaypath);\n + \tchar *key;\n + \tstruct update_data *ud = suc->update_data;\n + \tchar *displaypath = do_get_submodule_displaypath(ce->name, ud->prefix,\n +-\t\t\t\t\t\t\t ud->recursive_prefix);\n ++\t\t\t\t\t\t\t get_super_prefix());\n + \tstruct strbuf sb = STRBUF_INIT;\n + \tint needs_cloning = 0;\n + \tint need_free_url = 0;\n @@ builtin/submodule--helper.c: static void update_data_to_args(struct update_data *update_data, struct strvec *\n {\n \tenum submodule_update_type update_type = update_data->update_default;\n \n -\tstrvec_pushl(args, \"submodule--helper\", \"update\", \"--recursive\", NULL);\n -\tstrvec_pushf(args, \"--jobs=%d\", update_data->max_jobs);\n - \tif (update_data->displaypath)\n --\t\tstrvec_pushf(args, \"--recursive-prefix=%s/\",\n -+\t\tstrvec_pushf(args, \"--super-prefix=%s/\",\n - \t\t\t update_data->displaypath);\n + \tif (update_data->displaypath) {\n +-\t\tstrvec_push(args, \"--recursive-prefix\");\n ++\t\tstrvec_push(args, \"--super-prefix\");\n + \t\tstrvec_pushf(args, \"%s/\", update_data->displaypath);\n + \t}\n +\tstrvec_pushl(args, \"submodule--helper\", \"update\", \"--recursive\", NULL);\n +\tstrvec_pushf(args, \"--jobs=%d\", update_data->max_jobs);\n \tif (update_data->quiet)\n6: a21e8cd174d ! 7: 639f07e4b84 submodule--helper: remove display path helper\n @@ builtin/submodule--helper.c: static void init_submodule(const char *path, const\n \tsub = submodule_from_path(the_repository, null_oid(), path);\n \n @@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n + \tenum submodule_update_type update_type;\n + \tchar *key;\n + \tstruct update_data *ud = suc->update_data;\n +-\tchar *displaypath = do_get_submodule_displaypath(ce->name, ud->prefix,\n +-\t\t\t\t\t\t\t get_super_prefix());\n ++\tchar *displaypath = get_submodule_displaypath(ce->name, ud->prefix);\n + \tstruct strbuf sb = STRBUF_INIT;\n \tint needs_cloning = 0;\n \tint need_free_url = 0;\n - \n --\tdisplaypath = do_get_submodule_displaypath(ce->name,\n --\t\t\t\t\t\t suc->update_data->prefix,\n --\t\t\t\t\t\t get_super_prefix());\n -+\tdisplaypath =\n -+\t\tget_submodule_displaypath(ce->name, suc->update_data->prefix);\n - \n - \tif (ce_stage(ce)) {\n - \t\tstrbuf_addf(out, _(\"Skipping unmerged submodule %s\"), displaypath);\n @@ builtin/submodule--helper.c: static int update_submodule(struct update_data *update_data)\n {\n \tensure_core_worktree(update_data->sm_path);\nq\n"},{"id":"458297","messageId":"kl6ltu81sokr.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"58074","inReplyTo":"220701.865ykhdboc.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 00/18] submodule: remove \"--recursive-prefix\"","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-06-30T23:45:24Z","receivedAt":"2022-06-30T23:45:32Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> On Thu, Jun 30 2022, Glen Choo via GitGitGadget wrote:\n>\n>> = Series history\n>>\n>> Changes in v2:\n>>\n>> * Rebase onto ab/submodule-cleanup (previously master)\n>> * Cherry pick https://github.com/avar/git/commit/f445c57490d into 4/6\n>> * Style fixes in .c and tests\n>\n> Thanks for the update, Looks like something happened with GGG to send\n> the base series + yours, I confirmed that 1-13/18 are the same as my\n> series in gitster/ab/submodule-cleanup.\n\nYes, oops. I resent this as v3 (sans your base), but you can ignore\nthat :P\n\n> I played around with this a bit, and pushed some amends to\n> https://github.com/avar/git/tree/avar/pr-git-1282/chooglen/submodule/remove-recursive-prefix-v2;\n> which you should take more as the results of poking around, I think this\n> is fine as-is if Junio would like to pick it up, sans the possible bug\n> mentioned below.\n>\n> A range-diff of the changes I made follows below, changes:\n>\n> * Split up the \"add tests\" to a new commit :) You mentioned\n> \"test_expect_failure\", I just assumed we'd use test_expect_succes,\n> and then the 2nd commit has this test change:\n> \t\n> \tdiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\n> \tindex 9a076e025f3..6cc07460dd2 100755\n> \t--- a/t/t7406-submodule-update.sh\n> \t+++ b/t/t7406-submodule-update.sh\n> \t@@ -1163 +1163 @@ test_expect_success 'submodule update should skip unmerged submodules' '\n> \t- Skipping unmerged submodule middle//bottom\n> \t+ Skipping unmerged submodule middle/bottom\n> \t@@ -1176 +1176 @@ test_expect_success 'submodule update --recursive skip submodules with strategy=\n> \t- Skipping submodule '\\''../middle/'\\''\n> \t+ Skipping submodule '\\''middle/bottom'\\''\n>\n> Which I think makes it much easier to explain the change itself, the\n> first commit basically just says \"tests for existing behavior, see\n> the next commit for details\".\n\nAh, I see what you meant now. Makes sense.\n\n> * Used test_cmp instead of \"grep -F\", makes for easier to grok output :)\n>\n> * I had to squint a bit at the change from strvec_pushl() to\n> strvec_pushf().\n>\n> I.e. we're changing both that we're passing \"--foo bar\"\n> v.s. \"--foo=bar\" *and* \"bar\" v.s. \"bar/\", but as the commit message\n> notes it's just the latter that matters.\n>\n> Just a nit, but I think it's a bit easier to reason about just\n> changing the one thing we need to change there, although that means\n> giving the \"if\" braces. This also leaves the \"--super-prefix\" code\n> consistent with all the other places where we\n> \"strvec.*--super-prefix\" at the end.\n\nBoth sound good.\n\n> * We can just declare some of the variables and initialize them at the\n> beginning, but couldn't when they were a strbuf.\n\nI think this is just \"char *displaypath\" in 2/7 aka d1a47d302e\n(submodule--helper update: use display path helper, 2022-06-28)? That\nchange makes sense.\n\n> * Your 18/18 has some odd code formatting of \"var =\\n\\t\\tvalue(...\"\n> when we usually do \"var = value(...\\n...\".\n\nThanks for catching that. I could have sworn that came from \"make\nstyle\".\n\n> * You appear to have (and I haven't \"fixed\" it here) in 13/18 mentioned\n> \"While we're fixing the display names, also fix inconsistent quoting\n> of the submodule name\", but as the test_cmp shows we appear not to\n> have done that part at all? Is this referring to a forgotten change\n> where we should be saying (note the added '-quotes):\n>\n> Skipping unmerged submodule 'middle/bottom'\n>\n> Either that's what you mean & you forgot, or we're missing a test, or\n> I'm misunderstanding it...\n\nYes, this is a genuine error. In v2, I dropped this change (as you\nsuggested) and I neglected to amend the comit message..\n\n> I hope this helps, at least some of it...\n\nThanks! Yes it helps a lot. If you don't mind, I'll roll your commits +\nthe commit message fix as v4.\n\n> 1: 64c138df196 ! 1: 3d9977006d3 submodule--helper update: use display path helper\n> @@ Metadata\n> Author: Glen Choo \n> \n> ## Commit message ##\n> - submodule--helper update: use display path helper\n> + submodule--helper tests: add missing \"display path\" coverage\n> \n> There are two locations in prepare_to_clone_next_submodule() that\n> - manually calculate the submodule display path, but should just use\n> - do_get_submodule_displaypath() for consistency.\n> -\n> - Do this replacement and reorder the code slightly to avoid computing\n> - the display path twice.\n> -\n> - This code was never tested, and adding tests shows that both these sites\n> - have been computing the display path incorrectly ever since they were\n> - introduced in 48308681b0 (git submodule update: have a dedicated helper\n> - for cloning, 2016-02-29) [1]:\n> -\n> - - The first hunk puts a \"/\" between recursive_prefix and ce->name, but\n> - recursive_prefix already ends with \"/\".\n> - - The second hunk calls relative_path() on recursive_prefix and\n> - ce->name, but relative_path() only makes sense when both paths share\n> - the same base directory. This is never the case here:\n> - - recursive_prefix is the path from the topmost superproject to the\n> - current submodule\n> - - ce->name is the path from the root of the current submodule to its\n> - submodule.\n> - so, e.g. recursive_prefix=\"super\" and ce->name=\"submodule\" produces\n> - displayname=\"../super\" instead of \"super/submodule\".\n> -\n> - While we're fixing the display names, also fix inconsistent quoting of\n> - the submodule name.\n> -\n> - [1] I verified this by applying the tests to 48308681b0.\n> + manually calculate the submodule display path, as discussed in the\n> + next commit the \"Skipping\" output isn't exactly what we want, but\n> + let's test how we behave now, before changing the existing behavior.\n> \n> Signed-off-by: Glen Choo \n> -\n> - ## builtin/submodule--helper.c ##\n> -@@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n> - \tconst char *update_string;\n> - \tenum submodule_update_type update_type;\n> - \tchar *key;\n> --\tstruct strbuf displaypath_sb = STRBUF_INIT;\n> - \tstruct strbuf sb = STRBUF_INIT;\n> --\tconst char *displaypath = NULL;\n> -+\tchar *displaypath;\n> - \tint needs_cloning = 0;\n> - \tint need_free_url = 0;\n> - \n> -+\tdisplaypath = do_get_submodule_displaypath(ce->name,\n> -+\t\t\t\t\t\t suc->update_data->prefix,\n> -+\t\t\t\t\t\t suc->update_data->recursive_prefix);\n> -+\n> - \tif (ce_stage(ce)) {\n> --\t\tif (suc->update_data->recursive_prefix)\n> --\t\t\tstrbuf_addf(&sb, \"%s/%s\", suc->update_data->recursive_prefix, ce->name);\n> --\t\telse\n> --\t\t\tstrbuf_addstr(&sb, ce->name);\n> --\t\tstrbuf_addf(out, _(\"Skipping unmerged submodule %s\"), sb.buf);\n> -+\t\tstrbuf_addf(out, _(\"Skipping unmerged submodule %s\"), displaypath);\n> - \t\tstrbuf_addch(out, '\\n');\n> - \t\tgoto cleanup;\n> - \t}\n> - \n> - \tsub = submodule_from_path(the_repository, null_oid(), ce->name);\n> - \n> --\tif (suc->update_data->recursive_prefix)\n> --\t\tdisplaypath = relative_path(suc->update_data->recursive_prefix,\n> --\t\t\t\t\t ce->name, &displaypath_sb);\n> --\telse\n> --\t\tdisplaypath = ce->name;\n> --\n> - \tif (!sub) {\n> - \t\tnext_submodule_warn_missing(suc, out, displaypath);\n> - \t\tgoto cleanup;\n> -@@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n> - \t\t\t\t\t \"--no-single-branch\");\n> - \n> - cleanup:\n> --\tstrbuf_release(&displaypath_sb);\n> -+\tfree(displaypath);\n> - \tstrbuf_release(&sb);\n> - \tif (need_free_url)\n> - \t\tfree((void*)url);\n> + Signed-off-by: Ævar Arnfjörð Bjarmason \n> \n> ## t/t7406-submodule-update.sh ##\n> @@ t/t7406-submodule-update.sh: test_expect_success 'submodule update --filter sets partial clone settings' '\n> @@ t/t7406-submodule-update.sh: test_expect_success 'submodule update --filter sets\n> +\t# because of its unmerged state\n> +\ttest_config -C top-cloned submodule.middle.update !true &&\n> +\tgit -C top-cloned submodule update --recursive 2>actual.err &&\n> -+\tgrep -F \"Skipping unmerged submodule middle/bottom\" actual.err\n> ++\tcat >expect.err <<-\\EOF &&\n> ++\tSkipping unmerged submodule middle//bottom\n> ++\tEOF\n> ++\ttest_cmp expect.err actual.err\n> +'\n> +\n> +test_expect_success 'submodule update --recursive skip submodules with strategy=none' '\n> @@ t/t7406-submodule-update.sh: test_expect_success 'submodule update --filter sets\n> +\ttest_commit -C top-cloned/middle/bottom downstream_commit &&\n> +\tgit -C top-cloned/middle config submodule.bottom.update none &&\n> +\tgit -C top-cloned submodule update --recursive 2>actual.err &&\n> -+\tgrep -F \"Skipping submodule ${SQ}middle/bottom${SQ}\" actual.err\n> ++\tcat >expect.err <<-\\EOF &&\n> ++\tSkipping submodule '\\''../middle/'\\''\n> ++\tEOF\n> ++\ttest_cmp expect.err actual.err\n> +'\n> +\n> test_done\n> -: ----------- > 2: d1a47d302ee submodule--helper update: use display path helper\n> 2: d3e803a4630 ! 3: a5b30ac94e6 submodule--helper: don't recreate recursive prefix\n> @@ builtin/submodule--helper.c: static void update_data_to_args(struct update_data\n> -\tif (update_data->recursive_prefix)\n> -\t\tstrvec_pushl(args, \"--recursive-prefix\",\n> -\t\t\t update_data->recursive_prefix, NULL);\n> -+\tif (update_data->displaypath)\n> -+\t\tstrvec_pushf(args, \"--recursive-prefix=%s/\",\n> -+\t\t\t update_data->displaypath);\n> ++\tif (update_data->displaypath) {\n> ++\t\tstrvec_push(args, \"--recursive-prefix\");\n> ++\t\tstrvec_pushf(args, \"%s/\", update_data->displaypath);\n> ++\t}\n> \tif (update_data->quiet)\n> \t\tstrvec_push(args, \"--quiet\");\n> \tif (update_data->force)\n> 3: 1f7cf6ffaf1 = 4: 0ea102882a7 submodule--helper: use correct display path helper\n> 4: 85e65f143b6 = 5: 8085bc83a0c submodule--helper: remove unused SUPPORT_SUPER_PREFIX flags\n> 5: 1d8b6b244dc ! 6: 4c00a1b496a submodule--helper update: use --super-prefix\n> @@ builtin/submodule--helper.c: struct submodule_update_clone {\n> \tenum submodule_update_type update_default;\n> \tstruct object_id suboid;\n> @@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n> - \n> - \tdisplaypath = do_get_submodule_displaypath(ce->name,\n> - \t\t\t\t\t\t suc->update_data->prefix,\n> --\t\t\t\t\t\t suc->update_data->recursive_prefix);\n> -+\t\t\t\t\t\t get_super_prefix());\n> - \n> - \tif (ce_stage(ce)) {\n> - \t\tstrbuf_addf(out, _(\"Skipping unmerged submodule %s\"), displaypath);\n> + \tchar *key;\n> + \tstruct update_data *ud = suc->update_data;\n> + \tchar *displaypath = do_get_submodule_displaypath(ce->name, ud->prefix,\n> +-\t\t\t\t\t\t\t ud->recursive_prefix);\n> ++\t\t\t\t\t\t\t get_super_prefix());\n> + \tstruct strbuf sb = STRBUF_INIT;\n> + \tint needs_cloning = 0;\n> + \tint need_free_url = 0;\n> @@ builtin/submodule--helper.c: static void update_data_to_args(struct update_data *update_data, struct strvec *\n> {\n> \tenum submodule_update_type update_type = update_data->update_default;\n> \n> -\tstrvec_pushl(args, \"submodule--helper\", \"update\", \"--recursive\", NULL);\n> -\tstrvec_pushf(args, \"--jobs=%d\", update_data->max_jobs);\n> - \tif (update_data->displaypath)\n> --\t\tstrvec_pushf(args, \"--recursive-prefix=%s/\",\n> -+\t\tstrvec_pushf(args, \"--super-prefix=%s/\",\n> - \t\t\t update_data->displaypath);\n> + \tif (update_data->displaypath) {\n> +-\t\tstrvec_push(args, \"--recursive-prefix\");\n> ++\t\tstrvec_push(args, \"--super-prefix\");\n> + \t\tstrvec_pushf(args, \"%s/\", update_data->displaypath);\n> + \t}\n> +\tstrvec_pushl(args, \"submodule--helper\", \"update\", \"--recursive\", NULL);\n> +\tstrvec_pushf(args, \"--jobs=%d\", update_data->max_jobs);\n> \tif (update_data->quiet)\n> 6: a21e8cd174d ! 7: 639f07e4b84 submodule--helper: remove display path helper\n> @@ builtin/submodule--helper.c: static void init_submodule(const char *path, const\n> \tsub = submodule_from_path(the_repository, null_oid(), path);\n> \n> @@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n> + \tenum submodule_update_type update_type;\n> + \tchar *key;\n> + \tstruct update_data *ud = suc->update_data;\n> +-\tchar *displaypath = do_get_submodule_displaypath(ce->name, ud->prefix,\n> +-\t\t\t\t\t\t\t get_super_prefix());\n> ++\tchar *displaypath = get_submodule_displaypath(ce->name, ud->prefix);\n> + \tstruct strbuf sb = STRBUF_INIT;\n> \tint needs_cloning = 0;\n> \tint need_free_url = 0;\n> - \n> --\tdisplaypath = do_get_submodule_displaypath(ce->name,\n> --\t\t\t\t\t\t suc->update_data->prefix,\n> --\t\t\t\t\t\t get_super_prefix());\n> -+\tdisplaypath =\n> -+\t\tget_submodule_displaypath(ce->name, suc->update_data->prefix);\n> - \n> - \tif (ce_stage(ce)) {\n> - \t\tstrbuf_addf(out, _(\"Skipping unmerged submodule %s\"), displaypath);\n> @@ builtin/submodule--helper.c: static int update_submodule(struct update_data *update_data)\n> {\n> \tensure_core_worktree(update_data->sm_path);\n"},{"id":"458317","messageId":"20220701021157.88858-1-chooglen@google.com","threadId":"58074","inReplyTo":"20220630221147.45689-1-chooglen@google.com","subject":"[PATCH v4 0/7] submodule: remove \"--recursive-prefix\"","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-07-01T02:11:50Z","receivedAt":"2022-07-01T02:12:12Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Thanks again! As mentioned, I've taken the amendments Ævar posted in\n[1] and fixed the bug mentioned there. The only things I've changed\nwere some punctuation and changing the base commit to upstream's\nab/submodule-cleanup (which is based off a slightly older 'master').\n\nNote for Junio: this version is based on ab/submodule-cleanup.\n\n= Description\n\nThis series is a refactor of \"git submodule--helper update\" that replaces\n\"--recursive-prefix\" with \"--super-prefix\" (see Background). This was\ninitially motivated by:\n\n * Junio's suggestion to simplify the code [2] (in response to a memory leak\n found by Johannes Schindelin [3]).\n * A desire to use the module_list API + get_submodule_displaypath() outside\n of builtin/submodule--helper.c (I expect to use this to check out\n branches in each submodule).\n\nBut it also happens to remove some overly complicated/duplicated code that\nwas literally converted from shell :)\n\n= Background\n\nWhen recursing into nested submodules, Git commands keep track of the path\nfrom the superproject to the submodule in order to print a \"display path\" to\nthe user, e.g.\n\n Submodule path '../super/sub/nested-sub': checked out 'abcdef'\n\nFor historical reasons, \"git submodule--helper update\" uses\n\"--recursive-prefix\" for this purpose, but it should use \"--super-prefix\"\ninstead because:\n\n * That's what every other command uses (not just submodule--helper\n subcommands).\n * Using the \"--super-prefix\" helper functions makes the \"git\n submodule--helper update\" code simpler\n\n= Patch organization\n\n1/7 and 5/7 were contributed by Ævar (thanks!)\n\n* Patches 1-2/7 makes sure that display paths are only computed using\n display path helper functions ([do_]get_submodule_displaypath()) and\n fixes some display paths that we never realized were broken.\n* Patches 3-4/7 simplify and deduplicate some display path computations.\n* Patch 5/7 removes SUPPORT_SUPER_PREFIX where it's not needed.\n * This doesn't affect correctness, but we want to do this eventually, so\n do it now to make 6/7 a bit cleaner.\n* Patch 6/7 replaces \"--recursive-prefix\" with \"--super-prefix\", making\n do_get_submodule_displaypath() obsolete.\n* Patch 7/7 removes do_get_submodule_displaypath().\n\n= Series history\n\nChanges in v4:\n* Split patch 1 so that the missing test coverage is introduced in its\n own patch\n* Fix a stale commit message in 1/6 (now patch 2/7)\n* Use test_cmp instead of \"grep -F\"\n* Style fixes and improvements\n\nChanges in v3:\n* None (resend of v2 because v2 accidentally included\n ab/submodule-cleanup)\n\nChanges in v2:\n * Rebase onto ab/submodule-cleanup (previously master)\n * Cherry pick https://github.com/avar/git/commit/f445c57490d into 4/6\n * Style fixes in .c and tests\n\n= Future work\n\nAt the end of this series, get_submodule_displaypath() can be moved to\nsubmodule.h, which would make submodule.c:get_super_prefix_or_empty()\nobsolete. I have a patch that demonstrates this (CI run: [4]), but I decided\nto keep this series more focused on \"git submodule--helper update\" so that\nit's easier to review.\n\n[1] https://lore.kernel.org/git/220701.865ykhdboc.gmgdl@evledraar.gmail.com\n[2] https://lore.kernel.org/git/xmqq35g5xmmv.fsf@gitster.g\n[3] https://lore.kernel.org/git/877a45867ae368bf9e053caedcb6cf421e02344d.1655336146.git.gitgitgadget@gmail.com\n[4] https://github.com/chooglen/git/actions/runs/2572557584\n\nGlen Choo (6):\n submodule--helper tests: add missing \"display path\" coverage\n submodule--helper update: use display path helper\n submodule--helper: don't recreate recursive prefix\n submodule--helper: use correct display path helper\n submodule--helper update: use --super-prefix\n submodule--helper: remove display path helper\n\nÆvar Arnfjörð Bjarmason (1):\n submodule--helper: remove unused SUPPORT_SUPER_PREFIX flags\n\n builtin/submodule--helper.c | 86 ++++++++++---------------------------\n t/t7406-submodule-update.sh | 62 ++++++++++++++++++++++++++\n 2 files changed, 84 insertions(+), 64 deletions(-)\n\nRange-diff against v3:\n1: 8c82518d33 ! 1: 1f2ef5f6a2 submodule--helper update: use display path helper\n @@ Metadata\n Author: Glen Choo \n \n ## Commit message ##\n - submodule--helper update: use display path helper\n + submodule--helper tests: add missing \"display path\" coverage\n \n There are two locations in prepare_to_clone_next_submodule() that\n - manually calculate the submodule display path, but should just use\n - do_get_submodule_displaypath() for consistency.\n -\n - Do this replacement and reorder the code slightly to avoid computing\n - the display path twice.\n -\n - This code was never tested, and adding tests shows that both these sites\n - have been computing the display path incorrectly ever since they were\n - introduced in 48308681b0 (git submodule update: have a dedicated helper\n - for cloning, 2016-02-29) [1]:\n -\n - - The first hunk puts a \"/\" between recursive_prefix and ce->name, but\n - recursive_prefix already ends with \"/\".\n - - The second hunk calls relative_path() on recursive_prefix and\n - ce->name, but relative_path() only makes sense when both paths share\n - the same base directory. This is never the case here:\n - - recursive_prefix is the path from the topmost superproject to the\n - current submodule\n - - ce->name is the path from the root of the current submodule to its\n - submodule.\n - so, e.g. recursive_prefix=\"super\" and ce->name=\"submodule\" produces\n - displayname=\"../super\" instead of \"super/submodule\".\n -\n - [1] I verified this by applying the tests to 48308681b0.\n + manually calculate the submodule display path. As discussed in the\n + next commit the \"Skipping\" output isn't exactly what we want, but\n + let's test how we behave now, before changing the existing behavior.\n \n Signed-off-by: Glen Choo \n -\n - ## builtin/submodule--helper.c ##\n -@@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n - \tconst char *update_string;\n - \tenum submodule_update_type update_type;\n - \tchar *key;\n --\tstruct strbuf displaypath_sb = STRBUF_INIT;\n - \tstruct strbuf sb = STRBUF_INIT;\n --\tconst char *displaypath = NULL;\n -+\tchar *displaypath;\n - \tint needs_cloning = 0;\n - \tint need_free_url = 0;\n - \n -+\tdisplaypath = do_get_submodule_displaypath(ce->name,\n -+\t\t\t\t\t\t suc->update_data->prefix,\n -+\t\t\t\t\t\t suc->update_data->recursive_prefix);\n -+\n - \tif (ce_stage(ce)) {\n --\t\tif (suc->update_data->recursive_prefix)\n --\t\t\tstrbuf_addf(&sb, \"%s/%s\", suc->update_data->recursive_prefix, ce->name);\n --\t\telse\n --\t\t\tstrbuf_addstr(&sb, ce->name);\n --\t\tstrbuf_addf(out, _(\"Skipping unmerged submodule %s\"), sb.buf);\n -+\t\tstrbuf_addf(out, _(\"Skipping unmerged submodule %s\"), displaypath);\n - \t\tstrbuf_addch(out, '\\n');\n - \t\tgoto cleanup;\n - \t}\n - \n - \tsub = submodule_from_path(the_repository, null_oid(), ce->name);\n - \n --\tif (suc->update_data->recursive_prefix)\n --\t\tdisplaypath = relative_path(suc->update_data->recursive_prefix,\n --\t\t\t\t\t ce->name, &displaypath_sb);\n --\telse\n --\t\tdisplaypath = ce->name;\n --\n - \tif (!sub) {\n - \t\tnext_submodule_warn_missing(suc, out, displaypath);\n - \t\tgoto cleanup;\n -@@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n - \t\t\t\t\t \"--no-single-branch\");\n - \n - cleanup:\n --\tstrbuf_release(&displaypath_sb);\n -+\tfree(displaypath);\n - \tstrbuf_release(&sb);\n - \tif (need_free_url)\n - \t\tfree((void*)url);\n + Signed-off-by: Ævar Arnfjörð Bjarmason \n \n ## t/t7406-submodule-update.sh ##\n @@ t/t7406-submodule-update.sh: test_expect_success 'submodule update --filter sets partial clone settings' '\n @@ t/t7406-submodule-update.sh: test_expect_success 'submodule update --filter sets\n +\t# because of its unmerged state\n +\ttest_config -C top-cloned submodule.middle.update !true &&\n +\tgit -C top-cloned submodule update --recursive 2>actual.err &&\n -+\tgrep -F \"Skipping unmerged submodule middle/bottom\" actual.err\n ++\tcat >expect.err <<-\\EOF &&\n ++\tSkipping unmerged submodule middle//bottom\n ++\tEOF\n ++\ttest_cmp expect.err actual.err\n +'\n +\n +test_expect_success 'submodule update --recursive skip submodules with strategy=none' '\n @@ t/t7406-submodule-update.sh: test_expect_success 'submodule update --filter sets\n +\ttest_commit -C top-cloned/middle/bottom downstream_commit &&\n +\tgit -C top-cloned/middle config submodule.bottom.update none &&\n +\tgit -C top-cloned submodule update --recursive 2>actual.err &&\n -+\tgrep -F \"Skipping submodule ${SQ}middle/bottom${SQ}\" actual.err\n ++\tcat >expect.err <<-\\EOF &&\n ++\tSkipping submodule '\\''../middle/'\\''\n ++\tEOF\n ++\ttest_cmp expect.err actual.err\n +'\n +\n test_done\n-: ---------- > 2: 146b88eaa3 submodule--helper update: use display path helper\n2: 102ada974d ! 3: a744640cd3 submodule--helper: don't recreate recursive prefix\n @@ builtin/submodule--helper.c: static void update_data_to_args(struct update_data\n -\tif (update_data->recursive_prefix)\n -\t\tstrvec_pushl(args, \"--recursive-prefix\",\n -\t\t\t update_data->recursive_prefix, NULL);\n -+\tif (update_data->displaypath)\n -+\t\tstrvec_pushf(args, \"--recursive-prefix=%s/\",\n -+\t\t\t update_data->displaypath);\n ++\tif (update_data->displaypath) {\n ++\t\tstrvec_push(args, \"--recursive-prefix\");\n ++\t\tstrvec_pushf(args, \"%s/\", update_data->displaypath);\n ++\t}\n \tif (update_data->quiet)\n \t\tstrvec_push(args, \"--quiet\");\n \tif (update_data->force)\n3: b6dda65084 = 4: 0b45f2ff2b submodule--helper: use correct display path helper\n4: aa5d389bb8 = 5: 963c8ba07b submodule--helper: remove unused SUPPORT_SUPER_PREFIX flags\n5: 4dcfb9889f ! 6: a777fcf905 submodule--helper update: use --super-prefix\n @@ builtin/submodule--helper.c: struct submodule_update_clone {\n \tenum submodule_update_type update_default;\n \tstruct object_id suboid;\n @@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n - \n - \tdisplaypath = do_get_submodule_displaypath(ce->name,\n - \t\t\t\t\t\t suc->update_data->prefix,\n --\t\t\t\t\t\t suc->update_data->recursive_prefix);\n -+\t\t\t\t\t\t get_super_prefix());\n - \n - \tif (ce_stage(ce)) {\n - \t\tstrbuf_addf(out, _(\"Skipping unmerged submodule %s\"), displaypath);\n + \tchar *key;\n + \tstruct update_data *ud = suc->update_data;\n + \tchar *displaypath = do_get_submodule_displaypath(ce->name, ud->prefix,\n +-\t\t\t\t\t\t\t ud->recursive_prefix);\n ++\t\t\t\t\t\t\t get_super_prefix());\n + \tstruct strbuf sb = STRBUF_INIT;\n + \tint needs_cloning = 0;\n + \tint need_free_url = 0;\n @@ builtin/submodule--helper.c: static void update_data_to_args(struct update_data *update_data, struct strvec *\n {\n \tenum submodule_update_type update_type = update_data->update_default;\n \n -\tstrvec_pushl(args, \"submodule--helper\", \"update\", \"--recursive\", NULL);\n -\tstrvec_pushf(args, \"--jobs=%d\", update_data->max_jobs);\n - \tif (update_data->displaypath)\n --\t\tstrvec_pushf(args, \"--recursive-prefix=%s/\",\n -+\t\tstrvec_pushf(args, \"--super-prefix=%s/\",\n - \t\t\t update_data->displaypath);\n + \tif (update_data->displaypath) {\n +-\t\tstrvec_push(args, \"--recursive-prefix\");\n ++\t\tstrvec_push(args, \"--super-prefix\");\n + \t\tstrvec_pushf(args, \"%s/\", update_data->displaypath);\n + \t}\n +\tstrvec_pushl(args, \"submodule--helper\", \"update\", \"--recursive\", NULL);\n +\tstrvec_pushf(args, \"--jobs=%d\", update_data->max_jobs);\n \tif (update_data->quiet)\n6: 109c55236d ! 7: d47ea17cc2 submodule--helper: remove display path helper\n @@ builtin/submodule--helper.c: static void init_submodule(const char *path, const\n \tsub = submodule_from_path(the_repository, null_oid(), path);\n \n @@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n + \tenum submodule_update_type update_type;\n + \tchar *key;\n + \tstruct update_data *ud = suc->update_data;\n +-\tchar *displaypath = do_get_submodule_displaypath(ce->name, ud->prefix,\n +-\t\t\t\t\t\t\t get_super_prefix());\n ++\tchar *displaypath = get_submodule_displaypath(ce->name, ud->prefix);\n + \tstruct strbuf sb = STRBUF_INIT;\n \tint needs_cloning = 0;\n \tint need_free_url = 0;\n - \n --\tdisplaypath = do_get_submodule_displaypath(ce->name,\n --\t\t\t\t\t\t suc->update_data->prefix,\n --\t\t\t\t\t\t get_super_prefix());\n -+\tdisplaypath =\n -+\t\tget_submodule_displaypath(ce->name, suc->update_data->prefix);\n - \n - \tif (ce_stage(ce)) {\n - \t\tstrbuf_addf(out, _(\"Skipping unmerged submodule %s\"), displaypath);\n @@ builtin/submodule--helper.c: static int update_submodule(struct update_data *update_data)\n {\n \tensure_core_worktree(update_data->sm_path);\n\nbase-commit: 5b893f7d81eb7feb43662ed8663e2af76a76b4c8\n-- \n2.37.0.rc0.161.g10f37bed90-goog\n\n"},{"id":"458318","messageId":"20220701021157.88858-2-chooglen@google.com","threadId":"58074","inReplyTo":"20220701021157.88858-1-chooglen@google.com","subject":"[PATCH v4 1/7] submodule--helper tests: add missing \"display path\" coverage","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-07-01T02:11:51Z","receivedAt":"2022-07-01T02:12:14Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"There are two locations in prepare_to_clone_next_submodule() that\nmanually calculate the submodule display path. As discussed in the\nnext commit the \"Skipping\" output isn't exactly what we want, but\nlet's test how we behave now, before changing the existing behavior.\n\nSigned-off-by: Glen Choo \nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t7406-submodule-update.sh | 62 +++++++++++++++++++++++++++++++++++++\n 1 file changed, 62 insertions(+)\n\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex 06d804e213..9a076e025f 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -1116,4 +1116,66 @@ test_expect_success 'submodule update --filter sets partial clone settings' '\n \ttest_cmp_config -C super-filter/submodule blob:none remote.origin.partialclonefilter\n '\n \n+# NEEDSWORK: Clean up the tests so that we can reuse the test setup.\n+# Don't reuse the existing repos because the earlier tests have\n+# intentionally disruptive configurations.\n+test_expect_success 'setup clean recursive superproject' '\n+\tgit init bottom &&\n+\ttest_commit -C bottom \"bottom\" &&\n+\tgit init middle &&\n+\tgit -C middle submodule add ../bottom bottom &&\n+\tgit -C middle commit -m \"middle\" &&\n+\tgit init top &&\n+\tgit -C top submodule add ../middle middle &&\n+\tgit -C top commit -m \"top\" &&\n+\tgit clone --recurse-submodules top top-clean\n+'\n+\n+test_expect_success 'submodule update should skip unmerged submodules' '\n+\ttest_when_finished \"rm -fr top-cloned\" &&\n+\tcp -r top-clean top-cloned &&\n+\n+\t# Create an upstream commit in each repo, starting with bottom\n+\ttest_commit -C bottom upstream_commit &&\n+\t# Create middle commit\n+\tgit -C middle/bottom fetch &&\n+\tgit -C middle/bottom checkout -f FETCH_HEAD &&\n+\tgit -C middle add bottom &&\n+\tgit -C middle commit -m \"upstream_commit\" &&\n+\t# Create top commit\n+\tgit -C top/middle fetch &&\n+\tgit -C top/middle checkout -f FETCH_HEAD &&\n+\tgit -C top add middle &&\n+\tgit -C top commit -m \"upstream_commit\" &&\n+\n+\t# Create a downstream conflict\n+\ttest_commit -C top-cloned/middle/bottom downstream_commit &&\n+\tgit -C top-cloned/middle add bottom &&\n+\tgit -C top-cloned/middle commit -m \"downstream_commit\" &&\n+\tgit -C top-cloned/middle fetch --recurse-submodules origin &&\n+\ttest_must_fail git -C top-cloned/middle merge origin/main &&\n+\n+\t# Make the update of \"middle\" a no-op, otherwise we error out\n+\t# because of its unmerged state\n+\ttest_config -C top-cloned submodule.middle.update !true &&\n+\tgit -C top-cloned submodule update --recursive 2>actual.err &&\n+\tcat >expect.err <<-\\EOF &&\n+\tSkipping unmerged submodule middle//bottom\n+\tEOF\n+\ttest_cmp expect.err actual.err\n+'\n+\n+test_expect_success 'submodule update --recursive skip submodules with strategy=none' '\n+\ttest_when_finished \"rm -fr top-cloned\" &&\n+\tcp -r top-clean top-cloned &&\n+\n+\ttest_commit -C top-cloned/middle/bottom downstream_commit &&\n+\tgit -C top-cloned/middle config submodule.bottom.update none &&\n+\tgit -C top-cloned submodule update --recursive 2>actual.err &&\n+\tcat >expect.err <<-\\EOF &&\n+\tSkipping submodule '\\''../middle/'\\''\n+\tEOF\n+\ttest_cmp expect.err actual.err\n+'\n+\n test_done\n-- \n2.37.0.rc0.161.g10f37bed90-goog\n\n"},{"id":"458319","messageId":"20220701021157.88858-3-chooglen@google.com","threadId":"58074","inReplyTo":"20220701021157.88858-1-chooglen@google.com","subject":"[PATCH v4 2/7] submodule--helper update: use display path helper","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-07-01T02:11:52Z","receivedAt":"2022-07-01T02:12:16Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"There are two locations in prepare_to_clone_next_submodule() that\nmanually calculate the submodule display path, but should just use\ndo_get_submodule_displaypath() for consistency.\n\nDo this replacement and reorder the code slightly to avoid computing\nthe display path twice.\n\nUntil the preceding commit this code had never been tested, with our\nnewly added tests we can see that both these sites have been computing\nthe display path incorrectly ever since they were introduced in\n48308681b0 (git submodule update: have a dedicated helper for cloning,\n2016-02-29) [1]:\n\n- The first hunk puts a \"/\" between recursive_prefix and ce->name, but\n recursive_prefix already ends with \"/\".\n- The second hunk calls relative_path() on recursive_prefix and\n ce->name, but relative_path() only makes sense when both paths share\n the same base directory. This is never the case here:\n - recursive_prefix is the path from the topmost superproject to the\n current submodule\n - ce->name is the path from the root of the current submodule to its\n submodule.\n so, e.g. recursive_prefix=\"super\" and ce->name=\"submodule\" produces\n displayname=\"../super\" instead of \"super/submodule\".\n\n[1] I verified this by applying the tests to 48308681b0.\n\nSigned-off-by: Glen Choo \n---\n builtin/submodule--helper.c | 19 +++++--------------\n t/t7406-submodule-update.sh | 4 ++--\n 2 files changed, 7 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 389b900602..9381127d56 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1947,30 +1947,21 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n \tconst char *update_string;\n \tenum submodule_update_type update_type;\n \tchar *key;\n-\tstruct strbuf displaypath_sb = STRBUF_INIT;\n+\tstruct update_data *ud = suc->update_data;\n+\tchar *displaypath = do_get_submodule_displaypath(ce->name, ud->prefix,\n+\t\t\t\t\t\t\t ud->recursive_prefix);\n \tstruct strbuf sb = STRBUF_INIT;\n-\tconst char *displaypath = NULL;\n \tint needs_cloning = 0;\n \tint need_free_url = 0;\n \n \tif (ce_stage(ce)) {\n-\t\tif (suc->update_data->recursive_prefix)\n-\t\t\tstrbuf_addf(&sb, \"%s/%s\", suc->update_data->recursive_prefix, ce->name);\n-\t\telse\n-\t\t\tstrbuf_addstr(&sb, ce->name);\n-\t\tstrbuf_addf(out, _(\"Skipping unmerged submodule %s\"), sb.buf);\n+\t\tstrbuf_addf(out, _(\"Skipping unmerged submodule %s\"), displaypath);\n \t\tstrbuf_addch(out, '\\n');\n \t\tgoto cleanup;\n \t}\n \n \tsub = submodule_from_path(the_repository, null_oid(), ce->name);\n \n-\tif (suc->update_data->recursive_prefix)\n-\t\tdisplaypath = relative_path(suc->update_data->recursive_prefix,\n-\t\t\t\t\t ce->name, &displaypath_sb);\n-\telse\n-\t\tdisplaypath = ce->name;\n-\n \tif (!sub) {\n \t\tnext_submodule_warn_missing(suc, out, displaypath);\n \t\tgoto cleanup;\n@@ -2060,7 +2051,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n \t\t\t\t\t \"--no-single-branch\");\n \n cleanup:\n-\tstrbuf_release(&displaypath_sb);\n+\tfree(displaypath);\n \tstrbuf_release(&sb);\n \tif (need_free_url)\n \t\tfree((void*)url);\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex 9a076e025f..6cc07460dd 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -1160,7 +1160,7 @@ test_expect_success 'submodule update should skip unmerged submodules' '\n \ttest_config -C top-cloned submodule.middle.update !true &&\n \tgit -C top-cloned submodule update --recursive 2>actual.err &&\n \tcat >expect.err <<-\\EOF &&\n-\tSkipping unmerged submodule middle//bottom\n+\tSkipping unmerged submodule middle/bottom\n \tEOF\n \ttest_cmp expect.err actual.err\n '\n@@ -1173,7 +1173,7 @@ test_expect_success 'submodule update --recursive skip submodules with strategy=\n \tgit -C top-cloned/middle config submodule.bottom.update none &&\n \tgit -C top-cloned submodule update --recursive 2>actual.err &&\n \tcat >expect.err <<-\\EOF &&\n-\tSkipping submodule '\\''../middle/'\\''\n+\tSkipping submodule '\\''middle/bottom'\\''\n \tEOF\n \ttest_cmp expect.err actual.err\n '\n-- \n2.37.0.rc0.161.g10f37bed90-goog\n\n"},{"id":"458320","messageId":"20220701021157.88858-4-chooglen@google.com","threadId":"58074","inReplyTo":"20220701021157.88858-1-chooglen@google.com","subject":"[PATCH v4 3/7] submodule--helper: don't recreate recursive prefix","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-07-01T02:11:53Z","receivedAt":"2022-07-01T02:12:23Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"update_submodule() uses duplicated code to compute\nupdate_data->displaypath and next.recursive_prefix. The latter is just\nthe former with \"/\" appended to it, and since update_data->displaypath\nnot changed outside of this statement, we can just reuse the already\ncomputed result.\n\nWe can go one step further and remove the reference to\nnext.recursive_prefix altogether. Since it is only used in\nupdate_data_to_args() (to compute the \"--recursive-prefix\" flag for the\nrecursive update child process) we can just use the already computed\n.displaypath value of there.\n\nDelete the duplicated code, and remove the unnecessary reference to\nnext.recursive_prefix. As a bonus, this fixes a memory leak where\nprefixed_path was never freed (this leak was first reported in [1]).\n\n[1] https://lore.kernel.org/git/877a45867ae368bf9e053caedcb6cf421e02344d.1655336146.git.gitgitgadget@gmail.com\n\nSigned-off-by: Glen Choo \n---\n builtin/submodule--helper.c | 15 ++++-----------\n 1 file changed, 4 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 9381127d56..4a0eb05ba2 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2416,9 +2416,10 @@ static void update_data_to_args(struct update_data *update_data, struct strvec *\n \n \tstrvec_pushl(args, \"submodule--helper\", \"update\", \"--recursive\", NULL);\n \tstrvec_pushf(args, \"--jobs=%d\", update_data->max_jobs);\n-\tif (update_data->recursive_prefix)\n-\t\tstrvec_pushl(args, \"--recursive-prefix\",\n-\t\t\t update_data->recursive_prefix, NULL);\n+\tif (update_data->displaypath) {\n+\t\tstrvec_push(args, \"--recursive-prefix\");\n+\t\tstrvec_pushf(args, \"%s/\", update_data->displaypath);\n+\t}\n \tif (update_data->quiet)\n \t\tstrvec_push(args, \"--quiet\");\n \tif (update_data->force)\n@@ -2514,14 +2515,6 @@ static int update_submodule(struct update_data *update_data)\n \t\tstruct update_data next = *update_data;\n \t\tint res;\n \n-\t\tif (update_data->recursive_prefix)\n-\t\t\tprefixed_path = xstrfmt(\"%s%s/\", update_data->recursive_prefix,\n-\t\t\t\t\t\tupdate_data->sm_path);\n-\t\telse\n-\t\t\tprefixed_path = xstrfmt(\"%s/\", update_data->sm_path);\n-\n-\t\tnext.recursive_prefix = get_submodule_displaypath(prefixed_path,\n-\t\t\t\t\t\t\t\t update_data->prefix);\n \t\tnext.prefix = NULL;\n \t\toidcpy(&next.oid, null_oid());\n \t\toidcpy(&next.suboid, null_oid());\n-- \n2.37.0.rc0.161.g10f37bed90-goog\n\n"},{"id":"458321","messageId":"20220701021157.88858-5-chooglen@google.com","threadId":"58074","inReplyTo":"20220701021157.88858-1-chooglen@google.com","subject":"[PATCH v4 4/7] submodule--helper: use correct display path helper","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-07-01T02:11:54Z","receivedAt":"2022-07-01T02:12:36Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Replace a chunk of code in update_submodule() with an equivalent\ndo_get_submodule_displaypath() invocation. This is already tested by\nt/t7406-submodule-update.sh:'submodule update --init --recursive from\nsubdirectory', so no tests are added.\n\nThe two are equivalent because:\n\n- Exactly one of recursive_prefix|prefix is non-NULL at a time; prefix\n is set at the superproject level, and recursive_prefix is set when\n recursing into submodules. There is also a BUG() statement in\n get_submodule_displaypath() that asserts that both cannot be non-NULL.\n\n- In get_submodule_displaypath(), get_super_prefix() always returns NULL\n because \"--super-prefix\" is never passed. Thus calling it is\n equivalent to calling do_get_submodule_displaypath() with super_prefix\n = NULL.\n\nTherefore:\n\n- When recursive_prefix is non-NULL, prefix is NULL, and thus\n get_submodule_displaypath() just returns prefixed_path. This is\n identical to calling do_get_submodule_displaypath() with super_prefix\n = recursive_prefix because the return value is still the concatenation\n of recursive_prefix + update_data->sm_path.\n\n- When prefix is non-NULL, prefixed_path = update_data->sm_path. Thus\n calling get_submodule_displaypath() with prefixed_path is equivalent\n to calling do_get_submodule_displaypath() with update_data->sm_path\n\nSigned-off-by: Glen Choo \n---\n builtin/submodule--helper.c | 14 +++-----------\n 1 file changed, 3 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 4a0eb05ba2..65350bde4b 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2463,19 +2463,11 @@ static void update_data_to_args(struct update_data *update_data, struct strvec *\n \n static int update_submodule(struct update_data *update_data)\n {\n-\tchar *prefixed_path;\n-\n \tensure_core_worktree(update_data->sm_path);\n \n-\tif (update_data->recursive_prefix)\n-\t\tprefixed_path = xstrfmt(\"%s%s\", update_data->recursive_prefix,\n-\t\t\t\t\tupdate_data->sm_path);\n-\telse\n-\t\tprefixed_path = xstrdup(update_data->sm_path);\n-\n-\tupdate_data->displaypath = get_submodule_displaypath(prefixed_path,\n-\t\t\t\t\t\t\t update_data->prefix);\n-\tfree(prefixed_path);\n+\tupdate_data->displaypath = do_get_submodule_displaypath(update_data->sm_path,\n+\t\t\t\t\t\t\t\tupdate_data->prefix,\n+\t\t\t\t\t\t\t\tupdate_data->recursive_prefix);\n \n \tdetermine_submodule_update_strategy(the_repository, update_data->just_cloned,\n \t\t\t\t\t update_data->sm_path, update_data->update_default,\n-- \n2.37.0.rc0.161.g10f37bed90-goog\n\n"},{"id":"458322","messageId":"20220701021157.88858-7-chooglen@google.com","threadId":"58074","inReplyTo":"20220701021157.88858-1-chooglen@google.com","subject":"[PATCH v4 6/7] submodule--helper update: use --super-prefix","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-07-01T02:11:56Z","receivedAt":"2022-07-01T02:12:37Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Unlike the other subcommands, \"git submodule--helper update\" uses the\n\"--recursive-prefix\" flag instead of \"--super-prefix\". The two flags are\notherwise identical (they only serve to compute the 'display path' of a\nsubmodule), except that there is a dedicated helper function to get the\nvalue of \"--super-prefix\".\n\nThis inconsistency exists because \"git submodule update\" used to pass\n\"--recursive-prefix\" between shell and C (introduced in [1]) before\n\"--super-prefix\" was introduced (in [2]), and for simplicity, we kept\nthis name when \"git submodule--helper update\" was created.\n\nRemove \"--recursive-prefix\" and its associated code from \"git\nsubmodule--helper update\", replacing it with \"--super-prefix\".\n\nTo use \"--super-prefix\", module_update is marked with\nSUPPORT_SUPER_PREFIX. Note that module_clone must also be marked with\nSUPPORT_SUPER_PREFIX, otherwise the \"git submodule--helper clone\"\nsubprocess will fail check because \"--super-prefix\" is propagated via\nthe environment.\n\n[1] 48308681b0 (git submodule update: have a dedicated helper for\ncloning, 2016-02-29)\n[2] 74866d7579 (git: make super-prefix option, 2016-10-07)\n\nSigned-off-by: Glen Choo \n---\n builtin/submodule--helper.c | 30 ++++++++++--------------------\n 1 file changed, 10 insertions(+), 20 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex a7b40482b8..eb84e3a98b 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -477,22 +477,18 @@ static int starts_with_dot_dot_slash(const char *const path)\n \n struct init_cb {\n \tconst char *prefix;\n-\tconst char *superprefix;\n \tunsigned int flags;\n };\n #define INIT_CB_INIT { 0 }\n \n static void init_submodule(const char *path, const char *prefix,\n-\t\t\t const char *superprefix, unsigned int flags)\n+\t\t\t unsigned int flags)\n {\n \tconst struct submodule *sub;\n \tstruct strbuf sb = STRBUF_INIT;\n \tchar *upd = NULL, *url = NULL, *displaypath;\n \n-\t/* try superprefix from the environment, if it is not passed explicitly */\n-\tif (!superprefix)\n-\t\tsuperprefix = get_super_prefix();\n-\tdisplaypath = do_get_submodule_displaypath(path, prefix, superprefix);\n+\tdisplaypath = do_get_submodule_displaypath(path, prefix, get_super_prefix());\n \n \tsub = submodule_from_path(the_repository, null_oid(), path);\n \n@@ -566,7 +562,7 @@ static void init_submodule(const char *path, const char *prefix,\n static void init_submodule_cb(const struct cache_entry *list_item, void *cb_data)\n {\n \tstruct init_cb *info = cb_data;\n-\tinit_submodule(list_item->name, info->prefix, info->superprefix, info->flags);\n+\tinit_submodule(list_item->name, info->prefix, info->flags);\n }\n \n static int module_init(int argc, const char **argv, const char *prefix)\n@@ -1878,7 +1874,6 @@ struct submodule_update_clone {\n \n struct update_data {\n \tconst char *prefix;\n-\tconst char *recursive_prefix;\n \tconst char *displaypath;\n \tenum submodule_update_type update_default;\n \tstruct object_id suboid;\n@@ -1949,7 +1944,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n \tchar *key;\n \tstruct update_data *ud = suc->update_data;\n \tchar *displaypath = do_get_submodule_displaypath(ce->name, ud->prefix,\n-\t\t\t\t\t\t\t ud->recursive_prefix);\n+\t\t\t\t\t\t\t get_super_prefix());\n \tstruct strbuf sb = STRBUF_INIT;\n \tint needs_cloning = 0;\n \tint need_free_url = 0;\n@@ -2414,12 +2409,12 @@ static void update_data_to_args(struct update_data *update_data, struct strvec *\n {\n \tenum submodule_update_type update_type = update_data->update_default;\n \n-\tstrvec_pushl(args, \"submodule--helper\", \"update\", \"--recursive\", NULL);\n-\tstrvec_pushf(args, \"--jobs=%d\", update_data->max_jobs);\n \tif (update_data->displaypath) {\n-\t\tstrvec_push(args, \"--recursive-prefix\");\n+\t\tstrvec_push(args, \"--super-prefix\");\n \t\tstrvec_pushf(args, \"%s/\", update_data->displaypath);\n \t}\n+\tstrvec_pushl(args, \"submodule--helper\", \"update\", \"--recursive\", NULL);\n+\tstrvec_pushf(args, \"--jobs=%d\", update_data->max_jobs);\n \tif (update_data->quiet)\n \t\tstrvec_push(args, \"--quiet\");\n \tif (update_data->force)\n@@ -2467,7 +2462,7 @@ static int update_submodule(struct update_data *update_data)\n \n \tupdate_data->displaypath = do_get_submodule_displaypath(update_data->sm_path,\n \t\t\t\t\t\t\t\tupdate_data->prefix,\n-\t\t\t\t\t\t\t\tupdate_data->recursive_prefix);\n+\t\t\t\t\t\t\t\tget_super_prefix());\n \n \tdetermine_submodule_update_strategy(the_repository, update_data->just_cloned,\n \t\t\t\t\t update_data->sm_path, update_data->update_default,\n@@ -2591,10 +2586,6 @@ static int module_update(int argc, const char **argv, const char *prefix)\n \t\tOPT_STRING(0, \"prefix\", &opt.prefix,\n \t\t\t N_(\"path\"),\n \t\t\t N_(\"path into the working tree\")),\n-\t\tOPT_STRING(0, \"recursive-prefix\", &opt.recursive_prefix,\n-\t\t\t N_(\"path\"),\n-\t\t\t N_(\"path into the working tree, across nested \"\n-\t\t\t \"submodule boundaries\")),\n \t\tOPT_SET_INT(0, \"checkout\", &opt.update_default,\n \t\t\tN_(\"use the 'checkout' update strategy (default)\"),\n \t\t\tSM_UPDATE_CHECKOUT),\n@@ -2680,7 +2671,6 @@ static int module_update(int argc, const char **argv, const char *prefix)\n \t\t\tmodule_list_active(&list);\n \n \t\tinfo.prefix = opt.prefix;\n-\t\tinfo.superprefix = opt.recursive_prefix;\n \t\tif (opt.quiet)\n \t\t\tinfo.flags |= OPT_QUIET;\n \n@@ -3377,9 +3367,9 @@ struct cmd_struct {\n static struct cmd_struct commands[] = {\n \t{\"list\", module_list, 0},\n \t{\"name\", module_name, 0},\n-\t{\"clone\", module_clone, 0},\n+\t{\"clone\", module_clone, SUPPORT_SUPER_PREFIX},\n \t{\"add\", module_add, 0},\n-\t{\"update\", module_update, 0},\n+\t{\"update\", module_update, SUPPORT_SUPER_PREFIX},\n \t{\"resolve-relative-url-test\", resolve_relative_url_test, 0},\n \t{\"foreach\", module_foreach, SUPPORT_SUPER_PREFIX},\n \t{\"init\", module_init, 0},\n-- \n2.37.0.rc0.161.g10f37bed90-goog\n\n"},{"id":"458323","messageId":"20220701021157.88858-6-chooglen@google.com","threadId":"58074","inReplyTo":"20220701021157.88858-1-chooglen@google.com","subject":"[PATCH v4 5/7] submodule--helper: remove unused SUPPORT_SUPER_PREFIX flags","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-07-01T02:11:55Z","receivedAt":"2022-07-01T02:12:39Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"From: Ævar Arnfjörð Bjarmason \n\nRemove the SUPPORT_SUPER_PREFIX flag from \"add\", \"init\" and\n\"summary\". For the \"add\" command it hasn't been used since [1],\nlikewise for \"init\" and \"summary\" since [2] and [3], respectively.\n\nAs implemented in 74866d75793 (git: make super-prefix option,\n2016-10-07) the SUPPORT_SUPER_PREFIX flag in git.c applies for the\nentire command, but as implemented in 89c86265576 (submodule helper:\nsupport super prefix, 2016-12-08) we assert here in\ncmd_submodule__helper() that we're not getting the flag unexpectedly.\n\n1. 8c8195e9c3e (submodule--helper: introduce add-clone subcommand,\n 2021-07-10)\n2. 6e7c14e65c8 (submodule update --init: display correct path from\n submodule, 2017-01-06)\n3. 1cf823d8f00 (submodule: remove unnecessary `prefix` based option\n logic, 2021-06-22)\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \nSigned-off-by: Glen Choo \n---\n builtin/submodule--helper.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 65350bde4b..a7b40482b8 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -3378,15 +3378,15 @@ static struct cmd_struct commands[] = {\n \t{\"list\", module_list, 0},\n \t{\"name\", module_name, 0},\n \t{\"clone\", module_clone, 0},\n-\t{\"add\", module_add, SUPPORT_SUPER_PREFIX},\n+\t{\"add\", module_add, 0},\n \t{\"update\", module_update, 0},\n \t{\"resolve-relative-url-test\", resolve_relative_url_test, 0},\n \t{\"foreach\", module_foreach, SUPPORT_SUPER_PREFIX},\n-\t{\"init\", module_init, SUPPORT_SUPER_PREFIX},\n+\t{\"init\", module_init, 0},\n \t{\"status\", module_status, SUPPORT_SUPER_PREFIX},\n \t{\"sync\", module_sync, SUPPORT_SUPER_PREFIX},\n \t{\"deinit\", module_deinit, 0},\n-\t{\"summary\", module_summary, SUPPORT_SUPER_PREFIX},\n+\t{\"summary\", module_summary, 0},\n \t{\"push-check\", push_check, 0},\n \t{\"absorbgitdirs\", absorb_git_dirs, SUPPORT_SUPER_PREFIX},\n \t{\"is-active\", is_active, 0},\n-- \n2.37.0.rc0.161.g10f37bed90-goog\n\n"},{"id":"458324","messageId":"20220701021157.88858-8-chooglen@google.com","threadId":"58074","inReplyTo":"20220701021157.88858-1-chooglen@google.com","subject":"[PATCH v4 7/7] submodule--helper: remove display path helper","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-07-01T02:11:57Z","receivedAt":"2022-07-01T02:12:43Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"All invocations of do_get_submodule_displaypath() pass\nget_super_prefix() as the super_prefix arg, which is exactly the same\nas get_submodule_displaypath().\n\nReplace all calls to do_get_submodule_displaypath() with\nget_submodule_displaypath(), and since it has no more callers, remove\nit.\n\nSigned-off-by: Glen Choo \n---\n builtin/submodule--helper.c | 24 ++++++++----------------\n 1 file changed, 8 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex eb84e3a98b..405a4224c5 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -118,10 +118,11 @@ static int resolve_relative_url_test(int argc, const char **argv, const char *pr\n \treturn 0;\n }\n \n-static char *do_get_submodule_displaypath(const char *path,\n-\t\t\t\t\t const char *prefix,\n-\t\t\t\t\t const char *super_prefix)\n+/* the result should be freed by the caller. */\n+static char *get_submodule_displaypath(const char *path, const char *prefix)\n {\n+\tconst char *super_prefix = get_super_prefix();\n+\n \tif (prefix && super_prefix) {\n \t\tBUG(\"cannot have prefix '%s' and superprefix '%s'\",\n \t\t prefix, super_prefix);\n@@ -137,13 +138,6 @@ static char *do_get_submodule_displaypath(const char *path,\n \t}\n }\n \n-/* the result should be freed by the caller. */\n-static char *get_submodule_displaypath(const char *path, const char *prefix)\n-{\n-\tconst char *super_prefix = get_super_prefix();\n-\treturn do_get_submodule_displaypath(path, prefix, super_prefix);\n-}\n-\n static char *compute_rev_name(const char *sub_path, const char* object_id)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n@@ -488,7 +482,7 @@ static void init_submodule(const char *path, const char *prefix,\n \tstruct strbuf sb = STRBUF_INIT;\n \tchar *upd = NULL, *url = NULL, *displaypath;\n \n-\tdisplaypath = do_get_submodule_displaypath(path, prefix, get_super_prefix());\n+\tdisplaypath = get_submodule_displaypath(path, prefix);\n \n \tsub = submodule_from_path(the_repository, null_oid(), path);\n \n@@ -1943,8 +1937,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,\n \tenum submodule_update_type update_type;\n \tchar *key;\n \tstruct update_data *ud = suc->update_data;\n-\tchar *displaypath = do_get_submodule_displaypath(ce->name, ud->prefix,\n-\t\t\t\t\t\t\t get_super_prefix());\n+\tchar *displaypath = get_submodule_displaypath(ce->name, ud->prefix);\n \tstruct strbuf sb = STRBUF_INIT;\n \tint needs_cloning = 0;\n \tint need_free_url = 0;\n@@ -2460,9 +2453,8 @@ static int update_submodule(struct update_data *update_data)\n {\n \tensure_core_worktree(update_data->sm_path);\n \n-\tupdate_data->displaypath = do_get_submodule_displaypath(update_data->sm_path,\n-\t\t\t\t\t\t\t\tupdate_data->prefix,\n-\t\t\t\t\t\t\t\tget_super_prefix());\n+\tupdate_data->displaypath = get_submodule_displaypath(\n+\t\tupdate_data->sm_path, update_data->prefix);\n \n \tdetermine_submodule_update_strategy(the_repository, update_data->just_cloned,\n \t\t\t\t\t update_data->sm_path, update_data->update_default,\n-- \n2.37.0.rc0.161.g10f37bed90-goog\n\n"}]}