Re: [RFC PATCH 00/20] submodule: remove git-submodule.sh, create bare builtin/submodule.c

[Glen Choo <chooglen@google.com>]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

>> 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).
>
> That's quite the timing coincidence. I hacked this up yesterday,
> thinking that the submodule topic had been too quiet for a while, and
> wondering how hard it was to convert the rest of git-submodule.sh.
>
> It's more than 2x the length of yours, but gets to the point where we
> can "git rm git-submodule.sh".

Very cool. I've skimmed through all of the patches, which mostly look
good except for ~1-2 things.

Your series shows that there isn't any prohibitively difficult work left
to finish the conversion, which is great! The real problem IMO is the
potential for mechanical errors given how many lines this touches.

Here's a way of breaking apart the work that makes sense to me:

- Reuse the patches that prepare git-submodule.sh for the conversion,
  particularly 1-7/20 (create a "case" dispatch statement and its
  preceding patches).
- Keep my series that prepares "update", since that's the most tedious
  one to convert. If I don't dispatch to the "case" statement, I don't
  think it will even conflict with the preparatory series.

  Some of your patches make more sense than mine, and I'll incorporate
  them as necessary :)
- Dispatch subcommands using the "case" dispatch, including "update". We
  might have to do this slowly if we want things to be easy to eyeball.
- "git rm git-submodule.sh"!

>> Glen Choo (8):
>>   submodule update: remove intermediate parsing
>>   submodule update: pass options containing "[no-]"
>>   submodule update: pass options with stuck forms
>
> Yeah, this is the alternate approach I considered and ended up
> discarding. I.e. to make forward progress with migrating things away
> from the cmd_*() functions you either have to prepare things in
> advance and then sweep the rug from under them in one go.
>
> Or, as you're doing here teaching them about the options they're
> not-really-parsing anymore, but must know about because they're in a
> loop that ends with a "if unknown option, usage".

Yes, if you took as many steps as I did, your series would be way too
long :P

To convert "update", I don't think this many steps is necessary; I
prepared it this way primarily to make it easier for everyone to spot
how the options changed so that they can give feedback. Some of these
can be squashed in my reroll

>>   submodule--helper update: use one param per type
>
> Same as my 13/20, but I ended up doing it in a more narrow/smaller
> way. I tried your way and ran into some bug, then figured I'd do it
> more narrowly instead of debugging it.

Yeah your approach is easier to eyeball, so I'll do this instead.

>>   submodule update: remove -v, pass --quiet
>
> Hrm, so we don't need it at all then. Well, that's a bit simpler than
> my 1[45]/20 and 17/20 :)
>
> So yeah, definitely RFC-quality, but I ran into that one test that
> used -v, and then saw the missing docs etc. But no cheating, so I've
> left it in :)
>
> I do wonder if we should leave it in anyway, we never documented -v,
> but we *did* understand it, and if you look at:
>
>     git log -p -Gsay -- git-submodule.sh
>
> We used to have a lot more code impacted by it, but looking at this
> again now it would have only been for users of command-lines like:
>
>     git submodule --quiet update -v [...]
>
> I.e. where we already set the flag to the non-default quiet, and then
> used -v to flip it.
>
> I think at this point I've talked myself into "let's just remove it",
> but maybe...

On hindsight, what I did is definitely cheating ;)

My series also breaks the way we'd handle --quiet in "git submodule",
i.e.

   git submodule --quiet update

should be quiet, but isn't.

Your approach actually handles --quiet as per the original shell script,
which is a good enough reason to do it your way. We can think about
removing it later.

> Brief commentary on my patches, details in commit messages:
>
> Ævar Arnfjörð Bjarmason (20):
>   git-submodule.sh: remove unused sanitize_submodule_env()
>   git-submodule.sh: remove unused $prefix variable
>   git-submodule.sh: remove unused --super-prefix logic
>
> I removed a bit more dead code here than yours.
>
>   git-submodule.sh: normalize parsing of "--branch"
>   git-submodule.sh: normalize parsing of --cached
>
> This & various other prep commits (hereafter "easy prep") make
> subsequent one-time conversions of whole cmd_*() easier.
>
>   submodule--helper: rename "absorb-git-dirs" to "absorbgitdirs"
>   git-submodule.sh: create a "case" dispatch statement
>
> easy prep

This would all make sense in a preparatory series, with the exception of 
3/20 git-submodule.sh: remove unused --super-prefix logic.

We have several instances where we invoke submodule--helper directly
with --super-prefix, e.g. inside sync_submodule():
    
    if (flags & OPT_RECURSIVE) {
      struct child_process cpr = CHILD_PROCESS_INIT;

      cpr.git_cmd = 1;
      cpr.dir = path;
      prepare_submodule_repo_env(&cpr.env_array);

      strvec_push(&cpr.args, "--super-prefix"); /* Here */

I even have a (as of now private) patch that replaces "update"'s
--recursive-prefix with --super-prefix.

This probably wasn't caught in the tests because this only affects how
we calculate the submodule 'displayname'.

>   submodule--helper: pretend to be "git submodule" in "-h" output
>
> easy prep & bug fix for existing (on master) output bugs.
>
>   git-submodule.sh: dispatch "sync" to helper
>   git-submodule.sh: dispatch directly to helper
>   git-submodule.sh: dispatch "foreach" to helper
>
> These are easy conversions as the options 1=1 map after the above
> prep.

Yes, these are pretty easy. I'm worried about the number of lines
changed and the potential for mechanical errors, but we can roll these
more slowly if necessary.

>   submodule--helper: have --require-init imply --init
>   submodule--helper: understand --checkout, --merge and --rebase
>     synonyms
>   git-submodule doc: document the -v" option to "update"
>   submodule--helper: understand -v option for "update"
>
> not-so-easy prep for "cmd_update()"
>
>   git-submodule.sh: dispatch "update" to helper
>
> Full cmd_update() migration in one go.

Yeah, and since it's not-so-easy, it probably makes sense to continue to
keep my series around. I'll borrow some of these patches if that's ok :)

>   git-submodule.sh: use "$quiet", not "$GIT_QUIET"
>
> "easy prep", but this one is less overall churn if done at the end,
> but as noted above could/should maybe be dropped entirely.
>
>   git-submodule.sh: simplify parsing loop
>
> Not really needed, but I wanted to get the code as close to minimal
> for the next step, to eyeball the resulting sh v.s. C version.
>
>   submodule: make it a built-in, remove git-submodule.sh
>
> We now have a builtin/submodule.c *and* the current
> builtin/submodule--helper.c, and we even dispatch to "git
> submodule--helper" via run_command()!
>
> The idea is to be as close as possible to a bug-for-bug implementation
> of the shellscript, and that reviewers should be confident in being
> able to trace what commands we invoked before/after, we're invoking
> the same "git submodule--helper" commands.
>
> Of course we eventually want to get to some full union of
> builtin/submodule{,--helper}.c, but that can wait.
>
>   submodule: add a subprocess-less submodule.useBuiltin setting
>
> Wait, a useBuiltin setting to switch between two built-ins? Yeah,
> maybe it makes little sense, but here we get rid of the run_command()
> overhead, and could generally use the built-in to experiment with
> deeper integration between the two.
>
> ...

Interesting approach. It looks ok to me, but if we break up this series,
maybe this will be stale by the time we integrate the rest of the
changes?