From: Stefan Beller <sbeller@google.com>
Date: Thu, 04 Feb 2016 20:22:28 GMT
Subject: Re: [PATCH 6/8] git submodule update: have a dedicated helper for cloning
Message-ID: <CAGZ79kb-G=GHdtcH323LbHQ4V_azif04eHVuf8Z3gCeb-e=iKQ@mail.gmail.com>
In-Reply-To: <xmqqegctjnvm.fsf@gitster.mtv.corp.google.com>

On Wed, Feb 3, 2016 at 4:54 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> On Wed, Feb 3, 2016 at 3:24 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Stefan Beller <sbeller@google.com> writes:
>>>
>>>> +             if (ce_stage(ce)) {
>>>> +                     if (pp->recursive_prefix)
>>>> +                             strbuf_addf(err, "Skipping unmerged submodule %s/%s\n",
>>>> +                                     pp->recursive_prefix, ce->name);
>>
>> As a side question: Do we care about proper visual directory
>> separators in Windows?
>
> You know I do not do Windows ;-)  I'll leave the question for others
> to answer (I am trying not to be one of the the only small number of
> people who review code around here).
>
>> I never run into this BUG after having proper initialization, so maybe it's not
>> worth carrying this code around. (We have many other places where
>> submodule_from_{path, name} is used unchecked, so why would this place
>> be special?)
>
> That is why I wondered if moudule_list() is a better place to do so.
> That is where the list of everybody works on come from.

I do not think that is a better place as not every consumer of module_list
(and module_list_compute as the nested function) will need to use the
submodule caching API. So these consumers are not interested in possible
bugs in the submodule cache API nor do they want the performance hit
which comes from checking unrelated stuff in there.

As said, I only saw this bug when the cache was not initialized properly,
and then such a bug is to be expected. I'd rather remove it in a reroll.