{"thread":{"id":"58734","subject":"[PATCH 0/8] submodule: tests, cleanup to prepare for built-in","startedAt":"2022-11-02T07:54:13Z","lastAt":"2022-11-08T19:20:21Z","messageCount":35,"participants":["Ævar Arnfjörð Bjarmason","Glen Choo","Simpson, Phyllis","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":8},"messages":[{"id":"466283","messageId":"cover-0.8-00000000000-20221102T074148Z-avarab@gmail.com","threadId":"58734","inReplyTo":null,"subject":"[PATCH 0/8] submodule: tests, cleanup to prepare for built-in","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-02T07:53:57Z","receivedAt":"2022-11-02T07:54:13Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"I have a topic on-list to remove git-submodule.sh and create a\nbuiltin/submodule.c, i.e. promoting \"git submodule--helper\" to the\n\"real thing\"[1].\n\nGlen gave me a bunch of good feedback on it, including (but not\nlimited to) pointing out that we have outstanding dead code in\n[2][3].\n\nOnce I started pulling at that thread things became a lot simpler for\nthe re-roll of [1], e.g. the migration of git-submodule.sh's commands\ncan squash in the \"update\" step, as it's no longer a special-case.\n\nBut that also made the series larger, and it's conflicted with other\noutstanding patches. First René's strvec() cleanup in submodule.c, and\ncurrently with Glen's in-flight submodule topic.\n\nSo here's \"just the prep\" part of that split-out. See also [4] and [5]\nfor previous \"prep\" topics, we're getting closer...\n\nThis only adds missing test coverage, and deletes dead code that we'd\notherwise have to account for. Then 8/8 converts submodule--helper to\nuse the OPT_SUBCOMMAND() API in 8/8.\n\nCI & branch at: https://github.com/avar/git/tree/avar/submodule-builtin-final-prep\n\nFor a peek at the WIP re-roll of [1] that'll come after this:\nhttps://github.com/avar/git/compare/avar/submodule-builtin-final-prep...avar/submodule-sh-dispatch-to-helper-directly-3\n\n1. https://lore.kernel.org/git/cover-00.10-00000000000-20221017T115544Z-avarab@gmail.com/\n2. https://lore.kernel.org/git/kl6lpmemxg8p.fsf@chooglen-macbookpro.roam.corp.google.com/\n3. https://lore.kernel.org/git/kl6lv8oexiyy.fsf@chooglen-macbookpro.roam.corp.google.com/\n4. 361cbe6d6d2 (Merge branch 'ab/submodule-cleanup', 2022-07-14)\n5. f322e9f51b5 (Merge branch 'ab/submodule-helper-prep', 2022-09-13)\n\nÆvar Arnfjörð Bjarmason (8):\n submodule--helper: move \"config\" to a test-tool\n submodule tests: add tests for top-level flag output\n submodule tests: test for a \"foreach\" blind-spot\n submodule.c: refactor recursive block out of absorb function\n submodule API & \"absorbgitdirs\": remove \"----recursive\" option\n submodule--helper: remove --prefix from \"absorbgitdirs\"\n submodule--helper: drop \"update --prefix \" for \"-C update\"\n submodule--helper: use OPT_SUBCOMMAND() API\n\n builtin/rm.c | 3 +-\n builtin/submodule--helper.c | 139 ++++++--------------\n git-submodule.sh | 3 +-\n git.c | 2 +-\n submodule.c | 41 +++---\n submodule.h | 4 +-\n t/helper/test-submodule.c | 84 ++++++++++++\n t/t7400-submodule-basic.sh | 10 ++\n t/t7407-submodule-foreach.sh | 5 +\n t/t7411-submodule-config.sh | 28 ++--\n t/t7418-submodule-sparse-gitmodules.sh | 4 +-\n t/t7422-submodule-output.sh | 169 +++++++++++++++++++++++++\n 12 files changed, 349 insertions(+), 143 deletions(-)\n create mode 100755 t/t7422-submodule-output.sh\n\n-- \n2.38.0.1280.g8136eb6fab2\n\n"},{"id":"466284","messageId":"patch-1.8-b2649f96715-20221102T074148Z-avarab@gmail.com","threadId":"58734","inReplyTo":"cover-0.8-00000000000-20221102T074148Z-avarab@gmail.com","subject":"[PATCH 1/8] submodule--helper: move \"config\" to a test-tool","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-02T07:53:58Z","receivedAt":"2022-11-02T07:54:16Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"As with other moves to \"test-tool\" in f322e9f51b5 (Merge branch\n'ab/submodule-helper-prep', 2022-09-13) the \"config\" sub-command was\nonly used by our own tests.\n\nLet's move it over, and while doing so make it easier to reason about\nby splitting up the various uses for it into separate sub-commands, so\nthat we don't need to count arguments to see what it does.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/submodule--helper.c | 46 --------------\n t/helper/test-submodule.c | 84 ++++++++++++++++++++++++++\n t/t7411-submodule-config.sh | 28 ++++-----\n t/t7418-submodule-sparse-gitmodules.sh | 4 +-\n 4 files changed, 100 insertions(+), 62 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex a7683d35299..6250b95a6f7 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2861,51 +2861,6 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)\n \treturn ret;\n }\n \n-static int module_config(int argc, const char **argv, const char *prefix)\n-{\n-\tenum {\n-\t\tCHECK_WRITEABLE = 1,\n-\t\tDO_UNSET = 2\n-\t} command = 0;\n-\tstruct option module_config_options[] = {\n-\t\tOPT_CMDMODE(0, \"check-writeable\", &command,\n-\t\t\t N_(\"check if it is safe to write to the .gitmodules file\"),\n-\t\t\t CHECK_WRITEABLE),\n-\t\tOPT_CMDMODE(0, \"unset\", &command,\n-\t\t\t N_(\"unset the config in the .gitmodules file\"),\n-\t\t\t DO_UNSET),\n-\t\tOPT_END()\n-\t};\n-\tconst char *const git_submodule_helper_usage[] = {\n-\t\tN_(\"git submodule--helper config []\"),\n-\t\tN_(\"git submodule--helper config --unset \"),\n-\t\t\"git submodule--helper config --check-writeable\",\n-\t\tNULL\n-\t};\n-\n-\targc = parse_options(argc, argv, prefix, module_config_options,\n-\t\t\t git_submodule_helper_usage, PARSE_OPT_KEEP_ARGV0);\n-\n-\tif (argc == 1 && command == CHECK_WRITEABLE)\n-\t\treturn is_writing_gitmodules_ok() ? 0 : -1;\n-\n-\t/* Equivalent to ACTION_GET in builtin/config.c */\n-\tif (argc == 2 && command != DO_UNSET)\n-\t\treturn print_config_from_gitmodules(the_repository, argv[1]);\n-\n-\t/* Equivalent to ACTION_SET in builtin/config.c */\n-\tif (argc == 3 || (argc == 2 && command == DO_UNSET)) {\n-\t\tconst char *value = (argc == 3) ? argv[2] : NULL;\n-\n-\t\tif (!is_writing_gitmodules_ok())\n-\t\t\tdie(_(\"please make sure that the .gitmodules file is in the working tree\"));\n-\n-\t\treturn config_set_in_gitmodules_file_gently(argv[1], value);\n-\t}\n-\n-\tusage_with_options(git_submodule_helper_usage, module_config_options);\n-}\n-\n static int module_set_url(int argc, const char **argv, const char *prefix)\n {\n \tint quiet = 0;\n@@ -3424,7 +3379,6 @@ static struct cmd_struct commands[] = {\n \t{\"summary\", module_summary, 0},\n \t{\"push-check\", push_check, 0},\n \t{\"absorbgitdirs\", absorb_git_dirs, SUPPORT_SUPER_PREFIX},\n-\t{\"config\", module_config, 0},\n \t{\"set-url\", module_set_url, 0},\n \t{\"set-branch\", module_set_branch, 0},\n \t{\"create-branch\", module_create_branch, 0},\ndiff --git a/t/helper/test-submodule.c b/t/helper/test-submodule.c\nindex b7d117cd557..c7cb463a583 100644\n--- a/t/helper/test-submodule.c\n+++ b/t/helper/test-submodule.c\n@@ -111,10 +111,94 @@ static int cmd__submodule_resolve_relative_url(int argc, const char **argv)\n \treturn 0;\n }\n \n+static int cmd__submodule_config_list(int argc, const char **argv)\n+{\n+\tstruct option options[] = {\n+\t\tOPT_END()\n+\t};\n+\tconst char *const usage[] = {\n+\t\t\"test-tool submodule config-list \",\n+\t\tNULL\n+\t};\n+\targc = parse_options(argc, argv, \"test-tools\", options, usage,\n+\t\t\t PARSE_OPT_KEEP_ARGV0);\n+\n+\tsetup_git_directory();\n+\n+\tif (argc == 2)\n+\t\treturn print_config_from_gitmodules(the_repository, argv[1]);\n+\tusage_with_options(usage, options);\n+}\n+\n+static int cmd__submodule_config_set(int argc, const char **argv)\n+{\n+\tstruct option options[] = {\n+\t\tOPT_END()\n+\t};\n+\tconst char *const usage[] = {\n+\t\t\"test-tool submodule config-set \",\n+\t\tNULL\n+\t};\n+\targc = parse_options(argc, argv, \"test-tools\", options, usage,\n+\t\t\t PARSE_OPT_KEEP_ARGV0);\n+\n+\tsetup_git_directory();\n+\n+\t/* Equivalent to ACTION_SET in builtin/config.c */\n+\tif (argc == 3) {\n+\t\tif (!is_writing_gitmodules_ok())\n+\t\t\tdie(_(\"please make sure that the .gitmodules file is in the working tree\"));\n+\n+\t\treturn config_set_in_gitmodules_file_gently(argv[1], argv[2]);\n+\t}\n+\tusage_with_options(usage, options);\n+}\n+\n+static int cmd__submodule_config_unset(int argc, const char **argv)\n+{\n+\tstruct option options[] = {\n+\t\tOPT_END()\n+\t};\n+\tconst char *const usage[] = {\n+\t\t\"test-tool submodule config-unset \",\n+\t\tNULL\n+\t};\n+\n+\tsetup_git_directory();\n+\n+\tif (argc == 2) {\n+\t\tif (!is_writing_gitmodules_ok())\n+\t\t\tdie(_(\"please make sure that the .gitmodules file is in the working tree\"));\n+\t\treturn config_set_in_gitmodules_file_gently(argv[1], NULL);\n+\t}\n+\tusage_with_options(usage, options);\n+}\n+\n+static int cmd__submodule_config_writeable(int argc, const char **argv)\n+{\n+\tstruct option options[] = {\n+\t\tOPT_END()\n+\t};\n+\tconst char *const usage[] = {\n+\t\t\"test-tool submodule config-writeable\",\n+\t\tNULL\n+\t};\n+\tsetup_git_directory();\n+\n+\tif (argc == 1)\n+\t\treturn is_writing_gitmodules_ok() ? 0 : -1;\n+\n+\tusage_with_options(usage, options);\n+}\n+\n static struct test_cmd cmds[] = {\n \t{ \"check-name\", cmd__submodule_check_name },\n \t{ \"is-active\", cmd__submodule_is_active },\n \t{ \"resolve-relative-url\", cmd__submodule_resolve_relative_url},\n+\t{ \"config-list\", cmd__submodule_config_list },\n+\t{ \"config-set\", cmd__submodule_config_set },\n+\t{ \"config-unset\", cmd__submodule_config_unset },\n+\t{ \"config-writeable\", cmd__submodule_config_writeable },\n };\n \n int cmd__submodule(int argc, const char **argv)\ndiff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh\nindex c583c4e373a..2f57ecd177a 100755\n--- a/t/t7411-submodule-config.sh\n+++ b/t/t7411-submodule-config.sh\n@@ -140,15 +140,15 @@ test_expect_success 'error in history in fetchrecursesubmodule lets continue' '\n test_expect_success 'reading submodules config from the working tree with \"submodule--helper config\"' '\n \t(cd super &&\n \t\techo \"../submodule\" >expect &&\n-\t\tgit submodule--helper config submodule.submodule.url >actual &&\n+\t\ttest-tool submodule config-list submodule.submodule.url >actual &&\n \t\ttest_cmp expect actual\n \t)\n '\n \n test_expect_success 'unsetting submodules config from the working tree with \"submodule--helper config --unset\"' '\n \t(cd super &&\n-\t\tgit submodule--helper config --unset submodule.submodule.url &&\n-\t\tgit submodule--helper config submodule.submodule.url >actual &&\n+\t\ttest-tool submodule config-unset submodule.submodule.url &&\n+\t\ttest-tool submodule config-list submodule.submodule.url >actual &&\n \t\ttest_must_be_empty actual\n \t)\n '\n@@ -157,8 +157,8 @@ test_expect_success 'unsetting submodules config from the working tree with \"sub\n test_expect_success 'writing submodules config with \"submodule--helper config\"' '\n \t(cd super &&\n \t\techo \"new_url\" >expect &&\n-\t\tgit submodule--helper config submodule.submodule.url \"new_url\" &&\n-\t\tgit submodule--helper config submodule.submodule.url >actual &&\n+\t\ttest-tool submodule config-set submodule.submodule.url \"new_url\" &&\n+\t\ttest-tool submodule config-list submodule.submodule.url >actual &&\n \t\ttest_cmp expect actual\n \t)\n '\n@@ -167,14 +167,14 @@ test_expect_success 'overwriting unstaged submodules config with \"submodule--hel\n \ttest_when_finished \"git -C super checkout .gitmodules\" &&\n \t(cd super &&\n \t\techo \"newer_url\" >expect &&\n-\t\tgit submodule--helper config submodule.submodule.url \"newer_url\" &&\n-\t\tgit submodule--helper config submodule.submodule.url >actual &&\n+\t\ttest-tool submodule config-set submodule.submodule.url \"newer_url\" &&\n+\t\ttest-tool submodule config-list submodule.submodule.url >actual &&\n \t\ttest_cmp expect actual\n \t)\n '\n \n test_expect_success 'writeable .gitmodules when it is in the working tree' '\n-\tgit -C super submodule--helper config --check-writeable\n+\ttest-tool -C super submodule config-writeable\n '\n \n test_expect_success 'writeable .gitmodules when it is nowhere in the repository' '\n@@ -183,7 +183,7 @@ test_expect_success 'writeable .gitmodules when it is nowhere in the repository'\n \t(cd super &&\n \t\tgit rm .gitmodules &&\n \t\tgit commit -m \"remove .gitmodules from the current branch\" &&\n-\t\tgit submodule--helper config --check-writeable\n+\t\ttest-tool submodule config-writeable\n \t)\n '\n \n@@ -191,7 +191,7 @@ test_expect_success 'non-writeable .gitmodules when it is in the index but not i\n \ttest_when_finished \"git -C super checkout .gitmodules\" &&\n \t(cd super &&\n \t\trm -f .gitmodules &&\n-\t\ttest_must_fail git submodule--helper config --check-writeable\n+\t\ttest_must_fail test-tool submodule config-writeable\n \t)\n '\n \n@@ -200,7 +200,7 @@ test_expect_success 'non-writeable .gitmodules when it is in the current branch\n \ttest_when_finished \"git -C super reset --hard $ORIG\" &&\n \t(cd super &&\n \t\tgit rm .gitmodules &&\n-\t\ttest_must_fail git submodule--helper config --check-writeable\n+\t\ttest_must_fail test-tool submodule config-writeable\n \t)\n '\n \n@@ -208,11 +208,11 @@ test_expect_success 'reading submodules config from the index when .gitmodules i\n \tORIG=$(git -C super rev-parse HEAD) &&\n \ttest_when_finished \"git -C super reset --hard $ORIG\" &&\n \t(cd super &&\n-\t\tgit submodule--helper config submodule.submodule.url \"staged_url\" &&\n+\t\ttest-tool submodule config-set submodule.submodule.url \"staged_url\" &&\n \t\tgit add .gitmodules &&\n \t\trm -f .gitmodules &&\n \t\techo \"staged_url\" >expect &&\n-\t\tgit submodule--helper config submodule.submodule.url >actual &&\n+\t\ttest-tool submodule config-list submodule.submodule.url >actual &&\n \t\ttest_cmp expect actual\n \t)\n '\n@@ -223,7 +223,7 @@ test_expect_success 'reading submodules config from the current branch when .git\n \t(cd super &&\n \t\tgit rm .gitmodules &&\n \t\techo \"../submodule\" >expect &&\n-\t\tgit submodule--helper config submodule.submodule.url >actual &&\n+\t\ttest-tool submodule config-list submodule.submodule.url >actual &&\n \t\ttest_cmp expect actual\n \t)\n '\ndiff --git a/t/t7418-submodule-sparse-gitmodules.sh b/t/t7418-submodule-sparse-gitmodules.sh\nindex d5874200fdc..dde11ecce80 100755\n--- a/t/t7418-submodule-sparse-gitmodules.sh\n+++ b/t/t7418-submodule-sparse-gitmodules.sh\n@@ -50,12 +50,12 @@ test_expect_success 'sparse checkout setup which hides .gitmodules' '\n \n test_expect_success 'reading gitmodules config file when it is not checked out' '\n \techo \"../submodule\" >expect &&\n-\tgit -C super submodule--helper config submodule.submodule.url >actual &&\n+\ttest-tool -C super submodule config-list submodule.submodule.url >actual &&\n \ttest_cmp expect actual\n '\n \n test_expect_success 'not writing gitmodules config file when it is not checked out' '\n-\ttest_must_fail git -C super submodule--helper config submodule.submodule.url newurl &&\n+\ttest_must_fail test-tool -C super submodule config-set submodule.submodule.url newurl &&\n \ttest_path_is_missing super/.gitmodules\n '\n \n-- \n2.38.0.1280.g8136eb6fab2\n\n"},{"id":"466285","messageId":"patch-2.8-cda36b5b6e0-20221102T074148Z-avarab@gmail.com","threadId":"58734","inReplyTo":"cover-0.8-00000000000-20221102T074148Z-avarab@gmail.com","subject":"[PATCH 2/8] submodule tests: add tests for top-level flag output","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-02T07:53:59Z","receivedAt":"2022-11-02T07:54:17Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Exhaustively test for how combining various \"mixed-level\" \"git\nsubmodule\" option works. \"Mixed-level\" here means options that are\naccepted by a mixture of the top-level \"submodule\" command, and\ne.g. the \"status\" sub-command.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t7400-submodule-basic.sh | 10 +++\n t/t7422-submodule-output.sh | 169 ++++++++++++++++++++++++++++++++++++\n 2 files changed, 179 insertions(+)\n create mode 100755 t/t7422-submodule-output.sh\n\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex a989aafaf57..eae6a46ef3d 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -579,6 +579,16 @@ test_expect_success 'status should be \"modified\" after submodule commit' '\n \tgrep \"^+$rev2\" list\n '\n \n+test_expect_success '\"submodule --cached\" command forms should be identical' '\n+\tgit submodule status --cached >expect &&\n+\n+\tgit submodule --cached >actual &&\n+\ttest_cmp expect actual &&\n+\n+\tgit submodule --cached status >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'the --cached sha1 should be rev1' '\n \tgit submodule --cached status >list &&\n \tgrep \"^+$rev1\" list\ndiff --git a/t/t7422-submodule-output.sh b/t/t7422-submodule-output.sh\nnew file mode 100755\nindex 00000000000..3ac56c23f72\n--- /dev/null\n+++ b/t/t7422-submodule-output.sh\n@@ -0,0 +1,169 @@\n+#!/bin/sh\n+\n+test_description='submodule --cached, --quiet etc. output'\n+\n+. ./test-lib.sh\n+. \"$TEST_DIRECTORY\"/lib-t3100.sh\n+\n+setup_sub () {\n+\tlocal d=\"$1\" &&\n+\tshift &&\n+\tgit $@ clone . \"$d\" &&\n+\tgit $@ submodule add ./\"$d\"\n+}\n+\n+normalize_status () {\n+\tsed -e 's/-g[0-9a-f]*/-gHASH/'\n+}\n+\n+test_expect_success 'setup' '\n+\ttest_commit A &&\n+\ttest_commit B &&\n+\tsetup_sub S &&\n+\tsetup_sub S.D &&\n+\tsetup_sub S.C &&\n+\tsetup_sub S.C.D &&\n+\tsetup_sub X &&\n+\tgit add S* &&\n+\ttest_commit C &&\n+\n+\t# recursive in X/\n+\tgit -C X pull &&\n+\tGIT_ALLOW_PROTOCOL=file git -C X submodule update --init &&\n+\n+\t# dirty p\n+\tfor d in S.D X/S.D\n+\tdo\n+\t\techo dirty >S.D/A.t || return 1\n+\tdone &&\n+\n+\t# commit (for --cached)\n+\tfor d in S.C* X/S.C*\n+\tdo\n+\t\tgit -C \"$d\" reset --hard A || return 1\n+\tdone &&\n+\n+\t# dirty\n+\tfor d in S*.D X/S*.D\n+\tdo\n+\t\techo dirty >\"$d/C2.t\" || return 1\n+\tdone &&\n+\n+\tfor ref in A B C\n+\tdo\n+\t\t# Not different with SHA-1 and SHA-256, just (ab)usign\n+\t\t# test_oid_cache as a variable bag to avoid using\n+\t\t# $(git rev-parse ...).\n+\t\toid=$(git rev-parse $ref) &&\n+\t\ttest_oid_cache <<-EOF || return 1\n+\t\t$ref sha1:$oid\n+\t\t$ref sha256:$oid\n+\t\tEOF\n+\tdone\n+'\n+\n+for opts in \"\" \"status\"\n+do\n+\ttest_expect_success \"git submodule $opts\" '\n+\t\tsed -e \"s/^>//\" >expect <<-EOF &&\n+\t\t> $(test_oid B) S (B)\n+\t\t>+$(test_oid A) S.C (A)\n+\t\t>+$(test_oid A) S.C.D (A)\n+\t\t> $(test_oid B) S.D (B)\n+\t\t>+$(test_oid C) X (C)\n+\t\tEOF\n+\t\tgit submodule $opts >actual.raw &&\n+\t\tnormalize_status actual &&\n+\t\ttest_cmp expect actual\n+\t'\n+done\n+\n+for opts in \\\n+\t\"status --recursive\"\n+do\n+\ttest_expect_success \"git submodule $opts\" '\n+\t\tsed -e \"s/^>//\" >expect <<-EOF &&\n+\t\t> $(test_oid B) S (B)\n+\t\t>+$(test_oid A) S.C (A)\n+\t\t>+$(test_oid A) S.C.D (A)\n+\t\t> $(test_oid B) S.D (B)\n+\t\t>+$(test_oid C) X (C)\n+\t\t> $(test_oid B) X/S (B)\n+\t\t>+$(test_oid A) X/S.C (A)\n+\t\t>+$(test_oid A) X/S.C.D (A)\n+\t\t> $(test_oid B) X/S.D (B)\n+\t\t> $(test_oid B) X/X (B)\n+\t\tEOF\n+\t\tgit submodule $opts >actual.raw &&\n+\t\tnormalize_status actual &&\n+\t\ttest_cmp expect actual\n+\t'\n+done\n+\n+for opts in \\\n+\t\"--quiet\" \\\n+\t\"--quiet status\" \\\n+\t\"status --quiet\"\n+do\n+\ttest_expect_success \"git submodule $opts\" '\n+\t\tgit submodule $opts >out &&\n+\t\ttest_must_be_empty out\n+\t'\n+done\n+\n+for opts in \\\n+\t\"--cached\" \\\n+\t\"--cached status\" \\\n+\t\"status --cached\"\n+do\n+\ttest_expect_success \"git submodule $opts\" '\n+\t\tsed -e \"s/^>//\" >expect <<-EOF &&\n+\t\t> $(test_oid B) S (B)\n+\t\t>+$(test_oid B) S.C (B)\n+\t\t>+$(test_oid B) S.C.D (B)\n+\t\t> $(test_oid B) S.D (B)\n+\t\t>+$(test_oid B) X (B)\n+\t\tEOF\n+\t\tgit submodule $opts >actual.raw &&\n+\t\tnormalize_status actual &&\n+\t\ttest_cmp expect actual\n+\t'\n+done\n+\n+for opts in \\\n+\t\"--cached --quiet\" \\\n+\t\"--cached --quiet status\" \\\n+\t\"--cached status --quiet\" \\\n+\t\"--quiet status --cached\" \\\n+\t\"status --cached --quiet\"\n+do\n+\ttest_expect_success \"git submodule $opts\" '\n+\t\tgit submodule $opts >out &&\n+\t\ttest_must_be_empty out\n+\t'\n+done\n+\n+for opts in \\\n+\t\"status --cached --recursive\" \\\n+\t\"--cached status --recursive\"\n+do\n+\ttest_expect_success \"git submodule $opts\" '\n+\t\tsed -e \"s/^>//\" >expect <<-EOF &&\n+\t\t> $(test_oid B) S (B)\n+\t\t>+$(test_oid B) S.C (B)\n+\t\t>+$(test_oid B) S.C.D (B)\n+\t\t> $(test_oid B) S.D (B)\n+\t\t>+$(test_oid B) X (B)\n+\t\t> $(test_oid B) X/S (B)\n+\t\t>+$(test_oid B) X/S.C (B)\n+\t\t>+$(test_oid B) X/S.C.D (B)\n+\t\t> $(test_oid B) X/S.D (B)\n+\t\t> $(test_oid B) X/X (B)\n+\t\tEOF\n+\t\tgit submodule $opts >actual.raw &&\n+\t\tnormalize_status actual &&\n+\t\ttest_cmp expect actual\n+\t'\n+done\n+\n+test_done\n-- \n2.38.0.1280.g8136eb6fab2\n\n"},{"id":"466286","messageId":"patch-3.8-0ed1fc7fdf8-20221102T074148Z-avarab@gmail.com","threadId":"58734","inReplyTo":"cover-0.8-00000000000-20221102T074148Z-avarab@gmail.com","subject":"[PATCH 3/8] submodule tests: test for a \"foreach\" blind-spot","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-02T07:54:00Z","receivedAt":"2022-11-02T07:54:25Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"We tested for \"--\" followed by command names, but not for \"--\"\nfollowed by an argument that looks like an option, let's do that.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t7407-submodule-foreach.sh | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh\nindex 59bd1501667..8d7b234beb8 100755\n--- a/t/t7407-submodule-foreach.sh\n+++ b/t/t7407-submodule-foreach.sh\n@@ -154,6 +154,11 @@ test_expect_success 'use \"submodule foreach\" to checkout 2nd level submodule' '\n \t)\n '\n \n+test_expect_success 'usage: foreach -- --not-an-option' '\n+\ttest_expect_code 1 git submodule foreach -- --not-an-option &&\n+\ttest_expect_code 1 git -C clone2 submodule foreach -- --not-an-option\n+'\n+\n test_expect_success 'use \"foreach --recursive\" to checkout all submodules' '\n \t(\n \t\tcd clone2 &&\n-- \n2.38.0.1280.g8136eb6fab2\n\n"},{"id":"466287","messageId":"patch-4.8-f7adfbc13ae-20221102T074148Z-avarab@gmail.com","threadId":"58734","inReplyTo":"cover-0.8-00000000000-20221102T074148Z-avarab@gmail.com","subject":"[PATCH 4/8] submodule.c: refactor recursive block out of absorb function","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-02T07:54:01Z","receivedAt":"2022-11-02T07:54:27Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"A move and indentation-only change to move the\nABSORB_GITDIR_RECURSE_SUBMODULES case into its own function, which as\nwe'll see makes the subsequent commit changing this code much smaller.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n submodule.c | 30 ++++++++++++++++++------------\n 1 file changed, 18 insertions(+), 12 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex b958162d286..fe1e3f03905 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -2310,6 +2310,23 @@ static void relocate_single_git_dir_into_superproject(const char *path)\n \tstrbuf_release(&new_gitdir);\n }\n \n+static void absorb_git_dir_into_superproject_recurse(const char *path)\n+{\n+\n+\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\n+\tcp.dir = path;\n+\tcp.git_cmd = 1;\n+\tcp.no_stdin = 1;\n+\tstrvec_pushf(&cp.args, \"--super-prefix=%s%s/\",\n+\t\t get_super_prefix_or_empty(), path);\n+\tstrvec_pushl(&cp.args, \"submodule--helper\",\n+\t\t \"absorbgitdirs\", NULL);\n+\tprepare_submodule_repo_env(&cp.env);\n+\tif (run_command(&cp))\n+\t\tdie(_(\"could not recurse into submodule '%s'\"), path);\n+}\n+\n /*\n * Migrate the git directory of the submodule given by path from\n * having its git directory within the working tree to the git dir nested\n@@ -2366,21 +2383,10 @@ void absorb_git_dir_into_superproject(const char *path,\n \tstrbuf_release(&gitdir);\n \n \tif (flags & ABSORB_GITDIR_RECURSE_SUBMODULES) {\n-\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n-\n \t\tif (flags & ~ABSORB_GITDIR_RECURSE_SUBMODULES)\n \t\t\tBUG(\"we don't know how to pass the flags down?\");\n \n-\t\tcp.dir = path;\n-\t\tcp.git_cmd = 1;\n-\t\tcp.no_stdin = 1;\n-\t\tstrvec_pushf(&cp.args, \"--super-prefix=%s%s/\",\n-\t\t\t get_super_prefix_or_empty(), path);\n-\t\tstrvec_pushl(&cp.args, \"submodule--helper\",\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+\t\tabsorb_git_dir_into_superproject_recurse(path);\n \t}\n }\n \n-- \n2.38.0.1280.g8136eb6fab2\n\n"},{"id":"466288","messageId":"patch-5.8-2b8afd73b9b-20221102T074148Z-avarab@gmail.com","threadId":"58734","inReplyTo":"cover-0.8-00000000000-20221102T074148Z-avarab@gmail.com","subject":"[PATCH 5/8] submodule API & \"absorbgitdirs\": remove \"----recursive\" option","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-02T07:54:02Z","receivedAt":"2022-11-02T07:54:30Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Remove the \"----recursive\" option to \"git submodule--helper\nabsorbgitdirs\" (yes, with 4 dashes, not 2).\n\nThis option and all the \"else\" when \"flags &\nABSORB_GITDIR_RECURSE_SUBMODULES\" is false has never been used since\nit was added in f6f85861400 (submodule: add absorb-git-dir function,\n2016-12-12), which we'd have had to do as \"----recursive\", a\n\"--recursive\" would have errored out.\n\nIt would be nice to follow-up with an optbug() assertion to\nparse-options.c for such funnily named options, I manually validated\nthat this was the only long option whose name started with \"-\", but\nlet's skip adding such an assertion for now.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/rm.c | 3 +--\n builtin/submodule--helper.c | 8 ++------\n submodule.c | 13 +++----------\n submodule.h | 4 +---\n 4 files changed, 7 insertions(+), 21 deletions(-)\n\ndiff --git a/builtin/rm.c b/builtin/rm.c\nindex f0d025a4e23..05bfe20a469 100644\n--- a/builtin/rm.c\n+++ b/builtin/rm.c\n@@ -86,8 +86,7 @@ static void submodules_absorb_gitdir_if_needed(void)\n \t\t\tcontinue;\n \n \t\tif (!submodule_uses_gitfile(name))\n-\t\t\tabsorb_git_dir_into_superproject(name,\n-\t\t\t\tABSORB_GITDIR_RECURSE_SUBMODULES);\n+\t\t\tabsorb_git_dir_into_superproject(name);\n \t}\n }\n \ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 6250b95a6f7..8b4af8430dc 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1378,8 +1378,7 @@ static void deinit_submodule(const char *path, const char *prefix,\n \t\t\t\t\t \".git file by using absorbgitdirs.\"),\n \t\t\t\t\tdisplaypath);\n \n-\t\t\tabsorb_git_dir_into_superproject(path,\n-\t\t\t\t\t\t\t ABSORB_GITDIR_RECURSE_SUBMODULES);\n+\t\t\tabsorb_git_dir_into_superproject(path);\n \n \t\t}\n \n@@ -2830,13 +2829,10 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)\n \tint i;\n \tstruct pathspec pathspec = { 0 };\n \tstruct module_list list = MODULE_LIST_INIT;\n-\tunsigned flags = ABSORB_GITDIR_RECURSE_SUBMODULES;\n \tstruct option embed_gitdir_options[] = {\n \t\tOPT_STRING(0, \"prefix\", &prefix,\n \t\t\t N_(\"path\"),\n \t\t\t N_(\"path into the working tree\")),\n-\t\tOPT_BIT(0, \"--recursive\", &flags, N_(\"recurse into submodules\"),\n-\t\t\tABSORB_GITDIR_RECURSE_SUBMODULES),\n \t\tOPT_END()\n \t};\n \tconst char *const git_submodule_helper_usage[] = {\n@@ -2852,7 +2848,7 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)\n \t\tgoto cleanup;\n \n \tfor (i = 0; i < list.nr; i++)\n-\t\tabsorb_git_dir_into_superproject(list.entries[i]->name, flags);\n+\t\tabsorb_git_dir_into_superproject(list.entries[i]->name);\n \n \tret = 0;\n cleanup:\ndiff --git a/submodule.c b/submodule.c\nindex fe1e3f03905..8fa2ad457b2 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -2139,8 +2139,7 @@ int submodule_move_head(const char *path,\n \tif (!(flags & SUBMODULE_MOVE_HEAD_DRY_RUN)) {\n \t\tif (old_head) {\n \t\t\tif (!submodule_uses_gitfile(path))\n-\t\t\t\tabsorb_git_dir_into_superproject(path,\n-\t\t\t\t\tABSORB_GITDIR_RECURSE_SUBMODULES);\n+\t\t\t\tabsorb_git_dir_into_superproject(path);\n \t\t} else {\n \t\t\tstruct strbuf gitdir = STRBUF_INIT;\n \t\t\tsubmodule_name_to_gitdir(&gitdir, the_repository,\n@@ -2332,8 +2331,7 @@ static void absorb_git_dir_into_superproject_recurse(const char *path)\n * having its git directory within the working tree to the git dir nested\n * in its superprojects git dir under modules/.\n */\n-void absorb_git_dir_into_superproject(const char *path,\n-\t\t\t\t unsigned flags)\n+void absorb_git_dir_into_superproject(const char *path)\n {\n \tint err_code;\n \tconst char *sub_git_dir;\n@@ -2382,12 +2380,7 @@ void absorb_git_dir_into_superproject(const char *path,\n \t}\n \tstrbuf_release(&gitdir);\n \n-\tif (flags & ABSORB_GITDIR_RECURSE_SUBMODULES) {\n-\t\tif (flags & ~ABSORB_GITDIR_RECURSE_SUBMODULES)\n-\t\t\tBUG(\"we don't know how to pass the flags down?\");\n-\n-\t\tabsorb_git_dir_into_superproject_recurse(path);\n-\t}\n+\tabsorb_git_dir_into_superproject_recurse(path);\n }\n \n int get_superproject_working_tree(struct strbuf *buf)\ndiff --git a/submodule.h b/submodule.h\nindex 6a9fec6de11..b52a4ff1e73 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -164,9 +164,7 @@ void submodule_unset_core_worktree(const struct submodule *sub);\n */\n void prepare_submodule_repo_env(struct strvec *env);\n \n-#define ABSORB_GITDIR_RECURSE_SUBMODULES (1<<0)\n-void absorb_git_dir_into_superproject(const char *path,\n-\t\t\t\t unsigned flags);\n+void absorb_git_dir_into_superproject(const char *path);\n \n /*\n * Return the absolute path of the working tree of the superproject, which this\n-- \n2.38.0.1280.g8136eb6fab2\n\n"},{"id":"466289","messageId":"patch-6.8-91208241070-20221102T074148Z-avarab@gmail.com","threadId":"58734","inReplyTo":"cover-0.8-00000000000-20221102T074148Z-avarab@gmail.com","subject":"[PATCH 6/8] submodule--helper: remove --prefix from \"absorbgitdirs\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-02T07:54:03Z","receivedAt":"2022-11-02T07:54:32Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Let's pass the \"-C \" option instead to \"absorbgitdirs\" from\nits only caller.\n\nWhen it was added in f6f85861400 (submodule: add absorb-git-dir\nfunction, 2016-12-12) there were other \"submodule--helper\" subcommands\nthat were invoked with \"-C \", so we could have done this all\nalong.\n\nSuggested-by: Glen Choo \nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/submodule--helper.c | 3 ---\n git-submodule.sh | 2 +-\n 2 files changed, 1 insertion(+), 4 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 8b4af8430dc..6bbefd34374 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2830,9 +2830,6 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)\n \tstruct pathspec pathspec = { 0 };\n \tstruct module_list list = MODULE_LIST_INIT;\n \tstruct option embed_gitdir_options[] = {\n-\t\tOPT_STRING(0, \"prefix\", &prefix,\n-\t\t\t N_(\"path\"),\n-\t\t\t N_(\"path into the working tree\")),\n \t\tOPT_END()\n \t};\n \tconst char *const git_submodule_helper_usage[] = {\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 5e5d21c010f..d359f171379 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -557,7 +557,7 @@ cmd_sync()\n \n cmd_absorbgitdirs()\n {\n-\tgit submodule--helper absorbgitdirs --prefix \"$wt_prefix\" \"$@\"\n+\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper absorbgitdirs \"$@\"\n }\n \n # This loop parses the command line arguments to find the\n-- \n2.38.0.1280.g8136eb6fab2\n\n"},{"id":"466290","messageId":"patch-7.8-77d4d5a6c09-20221102T074148Z-avarab@gmail.com","threadId":"58734","inReplyTo":"cover-0.8-00000000000-20221102T074148Z-avarab@gmail.com","subject":"[PATCH 7/8] submodule--helper: drop \"update --prefix \" for \"-C update\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-02T07:54:04Z","receivedAt":"2022-11-02T07:54:35Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Since 29a5e9e1ffe (submodule--helper update-clone: learn --init,\n2022-03-04) we've been passing \"-C \" from \"git-submodule.sh\"\nwhenever we pass \"--prefix \", so the latter is redundant to\nthe former. Let's drop the \"--prefix\" option.\n\nSuggested-by: Glen Choo \nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/submodule--helper.c | 4 +---\n git-submodule.sh | 1 -\n 2 files changed, 1 insertion(+), 4 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 6bbefd34374..2012ad31d7f 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2642,9 +2642,6 @@ static int module_update(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"traverse submodules recursively\")),\n \t\tOPT_BOOL('N', \"no-fetch\", &opt.nofetch,\n \t\t\t N_(\"don't fetch new objects from the remote site\")),\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_SET_INT(0, \"checkout\", &opt.update_default,\n \t\t\tN_(\"use the 'checkout' update strategy (default)\"),\n \t\t\tSM_UPDATE_CHECKOUT),\n@@ -2700,6 +2697,7 @@ static int module_update(int argc, const char **argv, const char *prefix)\n \t}\n \n \topt.filter_options = &filter_options;\n+\topt.prefix = prefix;\n \n \tif (opt.update_default)\n \t\topt.update_strategy.type = opt.update_default;\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex d359f171379..9a50f2e9124 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -343,7 +343,6 @@ cmd_update()\n \t\t${recursive:+--recursive} \\\n \t\t${init:+--init} \\\n \t\t${nofetch:+--no-fetch} \\\n-\t\t${wt_prefix:+--prefix \"$wt_prefix\"} \\\n \t\t${rebase:+--rebase} \\\n \t\t${merge:+--merge} \\\n \t\t${checkout:+--checkout} \\\n-- \n2.38.0.1280.g8136eb6fab2\n\n"},{"id":"466291","messageId":"patch-8.8-105853cd358-20221102T074148Z-avarab@gmail.com","threadId":"58734","inReplyTo":"cover-0.8-00000000000-20221102T074148Z-avarab@gmail.com","subject":"[PATCH 8/8] submodule--helper: use OPT_SUBCOMMAND() API","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-02T07:54:05Z","receivedAt":"2022-11-02T07:54:37Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Have the cmd_submodule__helper() use the OPT_SUBCOMMAND() API\nintroduced in fa83cc834da (parse-options: add support for parsing\nsubcommands, 2022-08-19).\n\nThis is only a marginal reduction in line count, but once we start\nunifying this with a yet-to-be-added \"builtin/submodule.c\" it'll be\nmuch easier to reason about those changes, as they'll both use\nOPT_SUBCOMMAND().\n\nWe don't need to worry about \"argv[0]\" being NULL in the die() because\nwe'd have errored out in parse_options() as we're not using\n\"PARSE_OPT_SUBCOMMAND_OPTIONAL\".\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/submodule--helper.c | 78 ++++++++++++++++++-------------------\n git.c | 2 +-\n 2 files changed, 39 insertions(+), 41 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 2012ad31d7f..0bc25dcf5ae 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -3350,47 +3350,45 @@ static int module_add(int argc, const char **argv, const char *prefix)\n \treturn ret;\n }\n \n-#define SUPPORT_SUPER_PREFIX (1<<0)\n-\n-struct cmd_struct {\n-\tconst char *cmd;\n-\tint (*fn)(int, const char **, const char *);\n-\tunsigned option;\n-};\n-\n-static struct cmd_struct commands[] = {\n-\t{\"clone\", module_clone, SUPPORT_SUPER_PREFIX},\n-\t{\"add\", module_add, 0},\n-\t{\"update\", module_update, SUPPORT_SUPER_PREFIX},\n-\t{\"foreach\", module_foreach, 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, 0},\n-\t{\"push-check\", push_check, 0},\n-\t{\"absorbgitdirs\", absorb_git_dirs, SUPPORT_SUPER_PREFIX},\n-\t{\"set-url\", module_set_url, 0},\n-\t{\"set-branch\", module_set_branch, 0},\n-\t{\"create-branch\", module_create_branch, 0},\n-};\n-\n int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\n {\n-\tint i;\n-\tif (argc < 2 || !strcmp(argv[1], \"-h\"))\n-\t\tusage(\"git submodule--helper \");\n-\n-\tfor (i = 0; i < ARRAY_SIZE(commands); i++) {\n-\t\tif (!strcmp(argv[1], commands[i].cmd)) {\n-\t\t\tif (get_super_prefix() &&\n-\t\t\t !(commands[i].option & SUPPORT_SUPER_PREFIX))\n-\t\t\t\tdie(_(\"%s doesn't support --super-prefix\"),\n-\t\t\t\t commands[i].cmd);\n-\t\t\treturn commands[i].fn(argc - 1, argv + 1, prefix);\n-\t\t}\n-\t}\n+\tconst char *cmd = argv[0];\n+\tconst char *subcmd;\n+\tparse_opt_subcommand_fn *fn = NULL;\n+\tconst char *const usage[] = {\n+\t\tN_(\"git submodule--helper \"),\n+\t\tNULL\n+\t};\n+\tstruct option options[] = {\n+\t\tOPT_SUBCOMMAND(\"clone\", &fn, module_clone),\n+\t\tOPT_SUBCOMMAND(\"add\", &fn, module_add),\n+\t\tOPT_SUBCOMMAND(\"update\", &fn, module_update),\n+\t\tOPT_SUBCOMMAND(\"foreach\", &fn, module_foreach),\n+\t\tOPT_SUBCOMMAND(\"init\", &fn, module_init),\n+\t\tOPT_SUBCOMMAND(\"status\", &fn, module_status),\n+\t\tOPT_SUBCOMMAND(\"sync\", &fn, module_sync),\n+\t\tOPT_SUBCOMMAND(\"deinit\", &fn, module_deinit),\n+\t\tOPT_SUBCOMMAND(\"summary\", &fn, module_summary),\n+\t\tOPT_SUBCOMMAND(\"push-check\", &fn, push_check),\n+\t\tOPT_SUBCOMMAND(\"absorbgitdirs\", &fn, absorb_git_dirs),\n+\t\tOPT_SUBCOMMAND(\"set-url\", &fn, module_set_url),\n+\t\tOPT_SUBCOMMAND(\"set-branch\", &fn, module_set_branch),\n+\t\tOPT_SUBCOMMAND(\"create-branch\", &fn, module_create_branch),\n+\t\tOPT_END()\n+\t};\n+\targc = parse_options(argc, argv, prefix, options, usage, 0);\n+\tsubcmd = argv[0];\n+\n+\tif (strcmp(subcmd, \"clone\") && strcmp(subcmd, \"update\") &&\n+\t strcmp(subcmd, \"foreach\") && strcmp(subcmd, \"status\") &&\n+\t strcmp(subcmd, \"sync\") && strcmp(subcmd, \"absorbgitdirs\") &&\n+\t get_super_prefix())\n+\t\t/*\n+\t\t * xstrfmt() rather than \"%s %s\" to keep the translated\n+\t\t * string identical to git.c's.\n+\t\t */\n+\t\tdie(_(\"%s doesn't support --super-prefix\"),\n+\t\t xstrfmt(\"'%s %s'\", cmd, subcmd));\n \n-\tdie(_(\"'%s' is not a valid submodule--helper \"\n-\t \"subcommand\"), argv[1]);\n+\treturn fn(argc, argv, prefix);\n }\ndiff --git a/git.c b/git.c\nindex ee7758dcb0e..fb69e605912 100644\n--- a/git.c\n+++ b/git.c\n@@ -610,7 +610,7 @@ static struct cmd_struct commands[] = {\n \t{ \"stash\", cmd_stash, RUN_SETUP | NEED_WORK_TREE },\n \t{ \"status\", cmd_status, RUN_SETUP | NEED_WORK_TREE },\n \t{ \"stripspace\", cmd_stripspace },\n-\t{ \"submodule--helper\", cmd_submodule__helper, RUN_SETUP | SUPPORT_SUPER_PREFIX | NO_PARSEOPT },\n+\t{ \"submodule--helper\", cmd_submodule__helper, RUN_SETUP | SUPPORT_SUPER_PREFIX },\n \t{ \"switch\", cmd_switch, RUN_SETUP | NEED_WORK_TREE },\n \t{ \"symbolic-ref\", cmd_symbolic_ref, RUN_SETUP },\n \t{ \"tag\", cmd_tag, RUN_SETUP | DELAY_PAGER_CONFIG },\n-- \n2.38.0.1280.g8136eb6fab2\n\n"},{"id":"466455","messageId":"kl6lfsezofk9.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"58734","inReplyTo":"patch-1.8-b2649f96715-20221102T074148Z-avarab@gmail.com","subject":"Re: [PATCH 1/8] submodule--helper: move \"config\" to a test-tool","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-11-03T22:09:58Z","receivedAt":"2022-11-03T22:10:13Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> As with other moves to \"test-tool\" in f322e9f51b5 (Merge branch\n> 'ab/submodule-helper-prep', 2022-09-13) the \"config\" sub-command was\n> only used by our own tests.\n>\n> Let's move it over, and while doing so make it easier to reason about\n> by splitting up the various uses for it into separate sub-commands, so\n> that we don't need to count arguments to see what it does.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> builtin/submodule--helper.c | 46 --------------\n> t/helper/test-submodule.c | 84 ++++++++++++++++++++++++++\n> t/t7411-submodule-config.sh | 28 ++++-----\n> t/t7418-submodule-sparse-gitmodules.sh | 4 +-\n> 4 files changed, 100 insertions(+), 62 deletions(-)\n>\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index a7683d35299..6250b95a6f7 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -2861,51 +2861,6 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)\n> \treturn ret;\n> }\n> \n> -static int module_config(int argc, const char **argv, const char *prefix)\n> -{\n> -\tenum {\n> -\t\tCHECK_WRITEABLE = 1,\n> -\t\tDO_UNSET = 2\n> -\t} command = 0;\n> -\tstruct option module_config_options[] = {\n> -\t\tOPT_CMDMODE(0, \"check-writeable\", &command,\n> -\t\t\t N_(\"check if it is safe to write to the .gitmodules file\"),\n> -\t\t\t CHECK_WRITEABLE),\n> -\t\tOPT_CMDMODE(0, \"unset\", &command,\n> -\t\t\t N_(\"unset the config in the .gitmodules file\"),\n> -\t\t\t DO_UNSET),\n> -\t\tOPT_END()\n> -\t};\n> -\tconst char *const git_submodule_helper_usage[] = {\n> -\t\tN_(\"git submodule--helper config []\"),\n> -\t\tN_(\"git submodule--helper config --unset \"),\n> -\t\t\"git submodule--helper config --check-writeable\",\n> -\t\tNULL\n> -\t};\n> -\n> -\targc = parse_options(argc, argv, prefix, module_config_options,\n> -\t\t\t git_submodule_helper_usage, PARSE_OPT_KEEP_ARGV0);\n> -\n> -\tif (argc == 1 && command == CHECK_WRITEABLE)\n> -\t\treturn is_writing_gitmodules_ok() ? 0 : -1;\n> -\n> -\t/* Equivalent to ACTION_GET in builtin/config.c */\n> -\tif (argc == 2 && command != DO_UNSET)\n> -\t\treturn print_config_from_gitmodules(the_repository, argv[1]);\n> -\n> -\t/* Equivalent to ACTION_SET in builtin/config.c */\n> -\tif (argc == 3 || (argc == 2 && command == DO_UNSET)) {\n> -\t\tconst char *value = (argc == 3) ? argv[2] : NULL;\n> -\n> -\t\tif (!is_writing_gitmodules_ok())\n> -\t\t\tdie(_(\"please make sure that the .gitmodules file is in the working tree\"));\n> -\n> -\t\treturn config_set_in_gitmodules_file_gently(argv[1], value);\n> -\t}\n> -\n> -\tusage_with_options(git_submodule_helper_usage, module_config_options);\n> -}\n> -\n> static int module_set_url(int argc, const char **argv, const char *prefix)\n> {\n> \tint quiet = 0;\n\n\"git submodule--helper config\" isn't familiar to me, so I did a bit of\ndigging. It looks like it falls into the category of submodule--helper\nsubcommmands that exist to expose functionality that's easy to do\nin-process, but otherwise inaccessible to git-submodule.sh. \n\nIts last use (I think) was in a6226fd772 (submodule--helper: convert the\nbulk of cmd_add() to C, 2021-08-10).\n\n> @@ -3424,7 +3379,6 @@ static struct cmd_struct commands[] = {\n> \t{\"summary\", module_summary, 0},\n> \t{\"push-check\", push_check, 0},\n> \t{\"absorbgitdirs\", absorb_git_dirs, SUPPORT_SUPER_PREFIX},\n> -\t{\"config\", module_config, 0},\n> \t{\"set-url\", module_set_url, 0},\n> \t{\"set-branch\", module_set_branch, 0},\n> \t{\"create-branch\", module_create_branch, 0},\n> diff --git a/t/helper/test-submodule.c b/t/helper/test-submodule.c\n> index b7d117cd557..c7cb463a583 100644\n> --- a/t/helper/test-submodule.c\n> +++ b/t/helper/test-submodule.c\n> @@ -111,10 +111,94 @@ static int cmd__submodule_resolve_relative_url(int argc, const char **argv)\n> \treturn 0;\n> }\n> \n> +static int cmd__submodule_config_list(int argc, const char **argv)\n> +{\n> +\tstruct option options[] = {\n> +\t\tOPT_END()\n> +\t};\n> +\tconst char *const usage[] = {\n> +\t\t\"test-tool submodule config-list \",\n> +\t\tNULL\n> +\t};\n> +\targc = parse_options(argc, argv, \"test-tools\", options, usage,\n> +\t\t\t PARSE_OPT_KEEP_ARGV0);\n> +\n> +\tsetup_git_directory();\n> +\n> +\tif (argc == 2)\n> +\t\treturn print_config_from_gitmodules(the_repository, argv[1]);\n> +\tusage_with_options(usage, options);\n> +}\n> +\n> +static int cmd__submodule_config_set(int argc, const char **argv)\n> +{\n> +\tstruct option options[] = {\n> +\t\tOPT_END()\n> +\t};\n> +\tconst char *const usage[] = {\n> +\t\t\"test-tool submodule config-set \",\n> +\t\tNULL\n> +\t};\n> +\targc = parse_options(argc, argv, \"test-tools\", options, usage,\n> +\t\t\t PARSE_OPT_KEEP_ARGV0);\n> +\n> +\tsetup_git_directory();\n> +\n> +\t/* Equivalent to ACTION_SET in builtin/config.c */\n> +\tif (argc == 3) {\n> +\t\tif (!is_writing_gitmodules_ok())\n> +\t\t\tdie(_(\"please make sure that the .gitmodules file is in the working tree\"));\n> +\n> +\t\treturn config_set_in_gitmodules_file_gently(argv[1], argv[2]);\n> +\t}\n> +\tusage_with_options(usage, options);\n> +}\n> +\n> +static int cmd__submodule_config_unset(int argc, const char **argv)\n> +{\n> +\tstruct option options[] = {\n> +\t\tOPT_END()\n> +\t};\n> +\tconst char *const usage[] = {\n> +\t\t\"test-tool submodule config-unset \",\n> +\t\tNULL\n> +\t};\n> +\n> +\tsetup_git_directory();\n> +\n> +\tif (argc == 2) {\n> +\t\tif (!is_writing_gitmodules_ok())\n> +\t\t\tdie(_(\"please make sure that the .gitmodules file is in the working tree\"));\n> +\t\treturn config_set_in_gitmodules_file_gently(argv[1], NULL);\n> +\t}\n> +\tusage_with_options(usage, options);\n> +}\n> +\n> +static int cmd__submodule_config_writeable(int argc, const char **argv)\n> +{\n> +\tstruct option options[] = {\n> +\t\tOPT_END()\n> +\t};\n> +\tconst char *const usage[] = {\n> +\t\t\"test-tool submodule config-writeable\",\n> +\t\tNULL\n> +\t};\n> +\tsetup_git_directory();\n> +\n> +\tif (argc == 1)\n> +\t\treturn is_writing_gitmodules_ok() ? 0 : -1;\n> +\n> +\tusage_with_options(usage, options);\n> +}\n> +\n> static struct test_cmd cmds[] = {\n> \t{ \"check-name\", cmd__submodule_check_name },\n> \t{ \"is-active\", cmd__submodule_is_active },\n> \t{ \"resolve-relative-url\", cmd__submodule_resolve_relative_url},\n> +\t{ \"config-list\", cmd__submodule_config_list },\n> +\t{ \"config-set\", cmd__submodule_config_set },\n> +\t{ \"config-unset\", cmd__submodule_config_unset },\n> +\t{ \"config-writeable\", cmd__submodule_config_writeable },\n> };\n> \n> int cmd__submodule(int argc, const char **argv)\n> diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh\n> index c583c4e373a..2f57ecd177a 100755\n> --- a/t/t7411-submodule-config.sh\n> +++ b/t/t7411-submodule-config.sh\n> @@ -140,15 +140,15 @@ test_expect_success 'error in history in fetchrecursesubmodule lets continue' '\n> test_expect_success 'reading submodules config from the working tree with \"submodule--helper config\"' '\n> \t(cd super &&\n> \t\techo \"../submodule\" >expect &&\n> -\t\tgit submodule--helper config submodule.submodule.url >actual &&\n> +\t\ttest-tool submodule config-list submodule.submodule.url >actual &&\n> \t\ttest_cmp expect actual\n> \t)\n> '\n> \n> test_expect_success 'unsetting submodules config from the working tree with \"submodule--helper config --unset\"' '\n> \t(cd super &&\n> -\t\tgit submodule--helper config --unset submodule.submodule.url &&\n> -\t\tgit submodule--helper config submodule.submodule.url >actual &&\n> +\t\ttest-tool submodule config-unset submodule.submodule.url &&\n> +\t\ttest-tool submodule config-list submodule.submodule.url >actual &&\n> \t\ttest_must_be_empty actual\n> \t)\n> '\n\nIn a few of the test names, we still say 'do X with \"submodule--helper\nconfig [...]\"'. I think just we can drop 'with \"submodule--helper config\n[...]\"', e.g.\n\n test_expect_sucess 'unsetting submodules config from the working tree'\n\n> @@ -157,8 +157,8 @@ test_expect_success 'unsetting submodules config from the working tree with \"sub\n> test_expect_success 'writing submodules config with \"submodule--helper config\"' '\n> \t(cd super &&\n> \t\techo \"new_url\" >expect &&\n> -\t\tgit submodule--helper config submodule.submodule.url \"new_url\" &&\n> -\t\tgit submodule--helper config submodule.submodule.url >actual &&\n> +\t\ttest-tool submodule config-set submodule.submodule.url \"new_url\" &&\n> +\t\ttest-tool submodule config-list submodule.submodule.url >actual &&\n> \t\ttest_cmp expect actual\n> \t)\n> '\n> @@ -167,14 +167,14 @@ test_expect_success 'overwriting unstaged submodules config with \"submodule--hel\n> \ttest_when_finished \"git -C super checkout .gitmodules\" &&\n> \t(cd super &&\n> \t\techo \"newer_url\" >expect &&\n> -\t\tgit submodule--helper config submodule.submodule.url \"newer_url\" &&\n> -\t\tgit submodule--helper config submodule.submodule.url >actual &&\n> +\t\ttest-tool submodule config-set submodule.submodule.url \"newer_url\" &&\n> +\t\ttest-tool submodule config-list submodule.submodule.url >actual &&\n> \t\ttest_cmp expect actual\n> \t)\n> '\n> \n> test_expect_success 'writeable .gitmodules when it is in the working tree' '\n> -\tgit -C super submodule--helper config --check-writeable\n> +\ttest-tool -C super submodule config-writeable\n> '\n> \n> test_expect_success 'writeable .gitmodules when it is nowhere in the repository' '\n> @@ -183,7 +183,7 @@ test_expect_success 'writeable .gitmodules when it is nowhere in the repository'\n> \t(cd super &&\n> \t\tgit rm .gitmodules &&\n> \t\tgit commit -m \"remove .gitmodules from the current branch\" &&\n> -\t\tgit submodule--helper config --check-writeable\n> +\t\ttest-tool submodule config-writeable\n> \t)\n> '\n\nThe value of the writeable/unwriteable tests feels quite low to me, but\nI am ok to keep them just to be safe.\n\n> \n> @@ -191,7 +191,7 @@ test_expect_success 'non-writeable .gitmodules when it is in the index but not i\n> \ttest_when_finished \"git -C super checkout .gitmodules\" &&\n> \t(cd super &&\n> \t\trm -f .gitmodules &&\n> -\t\ttest_must_fail git submodule--helper config --check-writeable\n> +\t\ttest_must_fail test-tool submodule config-writeable\n> \t)\n> '\n> \n> @@ -200,7 +200,7 @@ test_expect_success 'non-writeable .gitmodules when it is in the current branch\n> \ttest_when_finished \"git -C super reset --hard $ORIG\" &&\n> \t(cd super &&\n> \t\tgit rm .gitmodules &&\n> -\t\ttest_must_fail git submodule--helper config --check-writeable\n> +\t\ttest_must_fail test-tool submodule config-writeable\n> \t)\n> '\n> \n> @@ -208,11 +208,11 @@ test_expect_success 'reading submodules config from the index when .gitmodules i\n> \tORIG=$(git -C super rev-parse HEAD) &&\n> \ttest_when_finished \"git -C super reset --hard $ORIG\" &&\n> \t(cd super &&\n> -\t\tgit submodule--helper config submodule.submodule.url \"staged_url\" &&\n> +\t\ttest-tool submodule config-set submodule.submodule.url \"staged_url\" &&\n> \t\tgit add .gitmodules &&\n> \t\trm -f .gitmodules &&\n> \t\techo \"staged_url\" >expect &&\n> -\t\tgit submodule--helper config submodule.submodule.url >actual &&\n> +\t\ttest-tool submodule config-list submodule.submodule.url >actual &&\n> \t\ttest_cmp expect actual\n> \t)\n> '\n> @@ -223,7 +223,7 @@ test_expect_success 'reading submodules config from the current branch when .git\n> \t(cd super &&\n> \t\tgit rm .gitmodules &&\n> \t\techo \"../submodule\" >expect &&\n> -\t\tgit submodule--helper config submodule.submodule.url >actual &&\n> +\t\ttest-tool submodule config-list submodule.submodule.url >actual &&\n> \t\ttest_cmp expect actual\n> \t)\n> '\n> diff --git a/t/t7418-submodule-sparse-gitmodules.sh b/t/t7418-submodule-sparse-gitmodules.sh\n> index d5874200fdc..dde11ecce80 100755\n> --- a/t/t7418-submodule-sparse-gitmodules.sh\n> +++ b/t/t7418-submodule-sparse-gitmodules.sh\n> @@ -50,12 +50,12 @@ test_expect_success 'sparse checkout setup which hides .gitmodules' '\n> \n> test_expect_success 'reading gitmodules config file when it is not checked out' '\n> \techo \"../submodule\" >expect &&\n> -\tgit -C super submodule--helper config submodule.submodule.url >actual &&\n> +\ttest-tool -C super submodule config-list submodule.submodule.url >actual &&\n> \ttest_cmp expect actual\n> '\n> \n> test_expect_success 'not writing gitmodules config file when it is not checked out' '\n> -\ttest_must_fail git -C super submodule--helper config submodule.submodule.url newurl &&\n> +\ttest_must_fail test-tool -C super submodule config-set submodule.submodule.url newurl &&\n> \ttest_path_is_missing super/.gitmodules\n> '\n> \n> -- \n> 2.38.0.1280.g8136eb6fab2\n"},{"id":"466456","messageId":"kl6lcza3oemf.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"58734","inReplyTo":"patch-2.8-cda36b5b6e0-20221102T074148Z-avarab@gmail.com","subject":"Re: [PATCH 2/8] submodule tests: add tests for top-level flag output","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-11-03T22:30:16Z","receivedAt":"2022-11-03T22:30:23Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Exhaustively test for how combining various \"mixed-level\" \"git\n> submodule\" option works. \"Mixed-level\" here means options that are\n> accepted by a mixture of the top-level \"submodule\" command, and\n> e.g. the \"status\" sub-command.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> t/t7400-submodule-basic.sh | 10 +++\n> t/t7422-submodule-output.sh | 169 ++++++++++++++++++++++++++++++++++++\n> 2 files changed, 179 insertions(+)\n> create mode 100755 t/t7422-submodule-output.sh\n>\n> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\n> index a989aafaf57..eae6a46ef3d 100755\n> --- a/t/t7400-submodule-basic.sh\n> +++ b/t/t7400-submodule-basic.sh\n> @@ -579,6 +579,16 @@ test_expect_success 'status should be \"modified\" after submodule commit' '\n> \tgrep \"^+$rev2\" list\n> '\n> \n> +test_expect_success '\"submodule --cached\" command forms should be identical' '\n> +\tgit submodule status --cached >expect &&\n> +\n> +\tgit submodule --cached >actual &&\n> +\ttest_cmp expect actual &&\n> +\n> +\tgit submodule --cached status >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_expect_success 'the --cached sha1 should be rev1' '\n> \tgit submodule --cached status >list &&\n> \tgrep \"^+$rev1\" list\n> diff --git a/t/t7422-submodule-output.sh b/t/t7422-submodule-output.sh\n> new file mode 100755\n> index 00000000000..3ac56c23f72\n> --- /dev/null\n> +++ b/t/t7422-submodule-output.sh\n> @@ -0,0 +1,169 @@\n> +#!/bin/sh\n> +\n> +test_description='submodule --cached, --quiet etc. output'\n> +\n> +. ./test-lib.sh\n> +. \"$TEST_DIRECTORY\"/lib-t3100.sh\n> +\n> +setup_sub () {\n> +\tlocal d=\"$1\" &&\n> +\tshift &&\n> +\tgit $@ clone . \"$d\" &&\n> +\tgit $@ submodule add ./\"$d\"\n> +}\n> +\n> +normalize_status () {\n> +\tsed -e 's/-g[0-9a-f]*/-gHASH/'\n> +}\n> +\n> +test_expect_success 'setup' '\n> +\ttest_commit A &&\n> +\ttest_commit B &&\n> +\tsetup_sub S &&\n> +\tsetup_sub S.D &&\n> +\tsetup_sub S.C &&\n> +\tsetup_sub S.C.D &&\n> +\tsetup_sub X &&\n> +\tgit add S* &&\n> +\ttest_commit C &&\n> +\n> +\t# recursive in X/\n> +\tgit -C X pull &&\n> +\tGIT_ALLOW_PROTOCOL=file git -C X submodule update --init &&\n> +\n> +\t# dirty p\n> +\tfor d in S.D X/S.D\n> +\tdo\n> +\t\techo dirty >S.D/A.t || return 1\n> +\tdone &&\n\nI think this was meant to be replaced by the loop labelled # dirty ?\nThis doesn't look like it will work, e.g. $d isn't used.\n\n> +\n> +\t# commit (for --cached)\n> +\tfor d in S.C* X/S.C*\n> +\tdo\n> +\t\tgit -C \"$d\" reset --hard A || return 1\n> +\tdone &&\n> +\n> +\t# dirty\n> +\tfor d in S*.D X/S*.D\n> +\tdo\n> +\t\techo dirty >\"$d/C2.t\" || return 1\n> +\tdone &&\n> +\n> +\tfor ref in A B C\n> +\tdo\n> +\t\t# Not different with SHA-1 and SHA-256, just (ab)usign\n> +\t\t# test_oid_cache as a variable bag to avoid using\n> +\t\t# $(git rev-parse ...).\n> +\t\toid=$(git rev-parse $ref) &&\n> +\t\ttest_oid_cache <<-EOF || return 1\n> +\t\t$ref sha1:$oid\n> +\t\t$ref sha256:$oid\n> +\t\tEOF\n> +\tdone\n> +'\n> +\n> +for opts in \"\" \"status\"\n> +do\n> +\ttest_expect_success \"git submodule $opts\" '\n> +\t\tsed -e \"s/^>//\" >expect <<-EOF &&\n> +\t\t> $(test_oid B) S (B)\n> +\t\t>+$(test_oid A) S.C (A)\n> +\t\t>+$(test_oid A) S.C.D (A)\n> +\t\t> $(test_oid B) S.D (B)\n> +\t\t>+$(test_oid C) X (C)\n> +\t\tEOF\n> +\t\tgit submodule $opts >actual.raw &&\n> +\t\tnormalize_status actual &&\n> +\t\ttest_cmp expect actual\n> +\t'\n> +done\n> +\n> +for opts in \\\n> +\t\"status --recursive\"\n\nWe don't test \"--recursive status\" or \"--recursive\" because we don't\nparse the top level \"--recursive\" flag, right?\n\n> +do\n> +\ttest_expect_success \"git submodule $opts\" '\n> +\t\tsed -e \"s/^>//\" >expect <<-EOF &&\n> +\t\t> $(test_oid B) S (B)\n> +\t\t>+$(test_oid A) S.C (A)\n> +\t\t>+$(test_oid A) S.C.D (A)\n> +\t\t> $(test_oid B) S.D (B)\n> +\t\t>+$(test_oid C) X (C)\n> +\t\t> $(test_oid B) X/S (B)\n> +\t\t>+$(test_oid A) X/S.C (A)\n> +\t\t>+$(test_oid A) X/S.C.D (A)\n> +\t\t> $(test_oid B) X/S.D (B)\n> +\t\t> $(test_oid B) X/X (B)\n> +\t\tEOF\n> +\t\tgit submodule $opts >actual.raw &&\n> +\t\tnormalize_status actual &&\n> +\t\ttest_cmp expect actual\n> +\t'\n> +done\n> +\n> +for opts in \\\n> +\t\"--quiet\" \\\n> +\t\"--quiet status\" \\\n> +\t\"status --quiet\"\n> +do\n> +\ttest_expect_success \"git submodule $opts\" '\n> +\t\tgit submodule $opts >out &&\n> +\t\ttest_must_be_empty out\n> +\t'\n> +done\n> +\n> +for opts in \\\n> +\t\"--cached\" \\\n> +\t\"--cached status\" \\\n> +\t\"status --cached\"\n> +do\n> +\ttest_expect_success \"git submodule $opts\" '\n> +\t\tsed -e \"s/^>//\" >expect <<-EOF &&\n> +\t\t> $(test_oid B) S (B)\n> +\t\t>+$(test_oid B) S.C (B)\n> +\t\t>+$(test_oid B) S.C.D (B)\n> +\t\t> $(test_oid B) S.D (B)\n> +\t\t>+$(test_oid B) X (B)\n> +\t\tEOF\n> +\t\tgit submodule $opts >actual.raw &&\n> +\t\tnormalize_status actual &&\n> +\t\ttest_cmp expect actual\n> +\t'\n> +done\n> +\n> +for opts in \\\n> +\t\"--cached --quiet\" \\\n> +\t\"--cached --quiet status\" \\\n> +\t\"--cached status --quiet\" \\\n> +\t\"--quiet status --cached\" \\\n> +\t\"status --cached --quiet\"\n> +do\n> +\ttest_expect_success \"git submodule $opts\" '\n> +\t\tgit submodule $opts >out &&\n> +\t\ttest_must_be_empty out\n> +\t'\n> +done\n> +\n> +for opts in \\\n> +\t\"status --cached --recursive\" \\\n> +\t\"--cached status --recursive\"\n> +do\n> +\ttest_expect_success \"git submodule $opts\" '\n> +\t\tsed -e \"s/^>//\" >expect <<-EOF &&\n> +\t\t> $(test_oid B) S (B)\n> +\t\t>+$(test_oid B) S.C (B)\n> +\t\t>+$(test_oid B) S.C.D (B)\n> +\t\t> $(test_oid B) S.D (B)\n> +\t\t>+$(test_oid B) X (B)\n> +\t\t> $(test_oid B) X/S (B)\n> +\t\t>+$(test_oid B) X/S.C (B)\n> +\t\t>+$(test_oid B) X/S.C.D (B)\n> +\t\t> $(test_oid B) X/S.D (B)\n> +\t\t> $(test_oid B) X/X (B)\n> +\t\tEOF\n> +\t\tgit submodule $opts >actual.raw &&\n> +\t\tnormalize_status actual &&\n> +\t\ttest_cmp expect actual\n> +\t'\n> +done\n> +\n> +test_done\n> -- \n> 2.38.0.1280.g8136eb6fab2\n"},{"id":"466457","messageId":"kl6la657odjz.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"58734","inReplyTo":"patch-5.8-2b8afd73b9b-20221102T074148Z-avarab@gmail.com","subject":"Re: [PATCH 5/8] submodule API & \"absorbgitdirs\": remove \"----recursive\" option","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-11-03T22:53:20Z","receivedAt":"2022-11-03T22:53:28Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Remove the \"----recursive\" option to \"git submodule--helper\n> absorbgitdirs\" (yes, with 4 dashes, not 2).\n\no.O\n\nAt least this makes it pretty easy to grep for usage, and it makes sense\nthat we've never used it (otherwise this would've been caught).\n\n> diff --git a/submodule.c b/submodule.c\n> index fe1e3f03905..8fa2ad457b2 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -2332,8 +2331,7 @@ static void absorb_git_dir_into_superproject_recurse(const char *path)\n> * having its git directory within the working tree to the git dir nested\n> * in its superprojects git dir under modules/.\n> */\n> -void absorb_git_dir_into_superproject(const char *path,\n> -\t\t\t\t unsigned flags)\n> +void absorb_git_dir_into_superproject(const char *path)\n> {\n> \tint err_code;\n> \tconst char *sub_git_dir;\n> @@ -2382,12 +2380,7 @@ void absorb_git_dir_into_superproject(const char *path,\n> \t}\n> \tstrbuf_release(&gitdir);\n> \n> -\tif (flags & ABSORB_GITDIR_RECURSE_SUBMODULES) {\n> -\t\tif (flags & ~ABSORB_GITDIR_RECURSE_SUBMODULES)\n> -\t\t\tBUG(\"we don't know how to pass the flags down?\");\n> -\n> -\t\tabsorb_git_dir_into_superproject_recurse(path);\n> -\t}\n> +\tabsorb_git_dir_into_superproject_recurse(path);\n> }\n\nMaybe I'm misreading, but I don't follow this change.\n\nBefore, we recursed into the submodule only if the\nABSORB_GITDIR_RECURSE_SUBMODULES flag is set (which we now know is\nnever), but now we unconditionally recurse into the submodule.\n\nWouldn't it be more accurate to get rid of this recursing behavior\naltogether? i.e. drop the previous patch and delete that code when we\ndelete this conditional.\n"},{"id":"466461","messageId":"kl6l7d0bobt0.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"58734","inReplyTo":"patch-8.8-105853cd358-20221102T074148Z-avarab@gmail.com","subject":"Re: [PATCH 8/8] submodule--helper: use OPT_SUBCOMMAND() API","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-11-03T23:31:07Z","receivedAt":"2022-11-03T23:31:14Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Have the cmd_submodule__helper() use the OPT_SUBCOMMAND() API\n> introduced in fa83cc834da (parse-options: add support for parsing\n> subcommands, 2022-08-19).\n>\n> This is only a marginal reduction in line count, but once we start\n> unifying this with a yet-to-be-added \"builtin/submodule.c\" it'll be\n> much easier to reason about those changes, as they'll both use\n> OPT_SUBCOMMAND().\n\nEven if nothing else, this is a nice standardization change :)\n\n> We don't need to worry about \"argv[0]\" being NULL in the die() because\n> we'd have errored out in parse_options() as we're not using\n> \"PARSE_OPT_SUBCOMMAND_OPTIONAL\".\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> builtin/submodule--helper.c | 78 ++++++++++++++++++-------------------\n> git.c | 2 +-\n> 2 files changed, 39 insertions(+), 41 deletions(-)\n>\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index 2012ad31d7f..0bc25dcf5ae 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -3350,47 +3350,45 @@ static int module_add(int argc, const char **argv, const char *prefix)\n> \treturn ret;\n> }\n> \n> -#define SUPPORT_SUPER_PREFIX (1<<0)\n> -\n> -struct cmd_struct {\n> -\tconst char *cmd;\n> -\tint (*fn)(int, const char **, const char *);\n> -\tunsigned option;\n> -};\n> -\n> -static struct cmd_struct commands[] = {\n> -\t{\"clone\", module_clone, SUPPORT_SUPER_PREFIX},\n> -\t{\"add\", module_add, 0},\n> -\t{\"update\", module_update, SUPPORT_SUPER_PREFIX},\n> -\t{\"foreach\", module_foreach, 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, 0},\n> -\t{\"push-check\", push_check, 0},\n> -\t{\"absorbgitdirs\", absorb_git_dirs, SUPPORT_SUPER_PREFIX},\n> -\t{\"set-url\", module_set_url, 0},\n> -\t{\"set-branch\", module_set_branch, 0},\n> -\t{\"create-branch\", module_create_branch, 0},\n> -};\n> -\n> int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\n> {\n> -\tint i;\n> -\tif (argc < 2 || !strcmp(argv[1], \"-h\"))\n> -\t\tusage(\"git submodule--helper \");\n> -\n> -\tfor (i = 0; i < ARRAY_SIZE(commands); i++) {\n> -\t\tif (!strcmp(argv[1], commands[i].cmd)) {\n> -\t\t\tif (get_super_prefix() &&\n> -\t\t\t !(commands[i].option & SUPPORT_SUPER_PREFIX))\n> -\t\t\t\tdie(_(\"%s doesn't support --super-prefix\"),\n> -\t\t\t\t commands[i].cmd);\n> -\t\t\treturn commands[i].fn(argc - 1, argv + 1, prefix);\n> -\t\t}\n> -\t}\n> +\tconst char *cmd = argv[0];\n> +\tconst char *subcmd;\n> +\tparse_opt_subcommand_fn *fn = NULL;\n> +\tconst char *const usage[] = {\n> +\t\tN_(\"git submodule--helper \"),\n> +\t\tNULL\n> +\t};\n> +\tstruct option options[] = {\n> +\t\tOPT_SUBCOMMAND(\"clone\", &fn, module_clone),\n> +\t\tOPT_SUBCOMMAND(\"add\", &fn, module_add),\n> +\t\tOPT_SUBCOMMAND(\"update\", &fn, module_update),\n> +\t\tOPT_SUBCOMMAND(\"foreach\", &fn, module_foreach),\n> +\t\tOPT_SUBCOMMAND(\"init\", &fn, module_init),\n> +\t\tOPT_SUBCOMMAND(\"status\", &fn, module_status),\n> +\t\tOPT_SUBCOMMAND(\"sync\", &fn, module_sync),\n> +\t\tOPT_SUBCOMMAND(\"deinit\", &fn, module_deinit),\n> +\t\tOPT_SUBCOMMAND(\"summary\", &fn, module_summary),\n> +\t\tOPT_SUBCOMMAND(\"push-check\", &fn, push_check),\n> +\t\tOPT_SUBCOMMAND(\"absorbgitdirs\", &fn, absorb_git_dirs),\n> +\t\tOPT_SUBCOMMAND(\"set-url\", &fn, module_set_url),\n> +\t\tOPT_SUBCOMMAND(\"set-branch\", &fn, module_set_branch),\n> +\t\tOPT_SUBCOMMAND(\"create-branch\", &fn, module_create_branch),\n> +\t\tOPT_END()\n> +\t};\n> +\targc = parse_options(argc, argv, prefix, options, usage, 0);\n> +\tsubcmd = argv[0];\n> +\n> +\tif (strcmp(subcmd, \"clone\") && strcmp(subcmd, \"update\") &&\n> +\t strcmp(subcmd, \"foreach\") && strcmp(subcmd, \"status\") &&\n> +\t strcmp(subcmd, \"sync\") && strcmp(subcmd, \"absorbgitdirs\") &&\n> +\t get_super_prefix())\n> +\t\t/*\n> +\t\t * xstrfmt() rather than \"%s %s\" to keep the translated\n> +\t\t * string identical to git.c's.\n> +\t\t */\n> +\t\tdie(_(\"%s doesn't support --super-prefix\"),\n> +\t\t xstrfmt(\"'%s %s'\", cmd, subcmd));\n\nFYI I'm preparing a series to get rid of the SUPPORT_SUPER_PREFIX checks\nin both the top level and in submodule--helper (i.e. revisiting my\ncomplaints from [1]), but I haven't sent it out yet because I haven't\nfound the right way to motivate that change. Feel free to chime in on\nthat series when it comes out.\n\n[1] https://lore.kernel.org/git/20220630221147.45689-5-chooglen@google.com/\n\n> -\tdie(_(\"'%s' is not a valid submodule--helper \"\n> -\t \"subcommand\"), argv[1]);\n> +\treturn fn(argc, argv, prefix);\n> }\n> diff --git a/git.c b/git.c\n> index ee7758dcb0e..fb69e605912 100644\n> --- a/git.c\n> +++ b/git.c\n> @@ -610,7 +610,7 @@ static struct cmd_struct commands[] = {\n> \t{ \"stash\", cmd_stash, RUN_SETUP | NEED_WORK_TREE },\n> \t{ \"status\", cmd_status, RUN_SETUP | NEED_WORK_TREE },\n> \t{ \"stripspace\", cmd_stripspace },\n> -\t{ \"submodule--helper\", cmd_submodule__helper, RUN_SETUP | SUPPORT_SUPER_PREFIX | NO_PARSEOPT },\n> +\t{ \"submodule--helper\", cmd_submodule__helper, RUN_SETUP | SUPPORT_SUPER_PREFIX },\n> \t{ \"switch\", cmd_switch, RUN_SETUP | NEED_WORK_TREE },\n> \t{ \"symbolic-ref\", cmd_symbolic_ref, RUN_SETUP },\n> \t{ \"tag\", cmd_tag, RUN_SETUP | DELAY_PAGER_CONFIG },\n> -- \n> 2.38.0.1280.g8136eb6fab2\n"},{"id":"466470","messageId":"221104.86wn8bzeus.gmgdl@evledraar.gmail.com","threadId":"58734","inReplyTo":"kl6l7d0bobt0.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH 8/8] submodule--helper: use OPT_SUBCOMMAND() API","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-04T01:22:54Z","receivedAt":"2022-11-04T01:29:54Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Nov 03 2022, Glen Choo wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n> [...]\n>> +\tstruct option options[] = {\n>> +\t\tOPT_SUBCOMMAND(\"clone\", &fn, module_clone),\n>> +\t\tOPT_SUBCOMMAND(\"add\", &fn, module_add),\n>> +\t\tOPT_SUBCOMMAND(\"update\", &fn, module_update),\n>> +\t\tOPT_SUBCOMMAND(\"foreach\", &fn, module_foreach),\n>> +\t\tOPT_SUBCOMMAND(\"init\", &fn, module_init),\n>> +\t\tOPT_SUBCOMMAND(\"status\", &fn, module_status),\n>> +\t\tOPT_SUBCOMMAND(\"sync\", &fn, module_sync),\n>> +\t\tOPT_SUBCOMMAND(\"deinit\", &fn, module_deinit),\n>> +\t\tOPT_SUBCOMMAND(\"summary\", &fn, module_summary),\n>> +\t\tOPT_SUBCOMMAND(\"push-check\", &fn, push_check),\n>> +\t\tOPT_SUBCOMMAND(\"absorbgitdirs\", &fn, absorb_git_dirs),\n>> +\t\tOPT_SUBCOMMAND(\"set-url\", &fn, module_set_url),\n>> +\t\tOPT_SUBCOMMAND(\"set-branch\", &fn, module_set_branch),\n>> +\t\tOPT_SUBCOMMAND(\"create-branch\", &fn, module_create_branch),\n>> +\t\tOPT_END()\n>> +\t};\n>> +\targc = parse_options(argc, argv, prefix, options, usage, 0);\n>> +\tsubcmd = argv[0];\n>> +\n>> +\tif (strcmp(subcmd, \"clone\") && strcmp(subcmd, \"update\") &&\n>> +\t strcmp(subcmd, \"foreach\") && strcmp(subcmd, \"status\") &&\n>> +\t strcmp(subcmd, \"sync\") && strcmp(subcmd, \"absorbgitdirs\") &&\n>> +\t get_super_prefix())\n>> +\t\t/*\n>> +\t\t * xstrfmt() rather than \"%s %s\" to keep the translated\n>> +\t\t * string identical to git.c's.\n>> +\t\t */\n>> +\t\tdie(_(\"%s doesn't support --super-prefix\"),\n>> +\t\t xstrfmt(\"'%s %s'\", cmd, subcmd));\n>\n> FYI I'm preparing a series to get rid of the SUPPORT_SUPER_PREFIX checks\n> in both the top level and in submodule--helper (i.e. revisiting my\n> complaints from [1]), but I haven't sent it out yet because I haven't\n> found the right way to motivate that change. Feel free to chime in on\n> that series when it comes out.\n>\n> [1] https://lore.kernel.org/git/20220630221147.45689-5-chooglen@google.com/\n\nMaybe you have different plans, but keep at the WIP re-roll of what I\nhave after this, I've also removed all of that.\n\nBasically, ending up with:\n\t\n\t--- a/builtin.h\n\t+++ b/builtin.h\n\t@@ -224,7 +224,14 @@ int cmd_sparse_checkout(int argc, const char **argv, const char *prefix);\n\t int cmd_status(int argc, const char **argv, const char *prefix);\n\t int cmd_stash(int argc, const char **argv, const char *prefix);\n\t int cmd_stripspace(int argc, const char **argv, const char *prefix);\n\t+int cmd_submodule(int argc, const char **argv, const char *prefix);\n\t int cmd_submodule__helper(int argc, const char **argv, const char *prefix);\n\t+int cmd_submodule__helper_clone(int argc, const char **argv, const char *prefix);\n\t+int cmd_submodule_absorbgitdirs(int argc, const char **argv, const char *prefix);\n\t+int cmd_submodule_foreach(int argc, const char **argv, const char *prefix);\n\t+int cmd_submodule_status(int argc, const char **argv, const char *prefix);\n\t+int cmd_submodule_sync(int argc, const char **argv, const char *prefix);\n\t+int cmd_submodule_update(int argc, const char **argv, const char *prefix);\n\t int cmd_switch(int argc, const char **argv, const char *prefix);\n\t int cmd_symbolic_ref(int argc, const char **argv, const char *prefix);\n\t int cmd_tag(int argc, const char **argv, const char *prefix);\n\nAnd changes like:\n\t\n\t@@ -366,7 +365,7 @@ static void runcommand_in_submodule_cb(const struct cache_entry *list_item,\n\t \n\t strvec_pushl(&cpr.args, \"--super-prefix\", NULL);\n\t strvec_pushf(&cpr.args, \"%s/\", displaypath);\n\t- strvec_pushl(&cpr.args, \"submodule--helper\", \"foreach\", \"--recursive\",\n\t+ strvec_pushl(&cpr.args, \"submodule--helper-foreach\", \"--recursive\",\n\t NULL);\n\t \n\t if (info->quiet)\n\nI.e. when we call \"foreach\" we dispatch to cmd_submodule_foreach(), but\nwhen \"foreach\" needs to recurse it doesn't invoke a \"git submodule\nforeach\", instead it invokes \"git submodule--helper-foreach\".\n\nThe reason for doing that is that we can't promote the helper to a\nbuilt-in without also leaking implementation detail that we support the\nnow internal-only --super-prefix in the command itself.\n\nSo this code becomes:\n\t\n\t@@ -3352,43 +3304,17 @@ static int module_add(int argc, const char **argv, const char *prefix)\n\t \n\t int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\n\t {\n\t- const char *cmd = argv[0];\n\t- const char *subcmd;\n\t parse_opt_subcommand_fn *fn = NULL;\n\t const char *const usage[] = {\n\t N_(\"git submodule--helper \"),\n\t NULL\n\t };\n\t struct option options[] = {\n\t- OPT_SUBCOMMAND(\"clone\", &fn, module_clone),\n\t- OPT_SUBCOMMAND(\"add\", &fn, module_add),\n\t- OPT_SUBCOMMAND(\"update\", &fn, module_update),\n\t- OPT_SUBCOMMAND(\"foreach\", &fn, module_foreach),\n\t- OPT_SUBCOMMAND(\"init\", &fn, module_init),\n\t- OPT_SUBCOMMAND(\"status\", &fn, module_status),\n\t- OPT_SUBCOMMAND(\"sync\", &fn, module_sync),\n\t- OPT_SUBCOMMAND(\"deinit\", &fn, module_deinit),\n\t- OPT_SUBCOMMAND(\"summary\", &fn, module_summary),\n\t- OPT_SUBCOMMAND(\"push-check\", &fn, push_check),\n\t- OPT_SUBCOMMAND(\"absorbgitdirs\", &fn, absorb_git_dirs),\n\t- OPT_SUBCOMMAND(\"set-url\", &fn, module_set_url),\n\t- OPT_SUBCOMMAND(\"set-branch\", &fn, module_set_branch),\n\t- OPT_SUBCOMMAND(\"create-branch\", &fn, module_create_branch),\n\t+ OPT_SUBCOMMAND(\"push-check\", &fn, cmd_submodule_push_check),\n\t+ OPT_SUBCOMMAND(\"create-branch\", &fn, cmd_submodule_create_branch),\n\t OPT_END()\n\t };\n\t argc = parse_options(argc, argv, prefix, options, usage, 0);\n\t- subcmd = argv[0];\n\t-\n\t- if (strcmp(subcmd, \"clone\") && strcmp(subcmd, \"update\") &&\n\t- strcmp(subcmd, \"foreach\") && strcmp(subcmd, \"status\") &&\n\t- strcmp(subcmd, \"sync\") && strcmp(subcmd, \"absorbgitdirs\") &&\n\t- get_super_prefix())\n\t- /*\n\t- * xstrfmt() rather than \"%s %s\" to keep the translated\n\t- * string identical to git.c's.\n\t- */\n\t- die(_(\"%s doesn't support --super-prefix\"),\n\t- xstrfmt(\"'%s %s'\", cmd, subcmd));\n\t \n\t return fn(argc, argv, prefix);\n\t }\n\nI.e. for all the \"super-prefix\" ones we dispatch directly (and maybe I\nshould just do that too for those two stragglers).\n\nIt's ugly, but it's only ugly on the inside, but if you can think of a\nbetter way to do it...\n"},{"id":"466474","messageId":"221104.86fsezze6p.gmgdl@evledraar.gmail.com","threadId":"58734","inReplyTo":"kl6la657odjz.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH 5/8] submodule API & \"absorbgitdirs\": remove \"----recursive\" option","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-04T01:42:59Z","receivedAt":"2022-11-04T01:44:22Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Nov 03 2022, Glen Choo wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> Remove the \"----recursive\" option to \"git submodule--helper\n>> absorbgitdirs\" (yes, with 4 dashes, not 2).\n>\n> o.O\n>\n> At least this makes it pretty easy to grep for usage, and it makes sense\n> that we've never used it (otherwise this would've been caught).\n>\n>> diff --git a/submodule.c b/submodule.c\n>> index fe1e3f03905..8fa2ad457b2 100644\n>> --- a/submodule.c\n>> +++ b/submodule.c\n>> @@ -2332,8 +2331,7 @@ static void absorb_git_dir_into_superproject_recurse(const char *path)\n>> * having its git directory within the working tree to the git dir nested\n>> * in its superprojects git dir under modules/.\n>> */\n>> -void absorb_git_dir_into_superproject(const char *path,\n>> -\t\t\t\t unsigned flags)\n>> +void absorb_git_dir_into_superproject(const char *path)\n>> {\n>> \tint err_code;\n>> \tconst char *sub_git_dir;\n>> @@ -2382,12 +2380,7 @@ void absorb_git_dir_into_superproject(const char *path,\n>> \t}\n>> \tstrbuf_release(&gitdir);\n>> \n>> -\tif (flags & ABSORB_GITDIR_RECURSE_SUBMODULES) {\n>> -\t\tif (flags & ~ABSORB_GITDIR_RECURSE_SUBMODULES)\n>> -\t\t\tBUG(\"we don't know how to pass the flags down?\");\n>> -\n>> -\t\tabsorb_git_dir_into_superproject_recurse(path);\n>> -\t}\n>> +\tabsorb_git_dir_into_superproject_recurse(path);\n>> }\n>\n> Maybe I'm misreading, but I don't follow this change.\n>\n> Before, we recursed into the submodule only if the\n> ABSORB_GITDIR_RECURSE_SUBMODULES flag is set (which we now know is\n> never), but now we unconditionally recurse into the submodule.\n\nNo, it's always set. I.e. ----recursive did nothing, but the default was\nto always set ABSORB_GITDIR_RECURSE_SUBMODULES, so it was never not-set\n(and there was no --no---recursive user).\n\nSo we should be unconditionally going on this recursive path.\n\n"},{"id":"466512","messageId":"SA1PR09MB8703677F86916FC099E2878F903B9@SA1PR09MB8703.namprd09.prod.outlook.com","threadId":"58734","inReplyTo":"221104.86fsezze6p.gmgdl@evledraar.gmail.com","subject":"FW: [PATCH 5/8] submodule API & \"absorbgitdirs\": remove \"----recursive\" option","fromName":"Simpson, Phyllis","fromEmail":"psimpson@isdh.in.gov","sentAt":"2022-11-04T13:07:25Z","receivedAt":"2022-11-04T13:07:31Z","isPatch":true,"sender":{"key":"psimpson@isdh.in.gov","avatar":null},"body":"Hello.\n\nI do not wish to get these messages any longer.\n\nThank you.\n\nPhyllis \n\nP Simpson  |  Application Systems Analyst Programmer - Int\nOffice of Technology & Cybersecurity\noffice: 317-233-8477\npsimpson@health.in.gov\nhealth.in.gov\n   \n\n\n\n\n\n-----Original Message-----\nFrom: Ævar Arnfjörð Bjarmason \nSent: Thursday, November 3, 2022 9:43 PM\nTo: Glen Choo \nCc: git@vger.kernel.org; Junio C Hamano ; Emily Shaffer \nSubject: Re: [PATCH 5/8] submodule API & \"absorbgitdirs\": remove \"----recursive\" option\n\n**** This is an EXTERNAL email. Exercise caution. DO NOT open attachments or click links from unknown senders or unexpected email. **** ________________________________\n\nOn Thu, Nov 03 2022, Glen Choo wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> Remove the \"----recursive\" option to \"git submodule--helper \n>> absorbgitdirs\" (yes, with 4 dashes, not 2).\n>\n> o.O\n>\n> At least this makes it pretty easy to grep for usage, and it makes \n> sense that we've never used it (otherwise this would've been caught).\n>\n>> diff --git a/submodule.c b/submodule.c index fe1e3f03905..8fa2ad457b2 \n>> 100644\n>> --- a/submodule.c\n>> +++ b/submodule.c\n>> @@ -2332,8 +2331,7 @@ static void absorb_git_dir_into_superproject_recurse(const char *path)\n>> * having its git directory within the working tree to the git dir nested\n>> * in its superprojects git dir under modules/.\n>> */\n>> -void absorb_git_dir_into_superproject(const char *path,\n>> - unsigned flags)\n>> +void absorb_git_dir_into_superproject(const char *path)\n>> {\n>> int err_code;\n>> const char *sub_git_dir;\n>> @@ -2382,12 +2380,7 @@ void absorb_git_dir_into_superproject(const char *path,\n>> }\n>> strbuf_release(&gitdir);\n>>\n>> - if (flags & ABSORB_GITDIR_RECURSE_SUBMODULES) {\n>> - if (flags & ~ABSORB_GITDIR_RECURSE_SUBMODULES)\n>> - BUG(\"we don't know how to pass the flags down?\");\n>> -\n>> - absorb_git_dir_into_superproject_recurse(path);\n>> - }\n>> + absorb_git_dir_into_superproject_recurse(path);\n>> }\n>\n> Maybe I'm misreading, but I don't follow this change.\n>\n> Before, we recursed into the submodule only if the \n> ABSORB_GITDIR_RECURSE_SUBMODULES flag is set (which we now know is \n> never), but now we unconditionally recurse into the submodule.\n\nNo, it's always set. I.e. ----recursive did nothing, but the default was to always set ABSORB_GITDIR_RECURSE_SUBMODULES, so it was never not-set (and there was no --no---recursive user).\n\nSo we should be unconditionally going on this recursive path.\n"},{"id":"466544","messageId":"kl6l8rkqy7no.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"58734","inReplyTo":"221104.86wn8bzeus.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 8/8] submodule--helper: use OPT_SUBCOMMAND() API","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-11-04T17:02:51Z","receivedAt":"2022-11-04T17:02:58Z","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, Nov 03 2022, Glen Choo wrote:\n>\n>> Ævar Arnfjörð Bjarmason writes:\n>> [...]\n>>> +\tstruct option options[] = {\n>>> +\t\tOPT_SUBCOMMAND(\"clone\", &fn, module_clone),\n>>> +\t\tOPT_SUBCOMMAND(\"add\", &fn, module_add),\n>>> +\t\tOPT_SUBCOMMAND(\"update\", &fn, module_update),\n>>> +\t\tOPT_SUBCOMMAND(\"foreach\", &fn, module_foreach),\n>>> +\t\tOPT_SUBCOMMAND(\"init\", &fn, module_init),\n>>> +\t\tOPT_SUBCOMMAND(\"status\", &fn, module_status),\n>>> +\t\tOPT_SUBCOMMAND(\"sync\", &fn, module_sync),\n>>> +\t\tOPT_SUBCOMMAND(\"deinit\", &fn, module_deinit),\n>>> +\t\tOPT_SUBCOMMAND(\"summary\", &fn, module_summary),\n>>> +\t\tOPT_SUBCOMMAND(\"push-check\", &fn, push_check),\n>>> +\t\tOPT_SUBCOMMAND(\"absorbgitdirs\", &fn, absorb_git_dirs),\n>>> +\t\tOPT_SUBCOMMAND(\"set-url\", &fn, module_set_url),\n>>> +\t\tOPT_SUBCOMMAND(\"set-branch\", &fn, module_set_branch),\n>>> +\t\tOPT_SUBCOMMAND(\"create-branch\", &fn, module_create_branch),\n>>> +\t\tOPT_END()\n>>> +\t};\n>>> +\targc = parse_options(argc, argv, prefix, options, usage, 0);\n>>> +\tsubcmd = argv[0];\n>>> +\n>>> +\tif (strcmp(subcmd, \"clone\") && strcmp(subcmd, \"update\") &&\n>>> +\t strcmp(subcmd, \"foreach\") && strcmp(subcmd, \"status\") &&\n>>> +\t strcmp(subcmd, \"sync\") && strcmp(subcmd, \"absorbgitdirs\") &&\n>>> +\t get_super_prefix())\n>>> +\t\t/*\n>>> +\t\t * xstrfmt() rather than \"%s %s\" to keep the translated\n>>> +\t\t * string identical to git.c's.\n>>> +\t\t */\n>>> +\t\tdie(_(\"%s doesn't support --super-prefix\"),\n>>> +\t\t xstrfmt(\"'%s %s'\", cmd, subcmd));\n>>\n>> FYI I'm preparing a series to get rid of the SUPPORT_SUPER_PREFIX checks\n>> in both the top level and in submodule--helper (i.e. revisiting my\n>> complaints from [1]), but I haven't sent it out yet because I haven't\n>> found the right way to motivate that change. Feel free to chime in on\n>> that series when it comes out.\n>>\n>> [1] https://lore.kernel.org/git/20220630221147.45689-5-chooglen@google.com/\n>\n> Maybe you have different plans, but keep at the WIP re-roll of what I\n> have after this, I've also removed all of that.\n>\n> Basically, ending up with:\n> \t\n> \t--- a/builtin.h\n> \t+++ b/builtin.h\n> \t@@ -224,7 +224,14 @@ int cmd_sparse_checkout(int argc, const char **argv, const char *prefix);\n> \t int cmd_status(int argc, const char **argv, const char *prefix);\n> \t int cmd_stash(int argc, const char **argv, const char *prefix);\n> \t int cmd_stripspace(int argc, const char **argv, const char *prefix);\n> \t+int cmd_submodule(int argc, const char **argv, const char *prefix);\n> \t int cmd_submodule__helper(int argc, const char **argv, const char *prefix);\n> \t+int cmd_submodule__helper_clone(int argc, const char **argv, const char *prefix);\n> \t+int cmd_submodule_absorbgitdirs(int argc, const char **argv, const char *prefix);\n> \t+int cmd_submodule_foreach(int argc, const char **argv, const char *prefix);\n> \t+int cmd_submodule_status(int argc, const char **argv, const char *prefix);\n> \t+int cmd_submodule_sync(int argc, const char **argv, const char *prefix);\n> \t+int cmd_submodule_update(int argc, const char **argv, const char *prefix);\n> \t int cmd_switch(int argc, const char **argv, const char *prefix);\n> \t int cmd_symbolic_ref(int argc, const char **argv, const char *prefix);\n> \t int cmd_tag(int argc, const char **argv, const char *prefix);\n>\n> And changes like:\n> \t\n> \t@@ -366,7 +365,7 @@ static void runcommand_in_submodule_cb(const struct cache_entry *list_item,\n> \t \n> \t strvec_pushl(&cpr.args, \"--super-prefix\", NULL);\n> \t strvec_pushf(&cpr.args, \"%s/\", displaypath);\n> \t- strvec_pushl(&cpr.args, \"submodule--helper\", \"foreach\", \"--recursive\",\n> \t+ strvec_pushl(&cpr.args, \"submodule--helper-foreach\", \"--recursive\",\n> \t NULL);\n> \t \n> \t if (info->quiet)\n>\n> I.e. when we call \"foreach\" we dispatch to cmd_submodule_foreach(), but\n> when \"foreach\" needs to recurse it doesn't invoke a \"git submodule\n> foreach\", instead it invokes \"git submodule--helper-foreach\".\n>\n> The reason for doing that is that we can't promote the helper to a\n> built-in without also leaking implementation detail that we support the\n> now internal-only --super-prefix in the command itself.\n>\n> So this code becomes:\n> \t\n> \t@@ -3352,43 +3304,17 @@ static int module_add(int argc, const char **argv, const char *prefix)\n> \t \n> \t int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\n> \t {\n> \t- const char *cmd = argv[0];\n> \t- const char *subcmd;\n> \t parse_opt_subcommand_fn *fn = NULL;\n> \t const char *const usage[] = {\n> \t N_(\"git submodule--helper \"),\n> \t NULL\n> \t };\n> \t struct option options[] = {\n> \t- OPT_SUBCOMMAND(\"clone\", &fn, module_clone),\n> \t- OPT_SUBCOMMAND(\"add\", &fn, module_add),\n> \t- OPT_SUBCOMMAND(\"update\", &fn, module_update),\n> \t- OPT_SUBCOMMAND(\"foreach\", &fn, module_foreach),\n> \t- OPT_SUBCOMMAND(\"init\", &fn, module_init),\n> \t- OPT_SUBCOMMAND(\"status\", &fn, module_status),\n> \t- OPT_SUBCOMMAND(\"sync\", &fn, module_sync),\n> \t- OPT_SUBCOMMAND(\"deinit\", &fn, module_deinit),\n> \t- OPT_SUBCOMMAND(\"summary\", &fn, module_summary),\n> \t- OPT_SUBCOMMAND(\"push-check\", &fn, push_check),\n> \t- OPT_SUBCOMMAND(\"absorbgitdirs\", &fn, absorb_git_dirs),\n> \t- OPT_SUBCOMMAND(\"set-url\", &fn, module_set_url),\n> \t- OPT_SUBCOMMAND(\"set-branch\", &fn, module_set_branch),\n> \t- OPT_SUBCOMMAND(\"create-branch\", &fn, module_create_branch),\n> \t+ OPT_SUBCOMMAND(\"push-check\", &fn, cmd_submodule_push_check),\n> \t+ OPT_SUBCOMMAND(\"create-branch\", &fn, cmd_submodule_create_branch),\n> \t OPT_END()\n> \t };\n> \t argc = parse_options(argc, argv, prefix, options, usage, 0);\n> \t- subcmd = argv[0];\n> \t-\n> \t- if (strcmp(subcmd, \"clone\") && strcmp(subcmd, \"update\") &&\n> \t- strcmp(subcmd, \"foreach\") && strcmp(subcmd, \"status\") &&\n> \t- strcmp(subcmd, \"sync\") && strcmp(subcmd, \"absorbgitdirs\") &&\n> \t- get_super_prefix())\n> \t- /*\n> \t- * xstrfmt() rather than \"%s %s\" to keep the translated\n> \t- * string identical to git.c's.\n> \t- */\n> \t- die(_(\"%s doesn't support --super-prefix\"),\n> \t- xstrfmt(\"'%s %s'\", cmd, subcmd));\n> \t \n> \t return fn(argc, argv, prefix);\n> \t }\n>\n> I.e. for all the \"super-prefix\" ones we dispatch directly (and maybe I\n> should just do that too for those two stragglers).\n>\n> It's ugly, but it's only ugly on the inside, but if you can think of a\n> better way to do it...\n\nIf we _really_ wanted to keep the check, an alternative might be to\nteach the subcommand parser about SUPPORT_SUPER_PREFIX. But frankly, I\nthink this SUPPORT_SUPER_PREFIX check is far more trouble than it's\nworth, and I wouldn't want you to spend your time trying to find ways to\nkeep it only for it to be dropped altogether.\n\nHere's a snippet from the cover letter I'm working on, which will\nhopefully convince you that we don't need to worry about this any more.\n\n----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----\n\n When we introduced the internal-only \"--super-prefix\" in 74866d7579 (git: make\n super-prefix option, 2016-10-07), we specified that commands must prefix paths\n by it, and pathspecs must be parsed relative to it. That commit also includes\n safeguards to ensure that \"--super-prefix\" is used correctly, namely:\n\n - Only commands marked SUPPORT_SUPER_PREFIX can be invoked with \"--super-prefix\".\n - The super prefix is propagated via the GIT_INTERNAL_SUPER_PREFIX env var, so a\n non-SUPPORT_SUPER_PREFIX command cannot be invoked by a SUPPORT_SUPER_PREFIX\n one.\n\n However, its use is inconsistent, which is a strong reason to consider using\n better-scoped flags instead. For example..\n\n - Of the 4 commands that are SUPPORT_SUPER_PREFIX, only \"read-tree\" and\n \"submodule--helper\" do anything useful with it. \"fsmonitor--daemon\" has it\n to avoid the SUPPORT_SUPER_PREFIX warning [1]. \"checkout--worker\" doesn't have\n a documented reason, but since it can be invoked by \"read-tree\", it's\n presumably also a workaround.\n - \"read-tree\" and \"submodule--helper\" use different values for the super prefix;\n \"read-tree\" passes the path from the root of the superproject's tree to the\n submodule's gitlink, while \"submodule--helper\" passes the relative path of the\n original CWD to the submodule.\n - \"submodule--helper\" doesn't use \"--super-prefix\" to parse pathspecs, only to\n display paths.\n\n This series fixes replaces \"--super-prefix\" with such better-scoped flags, and\n fixes some bugs resulting from the SUPPORT_SUPER_PREFIX check.\n\n[1] 53fcfbc84f (fsmonitor--daemon: allow --super-prefix argument, 2022-05-26)\n\n----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----\n\nI figured out all the details, but it would look something like:\n\n- Add an internal-only \"--tree-super-prefix\" flag to \"git read-tree\",\n which sets a global variable that is read from unpack-trees.c.\n- Add an internal-only \"--wt-super-prefix\" flag to \"git\n submodule--helper\", which sets a global variable that is read from\n submodule.c. Unlike \"--super-prefix\", this won't be gated behind a\n SUPPORT_SUPER_PREFIX for each subcommand, since AFAICT, every\n subcommand is using this \"--wt-super-prefix\" correctly, so we can just\n make sure that new subcommands do too.\n- Remove the global \"--super-prefix\", the corresponding env var and\n SUPPORT_SUPER_PREFIX.\n"},{"id":"466545","messageId":"kl6l5yfuy7ew.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"58734","inReplyTo":"221104.86fsezze6p.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 5/8] submodule API & \"absorbgitdirs\": remove \"----recursive\" option","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-11-04T17:08:07Z","receivedAt":"2022-11-04T17:08:12Z","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, Nov 03 2022, Glen Choo wrote:\n>\n>> Ævar Arnfjörð Bjarmason writes:\n>>\n>>> Remove the \"----recursive\" option to \"git submodule--helper\n>>> absorbgitdirs\" (yes, with 4 dashes, not 2).\n>>\n>> o.O\n>>\n>> At least this makes it pretty easy to grep for usage, and it makes sense\n>> that we've never used it (otherwise this would've been caught).\n>>\n>>> diff --git a/submodule.c b/submodule.c\n>>> index fe1e3f03905..8fa2ad457b2 100644\n>>> --- a/submodule.c\n>>> +++ b/submodule.c\n>>> @@ -2332,8 +2331,7 @@ static void absorb_git_dir_into_superproject_recurse(const char *path)\n>>> * having its git directory within the working tree to the git dir nested\n>>> * in its superprojects git dir under modules/.\n>>> */\n>>> -void absorb_git_dir_into_superproject(const char *path,\n>>> -\t\t\t\t unsigned flags)\n>>> +void absorb_git_dir_into_superproject(const char *path)\n>>> {\n>>> \tint err_code;\n>>> \tconst char *sub_git_dir;\n>>> @@ -2382,12 +2380,7 @@ void absorb_git_dir_into_superproject(const char *path,\n>>> \t}\n>>> \tstrbuf_release(&gitdir);\n>>> \n>>> -\tif (flags & ABSORB_GITDIR_RECURSE_SUBMODULES) {\n>>> -\t\tif (flags & ~ABSORB_GITDIR_RECURSE_SUBMODULES)\n>>> -\t\t\tBUG(\"we don't know how to pass the flags down?\");\n>>> -\n>>> -\t\tabsorb_git_dir_into_superproject_recurse(path);\n>>> -\t}\n>>> +\tabsorb_git_dir_into_superproject_recurse(path);\n>>> }\n>>\n>> Maybe I'm misreading, but I don't follow this change.\n>>\n>> Before, we recursed into the submodule only if the\n>> ABSORB_GITDIR_RECURSE_SUBMODULES flag is set (which we now know is\n>> never), but now we unconditionally recurse into the submodule.\n>\n> No, it's always set. I.e. ----recursive did nothing, but the default was\n> to always set ABSORB_GITDIR_RECURSE_SUBMODULES, so it was never not-set\n> (and there was no --no---recursive user).\n>\n> So we should be unconditionally going on this recursive path.\n\nAh, because we initialize flags to ABSORB_GITDIR_RECURSE_SUBMODULES. I\nsee that this is also covered by t/t7412-submodule-absorbgitdirs.sh,\nwhich has a few nested submodules tests. Thanks!\n\nSince it's clear that recursing should be unconditional, I think we\ndon't need the previous patch, but I'm fine either way.\n"},{"id":"466546","messageId":"kl6l35ayy7ay.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"58734","inReplyTo":"cover-0.8-00000000000-20221102T074148Z-avarab@gmail.com","subject":"Re: [PATCH 0/8] submodule: tests, cleanup to prepare for built-in","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-11-04T17:10:29Z","receivedAt":"2022-11-04T17:10:34Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> I have a topic on-list to remove git-submodule.sh and create a\n> builtin/submodule.c, i.e. promoting \"git submodule--helper\" to the\n> \"real thing\"[1].\n>\n> Glen gave me a bunch of good feedback on it, including (but not\n> limited to) pointing out that we have outstanding dead code in\n> [2][3].\n>\n> Once I started pulling at that thread things became a lot simpler for\n> the re-roll of [1], e.g. the migration of git-submodule.sh's commands\n> can squash in the \"update\" step, as it's no longer a special-case.\n>\n> But that also made the series larger, and it's conflicted with other\n> outstanding patches. First René's strvec() cleanup in submodule.c, and\n> currently with Glen's in-flight submodule topic.\n>\n> So here's \"just the prep\" part of that split-out. See also [4] and [5]\n> for previous \"prep\" topics, we're getting closer...\n>\n> This only adds missing test coverage, and deletes dead code that we'd\n> otherwise have to account for. Then 8/8 converts submodule--helper to\n> use the OPT_SUBCOMMAND() API in 8/8.\n\nThanks for working at this so patiently :) I can't wait to see\ngit-submodule.sh gone.\n\nThe only issue I saw was the out-of-date test names in [1]. Otherwise, I\nthink this is ready to merge.\n\n[1] https://lore.kernel.org/git/kl6lfsezofk9.fsf@chooglen-macbookpro.roam.corp.google.com\n"},{"id":"466547","messageId":"Y2Vi95r+RqHPwbw8@nand.local","threadId":"58734","inReplyTo":"kl6l35ayy7ay.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH 0/8] submodule: tests, cleanup to prepare for built-in","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-04T19:07:35Z","receivedAt":"2022-11-04T19:07:40Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Nov 04, 2022 at 10:10:29AM -0700, Glen Choo wrote:\n> Thanks for working at this so patiently :) I can't wait to see\n> git-submodule.sh gone.\n>\n> The only issue I saw was the out-of-date test names in [1]. Otherwise, I\n> think this is ready to merge.\n>\n> [1] https://lore.kernel.org/git/kl6lfsezofk9.fsf@chooglen-macbookpro.roam.corp.google.com\n\nThanks for the review. I'll wait for a reroll on this topic and then we\ncan start merging it down to 'next' and so forth.\n\nThanks,\nTaylor\n"},{"id":"466584","messageId":"221105.86wn89wk4e.gmgdl@evledraar.gmail.com","threadId":"58734","inReplyTo":"kl6l8rkqy7no.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH 8/8] submodule--helper: use OPT_SUBCOMMAND() API","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-05T14:23:24Z","receivedAt":"2022-11-05T14:29:05Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Nov 04 2022, Glen Choo wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> On Thu, Nov 03 2022, Glen Choo wrote:\n>>\n>>> Ævar Arnfjörð Bjarmason writes:\n>>> [...]\n>>>> +\tstruct option options[] = {\n>>>> +\t\tOPT_SUBCOMMAND(\"clone\", &fn, module_clone),\n>>>> +\t\tOPT_SUBCOMMAND(\"add\", &fn, module_add),\n>>>> +\t\tOPT_SUBCOMMAND(\"update\", &fn, module_update),\n>>>> +\t\tOPT_SUBCOMMAND(\"foreach\", &fn, module_foreach),\n>>>> +\t\tOPT_SUBCOMMAND(\"init\", &fn, module_init),\n>>>> +\t\tOPT_SUBCOMMAND(\"status\", &fn, module_status),\n>>>> +\t\tOPT_SUBCOMMAND(\"sync\", &fn, module_sync),\n>>>> +\t\tOPT_SUBCOMMAND(\"deinit\", &fn, module_deinit),\n>>>> +\t\tOPT_SUBCOMMAND(\"summary\", &fn, module_summary),\n>>>> +\t\tOPT_SUBCOMMAND(\"push-check\", &fn, push_check),\n>>>> +\t\tOPT_SUBCOMMAND(\"absorbgitdirs\", &fn, absorb_git_dirs),\n>>>> +\t\tOPT_SUBCOMMAND(\"set-url\", &fn, module_set_url),\n>>>> +\t\tOPT_SUBCOMMAND(\"set-branch\", &fn, module_set_branch),\n>>>> +\t\tOPT_SUBCOMMAND(\"create-branch\", &fn, module_create_branch),\n>>>> +\t\tOPT_END()\n>>>> +\t};\n>>>> +\targc = parse_options(argc, argv, prefix, options, usage, 0);\n>>>> +\tsubcmd = argv[0];\n>>>> +\n>>>> +\tif (strcmp(subcmd, \"clone\") && strcmp(subcmd, \"update\") &&\n>>>> +\t strcmp(subcmd, \"foreach\") && strcmp(subcmd, \"status\") &&\n>>>> +\t strcmp(subcmd, \"sync\") && strcmp(subcmd, \"absorbgitdirs\") &&\n>>>> +\t get_super_prefix())\n>>>> +\t\t/*\n>>>> +\t\t * xstrfmt() rather than \"%s %s\" to keep the translated\n>>>> +\t\t * string identical to git.c's.\n>>>> +\t\t */\n>>>> +\t\tdie(_(\"%s doesn't support --super-prefix\"),\n>>>> +\t\t xstrfmt(\"'%s %s'\", cmd, subcmd));\n>>>\n>>> FYI I'm preparing a series to get rid of the SUPPORT_SUPER_PREFIX checks\n>>> in both the top level and in submodule--helper (i.e. revisiting my\n>>> complaints from [1]), but I haven't sent it out yet because I haven't\n>>> found the right way to motivate that change. Feel free to chime in on\n>>> that series when it comes out.\n>>>\n>>> [1] https://lore.kernel.org/git/20220630221147.45689-5-chooglen@google.com/\n>>\n>> Maybe you have different plans, but keep at the WIP re-roll of what I\n>> have after this, I've also removed all of that.\n>>\n>> Basically, ending up with:\n>> \t\n>> \t--- a/builtin.h\n>> \t+++ b/builtin.h\n>> \t@@ -224,7 +224,14 @@ int cmd_sparse_checkout(int argc, const char **argv, const char *prefix);\n>> \t int cmd_status(int argc, const char **argv, const char *prefix);\n>> \t int cmd_stash(int argc, const char **argv, const char *prefix);\n>> \t int cmd_stripspace(int argc, const char **argv, const char *prefix);\n>> \t+int cmd_submodule(int argc, const char **argv, const char *prefix);\n>> \t int cmd_submodule__helper(int argc, const char **argv, const char *prefix);\n>> \t+int cmd_submodule__helper_clone(int argc, const char **argv, const char *prefix);\n>> \t+int cmd_submodule_absorbgitdirs(int argc, const char **argv, const char *prefix);\n>> \t+int cmd_submodule_foreach(int argc, const char **argv, const char *prefix);\n>> \t+int cmd_submodule_status(int argc, const char **argv, const char *prefix);\n>> \t+int cmd_submodule_sync(int argc, const char **argv, const char *prefix);\n>> \t+int cmd_submodule_update(int argc, const char **argv, const char *prefix);\n>> \t int cmd_switch(int argc, const char **argv, const char *prefix);\n>> \t int cmd_symbolic_ref(int argc, const char **argv, const char *prefix);\n>> \t int cmd_tag(int argc, const char **argv, const char *prefix);\n>>\n>> And changes like:\n>> \t\n>> \t@@ -366,7 +365,7 @@ static void runcommand_in_submodule_cb(const struct cache_entry *list_item,\n>> \t \n>> \t strvec_pushl(&cpr.args, \"--super-prefix\", NULL);\n>> \t strvec_pushf(&cpr.args, \"%s/\", displaypath);\n>> \t- strvec_pushl(&cpr.args, \"submodule--helper\", \"foreach\", \"--recursive\",\n>> \t+ strvec_pushl(&cpr.args, \"submodule--helper-foreach\", \"--recursive\",\n>> \t NULL);\n>> \t \n>> \t if (info->quiet)\n>>\n>> I.e. when we call \"foreach\" we dispatch to cmd_submodule_foreach(), but\n>> when \"foreach\" needs to recurse it doesn't invoke a \"git submodule\n>> foreach\", instead it invokes \"git submodule--helper-foreach\".\n>>\n>> The reason for doing that is that we can't promote the helper to a\n>> built-in without also leaking implementation detail that we support the\n>> now internal-only --super-prefix in the command itself.\n>>\n>> So this code becomes:\n>> \t\n>> \t@@ -3352,43 +3304,17 @@ static int module_add(int argc, const char **argv, const char *prefix)\n>> \t \n>> \t int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\n>> \t {\n>> \t- const char *cmd = argv[0];\n>> \t- const char *subcmd;\n>> \t parse_opt_subcommand_fn *fn = NULL;\n>> \t const char *const usage[] = {\n>> \t N_(\"git submodule--helper \"),\n>> \t NULL\n>> \t };\n>> \t struct option options[] = {\n>> \t- OPT_SUBCOMMAND(\"clone\", &fn, module_clone),\n>> \t- OPT_SUBCOMMAND(\"add\", &fn, module_add),\n>> \t- OPT_SUBCOMMAND(\"update\", &fn, module_update),\n>> \t- OPT_SUBCOMMAND(\"foreach\", &fn, module_foreach),\n>> \t- OPT_SUBCOMMAND(\"init\", &fn, module_init),\n>> \t- OPT_SUBCOMMAND(\"status\", &fn, module_status),\n>> \t- OPT_SUBCOMMAND(\"sync\", &fn, module_sync),\n>> \t- OPT_SUBCOMMAND(\"deinit\", &fn, module_deinit),\n>> \t- OPT_SUBCOMMAND(\"summary\", &fn, module_summary),\n>> \t- OPT_SUBCOMMAND(\"push-check\", &fn, push_check),\n>> \t- OPT_SUBCOMMAND(\"absorbgitdirs\", &fn, absorb_git_dirs),\n>> \t- OPT_SUBCOMMAND(\"set-url\", &fn, module_set_url),\n>> \t- OPT_SUBCOMMAND(\"set-branch\", &fn, module_set_branch),\n>> \t- OPT_SUBCOMMAND(\"create-branch\", &fn, module_create_branch),\n>> \t+ OPT_SUBCOMMAND(\"push-check\", &fn, cmd_submodule_push_check),\n>> \t+ OPT_SUBCOMMAND(\"create-branch\", &fn, cmd_submodule_create_branch),\n>> \t OPT_END()\n>> \t };\n>> \t argc = parse_options(argc, argv, prefix, options, usage, 0);\n>> \t- subcmd = argv[0];\n>> \t-\n>> \t- if (strcmp(subcmd, \"clone\") && strcmp(subcmd, \"update\") &&\n>> \t- strcmp(subcmd, \"foreach\") && strcmp(subcmd, \"status\") &&\n>> \t- strcmp(subcmd, \"sync\") && strcmp(subcmd, \"absorbgitdirs\") &&\n>> \t- get_super_prefix())\n>> \t- /*\n>> \t- * xstrfmt() rather than \"%s %s\" to keep the translated\n>> \t- * string identical to git.c's.\n>> \t- */\n>> \t- die(_(\"%s doesn't support --super-prefix\"),\n>> \t- xstrfmt(\"'%s %s'\", cmd, subcmd));\n>> \t \n>> \t return fn(argc, argv, prefix);\n>> \t }\n>>\n>> I.e. for all the \"super-prefix\" ones we dispatch directly (and maybe I\n>> should just do that too for those two stragglers).\n>>\n>> It's ugly, but it's only ugly on the inside, but if you can think of a\n>> better way to do it...\n>\n> If we _really_ wanted to keep the check, an alternative might be to\n> teach the subcommand parser about SUPPORT_SUPER_PREFIX. But frankly, I\n> think this SUPPORT_SUPER_PREFIX check is far more trouble than it's\n> worth, and I wouldn't want you to spend your time trying to find ways to\n> keep it only for it to be dropped altogether.\n>\n> Here's a snippet from the cover letter I'm working on, which will\n> hopefully convince you that we don't need to worry about this any more.\n>\n> ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----\n>\n> When we introduced the internal-only \"--super-prefix\" in 74866d7579 (git: make\n> super-prefix option, 2016-10-07), we specified that commands must prefix paths\n> by it, and pathspecs must be parsed relative to it. That commit also includes\n> safeguards to ensure that \"--super-prefix\" is used correctly, namely:\n>\n> - Only commands marked SUPPORT_SUPER_PREFIX can be invoked with \"--super-prefix\".\n> - The super prefix is propagated via the GIT_INTERNAL_SUPER_PREFIX env var, so a\n> non-SUPPORT_SUPER_PREFIX command cannot be invoked by a SUPPORT_SUPER_PREFIX\n> one.\n>\n> However, its use is inconsistent, which is a strong reason to consider using\n> better-scoped flags instead. For example..\n>\n> - Of the 4 commands that are SUPPORT_SUPER_PREFIX, only \"read-tree\" and\n> \"submodule--helper\" do anything useful with it. \"fsmonitor--daemon\" has it\n> to avoid the SUPPORT_SUPER_PREFIX warning [1]. \"checkout--worker\" doesn't have\n> a documented reason, but since it can be invoked by \"read-tree\", it's\n> presumably also a workaround.\n> - \"read-tree\" and \"submodule--helper\" use different values for the super prefix;\n> \"read-tree\" passes the path from the root of the superproject's tree to the\n> submodule's gitlink, while \"submodule--helper\" passes the relative path of the\n> original CWD to the submodule.\n> - \"submodule--helper\" doesn't use \"--super-prefix\" to parse pathspecs, only to\n> display paths.\n>\n> This series fixes replaces \"--super-prefix\" with such better-scoped flags, and\n> fixes some bugs resulting from the SUPPORT_SUPER_PREFIX check.\n>\n> [1] 53fcfbc84f (fsmonitor--daemon: allow --super-prefix argument, 2022-05-26)\n>\n> ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----\n>\n> I figured out all the details, but it would look something like:\n>\n> - Add an internal-only \"--tree-super-prefix\" flag to \"git read-tree\",\n> which sets a global variable that is read from unpack-trees.c.\n> - Add an internal-only \"--wt-super-prefix\" flag to \"git\n> submodule--helper\", which sets a global variable that is read from\n> submodule.c. Unlike \"--super-prefix\", this won't be gated behind a\n> SUPPORT_SUPER_PREFIX for each subcommand, since AFAICT, every\n> subcommand is using this \"--wt-super-prefix\" correctly, so we can just\n> make sure that new subcommands do too.\n> - Remove the global \"--super-prefix\", the corresponding env var and\n> SUPPORT_SUPER_PREFIX.\n\nYes, I'm very much on the same page with removing it eventully. But as\nyou note above that'll take some doing.\n\nSo for the \"submodule as a built-in\" I didn't want to take it hostage to\nthat, but just to ensure that I:\n\n * Got it working\n * Did not implicitly leak \"this needs a --super-prefix\" from the\n current set of commands to any other commands, or to e.g. public\n \"submodule foreach\" just because we need it for the internal-only\n helper.\n\nAre you proposing that you'll submit another clean-up topic that the\nsubmodule-as-a-builtin will need to be queued on top of, in addition to\nthis topic?\n\nIf so I'd really prefer we defer those cleanups, which should also be\nmuch easier once submodule's fully in C, unless they're much more\ntrivial than I'm currently suspecting they are...\n"},{"id":"466679","messageId":"kl6lsfiuwur2.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"58734","inReplyTo":"221105.86wn89wk4e.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 8/8] submodule--helper: use OPT_SUBCOMMAND() API","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-11-07T17:16:01Z","receivedAt":"2022-11-07T17:16:09Z","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 Fri, Nov 04 2022, Glen Choo wrote:\n>\n>> Ævar Arnfjörð Bjarmason writes:\n>>\n>>> On Thu, Nov 03 2022, Glen Choo wrote:\n>>>\n>>>> Ævar Arnfjörð Bjarmason writes:\n>>>> [...]\n>>>>> +\tstruct option options[] = {\n>>>>> +\t\tOPT_SUBCOMMAND(\"clone\", &fn, module_clone),\n>>>>> +\t\tOPT_SUBCOMMAND(\"add\", &fn, module_add),\n>>>>> +\t\tOPT_SUBCOMMAND(\"update\", &fn, module_update),\n>>>>> +\t\tOPT_SUBCOMMAND(\"foreach\", &fn, module_foreach),\n>>>>> +\t\tOPT_SUBCOMMAND(\"init\", &fn, module_init),\n>>>>> +\t\tOPT_SUBCOMMAND(\"status\", &fn, module_status),\n>>>>> +\t\tOPT_SUBCOMMAND(\"sync\", &fn, module_sync),\n>>>>> +\t\tOPT_SUBCOMMAND(\"deinit\", &fn, module_deinit),\n>>>>> +\t\tOPT_SUBCOMMAND(\"summary\", &fn, module_summary),\n>>>>> +\t\tOPT_SUBCOMMAND(\"push-check\", &fn, push_check),\n>>>>> +\t\tOPT_SUBCOMMAND(\"absorbgitdirs\", &fn, absorb_git_dirs),\n>>>>> +\t\tOPT_SUBCOMMAND(\"set-url\", &fn, module_set_url),\n>>>>> +\t\tOPT_SUBCOMMAND(\"set-branch\", &fn, module_set_branch),\n>>>>> +\t\tOPT_SUBCOMMAND(\"create-branch\", &fn, module_create_branch),\n>>>>> +\t\tOPT_END()\n>>>>> +\t};\n>>>>> +\targc = parse_options(argc, argv, prefix, options, usage, 0);\n>>>>> +\tsubcmd = argv[0];\n>>>>> +\n>>>>> +\tif (strcmp(subcmd, \"clone\") && strcmp(subcmd, \"update\") &&\n>>>>> +\t strcmp(subcmd, \"foreach\") && strcmp(subcmd, \"status\") &&\n>>>>> +\t strcmp(subcmd, \"sync\") && strcmp(subcmd, \"absorbgitdirs\") &&\n>>>>> +\t get_super_prefix())\n>>>>> +\t\t/*\n>>>>> +\t\t * xstrfmt() rather than \"%s %s\" to keep the translated\n>>>>> +\t\t * string identical to git.c's.\n>>>>> +\t\t */\n>>>>> +\t\tdie(_(\"%s doesn't support --super-prefix\"),\n>>>>> +\t\t xstrfmt(\"'%s %s'\", cmd, subcmd));\n>>>>\n>>>> FYI I'm preparing a series to get rid of the SUPPORT_SUPER_PREFIX checks\n>>>> in both the top level and in submodule--helper (i.e. revisiting my\n>>>> complaints from [1]), but I haven't sent it out yet because I haven't\n>>>> found the right way to motivate that change. Feel free to chime in on\n>>>> that series when it comes out.\n>>>>\n>>>> [1] https://lore.kernel.org/git/20220630221147.45689-5-chooglen@google.com/\n>>>\n>>> Maybe you have different plans, but keep at the WIP re-roll of what I\n>>> have after this, I've also removed all of that.\n>>>\n>>> Basically, ending up with:\n>>> \t\n>>> \t--- a/builtin.h\n>>> \t+++ b/builtin.h\n>>> \t@@ -224,7 +224,14 @@ int cmd_sparse_checkout(int argc, const char **argv, const char *prefix);\n>>> \t int cmd_status(int argc, const char **argv, const char *prefix);\n>>> \t int cmd_stash(int argc, const char **argv, const char *prefix);\n>>> \t int cmd_stripspace(int argc, const char **argv, const char *prefix);\n>>> \t+int cmd_submodule(int argc, const char **argv, const char *prefix);\n>>> \t int cmd_submodule__helper(int argc, const char **argv, const char *prefix);\n>>> \t+int cmd_submodule__helper_clone(int argc, const char **argv, const char *prefix);\n>>> \t+int cmd_submodule_absorbgitdirs(int argc, const char **argv, const char *prefix);\n>>> \t+int cmd_submodule_foreach(int argc, const char **argv, const char *prefix);\n>>> \t+int cmd_submodule_status(int argc, const char **argv, const char *prefix);\n>>> \t+int cmd_submodule_sync(int argc, const char **argv, const char *prefix);\n>>> \t+int cmd_submodule_update(int argc, const char **argv, const char *prefix);\n>>> \t int cmd_switch(int argc, const char **argv, const char *prefix);\n>>> \t int cmd_symbolic_ref(int argc, const char **argv, const char *prefix);\n>>> \t int cmd_tag(int argc, const char **argv, const char *prefix);\n>>>\n>>> And changes like:\n>>> \t\n>>> \t@@ -366,7 +365,7 @@ static void runcommand_in_submodule_cb(const struct cache_entry *list_item,\n>>> \t \n>>> \t strvec_pushl(&cpr.args, \"--super-prefix\", NULL);\n>>> \t strvec_pushf(&cpr.args, \"%s/\", displaypath);\n>>> \t- strvec_pushl(&cpr.args, \"submodule--helper\", \"foreach\", \"--recursive\",\n>>> \t+ strvec_pushl(&cpr.args, \"submodule--helper-foreach\", \"--recursive\",\n>>> \t NULL);\n>>> \t \n>>> \t if (info->quiet)\n>>>\n>>> I.e. when we call \"foreach\" we dispatch to cmd_submodule_foreach(), but\n>>> when \"foreach\" needs to recurse it doesn't invoke a \"git submodule\n>>> foreach\", instead it invokes \"git submodule--helper-foreach\".\n>>>\n>>> The reason for doing that is that we can't promote the helper to a\n>>> built-in without also leaking implementation detail that we support the\n>>> now internal-only --super-prefix in the command itself.\n>>>\n>>> So this code becomes:\n>>> \t\n>>> \t@@ -3352,43 +3304,17 @@ static int module_add(int argc, const char **argv, const char *prefix)\n>>> \t \n>>> \t int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\n>>> \t {\n>>> \t- const char *cmd = argv[0];\n>>> \t- const char *subcmd;\n>>> \t parse_opt_subcommand_fn *fn = NULL;\n>>> \t const char *const usage[] = {\n>>> \t N_(\"git submodule--helper \"),\n>>> \t NULL\n>>> \t };\n>>> \t struct option options[] = {\n>>> \t- OPT_SUBCOMMAND(\"clone\", &fn, module_clone),\n>>> \t- OPT_SUBCOMMAND(\"add\", &fn, module_add),\n>>> \t- OPT_SUBCOMMAND(\"update\", &fn, module_update),\n>>> \t- OPT_SUBCOMMAND(\"foreach\", &fn, module_foreach),\n>>> \t- OPT_SUBCOMMAND(\"init\", &fn, module_init),\n>>> \t- OPT_SUBCOMMAND(\"status\", &fn, module_status),\n>>> \t- OPT_SUBCOMMAND(\"sync\", &fn, module_sync),\n>>> \t- OPT_SUBCOMMAND(\"deinit\", &fn, module_deinit),\n>>> \t- OPT_SUBCOMMAND(\"summary\", &fn, module_summary),\n>>> \t- OPT_SUBCOMMAND(\"push-check\", &fn, push_check),\n>>> \t- OPT_SUBCOMMAND(\"absorbgitdirs\", &fn, absorb_git_dirs),\n>>> \t- OPT_SUBCOMMAND(\"set-url\", &fn, module_set_url),\n>>> \t- OPT_SUBCOMMAND(\"set-branch\", &fn, module_set_branch),\n>>> \t- OPT_SUBCOMMAND(\"create-branch\", &fn, module_create_branch),\n>>> \t+ OPT_SUBCOMMAND(\"push-check\", &fn, cmd_submodule_push_check),\n>>> \t+ OPT_SUBCOMMAND(\"create-branch\", &fn, cmd_submodule_create_branch),\n>>> \t OPT_END()\n>>> \t };\n>>> \t argc = parse_options(argc, argv, prefix, options, usage, 0);\n>>> \t- subcmd = argv[0];\n>>> \t-\n>>> \t- if (strcmp(subcmd, \"clone\") && strcmp(subcmd, \"update\") &&\n>>> \t- strcmp(subcmd, \"foreach\") && strcmp(subcmd, \"status\") &&\n>>> \t- strcmp(subcmd, \"sync\") && strcmp(subcmd, \"absorbgitdirs\") &&\n>>> \t- get_super_prefix())\n>>> \t- /*\n>>> \t- * xstrfmt() rather than \"%s %s\" to keep the translated\n>>> \t- * string identical to git.c's.\n>>> \t- */\n>>> \t- die(_(\"%s doesn't support --super-prefix\"),\n>>> \t- xstrfmt(\"'%s %s'\", cmd, subcmd));\n>>> \t \n>>> \t return fn(argc, argv, prefix);\n>>> \t }\n>>>\n>>> I.e. for all the \"super-prefix\" ones we dispatch directly (and maybe I\n>>> should just do that too for those two stragglers).\n>>>\n>>> It's ugly, but it's only ugly on the inside, but if you can think of a\n>>> better way to do it...\n>>\n>> If we _really_ wanted to keep the check, an alternative might be to\n>> teach the subcommand parser about SUPPORT_SUPER_PREFIX. But frankly, I\n>> think this SUPPORT_SUPER_PREFIX check is far more trouble than it's\n>> worth, and I wouldn't want you to spend your time trying to find ways to\n>> keep it only for it to be dropped altogether.\n>>\n>> Here's a snippet from the cover letter I'm working on, which will\n>> hopefully convince you that we don't need to worry about this any more.\n>>\n>> ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----\n>>\n>> When we introduced the internal-only \"--super-prefix\" in 74866d7579 (git: make\n>> super-prefix option, 2016-10-07), we specified that commands must prefix paths\n>> by it, and pathspecs must be parsed relative to it. That commit also includes\n>> safeguards to ensure that \"--super-prefix\" is used correctly, namely:\n>>\n>> - Only commands marked SUPPORT_SUPER_PREFIX can be invoked with \"--super-prefix\".\n>> - The super prefix is propagated via the GIT_INTERNAL_SUPER_PREFIX env var, so a\n>> non-SUPPORT_SUPER_PREFIX command cannot be invoked by a SUPPORT_SUPER_PREFIX\n>> one.\n>>\n>> However, its use is inconsistent, which is a strong reason to consider using\n>> better-scoped flags instead. For example..\n>>\n>> - Of the 4 commands that are SUPPORT_SUPER_PREFIX, only \"read-tree\" and\n>> \"submodule--helper\" do anything useful with it. \"fsmonitor--daemon\" has it\n>> to avoid the SUPPORT_SUPER_PREFIX warning [1]. \"checkout--worker\" doesn't have\n>> a documented reason, but since it can be invoked by \"read-tree\", it's\n>> presumably also a workaround.\n>> - \"read-tree\" and \"submodule--helper\" use different values for the super prefix;\n>> \"read-tree\" passes the path from the root of the superproject's tree to the\n>> submodule's gitlink, while \"submodule--helper\" passes the relative path of the\n>> original CWD to the submodule.\n>> - \"submodule--helper\" doesn't use \"--super-prefix\" to parse pathspecs, only to\n>> display paths.\n>>\n>> This series fixes replaces \"--super-prefix\" with such better-scoped flags, and\n>> fixes some bugs resulting from the SUPPORT_SUPER_PREFIX check.\n>>\n>> [1] 53fcfbc84f (fsmonitor--daemon: allow --super-prefix argument, 2022-05-26)\n>>\n>> ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----\n>>\n>> I figured out all the details, but it would look something like:\n>>\n>> - Add an internal-only \"--tree-super-prefix\" flag to \"git read-tree\",\n>> which sets a global variable that is read from unpack-trees.c.\n>> - Add an internal-only \"--wt-super-prefix\" flag to \"git\n>> submodule--helper\", which sets a global variable that is read from\n>> submodule.c. Unlike \"--super-prefix\", this won't be gated behind a\n>> SUPPORT_SUPER_PREFIX for each subcommand, since AFAICT, every\n>> subcommand is using this \"--wt-super-prefix\" correctly, so we can just\n>> make sure that new subcommands do too.\n>> - Remove the global \"--super-prefix\", the corresponding env var and\n>> SUPPORT_SUPER_PREFIX.\n> So for the \"submodule as a built-in\" I didn't want to take it hostage to\n> that\n\nThat's a reasonable concern.\n\n> Are you proposing that you'll submit another clean-up topic that the\n> submodule-as-a-builtin will need to be queued on top of, in addition to\n> this topic?\n>\n> If so I'd really prefer we defer those cleanups, which should also be\n> much easier once submodule's fully in C, unless they're much more\n> trivial than I'm currently suspecting they are...\n\nYes, that is what I'm proposing (unfortunately). I don't like having\neither of us blocked on the other's work, but it seems like it really\ndoes work out better this way, e.g. this series makes it much easier to\nreplace \"git --super-prefix\" with \"git submodule--helper\n--something-else\" because we now use the options parser, and removing\nthe \"--super-prefix\" check makes the eventual conversion simpler because\nthat's one less behavior to worry about keeping.\n\nWe can see if my proposal makes sense when I send something out\n(possibly as RFC). I'll do that today/tomorrow.\n"},{"id":"466823","messageId":"cover-v2-0.9-00000000000-20221108T140501Z-avarab@gmail.com","threadId":"58734","inReplyTo":"cover-0.8-00000000000-20221102T074148Z-avarab@gmail.com","subject":"[PATCH v2 0/9] submodule: tests, cleanup to prepare for built-in","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-08T14:10:31Z","receivedAt":"2022-11-08T14:10:54Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"As noted in the v1[1] this is a prep series for getting us to a\nbuilt-in submodule.c, with a \"git rm git-submodule.sh\". As noted in\nthe v1 discussion (e.g. Taylor's [2]) this was mostly ready, but just\nneeded some final finishing touches.\n\nChanges since v1:\n\n* Noted the history of when \"submodule--helper config\" was last used,\n per Glen's comment.\n* Re-titled the tests htat no longer use the now-removed\n \"submodule--helper config\", per Glen's comment.\n* I removed _() from a string in what's left of \"submodule--helper\n config\", which is now a test helper. We should not be translating\n test helper strings.\n* Fixed a buggy for-loop in the t7422-submodule-output.sh test I'm\n adding.\n* Fixed a trivial typo in a comment in that test & in another nearby\n comment.\n\nPassing CI run & branch for this available at[3].\n\n1. https://lore.kernel.org/git/cover-0.8-00000000000-20221102T074148Z-avarab@gmail.com/\n2. https://lore.kernel.org/git/Y2Vi95r+RqHPwbw8@nand.local/\n3. https://github.com/avar/git/tree/avar/submodule-builtin-final-prep-2\n\nÆvar Arnfjörð Bjarmason (9):\n submodule--helper: move \"config\" to a test-tool\n submodule tests: add tests for top-level flag output\n submodule--helper: fix a memory leak in \"status\"\n submodule tests: test for a \"foreach\" blind-spot\n submodule.c: refactor recursive block out of absorb function\n submodule API & \"absorbgitdirs\": remove \"----recursive\" option\n submodule--helper: remove --prefix from \"absorbgitdirs\"\n submodule--helper: drop \"update --prefix \" for \"-C update\"\n submodule--helper: use OPT_SUBCOMMAND() API\n\n builtin/rm.c | 3 +-\n builtin/submodule--helper.c | 146 +++++++--------------\n git-submodule.sh | 3 +-\n git.c | 2 +-\n submodule.c | 41 +++---\n submodule.h | 4 +-\n t/helper/test-submodule.c | 84 ++++++++++++\n t/t7400-submodule-basic.sh | 10 ++\n t/t7407-submodule-foreach.sh | 5 +\n t/t7411-submodule-config.sh | 36 +++---\n t/t7418-submodule-sparse-gitmodules.sh | 4 +-\n t/t7422-submodule-output.sh | 170 +++++++++++++++++++++++++\n 12 files changed, 358 insertions(+), 150 deletions(-)\n create mode 100755 t/t7422-submodule-output.sh\n\nRange-diff against v1:\n 1: b2649f96715 ! 1: ca8f8b4bf63 submodule--helper: move \"config\" to a test-tool\n @@ Commit message\n 'ab/submodule-helper-prep', 2022-09-13) the \"config\" sub-command was\n only used by our own tests.\n \n + It was last used by \"git submodule\" itself in code that went away with\n + a6226fd772b (submodule--helper: convert the bulk of cmd_add() to C,\n + 2021-08-10).\n +\n Let's move it over, and while doing so make it easier to reason about\n by splitting up the various uses for it into separate sub-commands, so\n that we don't need to count arguments to see what it does.\n \n + This also has the advantage that we stop wasting future translator\n + time on this command, currently the usage information for this\n + internal-only tool has been translated into several languages. The use\n + of the \"_\" function has also been removed from the \"please make\n + sure...\" message.\n +\n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n ## builtin/submodule--helper.c ##\n @@ t/helper/test-submodule.c: static int cmd__submodule_resolve_relative_url(int ar\n +\t/* Equivalent to ACTION_SET in builtin/config.c */\n +\tif (argc == 3) {\n +\t\tif (!is_writing_gitmodules_ok())\n -+\t\t\tdie(_(\"please make sure that the .gitmodules file is in the working tree\"));\n ++\t\t\tdie(\"please make sure that the .gitmodules file is in the working tree\");\n +\n +\t\treturn config_set_in_gitmodules_file_gently(argv[1], argv[2]);\n +\t}\n @@ t/helper/test-submodule.c: static int cmd__submodule_resolve_relative_url(int ar\n +\n +\tif (argc == 2) {\n +\t\tif (!is_writing_gitmodules_ok())\n -+\t\t\tdie(_(\"please make sure that the .gitmodules file is in the working tree\"));\n ++\t\t\tdie(\"please make sure that the .gitmodules file is in the working tree\");\n +\t\treturn config_set_in_gitmodules_file_gently(argv[1], NULL);\n +\t}\n +\tusage_with_options(usage, options);\n @@ t/helper/test-submodule.c: static int cmd__submodule_resolve_relative_url(int ar\n \n ## t/t7411-submodule-config.sh ##\n @@ t/t7411-submodule-config.sh: test_expect_success 'error in history in fetchrecursesubmodule lets continue' '\n - test_expect_success 'reading submodules config from the working tree with \"submodule--helper config\"' '\n + \t)\n + '\n + \n +-test_expect_success 'reading submodules config from the working tree with \"submodule--helper config\"' '\n ++test_expect_success 'reading submodules config from the working tree' '\n \t(cd super &&\n \t\techo \"../submodule\" >expect &&\n -\t\tgit submodule--helper config submodule.submodule.url >actual &&\n @@ t/t7411-submodule-config.sh: test_expect_success 'error in history in fetchrecur\n \t)\n '\n \n - test_expect_success 'unsetting submodules config from the working tree with \"submodule--helper config --unset\"' '\n +-test_expect_success 'unsetting submodules config from the working tree with \"submodule--helper config --unset\"' '\n ++test_expect_success 'unsetting submodules config from the working tree' '\n \t(cd super &&\n -\t\tgit submodule--helper config --unset submodule.submodule.url &&\n -\t\tgit submodule--helper config submodule.submodule.url >actual &&\n @@ t/t7411-submodule-config.sh: test_expect_success 'error in history in fetchrecur\n \t\ttest_must_be_empty actual\n \t)\n '\n -@@ t/t7411-submodule-config.sh: test_expect_success 'unsetting submodules config from the working tree with \"sub\n - test_expect_success 'writing submodules config with \"submodule--helper config\"' '\n + \n + \n +-test_expect_success 'writing submodules config with \"submodule--helper config\"' '\n ++test_expect_success 'writing submodules config' '\n \t(cd super &&\n \t\techo \"new_url\" >expect &&\n -\t\tgit submodule--helper config submodule.submodule.url \"new_url\" &&\n @@ t/t7411-submodule-config.sh: test_expect_success 'unsetting submodules config fr\n \t\ttest_cmp expect actual\n \t)\n '\n -@@ t/t7411-submodule-config.sh: test_expect_success 'overwriting unstaged submodules config with \"submodule--hel\n + \n +-test_expect_success 'overwriting unstaged submodules config with \"submodule--helper config\"' '\n ++test_expect_success 'overwriting unstaged submodules config' '\n \ttest_when_finished \"git -C super checkout .gitmodules\" &&\n \t(cd super &&\n \t\techo \"newer_url\" >expect &&\n 2: cda36b5b6e0 ! 2: 5508c27f653 submodule tests: add tests for top-level flag output\n @@ t/t7422-submodule-output.sh (new)\n +\tgit -C X pull &&\n +\tGIT_ALLOW_PROTOCOL=file git -C X submodule update --init &&\n +\n -+\t# dirty p\n ++\t# dirty\n +\tfor d in S.D X/S.D\n +\tdo\n -+\t\techo dirty >S.D/A.t || return 1\n ++\t\techo dirty >\"$d\"/A.t || return 1\n +\tdone &&\n +\n +\t# commit (for --cached)\n @@ t/t7422-submodule-output.sh (new)\n +\n +\tfor ref in A B C\n +\tdo\n -+\t\t# Not different with SHA-1 and SHA-256, just (ab)usign\n ++\t\t# Not different with SHA-1 and SHA-256, just (ab)using\n +\t\t# test_oid_cache as a variable bag to avoid using\n +\t\t# $(git rev-parse ...).\n +\t\toid=$(git rev-parse $ref) &&\n -: ----------- > 3: a3529d7f9e0 submodule--helper: fix a memory leak in \"status\"\n 3: 0ed1fc7fdf8 = 4: c14cc0e14b8 submodule tests: test for a \"foreach\" blind-spot\n 4: f7adfbc13ae = 5: 459ea25125b submodule.c: refactor recursive block out of absorb function\n 5: 2b8afd73b9b = 6: 322a02c30fc submodule API & \"absorbgitdirs\": remove \"----recursive\" option\n 6: 91208241070 = 7: d1f4ac20a4f submodule--helper: remove --prefix from \"absorbgitdirs\"\n 7: 77d4d5a6c09 = 8: ac9ff05ef68 submodule--helper: drop \"update --prefix \" for \"-C update\"\n 8: 105853cd358 = 9: cccd92a829c submodule--helper: use OPT_SUBCOMMAND() API\n-- \n2.38.0.1464.gea6794aacbc\n\n"},{"id":"466824","messageId":"patch-v2-1.9-ca8f8b4bf63-20221108T140501Z-avarab@gmail.com","threadId":"58734","inReplyTo":"cover-v2-0.9-00000000000-20221108T140501Z-avarab@gmail.com","subject":"[PATCH v2 1/9] submodule--helper: move \"config\" to a test-tool","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-08T14:10:32Z","receivedAt":"2022-11-08T14:10:56Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"As with other moves to \"test-tool\" in f322e9f51b5 (Merge branch\n'ab/submodule-helper-prep', 2022-09-13) the \"config\" sub-command was\nonly used by our own tests.\n\nIt was last used by \"git submodule\" itself in code that went away with\na6226fd772b (submodule--helper: convert the bulk of cmd_add() to C,\n2021-08-10).\n\nLet's move it over, and while doing so make it easier to reason about\nby splitting up the various uses for it into separate sub-commands, so\nthat we don't need to count arguments to see what it does.\n\nThis also has the advantage that we stop wasting future translator\ntime on this command, currently the usage information for this\ninternal-only tool has been translated into several languages. The use\nof the \"_\" function has also been removed from the \"please make\nsure...\" message.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/submodule--helper.c | 46 --------------\n t/helper/test-submodule.c | 84 ++++++++++++++++++++++++++\n t/t7411-submodule-config.sh | 36 +++++------\n t/t7418-submodule-sparse-gitmodules.sh | 4 +-\n 4 files changed, 104 insertions(+), 66 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex a7683d35299..6250b95a6f7 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2861,51 +2861,6 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)\n \treturn ret;\n }\n \n-static int module_config(int argc, const char **argv, const char *prefix)\n-{\n-\tenum {\n-\t\tCHECK_WRITEABLE = 1,\n-\t\tDO_UNSET = 2\n-\t} command = 0;\n-\tstruct option module_config_options[] = {\n-\t\tOPT_CMDMODE(0, \"check-writeable\", &command,\n-\t\t\t N_(\"check if it is safe to write to the .gitmodules file\"),\n-\t\t\t CHECK_WRITEABLE),\n-\t\tOPT_CMDMODE(0, \"unset\", &command,\n-\t\t\t N_(\"unset the config in the .gitmodules file\"),\n-\t\t\t DO_UNSET),\n-\t\tOPT_END()\n-\t};\n-\tconst char *const git_submodule_helper_usage[] = {\n-\t\tN_(\"git submodule--helper config []\"),\n-\t\tN_(\"git submodule--helper config --unset \"),\n-\t\t\"git submodule--helper config --check-writeable\",\n-\t\tNULL\n-\t};\n-\n-\targc = parse_options(argc, argv, prefix, module_config_options,\n-\t\t\t git_submodule_helper_usage, PARSE_OPT_KEEP_ARGV0);\n-\n-\tif (argc == 1 && command == CHECK_WRITEABLE)\n-\t\treturn is_writing_gitmodules_ok() ? 0 : -1;\n-\n-\t/* Equivalent to ACTION_GET in builtin/config.c */\n-\tif (argc == 2 && command != DO_UNSET)\n-\t\treturn print_config_from_gitmodules(the_repository, argv[1]);\n-\n-\t/* Equivalent to ACTION_SET in builtin/config.c */\n-\tif (argc == 3 || (argc == 2 && command == DO_UNSET)) {\n-\t\tconst char *value = (argc == 3) ? argv[2] : NULL;\n-\n-\t\tif (!is_writing_gitmodules_ok())\n-\t\t\tdie(_(\"please make sure that the .gitmodules file is in the working tree\"));\n-\n-\t\treturn config_set_in_gitmodules_file_gently(argv[1], value);\n-\t}\n-\n-\tusage_with_options(git_submodule_helper_usage, module_config_options);\n-}\n-\n static int module_set_url(int argc, const char **argv, const char *prefix)\n {\n \tint quiet = 0;\n@@ -3424,7 +3379,6 @@ static struct cmd_struct commands[] = {\n \t{\"summary\", module_summary, 0},\n \t{\"push-check\", push_check, 0},\n \t{\"absorbgitdirs\", absorb_git_dirs, SUPPORT_SUPER_PREFIX},\n-\t{\"config\", module_config, 0},\n \t{\"set-url\", module_set_url, 0},\n \t{\"set-branch\", module_set_branch, 0},\n \t{\"create-branch\", module_create_branch, 0},\ndiff --git a/t/helper/test-submodule.c b/t/helper/test-submodule.c\nindex b7d117cd557..e060cc62268 100644\n--- a/t/helper/test-submodule.c\n+++ b/t/helper/test-submodule.c\n@@ -111,10 +111,94 @@ static int cmd__submodule_resolve_relative_url(int argc, const char **argv)\n \treturn 0;\n }\n \n+static int cmd__submodule_config_list(int argc, const char **argv)\n+{\n+\tstruct option options[] = {\n+\t\tOPT_END()\n+\t};\n+\tconst char *const usage[] = {\n+\t\t\"test-tool submodule config-list \",\n+\t\tNULL\n+\t};\n+\targc = parse_options(argc, argv, \"test-tools\", options, usage,\n+\t\t\t PARSE_OPT_KEEP_ARGV0);\n+\n+\tsetup_git_directory();\n+\n+\tif (argc == 2)\n+\t\treturn print_config_from_gitmodules(the_repository, argv[1]);\n+\tusage_with_options(usage, options);\n+}\n+\n+static int cmd__submodule_config_set(int argc, const char **argv)\n+{\n+\tstruct option options[] = {\n+\t\tOPT_END()\n+\t};\n+\tconst char *const usage[] = {\n+\t\t\"test-tool submodule config-set \",\n+\t\tNULL\n+\t};\n+\targc = parse_options(argc, argv, \"test-tools\", options, usage,\n+\t\t\t PARSE_OPT_KEEP_ARGV0);\n+\n+\tsetup_git_directory();\n+\n+\t/* Equivalent to ACTION_SET in builtin/config.c */\n+\tif (argc == 3) {\n+\t\tif (!is_writing_gitmodules_ok())\n+\t\t\tdie(\"please make sure that the .gitmodules file is in the working tree\");\n+\n+\t\treturn config_set_in_gitmodules_file_gently(argv[1], argv[2]);\n+\t}\n+\tusage_with_options(usage, options);\n+}\n+\n+static int cmd__submodule_config_unset(int argc, const char **argv)\n+{\n+\tstruct option options[] = {\n+\t\tOPT_END()\n+\t};\n+\tconst char *const usage[] = {\n+\t\t\"test-tool submodule config-unset \",\n+\t\tNULL\n+\t};\n+\n+\tsetup_git_directory();\n+\n+\tif (argc == 2) {\n+\t\tif (!is_writing_gitmodules_ok())\n+\t\t\tdie(\"please make sure that the .gitmodules file is in the working tree\");\n+\t\treturn config_set_in_gitmodules_file_gently(argv[1], NULL);\n+\t}\n+\tusage_with_options(usage, options);\n+}\n+\n+static int cmd__submodule_config_writeable(int argc, const char **argv)\n+{\n+\tstruct option options[] = {\n+\t\tOPT_END()\n+\t};\n+\tconst char *const usage[] = {\n+\t\t\"test-tool submodule config-writeable\",\n+\t\tNULL\n+\t};\n+\tsetup_git_directory();\n+\n+\tif (argc == 1)\n+\t\treturn is_writing_gitmodules_ok() ? 0 : -1;\n+\n+\tusage_with_options(usage, options);\n+}\n+\n static struct test_cmd cmds[] = {\n \t{ \"check-name\", cmd__submodule_check_name },\n \t{ \"is-active\", cmd__submodule_is_active },\n \t{ \"resolve-relative-url\", cmd__submodule_resolve_relative_url},\n+\t{ \"config-list\", cmd__submodule_config_list },\n+\t{ \"config-set\", cmd__submodule_config_set },\n+\t{ \"config-unset\", cmd__submodule_config_unset },\n+\t{ \"config-writeable\", cmd__submodule_config_writeable },\n };\n \n int cmd__submodule(int argc, const char **argv)\ndiff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh\nindex c583c4e373a..c0167944abd 100755\n--- a/t/t7411-submodule-config.sh\n+++ b/t/t7411-submodule-config.sh\n@@ -137,44 +137,44 @@ test_expect_success 'error in history in fetchrecursesubmodule lets continue' '\n \t)\n '\n \n-test_expect_success 'reading submodules config from the working tree with \"submodule--helper config\"' '\n+test_expect_success 'reading submodules config from the working tree' '\n \t(cd super &&\n \t\techo \"../submodule\" >expect &&\n-\t\tgit submodule--helper config submodule.submodule.url >actual &&\n+\t\ttest-tool submodule config-list submodule.submodule.url >actual &&\n \t\ttest_cmp expect actual\n \t)\n '\n \n-test_expect_success 'unsetting submodules config from the working tree with \"submodule--helper config --unset\"' '\n+test_expect_success 'unsetting submodules config from the working tree' '\n \t(cd super &&\n-\t\tgit submodule--helper config --unset submodule.submodule.url &&\n-\t\tgit submodule--helper config submodule.submodule.url >actual &&\n+\t\ttest-tool submodule config-unset submodule.submodule.url &&\n+\t\ttest-tool submodule config-list submodule.submodule.url >actual &&\n \t\ttest_must_be_empty actual\n \t)\n '\n \n \n-test_expect_success 'writing submodules config with \"submodule--helper config\"' '\n+test_expect_success 'writing submodules config' '\n \t(cd super &&\n \t\techo \"new_url\" >expect &&\n-\t\tgit submodule--helper config submodule.submodule.url \"new_url\" &&\n-\t\tgit submodule--helper config submodule.submodule.url >actual &&\n+\t\ttest-tool submodule config-set submodule.submodule.url \"new_url\" &&\n+\t\ttest-tool submodule config-list submodule.submodule.url >actual &&\n \t\ttest_cmp expect actual\n \t)\n '\n \n-test_expect_success 'overwriting unstaged submodules config with \"submodule--helper config\"' '\n+test_expect_success 'overwriting unstaged submodules config' '\n \ttest_when_finished \"git -C super checkout .gitmodules\" &&\n \t(cd super &&\n \t\techo \"newer_url\" >expect &&\n-\t\tgit submodule--helper config submodule.submodule.url \"newer_url\" &&\n-\t\tgit submodule--helper config submodule.submodule.url >actual &&\n+\t\ttest-tool submodule config-set submodule.submodule.url \"newer_url\" &&\n+\t\ttest-tool submodule config-list submodule.submodule.url >actual &&\n \t\ttest_cmp expect actual\n \t)\n '\n \n test_expect_success 'writeable .gitmodules when it is in the working tree' '\n-\tgit -C super submodule--helper config --check-writeable\n+\ttest-tool -C super submodule config-writeable\n '\n \n test_expect_success 'writeable .gitmodules when it is nowhere in the repository' '\n@@ -183,7 +183,7 @@ test_expect_success 'writeable .gitmodules when it is nowhere in the repository'\n \t(cd super &&\n \t\tgit rm .gitmodules &&\n \t\tgit commit -m \"remove .gitmodules from the current branch\" &&\n-\t\tgit submodule--helper config --check-writeable\n+\t\ttest-tool submodule config-writeable\n \t)\n '\n \n@@ -191,7 +191,7 @@ test_expect_success 'non-writeable .gitmodules when it is in the index but not i\n \ttest_when_finished \"git -C super checkout .gitmodules\" &&\n \t(cd super &&\n \t\trm -f .gitmodules &&\n-\t\ttest_must_fail git submodule--helper config --check-writeable\n+\t\ttest_must_fail test-tool submodule config-writeable\n \t)\n '\n \n@@ -200,7 +200,7 @@ test_expect_success 'non-writeable .gitmodules when it is in the current branch\n \ttest_when_finished \"git -C super reset --hard $ORIG\" &&\n \t(cd super &&\n \t\tgit rm .gitmodules &&\n-\t\ttest_must_fail git submodule--helper config --check-writeable\n+\t\ttest_must_fail test-tool submodule config-writeable\n \t)\n '\n \n@@ -208,11 +208,11 @@ test_expect_success 'reading submodules config from the index when .gitmodules i\n \tORIG=$(git -C super rev-parse HEAD) &&\n \ttest_when_finished \"git -C super reset --hard $ORIG\" &&\n \t(cd super &&\n-\t\tgit submodule--helper config submodule.submodule.url \"staged_url\" &&\n+\t\ttest-tool submodule config-set submodule.submodule.url \"staged_url\" &&\n \t\tgit add .gitmodules &&\n \t\trm -f .gitmodules &&\n \t\techo \"staged_url\" >expect &&\n-\t\tgit submodule--helper config submodule.submodule.url >actual &&\n+\t\ttest-tool submodule config-list submodule.submodule.url >actual &&\n \t\ttest_cmp expect actual\n \t)\n '\n@@ -223,7 +223,7 @@ test_expect_success 'reading submodules config from the current branch when .git\n \t(cd super &&\n \t\tgit rm .gitmodules &&\n \t\techo \"../submodule\" >expect &&\n-\t\tgit submodule--helper config submodule.submodule.url >actual &&\n+\t\ttest-tool submodule config-list submodule.submodule.url >actual &&\n \t\ttest_cmp expect actual\n \t)\n '\ndiff --git a/t/t7418-submodule-sparse-gitmodules.sh b/t/t7418-submodule-sparse-gitmodules.sh\nindex d5874200fdc..dde11ecce80 100755\n--- a/t/t7418-submodule-sparse-gitmodules.sh\n+++ b/t/t7418-submodule-sparse-gitmodules.sh\n@@ -50,12 +50,12 @@ test_expect_success 'sparse checkout setup which hides .gitmodules' '\n \n test_expect_success 'reading gitmodules config file when it is not checked out' '\n \techo \"../submodule\" >expect &&\n-\tgit -C super submodule--helper config submodule.submodule.url >actual &&\n+\ttest-tool -C super submodule config-list submodule.submodule.url >actual &&\n \ttest_cmp expect actual\n '\n \n test_expect_success 'not writing gitmodules config file when it is not checked out' '\n-\ttest_must_fail git -C super submodule--helper config submodule.submodule.url newurl &&\n+\ttest_must_fail test-tool -C super submodule config-set submodule.submodule.url newurl &&\n \ttest_path_is_missing super/.gitmodules\n '\n \n-- \n2.38.0.1464.gea6794aacbc\n\n"},{"id":"466825","messageId":"patch-v2-2.9-5508c27f653-20221108T140501Z-avarab@gmail.com","threadId":"58734","inReplyTo":"cover-v2-0.9-00000000000-20221108T140501Z-avarab@gmail.com","subject":"[PATCH v2 2/9] submodule tests: add tests for top-level flag output","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-08T14:10:33Z","receivedAt":"2022-11-08T14:11:04Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Exhaustively test for how combining various \"mixed-level\" \"git\nsubmodule\" option works. \"Mixed-level\" here means options that are\naccepted by a mixture of the top-level \"submodule\" command, and\ne.g. the \"status\" sub-command.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t7400-submodule-basic.sh | 10 +++\n t/t7422-submodule-output.sh | 169 ++++++++++++++++++++++++++++++++++++\n 2 files changed, 179 insertions(+)\n create mode 100755 t/t7422-submodule-output.sh\n\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex a989aafaf57..eae6a46ef3d 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -579,6 +579,16 @@ test_expect_success 'status should be \"modified\" after submodule commit' '\n \tgrep \"^+$rev2\" list\n '\n \n+test_expect_success '\"submodule --cached\" command forms should be identical' '\n+\tgit submodule status --cached >expect &&\n+\n+\tgit submodule --cached >actual &&\n+\ttest_cmp expect actual &&\n+\n+\tgit submodule --cached status >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'the --cached sha1 should be rev1' '\n \tgit submodule --cached status >list &&\n \tgrep \"^+$rev1\" list\ndiff --git a/t/t7422-submodule-output.sh b/t/t7422-submodule-output.sh\nnew file mode 100755\nindex 00000000000..1e9cdf1a68e\n--- /dev/null\n+++ b/t/t7422-submodule-output.sh\n@@ -0,0 +1,169 @@\n+#!/bin/sh\n+\n+test_description='submodule --cached, --quiet etc. output'\n+\n+. ./test-lib.sh\n+. \"$TEST_DIRECTORY\"/lib-t3100.sh\n+\n+setup_sub () {\n+\tlocal d=\"$1\" &&\n+\tshift &&\n+\tgit $@ clone . \"$d\" &&\n+\tgit $@ submodule add ./\"$d\"\n+}\n+\n+normalize_status () {\n+\tsed -e 's/-g[0-9a-f]*/-gHASH/'\n+}\n+\n+test_expect_success 'setup' '\n+\ttest_commit A &&\n+\ttest_commit B &&\n+\tsetup_sub S &&\n+\tsetup_sub S.D &&\n+\tsetup_sub S.C &&\n+\tsetup_sub S.C.D &&\n+\tsetup_sub X &&\n+\tgit add S* &&\n+\ttest_commit C &&\n+\n+\t# recursive in X/\n+\tgit -C X pull &&\n+\tGIT_ALLOW_PROTOCOL=file git -C X submodule update --init &&\n+\n+\t# dirty\n+\tfor d in S.D X/S.D\n+\tdo\n+\t\techo dirty >\"$d\"/A.t || return 1\n+\tdone &&\n+\n+\t# commit (for --cached)\n+\tfor d in S.C* X/S.C*\n+\tdo\n+\t\tgit -C \"$d\" reset --hard A || return 1\n+\tdone &&\n+\n+\t# dirty\n+\tfor d in S*.D X/S*.D\n+\tdo\n+\t\techo dirty >\"$d/C2.t\" || return 1\n+\tdone &&\n+\n+\tfor ref in A B C\n+\tdo\n+\t\t# Not different with SHA-1 and SHA-256, just (ab)using\n+\t\t# test_oid_cache as a variable bag to avoid using\n+\t\t# $(git rev-parse ...).\n+\t\toid=$(git rev-parse $ref) &&\n+\t\ttest_oid_cache <<-EOF || return 1\n+\t\t$ref sha1:$oid\n+\t\t$ref sha256:$oid\n+\t\tEOF\n+\tdone\n+'\n+\n+for opts in \"\" \"status\"\n+do\n+\ttest_expect_success \"git submodule $opts\" '\n+\t\tsed -e \"s/^>//\" >expect <<-EOF &&\n+\t\t> $(test_oid B) S (B)\n+\t\t>+$(test_oid A) S.C (A)\n+\t\t>+$(test_oid A) S.C.D (A)\n+\t\t> $(test_oid B) S.D (B)\n+\t\t>+$(test_oid C) X (C)\n+\t\tEOF\n+\t\tgit submodule $opts >actual.raw &&\n+\t\tnormalize_status actual &&\n+\t\ttest_cmp expect actual\n+\t'\n+done\n+\n+for opts in \\\n+\t\"status --recursive\"\n+do\n+\ttest_expect_success \"git submodule $opts\" '\n+\t\tsed -e \"s/^>//\" >expect <<-EOF &&\n+\t\t> $(test_oid B) S (B)\n+\t\t>+$(test_oid A) S.C (A)\n+\t\t>+$(test_oid A) S.C.D (A)\n+\t\t> $(test_oid B) S.D (B)\n+\t\t>+$(test_oid C) X (C)\n+\t\t> $(test_oid B) X/S (B)\n+\t\t>+$(test_oid A) X/S.C (A)\n+\t\t>+$(test_oid A) X/S.C.D (A)\n+\t\t> $(test_oid B) X/S.D (B)\n+\t\t> $(test_oid B) X/X (B)\n+\t\tEOF\n+\t\tgit submodule $opts >actual.raw &&\n+\t\tnormalize_status actual &&\n+\t\ttest_cmp expect actual\n+\t'\n+done\n+\n+for opts in \\\n+\t\"--quiet\" \\\n+\t\"--quiet status\" \\\n+\t\"status --quiet\"\n+do\n+\ttest_expect_success \"git submodule $opts\" '\n+\t\tgit submodule $opts >out &&\n+\t\ttest_must_be_empty out\n+\t'\n+done\n+\n+for opts in \\\n+\t\"--cached\" \\\n+\t\"--cached status\" \\\n+\t\"status --cached\"\n+do\n+\ttest_expect_success \"git submodule $opts\" '\n+\t\tsed -e \"s/^>//\" >expect <<-EOF &&\n+\t\t> $(test_oid B) S (B)\n+\t\t>+$(test_oid B) S.C (B)\n+\t\t>+$(test_oid B) S.C.D (B)\n+\t\t> $(test_oid B) S.D (B)\n+\t\t>+$(test_oid B) X (B)\n+\t\tEOF\n+\t\tgit submodule $opts >actual.raw &&\n+\t\tnormalize_status actual &&\n+\t\ttest_cmp expect actual\n+\t'\n+done\n+\n+for opts in \\\n+\t\"--cached --quiet\" \\\n+\t\"--cached --quiet status\" \\\n+\t\"--cached status --quiet\" \\\n+\t\"--quiet status --cached\" \\\n+\t\"status --cached --quiet\"\n+do\n+\ttest_expect_success \"git submodule $opts\" '\n+\t\tgit submodule $opts >out &&\n+\t\ttest_must_be_empty out\n+\t'\n+done\n+\n+for opts in \\\n+\t\"status --cached --recursive\" \\\n+\t\"--cached status --recursive\"\n+do\n+\ttest_expect_success \"git submodule $opts\" '\n+\t\tsed -e \"s/^>//\" >expect <<-EOF &&\n+\t\t> $(test_oid B) S (B)\n+\t\t>+$(test_oid B) S.C (B)\n+\t\t>+$(test_oid B) S.C.D (B)\n+\t\t> $(test_oid B) S.D (B)\n+\t\t>+$(test_oid B) X (B)\n+\t\t> $(test_oid B) X/S (B)\n+\t\t>+$(test_oid B) X/S.C (B)\n+\t\t>+$(test_oid B) X/S.C.D (B)\n+\t\t> $(test_oid B) X/S.D (B)\n+\t\t> $(test_oid B) X/X (B)\n+\t\tEOF\n+\t\tgit submodule $opts >actual.raw &&\n+\t\tnormalize_status actual &&\n+\t\ttest_cmp expect actual\n+\t'\n+done\n+\n+test_done\n-- \n2.38.0.1464.gea6794aacbc\n\n"},{"id":"466826","messageId":"patch-v2-3.9-a3529d7f9e0-20221108T140501Z-avarab@gmail.com","threadId":"58734","inReplyTo":"cover-v2-0.9-00000000000-20221108T140501Z-avarab@gmail.com","subject":"[PATCH v2 3/9] submodule--helper: fix a memory leak in \"status\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-08T14:10:34Z","receivedAt":"2022-11-08T14:11:15Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The \"status\" sub-command was leaking the \"struct strvec\" it was\nsetting up for the reasons explained in f92dbdbc6a8 (revisions API:\ndon't leak memory on argv elements that need free()-ing, 2022-08-02),\nso let's use the \"free_removed_argv_elements\" option to\nsetup_revisions() to fix the leak.\n\nEven if we did that, clobbering the \"diff_files_args.nr\" with the\nreturn value of setup_revisions() would leave leaks in place, but we\ncan just stop clobbering it.\n\nEver since that code was added in a9f8a37584a (submodule: port\nsubmodule subcommand 'status' from shell to C, 2017-10-06) we've had\nno reason to modify the \"nr\" member (\"argc\" at the time): The next use\nof \"diff_files_args\" after this is the \"strvec_clear()\" at the end of\nthe function.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/submodule--helper.c | 7 ++++---\n t/t7422-submodule-output.sh | 1 +\n 2 files changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 6250b95a6f7..ee6f2d34cba 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -616,6 +616,9 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,\n \tint diff_files_result;\n \tstruct strbuf buf = STRBUF_INIT;\n \tconst char *git_dir;\n+\tstruct setup_revision_opt opt = {\n+\t\t.free_removed_argv_elements = 1,\n+\t};\n \n \tif (!submodule_from_path(the_repository, null_oid(), path))\n \t\tdie(_(\"no submodule mapping found in .gitmodules for path '%s'\"),\n@@ -649,9 +652,7 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,\n \n \trepo_init_revisions(the_repository, &rev, NULL);\n \trev.abbrev = 0;\n-\tdiff_files_args.nr = setup_revisions(diff_files_args.nr,\n-\t\t\t\t\t diff_files_args.v,\n-\t\t\t\t\t &rev, NULL);\n+\tsetup_revisions(diff_files_args.nr, diff_files_args.v, &rev, &opt);\n \tdiff_files_result = run_diff_files(&rev, 0);\n \n \tif (!diff_result_code(&rev.diffopt, diff_files_result)) {\ndiff --git a/t/t7422-submodule-output.sh b/t/t7422-submodule-output.sh\nindex 1e9cdf1a68e..ab946ec9405 100755\n--- a/t/t7422-submodule-output.sh\n+++ b/t/t7422-submodule-output.sh\n@@ -2,6 +2,7 @@\n \n test_description='submodule --cached, --quiet etc. output'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-t3100.sh\n \n-- \n2.38.0.1464.gea6794aacbc\n\n"},{"id":"466827","messageId":"patch-v2-5.9-459ea25125b-20221108T140501Z-avarab@gmail.com","threadId":"58734","inReplyTo":"cover-v2-0.9-00000000000-20221108T140501Z-avarab@gmail.com","subject":"[PATCH v2 5/9] submodule.c: refactor recursive block out of absorb function","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-08T14:10:36Z","receivedAt":"2022-11-08T14:11:18Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"A move and indentation-only change to move the\nABSORB_GITDIR_RECURSE_SUBMODULES case into its own function, which as\nwe'll see makes the subsequent commit changing this code much smaller.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n submodule.c | 30 ++++++++++++++++++------------\n 1 file changed, 18 insertions(+), 12 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex b958162d286..fe1e3f03905 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -2310,6 +2310,23 @@ static void relocate_single_git_dir_into_superproject(const char *path)\n \tstrbuf_release(&new_gitdir);\n }\n \n+static void absorb_git_dir_into_superproject_recurse(const char *path)\n+{\n+\n+\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\n+\tcp.dir = path;\n+\tcp.git_cmd = 1;\n+\tcp.no_stdin = 1;\n+\tstrvec_pushf(&cp.args, \"--super-prefix=%s%s/\",\n+\t\t get_super_prefix_or_empty(), path);\n+\tstrvec_pushl(&cp.args, \"submodule--helper\",\n+\t\t \"absorbgitdirs\", NULL);\n+\tprepare_submodule_repo_env(&cp.env);\n+\tif (run_command(&cp))\n+\t\tdie(_(\"could not recurse into submodule '%s'\"), path);\n+}\n+\n /*\n * Migrate the git directory of the submodule given by path from\n * having its git directory within the working tree to the git dir nested\n@@ -2366,21 +2383,10 @@ void absorb_git_dir_into_superproject(const char *path,\n \tstrbuf_release(&gitdir);\n \n \tif (flags & ABSORB_GITDIR_RECURSE_SUBMODULES) {\n-\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n-\n \t\tif (flags & ~ABSORB_GITDIR_RECURSE_SUBMODULES)\n \t\t\tBUG(\"we don't know how to pass the flags down?\");\n \n-\t\tcp.dir = path;\n-\t\tcp.git_cmd = 1;\n-\t\tcp.no_stdin = 1;\n-\t\tstrvec_pushf(&cp.args, \"--super-prefix=%s%s/\",\n-\t\t\t get_super_prefix_or_empty(), path);\n-\t\tstrvec_pushl(&cp.args, \"submodule--helper\",\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+\t\tabsorb_git_dir_into_superproject_recurse(path);\n \t}\n }\n \n-- \n2.38.0.1464.gea6794aacbc\n\n"},{"id":"466828","messageId":"patch-v2-6.9-322a02c30fc-20221108T140501Z-avarab@gmail.com","threadId":"58734","inReplyTo":"cover-v2-0.9-00000000000-20221108T140501Z-avarab@gmail.com","subject":"[PATCH v2 6/9] submodule API & \"absorbgitdirs\": remove \"----recursive\" option","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-08T14:10:37Z","receivedAt":"2022-11-08T14:11:20Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Remove the \"----recursive\" option to \"git submodule--helper\nabsorbgitdirs\" (yes, with 4 dashes, not 2).\n\nThis option and all the \"else\" when \"flags &\nABSORB_GITDIR_RECURSE_SUBMODULES\" is false has never been used since\nit was added in f6f85861400 (submodule: add absorb-git-dir function,\n2016-12-12), which we'd have had to do as \"----recursive\", a\n\"--recursive\" would have errored out.\n\nIt would be nice to follow-up with an optbug() assertion to\nparse-options.c for such funnily named options, I manually validated\nthat this was the only long option whose name started with \"-\", but\nlet's skip adding such an assertion for now.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/rm.c | 3 +--\n builtin/submodule--helper.c | 8 ++------\n submodule.c | 13 +++----------\n submodule.h | 4 +---\n 4 files changed, 7 insertions(+), 21 deletions(-)\n\ndiff --git a/builtin/rm.c b/builtin/rm.c\nindex f0d025a4e23..05bfe20a469 100644\n--- a/builtin/rm.c\n+++ b/builtin/rm.c\n@@ -86,8 +86,7 @@ static void submodules_absorb_gitdir_if_needed(void)\n \t\t\tcontinue;\n \n \t\tif (!submodule_uses_gitfile(name))\n-\t\t\tabsorb_git_dir_into_superproject(name,\n-\t\t\t\tABSORB_GITDIR_RECURSE_SUBMODULES);\n+\t\t\tabsorb_git_dir_into_superproject(name);\n \t}\n }\n \ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex ee6f2d34cba..33f099dbc86 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1379,8 +1379,7 @@ static void deinit_submodule(const char *path, const char *prefix,\n \t\t\t\t\t \".git file by using absorbgitdirs.\"),\n \t\t\t\t\tdisplaypath);\n \n-\t\t\tabsorb_git_dir_into_superproject(path,\n-\t\t\t\t\t\t\t ABSORB_GITDIR_RECURSE_SUBMODULES);\n+\t\t\tabsorb_git_dir_into_superproject(path);\n \n \t\t}\n \n@@ -2831,13 +2830,10 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)\n \tint i;\n \tstruct pathspec pathspec = { 0 };\n \tstruct module_list list = MODULE_LIST_INIT;\n-\tunsigned flags = ABSORB_GITDIR_RECURSE_SUBMODULES;\n \tstruct option embed_gitdir_options[] = {\n \t\tOPT_STRING(0, \"prefix\", &prefix,\n \t\t\t N_(\"path\"),\n \t\t\t N_(\"path into the working tree\")),\n-\t\tOPT_BIT(0, \"--recursive\", &flags, N_(\"recurse into submodules\"),\n-\t\t\tABSORB_GITDIR_RECURSE_SUBMODULES),\n \t\tOPT_END()\n \t};\n \tconst char *const git_submodule_helper_usage[] = {\n@@ -2853,7 +2849,7 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)\n \t\tgoto cleanup;\n \n \tfor (i = 0; i < list.nr; i++)\n-\t\tabsorb_git_dir_into_superproject(list.entries[i]->name, flags);\n+\t\tabsorb_git_dir_into_superproject(list.entries[i]->name);\n \n \tret = 0;\n cleanup:\ndiff --git a/submodule.c b/submodule.c\nindex fe1e3f03905..8fa2ad457b2 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -2139,8 +2139,7 @@ int submodule_move_head(const char *path,\n \tif (!(flags & SUBMODULE_MOVE_HEAD_DRY_RUN)) {\n \t\tif (old_head) {\n \t\t\tif (!submodule_uses_gitfile(path))\n-\t\t\t\tabsorb_git_dir_into_superproject(path,\n-\t\t\t\t\tABSORB_GITDIR_RECURSE_SUBMODULES);\n+\t\t\t\tabsorb_git_dir_into_superproject(path);\n \t\t} else {\n \t\t\tstruct strbuf gitdir = STRBUF_INIT;\n \t\t\tsubmodule_name_to_gitdir(&gitdir, the_repository,\n@@ -2332,8 +2331,7 @@ static void absorb_git_dir_into_superproject_recurse(const char *path)\n * having its git directory within the working tree to the git dir nested\n * in its superprojects git dir under modules/.\n */\n-void absorb_git_dir_into_superproject(const char *path,\n-\t\t\t\t unsigned flags)\n+void absorb_git_dir_into_superproject(const char *path)\n {\n \tint err_code;\n \tconst char *sub_git_dir;\n@@ -2382,12 +2380,7 @@ void absorb_git_dir_into_superproject(const char *path,\n \t}\n \tstrbuf_release(&gitdir);\n \n-\tif (flags & ABSORB_GITDIR_RECURSE_SUBMODULES) {\n-\t\tif (flags & ~ABSORB_GITDIR_RECURSE_SUBMODULES)\n-\t\t\tBUG(\"we don't know how to pass the flags down?\");\n-\n-\t\tabsorb_git_dir_into_superproject_recurse(path);\n-\t}\n+\tabsorb_git_dir_into_superproject_recurse(path);\n }\n \n int get_superproject_working_tree(struct strbuf *buf)\ndiff --git a/submodule.h b/submodule.h\nindex 6a9fec6de11..b52a4ff1e73 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -164,9 +164,7 @@ void submodule_unset_core_worktree(const struct submodule *sub);\n */\n void prepare_submodule_repo_env(struct strvec *env);\n \n-#define ABSORB_GITDIR_RECURSE_SUBMODULES (1<<0)\n-void absorb_git_dir_into_superproject(const char *path,\n-\t\t\t\t unsigned flags);\n+void absorb_git_dir_into_superproject(const char *path);\n \n /*\n * Return the absolute path of the working tree of the superproject, which this\n-- \n2.38.0.1464.gea6794aacbc\n\n"},{"id":"466829","messageId":"patch-v2-7.9-d1f4ac20a4f-20221108T140501Z-avarab@gmail.com","threadId":"58734","inReplyTo":"cover-v2-0.9-00000000000-20221108T140501Z-avarab@gmail.com","subject":"[PATCH v2 7/9] submodule--helper: remove --prefix from \"absorbgitdirs\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-08T14:10:38Z","receivedAt":"2022-11-08T14:11:22Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Let's pass the \"-C \" option instead to \"absorbgitdirs\" from\nits only caller.\n\nWhen it was added in f6f85861400 (submodule: add absorb-git-dir\nfunction, 2016-12-12) there were other \"submodule--helper\" subcommands\nthat were invoked with \"-C \", so we could have done this all\nalong.\n\nSuggested-by: Glen Choo \nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/submodule--helper.c | 3 ---\n git-submodule.sh | 2 +-\n 2 files changed, 1 insertion(+), 4 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 33f099dbc86..fefedcf8097 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2831,9 +2831,6 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)\n \tstruct pathspec pathspec = { 0 };\n \tstruct module_list list = MODULE_LIST_INIT;\n \tstruct option embed_gitdir_options[] = {\n-\t\tOPT_STRING(0, \"prefix\", &prefix,\n-\t\t\t N_(\"path\"),\n-\t\t\t N_(\"path into the working tree\")),\n \t\tOPT_END()\n \t};\n \tconst char *const git_submodule_helper_usage[] = {\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 5e5d21c010f..d359f171379 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -557,7 +557,7 @@ cmd_sync()\n \n cmd_absorbgitdirs()\n {\n-\tgit submodule--helper absorbgitdirs --prefix \"$wt_prefix\" \"$@\"\n+\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper absorbgitdirs \"$@\"\n }\n \n # This loop parses the command line arguments to find the\n-- \n2.38.0.1464.gea6794aacbc\n\n"},{"id":"466830","messageId":"patch-v2-4.9-c14cc0e14b8-20221108T140501Z-avarab@gmail.com","threadId":"58734","inReplyTo":"cover-v2-0.9-00000000000-20221108T140501Z-avarab@gmail.com","subject":"[PATCH v2 4/9] submodule tests: test for a \"foreach\" blind-spot","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-08T14:10:35Z","receivedAt":"2022-11-08T14:11:28Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"We tested for \"--\" followed by command names, but not for \"--\"\nfollowed by an argument that looks like an option, let's do that.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t7407-submodule-foreach.sh | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh\nindex 59bd1501667..8d7b234beb8 100755\n--- a/t/t7407-submodule-foreach.sh\n+++ b/t/t7407-submodule-foreach.sh\n@@ -154,6 +154,11 @@ test_expect_success 'use \"submodule foreach\" to checkout 2nd level submodule' '\n \t)\n '\n \n+test_expect_success 'usage: foreach -- --not-an-option' '\n+\ttest_expect_code 1 git submodule foreach -- --not-an-option &&\n+\ttest_expect_code 1 git -C clone2 submodule foreach -- --not-an-option\n+'\n+\n test_expect_success 'use \"foreach --recursive\" to checkout all submodules' '\n \t(\n \t\tcd clone2 &&\n-- \n2.38.0.1464.gea6794aacbc\n\n"},{"id":"466831","messageId":"patch-v2-8.9-ac9ff05ef68-20221108T140501Z-avarab@gmail.com","threadId":"58734","inReplyTo":"cover-v2-0.9-00000000000-20221108T140501Z-avarab@gmail.com","subject":"[PATCH v2 8/9] submodule--helper: drop \"update --prefix \" for \"-C update\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-08T14:10:39Z","receivedAt":"2022-11-08T14:11:32Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Since 29a5e9e1ffe (submodule--helper update-clone: learn --init,\n2022-03-04) we've been passing \"-C \" from \"git-submodule.sh\"\nwhenever we pass \"--prefix \", so the latter is redundant to\nthe former. Let's drop the \"--prefix\" option.\n\nSuggested-by: Glen Choo \nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/submodule--helper.c | 4 +---\n git-submodule.sh | 1 -\n 2 files changed, 1 insertion(+), 4 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex fefedcf8097..03d1b58acaa 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2643,9 +2643,6 @@ static int module_update(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"traverse submodules recursively\")),\n \t\tOPT_BOOL('N', \"no-fetch\", &opt.nofetch,\n \t\t\t N_(\"don't fetch new objects from the remote site\")),\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_SET_INT(0, \"checkout\", &opt.update_default,\n \t\t\tN_(\"use the 'checkout' update strategy (default)\"),\n \t\t\tSM_UPDATE_CHECKOUT),\n@@ -2701,6 +2698,7 @@ static int module_update(int argc, const char **argv, const char *prefix)\n \t}\n \n \topt.filter_options = &filter_options;\n+\topt.prefix = prefix;\n \n \tif (opt.update_default)\n \t\topt.update_strategy.type = opt.update_default;\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex d359f171379..9a50f2e9124 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -343,7 +343,6 @@ cmd_update()\n \t\t${recursive:+--recursive} \\\n \t\t${init:+--init} \\\n \t\t${nofetch:+--no-fetch} \\\n-\t\t${wt_prefix:+--prefix \"$wt_prefix\"} \\\n \t\t${rebase:+--rebase} \\\n \t\t${merge:+--merge} \\\n \t\t${checkout:+--checkout} \\\n-- \n2.38.0.1464.gea6794aacbc\n\n"},{"id":"466832","messageId":"patch-v2-9.9-cccd92a829c-20221108T140501Z-avarab@gmail.com","threadId":"58734","inReplyTo":"cover-v2-0.9-00000000000-20221108T140501Z-avarab@gmail.com","subject":"[PATCH v2 9/9] submodule--helper: use OPT_SUBCOMMAND() API","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-08T14:10:40Z","receivedAt":"2022-11-08T14:11:32Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Have the cmd_submodule__helper() use the OPT_SUBCOMMAND() API\nintroduced in fa83cc834da (parse-options: add support for parsing\nsubcommands, 2022-08-19).\n\nThis is only a marginal reduction in line count, but once we start\nunifying this with a yet-to-be-added \"builtin/submodule.c\" it'll be\nmuch easier to reason about those changes, as they'll both use\nOPT_SUBCOMMAND().\n\nWe don't need to worry about \"argv[0]\" being NULL in the die() because\nwe'd have errored out in parse_options() as we're not using\n\"PARSE_OPT_SUBCOMMAND_OPTIONAL\".\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/submodule--helper.c | 78 ++++++++++++++++++-------------------\n git.c | 2 +-\n 2 files changed, 39 insertions(+), 41 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 03d1b58acaa..c75e9e86b06 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -3351,47 +3351,45 @@ static int module_add(int argc, const char **argv, const char *prefix)\n \treturn ret;\n }\n \n-#define SUPPORT_SUPER_PREFIX (1<<0)\n-\n-struct cmd_struct {\n-\tconst char *cmd;\n-\tint (*fn)(int, const char **, const char *);\n-\tunsigned option;\n-};\n-\n-static struct cmd_struct commands[] = {\n-\t{\"clone\", module_clone, SUPPORT_SUPER_PREFIX},\n-\t{\"add\", module_add, 0},\n-\t{\"update\", module_update, SUPPORT_SUPER_PREFIX},\n-\t{\"foreach\", module_foreach, 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, 0},\n-\t{\"push-check\", push_check, 0},\n-\t{\"absorbgitdirs\", absorb_git_dirs, SUPPORT_SUPER_PREFIX},\n-\t{\"set-url\", module_set_url, 0},\n-\t{\"set-branch\", module_set_branch, 0},\n-\t{\"create-branch\", module_create_branch, 0},\n-};\n-\n int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\n {\n-\tint i;\n-\tif (argc < 2 || !strcmp(argv[1], \"-h\"))\n-\t\tusage(\"git submodule--helper \");\n-\n-\tfor (i = 0; i < ARRAY_SIZE(commands); i++) {\n-\t\tif (!strcmp(argv[1], commands[i].cmd)) {\n-\t\t\tif (get_super_prefix() &&\n-\t\t\t !(commands[i].option & SUPPORT_SUPER_PREFIX))\n-\t\t\t\tdie(_(\"%s doesn't support --super-prefix\"),\n-\t\t\t\t commands[i].cmd);\n-\t\t\treturn commands[i].fn(argc - 1, argv + 1, prefix);\n-\t\t}\n-\t}\n+\tconst char *cmd = argv[0];\n+\tconst char *subcmd;\n+\tparse_opt_subcommand_fn *fn = NULL;\n+\tconst char *const usage[] = {\n+\t\tN_(\"git submodule--helper \"),\n+\t\tNULL\n+\t};\n+\tstruct option options[] = {\n+\t\tOPT_SUBCOMMAND(\"clone\", &fn, module_clone),\n+\t\tOPT_SUBCOMMAND(\"add\", &fn, module_add),\n+\t\tOPT_SUBCOMMAND(\"update\", &fn, module_update),\n+\t\tOPT_SUBCOMMAND(\"foreach\", &fn, module_foreach),\n+\t\tOPT_SUBCOMMAND(\"init\", &fn, module_init),\n+\t\tOPT_SUBCOMMAND(\"status\", &fn, module_status),\n+\t\tOPT_SUBCOMMAND(\"sync\", &fn, module_sync),\n+\t\tOPT_SUBCOMMAND(\"deinit\", &fn, module_deinit),\n+\t\tOPT_SUBCOMMAND(\"summary\", &fn, module_summary),\n+\t\tOPT_SUBCOMMAND(\"push-check\", &fn, push_check),\n+\t\tOPT_SUBCOMMAND(\"absorbgitdirs\", &fn, absorb_git_dirs),\n+\t\tOPT_SUBCOMMAND(\"set-url\", &fn, module_set_url),\n+\t\tOPT_SUBCOMMAND(\"set-branch\", &fn, module_set_branch),\n+\t\tOPT_SUBCOMMAND(\"create-branch\", &fn, module_create_branch),\n+\t\tOPT_END()\n+\t};\n+\targc = parse_options(argc, argv, prefix, options, usage, 0);\n+\tsubcmd = argv[0];\n+\n+\tif (strcmp(subcmd, \"clone\") && strcmp(subcmd, \"update\") &&\n+\t strcmp(subcmd, \"foreach\") && strcmp(subcmd, \"status\") &&\n+\t strcmp(subcmd, \"sync\") && strcmp(subcmd, \"absorbgitdirs\") &&\n+\t get_super_prefix())\n+\t\t/*\n+\t\t * xstrfmt() rather than \"%s %s\" to keep the translated\n+\t\t * string identical to git.c's.\n+\t\t */\n+\t\tdie(_(\"%s doesn't support --super-prefix\"),\n+\t\t xstrfmt(\"'%s %s'\", cmd, subcmd));\n \n-\tdie(_(\"'%s' is not a valid submodule--helper \"\n-\t \"subcommand\"), argv[1]);\n+\treturn fn(argc, argv, prefix);\n }\ndiff --git a/git.c b/git.c\nindex ee7758dcb0e..fb69e605912 100644\n--- a/git.c\n+++ b/git.c\n@@ -610,7 +610,7 @@ static struct cmd_struct commands[] = {\n \t{ \"stash\", cmd_stash, RUN_SETUP | NEED_WORK_TREE },\n \t{ \"status\", cmd_status, RUN_SETUP | NEED_WORK_TREE },\n \t{ \"stripspace\", cmd_stripspace },\n-\t{ \"submodule--helper\", cmd_submodule__helper, RUN_SETUP | SUPPORT_SUPER_PREFIX | NO_PARSEOPT },\n+\t{ \"submodule--helper\", cmd_submodule__helper, RUN_SETUP | SUPPORT_SUPER_PREFIX },\n \t{ \"switch\", cmd_switch, RUN_SETUP | NEED_WORK_TREE },\n \t{ \"symbolic-ref\", cmd_symbolic_ref, RUN_SETUP },\n \t{ \"tag\", cmd_tag, RUN_SETUP | DELAY_PAGER_CONFIG },\n-- \n2.38.0.1464.gea6794aacbc\n\n"},{"id":"466871","messageId":"kl6lh6z947ud.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"58734","inReplyTo":"cover-v2-0.9-00000000000-20221108T140501Z-avarab@gmail.com","subject":"Re: [PATCH v2 0/9] submodule: tests, cleanup to prepare for built-in","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-11-08T18:30:34Z","receivedAt":"2022-11-08T18:30:40Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Changes since v1:\n>\n> * Noted the history of when \"submodule--helper config\" was last used,\n> per Glen's comment.\n> * Re-titled the tests htat no longer use the now-removed\n> \"submodule--helper config\", per Glen's comment.\n> * I removed _() from a string in what's left of \"submodule--helper\n> config\", which is now a test helper. We should not be translating\n> test helper strings.\n> * Fixed a buggy for-loop in the t7422-submodule-output.sh test I'm\n> adding.\n> * Fixed a trivial typo in a comment in that test & in another nearby\n> comment.\n>\n\n[...]\n\n> Range-diff against v1:\n> 1: b2649f96715 ! 1: ca8f8b4bf63 submodule--helper: move \"config\" to a test-tool\n> @@ Commit message\n> 'ab/submodule-helper-prep', 2022-09-13) the \"config\" sub-command was\n> only used by our own tests.\n> \n> + It was last used by \"git submodule\" itself in code that went away with\n> + a6226fd772b (submodule--helper: convert the bulk of cmd_add() to C,\n> + 2021-08-10).\n> +\n> Let's move it over, and while doing so make it easier to reason about\n> by splitting up the various uses for it into separate sub-commands, so\n> that we don't need to count arguments to see what it does.\n> \n> + This also has the advantage that we stop wasting future translator\n> + time on this command, currently the usage information for this\n> + internal-only tool has been translated into several languages. The use\n> + of the \"_\" function has also been removed from the \"please make\n> + sure...\" message.\n> +\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> \n> ## builtin/submodule--helper.c ##\n> @@ t/helper/test-submodule.c: static int cmd__submodule_resolve_relative_url(int ar\n> +\t/* Equivalent to ACTION_SET in builtin/config.c */\n> +\tif (argc == 3) {\n> +\t\tif (!is_writing_gitmodules_ok())\n> -+\t\t\tdie(_(\"please make sure that the .gitmodules file is in the working tree\"));\n> ++\t\t\tdie(\"please make sure that the .gitmodules file is in the working tree\");\n> +\n> +\t\treturn config_set_in_gitmodules_file_gently(argv[1], argv[2]);\n> +\t}\n> @@ t/helper/test-submodule.c: static int cmd__submodule_resolve_relative_url(int ar\n> +\n> +\tif (argc == 2) {\n> +\t\tif (!is_writing_gitmodules_ok())\n> -+\t\t\tdie(_(\"please make sure that the .gitmodules file is in the working tree\"));\n> ++\t\t\tdie(\"please make sure that the .gitmodules file is in the working tree\");\n> +\t\treturn config_set_in_gitmodules_file_gently(argv[1], NULL);\n> +\t}\n> +\tusage_with_options(usage, options);\n> @@ t/helper/test-submodule.c: static int cmd__submodule_resolve_relative_url(int ar\n> \n> ## t/t7411-submodule-config.sh ##\n> @@ t/t7411-submodule-config.sh: test_expect_success 'error in history in fetchrecursesubmodule lets continue' '\n> - test_expect_success 'reading submodules config from the working tree with \"submodule--helper config\"' '\n> + \t)\n> + '\n> + \n> +-test_expect_success 'reading submodules config from the working tree with \"submodule--helper config\"' '\n> ++test_expect_success 'reading submodules config from the working tree' '\n> \t(cd super &&\n> \t\techo \"../submodule\" >expect &&\n> -\t\tgit submodule--helper config submodule.submodule.url >actual &&\n> @@ t/t7411-submodule-config.sh: test_expect_success 'error in history in fetchrecur\n> \t)\n> '\n> \n> - test_expect_success 'unsetting submodules config from the working tree with \"submodule--helper config --unset\"' '\n> +-test_expect_success 'unsetting submodules config from the working tree with \"submodule--helper config --unset\"' '\n> ++test_expect_success 'unsetting submodules config from the working tree' '\n> \t(cd super &&\n> -\t\tgit submodule--helper config --unset submodule.submodule.url &&\n> -\t\tgit submodule--helper config submodule.submodule.url >actual &&\n> @@ t/t7411-submodule-config.sh: test_expect_success 'error in history in fetchrecur\n> \t\ttest_must_be_empty actual\n> \t)\n> '\n> -@@ t/t7411-submodule-config.sh: test_expect_success 'unsetting submodules config from the working tree with \"sub\n> - test_expect_success 'writing submodules config with \"submodule--helper config\"' '\n> + \n> + \n> +-test_expect_success 'writing submodules config with \"submodule--helper config\"' '\n> ++test_expect_success 'writing submodules config' '\n> \t(cd super &&\n> \t\techo \"new_url\" >expect &&\n> -\t\tgit submodule--helper config submodule.submodule.url \"new_url\" &&\n> @@ t/t7411-submodule-config.sh: test_expect_success 'unsetting submodules config fr\n> \t\ttest_cmp expect actual\n> \t)\n> '\n> -@@ t/t7411-submodule-config.sh: test_expect_success 'overwriting unstaged submodules config with \"submodule--hel\n> + \n> +-test_expect_success 'overwriting unstaged submodules config with \"submodule--helper config\"' '\n> ++test_expect_success 'overwriting unstaged submodules config' '\n> \ttest_when_finished \"git -C super checkout .gitmodules\" &&\n> \t(cd super &&\n> \t\techo \"newer_url\" >expect &&\n> 2: cda36b5b6e0 ! 2: 5508c27f653 submodule tests: add tests for top-level flag output\n> @@ t/t7422-submodule-output.sh (new)\n> +\tgit -C X pull &&\n> +\tGIT_ALLOW_PROTOCOL=file git -C X submodule update --init &&\n> +\n> -+\t# dirty p\n> ++\t# dirty\n> +\tfor d in S.D X/S.D\n> +\tdo\n> -+\t\techo dirty >S.D/A.t || return 1\n> ++\t\techo dirty >\"$d\"/A.t || return 1\n> +\tdone &&\n> +\n> +\t# commit (for --cached)\n> @@ t/t7422-submodule-output.sh (new)\n> +\n> +\tfor ref in A B C\n> +\tdo\n> -+\t\t# Not different with SHA-1 and SHA-256, just (ab)usign\n> ++\t\t# Not different with SHA-1 and SHA-256, just (ab)using\n> +\t\t# test_oid_cache as a variable bag to avoid using\n> +\t\t# $(git rev-parse ...).\n> +\t\toid=$(git rev-parse $ref) &&\n> -: ----------- > 3: a3529d7f9e0 submodule--helper: fix a memory leak in \"status\"\n> 3: 0ed1fc7fdf8 = 4: c14cc0e14b8 submodule tests: test for a \"foreach\" blind-spot\n> 4: f7adfbc13ae = 5: 459ea25125b submodule.c: refactor recursive block out of absorb function\n> 5: 2b8afd73b9b = 6: 322a02c30fc submodule API & \"absorbgitdirs\": remove \"----recursive\" option\n> 6: 91208241070 = 7: d1f4ac20a4f submodule--helper: remove --prefix from \"absorbgitdirs\"\n> 7: 77d4d5a6c09 = 8: ac9ff05ef68 submodule--helper: drop \"update --prefix \" for \"-C update\"\n> 8: 105853cd358 = 9: cccd92a829c submodule--helper: use OPT_SUBCOMMAND() API\n\nThanks! This addresses all of my comments from v1, and I didn't \nspot any other issues during a quick 2nd pass.\n\n Reviewed-by: Glen Choo \n"},{"id":"466872","messageId":"221108.86v8npqonn.gmgdl@evledraar.gmail.com","threadId":"58734","inReplyTo":"kl6lh6z947ud.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH v2 0/9] submodule: tests, cleanup to prepare for built-in","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-08T18:34:08Z","receivedAt":"2022-11-08T18:36:36Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Nov 08 2022, Glen Choo wrote:\n\n> [...]\n> Thanks! This addresses all of my comments from v1, and I didn't \n> spot any other issues during a quick 2nd pass.\n>\n> Reviewed-by: Glen Choo \n\nThanks for the quick review!\n\nOne thing I completely forgot in the v2 CL: The 3/9 here is new. Stictly\nspeaking we could skip it, but I wanted to be able to mark the new test\nI added as as leak-free, and fixing the only leak it ran into was\ntrivial:\nhttps://lore.kernel.org/git/patch-v2-3.9-a3529d7f9e0-20221108T140501Z-avarab@gmail.com/\n\nSo rather than having another \"small leak fix for submodules\" topic\ndepend on this topic, I think it's OK to just bundle that up with this\none.\n"},{"id":"466885","messageId":"kl6ledud45jk.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"58734","inReplyTo":"221108.86v8npqonn.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 0/9] submodule: tests, cleanup to prepare for built-in","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-11-08T19:20:15Z","receivedAt":"2022-11-08T19:20:21Z","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 Tue, Nov 08 2022, Glen Choo wrote:\n>\n>> [...]\n>> Thanks! This addresses all of my comments from v1, and I didn't \n>> spot any other issues during a quick 2nd pass.\n>>\n>> Reviewed-by: Glen Choo \n>\n> Thanks for the quick review!\n>\n> One thing I completely forgot in the v2 CL: The 3/9 here is new. Stictly\n> speaking we could skip it, but I wanted to be able to mark the new test\n> I added as as leak-free, and fixing the only leak it ran into was\n> trivial:\n> https://lore.kernel.org/git/patch-v2-3.9-a3529d7f9e0-20221108T140501Z-avarab@gmail.com/\n>\n> So rather than having another \"small leak fix for submodules\" topic\n> depend on this topic, I think it's OK to just bundle that up with this\n> one.\n\nYes, it looked quite trivial. IMO bundling this was a more efficient use\nof time than sending another series.\n"}]}