Thanks! I'm happy to see this happen regardless of whose patches we use
:)
Reading the cover letter, I think it probably makes sense for this to
supersede gc/submodule-update. I haven't really looked at the changes
yet though, but I will soon.
Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes:
>> 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.
Fortunately I hadn't resumed work on this yet, so it works out :)
>>> 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)>
Ah that's right, I forgot that we have to pass it to "git" directly.
Thanks.
I wonder why we ever needed this. 89c8626557 (submodule helper: support
super prefix, 2016-12-08) doesn't really explain it, so it looks like
I'll have to dig around the ML.
>>> 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.
Makes sense. I suppose we don't have to overthink the conversion because
we will have to make the leap of faith to C at some point.
