Re: [PATCH 8/8] git submodule update: Have a dedicated helper for cloning

[Stefan Beller <sbeller@google.com>]

On Wed, Oct 21, 2015 at 2:23 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> I'd like to counter your argument with quoting code from update_clone
>> method:
>>
>>      run_processes_parallel(1, get_next_task, start_failure,
>> task_finished, &pp);
>>
>>      if (pp.print_unmatched) {
>>          printf("#unmatched\n");
>>          return 1;
>>      }
>>
>>      for_each_string_list_item(item, &pp.projectlines) {
>>          utf8_fprintf(stdout, "%s", item->string);
>>      }
>>
>> So we do already all the cloning first, and then once we did all of that
>> we just put out all accumulated lines of text. (It was harder to come up with
>> a sufficient file name than just storing it in memory. I don't think
>> memory is an
>> issue here, only a few bytes per submodule. So even 1000 submodules would
>> consume maybe 100kB)
>
> That does not sound like a counter-argument; two bad design choices
> compensating each other's shortcomings, perhaps ;-)

I was phrasing it worse than I meant to. I should have pointed out the
positive aspect of having first all clones done and then the
local post processing in the downstream pipe afterwards.

>
>> Having a file though would allow us to continue after human
>> interaction fixed one problem.
>
> Yes.  That does sound like a better design.

I don't think the proposed patches make it worse than it already is.
Before we have the "submodule--helper list" which tells downpipe to
do all the things. Now we just take out the cloning and make it an
upfront action, eventually faster due to parallelism.

Also I think we should not promote "git submodule" and specially
its update sub-command to be the best command available. Ideally
we want to rather implement implicit submodule handling in the
other commands such as clone, pull, fetch, checkout, merge, rebase.
and by that I mean better defaults than just "don't touch the submodules,
as that's the safest thing to do now".

>
> This obviously depends on the impact to the other part of what
> cmd_update() does, but your earlier idea to investigate the
> feasibility and usefulness of updating "clone --recurse-submodules"
> does sound like a good thing to do, too.  That's an excellent point.

I investigated and I think it's a bad idea now :)
Because of the --recursive switch we would need to do more than just

    submodules_init()
    run_parallel(.. clone_and_checkout...);

but each cloned submodule would need to be inspected for recursive
submodules again and then we would need to add that to the list of
submodules to process.

I estimate this is about as much work as improving "git submodule update"
to do uncontroversial checkouts in the first parallel phase.