{"thread":{"id":"37148","subject":"[PATCH] use local cloning if insteadOf makes a local URL","startedAt":"2014-07-17T07:09:32Z","lastAt":"2014-07-17T18:41:26Z","messageCount":3,"participants":["Michael Barabanov","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"246217","messageId":"1405580972-26078-1-git-send-email-michael.barabanov@windriver.com","threadId":"37148","inReplyTo":null,"subject":"[PATCH] use local cloning if insteadOf makes a local URL","fromName":"Michael Barabanov","fromEmail":"michael.barabanov@windriver.com","sentAt":"2014-07-17T07:09:32Z","receivedAt":"2014-07-17T07:09:32Z","isPatch":true,"sender":{"key":"michael.barabanov@windriver.com","avatar":null},"body":"Move the is_local logic to the place where origin remote has been setup and\ncheck if the remote url can be used to do local cloning.\n\nThis saves a lot of space (and time) in some of the mirroring scenarios that\ninvolve insteadOf rewrites.\n\nSigned-off-by: Michael Barabanov <michael.barabanov@windriver.com>\n---\n builtin/clone.c | 25 +++++++++++++------------\n 1 file changed, 13 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex f0dabec..bbd169c 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -799,18 +799,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\tdie(_(\"repository '%s' does not exist\"), repo_name);\n \telse\n \t\trepo = repo_name;\n-\tis_local = option_local != 0 && path && !is_bundle;\n-\tif (is_local) {\n-\t\tif (option_depth)\n-\t\t\twarning(_(\"--depth is ignored in local clones; use file:// instead.\"));\n-\t\tif (!access(mkpath(\"%s/shallow\", path), F_OK)) {\n-\t\t\tif (option_local > 0)\n-\t\t\t\twarning(_(\"source repository is shallow, ignoring --local\"));\n-\t\t\tis_local = 0;\n-\t\t}\n-\t}\n-\tif (option_local > 0 && !is_local)\n-\t\twarning(_(\"--local is ignored\"));\n \n \t/* no need to be strict, transport_set_option() will validate it again */\n \tif (option_depth && atoi(option_depth) < 1)\n@@ -903,6 +891,19 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \n \tremote = remote_get(option_origin);\n \ttransport = transport_get(remote, remote->url[0]);\n+\tpath = get_repo_path(remote->url[0], &is_bundle);\n+\tis_local = option_local != 0 && path && !is_bundle;\n+\tif (is_local) {\n+\t\tif (option_depth)\n+\t\t\twarning(_(\"--depth is ignored in local clones; use file:// instead.\"));\n+\t\tif (!access(mkpath(\"%s/shallow\", path), F_OK)) {\n+\t\t\tif (option_local > 0)\n+\t\t\t\twarning(_(\"source repository is shallow, ignoring --local\"));\n+\t\t\tis_local = 0;\n+\t\t}\n+\t}\n+\tif (option_local > 0 && !is_local)\n+\t\twarning(_(\"--local is ignored\"));\n \ttransport->cloning = 1;\n \n \tif (!transport->get_refs_list || (!is_local && !transport->fetch))\n-- \n1.9.1\n"},{"id":"246253","messageId":"xmqqwqbbq4qb.fsf@gitster.dls.corp.google.com","threadId":"37148","inReplyTo":"1405580972-26078-1-git-send-email-michael.barabanov@windriver.com","subject":"Re: [PATCH] use local cloning if insteadOf makes a local URL","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-17T17:23:24Z","receivedAt":"2014-07-17T17:23:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Barabanov <michael.barabanov@windriver.com> writes:\n> Move the is_local logic to the place where origin remote has been setup and\n> check if the remote url can be used to do local cloning.\n>\n> This saves a lot of space (and time) in some of the mirroring scenarios that\n> involve insteadOf rewrites.\n>\n> Signed-off-by: Michael Barabanov <michael.barabanov@windriver.com>\n> ---\n\nStrictly speaking, this change introduces a regression to those who\nwould rather use --no-local if they knew the are cloning from a\nlocal copy, but they can rewrite their insteadOf to use file:// URL\nas a workaround.\n\nThe code change looked OK from a cursory read, modulo possibly\nintroducing a new memory leak on the old value of \"path\" (I didn't\ncheck carefully).\n\nThanks.\n\n>  builtin/clone.c | 25 +++++++++++++------------\n>  1 file changed, 13 insertions(+), 12 deletions(-)\n>\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index f0dabec..bbd169c 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -799,18 +799,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n>  \t\tdie(_(\"repository '%s' does not exist\"), repo_name);\n>  \telse\n>  \t\trepo = repo_name;\n> -\tis_local = option_local != 0 && path && !is_bundle;\n> -\tif (is_local) {\n> -\t\tif (option_depth)\n> -\t\t\twarning(_(\"--depth is ignored in local clones; use file:// instead.\"));\n> -\t\tif (!access(mkpath(\"%s/shallow\", path), F_OK)) {\n> -\t\t\tif (option_local > 0)\n> -\t\t\t\twarning(_(\"source repository is shallow, ignoring --local\"));\n> -\t\t\tis_local = 0;\n> -\t\t}\n> -\t}\n> -\tif (option_local > 0 && !is_local)\n> -\t\twarning(_(\"--local is ignored\"));\n>  \n>  \t/* no need to be strict, transport_set_option() will validate it again */\n>  \tif (option_depth && atoi(option_depth) < 1)\n> @@ -903,6 +891,19 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n>  \n>  \tremote = remote_get(option_origin);\n>  \ttransport = transport_get(remote, remote->url[0]);\n> +\tpath = get_repo_path(remote->url[0], &is_bundle);\n> +\tis_local = option_local != 0 && path && !is_bundle;\n> +\tif (is_local) {\n> +\t\tif (option_depth)\n> +\t\t\twarning(_(\"--depth is ignored in local clones; use file:// instead.\"));\n> +\t\tif (!access(mkpath(\"%s/shallow\", path), F_OK)) {\n> +\t\t\tif (option_local > 0)\n> +\t\t\t\twarning(_(\"source repository is shallow, ignoring --local\"));\n> +\t\t\tis_local = 0;\n> +\t\t}\n> +\t}\n> +\tif (option_local > 0 && !is_local)\n> +\t\twarning(_(\"--local is ignored\"));\n>  \ttransport->cloning = 1;\n>  \n>  \tif (!transport->get_refs_list || (!is_local && !transport->fetch))\n"},{"id":"246261","messageId":"53C818D6.7020907@windriver.com","threadId":"37148","inReplyTo":"xmqqwqbbq4qb.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] use local cloning if insteadOf makes a local URL","fromName":"Michael Barabanov","fromEmail":"michael.barabanov@windriver.com","sentAt":"2014-07-17T18:41:26Z","receivedAt":"2014-07-17T18:41:26Z","isPatch":true,"sender":{"key":"michael.barabanov@windriver.com","avatar":null},"body":"\n\nOn 07/17/2014 10:23 AM, Junio C Hamano wrote:\n> Michael Barabanov <michael.barabanov@windriver.com> writes:\n>> Move the is_local logic to the place where origin remote has been setup and\n>> check if the remote url can be used to do local cloning.\n>>\n>> This saves a lot of space (and time) in some of the mirroring scenarios that\n>> involve insteadOf rewrites.\n>>\n>> Signed-off-by: Michael Barabanov <michael.barabanov@windriver.com>\n>> ---\n>\n> Strictly speaking, this change introduces a regression to those who\n> would rather use --no-local if they knew the are cloning from a\n> local copy, but they can rewrite their insteadOf to use file:// URL\n> as a workaround.\n\nPlus --no-local still works on command line, even if rewritten URL ends \nup to be local.\n\n>\n> The code change looked OK from a cursory read, modulo possibly\n> introducing a new memory leak on the old value of \"path\" (I didn't\n> check carefully).\n\nIn fact, I couldn't find where original \"path\" is freed anyway.\nMemory will be freed when the clone process exits of course.\n\nMichael.\n\n>\n> Thanks.\n>\n>>  builtin/clone.c | 25 +++++++++++++------------\n>>  1 file changed, 13 insertions(+), 12 deletions(-)\n>>\n>> diff --git a/builtin/clone.c b/builtin/clone.c\n>> index f0dabec..bbd169c 100644\n>> --- a/builtin/clone.c\n>> +++ b/builtin/clone.c\n>> @@ -799,18 +799,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n>>  \t\tdie(_(\"repository '%s' does not exist\"), repo_name);\n>>  \telse\n>>  \t\trepo = repo_name;\n>> -\tis_local = option_local != 0 && path && !is_bundle;\n>> -\tif (is_local) {\n>> -\t\tif (option_depth)\n>> -\t\t\twarning(_(\"--depth is ignored in local clones; use file:// instead.\"));\n>> -\t\tif (!access(mkpath(\"%s/shallow\", path), F_OK)) {\n>> -\t\t\tif (option_local > 0)\n>> -\t\t\t\twarning(_(\"source repository is shallow, ignoring --local\"));\n>> -\t\t\tis_local = 0;\n>> -\t\t}\n>> -\t}\n>> -\tif (option_local > 0 && !is_local)\n>> -\t\twarning(_(\"--local is ignored\"));\n>>\n>>  \t/* no need to be strict, transport_set_option() will validate it again */\n>>  \tif (option_depth && atoi(option_depth) < 1)\n>> @@ -903,6 +891,19 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n>>\n>>  \tremote = remote_get(option_origin);\n>>  \ttransport = transport_get(remote, remote->url[0]);\n>> +\tpath = get_repo_path(remote->url[0], &is_bundle);\n>> +\tis_local = option_local != 0 && path && !is_bundle;\n>> +\tif (is_local) {\n>> +\t\tif (option_depth)\n>> +\t\t\twarning(_(\"--depth is ignored in local clones; use file:// instead.\"));\n>> +\t\tif (!access(mkpath(\"%s/shallow\", path), F_OK)) {\n>> +\t\t\tif (option_local > 0)\n>> +\t\t\t\twarning(_(\"source repository is shallow, ignoring --local\"));\n>> +\t\t\tis_local = 0;\n>> +\t\t}\n>> +\t}\n>> +\tif (option_local > 0 && !is_local)\n>> +\t\twarning(_(\"--local is ignored\"));\n>>  \ttransport->cloning = 1;\n>>\n>>  \tif (!transport->get_refs_list || (!is_local && !transport->fetch))\n>\n"}]}