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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 4, 2016, 00:54 UTC
Message-ID: <xmqqegctjnvm.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CAGZ79kbeDBf=AfA2RUhkfjwJqJ7pr30xW3UTXqiha_EPpisvnw@mail.gmail.com>

Stefan Beller <sbeller@google.com> writes:

Show 10 quoted lines

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

Show 12 quoted lines

>>> +             /*
>>> +              * Looking up the url in .git/config.
>>> +              * We must not fall back to .gitmodules as we only want to process
>>> +              * configured submodules.
>>> +              */
>>> +             strbuf_reset(&sb);
>>
>> Doesn't this invalidate displaypath, when pp->recursive_prefix is in
>> effect?  It still seems to be used to create an error message just a
>> few lines below...
>
> Yes that is programmer error. Also the final cleanup of the strbuf is missing.

Yeah, the calling convention to relative_path() might be performant, but I agree that it is not the easiest API function to use correctly.