Re: [PATCH] submodule: implement `module_name` as a builtin helper

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 7, 2015, 21:14 UTC
Message-ID: <xmqqa8u2yf6e.fsf@gitster.dls.corp.google.com>
In-Reply-To: <CAGZ79kYtCgYRHuMcxNoi6f9+GYYYCq6aRTdvx4ZKELSuQErkVQ@mail.gmail.com>

Stefan Beller <sbeller@google.com> writes:

Show 23 quoted lines

> On Fri, Aug 7, 2015 at 1:17 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Jens Lehmann <Jens.Lehmann@web.de> writes:
>>
>> This change...
>>
>>>> @@ -723,10 +733,8 @@ int fetch_populated_submodules(const struct argv_array *options,
>>>>              if (!S_ISGITLINK(ce->ce_mode))
>>>>                      continue;
>>>>
>>>> -            name = ce->name;
>>>> -            name_for_path = unsorted_string_list_lookup(&config_name_for_path, ce->name);
>>>> -            if (name_for_path)
>>>> -                    name = name_for_path->util;
>>>> +            name_for_path = submodule_name_for_path(ce->name);
>>>> +            name =  name_for_path ? name_for_path : ce->name;
>>
>> ... interacts with Heiko's cached submodule config work that seems
>> to have stalled.
>
> We can drop that hunk as it only uses the new method
> `submodule_name_for_path` but doesn't change functionality.
> So if you want to keep Heikos work, I'll just resend the patch
> without that hunk.

Does such a result even make sense? Note that I wasn't talking about textual conflict.

If we followed what you just said, that patch will try to directly read the data in config_name_for_path string list, which is removed by Heiko's series, if I am reading it right.

In the new world order with Heiko's series, the way you grab submodule configuration data is to call submodule_from_path() or submodule_from_name() and they allow you to read from .gitmodules either in a commit (for historical state), the index, or from the working tree. Which should be much cleaner and goes in the right direction in the longer term.

And the best part of the story is that your module_name would be just calling submodule_from_path() and peeking into a field.

Show 5 quoted lines

> 2) Come up with a good thread pool abstraction
>    (Started as "[RFC/PATCH 0/4] parallel fetch for submodules" )
>    This abstraction (if done right) will allow us to use it in different places
>    easily. I started it as part of "git fetch --recurse-submodules" because
>    it is submodule related and reasonably sized

I personally think this gives the most bang-for-buck. Write that and expose it as "git submodule for-each-parallel", which takes the shell scriptlet that currently is the loop body of "while read mode sha1 stage sm_path" in update and clone. You will have immediate and large payback.

And you do not even need module_list and module_name written in C in order to do so. Not that these two are not trivial to implement, but the payoff from them is not as large as from for-each-parallel ;-)