On Wed, Feb 3, 2016 at 3:24 PM, Junio C Hamano <gits...@pobox.com> wrote:
> Stefan Beller <sbel...@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;
>> + }
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
