Re: [PATCH v5 9/9] submodule: move core cmd_update() logic to C

[Glen Choo <chooglen@google.com>]

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

> On Thu, Feb 03 2022, Ævar Arnfjörð Bjarmason wrote:
>
>> On Wed, Feb 02 2022, Glen Choo wrote:
>>
>>> - Junio pointed out that this conflicts with
>>>   es/superproject-aware-submodules [2]. I'm not sure which should be
>>>   based on which. If this does end up being based on
>>>   es/superproject-aware-submodules, it would probably be easier to
>>>   rebase as a series of smaller patches. Atharva noted that the
>>>   conflicts are mild though, so maybe it's not so bad.
>>
>> I think it makes sense to get this series through first, i.e. the
>> (supposedly) no-behavior-changing one, and then one that introduces new
>> submodule behavior.
>>
>> Particularly because for es/superproject-aware-submodules the main
>> selling point is a performance improvement, which as I noted in the
>> review for it I've been unable to observe once the C<->sh layer goes
>> away.
>>
>> I'm not saying it's not there, just that I don't think it's been shown
>> so far, IIRC there was some reference to some Google-internal network FS
>> that might or might not be helped by it...

I'll let the experts chime in, I don't think I can add anything useful
to the discussion.

>>> My ideal patch organization would be something like:
>>>
>>> - wrap some existing command in "git submodule--helper update" (probably
>>>   run-update-procedure)
>>> - absorb the surrounding sh code into "git submodule--helper
>>>   update" one command at-a-time i.e. deprecating and removing the
>>>   commands one at a time - instead of deprecating and removing them all
>>>   at once (like this patch), or deprecating all at once and removing
>>>   them one at a time (like v1).
>>
>> I do think atomic changes that don't leave dead code for removal later
>> are easier to read & reason about, whatever else is reorganized.
>>
>> I.e. not to have something where we replace all the running code, and
>> then remove already-unused code later.

I agree - otherwise patches aren't self-contianed and harder to merge.

>>> - If you think this alternative organization would be helpful for you
>>>   too, I will attempt it. This will take a while, but by the end you and
>>>   I will have effectively reviewed all of the code, so it should be easy
>>>   to finish up the review.
>>
>> I think it might, but I really don't know. We'll just have to see, so if
>> you want to take a stab at it that would be great.
>>
>> Maybe it's a good way forward. E.g. as af first small step we could turn:
>>
>>     while read -r quickabort sha1 just_cloned sm_path
>>     [...]
>>     die_if_unmatched "$quickabort" "$sha1"
>>
>> into version where we fold that die_if_unmatched() logic into the C
>> code, and then ensure-core-worktree etc.
>
> Sorry, that one makes no sense since it's an artifact of the shellscript
> implementation.

Whether or not it makes sense, I think it gets the point across i.e.
that we think folding into C can be done incrementally.

>>> - Otherwise e.g. maybe this is a huge waste of time, or you're already
>>>   really confident in the correctness of the sh -> c when you reviewed
>>>   the original patch, etc, I'll just review this patch as-is. I'd
>>>   appreciate any tips and tricks that might help :)
>>
>> I'm not really confident in it.
>>
>> I've read it, tested it as well as I could manage etc. but it's still a
>> very large change.
[...]
> But I tested the below on top of master, and it passes all tests, which
> isn't very promising...

A good enough (i.e. extremely comprehensive) test suite will all but
guarantee that no behavior has changed. Our test suite is nowhere near
that level and probably never will be, so we can't trust that things are
correct even if it passes tests.

So, for the sake of reviewability, I'll take a stab at reorganizing.
I'll be taking a long flight anyway, so I'll have big chunk of
non-company time to spend on this ;)