{"thread":{"id":"41634","subject":"[RFC/PATCH] clone: make 'git clone -c remote.origin.fetch=' work","startedAt":"2016-03-07T01:11:02Z","lastAt":"2016-04-01T04:20:01Z","messageCount":12,"participants":["SZEDER Gábor","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"280310","messageId":"1457313062-10073-1-git-send-email-szeder@ira.uka.de","threadId":"41634","inReplyTo":null,"subject":"[RFC/PATCH] clone: make 'git clone -c remote.origin.fetch=' work","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2016-03-07T01:11:02Z","receivedAt":"2016-03-07T01:11:02Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Configuration variables specified via 'git clone -c ='\n\"take effect immediately after the repository is initialized, but\nbefore the remote history is fetched\". This implies that any fetch\nrefspecs specified this way should already be taken into account\nduring the initial fetch and remote refs matching these refspecs\nshould be retrieved as well.\n\nThis never worked, however, not even when the feature was introduced\nin v1.7.7-rc0~90^2 (clone: accept config options on the command line,\n2011-06-09). While the given refspecs are written to the config file\nalright, no matching refs are fetched, because the initial fetch\nignores them and respects only that single default refspec.\n\nCheck whether there are any relevant configured fetch refspecs and\ntake those into account during the initial fetch, unless running 'git\nclone --single-branch'.\n\nSigned-off-by: SZEDER Gábor \n---\n\nI'm unsure what to do with the '-c --single-branch'\ncombination: it doesn't really make sense to me and can't imagaine a\nuse case where it would be useful... but perhaps I just lack\nimagination on this Sunday night. Hence the RFC.\n\n builtin/clone.c | 32 +++++++++++++++++++++++++-------\n t/t5708-clone-config.sh | 13 +++++++++++++\n 2 files changed, 38 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 9ac6c0144279..5b96b373675a 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -515,7 +515,7 @@ static struct ref *find_remote_branch(const struct ref *refs, const char *branch\n }\n \n static struct ref *wanted_peer_refs(const struct ref *refs,\n-\t\tstruct refspec *refspec)\n+\t\tstruct refspec *refspec, unsigned int refspec_count)\n {\n \tstruct ref *head = copy_ref(find_ref_by_name(refs, \"HEAD\"));\n \tstruct ref *local_refs = head;\n@@ -541,8 +541,11 @@ static struct ref *wanted_peer_refs(const struct ref *refs,\n \t\t\t/* if --branch=tag, pull the requested tag explicitly */\n \t\t\tget_fetch_map(remote_head, tag_refspec, &tail, 0);\n \t\t}\n-\t} else\n-\t\tget_fetch_map(refs, refspec, &tail, 0);\n+\t} else {\n+\t\tunsigned int i;\n+\t\tfor (i = 0; i < refspec_count; i++)\n+\t\t\tget_fetch_map(refs, &refspec[i], &tail, 0);\n+\t}\n \n \tif (!option_mirror && !option_single_branch)\n \t\tget_fetch_map(refs, tag_refspec, &tail, 0);\n@@ -840,7 +843,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tint err = 0, complete_refs_before_fetch = 1;\n \n \tstruct refspec *refspec;\n-\tconst char *fetch_pattern;\n+\tunsigned int refspec_count = 1;\n+\tconst char **fetch_patterns;\n+\tconst struct string_list *config_fetch_patterns;\n \n \tpacket_trace_identity(\"clone\");\n \targc = parse_options(argc, argv, prefix, builtin_clone_options,\n@@ -967,9 +972,21 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tif (option_reference.nr)\n \t\tsetup_reference();\n \n-\tfetch_pattern = value.buf;\n-\trefspec = parse_fetch_refspec(1, &fetch_pattern);\n+\tstrbuf_addf(&key, \"remote.%s.fetch\", option_origin);\n+\tconfig_fetch_patterns = git_config_get_value_multi(key.buf);\n+\tif (config_fetch_patterns)\n+\t\trefspec_count = 1 + config_fetch_patterns->nr;\n+\tfetch_patterns = xcalloc(refspec_count, sizeof(*fetch_patterns));\n+\tfetch_patterns[0] = value.buf;\n+\tif (config_fetch_patterns) {\n+\t\tstruct string_list_item *fp;\n+\t\tunsigned int i = 1;\n+\t\tfor_each_string_list_item(fp, config_fetch_patterns)\n+\t\t\tfetch_patterns[i++] = fp->string;\n+\t}\n+\trefspec = parse_fetch_refspec(refspec_count, fetch_patterns);\n \n+\tstrbuf_reset(&key);\n \tstrbuf_reset(&value);\n \n \tremote = remote_get(option_origin);\n@@ -1013,7 +1030,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \trefs = transport_get_remote_refs(transport);\n \n \tif (refs) {\n-\t\tmapped_refs = wanted_peer_refs(refs, refspec);\n+\t\tmapped_refs = wanted_peer_refs(refs, refspec, refspec_count);\n \t\t/*\n \t\t * transport_get_remote_refs() may return refs with null sha-1\n \t\t * in mapped_refs (see struct transport->get_refs_list\n@@ -1094,6 +1111,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tstrbuf_release(&value);\n \tjunk_mode = JUNK_LEAVE_ALL;\n \n+\tfree(fetch_patterns);\n \tfree(refspec);\n \treturn err;\n }\ndiff --git a/t/t5708-clone-config.sh b/t/t5708-clone-config.sh\nindex 27d730c0a720..377837e3539a 100755\n--- a/t/t5708-clone-config.sh\n+++ b/t/t5708-clone-config.sh\n@@ -37,4 +37,17 @@ test_expect_success 'clone -c config is available during clone' '\n \ttest_cmp expect child/file\n '\n \n+test_expect_success 'clone -c remote.origin.fetch= works' '\n+\trm -rf child &&\n+\tgit update-ref refs/grab/it refs/heads/master &&\n+\tgit update-ref refs/keep/out refs/heads/master &&\n+\tgit clone -c \"remote.origin.fetch=+refs/grab/*:refs/grab/*\" . child &&\n+\t(\n+\t\tcd child &&\n+\t\tgit for-each-ref --format=\"%(refname)\" refs/grab/ >../actual\n+\t) &&\n+\techo refs/grab/it >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.7.2.410.g92cb358\n"},{"id":"280313","messageId":"xmqqfuw3rrwb.fsf@gitster.mtv.corp.google.com","threadId":"41634","inReplyTo":"1457313062-10073-1-git-send-email-szeder@ira.uka.de","subject":"Re: [RFC/PATCH] clone: make 'git clone -c remote.origin.fetch=' work","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-07T01:36:04Z","receivedAt":"2016-03-07T01:36:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> Check whether there are any relevant configured fetch refspecs and\n> take those into account during the initial fetch, unless running 'git\n> clone --single-branch'.\n>\n> Signed-off-by: SZEDER Gábor \n> ---\n\nEven though I think the original description did not mean to include\nthe fetch refspecs when it talked about configuration taking effect, \nI think what this change wants to do probably makes sense.\n>\n> I'm unsure what to do with the '-c --single-branch'\n> combination: it doesn't really make sense to me and can't imagaine a\n> use case where it would be useful... but perhaps I just lack\n> imagination on this Sunday night. Hence the RFC.\n\nMy knee-jerk reaction is to change the last paragraph of your log\nmessage to read more like\n\n\tAlways read the fetch refspecs from the newly created config\n\tfile, and use that for the initial fetching.\n\nand do so even when running with \"--single-branch\".\n"},{"id":"280326","messageId":"20160307161931.Horde.TcdEVtHKgSMvScCDUKLclVq@webmail.informatik.kit.edu","threadId":"41634","inReplyTo":"xmqqfuw3rrwb.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/PATCH] clone: make 'git clone -c remote.origin.fetch=' work","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2016-03-07T15:19:31Z","receivedAt":"2016-03-07T15:19:31Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"\nQuoting Junio C Hamano :\n\n> SZEDER Gábor writes:\n>\n>> Check whether there are any relevant configured fetch refspecs and\n>> take those into account during the initial fetch, unless running 'git\n>> clone --single-branch'.\n>>\n>> Signed-off-by: SZEDER Gábor \n>> ---\n>\n> Even though I think the original description did not mean to include\n> the fetch refspecs when it talked about configuration taking effect,\n> I think what this change wants to do probably makes sense.\n\nWell, currently one would have to clone, set additional fetch refspecs,\nfetch again and repack. Using 'git clone -c ' would do it in\na single step, requiring fewer commands, less time, less data transfer\nand less disk space, which fits the justification of v1.7.7-rc0~90^2\nperfectly.\n\nOr init, add remote, set additional refspecs and fetch, which still\nrequires more commands, but at least doesn't transfer more data.\n\n>> I'm unsure what to do with the '-c --single-branch'\n>> combination: it doesn't really make sense to me and can't imagaine a\n>> use case where it would be useful... but perhaps I just lack\n>> imagination on this Sunday night. Hence the RFC.\n>\n> My knee-jerk reaction is to change the last paragraph of your log\n> message to read more like\n>\n> \tAlways read the fetch refspecs from the newly created config\n> \tfile, and use that for the initial fetching.\n>\n> and do so even when running with \"--single-branch\".\n\nOk, will change the '--single-branch' codepath as well.\n\nBut before doing so, to avoid a possible misunderstanding on my part:\nI'm not sure how literally you meant that \"from the newly created\nconfig file\" part, because it ignores refspecs specified via any\nother means, e.g. 'git -c clone'. I think the\ninitial fetch should be no different from \"regular\" fetches, and\nshould respect all configured fetch refspecs regardless where they\ncome from.\n"},{"id":"280327","messageId":"20160307153304.GA23010@sigill.intra.peff.net","threadId":"41634","inReplyTo":"20160307161931.Horde.TcdEVtHKgSMvScCDUKLclVq@webmail.informatik.kit.edu","subject":"Re: [RFC/PATCH] clone: make 'git clone -c remote.origin.fetch=' work","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-07T15:33:05Z","receivedAt":"2016-03-07T15:33:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 07, 2016 at 04:19:31PM +0100, SZEDER Gábor wrote:\n\n> >Even though I think the original description did not mean to include\n> >the fetch refspecs when it talked about configuration taking effect,\n> >I think what this change wants to do probably makes sense.\n> \n> Well, currently one would have to clone, set additional fetch refspecs,\n> fetch again and repack. Using 'git clone -c ' would do it in\n> a single step, requiring fewer commands, less time, less data transfer\n> and less disk space, which fits the justification of v1.7.7-rc0~90^2\n> perfectly.\n\nYeah, I think your change very much fits the spirit of what the original\ncommit was trying for.\n\n> >My knee-jerk reaction is to change the last paragraph of your log\n> >message to read more like\n> >\n> >\tAlways read the fetch refspecs from the newly created config\n> >\tfile, and use that for the initial fetching.\n> >\n> >and do so even when running with \"--single-branch\".\n> \n> Ok, will change the '--single-branch' codepath as well.\n> \n> But before doing so, to avoid a possible misunderstanding on my part:\n> I'm not sure how literally you meant that \"from the newly created\n> config file\" part, because it ignores refspecs specified via any\n> other means, e.g. 'git -c clone'. I think the\n> initial fetch should be no different from \"regular\" fetches, and\n> should respect all configured fetch refspecs regardless where they\n> come from.\n\nIMHO, we should stick to the conceptual model that \"git clone\" is:\n\n git init\n git config ... ;# set up remote, etc\n git fetch\n git checkout ;# obviously not for --bare\n\nThe implementation has to diverge from that to do certain optimizations,\nbut absent any good reason not to, I think we should aim for behaving\n\"as if\" those commands were run.\n\nIt certainly may produce surprising behavior at times, but at least it\nis a conceptually simple mental model. I do admit, though I haven't\nthought hard enough to know whether there are any terrible gotchas\nthere.\n\n-Peff\n"},{"id":"280330","messageId":"xmqqh9gioy67.fsf@gitster.mtv.corp.google.com","threadId":"41634","inReplyTo":"20160307153304.GA23010@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] clone: make 'git clone -c remote.origin.fetch=' work","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-07T20:01:04Z","receivedAt":"2016-03-07T20:01:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> IMHO, we should stick to the conceptual model that \"git clone\" is:\n>\n> git init\n> git config ... ;# set up remote, etc\n> git fetch\n> git checkout ;# obviously not for --bare\n>\n> The implementation has to diverge from that to do certain optimizations,\n> but absent any good reason not to, I think we should aim for behaving\n> \"as if\" those commands were run.\n\nYup, I 100% agree that we should aim for matching that mental model.\n"},{"id":"282230","messageId":"1459349623-16443-1-git-send-email-szeder@ira.uka.de","threadId":"41634","inReplyTo":"20160307153304.GA23010@sigill.intra.peff.net","subject":"[PATCH v2] clone: respect configured fetch respecs during initial fetch","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2016-03-30T14:53:43Z","receivedAt":"2016-03-30T14:53:43Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Conceptually 'git clone' should behave as if the following commands\nwere run:\n\n git init\n git config ... # set default configuration and origin remote\n git fetch\n git checkout # unless '--bare' is given\n\nHowever, that initial 'git fetch' behaves differently from any\nsubsequent fetches, because it takes only the default fetch refspec\ninto account and ignores all other fetch refspecs that might have\nbeen explicitly specified on the command line (e.g. 'git -c\nremote.origin.fetch= clone' or 'git clone -c ...').\n\nCheck whether there are any fetch refspecs configured for the origin\nremote and take all of them into account during the initial fetch as\nwell.\n\nSigned-off-by: SZEDER Gábor \n---\nChanges since previous (RFC) version:\n - new commit message\n - additional configured fetch refspecs are taken into account with\n '--single-branch' as well\n\n builtin/clone.c | 36 ++++++++++++++++++++++++++++--------\n t/t5708-clone-config.sh | 24 ++++++++++++++++++++++++\n 2 files changed, 52 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 661639255c56..5e2d2c21e456 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -515,7 +515,7 @@ static struct ref *find_remote_branch(const struct ref *refs, const char *branch\n }\n \n static struct ref *wanted_peer_refs(const struct ref *refs,\n-\t\tstruct refspec *refspec)\n+\t\tstruct refspec *refspec, unsigned int refspec_count)\n {\n \tstruct ref *head = copy_ref(find_ref_by_name(refs, \"HEAD\"));\n \tstruct ref *local_refs = head;\n@@ -536,13 +536,18 @@ static struct ref *wanted_peer_refs(const struct ref *refs,\n \t\t\twarning(_(\"Could not find remote branch %s to clone.\"),\n \t\t\t\toption_branch);\n \t\telse {\n-\t\t\tget_fetch_map(remote_head, refspec, &tail, 0);\n+\t\t\tunsigned int i;\n+\t\t\tfor (i = 0; i < refspec_count; i++)\n+\t\t\t\tget_fetch_map(remote_head, &refspec[i], &tail, 0);\n \n \t\t\t/* if --branch=tag, pull the requested tag explicitly */\n \t\t\tget_fetch_map(remote_head, tag_refspec, &tail, 0);\n \t\t}\n-\t} else\n-\t\tget_fetch_map(refs, refspec, &tail, 0);\n+\t} else {\n+\t\tunsigned int i;\n+\t\tfor (i = 0; i < refspec_count; i++)\n+\t\t\tget_fetch_map(refs, &refspec[i], &tail, 0);\n+\t}\n \n \tif (!option_mirror && !option_single_branch)\n \t\tget_fetch_map(refs, tag_refspec, &tail, 0);\n@@ -840,7 +845,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tint err = 0, complete_refs_before_fetch = 1;\n \n \tstruct refspec *refspec;\n-\tconst char *fetch_pattern;\n+\tunsigned int refspec_count = 1;\n+\tconst char **fetch_patterns;\n+\tconst struct string_list *config_fetch_patterns;\n \n \tpacket_trace_identity(\"clone\");\n \targc = parse_options(argc, argv, prefix, builtin_clone_options,\n@@ -967,9 +974,21 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tif (option_reference.nr)\n \t\tsetup_reference();\n \n-\tfetch_pattern = value.buf;\n-\trefspec = parse_fetch_refspec(1, &fetch_pattern);\n+\tstrbuf_addf(&key, \"remote.%s.fetch\", option_origin);\n+\tconfig_fetch_patterns = git_config_get_value_multi(key.buf);\n+\tif (config_fetch_patterns)\n+\t\trefspec_count = 1 + config_fetch_patterns->nr;\n+\tfetch_patterns = xcalloc(refspec_count, sizeof(*fetch_patterns));\n+\tfetch_patterns[0] = value.buf;\n+\tif (config_fetch_patterns) {\n+\t\tstruct string_list_item *fp;\n+\t\tunsigned int i = 1;\n+\t\tfor_each_string_list_item(fp, config_fetch_patterns)\n+\t\t\tfetch_patterns[i++] = fp->string;\n+\t}\n+\trefspec = parse_fetch_refspec(refspec_count, fetch_patterns);\n \n+\tstrbuf_reset(&key);\n \tstrbuf_reset(&value);\n \n \tremote = remote_get(option_origin);\n@@ -1013,7 +1032,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \trefs = transport_get_remote_refs(transport);\n \n \tif (refs) {\n-\t\tmapped_refs = wanted_peer_refs(refs, refspec);\n+\t\tmapped_refs = wanted_peer_refs(refs, refspec, refspec_count);\n \t\t/*\n \t\t * transport_get_remote_refs() may return refs with null sha-1\n \t\t * in mapped_refs (see struct transport->get_refs_list\n@@ -1094,6 +1113,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tstrbuf_release(&value);\n \tjunk_mode = JUNK_LEAVE_ALL;\n \n+\tfree(fetch_patterns);\n \tfree(refspec);\n \treturn err;\n }\ndiff --git a/t/t5708-clone-config.sh b/t/t5708-clone-config.sh\nindex 27d730c0a720..136a8611c7f3 100755\n--- a/t/t5708-clone-config.sh\n+++ b/t/t5708-clone-config.sh\n@@ -37,4 +37,28 @@ test_expect_success 'clone -c config is available during clone' '\n \ttest_cmp expect child/file\n '\n \n+test_expect_success 'clone -c remote.origin.fetch= works' '\n+\trm -rf child &&\n+\tgit update-ref refs/grab/it refs/heads/master &&\n+\tgit update-ref refs/keep/out refs/heads/master &&\n+\tgit clone -c \"remote.origin.fetch=+refs/grab/*:refs/grab/*\" . child &&\n+\t(\n+\t\tcd child &&\n+\t\tgit for-each-ref --format=\"%(refname)\" refs/grab/ >../actual\n+\t) &&\n+\techo refs/grab/it >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'git -c remote.origin.fetch= clone works' '\n+\trm -rf child &&\n+\tgit -c \"remote.origin.fetch=+refs/grab/*:refs/grab/*\" clone . child &&\n+\t(\n+\t\tcd child &&\n+\t\tgit for-each-ref --format=\"%(refname)\" refs/grab/ >../actual\n+\t) &&\n+\techo refs/grab/it >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.8.0.46.gb821760\n"},{"id":"282331","messageId":"xmqq1t6qr5c1.fsf@gitster.mtv.corp.google.com","threadId":"41634","inReplyTo":"1459349623-16443-1-git-send-email-szeder@ira.uka.de","subject":"Re: [PATCH v2] clone: respect configured fetch respecs during initial fetch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-31T16:15:58Z","receivedAt":"2016-03-31T16:15:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> Conceptually 'git clone' should behave as if the following commands\n> were run:\n>\n> git init\n> git config ... # set default configuration and origin remote\n> git fetch\n> git checkout # unless '--bare' is given\n>\n> However, that initial 'git fetch' behaves differently from any\n> subsequent fetches, because it takes only the default fetch refspec\n> into account and ignores all other fetch refspecs that might have\n> been explicitly specified on the command line (e.g. 'git -c\n> remote.origin.fetch= clone' or 'git clone -c ...').\n\nIs it really 'git fetch' behaves differently?\n\nWhat is missing in the above description is your expected behaviour\nof \"git -c var=val clone\", and without it we cannot tell if your\nexpectation is sane to begin with.\n\nIs the expectation like this?\n\n git init\n git config ... # set default configuration and origin remote\n git config var val # update with what \"-c var=val\" told us\n git fetch\n git checkout # unless '--bare' is given\n\nor is it something else?\n\nIs \"-c var=val\" adding to the config variables set by default, or is\nit replacing them? Does the choice depend on the nature of\nindividual variables, and if so what is the criteria?\n\nAre all \"-c var=val\" update the configuration of the resulting\nrepository? Or are certain \"var\"s treated as special and placed in\nthe config but not other \"var\"s? If the latter, what makes these\ncertain \"var\"s special?\n\nThese design decisions need to be explained so that they will serve\nto guide people to decide what other variables to propagate and how\nwhen they have to add new configuration variables in the future.\nOtherwise we'd end up with an inconsistent mess.\n\n> Check whether there are any fetch refspecs configured for the origin\n> remote and take all of them into account during the initial fetch as\n> well.\n>\n> Signed-off-by: SZEDER Gábor \n> ---\n> Changes since previous (RFC) version:\n> - new commit message\n> - additional configured fetch refspecs are taken into account with\n> '--single-branch' as well\n>\n> builtin/clone.c | 36 ++++++++++++++++++++++++++++--------\n> t/t5708-clone-config.sh | 24 ++++++++++++++++++++++++\n> 2 files changed, 52 insertions(+), 8 deletions(-)\n>\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index 661639255c56..5e2d2c21e456 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -515,7 +515,7 @@ static struct ref *find_remote_branch(const struct ref *refs, const char *branch\n> }\n> \n> static struct ref *wanted_peer_refs(const struct ref *refs,\n> -\t\tstruct refspec *refspec)\n> +\t\tstruct refspec *refspec, unsigned int refspec_count)\n> {\n> \tstruct ref *head = copy_ref(find_ref_by_name(refs, \"HEAD\"));\n> \tstruct ref *local_refs = head;\n> @@ -536,13 +536,18 @@ static struct ref *wanted_peer_refs(const struct ref *refs,\n> \t\t\twarning(_(\"Could not find remote branch %s to clone.\"),\n> \t\t\t\toption_branch);\n> \t\telse {\n> -\t\t\tget_fetch_map(remote_head, refspec, &tail, 0);\n> +\t\t\tunsigned int i;\n> +\t\t\tfor (i = 0; i < refspec_count; i++)\n> +\t\t\t\tget_fetch_map(remote_head, &refspec[i], &tail, 0);\n> \n> \t\t\t/* if --branch=tag, pull the requested tag explicitly */\n> \t\t\tget_fetch_map(remote_head, tag_refspec, &tail, 0);\n> \t\t}\n> -\t} else\n> -\t\tget_fetch_map(refs, refspec, &tail, 0);\n> +\t} else {\n> +\t\tunsigned int i;\n> +\t\tfor (i = 0; i < refspec_count; i++)\n> +\t\t\tget_fetch_map(refs, &refspec[i], &tail, 0);\n> +\t}\n> \n> \tif (!option_mirror && !option_single_branch)\n> \t\tget_fetch_map(refs, tag_refspec, &tail, 0);\n> @@ -840,7 +845,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n> \tint err = 0, complete_refs_before_fetch = 1;\n> \n> \tstruct refspec *refspec;\n> -\tconst char *fetch_pattern;\n> +\tunsigned int refspec_count = 1;\n> +\tconst char **fetch_patterns;\n> +\tconst struct string_list *config_fetch_patterns;\n> \n> \tpacket_trace_identity(\"clone\");\n> \targc = parse_options(argc, argv, prefix, builtin_clone_options,\n> @@ -967,9 +974,21 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n> \tif (option_reference.nr)\n> \t\tsetup_reference();\n> \n> -\tfetch_pattern = value.buf;\n> -\trefspec = parse_fetch_refspec(1, &fetch_pattern);\n> +\tstrbuf_addf(&key, \"remote.%s.fetch\", option_origin);\n> +\tconfig_fetch_patterns = git_config_get_value_multi(key.buf);\n> +\tif (config_fetch_patterns)\n> +\t\trefspec_count = 1 + config_fetch_patterns->nr;\n> +\tfetch_patterns = xcalloc(refspec_count, sizeof(*fetch_patterns));\n> +\tfetch_patterns[0] = value.buf;\n> +\tif (config_fetch_patterns) {\n> +\t\tstruct string_list_item *fp;\n> +\t\tunsigned int i = 1;\n> +\t\tfor_each_string_list_item(fp, config_fetch_patterns)\n> +\t\t\tfetch_patterns[i++] = fp->string;\n> +\t}\n> +\trefspec = parse_fetch_refspec(refspec_count, fetch_patterns);\n> \n> +\tstrbuf_reset(&key);\n> \tstrbuf_reset(&value);\n> \n> \tremote = remote_get(option_origin);\n> @@ -1013,7 +1032,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n> \trefs = transport_get_remote_refs(transport);\n> \n> \tif (refs) {\n> -\t\tmapped_refs = wanted_peer_refs(refs, refspec);\n> +\t\tmapped_refs = wanted_peer_refs(refs, refspec, refspec_count);\n> \t\t/*\n> \t\t * transport_get_remote_refs() may return refs with null sha-1\n> \t\t * in mapped_refs (see struct transport->get_refs_list\n> @@ -1094,6 +1113,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n> \tstrbuf_release(&value);\n> \tjunk_mode = JUNK_LEAVE_ALL;\n> \n> +\tfree(fetch_patterns);\n> \tfree(refspec);\n> \treturn err;\n> }\n> diff --git a/t/t5708-clone-config.sh b/t/t5708-clone-config.sh\n> index 27d730c0a720..136a8611c7f3 100755\n> --- a/t/t5708-clone-config.sh\n> +++ b/t/t5708-clone-config.sh\n> @@ -37,4 +37,28 @@ test_expect_success 'clone -c config is available during clone' '\n> \ttest_cmp expect child/file\n> '\n> \n> +test_expect_success 'clone -c remote.origin.fetch= works' '\n> +\trm -rf child &&\n> +\tgit update-ref refs/grab/it refs/heads/master &&\n> +\tgit update-ref refs/keep/out refs/heads/master &&\n> +\tgit clone -c \"remote.origin.fetch=+refs/grab/*:refs/grab/*\" . child &&\n> +\t(\n> +\t\tcd child &&\n> +\t\tgit for-each-ref --format=\"%(refname)\" refs/grab/ >../actual\n> +\t) &&\n> +\techo refs/grab/it >expect &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'git -c remote.origin.fetch= clone works' '\n> +\trm -rf child &&\n> +\tgit -c \"remote.origin.fetch=+refs/grab/*:refs/grab/*\" clone . child &&\n> +\t(\n> +\t\tcd child &&\n> +\t\tgit for-each-ref --format=\"%(refname)\" refs/grab/ >../actual\n> +\t) &&\n> +\techo refs/grab/it >expect &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_done\n"},{"id":"282369","messageId":"xmqq1t6qo4rd.fsf@gitster.mtv.corp.google.com","threadId":"41634","inReplyTo":"xmqq1t6qr5c1.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] clone: respect configured fetch respecs during initial fetch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-31T18:56:38Z","receivedAt":"2016-03-31T18:56:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Is the expectation like this?\n>\n> git init\n> git config ... # set default configuration and origin remote\n> git config var val # update with what \"-c var=val\" told us\n> git fetch\n> git checkout # unless '--bare' is given\n>\n> or is it something else?\n>\n> Is \"-c var=val\" adding to the config variables set by default, or is\n> it replacing them? Does the choice depend on the nature of\n> individual variables, and if so what is the criteria?\n>\n> Are all \"-c var=val\" update the configuration of the resulting\n> repository? Or are certain \"var\"s treated as special and placed in\n> the config but not other \"var\"s? If the latter, what makes these\n> certain \"var\"s special?\n>\n> These design decisions need to be explained so that they will serve\n> to guide people to decide what other variables to propagate and how\n> when they have to add new configuration variables in the future.\n> Otherwise we'd end up with an inconsistent mess.\n\nThe above did not start as rhetorical questions, but was merely me\nthinking aloud. However, it showed me a different approach might be\nmore appropriate.\n\nTaken as rhetorical questions, the sane answers to them would\nrevolve around \"we do not know the semantics of each and every\nconfiguration variable that will be given to this codepath, and by\ndefinition we will never know in advance the ones that will be\nintroduced later\". IOW, special casing -c remote.origin.fetch=spec\nis a bad idea.\n\nSo how about teaching \"git clone\" a new _option_ that is about what\nbranches are followed?\n\n\tgit clone $there --branches=\"master next pu\"\n\nwould give\n\n\t[remote \"origin\"]\n \tfetch = +refs/heads/master:refs/remotes/origin/master\n \tfetch = +refs/heads/next:refs/remotes/origin/next\n \tfetch = +refs/heads/pu:refs/remotes/origin/pu\n\ninstead of the usual\n\n\t[remote \"origin\"]\n\t\tfetch = +refs/heads/*:refs/remotes/origin/*\n\nAnd that can be made to work orthognonal to --single-branch by a\nsmall additional rule: if the branch given by -b (or their\nHEAD) is not part of --branches, then we add it to the set of\nbranches to be followed (i.e. if you give only --single-branch,\nwithout --branches, the set of branches to be followed will become\nthat single branch).\n\nHmm?\n"},{"id":"282389","messageId":"20160331225055.Horde.1X4mNM4L4J85rAeMFQzAEH_@webmail.informatik.kit.edu","threadId":"41634","inReplyTo":"xmqq1t6qo4rd.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] clone: respect configured fetch respecs during initial fetch","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2016-03-31T20:50:55Z","receivedAt":"2016-03-31T20:50:55Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"\nQuoting Junio C Hamano :\n\n> IOW, special casing -c remote.origin.fetch=spec\n> is a bad idea.\n\nI completely agree :)\nBut it's the other way around.\n\n'remote.origin.fetch=spec' during clone is special _now_, because the\ninitial fetch ignores it, no matter where it is set.\n\nMy patch makes it non-special, so that the initial fetch respects it,\nthe same way it already respects 'fetch.fsckObjects' and\n'fsck.unpackLimit', or the way the initial checkout respects e.g.\n'core.eol'.\n\n> So how about teaching \"git clone\" a new _option_ that is about what\n> branches are followed?\n>\n> \tgit clone $there --branches=\"master next pu\"\n>\n> would give\n>\n> \t[remote \"origin\"]\n> \tfetch = +refs/heads/master:refs/remotes/origin/master\n> \tfetch = +refs/heads/next:refs/remotes/origin/next\n> \tfetch = +refs/heads/pu:refs/remotes/origin/pu\n\nWithout my patch the initial fetch would ignore these refspecs, too.\n\n> instead of the usual\n>\n> \t[remote \"origin\"]\n> \t\tfetch = +refs/heads/*:refs/remotes/origin/*\n\nTyping only branch names is much shorter and simpler than typing the\nname of a config var and full refspecs, so this would be a nice\nsimplification of the UI for the case when the user is only\ninterested in certain branches. But it wouldn't help if the user\nwants to include 'refs/interesting/*' in the initial fetch.\n\n\n> And that can be made to work orthognonal to --single-branch by a\n> small additional rule: if the branch given by -b (or their\n> HEAD) is not part of --branches, then we add it to the set of\n> branches to be followed (i.e. if you give only --single-branch,\n> without --branches, the set of branches to be followed will become\n> that single branch).\n>\n> Hmm?\n"},{"id":"282405","messageId":"xmqqbn5ul12t.fsf@gitster.mtv.corp.google.com","threadId":"41634","inReplyTo":"20160331225055.Horde.1X4mNM4L4J85rAeMFQzAEH_@webmail.informatik.kit.edu","subject":"Re: [PATCH v2] clone: respect configured fetch respecs during initial fetch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-31T22:44:26Z","receivedAt":"2016-03-31T22:44:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> Quoting Junio C Hamano :\n>\n>> IOW, special casing -c remote.origin.fetch=spec\n>> is a bad idea.\n>\n> I completely agree :)\n> But it's the other way around.\n>\n> 'remote.origin.fetch=spec' during clone is special _now_, because the\n> initial fetch ignores it, no matter where it is set.\n>\n> My patch makes it non-special, so that the initial fetch respects it,\n> the same way it already respects 'fetch.fsckObjects' and\n> 'fsck.unpackLimit', or the way the initial checkout respects e.g.\n> 'core.eol'.\n\n... but does \"git -c core.eol clone\" leave that configuration in the\nresulting repository's .git/config? \"git -c user.name=foo\" for that\nmatter.\n\nThey may affect the one-shot operation but are not left in the\nresulting .git/config, which was what I was driving at. To make\nclone behave as if it is truly a short-hand of\n\n\tgit init\n\tgit config ;# with default and necessary tweaks to adjust\n\t\t ;# for things like -b, -o, --single-branch\n git fetch\n git checkout\n\nwhich by the way I think everybody agrees is a worth goal, then\nshouldn't the update to the current code be more like \"prepare the\ndefault config, tweak with whatever configuration necessary, and\nre-read the config before driving the equivalent of 'git fetch'?\"\n\nAnd the conclusion my rhetorical questions led to was that \"adjust\nfor things like...\" should not include what comes from \"-c var=val\"\nbecause there is no sensible way to incorporate them in general.\n\nThe most important point is that \"-c var=val\" is the wrong source of\ninformation to blindly propagete to the resulting .git/config. And\nthe point of \"--branches\" option is not that it would be short and\ntidy, but it is more targetted. With such an approach, nobody would\nimagine \"git -c random.var=value clone\" would be propagated into the\nresulting .git/config in a random and unspecified way.\n\nOnce you learn what custom set of refs the user wants to fetch, you\nwould need futzing of the refspecs like you did in your patch. That\npart of your patch is salvageable. The part that special cased the\ninformation that came from \"-c remote.origin.fetch\" while ignoring\nothers like user.name that came from exactly the same mechanism via\n\"-c user.name\" is not.\n"},{"id":"282418","messageId":"20160401023029.Horde.xfy5_x25TYi5be_HbswgxWI@webmail.informatik.kit.edu","threadId":"41634","inReplyTo":"xmqq1t6qr5c1.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] clone: respect configured fetch respecs during initial fetch","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2016-04-01T00:30:29Z","receivedAt":"2016-04-01T00:30:29Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"\nQuoting Junio C Hamano :\n\n> SZEDER Gábor writes:\n>\n>> Conceptually 'git clone' should behave as if the following commands\n>> were run:\n>>\n>> git init\n>> git config ... # set default configuration and origin remote\n>> git fetch\n>> git checkout # unless '--bare' is given\n>>\n>> However, that initial 'git fetch' behaves differently from any\n>> subsequent fetches, because it takes only the default fetch refspec\n>> into account and ignores all other fetch refspecs that might have\n>> been explicitly specified on the command line (e.g. 'git -c\n>> remote.origin.fetch= clone' or 'git clone -c ...').\n>\n> Is it really 'git fetch' behaves differently?\n\nCertainly.\n\n> What is missing in the above description is your expected behaviour\n> of \"git -c var=val clone\", and without it we cannot tell if your\n> expectation is sane to begin with.\n\nThese 'git -c var=val cmd' one-shot configuration parameters exist\nduring the lifespan of the command. Therefore, in case of 'git -c\nvar=val clone' they should exist while all the commands in our\nmental model are executed. IOW, those commands should behave as if\nthese configuration parameters were specified for them, see below.\n\n> Is the expectation like this?\n>\n> git init\n> git config ... # set default configuration and origin remote\n> git config var val # update with what \"-c var=val\" told us\n> git fetch\n> git checkout # unless '--bare' is given\n>\n> or is it something else?\n\nFor 'git -c var=val clone':\n\n git -c var=val init\n git -c var=val config ... # though this probably doesn't really\n # matter, as it is about writing the\n # configuration, and it gets the\n # to-be-written variables and values\n # in the \"...\" part anyway\n git -c var=val fetch\n git -c var=val checkout\n\nBeing one-shot configuration parameters, they shouldn't be written\nto the new repository's config file.\n\n'git clone -c var=val' is designed to be different:\n\n - it does write var=val into the new repository's config file\n\n - it specifies that var.val \"takes effect immediately after the\n repository is initialized, but before the remote history is\n fetched or any files checked out\".\n\nAdditionally, there may be relevant config variables defined in the\nglobal and system-wide config files, which of course should be\nrespected by all these commands.\n\nAnd it all works just fine as described above, even the initial fetch\nrespects most of the config variables, wherever specified, except for\nfetch refspecs which are completely ignored.\n\n> Is \"-c var=val\" adding to the config variables set by default, or is\n> it replacing them? Does the choice depend on the nature of\n> individual variables, and if so what is the criteria?\n\nIt's up to the individual command how it treats a particular config\nvariable: single-valued variables like 'fetch.fsckObjects' should\noverride (they already do), multi-valued variables like fetch refspecs\nshould be added.\nRunning as part of 'git clone' shouldn't matter at all.\n\nThis patch only affects how fetch refspecs are handled, the effects of\nother config variables are unchanged.\n\n> Are all \"-c var=val\" update the configuration of the resulting\n> repository? Or are certain \"var\"s treated as special and placed in\n> the config but not other \"var\"s? If the latter, what makes these\n> certain \"var\"s special?\n\nIn this regard it doesn't matter what 'val=var' is. What matters is\nhow the configuration parameter is specified (i.e. 'git -c var=val\nclone' vs. 'git clone -c var=val').\n\nThis patch doesn't change anything in this regard.\n"},{"id":"282428","messageId":"20160401062001.Horde.ONOWb-Prl7DzJnJKcoFoctm@webmail.informatik.kit.edu","threadId":"41634","inReplyTo":"xmqqbn5ul12t.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] clone: respect configured fetch respecs during initial fetch","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2016-04-01T04:20:01Z","receivedAt":"2016-04-01T04:20:01Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"\nQuoting Junio C Hamano :\n\n> SZEDER Gábor writes:\n>\n>> Quoting Junio C Hamano :\n>>\n>>> IOW, special casing -c remote.origin.fetch=spec\n>>> is a bad idea.\n>>\n>> I completely agree :)\n>> But it's the other way around.\n>>\n>> 'remote.origin.fetch=spec' during clone is special _now_, because the\n>> initial fetch ignores it, no matter where it is set.\n>>\n>> My patch makes it non-special, so that the initial fetch respects it,\n>> the same way it already respects 'fetch.fsckObjects' and\n>> 'fsck.unpackLimit', or the way the initial checkout respects e.g.\n>> 'core.eol'.\n>\n> ... but does \"git -c core.eol clone\" leave that configuration in the\n> resulting repository's .git/config? \"git -c user.name=foo\" for that\n> matter.\n\nNo, and it shouldn't.\n\n'git clone -c core.eol', however, should and indeed does.\n\n> They may affect the one-shot operation but are not left in the\n> resulting .git/config, which was what I was driving at. To make\n> clone behave as if it is truly a short-hand of\n>\n> \tgit init\n> \tgit config ;# with default and necessary tweaks to adjust\n> \t\t ;# for things like -b, -o, --single-branch\n> git fetch\n> git checkout\n>\n> which by the way I think everybody agrees is a worth goal, then\n> shouldn't the update to the current code be more like \"prepare the\n> default config, tweak with whatever configuration necessary, and\n> re-read the config before driving the equivalent of 'git fetch'?\"\n>\n> And the conclusion my rhetorical questions led to was that \"adjust\n> for things like...\" should not include what comes from \"-c var=val\"\n> because there is no sensible way to incorporate them in general.\n>\n> The most important point is that \"-c var=val\" is the wrong source of\n> information to blindly propagete to the resulting .git/config.\n\nIn case of 'git -c var=val clone', I agree, but then again, 'git clone\n-c var=val' was specifically designed to write that 'var=val' to the\nnew repository's config file.\n\nConfig variables set in the global or system-wide config files are not\nwritten to the new repository's config file either.\n\n> And\n> the point of \"--branches\" option is not that it would be short and\n> tidy, but it is more targetted. With such an approach, nobody would\n> imagine \"git -c random.var=value clone\" would be propagated into the\n> resulting .git/config in a random and unspecified way.\n>\n> Once you learn what custom set of refs the user wants to fetch, you\n> would need futzing of the refspecs like you did in your patch. That\n> part of your patch is salvageable. The part that special cased the\n> information that came from \"-c remote.origin.fetch\" while ignoring\n> others like user.name that came from exactly the same mechanism via\n> \"-c user.name\" is not.\n\nMy patch did not special case anything and it did not change anything\nwith respect to what config settings are written under what\ncircumstances to the new repository's config file.\nAll that already works properly and almost all those config settings\nare already taken into account when they should be by the commands in\nour conceptual model of 'git clone'. It doesn't matter at all where\nand how they were specified or whether they are written to the new\nrepository's config file or not, almost all of them are taken into\naccount.\n\nAlmost all, because there is that one exception: additional fetch\nrefspecs are ignored during the initial fetch.\n\nAnd all my patch did was to make the initial fetch aware of any\nadditional fetch refspecs which are configured when the initial\nfetch is executed. (and again: no matter where those refspecs were\nspecified and no matter whether they were written to the new config\nfile)\n\n\nEh, I guess I should have went for a refined version of the RFC's\ncommit message, rewriting it based on that conceptual modell caused\nway too much confusion.\n"}]}