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

From: Stefan Beller <sbeller@google.com>
Date: Apr 25, 2016, 17:15 UTC
Message-ID: <CAGZ79kZ-jszUtk4R+nWidmjGBFwAwymgr+nzghGSyUVqx=Y5-Q@mail.gmail.com>
In-Reply-To: <65302315-2C33-4801-887C-9AF3CB2F6BB6@gmail.com>

On Mon, Apr 25, 2016 at 5:18 AM, Lars Schneider <larsxschneider@gmail.com> wrote:

Show 8 quoted lines

>> @@ -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?

Show 10 quoted lines

>> @@ -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 :)