On Thu, Oct 11, 2018 at 3:41 PM Jonathan Tan [off-list ref] wrote:

quoted

+/*
+ * Initialize 'out' based on the provided submodule path.
+ *
+ * Unlike repo_submodule_init, this tolerates submodules not present
+ * in .gitmodules. NEEDSWORK: The repo_submodule_init behavior is
+ * preferrable. This function exists only to preserve historical behavior.

What do you mean by "The repo_submodule_init behavior is preferable"?

Preferable in the sense that it follows the definition of a submodule, but this
here works for "any repo" that happens to be at the gitlink.

 If
we need to preserve historical behavior, then it seems that the most
preferable one is something that meets our needs (like open_submodule()
in this patch).

Yes, I'll reword to drop the preferrable, but still state the difference.

I wonder if instead we'd want to introduce a

    repo_submodule_init(struct repository *submodule \
        struct repository *superproject \
        const char *path, \
        int tolerate_lookalikes)

Another patch proposes to replace the path
by a struct submodule, but for lookalikes, we do not have
a struct submodule to begin with (though in the other
patches we cook up a fake entry in the submodule config)

quoted

+static int open_submodule(struct repository *out, const char *path)
+{
+     struct strbuf sb = STRBUF_INIT;
+
+     if (submodule_to_gitdir(&sb, path) || repo_init(out, sb.buf, NULL)) {
+             strbuf_release(&sb);
+             return -1;
+     }
+
+     out->submodule_prefix = xstrdup(path);

Do we need to set submodule_prefix?

Good point! Thanks for catching this.

quoted

@@ -507,7 +533,7 @@ static void show_submodule_header(struct diff_options *o, const char *path,
      else if (is_null_oid(two))
              message = "(submodule deleted)";

-     if (add_submodule_odb(path)) {
+     if (open_submodule(sub, path) < 0) {

This function, as a side effect, writes the open repository to "sub" for
its caller to use. I think it's better if its callers open "sub" and
then pass it to show_submodule_header() if successful. If opening the
submodule is expected to fail sometimes (like it seems here), then we
can allow callers to also pass NULL, and document what happens when NULL
is passed.

That looks intriguing, I'll take a look. Note that we also pass
in **left and **right to have it assigned in there.

Also, repositories open in this way should also be freed.

Yes, thanks!