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

From: Stefan Beller <sbeller@google.com>
Date: Oct 21, 2015, 22:14 UTC
Message-ID: <CAGZ79kZiwf7AeCFmeA+FyF8p92+K5ZCEDySx07g+VJ4Dg8MK1Q@mail.gmail.com>
In-Reply-To: <xmqqvb9zudai.fsf@gitster.mtv.corp.google.com>

On Wed, Oct 21, 2015 at 2:23 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 26 quoted lines

> 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.

Show 5 quoted lines

>
>> 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".

Show 5 quoted lines

>
> 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.