{"thread":{"id":"58637","subject":"[PATCH 01/10] git-submodule.sh: create a \"case\" dispatch statement","startedAt":"2022-10-17T12:09:38Z","lastAt":"2022-10-20T23:19:03Z","messageCount":18,"participants":["Ævar Arnfjörð Bjarmason","Glen Choo"],"isPatch":true,"patchVersion":1,"patchTotal":10},"messages":[{"id":"465074","messageId":"patch-01.10-2379d5dc0e0-20221017T115544Z-avarab@gmail.com","threadId":"58637","inReplyTo":"cover-00.10-00000000000-20221017T115544Z-avarab@gmail.com","subject":"[PATCH 01/10] git-submodule.sh: create a \"case\" dispatch statement","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-17T12:09:16Z","receivedAt":"2022-10-17T12:09:38Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Create a \"case/esac\" dispatch statement at the end of git-submodule.sh\nand move the contents of the trivial cmd_absorbgitdirs() function to\nit.\n\nThis template will be expanded on in subsequent commits, but for now\nwe're moving the trivial \"git submodule absorb-git-dirs\" to it.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n git-submodule.sh | 14 ++++++++------\n 1 file changed, 8 insertions(+), 6 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 5e5d21c010f..b851d64aa62 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -555,11 +555,6 @@ cmd_sync()\n \tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper sync ${quiet:+--quiet} ${recursive:+--recursive} -- \"$@\"\n }\n \n-cmd_absorbgitdirs()\n-{\n-\tgit submodule--helper absorbgitdirs --prefix \"$wt_prefix\" \"$@\"\n-}\n-\n # This loop parses the command line arguments to find the\n # subcommand name to dispatch. Parsing of the subcommand specific\n # options are primarily done by the subcommand implementations.\n@@ -608,4 +603,11 @@ then\n \tusage\n fi\n \n-\"cmd_$(echo $command | sed -e s/-/_/g)\" \"$@\"\n+case \"$command\" in\n+absorbgitdirs)\n+\tgit submodule--helper \"$command\" --prefix \"$wt_prefix\" \"$@\"\n+\t;;\n+*)\n+\t\"cmd_$(echo $command | sed -e s/-/_/g)\" \"$@\"\n+\t;;\n+esac\n-- \n2.38.0.1091.gf9d18265e59\n\n"},{"id":"465075","messageId":"patch-02.10-46bf600820b-20221017T115544Z-avarab@gmail.com","threadId":"58637","inReplyTo":"cover-00.10-00000000000-20221017T115544Z-avarab@gmail.com","subject":"[PATCH 02/10] git-submodule.sh: dispatch \"sync\" to helper","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-17T12:09:17Z","receivedAt":"2022-10-17T12:09:42Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Remove the cmd_sync() wrapper for \"git submodule--helper sync\" in\nfavor of dispatching the raw command-line directly to the helper.\n\nAt this point we've already parsed out the optional \"--quiet\" flag\nthat we need to support for \"git submodule --quiet\" (as opposed to\n\"git submodule --quiet\").\n\nThis changes the output we'll display on invalid usage for the better,\nbefore this we'd emit e.g.:\n\n\t$ git submodule sync --blah\n\tusage: git submodule [--quiet] [--cached]\n\t or: [...many lines of \"or\" usage omitted...]\n\nBut now we'll emit the much more useful:\n\n\t$ git submodule sync --blah\n\terror: unknown option `blah'\n\tusage: git submodule sync [--quiet] [--recursive] []\n\n\t -q, --quiet suppress output of synchronizing submodule url\n\t --recursive recurse into nested submodules\n\nThis is because we'll now get as far as module_sync()'s failing call\nto parse_options() when we have invalid usage.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n git-submodule.sh | 37 ++++---------------------------------\n 1 file changed, 4 insertions(+), 33 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex b851d64aa62..3fdfe864d37 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -521,39 +521,6 @@ cmd_status()\n \n \tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper status ${quiet:+--quiet} ${cached:+--cached} ${recursive:+--recursive} -- \"$@\"\n }\n-#\n-# Sync remote urls for submodules\n-# This makes the value for remote.$remote.url match the value\n-# specified in .gitmodules.\n-#\n-cmd_sync()\n-{\n-\twhile test $# -ne 0\n-\tdo\n-\t\tcase \"$1\" in\n-\t\t-q|--quiet)\n-\t\t\tquiet=1\n-\t\t\tshift\n-\t\t\t;;\n-\t\t--recursive)\n-\t\t\trecursive=1\n-\t\t\tshift\n-\t\t\t;;\n-\t\t--)\n-\t\t\tshift\n-\t\t\tbreak\n-\t\t\t;;\n-\t\t-*)\n-\t\t\tusage\n-\t\t\t;;\n-\t\t*)\n-\t\t\tbreak\n-\t\t\t;;\n-\t\tesac\n-\tdone\n-\n-\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper sync ${quiet:+--quiet} ${recursive:+--recursive} -- \"$@\"\n-}\n \n # This loop parses the command line arguments to find the\n # subcommand name to dispatch. Parsing of the subcommand specific\n@@ -607,6 +574,10 @@ case \"$command\" in\n absorbgitdirs)\n \tgit submodule--helper \"$command\" --prefix \"$wt_prefix\" \"$@\"\n \t;;\n+sync)\n+\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper \"$command\" \\\n+\t\t${quiet:+--quiet} \"$@\"\n+\t;;\n *)\n \t\"cmd_$(echo $command | sed -e s/-/_/g)\" \"$@\"\n \t;;\n-- \n2.38.0.1091.gf9d18265e59\n\n"},{"id":"465077","messageId":"cover-00.10-00000000000-20221017T115544Z-avarab@gmail.com","threadId":"58637","inReplyTo":null,"subject":"[PATCH 00/10] submodule: make it a built-in, remove git-submodule.sh","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-17T12:09:15Z","receivedAt":"2022-10-17T12:09:44Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On \"master\" any special behavior in git-submodule.sh itself has almost\nentirely gone away, it's a trivial shimmy layer that dispatches to the\nbuilt-in \"submodule--helper\" command.\n\nThis series takes the logical next step and removes \"git-submodule.sh\"\nentirely, replacing it wtih a \"builtin/submodule.c\". We leave\n\"builtin/submodule--helper.c\" in-place and invoke its\ncmd_submodule__helper() from the much smaller\n\"builtin/submodule.c\". We should consolidate them, but let's leave\nthat cleanup for later.\n\nAn earlier version of this was submitted as an RFC in [1], since then\nwe've had various \"prep\" series's land to prepare for this final step,\nnamely:\n\n - 361cbe6d6d2 (Merge branch 'ab/submodule-cleanup', 2022-07-14)\n - 5647d743e39 (Merge branch 'ab/submodule-helper-prep' into ab/submodule-helper-leakfix, 2022-09-02)\n - b563638d2cf (Merge branch 'ab/submodule-helper-leakfix', 2022-09-14)\n\nThe first two made it much easier to take these last steps, as the\n\"submodule--helper\" interface was already mostly mirroring that of\n\"submodule\" itself. The \"ab/submodule-helper-leakfix\" series then made\nit easier to test that our object ownership was still sane.\n\nThere's a recent report at [2] that \"submodule--helper list\" going\naway in 5647d743e39 only left the much slower \"foreach 'echo $name'\"\nalternative. As 07/10 and 10/10 here note rewriting this in C makes\nthat 'echo' version only about 0.1x slower, instead of >6x slower.\n\nFor CI & a branch that can be cloned for this series see:\nhttps://github.com/avar/git/tree/avar/submodule-sh-dispatch-to-helper-directly-2\n\n1. https://lore.kernel.org/git/RFC-cover-00.20-00000000000-20220610T011725Z-avarab@gmail.com/\n2. https://lore.kernel.org/git/87czatrpyb.fsf@bernoul.li/\n\nÆvar Arnfjörð Bjarmason (10):\n git-submodule.sh: create a \"case\" dispatch statement\n git-submodule.sh: dispatch \"sync\" to helper\n git-submodule.sh: dispatch directly to helper\n git-submodule.sh: dispatch \"foreach\" to helper\n git-submodule.sh: dispatch \"update\" to helper\n git-submodule.sh: don't support top-level \"--cached\"\n submodule: make it a built-in, remove git-submodule.sh\n submodule: support \"--\" with no other arguments\n submodule: support sub-command-less \"--recursive\" option\n submodule: don't use a subprocess to invoke \"submodule--helper\"\n\n Documentation/git-submodule.txt | 2 +-\n Makefile | 2 +-\n builtin.h | 1 +\n builtin/submodule--helper.c | 3 +-\n builtin/submodule.c | 178 ++++++++++\n git-submodule.sh | 611 --------------------------------\n git.c | 1 +\n t/t7400-submodule-basic.sh | 27 +-\n 8 files changed, 200 insertions(+), 625 deletions(-)\n create mode 100644 builtin/submodule.c\n delete mode 100755 git-submodule.sh\n\nRange-diff:\n 1: 0e9f13822ef < -: ----------- git-submodule.sh: remove unused sanitize_submodule_env()\n 7: 9f5cfbb864a ! 1: 2379d5dc0e0 git-submodule.sh: create a \"case\" dispatch statement\n @@ Commit message\n \n ## git-submodule.sh ##\n @@ git-submodule.sh: cmd_sync()\n - \tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper sync ${GIT_QUIET:+--quiet} ${recursive:+--recursive} -- \"$@\"\n + \tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper sync ${quiet:+--quiet} ${recursive:+--recursive} -- \"$@\"\n }\n \n -cmd_absorbgitdirs()\n 9: bd0e4a4f8b8 ! 2: 46bf600820b git-submodule.sh: dispatch \"sync\" to helper\n @@ Commit message\n ## git-submodule.sh ##\n @@ git-submodule.sh: cmd_status()\n \n - \tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper status ${GIT_QUIET:+--quiet} ${cached:+--cached} ${recursive:+--recursive} -- \"$@\"\n + \tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper status ${quiet:+--quiet} ${cached:+--cached} ${recursive:+--recursive} -- \"$@\"\n }\n -#\n -# Sync remote urls for submodules\n @@ git-submodule.sh: cmd_status()\n -\tdo\n -\t\tcase \"$1\" in\n -\t\t-q|--quiet)\n --\t\t\tGIT_QUIET=1\n +-\t\t\tquiet=1\n -\t\t\tshift\n -\t\t\t;;\n -\t\t--recursive)\n @@ git-submodule.sh: cmd_status()\n -\t\tesac\n -\tdone\n -\n --\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper sync ${GIT_QUIET:+--quiet} ${recursive:+--recursive} -- \"$@\"\n +-\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper sync ${quiet:+--quiet} ${recursive:+--recursive} -- \"$@\"\n -}\n \n # This loop parses the command line arguments to find the\n @@ git-submodule.sh: case \"$command\" in\n \t;;\n +sync)\n +\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper \"$command\" \\\n -+\t\t${GIT_QUIET:+--quiet} \"$@\"\n ++\t\t${quiet:+--quiet} \"$@\"\n +\t;;\n *)\n \t\"cmd_$(echo $command | sed -e s/-/_/g)\" \"$@\"\n10: 498a1fd275b ! 3: 97cb470c96a git-submodule.sh: dispatch directly to helper\n @@ Commit message\n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n ## git-submodule.sh ##\n -@@ git-submodule.sh: GIT_PROTOCOL_FROM_USER=0\n - export GIT_PROTOCOL_FROM_USER\n +@@ git-submodule.sh: export GIT_PROTOCOL_FROM_USER\n \n command=\n + quiet=\n -branch=\n force=\n reference=\n @@ git-submodule.sh: GIT_PROTOCOL_FROM_USER=0\n -files=\n remote=\n nofetch=\n - update=\n + rebase=\n + merge=\n + checkout=\n -custom_name=\n depth=\n progress=\n @@ git-submodule.sh: jobs=\n -\t\t\tforce=$1\n -\t\t\t;;\n -\t\t-q|--quiet)\n --\t\t\tGIT_QUIET=1\n +-\t\t\tquiet=1\n -\t\t\t;;\n -\t\t--progress)\n -\t\t\tprogress=1\n @@ git-submodule.sh: jobs=\n -\t\tusage\n -\tfi\n -\n --\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper add ${GIT_QUIET:+--quiet} ${force:+--force} ${progress:+\"--progress\"} ${branch:+--branch \"$branch\"} ${reference_path:+--reference \"$reference_path\"} ${dissociate:+--dissociate} ${custom_name:+--name \"$custom_name\"} ${depth:+\"$depth\"} -- \"$@\"\n +-\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper add ${quiet:+--quiet} ${force:+--force} ${progress:+\"--progress\"} ${branch:+--branch \"$branch\"} ${reference_path:+--reference \"$reference_path\"} ${dissociate:+--dissociate} ${custom_name:+--name \"$custom_name\"} ${depth:+\"$depth\"} -- \"$@\"\n -}\n \n -#\n @@ git-submodule.sh: jobs=\n # submodule\n #\n @@ git-submodule.sh: cmd_foreach()\n - \tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper foreach ${GIT_QUIET:+--quiet} ${recursive:+--recursive} -- \"$@\"\n + \tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper foreach ${quiet:+--quiet} ${recursive:+--recursive} -- \"$@\"\n }\n \n -#\n @@ git-submodule.sh: cmd_foreach()\n -\tdo\n -\t\tcase \"$1\" in\n -\t\t-q|--quiet)\n --\t\t\tGIT_QUIET=1\n +-\t\t\tquiet=1\n -\t\t\t;;\n -\t\t--)\n -\t\t\tshift\n @@ git-submodule.sh: cmd_foreach()\n -\t\tshift\n -\tdone\n -\n --\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper init ${GIT_QUIET:+--quiet} -- \"$@\"\n +-\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper init ${quiet:+--quiet} -- \"$@\"\n -}\n -\n -#\n @@ git-submodule.sh: cmd_foreach()\n -\t\t\tforce=$1\n -\t\t\t;;\n -\t\t-q|--quiet)\n --\t\t\tGIT_QUIET=1\n +-\t\t\tquiet=1\n -\t\t\t;;\n -\t\t--all)\n -\t\t\tdeinit_all=t\n @@ git-submodule.sh: cmd_foreach()\n -\t\tshift\n -\tdone\n -\n --\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper deinit ${GIT_QUIET:+--quiet} ${force:+--force} ${deinit_all:+--all} -- \"$@\"\n +-\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper deinit ${quiet:+--quiet} ${force:+--force} ${deinit_all:+--all} -- \"$@\"\n -}\n -\n #\n @@ git-submodule.sh: cmd_update()\n -\t\tshift\n -\tdone\n -\n --\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper set-branch ${GIT_QUIET:+--quiet} ${branch:+--branch \"$branch\"} ${default:+--default} -- \"$@\"\n +-\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper set-branch ${quiet:+--quiet} ${branch:+--branch \"$branch\"} ${default:+--default} -- \"$@\"\n -}\n -\n -#\n @@ git-submodule.sh: cmd_update()\n -\tdo\n -\t\tcase \"$1\" in\n -\t\t-q|--quiet)\n --\t\t\tGIT_QUIET=1\n +-\t\t\tquiet=1\n -\t\t\t;;\n -\t\t--)\n -\t\t\tshift\n @@ git-submodule.sh: cmd_update()\n -\t\tshift\n -\tdone\n -\n --\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper set-url ${GIT_QUIET:+--quiet} -- \"$@\"\n +-\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper set-url ${quiet:+--quiet} -- \"$@\"\n -}\n -\n -#\n @@ git-submodule.sh: cmd_update()\n -\tdo\n -\t\tcase \"$1\" in\n -\t\t-q|--quiet)\n --\t\t\tGIT_QUIET=1\n +-\t\t\tquiet=1\n -\t\t\t;;\n -\t\t--cached)\n -\t\t\tcached=1\n @@ git-submodule.sh: cmd_update()\n -\t\tshift\n -\tdone\n -\n --\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper status ${GIT_QUIET:+--quiet} ${cached:+--cached} ${recursive:+--recursive} -- \"$@\"\n +-\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper status ${quiet:+--quiet} ${cached:+--cached} ${recursive:+--recursive} -- \"$@\"\n -}\n -\n # This loop parses the command line arguments to find the\n @@ git-submodule.sh: case \"$command\" in\n \t;;\n -sync)\n -\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper \"$command\" \\\n --\t\t${GIT_QUIET:+--quiet} \"$@\"\n +-\t\t${quiet:+--quiet} \"$@\"\n +foreach | update)\n +\t\"cmd_$command\" \"$@\"\n \t;;\n @@ git-submodule.sh: case \"$command\" in\n -\t\"cmd_$(echo $command | sed -e s/-/_/g)\" \"$@\"\n +add | init | deinit | set-branch | set-url | status | summary | sync)\n +\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper \"$command\" \\\n -+\t\t${GIT_QUIET:+--quiet} ${cached:+--cached} \"$@\"\n ++\t\t${quiet:+--quiet} ${cached:+--cached} \"$@\"\n \t;;\n esac\n11: 625320e13b9 ! 4: db6a09ee42a git-submodule.sh: dispatch \"foreach\" to helper\n @@ Commit message\n \n ## builtin/submodule--helper.c ##\n @@ builtin/submodule--helper.c: static int module_foreach(int argc, const char **argv, const char *prefix)\n - \t};\n + \tint ret = 1;\n \n \targc = parse_options(argc, argv, prefix, module_foreach_options,\n -\t\t\t git_submodule_helper_usage, 0);\n @@ builtin/submodule--helper.c: static int module_foreach(int argc, const char **ar\n +\t\t\t PARSE_OPT_STOP_AT_NON_OPTION);\n \n \tif (module_list_compute(0, NULL, prefix, &pathspec, &list) < 0)\n - \t\treturn 1;\n + \t\tgoto cleanup;\n \n ## git-submodule.sh ##\n @@ git-submodule.sh: jobs=\n @@ git-submodule.sh: jobs=\n -\tdo\n -\t\tcase \"$1\" in\n -\t\t-q|--quiet)\n --\t\t\tGIT_QUIET=1\n +-\t\t\tquiet=1\n -\t\t\t;;\n -\t\t--recursive)\n -\t\t\trecursive=1\n @@ git-submodule.sh: jobs=\n -\t\tshift\n -\tdone\n -\n --\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper foreach ${GIT_QUIET:+--quiet} ${recursive:+--recursive} -- \"$@\"\n +-\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper foreach ${quiet:+--quiet} ${recursive:+--recursive} -- \"$@\"\n -}\n -\n #\n @@ git-submodule.sh: case \"$command\" in\n -add | init | deinit | set-branch | set-url | status | summary | sync)\n +add | foreach | init | deinit | set-branch | set-url | status | summary | sync)\n \tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper \"$command\" \\\n - \t\t${GIT_QUIET:+--quiet} ${cached:+--cached} \"$@\"\n + \t\t${quiet:+--quiet} ${cached:+--cached} \"$@\"\n \t;;\n16: 08abadda7c3 ! 5: 7d9c13eb637 git-submodule.sh: dispatch \"update\" to helper\n @@ Commit message\n \n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n - ## Documentation/git-submodule.txt ##\n -@@ Documentation/git-submodule.txt: SYNOPSIS\n - 'git submodule' [--quiet] status [--cached] [--recursive] [--] [...]\n - 'git submodule' [--quiet] init [--] [...]\n - 'git submodule' [--quiet] deinit [-f|--force] (--all|[--] ...)\n --'git submodule' [--quiet] update [-v] [] [--] [...]\n -+'git submodule' [--quiet] update [-v | --verbose] [] [--] [...]\n - 'git submodule' [--quiet] set-branch [] [--] \n - 'git submodule' [--quiet] set-url [--] \n - 'git submodule' [--quiet] summary [] [--] [...]\n -@@ Documentation/git-submodule.txt: If you really want to remove a submodule from the repository and commit\n - that use linkgit:git-rm[1] instead. See linkgit:gitsubmodules[7] for removal\n - options.\n - \n --update [-v] [--init] [--remote] [-N|--no-fetch] [--[no-]recommend-shallow] [-f|--force] [--checkout|--rebase|--merge] [--reference ] [--depth ] [--recursive] [--jobs ] [--[no-]single-branch] [--filter ] [--] [...]::\n -+update [-v | --verbose] [--init] [--remote] [-N|--no-fetch] [--[no-]recommend-shallow] [-f|--force] [--checkout|--rebase|--merge] [--reference ] [--depth ] [--recursive] [--jobs ] [--[no-]single-branch] [--filter ] [--] [...]::\n - +\n - --\n - Update the registered submodules to match what the superproject\n -@@ Documentation/git-submodule.txt: OPTIONS\n - \tOnly print error messages.\n - \n - -v::\n -+--verbose::\n - \tDon't be quiet. This option is only valid for the update command.\n - \n - --progress::\n -\n ## git-submodule.sh ##\n -@@ git-submodule.sh: GIT_PROTOCOL_FROM_USER=0\n - export GIT_PROTOCOL_FROM_USER\n +@@ git-submodule.sh: export GIT_PROTOCOL_FROM_USER\n \n command=\n + quiet=\n -force=\n -reference=\n cached=\n @@ git-submodule.sh: GIT_PROTOCOL_FROM_USER=0\n -\tdo\n -\t\tcase \"$1\" in\n -\t\t-q|--quiet)\n --\t\t\tGIT_QUIET=1\n --\t\t\t;;\n --\t\t-v)\n --\t\t\tunset GIT_QUIET\n +-\t\t\tquiet=1\n -\t\t\t;;\n -\t\t--progress)\n -\t\t\tprogress=1\n @@ git-submodule.sh: GIT_PROTOCOL_FROM_USER=0\n -\tdone\n -\n -\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper update \\\n --\t\t${GIT_QUIET:+--quiet} \\\n +-\t\t${quiet:+--quiet} \\\n -\t\t${force:+--force} \\\n -\t\t${progress:+\"--progress\"} \\\n -\t\t${remote:+--remote} \\\n @@ git-submodule.sh: absorbgitdirs)\n update)\n -\tcmd_update \"$@\"\n +\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper \"$command\" \\\n -+\t\t${GIT_QUIET:+--quiet} ${wt_prefix:+--prefix \"$wt_prefix\"} \"$@\"\n ++\t\t${quiet:+--quiet} ${wt_prefix:+--prefix \"$wt_prefix\"} \"$@\"\n \t;;\n add | foreach | init | deinit | set-branch | set-url | status | summary | sync)\n \tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper \"$command\" \\\n 4: f27723aa0a2 ! 6: 25fadf3ffc1 git-submodule.sh: normalize parsing of \"--branch\"\n @@ Metadata\n Author: Ævar Arnfjörð Bjarmason \n \n ## Commit message ##\n - git-submodule.sh: normalize parsing of \"--branch\"\n + git-submodule.sh: don't support top-level \"--cached\"\n \n - In 5c08dbbdf1a (git-submodule: fix subcommand parser, 2008-01-15) the\n - \"--branch\" option was supported as an option to \"git submodule\"\n - itself, i.e. \"git submodule --branch\" as a side-effect of its\n - implementation.\n + Since the preceding commit all sub-commands of \"git submodule\" have\n + been dispatched to \"git submodule--helper\" directly, we therefore\n + don't need to emit \"usage()\" if we see \"--cached\" without the\n + sub-command being \"status\" or \"summary\", we can trust that\n + parse_options() will spot that and barf on it.\n \n - Then in b57e8119e6e (submodule: teach set-branch subcommand,\n - 2019-02-08) when the \"set-branch\" subcommand was added the assertion\n - that we shouldn't have \"--branch\" anywhere except as an argument to\n - \"add\" and \"set-branch\" was copy/pasted from the adjacent check for\n - \"--cache\" added (or rather modified) in 496eeeb19b9 (git-submodule.sh:\n - avoid \"test -a/-o \", 2014-06-10).\n + This does change one obscure aspect of undocumented behavior, for\n + \"status\" and \"summary\" we supported these undocumented forms:\n \n - But there's been a logic error in that check, this looked like it\n - should be supporting:\n + git submodule --cached (status | summary)\n \n - git submodule --branch (add | set-branch) []\n + As noted in a preceding commit to git-submodule.sh which removed the\n + \"--branch\" special-case, this comes down to emergent behavior seen in\n + 5c08dbbdf1a (git-submodule: fix subcommand parser,\n + 2008-01-15). I.e. we wanted to support was for subcommand-less invocations like:\n \n - But due to \"||\" in the condition (as opposed to \"&&\" for \"--cache\") if\n - we have \"--branch\" here already we'll emit usage, even for \"add\" and\n - \"set-branch\".\n + git submodule --cached\n \n - Since nobody's complained about \"--branch \" not being\n - supported as argument to \"git submodule\" itself, i.e. we want to\n - support:\n + To be synonymous with invocations that explicitly named the \"status\"\n + sub-command:\n \n - git submodule (add | set-branch) --branch []\n + git submodule status --cached\n \n - But not the first form noted above. Let's just remove this code, we've\n - never documented \"--branch\" as a top-level option (unlike \"--quiet\"),\n - so this looks like it was an accident of the implementation, which we\n - broke v2.22.0, so we also know it must not have been important to\n - anyone.\n + But we did not intend to mix the two, and allow \"--cached\" to be an\n + option to the top-level \"submodule\" command when the \"status\" or\n + \"summary\" sub-commands were explicitly provided.\n +\n + Let's remove this undocumented edge case, which makes a subsequent\n + removal of git-submodule.sh easier to reason about. The test case\n + added here is duplicated from the existing for-loop, except for the\n + different and desired handling of \"git submodule --cached status\".\n \n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n ## git-submodule.sh ##\n @@ git-submodule.sh: do\n - \t-q|--quiet)\n - \t\tGIT_QUIET=1\n + \t\tquiet=1\n \t\t;;\n --\t-b|--branch)\n --\t\tcase \"$2\" in\n --\t\t'')\n --\t\t\tusage\n --\t\t\t;;\n --\t\tesac\n --\t\tbranch=\"$2\"; shift\n --\t\t;;\n \t--cached)\n - \t\tcached=\"$1\"\n +-\t\tcached=1\n ++\t\tif test -z \"$command\"\n ++\t\tthen\n ++\t\t\tcached=1 &&\n ++\t\t\tshift &&\n ++\t\t\tbreak\n ++\t\telse\n ++\t\t\tusage\n ++\t\tfi\n \t\t;;\n + \t--)\n + \t\tbreak\n @@ git-submodule.sh: then\n fi\n fi\n \n --# \"-b branch\" is accepted only by \"add\" and \"set-branch\"\n --if test -n \"$branch\" && (test \"$command\" != add || test \"$command\" != set-branch)\n +-# \"--cached\" is accepted only by \"status\" and \"summary\"\n +-if test -n \"$cached\" && test \"$command\" != status && test \"$command\" != summary\n -then\n -\tusage\n -fi\n -\n - # \"--cached\" is accepted only by \"status\" and \"summary\"\n - if test -n \"$cached\" && test \"$command\" != status && test \"$command\" != summary\n - then\n + case \"$command\" in\n + absorbgitdirs)\n + \tgit submodule--helper \"$command\" --prefix \"$wt_prefix\" \"$@\"\n +\n + ## t/t7400-submodule-basic.sh ##\n +@@ t/t7400-submodule-basic.sh: test_expect_success 'submodule usage: status --' '\n + \ttest_expect_code 1 git submodule --end-of-options\n + '\n + \n +-for opt in '--quiet' '--cached'\n ++for opt in '--quiet'\n + do\n + \ttest_expect_success \"submodule usage: status $opt\" '\n + \t\tgit submodule $opt &&\n +@@ t/t7400-submodule-basic.sh: do\n + \t'\n + done\n + \n ++for opt in '--cached'\n ++do\n ++\ttest_expect_success \"submodule usage: status $opt\" '\n ++\t\tgit submodule $opt &&\n ++\t\tgit submodule status $opt &&\n ++\t\ttest_expect_code 1 git submodule $opt status >out 2>err &&\n ++\t\tgrep \"^usage: git submodule\" err &&\n ++\t\ttest_must_be_empty out\n ++\t'\n ++done\n ++\n + test_expect_success 'submodule deinit works on empty repository' '\n + \tgit submodule deinit --all\n + '\n +@@ t/t7400-submodule-basic.sh: test_expect_success 'status should be \"modified\" after submodule commit' '\n + '\n + \n + test_expect_success 'the --cached sha1 should be rev1' '\n +-\tgit submodule --cached status >list &&\n ++\tgit submodule status --cached >list &&\n + \tgrep \"^+$rev1\" list\n + '\n + \n19: 1423950de08 ! 7: 2c77ed38d90 submodule: make it a built-in, remove git-submodule.sh\n @@ Metadata\n ## Commit message ##\n submodule: make it a built-in, remove git-submodule.sh\n \n - Replace the now-trivial git-submodule.sh script with a built-in\n - builtin/submodule.c. For now this new command is only a dumb\n + Replace the \"git-submodule.sh\" script with a built-in\n + \"builtin/submodule.c. For\" now this new command is only a dumb\n dispatcher that uses run-command.c to invoke \"git submodule--helper\",\n just as \"git-submodule.sh\" used to do.\n \n - This is obviously not ideal, and we should follow-up and merge the\n - builtin/submodule--helper.c code into builtin/submodule.c, but doing it\n - this way makes it easy to review that this new C implementation isn't\n - doing anything more clever than the old shellscript implementation.\n + This is obviously not ideal, and we should eventually follow-up and\n + merge the \"builtin/submodule--helper.c\" code into\n + \"builtin/submodule.c\". Doing it this way makes it easy to review that\n + this new C implementation isn't doing anything more clever than the\n + old shellscript implementation.\n \n - The \"define BUILTIN_\" macros will help with that, i.e. the usage\n - information we emit can be merged with what\n - builtin/submodule--helper.c is now emitting. See\n - 8757b35d443 (commit-graph: define common usage with a macro,\n - 2021-08-23) and 1e91d3faf6c (reflog: move \"usage\" variables and use\n - macros, 2022-03-17) for prior art using this pattern.\n + This is a large win for performance, we're now more than 4x as fast as\n + before in terms of the fixed cost of invoking any \"git submodule\"\n + command[1]:\n +\n + $ git hyperfine -L rev HEAD~1,HEAD -s 'make CFLAGS=-O3' './git --exec-path=$PWD submodule foreach \"echo \\$name\"'\n + Benchmark 1: ./git --exec-path=$PWD submodule foreach \"echo \\$name\"' in 'HEAD~1\n + Time (mean ± σ): 42.2 ms ± 0.4 ms [User: 34.9 ms, System: 9.1 ms]\n + Range (min … max): 41.3 ms … 43.2 ms 70 runs\n +\n + Benchmark 2: ./git --exec-path=$PWD submodule foreach \"echo \\$name\"' in 'HEAD\n + Time (mean ± σ): 9.7 ms ± 0.1 ms [User: 7.6 ms, System: 2.2 ms]\n + Range (min … max): 9.5 ms … 10.3 ms 282 runs\n +\n + Summary\n + './git --exec-path=$PWD submodule foreach \"echo \\$name\"' in 'HEAD' ran\n + 4.33 ± 0.07 times faster than './git --exec-path=$PWD submodule foreach \"echo \\$name\"' in 'HEAD~1'\n +\n + We're taking pains here to faithfully reproduce existing\n + \"git-submodule.sh\" behavior, even when that behavior is stupid. Some\n + of it we'll fix in subsequent commits, but let's first faithfully\n + reproduce the behavior.\n +\n + One exception is the change in the behavior of the exit code\n + stand-alone \"-h\" and \"--\" yield, see the altered tests. Returning 129\n + instead of 0 and 1 for \"-h\" and \"--\" respectively is a concession to\n + basic sanity.\n +\n + The pattern of using \"define BUILTIN_\" macros here isn't needed for\n + now, but as we'll move code out of \"builtin/submodule--helper.c\" we'll\n + want to re-use these strings. See 8757b35d443 (commit-graph: define\n + common usage with a macro, 2021-08-23) and 1e91d3faf6c (reflog: move\n + \"usage\" variables and use macros, 2022-03-17) for prior art using this\n + pattern.\n \n The \"(argc < 2 || !strcmp(argv[1], \"-h\"))\" path at the top of\n cmd_submodule__helper() could now be a \"(argc < 2)\" if not for\n t0012-help.sh (which invokes all built-ins manually with \"-h\"). Let's\n leave it for now, eventually we'll consolidate the two.\n \n + 1. Using the \"git hyperfine\" wrapper for \"hyperfine\":\n + https://lore.kernel.org/git/211201.86r1aw9gbd.gmgdl@evledraar.gmail.com/\n +\n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n ## Makefile ##\n @@ builtin/submodule.c (new)\n +};\n +\n +static void setup_helper_args(int argc, const char **argv, const char *prefix,\n -+\t\t\t int quiet, int cached, struct strvec *args)\n ++\t\t\t int quiet, int cached, struct strvec *args,\n ++\t\t\t const struct option *options)\n +{\n +\tconst char *cmd;\n +\tint do_quiet_cache = 1;\n @@ builtin/submodule.c (new)\n +\t\treturn;\n +\t}\n +\n ++\t/* Did we get --cached with a command? */\n ++\tif (cached)\n ++\t\tusage_msg_optf(_(\"'%s' option is only supported with explicit 'status'\"),\n ++\t\t\t git_submodule_usage, options, \"--cached\");\n ++\n ++\n +\t/* Either a valid command, or submodule--helper will barf! */\n +\tcmd = argv[0];\n +\tstrvec_push(args, cmd);\n +\targv++;\n +\targc--;\n +\n ++\t/*\n ++\t * This is stupid, but don't support \"[--]\" to\n ++\t * subcommand-less \"git-submodule\" for now.\n ++\t */\n ++\tif (!strcmp(cmd, \"--\") || !strcmp(cmd, \"--end-of-options\"))\n ++\t\tusage_msg_optf(_(\"need explicit sub-command name to delimit with '%s'\"),\n ++\t\t\t git_submodule_usage, options, cmd);\n ++\n +\t/* Options that need to go before user-supplied options */\n +\tif (!strcmp(cmd, \"absorbgitdirs\"))\n +\t\tdo_quiet_cache = 0;\n @@ builtin/submodule.c (new)\n +\t\t\t N_(\"print the OID of submodules\")),\n +\t\tOPT_END()\n +\t};\n -+\tint ret;\n +\n +\targc = parse_options(argc, argv, prefix, options, git_submodule_usage,\n -+\t\t\t PARSE_OPT_STOP_AT_NON_OPTION);\n ++\t\t\t PARSE_OPT_STOP_AT_NON_OPTION | PARSE_OPT_KEEP_DASHDASH);\n +\n +\t/*\n +\t * Tell the rest of git that any URLs we get don't come\n +\t * directly from the user, so it can apply policy as appropriate.\n +\t */\n -+\tstrvec_push(&cp.env_array, \"GIT_PROTOCOL_FROM_USER=0\");\n ++\tstrvec_push(&cp.env, \"GIT_PROTOCOL_FROM_USER=0\");\n +\tsetup_helper_args(argc, argv, prefix, opt_quiet, opt_cached,\n -+\t\t\t &cp.args);\n ++\t\t\t &cp.args, options);\n +\n +\tcp.git_cmd = 1;\n +\tcp.no_stdin = 0; /* for git submodule foreach */\n +\tcp.dir = startup_info->original_cwd;\n -+\tret = run_command(&cp);\n +\n -+\treturn ret;\n ++\treturn run_command(&cp);\n +}\n \n ## git-submodule.sh (deleted) ##\n @@ git-submodule.sh (deleted)\n - or: $dashless [--quiet] status [--cached] [--recursive] [--] [...]\n - or: $dashless [--quiet] init [--] [...]\n - or: $dashless [--quiet] deinit [-f|--force] (--all| [--] ...)\n -- or: $dashless [--quiet] update [-v] [--init [--filter=]] [--remote] [-N|--no-fetch] [-f|--force] [--checkout|--merge|--rebase] [--[no-]recommend-shallow] [--reference ] [--recursive] [--[no-]single-branch] [--] [...]\n +- or: $dashless [--quiet] update [--init [--filter=]] [--remote] [-N|--no-fetch] [-f|--force] [--checkout|--merge|--rebase] [--[no-]recommend-shallow] [--reference ] [--recursive] [--[no-]single-branch] [--] [...]\n - or: $dashless [--quiet] set-branch (--default|--branch ) [--] \n - or: $dashless [--quiet] set-url [--] \n - or: $dashless [--quiet] summary [--cached|--files] [--summary-limit ] [commit] [--] [...]\n @@ git-submodule.sh (deleted)\n -GIT_PROTOCOL_FROM_USER=0\n -export GIT_PROTOCOL_FROM_USER\n -\n +-command=\n -quiet=\n -cached=\n -\n --while test $# != 0\n +-while test $# != 0 && test -z \"$command\"\n -do\n -\tcase \"$1\" in\n +-\tadd | foreach | init | deinit | update | set-branch | set-url | status | summary | sync | absorbgitdirs)\n +-\t\tcommand=$1\n +-\t\t;;\n -\t-q|--quiet)\n --\t\tquiet=1 &&\n --\t\tshift\n +-\t\tquiet=1\n -\t\t;;\n -\t--cached)\n --\t\tcached=1 &&\n --\t\tshift\n +-\t\tif test -z \"$command\"\n +-\t\tthen\n +-\t\t\tcached=1 &&\n +-\t\t\tshift &&\n +-\t\t\tbreak\n +-\t\telse\n +-\t\t\tusage\n +-\t\tfi\n +-\t\t;;\n +-\t--)\n +-\t\tbreak\n +-\t\t;;\n +-\t-*)\n +-\t\tusage\n -\t\t;;\n -\t*)\n -\t\tbreak\n -\t\t;;\n -\tesac\n +-\tshift\n -done\n -\n -# No command word defaults to \"status\"\n --command=\n --if test $# = 0\n +-if test -z \"$command\"\n -then\n +- if test $# = 0\n +- then\n -\tcommand=status\n --else\n --\tcase \"$1\" in\n --\tadd | foreach | init | deinit | update | set-branch | set-url | status | summary | sync | absorbgitdirs)\n --\t\tcommand=$1 &&\n --\t\tshift\n --\t\t;;\n --\t*)\n --\t\tusage\n --\tesac\n +- else\n +-\tusage\n +- fi\n -fi\n -\n -case \"$command\" in\n @@ git-submodule.sh (deleted)\n -\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper \"$command\" \\\n -\t\t${quiet:+--quiet} ${wt_prefix:+--prefix \"$wt_prefix\"} \"$@\"\n -\t;;\n --*)\n +-add | foreach | init | deinit | set-branch | set-url | status | summary | sync)\n -\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper \"$command\" \\\n -\t\t${quiet:+--quiet} ${cached:+--cached} \"$@\"\n -\t;;\n @@ git.c: static struct cmd_struct commands[] = {\n \t{ \"submodule--helper\", cmd_submodule__helper, RUN_SETUP | SUPPORT_SUPER_PREFIX | NO_PARSEOPT },\n \t{ \"switch\", cmd_switch, RUN_SETUP | NEED_WORK_TREE },\n \t{ \"symbolic-ref\", cmd_symbolic_ref, RUN_SETUP },\n +\n + ## t/t7400-submodule-basic.sh ##\n +@@ t/t7400-submodule-basic.sh: export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n + . ./test-lib.sh\n + \n + test_expect_success 'submodule usage: -h' '\n +-\tgit submodule -h >out 2>err &&\n ++\ttest_expect_code 129 git submodule -h >out 2>err &&\n + \tgrep \"^usage: git submodule\" out &&\n + \ttest_must_be_empty err\n + '\n + \n + test_expect_success 'submodule usage: --recursive' '\n +-\ttest_expect_code 1 git submodule --recursive >out 2>err &&\n +-\tgrep \"^usage: git submodule\" err &&\n +-\ttest_must_be_empty out\n ++\ttest_expect_code 129 git submodule --recursive\n + '\n + \n + test_expect_success 'submodule usage: status --' '\n +-\ttest_expect_code 1 git submodule -- &&\n +-\ttest_expect_code 1 git submodule --end-of-options\n ++\ttest_expect_code 129 git submodule -- &&\n ++\ttest_expect_code 129 git submodule --end-of-options\n + '\n + \n + for opt in '--quiet'\n +@@ t/t7400-submodule-basic.sh: do\n + \ttest_expect_success \"submodule usage: status $opt\" '\n + \t\tgit submodule $opt &&\n + \t\tgit submodule status $opt &&\n +-\t\ttest_expect_code 1 git submodule $opt status >out 2>err &&\n ++\t\ttest_expect_code 129 git submodule $opt status >out 2>err &&\n + \t\tgrep \"^usage: git submodule\" err &&\n + \t\ttest_must_be_empty out\n + \t'\n 3: 6c774505ac5 ! 8: 8dbb81e2fa5 git-submodule.sh: remove unused --super-prefix logic\n @@ Metadata\n Author: Ævar Arnfjörð Bjarmason \n \n ## Commit message ##\n - git-submodule.sh: remove unused --super-prefix logic\n + submodule: support \"--\" with no other arguments\n \n - The \"$prefix\" variable has not been set since b3c5f5cb048 (submodule:\n - move core cmd_update() logic to C, 2022-03-15), so we'd never pass the\n - --super-prefix option to \"git submodule--helper\", and can therefore\n - remove the handling of it from builtin/submodule--helper.c as well.\n + Address an edge case that \"git-submodule.sh\" has had all along, but\n + which became painfully obvious in the *.sh to *.c migration in the\n + preceding commit.\n +\n + We didn't support the \"--\" delimiter in the argument-less\n + invocation. Let's not bend over backwards to behave unusually in this\n + scenario, simply accepting \"--\" is harmless.\n \n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n - ## builtin/submodule--helper.c ##\n -@@ builtin/submodule--helper.c: static int module_add(int argc, const char **argv, const char *prefix)\n - \treturn 0;\n - }\n + ## Documentation/git-submodule.txt ##\n +@@ Documentation/git-submodule.txt: git-submodule - Initialize, update or inspect submodules\n + SYNOPSIS\n + --------\n + [verse]\n +-'git submodule' [--quiet] [--cached]\n ++'git submodule' [--quiet] [--cached] [--]\n + 'git submodule' [--quiet] add [] [--] []\n + 'git submodule' [--quiet] status [--cached] [--recursive] [--] [...]\n + 'git submodule' [--quiet] init [--] [...]\n +\n + ## builtin/submodule.c ##\n +@@\n + #include \"strvec.h\"\n + \n + #define BUILTIN_SUBMODULE_USAGE \\\n +-\t\"git submodule [--quiet] [--cached]\"\n ++\t\"git submodule [--quiet] [--cached] [--]\"\n \n --#define SUPPORT_SUPER_PREFIX (1<<0)\n + #define BUILTIN_SUBMODULE_ADD_USAGE \\\n + \tN_(\"git submodule [--quiet] add [-b ] [-f | --force] [--name ]\\n\" \\\n +@@ builtin/submodule.c: static void setup_helper_args(int argc, const char **argv, const char *prefix,\n + \targv++;\n + \targc--;\n + \n +-\t/*\n +-\t * This is stupid, but don't support \"[--]\" to\n +-\t * subcommand-less \"git-submodule\" for now.\n +-\t */\n +-\tif (!strcmp(cmd, \"--\") || !strcmp(cmd, \"--end-of-options\"))\n +-\t\tusage_msg_optf(_(\"need explicit sub-command name to delimit with '%s'\"),\n +-\t\t\t git_submodule_usage, options, cmd);\n -\n - struct cmd_struct {\n - \tconst char *cmd;\n - \tint (*fn)(int, const char **, const char *);\n --\tunsigned option;\n - };\n + \t/* Options that need to go before user-supplied options */\n + \tif (!strcmp(cmd, \"absorbgitdirs\"))\n + \t\tdo_quiet_cache = 0;\n +@@ builtin/submodule.c: int cmd_submodule(int argc, const char **argv, const char *prefix)\n + \t};\n \n - static struct cmd_struct commands[] = {\n --\t{\"list\", module_list, 0},\n --\t{\"name\", module_name, 0},\n --\t{\"clone\", module_clone, 0},\n --\t{\"add\", module_add, SUPPORT_SUPER_PREFIX},\n --\t{\"update\", module_update, 0},\n --\t{\"resolve-relative-url-test\", resolve_relative_url_test, 0},\n --\t{\"foreach\", module_foreach, SUPPORT_SUPER_PREFIX},\n --\t{\"init\", module_init, SUPPORT_SUPER_PREFIX},\n --\t{\"status\", module_status, SUPPORT_SUPER_PREFIX},\n --\t{\"sync\", module_sync, SUPPORT_SUPER_PREFIX},\n --\t{\"deinit\", module_deinit, 0},\n --\t{\"summary\", module_summary, SUPPORT_SUPER_PREFIX},\n --\t{\"push-check\", push_check, 0},\n --\t{\"absorb-git-dirs\", absorb_git_dirs, SUPPORT_SUPER_PREFIX},\n --\t{\"is-active\", is_active, 0},\n --\t{\"check-name\", check_name, 0},\n --\t{\"config\", module_config, 0},\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 -+\t{\"list\", module_list},\n -+\t{\"name\", module_name},\n -+\t{\"clone\", module_clone},\n -+\t{\"add\", module_add},\n -+\t{\"update\", module_update},\n -+\t{\"resolve-relative-url-test\", resolve_relative_url_test},\n -+\t{\"foreach\", module_foreach},\n -+\t{\"init\", module_init},\n -+\t{\"status\", module_status},\n -+\t{\"sync\", module_sync},\n -+\t{\"deinit\", module_deinit},\n -+\t{\"summary\", module_summary},\n -+\t{\"push-check\", push_check},\n -+\t{\"absorb-git-dirs\", absorb_git_dirs},\n -+\t{\"is-active\", is_active},\n -+\t{\"check-name\", check_name},\n -+\t{\"config\", module_config},\n -+\t{\"set-url\", module_set_url},\n -+\t{\"set-branch\", module_set_branch},\n -+\t{\"create-branch\", module_create_branch},\n - };\n + \targc = parse_options(argc, argv, prefix, options, git_submodule_usage,\n +-\t\t\t PARSE_OPT_STOP_AT_NON_OPTION | PARSE_OPT_KEEP_DASHDASH);\n ++\t\t\t PARSE_OPT_STOP_AT_NON_OPTION);\n \n - int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\n -@@ builtin/submodule--helper.c: int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\n - \tif (argc < 2 || !strcmp(argv[1], \"-h\"))\n - \t\tusage(\"git submodule--helper \");\n + \t/*\n + \t * Tell the rest of git that any URLs we get don't come\n +\n + ## t/t7400-submodule-basic.sh ##\n +@@ t/t7400-submodule-basic.sh: test_expect_success 'submodule usage: --recursive' '\n + '\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 -+\tfor (i = 0; i < ARRAY_SIZE(commands); i++)\n -+\t\tif (!strcmp(argv[1], commands[i].cmd))\n - \t\t\treturn commands[i].fn(argc - 1, argv + 1, prefix);\n --\t\t}\n --\t}\n + test_expect_success 'submodule usage: status --' '\n +-\ttest_expect_code 129 git submodule -- &&\n +-\ttest_expect_code 129 git submodule --end-of-options\n ++\tgit submodule -- &&\n ++\tgit submodule --end-of-options\n + '\n \n - \tdie(_(\"'%s' is not a valid submodule--helper \"\n - \t \"subcommand\"), argv[1]);\n + for opt in '--quiet'\n20: b2aaad5c008 ! 9: 7f232c5e503 submodule: add a subprocess-less submodule.useBuiltin setting\n @@ Metadata\n Author: Ævar Arnfjörð Bjarmason \n \n ## Commit message ##\n - submodule: add a subprocess-less submodule.useBuiltin setting\n + submodule: support sub-command-less \"--recursive\" option\n \n - Add an experimental setting to avoid the subprocess invocation of \"git\n - submodule--helper\", instead we'll call cmd_submodule__helper()\n - directly.\n + The inability to specify \"--recursive\" when we're not providing a\n + sub-command name appears to have been an omission in 15fc56a8536 (git\n + submodule foreach: Add --recursive to recurse into nested submodules,\n + 2009-08-19). Let's support it along with the other \"status\" options.\n \n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n - ## Documentation/config/submodule.txt ##\n -@@ Documentation/config/submodule.txt: setting.\n - * `branch` is supported only if `submodule.propagateBranches` is\n - enabled\n - \n -+submodule.useBuiltin::\n -+\tSet to `true` to use a faster but possibly less stable subprocess-less\n -+\timplementation of linkgit:git-submodule[1]. Is `false` by default.\n -+\n - submodule.propagateBranches::\n - \t[EXPERIMENTAL] A boolean that enables branching support when\n - \tusing `--recurse-submodules` or `submodule.recurse=true`.\n + ## Documentation/git-submodule.txt ##\n +@@ Documentation/git-submodule.txt: git-submodule - Initialize, update or inspect submodules\n + SYNOPSIS\n + --------\n + [verse]\n +-'git submodule' [--quiet] [--cached] [--]\n ++'git submodule' [--quiet] [--cached] [--recursive] [--]\n + 'git submodule' [--quiet] add [] [--] []\n + 'git submodule' [--quiet] status [--cached] [--recursive] [--] [...]\n + 'git submodule' [--quiet] init [--] [...]\n \n ## builtin/submodule.c ##\n -@@\n - #include \"parse-options.h\"\n - #include \"run-command.h\"\n - #include \"strvec.h\"\n -+#include \"config.h\"\n +@@ builtin/submodule.c: static const char * const git_submodule_usage[] = {\n + };\n \n - #define BUILTIN_SUBMODULE_USAGE \\\n - \t\"git submodule [--quiet] [--cached]\"\n + static void setup_helper_args(int argc, const char **argv, const char *prefix,\n +-\t\t\t int quiet, int cached, struct strvec *args,\n ++\t\t\t int quiet, int cached, int recursive,\n ++\t\t\t struct strvec *args,\n + \t\t\t const struct option *options)\n + {\n + \tconst char *cmd;\n @@ builtin/submodule.c: static void setup_helper_args(int argc, const char **argv, const char *prefix,\n - \tstrvec_pushv(args, argv);\n - }\n + \t\treturn;\n + \t}\n \n -+static int get_use_builtin(void)\n -+{\n -+\tint v;\n -+\n -+\tif (git_env_bool(\"GIT_TEST_SUBMODULE_USE_BUILTIN\", 0))\n -+\t\tv = 1;\n -+\telse if (!git_config_get_bool(\"submodule.usebuiltin\", &v))\n -+\t\t;\n -+\telse if (!git_config_get_bool(\"feature.experimental\", &v))\n -+\t ;\n -+\n -+\treturn v;\n -+}\n +-\t/* Did we get --cached with a command? */\n ++\t/* Did we get a forbidden top-level option with a command? */\n + \tif (cached)\n + \t\tusage_msg_optf(_(\"'%s' option is only supported with explicit 'status'\"),\n + \t\t\t git_submodule_usage, options, \"--cached\");\n ++\tif (recursive)\n ++\t\tusage_msg_optf(_(\"'%s' option is only supported with explicit 'status'\"),\n ++\t\t\t git_submodule_usage, options, \"--recursive\");\n + \n + \n + \t/* Either a valid command, or submodule--helper will barf! */\n +@@ builtin/submodule.c: static void setup_helper_args(int argc, const char **argv, const char *prefix,\n + \targc--;\n + \n + \t/* Options that need to go before user-supplied options */\n ++\tif (!strcmp(cmd, \"status\") && recursive)\n ++\t\tstrvec_push(args, \"--recursive\");\n +\n - int cmd_submodule(int argc, const char **argv, const char *prefix)\n + \tif (!strcmp(cmd, \"absorbgitdirs\"))\n + \t\tdo_quiet_cache = 0;\n + \telse if (!strcmp(cmd, \"update\"))\n +@@ builtin/submodule.c: int cmd_submodule(int argc, const char **argv, const char *prefix)\n {\n \tint opt_quiet = 0;\n \tint opt_cached = 0;\n ++\tint opt_recursive = 0;\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n -+\tstruct strvec args = STRVEC_INIT;\n \tstruct option options[] = {\n \t\tOPT__QUIET(&opt_quiet, N_(\"be quiet\")),\n \t\tOPT_BOOL(0, \"cached\", &opt_cached,\n \t\t\t N_(\"print the OID of submodules\")),\n ++\t\tOPT_BOOL(0, \"recursive\", &opt_recursive,\n ++\t\t\t N_(\"recurse into nested submodules\")),\n \t\tOPT_END()\n \t};\n -+\tconst int use_builtin = get_use_builtin();\n - \tint ret;\n \n - \targc = parse_options(argc, argv, prefix, options, git_submodule_usage,\n @@ builtin/submodule.c: int cmd_submodule(int argc, const char **argv, const char *prefix)\n - \t * Tell the rest of git that any URLs we get don't come\n - \t * directly from the user, so it can apply policy as appropriate.\n \t */\n --\tstrvec_push(&cp.env_array, \"GIT_PROTOCOL_FROM_USER=0\");\n -+\tif (use_builtin)\n -+\t\txsetenv(\"GIT_PROTOCOL_FROM_USER\", \"0\", 1);\n -+\telse\n -+\t\tstrvec_push(&cp.env_array, \"GIT_PROTOCOL_FROM_USER=0\");\n -+\n + \tstrvec_push(&cp.env, \"GIT_PROTOCOL_FROM_USER=0\");\n \tsetup_helper_args(argc, argv, prefix, opt_quiet, opt_cached,\n --\t\t\t &cp.args);\n -+\t\t\t use_builtin ? &args : &cp.args);\n -+\n -+\tif (use_builtin) {\n -+\t\tret = cmd_submodule__helper(args.nr, args.v, prefix);\n -+\t\tgoto cleanup;\n -+\t}\n +-\t\t\t &cp.args, options);\n ++\t\t\t opt_recursive, &cp.args, options);\n \n \tcp.git_cmd = 1;\n \tcp.no_stdin = 0; /* for git submodule foreach */\n - \tcp.dir = startup_info->original_cwd;\n - \tret = run_command(&cp);\n - \n -+cleanup:\n -+\tif (!use_builtin)\n -+\t\t/* TODO: Double free? */\n -+\t\tstrvec_clear(&args);\n -+\n - \treturn ret;\n - }\n -\n - ## ci/run-build-and-tests.sh ##\n -@@ ci/run-build-and-tests.sh: linux-TEST-vars)\n - \texport GIT_TEST_MULTI_PACK_INDEX=1\n - \texport GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP=1\n - \texport GIT_TEST_ADD_I_USE_BUILTIN=0\n -+\texport GIT_TEST_SUBMODULE_USE_BUILTIN=1\n - \texport GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=master\n - \texport GIT_TEST_WRITE_REV_INDEX=1\n - \texport GIT_TEST_CHECKOUT_WORKERS=2\n \n - ## t/README ##\n -@@ t/README: GIT_TEST_ADD_I_USE_BUILTIN=, when false, disables the\n - built-in version of git add -i. See 'add.interactive.useBuiltin' in\n - git-config(1).\n + ## t/t7400-submodule-basic.sh ##\n +@@ t/t7400-submodule-basic.sh: test_expect_success 'submodule usage: -h' '\n + \ttest_must_be_empty err\n + '\n \n -+GIT_TEST_SUBMODULE_USE_BUILTIN=, when true, enables the\n -+built-in subprocess-less invocation of \"git submodule--helper\".\n -+See 'submodule.useBuiltin' in git-config(1).\n -+\n - GIT_TEST_INDEX_THREADS= enables exercising the multi-threaded loading\n - of the index for the whole test suite by bypassing the default number of\n - cache entries and thread minimums. Setting this to 1 will make the\n +-test_expect_success 'submodule usage: --recursive' '\n +-\ttest_expect_code 129 git submodule --recursive\n +-'\n +-\n + test_expect_success 'submodule usage: status --' '\n + \tgit submodule -- &&\n + \tgit submodule --end-of-options\n +@@ t/t7400-submodule-basic.sh: do\n + \t'\n + done\n + \n +-for opt in '--cached'\n ++for opt in '--cached' '--recursive'\n + do\n + \ttest_expect_success \"submodule usage: status $opt\" '\n + \t\tgit submodule $opt &&\n 2: 8fcd832e58f ! 10: 81f138e460c git-submodule.sh: remove unused $prefix variable\n @@ Metadata\n Author: Ævar Arnfjörð Bjarmason \n \n ## Commit message ##\n - git-submodule.sh: remove unused $prefix variable\n + submodule: don't use a subprocess to invoke \"submodule--helper\"\n \n - Remove the $prefix variable which is never set, and never resulted in\n - an option being passed to our \"git submodule--helper\". The\n - --recursive-prefix option to \"git submodule--helper\" is still used,\n - but only when it invokes itself.\n + In a preceding commit we created \"builtin/submodule.c\" and faithfully\n + tried to reproduce every aspect of \"git-submodule.sh\", including its\n + invocation of \"git submodule--helper\" as a sub-process.\n +\n + Let's do away with the sub-process and invoke\n + \"cmd_submodule__helper()\" directly. Eventually we'll want to do away\n + with \"builtin/submodule--helper.c\" altogether, but let's not do that\n + for now to avoid conflicts with other in-flight topics. Even without\n + those conflicts the resulting diff would be large. We can leave that\n + for a later cleanup.\n +\n + This speeds up invocations of all \"git submodule\" commands, E.g. a\n + trivial \"foreach\" command on git.git is around 1.50 times\n + faster[1]. For more expensive commands this'll make less of a\n + difference, as the fixed cost of invoking the sub-process will be\n + amortized away.\n +\n + $ git hyperfine -L rev HEAD~1,HEAD -s 'make CFLAGS=-O3' './git submodule foreach \"echo \\$name\"'\n + Benchmark 1: ./git submodule foreach \"echo \\$name\"' in 'HEAD~1\n + Time (mean ± σ): 9.7 ms ± 0.1 ms [User: 7.6 ms, System: 2.1 ms]\n + Range (min … max): 9.4 ms … 10.2 ms 285 runs\n +\n + Benchmark 2: ./git submodule foreach \"echo \\$name\"' in 'HEAD\n + Time (mean ± σ): 6.6 ms ± 0.1 ms [User: 5.1 ms, System: 1.5 ms]\n + Range (min … max): 6.2 ms … 7.2 ms 414 runs\n +\n + Summary\n + './git submodule foreach \"echo \\$name\"' in 'HEAD' ran\n + 1.48 ± 0.04 times faster than './git submodule foreach \"echo \\$name\"' in 'HEAD~1'\n +\n + It's also worth noting that some users were using e.g. \"git\n + submodule--helper list\" directly for performance reasons[2]. With\n + 31955475d1c (submodule--helper: remove unused \"list\" helper,\n + 2022-09-01) released with v2.38.0 the \"list\" command was no longer\n + provided. Users who had to switch to \"git submodule--helper foreach\"\n + were given a command that (on my system) is around 6.5x slower.\n +\n + Now the \"foreach\" is around 0.10x slower (due to the slight shell\n + overhead), with 31955475d1c reverted on top of this:\n +\n + $ hyperfine './git submodule--helper list' './git submodule foreach --quiet \"echo \\$name\"' --warmup 10\n + Benchmark 1: ./git submodule--helper list\n + Time (mean ± σ): 6.4 ms ± 0.1 ms [User: 5.0 ms, System: 1.5 ms]\n + Range (min … max): 6.2 ms … 7.2 ms 427 runs\n +\n + Benchmark 2: ./git submodule foreach --quiet \"echo \\$name\"\n + Time (mean ± σ): 7.0 ms ± 0.1 ms [User: 4.8 ms, System: 2.3 ms]\n + Range (min … max): 6.8 ms … 7.4 ms 390 runs\n +\n + Summary\n + './git submodule--helper list' ran\n + 1.10 ± 0.03 times faster than './git submodule foreach --quiet \"echo \\$name\"'\n +\n + I think it would make sense to implement a \"--format\" option for \"git\n + submodule foreach\" to help anyone who cares about that remaining\n + performance (and to improve the API, e.g. by supporting \"-z\"), but as\n + far as performance goes this makes the runtime acceptable again.\n +\n + The pattern in \"cmd_submodule_builtin()\" of saving \"struct strvec\"\n + arguments to a \"struct string_list\" and free()-ing them after the\n + \"argv\" has been modified by \"cmd_submodule__helper()\" is new, without\n + it we'd get various already-passing tests failing under SANITIZE=leak.\n +\n + 1. Using the \"git hyperfine\" wrapper for \"hyperfine\":\n + https://lore.kernel.org/git/211201.86r1aw9gbd.gmgdl@evledraar.gmail.com/\n + 2. https://lore.kernel.org/git/87czatrpyb.fsf@bernoul.li/\n \n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n - ## git-submodule.sh ##\n -@@ git-submodule.sh: files=\n - remote=\n - nofetch=\n - update=\n --prefix=\n - custom_name=\n - depth=\n - progress=\n -@@ git-submodule.sh: cmd_add()\n - \t\tusage\n - \tfi\n - \n --\tgit ${wt_prefix:+-C \"$wt_prefix\"} ${prefix:+--super-prefix \"$prefix\"} submodule--helper add ${GIT_QUIET:+--quiet} ${force:+--force} ${progress:+\"--progress\"} ${branch:+--branch \"$branch\"} ${reference_path:+--reference \"$reference_path\"} ${dissociate:+--dissociate} ${custom_name:+--name \"$custom_name\"} ${depth:+\"$depth\"} -- \"$@\"\n -+\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper add ${GIT_QUIET:+--quiet} ${force:+--force} ${progress:+\"--progress\"} ${branch:+--branch \"$branch\"} ${reference_path:+--reference \"$reference_path\"} ${dissociate:+--dissociate} ${custom_name:+--name \"$custom_name\"} ${depth:+\"$depth\"} -- \"$@\"\n + ## builtin/submodule.c ##\n +@@ builtin/submodule.c: static void setup_helper_args(int argc, const char **argv, const char *prefix,\n + \tstrvec_pushv(args, argv);\n }\n \n - #\n -@@ git-submodule.sh: cmd_init()\n - \t\tshift\n - \tdone\n ++static int cmd_submodule_builtin(struct strvec *args, const char *prefix)\n ++{\n ++\tsize_t i;\n ++\tstruct string_list to_free = STRING_LIST_INIT_DUP;\n ++\tint ret;\n ++\n ++\t/*\n ++\t * The cmd_submodule__helper() will treat the argv as\n ++\t * its own and modify it, so e.g. for \"git submodule\n ++\t * add\" the \"add\" argument will be removed, and we'll\n ++\t * thus leak from the strvec_push()'s in\n ++\t * setup_helper_args().\n ++\t *\n ++\t * So in lieu of some generic \"snapshot for a free\"\n ++\t * API for \"struct strvec\" squirrel away the pointers\n ++\t * to free with string_list_clear() later.\n ++\t */\n ++\tfor (i = 0; i < args->nr; i++)\n ++\t\tstring_list_append_nodup(&to_free, (char *)args->v[i]);\n ++\n ++\tret = cmd_submodule__helper(args->nr, args->v, prefix);\n ++\n ++\tstring_list_clear(&to_free, 0);\n ++\tfree(strvec_detach(args));\n ++\n ++\treturn ret;\n ++}\n ++\n + int cmd_submodule(int argc, const char **argv, const char *prefix)\n + {\n + \tint opt_quiet = 0;\n + \tint opt_cached = 0;\n + \tint opt_recursive = 0;\n +-\tstruct child_process cp = CHILD_PROCESS_INIT;\n ++\tstruct strvec args = STRVEC_INIT;\n + \tstruct option options[] = {\n + \t\tOPT__QUIET(&opt_quiet, N_(\"be quiet\")),\n + \t\tOPT_BOOL(0, \"cached\", &opt_cached,\n +@@ builtin/submodule.c: int cmd_submodule(int argc, const char **argv, const char *prefix)\n + \t * Tell the rest of git that any URLs we get don't come\n + \t * directly from the user, so it can apply policy as appropriate.\n + \t */\n +-\tstrvec_push(&cp.env, \"GIT_PROTOCOL_FROM_USER=0\");\n +-\tsetup_helper_args(argc, argv, prefix, opt_quiet, opt_cached,\n +-\t\t\t opt_recursive, &cp.args, options);\n ++\txsetenv(\"GIT_PROTOCOL_FROM_USER\", \"0\", 1);\n \n --\tgit ${wt_prefix:+-C \"$wt_prefix\"} ${prefix:+--super-prefix \"$prefix\"} submodule--helper init ${GIT_QUIET:+--quiet} -- \"$@\"\n -+\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper init ${GIT_QUIET:+--quiet} -- \"$@\"\n - }\n +-\tcp.git_cmd = 1;\n +-\tcp.no_stdin = 0; /* for git submodule foreach */\n +-\tcp.dir = startup_info->original_cwd;\n ++\tsetup_helper_args(argc, argv, prefix, opt_quiet, opt_cached,\n ++\t\t\t opt_recursive, &args, options);\n \n - #\n -@@ git-submodule.sh: cmd_update()\n - \t\t${init:+--init} \\\n - \t\t${nofetch:+--no-fetch} \\\n - \t\t${wt_prefix:+--prefix \"$wt_prefix\"} \\\n --\t\t${prefix:+--recursive-prefix \"$prefix\"} \\\n - \t\t${update:+--update \"$update\"} \\\n - \t\t${reference:+\"$reference\"} \\\n - \t\t${dissociate:+\"--dissociate\"} \\\n +-\treturn run_command(&cp);\n ++\treturn cmd_submodule_builtin(&args, prefix);\n + }\n 5: 124c062e3a1 < -: ----------- git-submodule.sh: normalize parsing of --cached\n 6: b1ca1183885 < -: ----------- submodule--helper: rename \"absorb-git-dirs\" to \"absorbgitdirs\"\n 8: 0c1a5063653 < -: ----------- submodule--helper: pretend to be \"git submodule\" in \"-h\" output\n12: 57b9df29ea6 < -: ----------- submodule--helper: have --require-init imply --init\n13: 20db979a094 < -: ----------- submodule--helper: understand --checkout, --merge and --rebase synonyms\n14: 1cb40a5f42e < -: ----------- git-submodule doc: document the -v\" option to \"update\"\n15: 0c388eed1d1 < -: ----------- submodule--helper: understand -v option for \"update\"\n17: 59a72296967 < -: ----------- git-submodule.sh: use \"$quiet\", not \"$GIT_QUIET\"\n18: c5796878f0b < -: ----------- git-submodule.sh: simplify parsing loop\n-- \n2.38.0.1091.gf9d18265e59\n\n"},{"id":"465076","messageId":"patch-04.10-db6a09ee42a-20221017T115544Z-avarab@gmail.com","threadId":"58637","inReplyTo":"cover-00.10-00000000000-20221017T115544Z-avarab@gmail.com","subject":"[PATCH 04/10] git-submodule.sh: dispatch \"foreach\" to helper","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-17T12:09:19Z","receivedAt":"2022-10-17T12:09:46Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Dispatch the \"git submodule foreach\" command directly to \"git\nsubmodule--helper foreach\". This case requires the addition of the\nPARSE_OPT_STOP_AT_NON_OPTION flag, since the shellscript was\nunconditionally adding \"--\" to the \"git submodule--helper\"\ncommand-line.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/submodule--helper.c | 3 ++-\n git-submodule.sh | 37 +++----------------------------------\n 2 files changed, 5 insertions(+), 35 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 0b4acb442b2..d11e1003019 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -403,7 +403,8 @@ static int module_foreach(int argc, const char **argv, const char *prefix)\n \tint ret = 1;\n \n \targc = parse_options(argc, argv, prefix, module_foreach_options,\n-\t\t\t git_submodule_helper_usage, 0);\n+\t\t\t git_submodule_helper_usage,\n+\t\t\t PARSE_OPT_STOP_AT_NON_OPTION);\n \n \tif (module_list_compute(0, NULL, prefix, &pathspec, &list) < 0)\n \t\tgoto cleanup;\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 2bdff5119c1..7874e33beea 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -50,37 +50,6 @@ jobs=\n recommend_shallow=\n filter=\n \n-\n-# Execute an arbitrary command sequence in each checked out\n-# submodule\n-#\n-# $@ = command to execute\n-#\n-cmd_foreach()\n-{\n-\t# parse $args after \"submodule ... foreach\".\n-\twhile test $# -ne 0\n-\tdo\n-\t\tcase \"$1\" in\n-\t\t-q|--quiet)\n-\t\t\tquiet=1\n-\t\t\t;;\n-\t\t--recursive)\n-\t\t\trecursive=1\n-\t\t\t;;\n-\t\t-*)\n-\t\t\tusage\n-\t\t\t;;\n-\t\t*)\n-\t\t\tbreak\n-\t\t\t;;\n-\t\tesac\n-\t\tshift\n-\tdone\n-\n-\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper foreach ${quiet:+--quiet} ${recursive:+--recursive} -- \"$@\"\n-}\n-\n #\n # Update each submodule path to correct revision, using clone and checkout as needed\n #\n@@ -263,10 +232,10 @@ case \"$command\" in\n absorbgitdirs)\n \tgit submodule--helper \"$command\" --prefix \"$wt_prefix\" \"$@\"\n \t;;\n-foreach | update)\n-\t\"cmd_$command\" \"$@\"\n+update)\n+\tcmd_update \"$@\"\n \t;;\n-add | init | deinit | set-branch | set-url | status | summary | sync)\n+add | foreach | init | deinit | set-branch | set-url | status | summary | sync)\n \tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper \"$command\" \\\n \t\t${quiet:+--quiet} ${cached:+--cached} \"$@\"\n \t;;\n-- \n2.38.0.1091.gf9d18265e59\n\n"},{"id":"465078","messageId":"patch-03.10-97cb470c96a-20221017T115544Z-avarab@gmail.com","threadId":"58637","inReplyTo":"cover-00.10-00000000000-20221017T115544Z-avarab@gmail.com","subject":"[PATCH 03/10] git-submodule.sh: dispatch directly to helper","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-17T12:09:18Z","receivedAt":"2022-10-17T12:09:48Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the dispatching logic in \"git-submodule.sh\" for the \"add\",\n\"init\", \"deinit\", \"set-branch\", \"set-url\", \"summary\" and \"status\"\nsub-commands to do away with the argument parsing in git-submodule.sh,\nand instead dispatch directly to \"git submodule--helper\".\n\nAs in a preceding commit the only functional change here should be\nthat on invalid options we'll now emit more targeted \"-h\" output.\n\nThe isnumber() helper function was only used in the now-removed\ncmd_summary(), the same goes for the $files variable. The $custom_name\nand $branch variables were only used in cmd_add().\n\nSince there are no dashed commands anymore in git-submodule.sh we can\nget rid of the \"$(echo | sed ...)\" one-liner to change e.g. \"set-url\"\nto \"set_url\".\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n git-submodule.sh | 321 +----------------------------------------------\n 1 file changed, 5 insertions(+), 316 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 3fdfe864d37..2bdff5119c1 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -31,20 +31,17 @@ export GIT_PROTOCOL_FROM_USER\n \n command=\n quiet=\n-branch=\n force=\n reference=\n cached=\n recursive=\n init=\n require_init=\n-files=\n remote=\n nofetch=\n rebase=\n merge=\n checkout=\n-custom_name=\n depth=\n progress=\n dissociate=\n@@ -53,86 +50,7 @@ jobs=\n recommend_shallow=\n filter=\n \n-isnumber()\n-{\n-\tn=$(($1 + 0)) 2>/dev/null && test \"$n\" = \"$1\"\n-}\n-\n-#\n-# Add a new submodule to the working tree, .gitmodules and the index\n-#\n-# $@ = repo path\n-#\n-# optional branch is stored in global branch variable\n-#\n-cmd_add()\n-{\n-\t# parse $args after \"submodule ... add\".\n-\treference_path=\n-\twhile test $# -ne 0\n-\tdo\n-\t\tcase \"$1\" in\n-\t\t-b | --branch)\n-\t\t\tcase \"$2\" in '') usage ;; esac\n-\t\t\tbranch=$2\n-\t\t\tshift\n-\t\t\t;;\n-\t\t-f | --force)\n-\t\t\tforce=$1\n-\t\t\t;;\n-\t\t-q|--quiet)\n-\t\t\tquiet=1\n-\t\t\t;;\n-\t\t--progress)\n-\t\t\tprogress=1\n-\t\t\t;;\n-\t\t--reference)\n-\t\t\tcase \"$2\" in '') usage ;; esac\n-\t\t\treference_path=$2\n-\t\t\tshift\n-\t\t\t;;\n-\t\t--reference=*)\n-\t\t\treference_path=\"${1#--reference=}\"\n-\t\t\t;;\n-\t\t--dissociate)\n-\t\t\tdissociate=1\n-\t\t\t;;\n-\t\t--name)\n-\t\t\tcase \"$2\" in '') usage ;; esac\n-\t\t\tcustom_name=$2\n-\t\t\tshift\n-\t\t\t;;\n-\t\t--depth)\n-\t\t\tcase \"$2\" in '') usage ;; esac\n-\t\t\tdepth=\"--depth=$2\"\n-\t\t\tshift\n-\t\t\t;;\n-\t\t--depth=*)\n-\t\t\tdepth=$1\n-\t\t\t;;\n-\t\t--)\n-\t\t\tshift\n-\t\t\tbreak\n-\t\t\t;;\n-\t\t-*)\n-\t\t\tusage\n-\t\t\t;;\n-\t\t*)\n-\t\t\tbreak\n-\t\t\t;;\n-\t\tesac\n-\t\tshift\n-\tdone\n-\n-\tif test -z \"$1\"\n-\tthen\n-\t\tusage\n-\tfi\n-\n-\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper add ${quiet:+--quiet} ${force:+--force} ${progress:+\"--progress\"} ${branch:+--branch \"$branch\"} ${reference_path:+--reference \"$reference_path\"} ${dissociate:+--dissociate} ${custom_name:+--name \"$custom_name\"} ${depth:+\"$depth\"} -- \"$@\"\n-}\n \n-#\n # Execute an arbitrary command sequence in each checked out\n # submodule\n #\n@@ -163,73 +81,6 @@ cmd_foreach()\n \tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper foreach ${quiet:+--quiet} ${recursive:+--recursive} -- \"$@\"\n }\n \n-#\n-# Register submodules in .git/config\n-#\n-# $@ = requested paths (default to all)\n-#\n-cmd_init()\n-{\n-\t# parse $args after \"submodule ... init\".\n-\twhile test $# -ne 0\n-\tdo\n-\t\tcase \"$1\" in\n-\t\t-q|--quiet)\n-\t\t\tquiet=1\n-\t\t\t;;\n-\t\t--)\n-\t\t\tshift\n-\t\t\tbreak\n-\t\t\t;;\n-\t\t-*)\n-\t\t\tusage\n-\t\t\t;;\n-\t\t*)\n-\t\t\tbreak\n-\t\t\t;;\n-\t\tesac\n-\t\tshift\n-\tdone\n-\n-\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper init ${quiet:+--quiet} -- \"$@\"\n-}\n-\n-#\n-# Unregister submodules from .git/config and remove their work tree\n-#\n-cmd_deinit()\n-{\n-\t# parse $args after \"submodule ... deinit\".\n-\tdeinit_all=\n-\twhile test $# -ne 0\n-\tdo\n-\t\tcase \"$1\" in\n-\t\t-f|--force)\n-\t\t\tforce=$1\n-\t\t\t;;\n-\t\t-q|--quiet)\n-\t\t\tquiet=1\n-\t\t\t;;\n-\t\t--all)\n-\t\t\tdeinit_all=t\n-\t\t\t;;\n-\t\t--)\n-\t\t\tshift\n-\t\t\tbreak\n-\t\t\t;;\n-\t\t-*)\n-\t\t\tusage\n-\t\t\t;;\n-\t\t*)\n-\t\t\tbreak\n-\t\t\t;;\n-\t\tesac\n-\t\tshift\n-\tdone\n-\n-\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper deinit ${quiet:+--quiet} ${force:+--force} ${deinit_all:+--all} -- \"$@\"\n-}\n-\n #\n # Update each submodule path to correct revision, using clone and checkout as needed\n #\n@@ -360,168 +211,6 @@ cmd_update()\n \t\t\"$@\"\n }\n \n-#\n-# Configures a submodule's default branch\n-#\n-# $@ = requested path\n-#\n-cmd_set_branch() {\n-\tdefault=\n-\tbranch=\n-\n-\twhile test $# -ne 0\n-\tdo\n-\t\tcase \"$1\" in\n-\t\t-q|--quiet)\n-\t\t\t# we don't do anything with this but we need to accept it\n-\t\t\t;;\n-\t\t-d|--default)\n-\t\t\tdefault=1\n-\t\t\t;;\n-\t\t-b|--branch)\n-\t\t\tcase \"$2\" in '') usage ;; esac\n-\t\t\tbranch=$2\n-\t\t\tshift\n-\t\t\t;;\n-\t\t--)\n-\t\t\tshift\n-\t\t\tbreak\n-\t\t\t;;\n-\t\t-*)\n-\t\t\tusage\n-\t\t\t;;\n-\t\t*)\n-\t\t\tbreak\n-\t\t\t;;\n-\t\tesac\n-\t\tshift\n-\tdone\n-\n-\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper set-branch ${quiet:+--quiet} ${branch:+--branch \"$branch\"} ${default:+--default} -- \"$@\"\n-}\n-\n-#\n-# Configures a submodule's remote url\n-#\n-# $@ = requested path, requested url\n-#\n-cmd_set_url() {\n-\twhile test $# -ne 0\n-\tdo\n-\t\tcase \"$1\" in\n-\t\t-q|--quiet)\n-\t\t\tquiet=1\n-\t\t\t;;\n-\t\t--)\n-\t\t\tshift\n-\t\t\tbreak\n-\t\t\t;;\n-\t\t-*)\n-\t\t\tusage\n-\t\t\t;;\n-\t\t*)\n-\t\t\tbreak\n-\t\t\t;;\n-\t\tesac\n-\t\tshift\n-\tdone\n-\n-\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper set-url ${quiet:+--quiet} -- \"$@\"\n-}\n-\n-#\n-# Show commit summary for submodules in index or working tree\n-#\n-# If '--cached' is given, show summary between index and given commit,\n-# or between working tree and given commit\n-#\n-# $@ = [commit (default 'HEAD'),] requested paths (default all)\n-#\n-cmd_summary() {\n-\tsummary_limit=-1\n-\tfor_status=\n-\tdiff_cmd=diff-index\n-\n-\t# parse $args after \"submodule ... summary\".\n-\twhile test $# -ne 0\n-\tdo\n-\t\tcase \"$1\" in\n-\t\t--cached)\n-\t\t\tcached=1\n-\t\t\t;;\n-\t\t--files)\n-\t\t\tfiles=\"$1\"\n-\t\t\t;;\n-\t\t--for-status)\n-\t\t\tfor_status=\"$1\"\n-\t\t\t;;\n-\t\t-n|--summary-limit)\n-\t\t\tsummary_limit=\"$2\"\n-\t\t\tisnumber \"$summary_limit\" || usage\n-\t\t\tshift\n-\t\t\t;;\n-\t\t--summary-limit=*)\n-\t\t\tsummary_limit=\"${1#--summary-limit=}\"\n-\t\t\tisnumber \"$summary_limit\" || usage\n-\t\t\t;;\n-\t\t--)\n-\t\t\tshift\n-\t\t\tbreak\n-\t\t\t;;\n-\t\t-*)\n-\t\t\tusage\n-\t\t\t;;\n-\t\t*)\n-\t\t\tbreak\n-\t\t\t;;\n-\t\tesac\n-\t\tshift\n-\tdone\n-\n-\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper summary ${files:+--files} ${cached:+--cached} ${for_status:+--for-status} ${summary_limit:+-n $summary_limit} -- \"$@\"\n-}\n-#\n-# List all submodules, prefixed with:\n-# - submodule not initialized\n-# + different revision checked out\n-#\n-# If --cached was specified the revision in the index will be printed\n-# instead of the currently checked out revision.\n-#\n-# $@ = requested paths (default to all)\n-#\n-cmd_status()\n-{\n-\t# parse $args after \"submodule ... status\".\n-\twhile test $# -ne 0\n-\tdo\n-\t\tcase \"$1\" in\n-\t\t-q|--quiet)\n-\t\t\tquiet=1\n-\t\t\t;;\n-\t\t--cached)\n-\t\t\tcached=1\n-\t\t\t;;\n-\t\t--recursive)\n-\t\t\trecursive=1\n-\t\t\t;;\n-\t\t--)\n-\t\t\tshift\n-\t\t\tbreak\n-\t\t\t;;\n-\t\t-*)\n-\t\t\tusage\n-\t\t\t;;\n-\t\t*)\n-\t\t\tbreak\n-\t\t\t;;\n-\t\tesac\n-\t\tshift\n-\tdone\n-\n-\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper status ${quiet:+--quiet} ${cached:+--cached} ${recursive:+--recursive} -- \"$@\"\n-}\n-\n # This loop parses the command line arguments to find the\n # subcommand name to dispatch. Parsing of the subcommand specific\n # options are primarily done by the subcommand implementations.\n@@ -574,11 +263,11 @@ case \"$command\" in\n absorbgitdirs)\n \tgit submodule--helper \"$command\" --prefix \"$wt_prefix\" \"$@\"\n \t;;\n-sync)\n-\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper \"$command\" \\\n-\t\t${quiet:+--quiet} \"$@\"\n+foreach | update)\n+\t\"cmd_$command\" \"$@\"\n \t;;\n-*)\n-\t\"cmd_$(echo $command | sed -e s/-/_/g)\" \"$@\"\n+add | init | deinit | set-branch | set-url | status | summary | sync)\n+\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper \"$command\" \\\n+\t\t${quiet:+--quiet} ${cached:+--cached} \"$@\"\n \t;;\n esac\n-- \n2.38.0.1091.gf9d18265e59\n\n"},{"id":"465079","messageId":"patch-06.10-25fadf3ffc1-20221017T115544Z-avarab@gmail.com","threadId":"58637","inReplyTo":"cover-00.10-00000000000-20221017T115544Z-avarab@gmail.com","subject":"[PATCH 06/10] git-submodule.sh: don't support top-level \"--cached\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-17T12:09:21Z","receivedAt":"2022-10-17T12:09:53Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Since the preceding commit all sub-commands of \"git submodule\" have\nbeen dispatched to \"git submodule--helper\" directly, we therefore\ndon't need to emit \"usage()\" if we see \"--cached\" without the\nsub-command being \"status\" or \"summary\", we can trust that\nparse_options() will spot that and barf on it.\n\nThis does change one obscure aspect of undocumented behavior, for\n\"status\" and \"summary\" we supported these undocumented forms:\n\n git submodule --cached (status | summary)\n\nAs noted in a preceding commit to git-submodule.sh which removed the\n\"--branch\" special-case, this comes down to emergent behavior seen in\n5c08dbbdf1a (git-submodule: fix subcommand parser,\n2008-01-15). I.e. we wanted to support was for subcommand-less invocations like:\n\n git submodule --cached\n\nTo be synonymous with invocations that explicitly named the \"status\"\nsub-command:\n\n git submodule status --cached\n\nBut we did not intend to mix the two, and allow \"--cached\" to be an\noption to the top-level \"submodule\" command when the \"status\" or\n\"summary\" sub-commands were explicitly provided.\n\nLet's remove this undocumented edge case, which makes a subsequent\nremoval of git-submodule.sh easier to reason about. The test case\nadded here is duplicated from the existing for-loop, except for the\ndifferent and desired handling of \"git submodule --cached status\".\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n git-submodule.sh | 15 ++++++++-------\n t/t7400-submodule-basic.sh | 15 +++++++++++++--\n 2 files changed, 21 insertions(+), 9 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex ac2f95c1285..4f8f62ce981 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -43,7 +43,14 @@ do\n \t\tquiet=1\n \t\t;;\n \t--cached)\n-\t\tcached=1\n+\t\tif test -z \"$command\"\n+\t\tthen\n+\t\t\tcached=1 &&\n+\t\t\tshift &&\n+\t\t\tbreak\n+\t\telse\n+\t\t\tusage\n+\t\tfi\n \t\t;;\n \t--)\n \t\tbreak\n@@ -69,12 +76,6 @@ then\n fi\n fi\n \n-# \"--cached\" is accepted only by \"status\" and \"summary\"\n-if test -n \"$cached\" && test \"$command\" != status && test \"$command\" != summary\n-then\n-\tusage\n-fi\n-\n case \"$command\" in\n absorbgitdirs)\n \tgit submodule--helper \"$command\" --prefix \"$wt_prefix\" \"$@\"\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex b50db3f1031..d8f7d6ee29a 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -31,7 +31,7 @@ test_expect_success 'submodule usage: status --' '\n \ttest_expect_code 1 git submodule --end-of-options\n '\n \n-for opt in '--quiet' '--cached'\n+for opt in '--quiet'\n do\n \ttest_expect_success \"submodule usage: status $opt\" '\n \t\tgit submodule $opt &&\n@@ -40,6 +40,17 @@ do\n \t'\n done\n \n+for opt in '--cached'\n+do\n+\ttest_expect_success \"submodule usage: status $opt\" '\n+\t\tgit submodule $opt &&\n+\t\tgit submodule status $opt &&\n+\t\ttest_expect_code 1 git submodule $opt status >out 2>err &&\n+\t\tgrep \"^usage: git submodule\" err &&\n+\t\ttest_must_be_empty out\n+\t'\n+done\n+\n test_expect_success 'submodule deinit works on empty repository' '\n \tgit submodule deinit --all\n '\n@@ -576,7 +587,7 @@ test_expect_success 'status should be \"modified\" after submodule commit' '\n '\n \n test_expect_success 'the --cached sha1 should be rev1' '\n-\tgit submodule --cached status >list &&\n+\tgit submodule status --cached >list &&\n \tgrep \"^+$rev1\" list\n '\n \n-- \n2.38.0.1091.gf9d18265e59\n\n"},{"id":"465080","messageId":"patch-05.10-7d9c13eb637-20221017T115544Z-avarab@gmail.com","threadId":"58637","inReplyTo":"cover-00.10-00000000000-20221017T115544Z-avarab@gmail.com","subject":"[PATCH 05/10] git-submodule.sh: dispatch \"update\" to helper","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-17T12:09:20Z","receivedAt":"2022-10-17T12:09:55Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Dispatch the \"update\" command directly to \"git submodule--helper\nupdate\", rather than doing our own argument parsing in\ngit-submodule.sh. As this is the last cmd_*() function to be removed\nfrom git-submodule.sh we can do some larger cleanup of that file as a\nresult.\n\nAs noted in a preceding commit the only behavior change here should be\nthe desirable change of better \"-h\" output, and that this\nimplementation understands the \"--verbose\" synonym for \"-v\". Let's\nupdate the documentation to reflect the new \"--verbose\" synonym.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n git-submodule.sh | 156 +----------------------------------------------\n 1 file changed, 2 insertions(+), 154 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 7874e33beea..ac2f95c1285 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -31,160 +31,7 @@ export GIT_PROTOCOL_FROM_USER\n \n command=\n quiet=\n-force=\n-reference=\n cached=\n-recursive=\n-init=\n-require_init=\n-remote=\n-nofetch=\n-rebase=\n-merge=\n-checkout=\n-depth=\n-progress=\n-dissociate=\n-single_branch=\n-jobs=\n-recommend_shallow=\n-filter=\n-\n-#\n-# Update each submodule path to correct revision, using clone and checkout as needed\n-#\n-# $@ = requested paths (default to all)\n-#\n-cmd_update()\n-{\n-\t# parse $args after \"submodule ... update\".\n-\twhile test $# -ne 0\n-\tdo\n-\t\tcase \"$1\" in\n-\t\t-q|--quiet)\n-\t\t\tquiet=1\n-\t\t\t;;\n-\t\t--progress)\n-\t\t\tprogress=1\n-\t\t\t;;\n-\t\t-i|--init)\n-\t\t\tinit=1\n-\t\t\t;;\n-\t\t--require-init)\n-\t\t\trequire_init=1\n-\t\t\t;;\n-\t\t--remote)\n-\t\t\tremote=1\n-\t\t\t;;\n-\t\t-N|--no-fetch)\n-\t\t\tnofetch=1\n-\t\t\t;;\n-\t\t-f|--force)\n-\t\t\tforce=$1\n-\t\t\t;;\n-\t\t-r|--rebase)\n-\t\t\trebase=1\n-\t\t\t;;\n-\t\t--reference)\n-\t\t\tcase \"$2\" in '') usage ;; esac\n-\t\t\treference=\"--reference=$2\"\n-\t\t\tshift\n-\t\t\t;;\n-\t\t--reference=*)\n-\t\t\treference=\"$1\"\n-\t\t\t;;\n-\t\t--dissociate)\n-\t\t\tdissociate=1\n-\t\t\t;;\n-\t\t-m|--merge)\n-\t\t\tmerge=1\n-\t\t\t;;\n-\t\t--recursive)\n-\t\t\trecursive=1\n-\t\t\t;;\n-\t\t--checkout)\n-\t\t\tcheckout=1\n-\t\t\t;;\n-\t\t--recommend-shallow)\n-\t\t\trecommend_shallow=\"--recommend-shallow\"\n-\t\t\t;;\n-\t\t--no-recommend-shallow)\n-\t\t\trecommend_shallow=\"--no-recommend-shallow\"\n-\t\t\t;;\n-\t\t--depth)\n-\t\t\tcase \"$2\" in '') usage ;; esac\n-\t\t\tdepth=\"--depth=$2\"\n-\t\t\tshift\n-\t\t\t;;\n-\t\t--depth=*)\n-\t\t\tdepth=$1\n-\t\t\t;;\n-\t\t-j|--jobs)\n-\t\t\tcase \"$2\" in '') usage ;; esac\n-\t\t\tjobs=\"--jobs=$2\"\n-\t\t\tshift\n-\t\t\t;;\n-\t\t--jobs=*)\n-\t\t\tjobs=$1\n-\t\t\t;;\n-\t\t--single-branch)\n-\t\t\tsingle_branch=\"--single-branch\"\n-\t\t\t;;\n-\t\t--no-single-branch)\n-\t\t\tsingle_branch=\"--no-single-branch\"\n-\t\t\t;;\n-\t\t--filter)\n-\t\t\tcase \"$2\" in '') usage ;; esac\n-\t\t\tfilter=\"--filter=$2\"\n-\t\t\tshift\n-\t\t\t;;\n-\t\t--filter=*)\n-\t\t\tfilter=\"$1\"\n-\t\t\t;;\n-\t\t--)\n-\t\t\tshift\n-\t\t\tbreak\n-\t\t\t;;\n-\t\t-*)\n-\t\t\tusage\n-\t\t\t;;\n-\t\t*)\n-\t\t\tbreak\n-\t\t\t;;\n-\t\tesac\n-\t\tshift\n-\tdone\n-\n-\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper update \\\n-\t\t${quiet:+--quiet} \\\n-\t\t${force:+--force} \\\n-\t\t${progress:+\"--progress\"} \\\n-\t\t${remote:+--remote} \\\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-\t\t${reference:+\"$reference\"} \\\n-\t\t${dissociate:+\"--dissociate\"} \\\n-\t\t${depth:+\"$depth\"} \\\n-\t\t${require_init:+--require-init} \\\n-\t\t${dissociate:+\"--dissociate\"} \\\n-\t\t$single_branch \\\n-\t\t$recommend_shallow \\\n-\t\t$jobs \\\n-\t\t$filter \\\n-\t\t-- \\\n-\t\t\"$@\"\n-}\n-\n-# This loop parses the command line arguments to find the\n-# subcommand name to dispatch. Parsing of the subcommand specific\n-# options are primarily done by the subcommand implementations.\n-# Subcommand specific options such as --branch and --cached are\n-# parsed here as well, for backward compatibility.\n \n while test $# != 0 && test -z \"$command\"\n do\n@@ -233,7 +80,8 @@ absorbgitdirs)\n \tgit submodule--helper \"$command\" --prefix \"$wt_prefix\" \"$@\"\n \t;;\n update)\n-\tcmd_update \"$@\"\n+\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper \"$command\" \\\n+\t\t${quiet:+--quiet} ${wt_prefix:+--prefix \"$wt_prefix\"} \"$@\"\n \t;;\n add | foreach | init | deinit | set-branch | set-url | status | summary | sync)\n \tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper \"$command\" \\\n-- \n2.38.0.1091.gf9d18265e59\n\n"},{"id":"465081","messageId":"patch-08.10-8dbb81e2fa5-20221017T115544Z-avarab@gmail.com","threadId":"58637","inReplyTo":"cover-00.10-00000000000-20221017T115544Z-avarab@gmail.com","subject":"[PATCH 08/10] submodule: support \"--\" with no other arguments","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-17T12:09:23Z","receivedAt":"2022-10-17T12:10:09Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Address an edge case that \"git-submodule.sh\" has had all along, but\nwhich became painfully obvious in the *.sh to *.c migration in the\npreceding commit.\n\nWe didn't support the \"--\" delimiter in the argument-less\ninvocation. Let's not bend over backwards to behave unusually in this\nscenario, simply accepting \"--\" is harmless.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/git-submodule.txt | 2 +-\n builtin/submodule.c | 12 ++----------\n t/t7400-submodule-basic.sh | 4 ++--\n 3 files changed, 5 insertions(+), 13 deletions(-)\n\ndiff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt\nindex 4d3ab6b9f92..345ebcafb9c 100644\n--- a/Documentation/git-submodule.txt\n+++ b/Documentation/git-submodule.txt\n@@ -9,7 +9,7 @@ git-submodule - Initialize, update or inspect submodules\n SYNOPSIS\n --------\n [verse]\n-'git submodule' [--quiet] [--cached]\n+'git submodule' [--quiet] [--cached] [--]\n 'git submodule' [--quiet] add [] [--] []\n 'git submodule' [--quiet] status [--cached] [--recursive] [--] [...]\n 'git submodule' [--quiet] init [--] [...]\ndiff --git a/builtin/submodule.c b/builtin/submodule.c\nindex 7e3499f3376..1d77f2d0964 100644\n--- a/builtin/submodule.c\n+++ b/builtin/submodule.c\n@@ -8,7 +8,7 @@\n #include \"strvec.h\"\n \n #define BUILTIN_SUBMODULE_USAGE \\\n-\t\"git submodule [--quiet] [--cached]\"\n+\t\"git submodule [--quiet] [--cached] [--]\"\n \n #define BUILTIN_SUBMODULE_ADD_USAGE \\\n \tN_(\"git submodule [--quiet] add [-b ] [-f | --force] [--name ]\\n\" \\\n@@ -91,14 +91,6 @@ static void setup_helper_args(int argc, const char **argv, const char *prefix,\n \targv++;\n \targc--;\n \n-\t/*\n-\t * This is stupid, but don't support \"[--]\" to\n-\t * subcommand-less \"git-submodule\" for now.\n-\t */\n-\tif (!strcmp(cmd, \"--\") || !strcmp(cmd, \"--end-of-options\"))\n-\t\tusage_msg_optf(_(\"need explicit sub-command name to delimit with '%s'\"),\n-\t\t\t git_submodule_usage, options, cmd);\n-\n \t/* Options that need to go before user-supplied options */\n \tif (!strcmp(cmd, \"absorbgitdirs\"))\n \t\tdo_quiet_cache = 0;\n@@ -133,7 +125,7 @@ int cmd_submodule(int argc, const char **argv, const char *prefix)\n \t};\n \n \targc = parse_options(argc, argv, prefix, options, git_submodule_usage,\n-\t\t\t PARSE_OPT_STOP_AT_NON_OPTION | PARSE_OPT_KEEP_DASHDASH);\n+\t\t\t PARSE_OPT_STOP_AT_NON_OPTION);\n \n \t/*\n \t * Tell the rest of git that any URLs we get don't come\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex 19df3407ef1..c524398e805 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -25,8 +25,8 @@ test_expect_success 'submodule usage: --recursive' '\n '\n \n test_expect_success 'submodule usage: status --' '\n-\ttest_expect_code 129 git submodule -- &&\n-\ttest_expect_code 129 git submodule --end-of-options\n+\tgit submodule -- &&\n+\tgit submodule --end-of-options\n '\n \n for opt in '--quiet'\n-- \n2.38.0.1091.gf9d18265e59\n\n"},{"id":"465082","messageId":"patch-09.10-7f232c5e503-20221017T115544Z-avarab@gmail.com","threadId":"58637","inReplyTo":"cover-00.10-00000000000-20221017T115544Z-avarab@gmail.com","subject":"[PATCH 09/10] submodule: support sub-command-less \"--recursive\" option","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-17T12:09:24Z","receivedAt":"2022-10-17T12:10:16Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The inability to specify \"--recursive\" when we're not providing a\nsub-command name appears to have been an omission in 15fc56a8536 (git\nsubmodule foreach: Add --recursive to recurse into nested submodules,\n2009-08-19). Let's support it along with the other \"status\" options.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/git-submodule.txt | 2 +-\n builtin/submodule.c | 16 +++++++++++++---\n t/t7400-submodule-basic.sh | 6 +-----\n 3 files changed, 15 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt\nindex 345ebcafb9c..0c918390f2f 100644\n--- a/Documentation/git-submodule.txt\n+++ b/Documentation/git-submodule.txt\n@@ -9,7 +9,7 @@ git-submodule - Initialize, update or inspect submodules\n SYNOPSIS\n --------\n [verse]\n-'git submodule' [--quiet] [--cached] [--]\n+'git submodule' [--quiet] [--cached] [--recursive] [--]\n 'git submodule' [--quiet] add [] [--] []\n 'git submodule' [--quiet] status [--cached] [--recursive] [--] [...]\n 'git submodule' [--quiet] init [--] [...]\ndiff --git a/builtin/submodule.c b/builtin/submodule.c\nindex 1d77f2d0964..ca8e273b6e9 100644\n--- a/builtin/submodule.c\n+++ b/builtin/submodule.c\n@@ -64,7 +64,8 @@ static const char * const git_submodule_usage[] = {\n };\n \n static void setup_helper_args(int argc, const char **argv, const char *prefix,\n-\t\t\t int quiet, int cached, struct strvec *args,\n+\t\t\t int quiet, int cached, int recursive,\n+\t\t\t struct strvec *args,\n \t\t\t const struct option *options)\n {\n \tconst char *cmd;\n@@ -79,10 +80,13 @@ static void setup_helper_args(int argc, const char **argv, const char *prefix,\n \t\treturn;\n \t}\n \n-\t/* Did we get --cached with a command? */\n+\t/* Did we get a forbidden top-level option with a command? */\n \tif (cached)\n \t\tusage_msg_optf(_(\"'%s' option is only supported with explicit 'status'\"),\n \t\t\t git_submodule_usage, options, \"--cached\");\n+\tif (recursive)\n+\t\tusage_msg_optf(_(\"'%s' option is only supported with explicit 'status'\"),\n+\t\t\t git_submodule_usage, options, \"--recursive\");\n \n \n \t/* Either a valid command, or submodule--helper will barf! */\n@@ -92,6 +96,9 @@ static void setup_helper_args(int argc, const char **argv, const char *prefix,\n \targc--;\n \n \t/* Options that need to go before user-supplied options */\n+\tif (!strcmp(cmd, \"status\") && recursive)\n+\t\tstrvec_push(args, \"--recursive\");\n+\n \tif (!strcmp(cmd, \"absorbgitdirs\"))\n \t\tdo_quiet_cache = 0;\n \telse if (!strcmp(cmd, \"update\"))\n@@ -116,11 +123,14 @@ int cmd_submodule(int argc, const char **argv, const char *prefix)\n {\n \tint opt_quiet = 0;\n \tint opt_cached = 0;\n+\tint opt_recursive = 0;\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n \tstruct option options[] = {\n \t\tOPT__QUIET(&opt_quiet, N_(\"be quiet\")),\n \t\tOPT_BOOL(0, \"cached\", &opt_cached,\n \t\t\t N_(\"print the OID of submodules\")),\n+\t\tOPT_BOOL(0, \"recursive\", &opt_recursive,\n+\t\t\t N_(\"recurse into nested submodules\")),\n \t\tOPT_END()\n \t};\n \n@@ -133,7 +143,7 @@ int cmd_submodule(int argc, const char **argv, const char *prefix)\n \t */\n \tstrvec_push(&cp.env, \"GIT_PROTOCOL_FROM_USER=0\");\n \tsetup_helper_args(argc, argv, prefix, opt_quiet, opt_cached,\n-\t\t\t &cp.args, options);\n+\t\t\t opt_recursive, &cp.args, options);\n \n \tcp.git_cmd = 1;\n \tcp.no_stdin = 0; /* for git submodule foreach */\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex c524398e805..7cafc2e1102 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -20,10 +20,6 @@ test_expect_success 'submodule usage: -h' '\n \ttest_must_be_empty err\n '\n \n-test_expect_success 'submodule usage: --recursive' '\n-\ttest_expect_code 129 git submodule --recursive\n-'\n-\n test_expect_success 'submodule usage: status --' '\n \tgit submodule -- &&\n \tgit submodule --end-of-options\n@@ -38,7 +34,7 @@ do\n \t'\n done\n \n-for opt in '--cached'\n+for opt in '--cached' '--recursive'\n do\n \ttest_expect_success \"submodule usage: status $opt\" '\n \t\tgit submodule $opt &&\n-- \n2.38.0.1091.gf9d18265e59\n\n"},{"id":"465083","messageId":"patch-07.10-2c77ed38d90-20221017T115544Z-avarab@gmail.com","threadId":"58637","inReplyTo":"cover-00.10-00000000000-20221017T115544Z-avarab@gmail.com","subject":"[PATCH 07/10] submodule: make it a built-in, remove git-submodule.sh","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-17T12:09:22Z","receivedAt":"2022-10-17T12:10:18Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Replace the \"git-submodule.sh\" script with a built-in\n\"builtin/submodule.c. For\" now this new command is only a dumb\ndispatcher that uses run-command.c to invoke \"git submodule--helper\",\njust as \"git-submodule.sh\" used to do.\n\nThis is obviously not ideal, and we should eventually follow-up and\nmerge the \"builtin/submodule--helper.c\" code into\n\"builtin/submodule.c\". Doing it this way makes it easy to review that\nthis new C implementation isn't doing anything more clever than the\nold shellscript implementation.\n\nThis is a large win for performance, we're now more than 4x as fast as\nbefore in terms of the fixed cost of invoking any \"git submodule\"\ncommand[1]:\n\n\t$ git hyperfine -L rev HEAD~1,HEAD -s 'make CFLAGS=-O3' './git --exec-path=$PWD submodule foreach \"echo \\$name\"'\n\tBenchmark 1: ./git --exec-path=$PWD submodule foreach \"echo \\$name\"' in 'HEAD~1\n\t Time (mean ± σ): 42.2 ms ± 0.4 ms [User: 34.9 ms, System: 9.1 ms]\n\t Range (min … max): 41.3 ms … 43.2 ms 70 runs\n\n\tBenchmark 2: ./git --exec-path=$PWD submodule foreach \"echo \\$name\"' in 'HEAD\n\t Time (mean ± σ): 9.7 ms ± 0.1 ms [User: 7.6 ms, System: 2.2 ms]\n\t Range (min … max): 9.5 ms … 10.3 ms 282 runs\n\n\tSummary\n\t './git --exec-path=$PWD submodule foreach \"echo \\$name\"' in 'HEAD' ran\n\t 4.33 ± 0.07 times faster than './git --exec-path=$PWD submodule foreach \"echo \\$name\"' in 'HEAD~1'\n\nWe're taking pains here to faithfully reproduce existing\n\"git-submodule.sh\" behavior, even when that behavior is stupid. Some\nof it we'll fix in subsequent commits, but let's first faithfully\nreproduce the behavior.\n\nOne exception is the change in the behavior of the exit code\nstand-alone \"-h\" and \"--\" yield, see the altered tests. Returning 129\ninstead of 0 and 1 for \"-h\" and \"--\" respectively is a concession to\nbasic sanity.\n\nThe pattern of using \"define BUILTIN_\" macros here isn't needed for\nnow, but as we'll move code out of \"builtin/submodule--helper.c\" we'll\nwant to re-use these strings. See 8757b35d443 (commit-graph: define\ncommon usage with a macro, 2021-08-23) and 1e91d3faf6c (reflog: move\n\"usage\" variables and use macros, 2022-03-17) for prior art using this\npattern.\n\nThe \"(argc < 2 || !strcmp(argv[1], \"-h\"))\" path at the top of\ncmd_submodule__helper() could now be a \"(argc < 2)\" if not for\nt0012-help.sh (which invokes all built-ins manually with \"-h\"). Let's\nleave it for now, eventually we'll consolidate the two.\n\n1. Using the \"git hyperfine\" wrapper for \"hyperfine\":\n https://lore.kernel.org/git/211201.86r1aw9gbd.gmgdl@evledraar.gmail.com/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Makefile | 2 +-\n builtin.h | 1 +\n builtin/submodule.c | 151 +++++++++++++++++++++++++++++++++++++\n git-submodule.sh | 91 ----------------------\n git.c | 1 +\n t/t7400-submodule-basic.sh | 12 ++-\n 6 files changed, 159 insertions(+), 99 deletions(-)\n create mode 100644 builtin/submodule.c\n delete mode 100755 git-submodule.sh\n\ndiff --git a/Makefile b/Makefile\nindex 6bfb62cbe94..d8e2c02ad42 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -635,7 +635,6 @@ SCRIPT_SH += git-merge-resolve.sh\n SCRIPT_SH += git-mergetool.sh\n SCRIPT_SH += git-quiltimport.sh\n SCRIPT_SH += git-request-pull.sh\n-SCRIPT_SH += git-submodule.sh\n SCRIPT_SH += git-web--browse.sh\n \n SCRIPT_LIB += git-mergetool--lib\n@@ -1235,6 +1234,7 @@ BUILTIN_OBJS += builtin/show-ref.o\n BUILTIN_OBJS += builtin/sparse-checkout.o\n BUILTIN_OBJS += builtin/stash.o\n BUILTIN_OBJS += builtin/stripspace.o\n+BUILTIN_OBJS += builtin/submodule.o\n BUILTIN_OBJS += builtin/submodule--helper.o\n BUILTIN_OBJS += builtin/symbolic-ref.o\n BUILTIN_OBJS += builtin/tag.o\ndiff --git a/builtin.h b/builtin.h\nindex 8901a34d6bf..475c79b6a5a 100644\n--- a/builtin.h\n+++ b/builtin.h\n@@ -224,6 +224,7 @@ int cmd_sparse_checkout(int argc, const char **argv, const char *prefix);\n int cmd_status(int argc, const char **argv, const char *prefix);\n int cmd_stash(int argc, const char **argv, const char *prefix);\n int cmd_stripspace(int argc, const char **argv, const char *prefix);\n+int cmd_submodule(int argc, const char **argv, const char *prefix);\n int cmd_submodule__helper(int argc, const char **argv, const char *prefix);\n int cmd_switch(int argc, const char **argv, const char *prefix);\n int cmd_symbolic_ref(int argc, const char **argv, const char *prefix);\ndiff --git a/builtin/submodule.c b/builtin/submodule.c\nnew file mode 100644\nindex 00000000000..7e3499f3376\n--- /dev/null\n+++ b/builtin/submodule.c\n@@ -0,0 +1,151 @@\n+/*\n+ * Copyright (c) 2007-2022 Lars Hjemli & others\n+ * Copyright(c) 2022 Ævar Arnfjörð Bjarmason\n+ */\n+#include \"builtin.h\"\n+#include \"parse-options.h\"\n+#include \"run-command.h\"\n+#include \"strvec.h\"\n+\n+#define BUILTIN_SUBMODULE_USAGE \\\n+\t\"git submodule [--quiet] [--cached]\"\n+\n+#define BUILTIN_SUBMODULE_ADD_USAGE \\\n+\tN_(\"git submodule [--quiet] add [-b ] [-f | --force] [--name ]\\n\" \\\n+\t \" [--reference ] [--] []\")\n+\n+#define BUILTIN_SUBMODULE_STATUS_USAGE \\\n+\tN_(\"git submodule [--quiet] status [--cached] [--recursive] [--] [...]\")\n+\n+#define BUILTIN_SUBMODULE_INIT_USAGE \\\n+\tN_(\"git submodule [--quiet] init [--] [...]\")\n+\n+#define BUILTIN_SUBMODULE_DEINIT_USAGE \\\n+\tN_(\"git submodule [--quiet] deinit [-f | --force] (--all | [--] ...)\")\n+\n+#define BUILTIN_SUBMODULE_UPDATE_USAGE \\\n+\tN_(\"git submodule [--quiet] update [-v] [--init [--filter=]]\\n\" \\\n+\t \" [--remote] [-N | --no-fetch] [-f | --force] [--checkout |--merge | --rebase]\\n\" \\\n+\t \" [--[no-]recommend-shallow] [--reference ] [--recursive]\\n\" \\\n+\t \" [--[no-]single-branch] [--] [...]\")\n+\n+#define BUILTIN_SUBMODULE_SET_BRANCH_USAGE \\\n+\tN_(\"git submodule [--quiet] set-branch (--default | --branch ) [--] \")\n+\n+#define BUILTIN_SUBMODULE_SET_URL_USAGE \\\n+\tN_(\"git submodule [--quiet] set-url [--] \")\n+\n+#define BUILTIN_SUBMODULE_SUMMARY_USAGE \\\n+\tN_(\"git submodule [--quiet] summary [--cached | --files] [--summary-limit ]\\n\" \\\n+\t \" [commit] [--] [...]\")\n+#define BUILTIN_SUBMODULE_FOREACH_USAGE \\\n+\tN_(\"git submodule [--quiet] foreach [--recursive] \")\n+\n+#define BUILTIN_SUBMODULE_SYNC_USAGE \\\n+\tN_(\"git submodule [--quiet] sync [--recursive] [--] [...]\")\n+\n+#define BUILTIN_SUBMODULE_ABSORBGITDIRS_USAGE \\\n+\tN_(\"git submodule [--quiet] absorbgitdirs [--] [...]\")\n+\n+static const char * const git_submodule_usage[] = {\n+\tBUILTIN_SUBMODULE_USAGE,\n+\tBUILTIN_SUBMODULE_ADD_USAGE,\n+\tBUILTIN_SUBMODULE_STATUS_USAGE,\n+\tBUILTIN_SUBMODULE_INIT_USAGE,\n+\tBUILTIN_SUBMODULE_DEINIT_USAGE,\n+\tBUILTIN_SUBMODULE_UPDATE_USAGE,\n+\tBUILTIN_SUBMODULE_SET_BRANCH_USAGE,\n+\tBUILTIN_SUBMODULE_SET_URL_USAGE,\n+\tBUILTIN_SUBMODULE_SUMMARY_USAGE,\n+\tBUILTIN_SUBMODULE_FOREACH_USAGE,\n+\tBUILTIN_SUBMODULE_SYNC_USAGE,\n+\tBUILTIN_SUBMODULE_ABSORBGITDIRS_USAGE,\n+\tNULL,\n+};\n+\n+static void setup_helper_args(int argc, const char **argv, const char *prefix,\n+\t\t\t int quiet, int cached, struct strvec *args,\n+\t\t\t const struct option *options)\n+{\n+\tconst char *cmd;\n+\tint do_quiet_cache = 1;\n+\tint do_prefix = 1;\n+\n+\tstrvec_push(args, \"submodule--helper\");\n+\n+\t/* No command word defaults to \"status\" */\n+\tif (!argc) {\n+\t\tstrvec_push(args, \"status\");\n+\t\treturn;\n+\t}\n+\n+\t/* Did we get --cached with a command? */\n+\tif (cached)\n+\t\tusage_msg_optf(_(\"'%s' option is only supported with explicit 'status'\"),\n+\t\t\t git_submodule_usage, options, \"--cached\");\n+\n+\n+\t/* Either a valid command, or submodule--helper will barf! */\n+\tcmd = argv[0];\n+\tstrvec_push(args, cmd);\n+\targv++;\n+\targc--;\n+\n+\t/*\n+\t * This is stupid, but don't support \"[--]\" to\n+\t * subcommand-less \"git-submodule\" for now.\n+\t */\n+\tif (!strcmp(cmd, \"--\") || !strcmp(cmd, \"--end-of-options\"))\n+\t\tusage_msg_optf(_(\"need explicit sub-command name to delimit with '%s'\"),\n+\t\t\t git_submodule_usage, options, cmd);\n+\n+\t/* Options that need to go before user-supplied options */\n+\tif (!strcmp(cmd, \"absorbgitdirs\"))\n+\t\tdo_quiet_cache = 0;\n+\telse if (!strcmp(cmd, \"update\"))\n+\t\t;\n+\telse\n+\t\tdo_prefix = 0;\n+\tif (do_quiet_cache) {\n+\t\tif (quiet)\n+\t\t\tstrvec_push(args, \"--quiet\");\n+\t\tif (cached)\n+\t\t\tstrvec_push(args, \"--cached\");\n+\n+\t\tif (prefix && do_prefix)\n+\t\t\tstrvec_pushl(args, \"--prefix\", prefix, NULL);\n+\t}\n+\n+\t/* All commands get argv, including a \"--\", if any */\n+\tstrvec_pushv(args, argv);\n+}\n+\n+int cmd_submodule(int argc, const char **argv, const char *prefix)\n+{\n+\tint opt_quiet = 0;\n+\tint opt_cached = 0;\n+\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\tstruct option options[] = {\n+\t\tOPT__QUIET(&opt_quiet, N_(\"be quiet\")),\n+\t\tOPT_BOOL(0, \"cached\", &opt_cached,\n+\t\t\t N_(\"print the OID of submodules\")),\n+\t\tOPT_END()\n+\t};\n+\n+\targc = parse_options(argc, argv, prefix, options, git_submodule_usage,\n+\t\t\t PARSE_OPT_STOP_AT_NON_OPTION | PARSE_OPT_KEEP_DASHDASH);\n+\n+\t/*\n+\t * Tell the rest of git that any URLs we get don't come\n+\t * directly from the user, so it can apply policy as appropriate.\n+\t */\n+\tstrvec_push(&cp.env, \"GIT_PROTOCOL_FROM_USER=0\");\n+\tsetup_helper_args(argc, argv, prefix, opt_quiet, opt_cached,\n+\t\t\t &cp.args, options);\n+\n+\tcp.git_cmd = 1;\n+\tcp.no_stdin = 0; /* for git submodule foreach */\n+\tcp.dir = startup_info->original_cwd;\n+\n+\treturn run_command(&cp);\n+}\ndiff --git a/git-submodule.sh b/git-submodule.sh\ndeleted file mode 100755\nindex 4f8f62ce981..00000000000\n--- a/git-submodule.sh\n+++ /dev/null\n@@ -1,91 +0,0 @@\n-#!/bin/sh\n-#\n-# git-submodule.sh: add, init, update or list git submodules\n-#\n-# Copyright (c) 2007 Lars Hjemli\n-\n-dashless=$(basename \"$0\" | sed -e 's/-/ /')\n-USAGE=\"[--quiet] [--cached]\n- or: $dashless [--quiet] add [-b ] [-f|--force] [--name ] [--reference ] [--] []\n- or: $dashless [--quiet] status [--cached] [--recursive] [--] [...]\n- or: $dashless [--quiet] init [--] [...]\n- or: $dashless [--quiet] deinit [-f|--force] (--all| [--] ...)\n- or: $dashless [--quiet] update [--init [--filter=]] [--remote] [-N|--no-fetch] [-f|--force] [--checkout|--merge|--rebase] [--[no-]recommend-shallow] [--reference ] [--recursive] [--[no-]single-branch] [--] [...]\n- or: $dashless [--quiet] set-branch (--default|--branch ) [--] \n- or: $dashless [--quiet] set-url [--] \n- or: $dashless [--quiet] summary [--cached|--files] [--summary-limit ] [commit] [--] [...]\n- or: $dashless [--quiet] foreach [--recursive] \n- or: $dashless [--quiet] sync [--recursive] [--] [...]\n- or: $dashless [--quiet] absorbgitdirs [--] [...]\"\n-OPTIONS_SPEC=\n-SUBDIRECTORY_OK=Yes\n-. git-sh-setup\n-require_work_tree\n-wt_prefix=$(git rev-parse --show-prefix)\n-cd_to_toplevel\n-\n-# Tell the rest of git that any URLs we get don't come\n-# directly from the user, so it can apply policy as appropriate.\n-GIT_PROTOCOL_FROM_USER=0\n-export GIT_PROTOCOL_FROM_USER\n-\n-command=\n-quiet=\n-cached=\n-\n-while test $# != 0 && test -z \"$command\"\n-do\n-\tcase \"$1\" in\n-\tadd | foreach | init | deinit | update | set-branch | set-url | status | summary | sync | absorbgitdirs)\n-\t\tcommand=$1\n-\t\t;;\n-\t-q|--quiet)\n-\t\tquiet=1\n-\t\t;;\n-\t--cached)\n-\t\tif test -z \"$command\"\n-\t\tthen\n-\t\t\tcached=1 &&\n-\t\t\tshift &&\n-\t\t\tbreak\n-\t\telse\n-\t\t\tusage\n-\t\tfi\n-\t\t;;\n-\t--)\n-\t\tbreak\n-\t\t;;\n-\t-*)\n-\t\tusage\n-\t\t;;\n-\t*)\n-\t\tbreak\n-\t\t;;\n-\tesac\n-\tshift\n-done\n-\n-# No command word defaults to \"status\"\n-if test -z \"$command\"\n-then\n- if test $# = 0\n- then\n-\tcommand=status\n- else\n-\tusage\n- fi\n-fi\n-\n-case \"$command\" in\n-absorbgitdirs)\n-\tgit submodule--helper \"$command\" --prefix \"$wt_prefix\" \"$@\"\n-\t;;\n-update)\n-\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper \"$command\" \\\n-\t\t${quiet:+--quiet} ${wt_prefix:+--prefix \"$wt_prefix\"} \"$@\"\n-\t;;\n-add | foreach | init | deinit | set-branch | set-url | status | summary | sync)\n-\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper \"$command\" \\\n-\t\t${quiet:+--quiet} ${cached:+--cached} \"$@\"\n-\t;;\n-esac\ndiff --git a/git.c b/git.c\nindex da411c53822..2ad14f1e38f 100644\n--- a/git.c\n+++ b/git.c\n@@ -610,6 +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\", cmd_submodule, RUN_SETUP | NEED_WORK_TREE },\n \t{ \"submodule--helper\", cmd_submodule__helper, RUN_SETUP | SUPPORT_SUPER_PREFIX | NO_PARSEOPT },\n \t{ \"switch\", cmd_switch, RUN_SETUP | NEED_WORK_TREE },\n \t{ \"symbolic-ref\", cmd_symbolic_ref, RUN_SETUP },\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex d8f7d6ee29a..19df3407ef1 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -15,20 +15,18 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n . ./test-lib.sh\n \n test_expect_success 'submodule usage: -h' '\n-\tgit submodule -h >out 2>err &&\n+\ttest_expect_code 129 git submodule -h >out 2>err &&\n \tgrep \"^usage: git submodule\" out &&\n \ttest_must_be_empty err\n '\n \n test_expect_success 'submodule usage: --recursive' '\n-\ttest_expect_code 1 git submodule --recursive >out 2>err &&\n-\tgrep \"^usage: git submodule\" err &&\n-\ttest_must_be_empty out\n+\ttest_expect_code 129 git submodule --recursive\n '\n \n test_expect_success 'submodule usage: status --' '\n-\ttest_expect_code 1 git submodule -- &&\n-\ttest_expect_code 1 git submodule --end-of-options\n+\ttest_expect_code 129 git submodule -- &&\n+\ttest_expect_code 129 git submodule --end-of-options\n '\n \n for opt in '--quiet'\n@@ -45,7 +43,7 @@ do\n \ttest_expect_success \"submodule usage: status $opt\" '\n \t\tgit submodule $opt &&\n \t\tgit submodule status $opt &&\n-\t\ttest_expect_code 1 git submodule $opt status >out 2>err &&\n+\t\ttest_expect_code 129 git submodule $opt status >out 2>err &&\n \t\tgrep \"^usage: git submodule\" err &&\n \t\ttest_must_be_empty out\n \t'\n-- \n2.38.0.1091.gf9d18265e59\n\n"},{"id":"465084","messageId":"patch-10.10-81f138e460c-20221017T115544Z-avarab@gmail.com","threadId":"58637","inReplyTo":"cover-00.10-00000000000-20221017T115544Z-avarab@gmail.com","subject":"[PATCH 10/10] submodule: don't use a subprocess to invoke \"submodule--helper\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-17T12:09:25Z","receivedAt":"2022-10-17T12:10:20Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"In a preceding commit we created \"builtin/submodule.c\" and faithfully\ntried to reproduce every aspect of \"git-submodule.sh\", including its\ninvocation of \"git submodule--helper\" as a sub-process.\n\nLet's do away with the sub-process and invoke\n\"cmd_submodule__helper()\" directly. Eventually we'll want to do away\nwith \"builtin/submodule--helper.c\" altogether, but let's not do that\nfor now to avoid conflicts with other in-flight topics. Even without\nthose conflicts the resulting diff would be large. We can leave that\nfor a later cleanup.\n\nThis speeds up invocations of all \"git submodule\" commands, E.g. a\ntrivial \"foreach\" command on git.git is around 1.50 times\nfaster[1]. For more expensive commands this'll make less of a\ndifference, as the fixed cost of invoking the sub-process will be\namortized away.\n\n\t$ git hyperfine -L rev HEAD~1,HEAD -s 'make CFLAGS=-O3' './git submodule foreach \"echo \\$name\"'\n\tBenchmark 1: ./git submodule foreach \"echo \\$name\"' in 'HEAD~1\n\t Time (mean ± σ): 9.7 ms ± 0.1 ms [User: 7.6 ms, System: 2.1 ms]\n\t Range (min … max): 9.4 ms … 10.2 ms 285 runs\n\n\tBenchmark 2: ./git submodule foreach \"echo \\$name\"' in 'HEAD\n\t Time (mean ± σ): 6.6 ms ± 0.1 ms [User: 5.1 ms, System: 1.5 ms]\n\t Range (min … max): 6.2 ms … 7.2 ms 414 runs\n\n\tSummary\n\t './git submodule foreach \"echo \\$name\"' in 'HEAD' ran\n\t 1.48 ± 0.04 times faster than './git submodule foreach \"echo \\$name\"' in 'HEAD~1'\n\nIt's also worth noting that some users were using e.g. \"git\nsubmodule--helper list\" directly for performance reasons[2]. With\n31955475d1c (submodule--helper: remove unused \"list\" helper,\n2022-09-01) released with v2.38.0 the \"list\" command was no longer\nprovided. Users who had to switch to \"git submodule--helper foreach\"\nwere given a command that (on my system) is around 6.5x slower.\n\nNow the \"foreach\" is around 0.10x slower (due to the slight shell\noverhead), with 31955475d1c reverted on top of this:\n\n\t$ hyperfine './git submodule--helper list' './git submodule foreach --quiet \"echo \\$name\"' --warmup 10\n\tBenchmark 1: ./git submodule--helper list\n\t Time (mean ± σ): 6.4 ms ± 0.1 ms [User: 5.0 ms, System: 1.5 ms]\n\t Range (min … max): 6.2 ms … 7.2 ms 427 runs\n\n\tBenchmark 2: ./git submodule foreach --quiet \"echo \\$name\"\n\t Time (mean ± σ): 7.0 ms ± 0.1 ms [User: 4.8 ms, System: 2.3 ms]\n\t Range (min … max): 6.8 ms … 7.4 ms 390 runs\n\n\tSummary\n\t './git submodule--helper list' ran\n\t 1.10 ± 0.03 times faster than './git submodule foreach --quiet \"echo \\$name\"'\n\nI think it would make sense to implement a \"--format\" option for \"git\nsubmodule foreach\" to help anyone who cares about that remaining\nperformance (and to improve the API, e.g. by supporting \"-z\"), but as\nfar as performance goes this makes the runtime acceptable again.\n\nThe pattern in \"cmd_submodule_builtin()\" of saving \"struct strvec\"\narguments to a \"struct string_list\" and free()-ing them after the\n\"argv\" has been modified by \"cmd_submodule__helper()\" is new, without\nit we'd get various already-passing tests failing under SANITIZE=leak.\n\n1. Using the \"git hyperfine\" wrapper for \"hyperfine\":\n https://lore.kernel.org/git/211201.86r1aw9gbd.gmgdl@evledraar.gmail.com/\n2. https://lore.kernel.org/git/87czatrpyb.fsf@bernoul.li/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/submodule.c | 41 +++++++++++++++++++++++++++++++++--------\n 1 file changed, 33 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/submodule.c b/builtin/submodule.c\nindex ca8e273b6e9..13e7064b03f 100644\n--- a/builtin/submodule.c\n+++ b/builtin/submodule.c\n@@ -119,12 +119,40 @@ static void setup_helper_args(int argc, const char **argv, const char *prefix,\n \tstrvec_pushv(args, argv);\n }\n \n+static int cmd_submodule_builtin(struct strvec *args, const char *prefix)\n+{\n+\tsize_t i;\n+\tstruct string_list to_free = STRING_LIST_INIT_DUP;\n+\tint ret;\n+\n+\t/*\n+\t * The cmd_submodule__helper() will treat the argv as\n+\t * its own and modify it, so e.g. for \"git submodule\n+\t * add\" the \"add\" argument will be removed, and we'll\n+\t * thus leak from the strvec_push()'s in\n+\t * setup_helper_args().\n+\t *\n+\t * So in lieu of some generic \"snapshot for a free\"\n+\t * API for \"struct strvec\" squirrel away the pointers\n+\t * to free with string_list_clear() later.\n+\t */\n+\tfor (i = 0; i < args->nr; i++)\n+\t\tstring_list_append_nodup(&to_free, (char *)args->v[i]);\n+\n+\tret = cmd_submodule__helper(args->nr, args->v, prefix);\n+\n+\tstring_list_clear(&to_free, 0);\n+\tfree(strvec_detach(args));\n+\n+\treturn ret;\n+}\n+\n int cmd_submodule(int argc, const char **argv, const char *prefix)\n {\n \tint opt_quiet = 0;\n \tint opt_cached = 0;\n \tint opt_recursive = 0;\n-\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\tstruct strvec args = STRVEC_INIT;\n \tstruct option options[] = {\n \t\tOPT__QUIET(&opt_quiet, N_(\"be quiet\")),\n \t\tOPT_BOOL(0, \"cached\", &opt_cached,\n@@ -141,13 +169,10 @@ int cmd_submodule(int argc, const char **argv, const char *prefix)\n \t * Tell the rest of git that any URLs we get don't come\n \t * directly from the user, so it can apply policy as appropriate.\n \t */\n-\tstrvec_push(&cp.env, \"GIT_PROTOCOL_FROM_USER=0\");\n-\tsetup_helper_args(argc, argv, prefix, opt_quiet, opt_cached,\n-\t\t\t opt_recursive, &cp.args, options);\n+\txsetenv(\"GIT_PROTOCOL_FROM_USER\", \"0\", 1);\n \n-\tcp.git_cmd = 1;\n-\tcp.no_stdin = 0; /* for git submodule foreach */\n-\tcp.dir = startup_info->original_cwd;\n+\tsetup_helper_args(argc, argv, prefix, opt_quiet, opt_cached,\n+\t\t\t opt_recursive, &args, options);\n \n-\treturn run_command(&cp);\n+\treturn cmd_submodule_builtin(&args, prefix);\n }\n-- \n2.38.0.1091.gf9d18265e59\n\n"},{"id":"465343","messageId":"kl6l1qr2z0ok.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"58637","inReplyTo":"patch-02.10-46bf600820b-20221017T115544Z-avarab@gmail.com","subject":"Re: [PATCH 02/10] git-submodule.sh: dispatch \"sync\" to helper","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-10-20T20:42:51Z","receivedAt":"2022-10-20T20:43: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> Remove the cmd_sync() wrapper for \"git submodule--helper sync\" in\n> favor of dispatching the raw command-line directly to the helper.\n>\n> At this point we've already parsed out the optional \"--quiet\" flag\n> that we need to support for \"git submodule --quiet\" (as opposed to\n> \"git submodule --quiet\").\n\nFor a moment, I thought this was saying that we're dropping support for\n\"git submodule --quiet\" in favor of \"git submodule\n--quiet\", but that's not true. Both are still supported, albeit in\nslightly different ways:\n\n- The subcommand \"--quiet\" flag is passed via $@\n\n- The top level \"--quiet\" flag is parsed when we parse top level flags,\n which sets quiet=1, and thus passes \"--quiet\" to submodule--helper.\n We also `shift` out the \"--quiet\" so there's no fear of passing\n \"--quiet\" twice.\n\nHooray!\n\n"},{"id":"465349","messageId":"kl6ly1taxkn4.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"58637","inReplyTo":"patch-04.10-db6a09ee42a-20221017T115544Z-avarab@gmail.com","subject":"Re: [PATCH 04/10] git-submodule.sh: dispatch \"foreach\" to helper","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-10-20T21:14:39Z","receivedAt":"2022-10-20T21:14:47Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Dispatch the \"git submodule foreach\" command directly to \"git\n> submodule--helper foreach\". This case requires the addition of the\n> PARSE_OPT_STOP_AT_NON_OPTION flag, since the shellscript was\n> unconditionally adding \"--\" to the \"git submodule--helper\"\n> command-line.\n\nThe commands in the previous patch also unconditionally add \"--\", so\nthis wasn't clear to me on first read.\n\nIt might be clearer to include some extra context, which is that \"git\nsubmodule foreach\" is different because everything after \"foreach\"\nshould be treated like a single comand to run even if they are passed as\nseparate strings, e.g. from t/t7407-submodule-foreach.sh.21, these two\nshould be equivalent:\n\n git submodule foreach \"echo be --quiet\"\n\nvs\n\n git submodule foreach echo be --quiet\n\nSo PARSE_OPT_STOP_AT_NON_OPTION is necessary in order to stop us from\nintepreting option-like args as args to \"git submodule foreach\" instead\nof as part of the command to run. Without PARSE_OPT_STOP_AT_NON_OPTION,\n\n git submodule foreach echo be --quiet\n\nbecomes more like\n\n git submodule foreach --quiet \"echo be\"\n\nThere could have been a subtle change in behavior, since \"git submodule\nforeach -- --foo\" will be parsed as if \"--foo\" is the command, but\nPARSE_OPT_STOP_AT_NON_OPTION will happily accept \"--foo\" as an option.\nBut we could never get into this situation anyway since we used to emit\nusage on the first option-like arg...\n\n> -# Execute an arbitrary command sequence in each checked out\n> -# submodule\n> -#\n> -# $@ = command to execute\n> -#\n> -cmd_foreach()\n> -{\n> -\t# parse $args after \"submodule ... foreach\".\n> -\twhile test $# -ne 0\n> -\tdo\n> -\t\tcase \"$1\" in\n> -\t\t-q|--quiet)\n> -\t\t\tquiet=1\n> -\t\t\t;;\n> -\t\t--recursive)\n> -\t\t\trecursive=1\n> -\t\t\t;;\n> -\t\t-*)\n> -\t\t\tusage\n> -\t\t\t;;\n\ni.e. here. \n\nSo this patch looks safe.\n\n"},{"id":"465352","messageId":"kl6lv8oexiyy.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"58637","inReplyTo":"patch-05.10-7d9c13eb637-20221017T115544Z-avarab@gmail.com","subject":"Re: [PATCH 05/10] git-submodule.sh: dispatch \"update\" to helper","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-10-20T21:50:45Z","receivedAt":"2022-10-20T21:50:52Z","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 noted in a preceding commit the only behavior change here should be\n> the desirable change of better \"-h\" output, and that this\n> implementation understands the \"--verbose\" synonym for \"-v\". Let's\n> update the documentation to reflect the new \"--verbose\" synonym.\n\nHm, I didn't see this change in the patch.\n\n> -\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper update \\\n> -\t\t${quiet:+--quiet} \\\n> -\t\t${force:+--force} \\\n> -\t\t${progress:+\"--progress\"} \\\n> -\t\t${remote:+--remote} \\\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> -\t\t${reference:+\"$reference\"} \\\n> -\t\t${dissociate:+\"--dissociate\"} \\\n> -\t\t${depth:+\"$depth\"} \\\n> -\t\t${require_init:+--require-init} \\\n> -\t\t${dissociate:+\"--dissociate\"} \\\n> -\t\t$single_branch \\\n> -\t\t$recommend_shallow \\\n> -\t\t$jobs \\\n> -\t\t$filter \\\n> -\t\t-- \\\n> -\t\t\"$@\"\n> -}\n\n[...]\n\n> -\n> -# This loop parses the command line arguments to find the\n> -# subcommand name to dispatch. Parsing of the subcommand specific\n> -# options are primarily done by the subcommand implementations.\n> -# Subcommand specific options such as --branch and --cached are\n> -# parsed here as well, for backward compatibility.\n\nThis comment still seems relevant as of this patch.\n\n> while test $# != 0 && test -z \"$command\"\n> do\n> @@ -233,7 +80,8 @@ absorbgitdirs)\n> \tgit submodule--helper \"$command\" --prefix \"$wt_prefix\" \"$@\"\n> \t;;\n> update)\n> -\tcmd_update \"$@\"\n> +\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper \"$command\" \\\n> +\t\t${quiet:+--quiet} ${wt_prefix:+--prefix \"$wt_prefix\"} \"$@\"\n> \t;;\n> add | foreach | init | deinit | set-branch | set-url | status | summary | sync)\n> \tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper \"$command\" \\\n\nI haven't read ahead to see whether this was fixed, but it looks like\nthe reason we couldn't combine \"update\" into this arms is that \"update\"\nconsumes \"wt_prefix\" twice. \n\nThere's no good reason to have \"--prefix\" at all actually. \"git\nsubmodule update\" used to use that instead of -C, and we could have\nremoved it once we passed -C in 29a5e9e1ff (submodule--helper\nupdate-clone: learn --init, 2022-03-04). That simplification got lost in\nthe big shell -> C conversion, but we could do it quite easily right\nnow, e.g.\n\n diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n index d11e100301..a4f59e91c5 100644\n --- a/builtin/submodule--helper.c\n +++ b/builtin/submodule--helper.c\n @@ -2636,9 +2636,6 @@ static int module_update(int argc, const char **argv, const char *prefix)\n N_(\"traverse submodules recursively\")),\n OPT_BOOL('N', \"no-fetch\", &opt.nofetch,\n 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 OPT_SET_INT(0, \"checkout\", &opt.update_default,\n N_(\"use the 'checkout' update strategy (default)\"),\n SM_UPDATE_CHECKOUT),\n @@ -2694,6 +2691,7 @@ static int module_update(int argc, const char **argv, const char *prefix)\n }\n\n opt.filter_options = &filter_options;\n +\topt.prefix = prefix;\n\n if (opt.update_default)\n opt.update_strategy.type = opt.update_default;\n diff --git a/git-submodule.sh b/git-submodule.sh\n index ac2f95c128..2787aaa60c 100755\n --- a/git-submodule.sh\n +++ b/git-submodule.sh\n @@ -79,11 +79,8 @@ case \"$command\" in\n absorbgitdirs)\n git submodule--helper \"$command\" --prefix \"$wt_prefix\" \"$@\"\n ;;\n -update)\n -\tgit ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper \"$command\" \\\n -\t\t${quiet:+--quiet} ${wt_prefix:+--prefix \"$wt_prefix\"} \"$@\"\n -\t;;\n -add | foreach | init | deinit | set-branch | set-url | status | summary | sync)\n +\n +add | foreach | init | deinit | set-branch | set-url | status | summary | sync | update)\n git ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper \"$command\" \\\n ${quiet:+--quiet} ${cached:+--cached} \"$@\"\n ;;\n\n> -- \n> 2.38.0.1091.gf9d18265e59\n"},{"id":"465355","messageId":"kl6lsfjixhuz.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"58637","inReplyTo":"patch-06.10-25fadf3ffc1-20221017T115544Z-avarab@gmail.com","subject":"Re: [PATCH 06/10] git-submodule.sh: don't support top-level \"--cached\"","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-10-20T22:14:44Z","receivedAt":"2022-10-20T22:14:50Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Since the preceding commit all sub-commands of \"git submodule\" have\n> been dispatched to \"git submodule--helper\" directly, we therefore\n> don't need to emit \"usage()\" if we see \"--cached\" without the\n> sub-command being \"status\" or \"summary\", we can trust that\n> parse_options() will spot that and barf on it.\n>\n> This does change one obscure aspect of undocumented behavior, for\n> \"status\" and \"summary\" we supported these undocumented forms:\n>\n> git submodule --cached (status | summary)\n>\n> As noted in a preceding commit to git-submodule.sh which removed the\n> \"--branch\" special-case, this comes down to emergent behavior seen in\n> 5c08dbbdf1a (git-submodule: fix subcommand parser,\n> 2008-01-15). I.e. we wanted to support was for subcommand-less invocations like:\n>\n> git submodule --cached\n>\n> To be synonymous with invocations that explicitly named the \"status\"\n> sub-command:\n>\n> git submodule status --cached\n>\n> But we did not intend to mix the two, and allow \"--cached\" to be an\n> option to the top-level \"submodule\" command when the \"status\" or\n> \"summary\" sub-commands were explicitly provided.\n>\n> Let's remove this undocumented edge case, which makes a subsequent\n> removal of git-submodule.sh easier to reason about. The test case\n> added here is duplicated from the existing for-loop, except for the\n> different and desired handling of \"git submodule --cached status\".\n\nMakes sense, I completely agree.\n\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index ac2f95c1285..4f8f62ce981 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -43,7 +43,14 @@ do\n> \t\tquiet=1\n> \t\t;;\n> \t--cached)\n> -\t\tcached=1\n> +\t\tif test -z \"$command\"\n> +\t\tthen\n> +\t\t\tcached=1 &&\n> +\t\t\tshift &&\n> +\t\t\tbreak\n> +\t\telse\n> +\t\t\tusage\n> +\t\tfi\n> \t\t;;\n> \t--)\n> \t\tbreak\n\nDo we need the 'if test -z \"$command\"'? This is in a 'while test $# != 0\n&& test -z \"$command\"' loop, so it seems unnecessary. I've tried\nremoving it and it seems to work just fine.\n\n> @@ -69,12 +76,6 @@ then\n> fi\n> fi\n> \n> -# \"--cached\" is accepted only by \"status\" and \"summary\"\n> -if test -n \"$cached\" && test \"$command\" != status && test \"$command\" != summary\n> -then\n> -\tusage\n> -fi\n> -\n> case \"$command\" in\n> absorbgitdirs)\n> \tgit submodule--helper \"$command\" --prefix \"$wt_prefix\" \"$@\"\n> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\n> index b50db3f1031..d8f7d6ee29a 100755\n> --- a/t/t7400-submodule-basic.sh\n> +++ b/t/t7400-submodule-basic.sh\n> @@ -31,7 +31,7 @@ test_expect_success 'submodule usage: status --' '\n> \ttest_expect_code 1 git submodule --end-of-options\n> '\n> \n> -for opt in '--quiet' '--cached'\n> +for opt in '--quiet'\n> do\n> \ttest_expect_success \"submodule usage: status $opt\" '\n> \t\tgit submodule $opt &&\n> @@ -40,6 +40,17 @@ do\n> \t'\n> done\n> \n> +for opt in '--cached'\n> +do\n> +\ttest_expect_success \"submodule usage: status $opt\" '\n> +\t\tgit submodule $opt &&\n> +\t\tgit submodule status $opt &&\n> +\t\ttest_expect_code 1 git submodule $opt status >out 2>err &&\n> +\t\tgrep \"^usage: git submodule\" err &&\n> +\t\ttest_must_be_empty out\n> +\t'\n> +done\n> +\n\nNow that there's only a single opt in each of these tests, do we still\nwant to keep the for loop?\n\n> test_expect_success 'submodule deinit works on empty repository' '\n> \tgit submodule deinit --all\n> '\n> @@ -576,7 +587,7 @@ test_expect_success 'status should be \"modified\" after submodule commit' '\n> '\n> \n> test_expect_success 'the --cached sha1 should be rev1' '\n> -\tgit submodule --cached status >list &&\n> +\tgit submodule status --cached >list &&\n> \tgrep \"^+$rev1\" list\n> '\n> \n> -- \n> 2.38.0.1091.gf9d18265e59\n"},{"id":"465367","messageId":"kl6lpmemxg8p.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"58637","inReplyTo":"patch-07.10-2c77ed38d90-20221017T115544Z-avarab@gmail.com","subject":"Re: [PATCH 07/10] submodule: make it a built-in, remove git-submodule.sh","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-10-20T22:49:42Z","receivedAt":"2022-10-20T22:49:48Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Replace the \"git-submodule.sh\" script with a built-in\n> \"builtin/submodule.c. For\" now this new command is only a dumb\n> dispatcher that uses run-command.c to invoke \"git submodule--helper\",\n> just as \"git-submodule.sh\" used to do.\n>\n> This is obviously not ideal, and we should eventually follow-up and\n> merge the \"builtin/submodule--helper.c\" code into\n> \"builtin/submodule.c\". Doing it this way makes it easy to review that\n> this new C implementation isn't doing anything more clever than the\n> old shellscript implementation.\n>\n> This is a large win for performance, we're now more than 4x as fast as\n> before in terms of the fixed cost of invoking any \"git submodule\"\n> command[1]:\n>\n> \t$ git hyperfine -L rev HEAD~1,HEAD -s 'make CFLAGS=-O3' './git --exec-path=$PWD submodule foreach \"echo \\$name\"'\n> \tBenchmark 1: ./git --exec-path=$PWD submodule foreach \"echo \\$name\"' in 'HEAD~1\n> \t Time (mean ± σ): 42.2 ms ± 0.4 ms [User: 34.9 ms, System: 9.1 ms]\n> \t Range (min … max): 41.3 ms … 43.2 ms 70 runs\n>\n> \tBenchmark 2: ./git --exec-path=$PWD submodule foreach \"echo \\$name\"' in 'HEAD\n> \t Time (mean ± σ): 9.7 ms ± 0.1 ms [User: 7.6 ms, System: 2.2 ms]\n> \t Range (min … max): 9.5 ms … 10.3 ms 282 runs\n>\n> \tSummary\n> \t './git --exec-path=$PWD submodule foreach \"echo \\$name\"' in 'HEAD' ran\n> \t 4.33 ± 0.07 times faster than './git --exec-path=$PWD submodule foreach \"echo \\$name\"' in 'HEAD~1'\n>\n> We're taking pains here to faithfully reproduce existing\n> \"git-submodule.sh\" behavior, even when that behavior is stupid. Some\n> of it we'll fix in subsequent commits, but let's first faithfully\n> reproduce the behavior.\n>\n> One exception is the change in the behavior of the exit code\n> stand-alone \"-h\" and \"--\" yield, see the altered tests. Returning 129\n> instead of 0 and 1 for \"-h\" and \"--\" respectively is a concession to\n> basic sanity.\n\nSounds reasonable.\n\n> The pattern of using \"define BUILTIN_\" macros here isn't needed for\n> now, but as we'll move code out of \"builtin/submodule--helper.c\" we'll\n> want to re-use these strings. See 8757b35d443 (commit-graph: define\n> common usage with a macro, 2021-08-23) and 1e91d3faf6c (reflog: move\n> \"usage\" variables and use macros, 2022-03-17) for prior art using this\n> pattern.\n>\n> The \"(argc < 2 || !strcmp(argv[1], \"-h\"))\" path at the top of\n> cmd_submodule__helper() could now be a \"(argc < 2)\" if not for\n> t0012-help.sh (which invokes all built-ins manually with \"-h\"). Let's\n> leave it for now, eventually we'll consolidate the two.\n>\n> 1. Using the \"git hyperfine\" wrapper for \"hyperfine\":\n> https://lore.kernel.org/git/211201.86r1aw9gbd.gmgdl@evledraar.gmail.com/\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> Makefile | 2 +-\n> builtin.h | 1 +\n> builtin/submodule.c | 151 +++++++++++++++++++++++++++++++++++++\n> git-submodule.sh | 91 ----------------------\n> git.c | 1 +\n> t/t7400-submodule-basic.sh | 12 ++-\n> 6 files changed, 159 insertions(+), 99 deletions(-)\n> create mode 100644 builtin/submodule.c\n> delete mode 100755 git-submodule.sh\n>\n> diff --git a/Makefile b/Makefile\n> index 6bfb62cbe94..d8e2c02ad42 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -635,7 +635,6 @@ SCRIPT_SH += git-merge-resolve.sh\n> SCRIPT_SH += git-mergetool.sh\n> SCRIPT_SH += git-quiltimport.sh\n> SCRIPT_SH += git-request-pull.sh\n> -SCRIPT_SH += git-submodule.sh\n> SCRIPT_SH += git-web--browse.sh\n> \n> SCRIPT_LIB += git-mergetool--lib\n> @@ -1235,6 +1234,7 @@ BUILTIN_OBJS += builtin/show-ref.o\n> BUILTIN_OBJS += builtin/sparse-checkout.o\n> BUILTIN_OBJS += builtin/stash.o\n> BUILTIN_OBJS += builtin/stripspace.o\n> +BUILTIN_OBJS += builtin/submodule.o\n> BUILTIN_OBJS += builtin/submodule--helper.o\n> BUILTIN_OBJS += builtin/symbolic-ref.o\n> BUILTIN_OBJS += builtin/tag.o\n> diff --git a/builtin.h b/builtin.h\n> index 8901a34d6bf..475c79b6a5a 100644\n> --- a/builtin.h\n> +++ b/builtin.h\n> @@ -224,6 +224,7 @@ int cmd_sparse_checkout(int argc, const char **argv, const char *prefix);\n> int cmd_status(int argc, const char **argv, const char *prefix);\n> int cmd_stash(int argc, const char **argv, const char *prefix);\n> int cmd_stripspace(int argc, const char **argv, const char *prefix);\n> +int cmd_submodule(int argc, const char **argv, const char *prefix);\n> int cmd_submodule__helper(int argc, const char **argv, const char *prefix);\n> int cmd_switch(int argc, const char **argv, const char *prefix);\n> int cmd_symbolic_ref(int argc, const char **argv, const char *prefix);\n> diff --git a/builtin/submodule.c b/builtin/submodule.c\n> new file mode 100644\n> index 00000000000..7e3499f3376\n> --- /dev/null\n> +++ b/builtin/submodule.c\n> @@ -0,0 +1,151 @@\n> +/*\n> + * Copyright (c) 2007-2022 Lars Hjemli & others\n> + * Copyright(c) 2022 Ævar Arnfjörð Bjarmason\n> + */\n> +#include \"builtin.h\"\n> +#include \"parse-options.h\"\n> +#include \"run-command.h\"\n> +#include \"strvec.h\"\n> +\n> +#define BUILTIN_SUBMODULE_USAGE \\\n> +\t\"git submodule [--quiet] [--cached]\"\n> +\n> +#define BUILTIN_SUBMODULE_ADD_USAGE \\\n> +\tN_(\"git submodule [--quiet] add [-b ] [-f | --force] [--name ]\\n\" \\\n> +\t \" [--reference ] [--] []\")\n> +\n> +#define BUILTIN_SUBMODULE_STATUS_USAGE \\\n> +\tN_(\"git submodule [--quiet] status [--cached] [--recursive] [--] [...]\")\n> +\n> +#define BUILTIN_SUBMODULE_INIT_USAGE \\\n> +\tN_(\"git submodule [--quiet] init [--] [...]\")\n> +\n> +#define BUILTIN_SUBMODULE_DEINIT_USAGE \\\n> +\tN_(\"git submodule [--quiet] deinit [-f | --force] (--all | [--] ...)\")\n> +\n> +#define BUILTIN_SUBMODULE_UPDATE_USAGE \\\n> +\tN_(\"git submodule [--quiet] update [-v] [--init [--filter=]]\\n\" \\\n> +\t \" [--remote] [-N | --no-fetch] [-f | --force] [--checkout |--merge | --rebase]\\n\" \\\n> +\t \" [--[no-]recommend-shallow] [--reference ] [--recursive]\\n\" \\\n> +\t \" [--[no-]single-branch] [--] [...]\")\n> +\n> +#define BUILTIN_SUBMODULE_SET_BRANCH_USAGE \\\n> +\tN_(\"git submodule [--quiet] set-branch (--default | --branch ) [--] \")\n> +\n> +#define BUILTIN_SUBMODULE_SET_URL_USAGE \\\n> +\tN_(\"git submodule [--quiet] set-url [--] \")\n> +\n> +#define BUILTIN_SUBMODULE_SUMMARY_USAGE \\\n> +\tN_(\"git submodule [--quiet] summary [--cached | --files] [--summary-limit ]\\n\" \\\n> +\t \" [commit] [--] [...]\")\n> +#define BUILTIN_SUBMODULE_FOREACH_USAGE \\\n> +\tN_(\"git submodule [--quiet] foreach [--recursive] \")\n> +\n> +#define BUILTIN_SUBMODULE_SYNC_USAGE \\\n> +\tN_(\"git submodule [--quiet] sync [--recursive] [--] [...]\")\n> +\n> +#define BUILTIN_SUBMODULE_ABSORBGITDIRS_USAGE \\\n> +\tN_(\"git submodule [--quiet] absorbgitdirs [--] [...]\")\n> +\n\nI was surprised to see that these strings aren't deduped from the ones\nin builtin/submodule--helper.c, and are, in fact, different. Is there a\nreason for that?\n\n> +static const char * const git_submodule_usage[] = {\n> +\tBUILTIN_SUBMODULE_USAGE,\n> +\tBUILTIN_SUBMODULE_ADD_USAGE,\n> +\tBUILTIN_SUBMODULE_STATUS_USAGE,\n> +\tBUILTIN_SUBMODULE_INIT_USAGE,\n> +\tBUILTIN_SUBMODULE_DEINIT_USAGE,\n> +\tBUILTIN_SUBMODULE_UPDATE_USAGE,\n> +\tBUILTIN_SUBMODULE_SET_BRANCH_USAGE,\n> +\tBUILTIN_SUBMODULE_SET_URL_USAGE,\n> +\tBUILTIN_SUBMODULE_SUMMARY_USAGE,\n> +\tBUILTIN_SUBMODULE_FOREACH_USAGE,\n> +\tBUILTIN_SUBMODULE_SYNC_USAGE,\n> +\tBUILTIN_SUBMODULE_ABSORBGITDIRS_USAGE,\n> +\tNULL,\n> +};\n> +\n> +static void setup_helper_args(int argc, const char **argv, const char *prefix,\n> +\t\t\t int quiet, int cached, struct strvec *args,\n> +\t\t\t const struct option *options)\n> +{\n> +\tconst char *cmd;\n> +\tint do_quiet_cache = 1;\n> +\tint do_prefix = 1;\n> +\n> +\tstrvec_push(args, \"submodule--helper\");\n> +\n> +\t/* No command word defaults to \"status\" */\n> +\tif (!argc) {\n> +\t\tstrvec_push(args, \"status\");\n> +\t\treturn;\n> +\t}\n\nWe can't return without processing \"--cached\", e.g. removing the\nexplicit \"status\" subcommand like so fails.\n\n diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\n index d8f7d6ee29..5e61cef18e 100755\n --- a/t/t7400-submodule-basic.sh\n +++ b/t/t7400-submodule-basic.sh\n @@ -587,7 +587,7 @@ test_expect_success 'status should be \"modified\" after submodule commit' '\n '\n\n test_expect_success 'the --cached sha1 should be rev1' '\n -\tgit submodule status --cached >list &&\n +\tgit submodule --cached >list &&\n grep \"^+$rev1\" list\n '\n\n> +\n> +\t/* Did we get --cached with a command? */\n> +\tif (cached)\n> +\t\tusage_msg_optf(_(\"'%s' option is only supported with explicit 'status'\"),\n> +\t\t\t git_submodule_usage, options, \"--cached\");\n> +\n> +\n> +\t/* Either a valid command, or submodule--helper will barf! */\n> +\tcmd = argv[0];\n\nsubmodule--helper does die, but the help message that it emits is\nsubmodule--helper specific, instead of the \"git submodule\" usage string\nfrom before.\n\n> +\tstrvec_push(args, cmd);\n> +\targv++;\n> +\targc--;\n> +\n> +\t/*\n> +\t * This is stupid, but don't support \"[--]\" to\n> +\t * subcommand-less \"git-submodule\" for now.\n> +\t */\n> +\tif (!strcmp(cmd, \"--\") || !strcmp(cmd, \"--end-of-options\"))\n> +\t\tusage_msg_optf(_(\"need explicit sub-command name to delimit with '%s'\"),\n> +\t\t\t git_submodule_usage, options, cmd);\n\nIf this is the only \"stupid\" behavior, maybe just call this out\nspecifically in the commit message.\n\n> +\n> +\t/* Options that need to go before user-supplied options */\n> +\tif (!strcmp(cmd, \"absorbgitdirs\"))\n> +\t\tdo_quiet_cache = 0;\n> +\telse if (!strcmp(cmd, \"update\"))\n> +\t\t;\n> +\telse\n> +\t\tdo_prefix = 0;\n> +\tif (do_quiet_cache) {\n> +\t\tif (quiet)\n> +\t\t\tstrvec_push(args, \"--quiet\");\n> +\t\tif (cached)\n> +\t\t\tstrvec_push(args, \"--cached\");\n> +\n> +\t\tif (prefix && do_prefix)\n> +\t\t\tstrvec_pushl(args, \"--prefix\", prefix, NULL);\n> +\t}\n\nThis looks like a good reason to get rid of \"--prefix\" from \"git\nsubmodule--helper update\" like I mentioned upthread.\n\nI didn't notice earlier that absorbgitdirs had the same problem, but\nthat's an even easier fixup:\n\n diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n index d11e100301..1f7f142270 100644\n --- a/builtin/submodule--helper.c\n +++ b/builtin/submodule--helper.c\n @@ -2825,9 +2825,6 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)\n struct module_list list = MODULE_LIST_INIT;\n unsigned flags = ABSORB_GITDIR_RECURSE_SUBMODULES;\n struct 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 OPT_BIT(0, \"--recursive\", &flags, N_(\"recurse into submodules\"),\n ABSORB_GITDIR_RECURSE_SUBMODULES),\n OPT_END()\n"},{"id":"465371","messageId":"kl6lmt9qxfj5.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"58637","inReplyTo":"patch-09.10-7f232c5e503-20221017T115544Z-avarab@gmail.com","subject":"Re: [PATCH 09/10] submodule: support sub-command-less \"--recursive\" option","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-10-20T23:05:02Z","receivedAt":"2022-10-20T23:05: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> The inability to specify \"--recursive\" when we're not providing a\n> sub-command name appears to have been an omission in 15fc56a8536 (git\n> submodule foreach: Add --recursive to recurse into nested submodules,\n> 2009-08-19). Let's support it along with the other \"status\" options.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> Documentation/git-submodule.txt | 2 +-\n> builtin/submodule.c | 16 +++++++++++++---\n> t/t7400-submodule-basic.sh | 6 +-----\n> 3 files changed, 15 insertions(+), 9 deletions(-)\n>\n> diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt\n> index 345ebcafb9c..0c918390f2f 100644\n> --- a/Documentation/git-submodule.txt\n> +++ b/Documentation/git-submodule.txt\n> @@ -9,7 +9,7 @@ git-submodule - Initialize, update or inspect submodules\n> SYNOPSIS\n> --------\n> [verse]\n> -'git submodule' [--quiet] [--cached] [--]\n> +'git submodule' [--quiet] [--cached] [--recursive] [--]\n> 'git submodule' [--quiet] add [] [--] []\n> 'git submodule' [--quiet] status [--cached] [--recursive] [--] [...]\n> 'git submodule' [--quiet] init [--] [...]\n> diff --git a/builtin/submodule.c b/builtin/submodule.c\n> index 1d77f2d0964..ca8e273b6e9 100644\n> --- a/builtin/submodule.c\n> +++ b/builtin/submodule.c\n> @@ -64,7 +64,8 @@ static const char * const git_submodule_usage[] = {\n> };\n> \n> static void setup_helper_args(int argc, const char **argv, const char *prefix,\n> -\t\t\t int quiet, int cached, struct strvec *args,\n> +\t\t\t int quiet, int cached, int recursive,\n> +\t\t\t struct strvec *args,\n> \t\t\t const struct option *options)\n> {\n> \tconst char *cmd;\n> @@ -79,10 +80,13 @@ static void setup_helper_args(int argc, const char **argv, const char *prefix,\n> \t\treturn;\n> \t}\n> \n> -\t/* Did we get --cached with a command? */\n> +\t/* Did we get a forbidden top-level option with a command? */\n> \tif (cached)\n> \t\tusage_msg_optf(_(\"'%s' option is only supported with explicit 'status'\"),\n> \t\t\t git_submodule_usage, options, \"--cached\");\n> +\tif (recursive)\n> +\t\tusage_msg_optf(_(\"'%s' option is only supported with explicit 'status'\"),\n> +\t\t\t git_submodule_usage, options, \"--recursive\");\n> \n> \n> \t/* Either a valid command, or submodule--helper will barf! */\n> @@ -92,6 +96,9 @@ static void setup_helper_args(int argc, const char **argv, const char *prefix,\n> \targc--;\n> \n> \t/* Options that need to go before user-supplied options */\n> +\tif (!strcmp(cmd, \"status\") && recursive)\n> +\t\tstrvec_push(args, \"--recursive\");\n> +\n\nUnless I'm missing something, doesn't this do nothing because we don't\nset cmd = \"status\" when there is no subcommand, and instead, we return\nearly?\n\n> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\n> index c524398e805..7cafc2e1102 100755\n> --- a/t/t7400-submodule-basic.sh\n> +++ b/t/t7400-submodule-basic.sh\n> @@ -20,10 +20,6 @@ test_expect_success 'submodule usage: -h' '\n> \ttest_must_be_empty err\n> '\n> \n> -test_expect_success 'submodule usage: --recursive' '\n> -\ttest_expect_code 129 git submodule --recursive\n> -'\n> -\n> test_expect_success 'submodule usage: status --' '\n> \tgit submodule -- &&\n> \tgit submodule --end-of-options\n> @@ -38,7 +34,7 @@ do\n> \t'\n> done\n> \n> -for opt in '--cached'\n> +for opt in '--cached' '--recursive'\n> do\n> \ttest_expect_success \"submodule usage: status $opt\" '\n> \t\tgit submodule $opt &&\n\nFrustratingly, it's not easy for me to test my hypothesis above because\nthere don't seem to be any tests for the output of \"git submodule status\n--recursive\" :(\n\n> -- \n> 2.38.0.1091.gf9d18265e59\n"},{"id":"465379","messageId":"kl6lk04uxew2.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"58637","inReplyTo":"patch-10.10-81f138e460c-20221017T115544Z-avarab@gmail.com","subject":"Re: [PATCH 10/10] submodule: don't use a subprocess to invoke \"submodule--helper\"","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-10-20T23:18:53Z","receivedAt":"2022-10-20T23:19:03Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> In a preceding commit we created \"builtin/submodule.c\" and faithfully\n> tried to reproduce every aspect of \"git-submodule.sh\", including its\n> invocation of \"git submodule--helper\" as a sub-process.\n>\n> Let's do away with the sub-process and invoke\n> \"cmd_submodule__helper()\" directly. Eventually we'll want to do away\n> with \"builtin/submodule--helper.c\" altogether, but let's not do that\n> for now to avoid conflicts with other in-flight topics. Even without\n> those conflicts the resulting diff would be large. We can leave that\n> for a later cleanup.\n\nThanks! e.g. this managed to avoid conflicts with my newly sent out\n\"submodule clone with branches\" v2 [1]\n\n[1] https://lore.kernel.org/git/pull.1321.v2.git.git.1666297238.gitgitgadget@gmail.com\n\n> It's also worth noting that some users were using e.g. \"git\n> submodule--helper list\" directly for performance reasons[2]. With\n> 31955475d1c (submodule--helper: remove unused \"list\" helper,\n> 2022-09-01) released with v2.38.0 the \"list\" command was no longer\n> provided.\n\n[...]\n\n> I think it would make sense to implement a \"--format\" option for \"git\n> submodule foreach\" to help anyone who cares about that remaining\n> performance (and to improve the API, e.g. by supporting \"-z\"), but as\n> far as performance goes this makes the runtime acceptable again.\n\nFWIW, I don't think that dropping \"git submodule--helper list\" was a\nmistake, since all of \"git submodule--helper\" was meant to be internal\nanyway. But if users find it useful, we could add actually supported\nfunctionality like what you propose here.\n\n> The pattern in \"cmd_submodule_builtin()\" of saving \"struct strvec\"\n> arguments to a \"struct string_list\" and free()-ing them after the\n> \"argv\" has been modified by \"cmd_submodule__helper()\" is new, without\n> it we'd get various already-passing tests failing under SANITIZE=leak.\n\n[...] \n\n> +static int cmd_submodule_builtin(struct strvec *args, const char *prefix)\n> +{\n> +\tsize_t i;\n> +\tstruct string_list to_free = STRING_LIST_INIT_DUP;\n> +\tint ret;\n> +\n> +\t/*\n> +\t * The cmd_submodule__helper() will treat the argv as\n> +\t * its own and modify it, so e.g. for \"git submodule\n> +\t * add\" the \"add\" argument will be removed, and we'll\n> +\t * thus leak from the strvec_push()'s in\n> +\t * setup_helper_args().\n> +\t *\n> +\t * So in lieu of some generic \"snapshot for a free\"\n> +\t * API for \"struct strvec\" squirrel away the pointers\n> +\t * to free with string_list_clear() later.\n> +\t */\n> +\tfor (i = 0; i < args->nr; i++)\n> +\t\tstring_list_append_nodup(&to_free, (char *)args->v[i]);\n> +\n> +\tret = cmd_submodule__helper(args->nr, args->v, prefix);\n> +\n> +\tstring_list_clear(&to_free, 0);\n\nHm, so this trick works because we init STRING_LIST_INIT_DUP to make the\nstring_list think that it owns its strings, but when we append, we use\nstring_list_append_nodup(), which doesn't dup the strings. This\n'moves' the strings into the string_list and causes them to be freed\nwhen we call string_list_clear().\n\nSounds reasonable enough to me, but I'm not an expert on leaks :)\n\n> +\tfree(strvec_detach(args));\n> +\n> +\treturn ret;\n> +}\n> +\n> int cmd_submodule(int argc, const char **argv, const char *prefix)\n> {\n> \tint opt_quiet = 0;\n> \tint opt_cached = 0;\n> \tint opt_recursive = 0;\n> -\tstruct child_process cp = CHILD_PROCESS_INIT;\n> +\tstruct strvec args = STRVEC_INIT;\n> \tstruct option options[] = {\n> \t\tOPT__QUIET(&opt_quiet, N_(\"be quiet\")),\n> \t\tOPT_BOOL(0, \"cached\", &opt_cached,\n> @@ -141,13 +169,10 @@ int cmd_submodule(int argc, const char **argv, const char *prefix)\n> \t * Tell the rest of git that any URLs we get don't come\n> \t * directly from the user, so it can apply policy as appropriate.\n> \t */\n> -\tstrvec_push(&cp.env, \"GIT_PROTOCOL_FROM_USER=0\");\n> -\tsetup_helper_args(argc, argv, prefix, opt_quiet, opt_cached,\n> -\t\t\t opt_recursive, &cp.args, options);\n> +\txsetenv(\"GIT_PROTOCOL_FROM_USER\", \"0\", 1);\n> \n> -\tcp.git_cmd = 1;\n> -\tcp.no_stdin = 0; /* for git submodule foreach */\n> -\tcp.dir = startup_info->original_cwd;\n> +\tsetup_helper_args(argc, argv, prefix, opt_quiet, opt_cached,\n> +\t\t\t opt_recursive, &args, options);\n> \n> -\treturn run_command(&cp);\n> +\treturn cmd_submodule_builtin(&args, prefix);\n> }\n> -- \n> 2.38.0.1091.gf9d18265e59\n"}]}