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

From: Stefan Beller <sbeller@google.com>
Date: Feb 3, 2016, 23:41 UTC
Message-ID: <CAGZ79kbeDBf=AfA2RUhkfjwJqJ7pr30xW3UTXqiha_EPpisvnw@mail.gmail.com>
In-Reply-To: <xmqqr3gtjs23.fsf@gitster.mtv.corp.google.com>

On Wed, Feb 3, 2016 at 3:24 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 6 quoted lines

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

Show 21 quoted lines

>> +                     else
>> +                             strbuf_addf(err, "Skipping unmerged submodule %s\n",
>> +                                     ce->name);
>> +                     continue;
>
> This raised my eyebrow somewhat, until I realized that it is OK
> because module_list prepared by the caller has only one of the
> unmerged entries for the same path to avoid duplicates.
>
>> +             }
>> +
>> +             sub = submodule_from_path(null_sha1, ce->name);
>> +             if (!sub) {
>> +                     strbuf_addf(err, "BUG: internal error managing submodules. "
>> +                                 "The cache could not locate '%s'", ce->name);
>> +                     pp->print_unmatched = 1;
>> +                     continue;
>
> Interesting.
>
> I am wondering if this check should go to module_list_compute().

Actually this has evolved from a debugging leftover camouflaged as a useful thing. I added that and then realized I did not add

        gitmodules_config();
        git_config(git_submodule_config, NULL);

before, but that code remained there as it seemed useful to me at the time.

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?)

Show 29 quoted lines

>
>> +             }
>> +
>> +             if (pp->recursive_prefix)
>> +                     displaypath = relative_path(pp->recursive_prefix, ce->name, &sb);
>> +             else
>> +                     displaypath = ce->name;
>> +
>> +             if (pp->update)
>> +                     update_module = pp->update;
>> +             if (!update_module)
>> +                     update_module = sub->update;
>> +             if (!update_module)
>> +                     update_module = "checkout";
>> +             if (!strcmp(update_module, "none")) {
>> +                     strbuf_addf(err, "Skipping submodule '%s'\n", displaypath);
>> +                     continue;
>> +             }
>> +
>> +             /*
>> +              * 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.

Show 13 quoted lines

>
>> +             strbuf_addf(&sb, "submodule.%s.url", sub->name);
>> +             git_config_get_string(sb.buf, &url);
>> +             if (!url) {
>> +                     /*
>> +                      * Only mention uninitialized submodules when its
>> +                      * path have been specified
>> +                      */
>> +                     if (pp->pathspec.nr)
>> +                             strbuf_addf(err, _("Submodule path '%s' not initialized\n"
>> +                                     "Maybe you want to use 'update --init'?"), displaypath);
>> +                     continue;
>> +             }