Re: [PATCH 1/3] submodule clone: pass along `local` option

[Stefan Beller <sbeller@google.com>]

On Mon, Apr 25, 2016 at 5:18 AM, Lars Schneider
<larsxschneider@gmail.com> wrote:
>> @@ -140,6 +141,10 @@ static int clone_submodule(const char *path, const char *gitdir, const char *url
>>               argv_array_pushl(&cp.args, "--reference", reference, NULL);
>>       if (gitdir && *gitdir)
>>               argv_array_pushl(&cp.args, "--separate-git-dir", gitdir, NULL);
>> +     if (local == 1)
> bikeshedding: Why do you use == 1 here and !local below? I would either compare both against
> integers or none ("if (local)" should work here, too, no?). Or is this a Git coding guideline
> that I am not yet aware of?

"if (local)" doesn't work as it includes -1 as well.
What I want to express here is:

    switch (local) {
    case 1: argv_array_push(&cp.args, "--local"); break; /* local == 1
did that */
    case 0: argv_array_push(&cp.args, "--no-local"); break; /* !local*/
    case -1: default: /* pass nothing */
    }

This seems to be more expressive now, so I'll replace with that?

>> @@ -180,6 +186,8 @@ static int module_clone(int argc, const char **argv, const char *prefix)
>>               OPT_STRING(0, "depth", &depth,
>>                          N_("string"),
>>                          N_("depth for shallow clones")),
>> +             OPT_BOOL(0, "local", &local,
>> +                      N_("to clone from a local repository")),
> TBH I think "local" could be misleading here. How about "--pass-transfer-protocol-on-to-submodules"
> or something? If I understand this option correctly then this could be useful for other cases besides
> "local". Imagine you clone a repo via HTTPS that has references to SSH submodules on the same
> server. If you don't have a proper SSH setup then the submodule clone will fail.

Good point. We just converted an internal repository to use relative
path for submodules instead of
absolute paths and that broke the auth (they used different protocols,
which we were unaware of)

However  "--pass-transfer-protocol-on-to-submodules" would mean more
than just "--[no-]local,
but also passing on the protocol prefix (which can be http://,
https://, ssh://, git:// or more),
so it would enlarge the scope of this series a lot. (Original purpose
of this patch was to allow
not using the explicit file:// protocol in the test for the next
patch. But I see passing on the protocol
as a new issue worth resolving :)