Re: [PATCH v2 00/12] submodule: make "git submodule--helper" behave like "git submodule"

>> 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"!
>
> Hopefully there's no stepping on toes here, but I thought I'd send
> this out now (I went back to the laptop) to avoid the duplicate work,
> since I'd already attempted combining the two, and this is the result.

>>> 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'.
>
> This is still in this series as 02/12. I think you've misunderstood
> that code, it *is* invoking "git submodule--helper" with
> "--super-prefix", but the option is passed as:
>
>     git --super-prefix <path> submodule--helper
>
> And not as:
>
>     git submodule--helper --super-prefix <path>
>
> This is thus handled by other code before builtin/submodule--helper.c,
> and it doesn't need to handle it.
>
> But anyway, this is confusing, so I updated the commit message (seen
> in the range-diff below)>

>>>   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 :)
>
> The proposal in *this series* is to leave this aside for now, but
> generally I wonder what part of it you find not-so-easy.
>
> Personally I find it much harder to carefully review the way you
> proposed to do it, i.e. to "buffer up" options that we "don't handle",
> but actually need to sort-of handle, as we'd still like to die if we
> have unknown options.
>
> Particularly since shellscript quoting etc. is a pain with that sort
> of thing, as it doesn't have any real list or key-value
> datastructures.
>
> Whereas getting it to the point where we're clearly just passing
> options as-is through beforehand, and then simply dropping the wrapper
> is, I think, much easier to review. You only need to trust or check
> that e.g. "git submodule--helper update" also supports a "--progress"
> option or whatever, and/or that we've got coverage for it.