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

[Stefan Beller <sbeller@google.com>]

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?

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

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

>
>> +             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;
>> +             }