mbox series

[0/8,RFC] submodule update: parse all options in C

Message ID pull.1275.git.git.1654820781.gitgitgadget@gmail.com (mailing list archive)
Headers show
Series submodule update: parse all options in C | expand

Message

Philippe Blain via GitGitGadget June 10, 2022, 12:26 a.m. UTC 
As a follow up to ar/submodule-update [1] and its successors
gc/submodule-update-part* [2] [3], this series converts the last remaining
piece of "git submodule update" into C, namely, the option parsing in
git-submodule.sh.

As a result, git-submodule.sh::cmd_update() is now an (almost) one-liner:

cmd_update() { git ${wt_prefix:+-C "$wt_prefix"} submodule--helper update
${wt_prefix:+--prefix "$wt_prefix"}
"$@" }

and best of all, "git submodule update" now shows a usage string for its own
subcommand instead of a giant usage string for all of "git submodule" :)

Given how many options "git submodule update" accepts, this series takes a
gradual approach:

 1. Create a variable opts, which holds the literal options we want to pass
    to "git submodule--helper update". Then, for each option...
 2. If "git submodule--helper update" already understands the string option,
    append it to opts and remove any special handling (1-3/8).
 3. Otherwise, if the option makes sense, teach "git submodule--helper
    update" to understand the option. Goto 2. (4-5/8).
 4. Otherwise, if the option makes no sense, drop it (6/8).
 5. When we've processed all options, delete all the option parsing code
    (7/8) and clean up (8/8).

I read over the other "git submodule" subcommands very briefly to see how
much work this would be if we were to repeat the process for all
subcommands. It should be relatively simple - most "git submodule--helper"
commands already understand every CLI option that they need to, so we can
skip step 3 (i.e. the tedious one).

I'm sending this as an RFC because even though this series is pretty
low-risk, the previous topics [1] [2] [3] had their fair share of
regressions (even when the conversions seemed 'obviously correct') and
showed that the test coverage of "git submodule "update" is quite poor. I'd
like to get feedback on the general approach before I invest more time into
improving the test coverage and/or checking the before/after output for
regressions [4].

[1] https://lore.kernel.org/git/20210907115932.36068-1-raykar.ath@gmail.com/
[2] Part 1
https://lore.kernel.org/git/20220305001401.20888-1-chooglen@google.com/ [3]
Part 2
https://lore.kernel.org/git/20220315210925.79289-1-chooglen@google.com/ [4]
Ævar suggested a way to do this using the test suite and "--tee" in
https://lore.kernel.org/git/220214.86h791xsza.gmgdl@evledraar.gmail.com/

Glen Choo (8):
  submodule update: remove intermediate parsing
  submodule update: pass options containing "[no-]"
  submodule update: pass options with stuck forms
  submodule update: pass --require-init and --init
  submodule--helper update: use one param per type
  submodule update: remove -v, pass --quiet
  submodule update: stop parsing options in .sh
  submodule update: remove never-used expansion

 builtin/submodule--helper.c |  41 +++++++----
 git-submodule.sh            | 131 ------------------------------------
 t/t7406-submodule-update.sh |   2 +-
 3 files changed, 29 insertions(+), 145 deletions(-)


base-commit: 1e59178e3f65880188caedb965e70db5ceeb2d64
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1275%2Fchooglen%2Fsubmodule%2Fparse-update-opts-in-c-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1275/chooglen/submodule/parse-update-opts-in-c-v1
Pull-Request: https://github.com/git/git/pull/1275