Re: [PATCHv3 6/9] clone: implement optional references

From: Stefan Beller <sbeller@google.com>
Date: Aug 9, 2016, 18:20 UTC
Message-ID: <CAGZ79kbS1_DULjf8gPram+zNLe-xRShETotq+at+r7bmWrUyxA@mail.gmail.com>
In-Reply-To: <xmqq4m6tbzfd.fsf@gitster.mtv.corp.google.com>

On Tue, Aug 9, 2016 at 10:54 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 8 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
>
>> If I am reading the implementation of real_path_internal()
>> correctly, the most relevant reason that an "if-able" repository
>> cannot be used causes real_path_if_valid() to return NULL.
>
> Oops, too many proofreading and rephrasing.  Please insert "I do not
> think that" before "the most relevant".

Side note:
  It took me a while to mentally process and reread this.
  It would have been easier with just
  s/the most relevant reason/I do not think that the most relevant reason/
  but that is just me being used to s/ expressions by now.

I think our emails nearly crossed. In the other thread I wondered if you had different expectations on the -if-able part than I do.

For the expected use case I do expect that the missing path is the most relevant thing (the submodule is not checked out).

Of course the submodule may be cloned shallow as it was recommended in the .gitmodules file, so we should also care about that use case.

> Let's say your superproject borrows from ~/w/super, whose submodule
> repositories (if cloned) are directly in "~/w/super/.git/modules/".
> When you clone a submodule X for your superproject, you allow it to
> borrow from "~/w/super/.git/modules/X" if there is one available.

which is why we need to pass in ..../X/ with an ending slash. See the comment in 8/9.

+               /*
+                * We need to end the new path with '/' to mark it as a dir,
+                * otherwise a submodule name containing '/' will be broken
+                * as the last part of a missing submodule reference would
+                * be taken as a file name.
+                */
+               strbuf_addf(&sb, "modules/%s/", sas->submodule_name);

Thinking about that we may want to not rely on such a hack, but make it clearer.

Thanks, Stefan