Re: [PATCH] clone: allow cloning local paths with colons in them

From: Duy Nguyen <pclouds@gmail.com>
Date: Apr 28, 2013, 00:19 UTC
Message-ID: <CACsJy8DEMrv08D3wGvebV+W73TA8eTH58KHybXpQQbsSJ5NKZA@mail.gmail.com>
In-Reply-To: <7vip37u11a.fsf@alter.siamese.dyndns.org>

On Sun, Apr 28, 2013 at 4:16 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 29 quoted lines

> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>
>> diff --git a/connect.c b/connect.c
>> index f57efd0..b568f10 100644
>> --- a/connect.c
>> +++ b/connect.c
>> @@ -551,8 +551,11 @@ struct child_process *git_connect(int fd[2], const char *url_orig,
>>       path = strchr(end, c);
>>       if (path && !has_dos_drive_prefix(end)) {
>>               if (c == ':') {
>> -                     protocol = PROTO_SSH;
>> -                     *path++ = '\0';
>> +                     if (!strchr(url, '/') || strchr(url, '/') >= path) {
>> +                             protocol = PROTO_SSH;
>> +                             *path++ = '\0';
>> +                     } else
>> +                             path = end
>>               }
>
> That was fairly hard to grok. Is that equivalent to this?
>
>                 if (c == ':' && path < strchrnul(host, '/')) {
>                         /* is the first slash past the colon? */
>                         protocol = PROTO_SSH;
>                         *path++ = '\0';
>                 } else {
>                         path = end;
>                 }
>

The original code is already hard to grok so I may be mistaken here. But I think it's not the same. For the case when c == '/', it will do "path = end;", which is unintended. It should keep the current "path" value (i.e. == strchr(end, '/')). The use of "strchrnul(host, '/')" is good though. -- Duy