{"thread":{"id":"45962","subject":"[PATCHv3 0/4] clone: respect configured fetch respecs during initial fetch","startedAt":"2017-05-15T11:06:18Z","lastAt":"2017-08-12T00:49:12Z","messageCount":48,"participants":["SZEDER Gábor","Jeff King","Junio C Hamano","Ævar Arnfjörð Bjarmason","Jonathan Nieder"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"319770","messageId":"20170515110557.11913-1-szeder.dev@gmail.com","threadId":"45962","inReplyTo":null,"subject":"[PATCHv3 0/4] clone: respect configured fetch respecs during initial fetch","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-05-15T11:05:53Z","receivedAt":"2017-05-15T11:06:18Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"This is a reroll of sg/clone-refspec-from-command-line-config.\nSorry for the delay, family visit.\n\nThe first patch is the updated version of what is now the first commit\nof that topic. The changes are those mentioned in [1]:\n\n - updated commit message,\n - renamed 'refspec_count' to 'refspec_nr',\n - use the parsed fetch refspecs returned by remote.c:remote_get()\n instead of parsing them ourselves (look at the third hunk of the\n diff of builtin/clone.c, how much shorter it looks),\n - modified tests to check that refs matching the default refspecs are\n transferred as well, and\n - added a test for the combination of '-c\n remote..fetch= --origin='.\n\nThe second patch is a doc update to warn users that not all\nconfiguration variables are supported via 'git clone -c ...' at the\nmoment.\n\nPatches 3 and 4 are the last two patches from Peff from this morning\n[2]. I picked those up, because his last patch required a bit of\nvariable name adjustments. I didn't pick up his first patch, because\nusing remote_get() already factors out refspec parsing.\n\n[1] - http://public-inbox.org/git/xmqq4lwu7r0s.fsf@gitster.mtv.corp.google.com/T/#u\n[2] - http://public-inbox.org/git/20170515074617.wsdzogshc4ilnlsb@sigill.intra.peff.net/T/#m021eadff5d1e4351d99a27096090be39f53df961\n\nJeff King (2):\n remote: drop free_refspecs() function\n clone: use free_refspec() to free refspec list\n\nSZEDER Gábor (2):\n clone: respect additional configured fetch refspecs during initial\n fetch\n Documentation/clone: document ignored configuration variables\n\n Documentation/git-clone.txt | 4 ++++\n builtin/clone.c | 22 ++++++++++++++++------\n remote.c | 28 ++++++----------------------\n t/t5611-clone-config.sh | 44 ++++++++++++++++++++++++++++++++++++++++++++\n 4 files changed, 70 insertions(+), 28 deletions(-)\n\n-- \n2.13.0.35.g14b6294b1\n\n"},{"id":"319771","messageId":"20170515110557.11913-3-szeder.dev@gmail.com","threadId":"45962","inReplyTo":"20170515110557.11913-1-szeder.dev@gmail.com","subject":"[PATCHv3 2/4] Documentation/clone: document ignored configuration variables","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-05-15T11:05:55Z","receivedAt":"2017-05-15T11:06:38Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Due to limitations/bugs in the current implementation, some\nconfiguration variables specified via 'git clone -c var=val' (or 'git\n-c var=val clone') are ignored during the initial fetch and checkout.\n\nLet the users know which configuration variables are known to be\nignored ('remote.origin.mirror' and 'remote.origin.tagOpt') under the\ndocumentation of 'git clone -c'.\n\nSigned-off-by: SZEDER Gábor \n---\n Documentation/git-clone.txt | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt\nindex ec41d3d69..4f1e7d4ba 100644\n--- a/Documentation/git-clone.txt\n+++ b/Documentation/git-clone.txt\n@@ -186,6 +186,10 @@ objects from the source repository into a pack in the cloned repository.\n \tvalues are given for the same key, each value will be written to\n \tthe config file. This makes it safe, for example, to add\n \tadditional fetch refspecs to the origin remote.\n+\tNote that due to limitations of the current implementation some\n+\tconfiguration variables don't take effect during the initial\n+\tfetch and checkout. Configuration variables known to not take\n+\teffect are: `remote..mirror` and `remote..tagOpt`.\n \n --depth ::\n \tCreate a 'shallow' clone with a history truncated to the\n-- \n2.13.0.35.g14b6294b1\n\n"},{"id":"319772","messageId":"20170515110557.11913-2-szeder.dev@gmail.com","threadId":"45962","inReplyTo":"20170515110557.11913-1-szeder.dev@gmail.com","subject":"[PATCHv3 1/4] clone: respect additional configured fetch refspecs during initial fetch","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-05-15T11:05:54Z","receivedAt":"2017-05-15T11:06:40Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The initial fetch during a clone doesn't transfer refs matching\nadditional fetch refspecs given on the command line as configuration\nvariables. This contradicts to the documentation stating that\nconfiguration variables specified via 'git clone -c = ...'\n\"take effect immediately after the repository is initialized, but\nbefore the remote history is fetched\" and the given example\nspecifically mentions \"adding additional fetch refspecs to the origin\nremote\". Furthermore, one-shot configuration variables specified via\n'git -c = clone ...', though not written to the newly\ncreated repository's config file, live during the lifetime of the\n'clone' command, including the initial fetch. This implies that any\nfetch refspecs specified this way should already be taken into account\nduring the initial fetch.\n\nThe reason is that the initial fetch is not a fully fledged 'git\nfetch' but a bunch of direct calls into the fetch/transport machinery,\nbypassing parts of 'git fetch' that processes configured fetch\nrefspecs. The configured refspecs are, however, read and parsed\nproperly when clone calls remote.c:remote_get(), but it never looks at\nthe parsed refspecs in the resulting 'struct remote'.\n\nModify clone to take the configured fetch refspecs into account to\nretrieve all matching refs during the initial fetch. Note that the\nconfiguration at that point only includes the fetch refspecs specified\nby the user, but it doesn't include the default fetch refspec, so we\nhave to append it manually at the end of the parsed refspecs array.\n\nAdd tests to check that refspecs given both via 'git clone -c ...' and\n'git -c ... clone' retrieve all refs matching either the default or\nthe additional refspecs, and that it works even when the user\nspecifies the remote name via '--origin='.\n\nSigned-off-by: SZEDER Gábor \n---\n builtin/clone.c | 20 +++++++++++++++-----\n t/t5611-clone-config.sh | 44 ++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 59 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex a35d62293..4144190da 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -520,7 +520,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_nr)\n {\n \tstruct ref *head = copy_ref(find_ref_by_name(refs, \"HEAD\"));\n \tstruct ref *local_refs = head;\n@@ -541,13 +541,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_nr; 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_nr; 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@@ -989,6 +994,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tstrbuf_reset(&value);\n \n \tremote = remote_get(option_origin);\n+\tREALLOC_ARRAY(remote->fetch, remote->fetch_refspec_nr + 1);\n+\tmemcpy(remote->fetch+remote->fetch_refspec_nr, refspec,\n+\t sizeof(*refspec));\n+\n \ttransport = transport_get(remote, remote->url[0]);\n \ttransport_set_verbosity(transport, option_verbosity, option_progress);\n \ttransport->family = family;\n@@ -1029,7 +1038,8 @@ 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, remote->fetch,\n+\t\t\t\t\t remote->fetch_refspec_nr + 1);\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\ndiff --git a/t/t5611-clone-config.sh b/t/t5611-clone-config.sh\nindex e4850b778..114b53920 100755\n--- a/t/t5611-clone-config.sh\n+++ b/t/t5611-clone-config.sh\n@@ -37,6 +37,50 @@ 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/leave/out refs/heads/master &&\n+\tgit clone -c \"remote.origin.fetch=+refs/grab/*:refs/grab/*\" . child &&\n+\tgit -C child for-each-ref --format=\"%(refname)\" >actual &&\n+\tcat >expect <<-EOF &&\n+\trefs/grab/it\n+\trefs/heads/master\n+\trefs/remotes/origin/HEAD\n+\trefs/remotes/origin/master\n+\tEOF\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+\tgit -C child for-each-ref --format=\"%(refname)\" >actual &&\n+\tcat >expect <<-EOF &&\n+\trefs/grab/it\n+\trefs/heads/master\n+\trefs/remotes/origin/HEAD\n+\trefs/remotes/origin/master\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'clone -c remote..fetch= --origin=' '\n+\trm -rf child &&\n+\tgit clone --origin=upstream \\\n+\t\t-c \"remote.upstream.fetch=+refs/grab/*:refs/grab/*\" \\\n+\t\t-c \"remote.origin.fetch=+refs/leave/*:refs/leave/*\" \\\n+\t\t. child &&\n+\tgit -C child for-each-ref --format=\"%(refname)\" >actual &&\n+\tcat >expect <<-EOF &&\n+\trefs/grab/it\n+\trefs/heads/master\n+\trefs/remotes/upstream/HEAD\n+\trefs/remotes/upstream/master\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n # Tests for the hidden file attribute on windows\n is_hidden () {\n \t# Use the output of `attrib`, ignore the absolute path\n-- \n2.13.0.35.g14b6294b1\n\n"},{"id":"319773","messageId":"20170515110557.11913-4-szeder.dev@gmail.com","threadId":"45962","inReplyTo":"20170515110557.11913-1-szeder.dev@gmail.com","subject":"[PATCHv3 3/4] remote: drop free_refspecs() function","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-05-15T11:05:56Z","receivedAt":"2017-05-15T11:06:49Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"From: Jeff King \n\nWe already have free_refspec(), a public function which does\nthe same thing as the static free_refspecs(). Let's just\nkeep one. There are two minor differences between the\nfunctions:\n\n 1. free_refspecs() is a noop when the refspec argument is\n NULL. This probably doesn't matter in practice. The\n nr_refspec parameter would presumably be 0 in that\n case, skipping the loop. And free(NULL) is explicitly\n OK. But it doesn't hurt for us to port this extra\n safety to free_refspec(), as one of the callers passes\n a funny \"i+1\" count.\n\n 2. The order of arguments is reversed between the two\n functions. This patch uses the already-public order of\n free_refspec(), as it matches the argument order on the\n parsing side.\n\nSigned-off-by: Jeff King \n---\n remote.c | 28 ++++++----------------------\n 1 file changed, 6 insertions(+), 22 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex ad6c5424e..7918e0dac 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -473,26 +473,6 @@ static void read_config(void)\n \talias_all_urls();\n }\n \n-/*\n- * This function frees a refspec array.\n- * Warning: code paths should be checked to ensure that the src\n- * and dst pointers are always freeable pointers as well\n- * as the refspec pointer itself.\n- */\n-static void free_refspecs(struct refspec *refspec, int nr_refspec)\n-{\n-\tint i;\n-\n-\tif (!refspec)\n-\t\treturn;\n-\n-\tfor (i = 0; i < nr_refspec; i++) {\n-\t\tfree(refspec[i].src);\n-\t\tfree(refspec[i].dst);\n-\t}\n-\tfree(refspec);\n-}\n-\n static struct refspec *parse_refspec_internal(int nr_refspec, const char **refspec, int fetch, int verify)\n {\n \tint i;\n@@ -606,7 +586,7 @@ static struct refspec *parse_refspec_internal(int nr_refspec, const char **refsp\n \t\t * since it is only possible to reach this point from within\n \t\t * the for loop above.\n \t\t */\n-\t\tfree_refspecs(rs, i+1);\n+\t\tfree_refspec(i+1, rs);\n \t\treturn NULL;\n \t}\n \tdie(\"Invalid refspec '%s'\", refspec[i]);\n@@ -617,7 +597,7 @@ int valid_fetch_refspec(const char *fetch_refspec_str)\n \tstruct refspec *refspec;\n \n \trefspec = parse_refspec_internal(1, &fetch_refspec_str, 1, 1);\n-\tfree_refspecs(refspec, 1);\n+\tfree_refspec(1, refspec);\n \treturn !!refspec;\n }\n \n@@ -634,6 +614,10 @@ static struct refspec *parse_push_refspec(int nr_refspec, const char **refspec)\n void free_refspec(int nr_refspec, struct refspec *refspec)\n {\n \tint i;\n+\n+\tif (!refspec)\n+\t\treturn;\n+\n \tfor (i = 0; i < nr_refspec; i++) {\n \t\tfree(refspec[i].src);\n \t\tfree(refspec[i].dst);\n-- \n2.13.0.35.g14b6294b1\n\n"},{"id":"319774","messageId":"20170515110557.11913-5-szeder.dev@gmail.com","threadId":"45962","inReplyTo":"20170515110557.11913-1-szeder.dev@gmail.com","subject":"[PATCHv3 4/4] clone: use free_refspec() to free refspec list","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-05-15T11:05:57Z","receivedAt":"2017-05-15T11:06:52Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"From: Jeff King \n\nUsing free() on a refspec was always leaky, as its string\nfields also need freed. But it became more so when ad00f128d\n(clone: respect configured fetch respecs during initial\nfetch, 2016-03-30) taught clone to create a list of\nrefspecs, each of which need to be freed.\n\n[sg: adjusted the function parameters.]\n\nSigned-off-by: Jeff King \nSigned-off-by: SZEDER Gábor \n---\n builtin/clone.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 4144190da..4bf28d7f5 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -1120,6 +1120,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tstrbuf_release(&value);\n \tjunk_mode = JUNK_LEAVE_ALL;\n \n-\tfree(refspec);\n+\tfree_refspec(remote->fetch_refspec_nr + 1, remote->fetch);\n \treturn err;\n }\n-- \n2.13.0.35.g14b6294b1\n\n"},{"id":"319775","messageId":"CAM0VKjkr+EBEErQZxW-prrcdAyF4D-icU_ao9vvO3yJ1HWhmhw@mail.gmail.com","threadId":"45962","inReplyTo":"20170515110557.11913-5-szeder.dev@gmail.com","subject":"Re: [PATCHv3 4/4] clone: use free_refspec() to free refspec list","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-05-15T11:29:07Z","receivedAt":"2017-05-15T11:29:13Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, May 15, 2017 at 1:05 PM, SZEDER Gábor wrote:\n> From: Jeff King \n>\n> Using free() on a refspec was always leaky, as its string\n> fields also need freed. But it became more so when ad00f128d\n> (clone: respect configured fetch respecs during initial\n> fetch, 2016-03-30) taught clone to create a list of\n> refspecs, each of which need to be freed.\n>\n> [sg: adjusted the function parameters.]\n>\n> Signed-off-by: Jeff King \n> Signed-off-by: SZEDER Gábor \n> ---\n> builtin/clone.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index 4144190da..4bf28d7f5 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -1120,6 +1120,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n> strbuf_release(&value);\n> junk_mode = JUNK_LEAVE_ALL;\n>\n> - free(refspec);\n> + free_refspec(remote->fetch_refspec_nr + 1, remote->fetch);\n> return err;\n> }\n\nErm... I should have given a bit more thought to this last patch,\nshouldn't I?\n\nFirst, the unchanged commit message is now (i.e. by using the parsed\nrefspecs returned by remote_get()) completely outdated.\nSecond, while it properly frees those refspecs, i.e. the array and all\nits string fields, it will now leak the memory pointed by the\n'refspec' variable. However, why free just that one field of the\n'struct *remote'? Alas, we don't seem to have a free_remote()\nfunction...\n"},{"id":"319847","messageId":"20170515224615.f6hnnfngwpierqk4@sigill.intra.peff.net","threadId":"45962","inReplyTo":"20170515110557.11913-1-szeder.dev@gmail.com","subject":"Re: [PATCHv3 0/4] clone: respect configured fetch respecs during initial fetch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-15T22:46:15Z","receivedAt":"2017-05-15T22:46:23Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 15, 2017 at 01:05:53PM +0200, SZEDER Gábor wrote:\n\n> This is a reroll of sg/clone-refspec-from-command-line-config.\n> Sorry for the delay, family visit.\n\nNo problem. Thanks for letting us know before it went to 'next'. ;)\n\n> The first patch is the updated version of what is now the first commit\n> of that topic. The changes are those mentioned in [1]:\n> \n> - updated commit message,\n> - renamed 'refspec_count' to 'refspec_nr',\n\nGood.\n\n> - use the parsed fetch refspecs returned by remote.c:remote_get()\n> instead of parsing them ourselves (look at the third hunk of the\n> diff of builtin/clone.c, how much shorter it looks),\n\nYeah, that is much nicer. It does feel a little dirty modifying\nremote->fetch, though. I'll comment on the specific patch.\n\n> - modified tests to check that refs matching the default refspecs are\n> transferred as well, and\n> - added a test for the combination of '-c\n> remote..fetch= --origin='.\n\nSounds good.\n\n> The second patch is a doc update to warn users that not all\n> configuration variables are supported via 'git clone -c ...' at the\n> moment.\n\nGood idea.\n\n> Patches 3 and 4 are the last two patches from Peff from this morning\n> [2]. I picked those up, because his last patch required a bit of\n> variable name adjustments. I didn't pick up his first patch, because\n> using remote_get() already factors out refspec parsing.\n\nMakes sense. Thanks for including them.\n\n-Peff\n"},{"id":"319848","messageId":"20170515230727.hw75whugf25asuor@sigill.intra.peff.net","threadId":"45962","inReplyTo":"20170515110557.11913-2-szeder.dev@gmail.com","subject":"Re: [PATCHv3 1/4] clone: respect additional configured fetch refspecs during initial fetch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-15T23:07:27Z","receivedAt":"2017-05-15T23:07:38Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 15, 2017 at 01:05:54PM +0200, SZEDER Gábor wrote:\n\n> The initial fetch during a clone doesn't transfer refs matching\n> additional fetch refspecs given on the command line as configuration\n> variables. This contradicts to the documentation stating that\n\nMinor gramm-o: s/to the/the/\n\n> @@ -989,6 +994,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n> \tstrbuf_reset(&value);\n> \n> \tremote = remote_get(option_origin);\n> +\tREALLOC_ARRAY(remote->fetch, remote->fetch_refspec_nr + 1);\n> +\tmemcpy(remote->fetch+remote->fetch_refspec_nr, refspec,\n> +\t sizeof(*refspec));\n\nHere we append to remote->fetch. We are assuming then that\nremote->fetch_refspec has already been parsed into remote->fetch. Which\nI think it always is by remote_get(), but given that it lazy-parses in\nsome cases, it feels a little dangerous.\n\nI also notice that you don't touch remote->fetch_refspec_nr, nor\nfetch_refspec_alloc. So the remote struct doesn't actually know about\nthis entry. It would probably be wrong if you _did_ update them,\nbecause remote->fetch_refspec (the list of refspec strings) would not\nhave a matching entry, and would potentially access uninitialized\nmemory.\n\nI think the whole thing would be a lot less messy if \"struct remote\" let\nyou add a new refspec (as a string) after the initial parse, and it\nwould handle the details. Just making the existing add_fetch_refspec()\npublic isn't quite enough, because you'd need to invalidate and re-parse\nthe matching \"fetch\" array, too. Something like:\n\ndiff --git a/remote.c b/remote.c\nindex 9c8912ab1..0881ed32c 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -2319,3 +2319,17 @@ void apply_push_cas(struct push_cas_option *cas,\n \tfor (ref = remote_refs; ref; ref = ref->next)\n \t\tapply_cas(cas, remote, ref);\n }\n+\n+void remote_add_fetch_refspec(struct remote *remote, const char *refspec)\n+{\n+\tadd_fetch_refspec(remote, refspec);\n+\tif (remote->fetch) {\n+\t\tstruct refspec *parsed;\n+\n+\t\tparsed = parse_refspec_internal(1, &refspec, 1, 0);\n+\t\tREALLOC_ARRAY(remote->fetch, remote->fetch_refspec_nr);\n+\t\tremote->fetch[remote->fetch_refspec_nr - 1] = *parsed;\n+\t\t/* Not free_refspec, as we copied its pointers above */\n+\t\tfree(parsed);\n+\t}\n+}\n\nThat feels a bit dirty, too, but at least it's not reaching across\nmodule boundaries. I think the cleanest thing would be to actually add\nit to the config before calling remote_get().\n\nI think in the earlier discussion you mentioned there are some ordering\nproblems with writing out the new on-disk config. But could we add it to\nthe temporary environment, like:\n\n strbuf_addf(&key, \"remote.%s.fetch=%s\", option_origin, refspec_pattern);\n git_config_push_parameter(key.buf);\n\n?\n\nCome to think of it, though, I thought the reason we weren't using\nremote_get() in the first place is that some code paths (like\nsingle-branch) needed to actually get the remote ref list before we knew\nthe refspec? So how does this approach work at all? :)\n\nI guess that doesn't apply here. We always feed the transport code with\na broad refspec, and then narrow it down later. It's only that we can't\nwrite the final config to disk until we've computed the correct branch\nbased on the remote refs. gotten the branch.\n\nIf all that's correct, then I think the push_parameter() thing would\nwork. It does feel like a round-a-bout way to solve the problem, but\nit's at least manipulating solid, public APIs.\n\n-Peff\n"},{"id":"319850","messageId":"20170515231009.2tvukcbmfywnqlaw@sigill.intra.peff.net","threadId":"45962","inReplyTo":"CAM0VKjkr+EBEErQZxW-prrcdAyF4D-icU_ao9vvO3yJ1HWhmhw@mail.gmail.com","subject":"Re: [PATCHv3 4/4] clone: use free_refspec() to free refspec list","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-15T23:10:10Z","receivedAt":"2017-05-15T23:10:16Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 15, 2017 at 01:29:07PM +0200, SZEDER Gábor wrote:\n\n> On Mon, May 15, 2017 at 1:05 PM, SZEDER Gábor wrote:\n> > From: Jeff King \n> >\n> > Using free() on a refspec was always leaky, as its string\n> > fields also need freed. But it became more so when ad00f128d\n> > (clone: respect configured fetch respecs during initial\n> > fetch, 2016-03-30) taught clone to create a list of\n> > refspecs, each of which need to be freed.\n> >\n> > [sg: adjusted the function parameters.]\n> >\n> > Signed-off-by: Jeff King \n> > Signed-off-by: SZEDER Gábor \n> > ---\n> > builtin/clone.c | 2 +-\n> > 1 file changed, 1 insertion(+), 1 deletion(-)\n> >\n> > diff --git a/builtin/clone.c b/builtin/clone.c\n> > index 4144190da..4bf28d7f5 100644\n> > --- a/builtin/clone.c\n> > +++ b/builtin/clone.c\n> > @@ -1120,6 +1120,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n> > strbuf_release(&value);\n> > junk_mode = JUNK_LEAVE_ALL;\n> >\n> > - free(refspec);\n> > + free_refspec(remote->fetch_refspec_nr + 1, remote->fetch);\n> > return err;\n> > }\n> \n> Erm... I should have given a bit more thought to this last patch,\n> shouldn't I?\n> \n> First, the unchanged commit message is now (i.e. by using the parsed\n> refspecs returned by remote_get()) completely outdated.\n> Second, while it properly frees those refspecs, i.e. the array and all\n> its string fields, it will now leak the memory pointed by the\n> 'refspec' variable. However, why free just that one field of the\n> 'struct *remote'? Alas, we don't seem to have a free_remote()\n> function...\n\nRight, the \"struct remote\" fields are meant to last the program\nlifetime, and just go away when the program does. So they are never\nfreed. We could just lump this in with them, and remove the free()\nentirely.\n\nIt's a little funny with your patch as-is, because \"struct remote\"\ndoesn't actually know about the refspec we stuck on the end. But since\nit doesn't free its refspecs anyway, it's not any more leaky than all\nthe other entries.\n\n-Peff\n"},{"id":"320527","messageId":"xmqqa864kops.fsf@gitster.mtv.corp.google.com","threadId":"45962","inReplyTo":"CAM0VKjkr+EBEErQZxW-prrcdAyF4D-icU_ao9vvO3yJ1HWhmhw@mail.gmail.com","subject":"Re: [PATCHv3 4/4] clone: use free_refspec() to free refspec list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-23T07:38:07Z","receivedAt":"2017-05-23T07:38:15Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> On Mon, May 15, 2017 at 1:05 PM, SZEDER Gábor wrote:\n>> From: Jeff King \n>>\n>> Using free() on a refspec was always leaky, as its string\n>> fields also need freed. But it became more so when ad00f128d\n>> (clone: respect configured fetch respecs during initial\n>> fetch, 2016-03-30) taught clone to create a list of\n>> refspecs, each of which need to be freed.\n>>\n>> [sg: adjusted the function parameters.]\n>>\n>> Signed-off-by: Jeff King \n>> Signed-off-by: SZEDER Gábor \n>> ---\n>> builtin/clone.c | 2 +-\n>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>>\n>> diff --git a/builtin/clone.c b/builtin/clone.c\n>> index 4144190da..4bf28d7f5 100644\n>> --- a/builtin/clone.c\n>> +++ b/builtin/clone.c\n>> @@ -1120,6 +1120,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n>> strbuf_release(&value);\n>> junk_mode = JUNK_LEAVE_ALL;\n>>\n>> - free(refspec);\n>> + free_refspec(remote->fetch_refspec_nr + 1, remote->fetch);\n>> return err;\n>> }\n>\n> Erm... I should have given a bit more thought to this last patch,\n> shouldn't I?\n>\n> First, the unchanged commit message is now (i.e. by using the parsed\n> refspecs returned by remote_get()) completely outdated.\n> Second, while it properly frees those refspecs, i.e. the array and all\n> its string fields, it will now leak the memory pointed by the\n> 'refspec' variable. However, why free just that one field of the\n> 'struct *remote'? Alas, we don't seem to have a free_remote()\n> function...\n\nI was sifting entries in the draft \"What's cooking\" report to find\ntopics to merge to 'next'. I read the series over and as Peff said\nin his <20170515224615.f6hnnfngwpierqk4@sigill.intra.peff.net>, I\nthink the series overall is good.\n\nDo you want to update the proposed log message for this one before\nthe entire thing is merged to 'next'?\n"},{"id":"320550","messageId":"20170523112734.ftjdrgzesmfcau2r@sigill.intra.peff.net","threadId":"45962","inReplyTo":"xmqqa864kops.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv3 4/4] clone: use free_refspec() to free refspec list","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-23T11:27:35Z","receivedAt":"2017-05-23T11:27:44Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 23, 2017 at 04:38:07PM +0900, Junio C Hamano wrote:\n\n> > First, the unchanged commit message is now (i.e. by using the parsed\n> > refspecs returned by remote_get()) completely outdated.\n> > Second, while it properly frees those refspecs, i.e. the array and all\n> > its string fields, it will now leak the memory pointed by the\n> > 'refspec' variable. However, why free just that one field of the\n> > 'struct *remote'? Alas, we don't seem to have a free_remote()\n> > function...\n> \n> I was sifting entries in the draft \"What's cooking\" report to find\n> topics to merge to 'next'. I read the series over and as Peff said\n> in his <20170515224615.f6hnnfngwpierqk4@sigill.intra.peff.net>, I\n> think the series overall is good.\n> \n> Do you want to update the proposed log message for this one before\n> the entire thing is merged to 'next'?\n\nI didn't quite say that it was good. I think the overall direction is\ngood, but I had some comments on patch 1. Maybe we want to accept the\nugliness there, but I had some suggestions that might make it less bad.\n\n-Peff\n"},{"id":"320552","messageId":"CAM0VKjm5m9sTJtyAq_4O6RWJQ71v1c-sD+ARei=57rWwijMHpw@mail.gmail.com","threadId":"45962","inReplyTo":"xmqqa864kops.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv3 4/4] clone: use free_refspec() to free refspec list","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-05-23T12:06:09Z","receivedAt":"2017-05-23T12:06:16Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, May 23, 2017 at 9:38 AM, Junio C Hamano wrote:\n\n> I was sifting entries in the draft \"What's cooking\" report to find\n> topics to merge to 'next'. I read the series over and as Peff said\n> in his <20170515224615.f6hnnfngwpierqk4@sigill.intra.peff.net>, I\n> think the series overall is good.\n>\n> Do you want to update the proposed log message for this one before\n> the entire thing is merged to 'next'?\n\nI followed Peff's advice on adding the default fetch refspec to this\ntemporary configuration environment (what's the proper term for\nthat?), which resulted in the simplest and cleanest code yet, and, as\na by product made this last patch moot, so I will drop it in the next\nversion. The code is ready, but the change requires updates to the\nfirst patch's commit message, maybe I can get around to it in the\nevening.\n"},{"id":"320813","messageId":"20170526100403.19270-1-szeder.dev@gmail.com","threadId":"45962","inReplyTo":"20170515230727.hw75whugf25asuor@sigill.intra.peff.net","subject":"Re: [PATCHv3 1/4] clone: respect additional configured fetch refspecs during initial fetch","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-05-26T10:04:03Z","receivedAt":"2017-05-26T10:04:23Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, May 16, 2017 at 1:07 AM, Jeff King wrote:\n>> @@ -989,6 +994,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n>> strbuf_reset(&value);\n>>\n>> remote = remote_get(option_origin);\n>> + REALLOC_ARRAY(remote->fetch, remote->fetch_refspec_nr + 1);\n>> + memcpy(remote->fetch+remote->fetch_refspec_nr, refspec,\n>> + sizeof(*refspec));\n>\n> Here we append to remote->fetch. We are assuming then that\n> remote->fetch_refspec has already been parsed into remote->fetch. Which\n> I think it always is by remote_get(),\n\nRight.\n\n> but given that it lazy-parses in\n> some cases, it feels a little dangerous.\n\nI'm not sure about lazy parsing.\nremote_get() returns a fully-parsed, cached struct remote instance\nwithout re-reading the configuration, so all fields directly\ncorresponding to configuration variables stay the same. However, it\ndoes parse fetch and push refspecs on every invocation. So if it were\nto be called to return the origin remote more than once during\ncloning, then the default refspec would get lost on subsequent\ninvocations. Is this what you meant with dangerous?\n\n(Sidenote: and it would leak some memory, too, because it re-parses\nthe refspecs without free()ing the results of the previous\ninvocation.)\n\nYour proposed function to add a refspec as a string would eliminate\nthis danger.\n\n> I think in the earlier discussion you mentioned there are some\n> ordering\n> problems with writing out the new on-disk config. But could we add\n> it to\n> the temporary environment, like:\n>\n> strbuf_addf(&key, \"remote.%s.fetch=%s\", option_origin,\n> refspec_pattern);\n> git_config_push_parameter(key.buf);\n>\n> ?\n\n> If all that's correct, then I think the push_parameter() thing would\n> work. It does feel like a round-a-bout way to solve the problem, but\n> it's at least manipulating solid, public APIs.\n\nIt certainly looks better, see the patch below the scissors for\nreference, and I thought it works because until last night I only run\nthe corresponding test script (t5611-clone-config), though I know very\nwell that \"Thou shalt always run the full test suite!\" :)\n\nUnfortunately, putting the default refspec into this temporary\nconfiguration environment breaks a few submodule tests\n(t5614-clone-submodules or t5614-clone-submodules-shallow (it's got\nrenamed between this topic and master), t7407-submodule-foreach,\nt7410-submodule-checkout-to), because it \"leaks\" to the submodule\nenvironment.\n\n\n -- >8 --\n\nSubject: [PATCH] clone: respect additional configured fetch refspecs during\n initial fetch\n\nThe initial fetch during a clone doesn't transfer refs matching\nadditional fetch refspecs given on the command line as configuration\nvariables. This contradicts the documentation stating that\nconfiguration variables specified via 'git clone -c = ...'\n\"take effect immediately after the repository is initialized, but\nbefore the remote history is fetched\" and the given example\nspecifically mentions \"adding additional fetch refspecs to the origin\nremote\". Furthermore, one-shot configuration variables specified via\n'git -c = clone ...', though not written to the newly\ncreated repository's config file, live during the lifetime of the\n'clone' command, including the initial fetch. All this implies that\nany fetch refspecs specified this way should already be taken into\naccount during the initial fetch.\n\nThe reason for this is that the initial fetch is not a fully fledged\n'git fetch' but a bunch of direct calls into the fetch/transport\nmachinery with clone's own refs-to-refspec matching logic, which\nbypasses parts of 'git fetch' processing configured fetch refspecs.\nThis logic only considers a single default refspec, potentially\ninfluenced by options like '--single-branch' and '--mirror'. The\nconfigured refspecs are, however, already read and parsed properly\nwhen clone calls remote.c:remote_get(), but it never looks at the\nparsed refspecs in the resulting 'struct remote'.\n\nModify clone to take the configured fetch refspecs into account to\nretrieve all matching refs during the initial fetch. Note that the\nconfiguration at that point only includes the fetch refspecs specified\nby the user, but it doesn't include the default fetch refspec.\nTo keep the code simple and parsing and memory management of the\nrefspecs in one place, add the default fetch refspec to the temporary\nconfiguration environment, so remote_get() can parse it along with all\nother refspecs that might have been specified on the command line.\n\nAdd tests to check that refspecs given both via 'git clone -c ...' and\n'git -c ... clone' retrieve all refs matching either the default or\nthe additional refspecs, and that it works even when the user\nspecifies an alternative remote name via '--origin='.\n\nSigned-off-by: SZEDER Gábor \n---\n builtin/clone.c | 32 ++++++++++++++++----------------\n t/t5611-clone-config.sh | 44 ++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 60 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex a35d62293..40f4a327b 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -520,7 +520,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_nr)\n {\n \tstruct ref *head = copy_ref(find_ref_by_name(refs, \"HEAD\"));\n \tstruct ref *local_refs = head;\n@@ -541,13 +541,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_nr; 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_nr; 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@@ -848,16 +853,13 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tconst struct ref *our_head_points_at;\n \tstruct ref *mapped_refs;\n \tconst struct ref *ref;\n-\tstruct strbuf key = STRBUF_INIT, value = STRBUF_INIT;\n+\tstruct strbuf key = STRBUF_INIT, default_refspec_config = STRBUF_INIT;\n \tstruct strbuf branch_top = STRBUF_INIT, reflog_msg = STRBUF_INIT;\n \tstruct transport *transport = NULL;\n \tconst char *src_ref_prefix = \"refs/heads/\";\n \tstruct remote *remote;\n \tint err = 0, complete_refs_before_fetch = 1;\n \n-\tstruct refspec *refspec;\n-\tconst char *fetch_pattern;\n-\n \tpacket_trace_identity(\"clone\");\n \targc = parse_options(argc, argv, prefix, builtin_clone_options,\n \t\t\t builtin_clone_usage, 0);\n@@ -975,7 +977,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\tstrbuf_addf(&branch_top, \"refs/remotes/%s/\", option_origin);\n \t}\n \n-\tstrbuf_addf(&value, \"+%s*:%s*\", src_ref_prefix, branch_top.buf);\n \tstrbuf_addf(&key, \"remote.%s.url\", option_origin);\n \tgit_config_set(key.buf, repo);\n \tstrbuf_reset(&key);\n@@ -983,10 +984,9 @@ 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-\n-\tstrbuf_reset(&value);\n+\tstrbuf_addf(&default_refspec_config, \"remote.%s.fetch=+%s*:%s*\",\n+\t\t option_origin, src_ref_prefix, branch_top.buf);\n+\tgit_config_push_parameter(default_refspec_config.buf);\n \n \tremote = remote_get(option_origin);\n \ttransport = transport_get(remote, remote->url[0]);\n@@ -1029,7 +1029,8 @@ 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, remote->fetch,\n+\t\t\t\t\t remote->fetch_refspec_nr);\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@@ -1107,9 +1108,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tstrbuf_release(&reflog_msg);\n \tstrbuf_release(&branch_top);\n \tstrbuf_release(&key);\n-\tstrbuf_release(&value);\n+\tstrbuf_release(&default_refspec_config);\n \tjunk_mode = JUNK_LEAVE_ALL;\n \n-\tfree(refspec);\n \treturn err;\n }\ndiff --git a/t/t5611-clone-config.sh b/t/t5611-clone-config.sh\nindex e4850b778..114b53920 100755\n--- a/t/t5611-clone-config.sh\n+++ b/t/t5611-clone-config.sh\n@@ -37,6 +37,50 @@ 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/leave/out refs/heads/master &&\n+\tgit clone -c \"remote.origin.fetch=+refs/grab/*:refs/grab/*\" . child &&\n+\tgit -C child for-each-ref --format=\"%(refname)\" >actual &&\n+\tcat >expect <<-EOF &&\n+\trefs/grab/it\n+\trefs/heads/master\n+\trefs/remotes/origin/HEAD\n+\trefs/remotes/origin/master\n+\tEOF\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+\tgit -C child for-each-ref --format=\"%(refname)\" >actual &&\n+\tcat >expect <<-EOF &&\n+\trefs/grab/it\n+\trefs/heads/master\n+\trefs/remotes/origin/HEAD\n+\trefs/remotes/origin/master\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'clone -c remote..fetch= --origin=' '\n+\trm -rf child &&\n+\tgit clone --origin=upstream \\\n+\t\t-c \"remote.upstream.fetch=+refs/grab/*:refs/grab/*\" \\\n+\t\t-c \"remote.origin.fetch=+refs/leave/*:refs/leave/*\" \\\n+\t\t. child &&\n+\tgit -C child for-each-ref --format=\"%(refname)\" >actual &&\n+\tcat >expect <<-EOF &&\n+\trefs/grab/it\n+\trefs/heads/master\n+\trefs/remotes/upstream/HEAD\n+\trefs/remotes/upstream/master\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n # Tests for the hidden file attribute on windows\n is_hidden () {\n \t# Use the output of `attrib`, ignore the absolute path\n-- \n2.13.0.35.g14b6294b1\n\n"},{"id":"320815","messageId":"20170526133024.g5rztiv3lea37n72@sigill.intra.peff.net","threadId":"45962","inReplyTo":"20170526100403.19270-1-szeder.dev@gmail.com","subject":"Re: [PATCHv3 1/4] clone: respect additional configured fetch refspecs during initial fetch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-26T13:30:24Z","receivedAt":"2017-05-26T13:30:31Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 26, 2017 at 12:04:03PM +0200, SZEDER Gábor wrote:\n\n> > but given that it lazy-parses in\n> > some cases, it feels a little dangerous.\n> \n> I'm not sure about lazy parsing.\n> remote_get() returns a fully-parsed, cached struct remote instance\n> without re-reading the configuration, so all fields directly\n> corresponding to configuration variables stay the same. However, it\n> does parse fetch and push refspecs on every invocation. So if it were\n> to be called to return the origin remote more than once during\n> cloning, then the default refspec would get lost on subsequent\n> invocations. Is this what you meant with dangerous?\n\nMore or less. I actually didn't look far enough to see under what\ncircumstances we might re-parse (or might not have parsed when we add\nour extra refspec), but that's definitely the sort of thing I was\nworried about.\n\n> (Sidenote: and it would leak some memory, too, because it re-parses\n> the refspecs without free()ing the results of the previous\n> invocation.)\n\nYes, I'd argue that the current code is buggy, since:\n\n x = remote_get(\"foo\");\n y = remote_get(\"foo\");\n\nis a guaranteed leak. It seems like remote_get_1() should protect the\ncall to parse_fetch_refspec() by checking whether ret->fetch is NULL\n(and ditto for ret->push).\n\n> Your proposed function to add a refspec as a string would eliminate\n> this danger.\n\nYeah, I think it's not that hard to support. I'd just rather have the\nlogic inside remote.c, rather than infecting the caller with the\ncomplexity.\n\n> It certainly looks better, see the patch below the scissors for\n> reference, and I thought it works because until last night I only run\n> the corresponding test script (t5611-clone-config), though I know very\n> well that \"Thou shalt always run the full test suite!\" :)\n> \n> Unfortunately, putting the default refspec into this temporary\n> configuration environment breaks a few submodule tests\n> (t5614-clone-submodules or t5614-clone-submodules-shallow (it's got\n> renamed between this topic and master), t7407-submodule-foreach,\n> t7410-submodule-checkout-to), because it \"leaks\" to the submodule\n> environment.\n\nDoh, of course. I didn't think of that. That's probably a bad direction,\nthen, as there's no \"just for this process\" global config.\n\n-Peff\n"},{"id":"320816","messageId":"CAM0VKjkSMnemwPbFihXiQui3wm_wYmQeQmgYrFs5bfsH1jMg1A@mail.gmail.com","threadId":"45962","inReplyTo":"20170515110557.11913-3-szeder.dev@gmail.com","subject":"Re: [PATCHv3 2/4] Documentation/clone: document ignored configuration variables","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-05-26T14:01:47Z","receivedAt":"2017-05-26T14:01:53Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, May 15, 2017 at 1:05 PM, SZEDER Gábor wrote:\n> Due to limitations/bugs in the current implementation, some\n> configuration variables specified via 'git clone -c var=val' (or 'git\n> -c var=val clone') are ignored during the initial fetch and checkout.\n>\n> Let the users know which configuration variables are known to be\n> ignored ('remote.origin.mirror' and 'remote.origin.tagOpt') under the\n> documentation of 'git clone -c'.\n>\n> Signed-off-by: SZEDER Gábor \n> ---\n> Documentation/git-clone.txt | 4 ++++\n> 1 file changed, 4 insertions(+)\n>\n> diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt\n> index ec41d3d69..4f1e7d4ba 100644\n> --- a/Documentation/git-clone.txt\n> +++ b/Documentation/git-clone.txt\n> @@ -186,6 +186,10 @@ objects from the source repository into a pack in the cloned repository.\n> values are given for the same key, each value will be written to\n> the config file. This makes it safe, for example, to add\n> additional fetch refspecs to the origin remote.\n> + Note that due to limitations of the current implementation some\n> + configuration variables don't take effect during the initial\n> + fetch and checkout. Configuration variables known to not take\n> + effect are: `remote..mirror` and `remote..tagOpt`.\n\nA few notes to this patch, because I didn't like that \"known to not\ntake effect\" expression, and looked into how some configuration\nvariables are handled during the initial fetch and checkout. Far from\ncomprehensive, but here they are anyway:\n\nConcerning the initial fetch:\n\n - Configuration variables influencing the refspecs to be fetched are\n currently ignored. These are the fetch refspecs, of course, and\n remote..{mirror,tagOpt}.\n This series makes fetch refspecs work. The other two are mentioned\n in this patch, and are less urgent, because their functionality is\n or will soon be available through command line options (--mirror\n and --no-tags).\n - remote..url is strange. It's not just ignored, it's ignored\n so much that it isn't even written to the config file when\n specified as 'git clone -c ...'. Nonetheless, specifying it for\n 'git clone' doesn't make much sense in the first place, does it?\n So I think it's actually good that it's ignored and it isn't worth\n mentioning it in the docs.\n - Some fetch-related config variables, e.g.\n remote..{prune,skipDefaultUpdate,skipFetchAll} or\n fetch.{prune,output}, don't matter during the initial fetch.\n - Other fetch-related config variables, e.g.\n fetch.{fsckObjects,unpackLimit}, are handled deep down in\n fetch-pack and work properly.\n - Transport-specific config variables, e.g. url..insteadOf,\n remote..{proxy,uploadpack,vcs}, or http.*, if applicable, are\n handled in the transport layer or remote helper.\n - I'm not sure about submodule-related config variables, but there\n are command line options for that.\n\nI'm not sure about the initial checkout, in particular I'm not sure\nhow many configuration variables there are that could/should influence\nthe initial checkout (or any checkout, for that matter).\n\n - core.autocrlf works properly, we even have a test for that.\n - filter..smudge is read during initial checkout, but I'm not\n sure whether that should do anything, since no attributes files\n exist at that point.\n - I glanced through builtin/checkout.c to see whether it looks at any\n configuration variables that clone doesn't, and while it does so,\n it seems to only look at variables that are either irrelevant\n during the initial checkout, e.g. 'diff.ignoresubmodules' and\n 'merge.conflictstyle', or are submodule-specific, and about those I\n have no idea.\n\n\nLike I said, far from comprehensive, but I think at least the fetch\npart is well covered.\n"},{"id":"320983","messageId":"xmqq8tlfdmpg.fsf@gitster.mtv.corp.google.com","threadId":"45962","inReplyTo":"20170526133024.g5rztiv3lea37n72@sigill.intra.peff.net","subject":"Re: [PATCHv3 1/4] clone: respect additional configured fetch refspecs during initial fetch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-30T03:53:47Z","receivedAt":"2017-05-30T03:53:56Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, May 26, 2017 at 12:04:03PM +0200, SZEDER Gábor wrote:\n>\n>> Unfortunately, putting the default refspec into this temporary\n>> configuration environment breaks a few submodule tests\n>> (t5614-clone-submodules or t5614-clone-submodules-shallow (it's got\n>> renamed between this topic and master), t7407-submodule-foreach,\n>> t7410-submodule-checkout-to), because it \"leaks\" to the submodule\n>> environment.\n>\n> Doh, of course. I didn't think of that. That's probably a bad direction,\n> then, as there's no \"just for this process\" global config.\n\nYuck, right. So... do we have a further plan for this topic, or are\nthe patches more or less good as they are?\n"},{"id":"320984","messageId":"20170530035530.s2uqeityyhrdp5sx@sigill.intra.peff.net","threadId":"45962","inReplyTo":"xmqq8tlfdmpg.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv3 1/4] clone: respect additional configured fetch refspecs during initial fetch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-30T03:55:31Z","receivedAt":"2017-05-30T03:55:39Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 30, 2017 at 12:53:47PM +0900, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > On Fri, May 26, 2017 at 12:04:03PM +0200, SZEDER Gábor wrote:\n> >\n> >> Unfortunately, putting the default refspec into this temporary\n> >> configuration environment breaks a few submodule tests\n> >> (t5614-clone-submodules or t5614-clone-submodules-shallow (it's got\n> >> renamed between this topic and master), t7407-submodule-foreach,\n> >> t7410-submodule-checkout-to), because it \"leaks\" to the submodule\n> >> environment.\n> >\n> > Doh, of course. I didn't think of that. That's probably a bad direction,\n> > then, as there's no \"just for this process\" global config.\n> \n> Yuck, right. So... do we have a further plan for this topic, or are\n> the patches more or less good as they are?\n\nI was hoping we'd see one more iteration with the abstracted function\nfor adding to the \"struct remote\". Gábor, let me know if you're too sick\nof the topic, and I can try to re-spin it.\n\n-Peff\n"},{"id":"321005","messageId":"CAM0VKjnOSxQg_VCBO2cgtbqesmNYx+e_H7m=36PsNWi9K9rQ1Q@mail.gmail.com","threadId":"45962","inReplyTo":"xmqq8tlfdmpg.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv3 1/4] clone: respect additional configured fetch refspecs during initial fetch","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-05-30T07:11:28Z","receivedAt":"2017-05-30T07:11:34Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, May 30, 2017 at 5:53 AM, Junio C Hamano wrote:\n> So... do we have a further plan for this topic, or are\n> the patches more or less good as they are?\n\nYup, here it comes.\n\nThe main change since v3 is that clone's default refspec is now added\nto the 'struct remote' using a function based on Peff's\nrecommendation, with slight changes:\n\n - I call that function add_and_parse_fetch_refspec(), because, well,\n that's what it does, and\n - I dropped the if (remote->fetch) condition, because the function\n should work when there are no refspecs in the configuration, too.\n\nI also dropped Peff's two patches that were included in v3, because:\n\n - his last patch doesn't apply anymore, because the variable it frees\n properly doesn't exist anymore, and\n - with that patch gone his second patch is not tangled up with this\n topic, so I think it should go on its own branch.\n\nInterdiff against the first two patch of v3 below.\n\n\nSZEDER Gábor (2):\n clone: respect additional configured fetch refspecs during initial\n fetch\n Documentation/clone: document ignored configuration variables\n\n Documentation/git-clone.txt | 4 ++++\n builtin/clone.c | 34 +++++++++++++++++-----------------\n remote.c | 13 +++++++++++++\n remote.h | 1 +\n t/t5611-clone-config.sh | 44 ++++++++++++++++++++++++++++++++++++++++++++\n 5 files changed, 79 insertions(+), 17 deletions(-)\n\n--\n2.13.0.35.g14b6294b1\n\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 4144190da..4157922d8 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -853,16 +853,13 @@ int cmd_clone(int argc, const char **argv, const\nchar *prefix)\n const struct ref *our_head_points_at;\n struct ref *mapped_refs;\n const struct ref *ref;\n- struct strbuf key = STRBUF_INIT, value = STRBUF_INIT;\n+ struct strbuf key = STRBUF_INIT, default_refspec = STRBUF_INIT;\n struct strbuf branch_top = STRBUF_INIT, reflog_msg = STRBUF_INIT;\n struct transport *transport = NULL;\n const char *src_ref_prefix = \"refs/heads/\";\n struct remote *remote;\n int err = 0, complete_refs_before_fetch = 1;\n\n- struct refspec *refspec;\n- const char *fetch_pattern;\n-\n packet_trace_identity(\"clone\");\n argc = parse_options(argc, argv, prefix, builtin_clone_options,\n builtin_clone_usage, 0);\n@@ -980,7 +977,6 @@ int cmd_clone(int argc, const char **argv, const\nchar *prefix)\n strbuf_addf(&branch_top, \"refs/remotes/%s/\", option_origin);\n }\n\n- strbuf_addf(&value, \"+%s*:%s*\", src_ref_prefix, branch_top.buf);\n strbuf_addf(&key, \"remote.%s.url\", option_origin);\n git_config_set(key.buf, repo);\n strbuf_reset(&key);\n@@ -988,15 +984,10 @@ int cmd_clone(int argc, const char **argv, const\nchar *prefix)\n if (option_reference.nr)\n setup_reference();\n\n- fetch_pattern = value.buf;\n- refspec = parse_fetch_refspec(1, &fetch_pattern);\n-\n- strbuf_reset(&value);\n-\n remote = remote_get(option_origin);\n- REALLOC_ARRAY(remote->fetch, remote->fetch_refspec_nr + 1);\n- memcpy(remote->fetch+remote->fetch_refspec_nr, refspec,\n- sizeof(*refspec));\n+ strbuf_addf(&default_refspec, \"+%s*:%s*\", src_ref_prefix,\n+ branch_top.buf);\n+ add_and_parse_fetch_refspec(remote, default_refspec.buf);\n\n transport = transport_get(remote, remote->url[0]);\n transport_set_verbosity(transport, option_verbosity, option_progress);\n@@ -1039,7 +1030,7 @@ int cmd_clone(int argc, const char **argv, const\nchar *prefix)\n\n if (refs) {\n mapped_refs = wanted_peer_refs(refs, remote->fetch,\n- remote->fetch_refspec_nr + 1);\n+ remote->fetch_refspec_nr);\n /*\n * transport_get_remote_refs() may return refs with null sha-1\n * in mapped_refs (see struct transport->get_refs_list\n@@ -1117,9 +1108,8 @@ int cmd_clone(int argc, const char **argv, const\nchar *prefix)\n strbuf_release(&reflog_msg);\n strbuf_release(&branch_top);\n strbuf_release(&key);\n- strbuf_release(&value);\n+ strbuf_release(&default_refspec);\n junk_mode = JUNK_LEAVE_ALL;\n\n- free(refspec);\n return err;\n }\ndiff --git a/remote.c b/remote.c\nindex ad6c5424e..b8fd09dc9 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -626,6 +626,19 @@ struct refspec *parse_fetch_refspec(int\nnr_refspec, const char **refspec)\n return parse_refspec_internal(nr_refspec, refspec, 1, 0);\n }\n\n+void add_and_parse_fetch_refspec(struct remote *remote, const char *refspec)\n+{\n+ struct refspec *rs;\n+\n+ add_fetch_refspec(remote, refspec);\n+ rs = parse_fetch_refspec(1, &refspec);\n+ REALLOC_ARRAY(remote->fetch, remote->fetch_refspec_nr);\n+ remote->fetch[remote->fetch_refspec_nr - 1] = *rs;\n+\n+ /* Not free_refspecs(), as we copied its pointers above */\n+ free(rs);\n+}\n+\n static struct refspec *parse_push_refspec(int nr_refspec, const char **refspec)\n {\n return parse_refspec_internal(nr_refspec, refspec, 0, 0);\ndiff --git a/remote.h b/remote.h\nindex 924881169..9ad8c1085 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -164,6 +164,7 @@ struct ref *ref_remove_duplicates(struct ref *ref_map);\n\n int valid_fetch_refspec(const char *refspec);\n struct refspec *parse_fetch_refspec(int nr_refspec, const char **refspec);\n+void add_and_parse_fetch_refspec(struct remote *remote, const char *refspec);\n\n void free_refspec(int nr_refspec, struct refspec *refspec);\n"},{"id":"321006","messageId":"20170530071244.32257-1-szeder.dev@gmail.com","threadId":"45962","inReplyTo":"CAM0VKjnOSxQg_VCBO2cgtbqesmNYx+e_H7m=36PsNWi9K9rQ1Q@mail.gmail.com","subject":"[PATCHv4 1/2] clone: respect additional configured fetch refspecs during initial fetch","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-05-30T07:12:43Z","receivedAt":"2017-05-30T07:12:55Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The initial fetch during a clone doesn't transfer refs matching\nadditional fetch refspecs given on the command line as configuration\nvariables. This contradicts the documentation stating that\nconfiguration variables specified via 'git clone -c = ...'\n\"take effect immediately after the repository is initialized, but\nbefore the remote history is fetched\" and the given example\nspecifically mentions \"adding additional fetch refspecs to the origin\nremote\". Furthermore, one-shot configuration variables specified via\n'git -c = clone ...', though not written to the newly\ncreated repository's config file, live during the lifetime of the\n'clone' command, including the initial fetch. All this implies that\nany fetch refspecs specified this way should already be taken into\naccount during the initial fetch.\n\nThe reason for this is that the initial fetch is not a fully fledged\n'git fetch' but a bunch of direct calls into the fetch/transport\nmachinery with clone's own refs-to-refspec matching logic, which\nbypasses parts of 'git fetch' processing configured fetch refspecs.\nThis logic only considers a single default refspec, potentially\ninfluenced by options like '--single-branch' and '--mirror'. The\nconfigured refspecs are, however, already read and parsed properly\nwhen clone calls remote.c:remote_get(), but it never looks at the\nparsed refspecs in the resulting 'struct remote'.\n\nModify clone to take the configured fetch refspecs into account to\nretrieve all matching refs during the initial fetch. Note that the\nconfiguration at that point only includes the fetch refspecs specified\nby the user, but it doesn't include the default fetch refspec. Add a\nfunction to the remote API which encapsulates adding a fetch refspec\nto a 'struct remote' and parsing it, including all the necessary\nmemory management housekeeping as well. Use this new function to add\nclone's default fetch refspec to the other refspecs the user might\nhave specified.\n\nAdd tests to check that refspecs given both via 'git clone -c ...' and\n'git -c ... clone' retrieve all refs matching either the default or\nthe additional refspecs, and that it works even when the user\nspecifies an alternative remote name via '--origin='.\n\nHelped-by: Jeff King \nSigned-off-by: SZEDER Gábor \n---\n builtin/clone.c | 34 +++++++++++++++++-----------------\n remote.c | 13 +++++++++++++\n remote.h | 1 +\n t/t5611-clone-config.sh | 44 ++++++++++++++++++++++++++++++++++++++++++++\n 4 files changed, 75 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex a35d62293..4157922d8 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -520,7 +520,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_nr)\n {\n \tstruct ref *head = copy_ref(find_ref_by_name(refs, \"HEAD\"));\n \tstruct ref *local_refs = head;\n@@ -541,13 +541,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_nr; 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_nr; 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@@ -848,16 +853,13 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tconst struct ref *our_head_points_at;\n \tstruct ref *mapped_refs;\n \tconst struct ref *ref;\n-\tstruct strbuf key = STRBUF_INIT, value = STRBUF_INIT;\n+\tstruct strbuf key = STRBUF_INIT, default_refspec = STRBUF_INIT;\n \tstruct strbuf branch_top = STRBUF_INIT, reflog_msg = STRBUF_INIT;\n \tstruct transport *transport = NULL;\n \tconst char *src_ref_prefix = \"refs/heads/\";\n \tstruct remote *remote;\n \tint err = 0, complete_refs_before_fetch = 1;\n \n-\tstruct refspec *refspec;\n-\tconst char *fetch_pattern;\n-\n \tpacket_trace_identity(\"clone\");\n \targc = parse_options(argc, argv, prefix, builtin_clone_options,\n \t\t\t builtin_clone_usage, 0);\n@@ -975,7 +977,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\tstrbuf_addf(&branch_top, \"refs/remotes/%s/\", option_origin);\n \t}\n \n-\tstrbuf_addf(&value, \"+%s*:%s*\", src_ref_prefix, branch_top.buf);\n \tstrbuf_addf(&key, \"remote.%s.url\", option_origin);\n \tgit_config_set(key.buf, repo);\n \tstrbuf_reset(&key);\n@@ -983,12 +984,11 @@ 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-\n-\tstrbuf_reset(&value);\n-\n \tremote = remote_get(option_origin);\n+\tstrbuf_addf(&default_refspec, \"+%s*:%s*\", src_ref_prefix,\n+\t\t branch_top.buf);\n+\tadd_and_parse_fetch_refspec(remote, default_refspec.buf);\n+\n \ttransport = transport_get(remote, remote->url[0]);\n \ttransport_set_verbosity(transport, option_verbosity, option_progress);\n \ttransport->family = family;\n@@ -1029,7 +1029,8 @@ 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, remote->fetch,\n+\t\t\t\t\t remote->fetch_refspec_nr);\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@@ -1107,9 +1108,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tstrbuf_release(&reflog_msg);\n \tstrbuf_release(&branch_top);\n \tstrbuf_release(&key);\n-\tstrbuf_release(&value);\n+\tstrbuf_release(&default_refspec);\n \tjunk_mode = JUNK_LEAVE_ALL;\n \n-\tfree(refspec);\n \treturn err;\n }\ndiff --git a/remote.c b/remote.c\nindex ad6c5424e..b8fd09dc9 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -626,6 +626,19 @@ struct refspec *parse_fetch_refspec(int nr_refspec, const char **refspec)\n \treturn parse_refspec_internal(nr_refspec, refspec, 1, 0);\n }\n \n+void add_and_parse_fetch_refspec(struct remote *remote, const char *refspec)\n+{\n+\tstruct refspec *rs;\n+\n+\tadd_fetch_refspec(remote, refspec);\n+\trs = parse_fetch_refspec(1, &refspec);\n+\tREALLOC_ARRAY(remote->fetch, remote->fetch_refspec_nr);\n+\tremote->fetch[remote->fetch_refspec_nr - 1] = *rs;\n+\n+\t/* Not free_refspecs(), as we copied its pointers above */\n+\tfree(rs);\n+}\n+\n static struct refspec *parse_push_refspec(int nr_refspec, const char **refspec)\n {\n \treturn parse_refspec_internal(nr_refspec, refspec, 0, 0);\ndiff --git a/remote.h b/remote.h\nindex 924881169..9ad8c1085 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -164,6 +164,7 @@ struct ref *ref_remove_duplicates(struct ref *ref_map);\n \n int valid_fetch_refspec(const char *refspec);\n struct refspec *parse_fetch_refspec(int nr_refspec, const char **refspec);\n+void add_and_parse_fetch_refspec(struct remote *remote, const char *refspec);\n \n void free_refspec(int nr_refspec, struct refspec *refspec);\n \ndiff --git a/t/t5611-clone-config.sh b/t/t5611-clone-config.sh\nindex e4850b778..114b53920 100755\n--- a/t/t5611-clone-config.sh\n+++ b/t/t5611-clone-config.sh\n@@ -37,6 +37,50 @@ 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/leave/out refs/heads/master &&\n+\tgit clone -c \"remote.origin.fetch=+refs/grab/*:refs/grab/*\" . child &&\n+\tgit -C child for-each-ref --format=\"%(refname)\" >actual &&\n+\tcat >expect <<-EOF &&\n+\trefs/grab/it\n+\trefs/heads/master\n+\trefs/remotes/origin/HEAD\n+\trefs/remotes/origin/master\n+\tEOF\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+\tgit -C child for-each-ref --format=\"%(refname)\" >actual &&\n+\tcat >expect <<-EOF &&\n+\trefs/grab/it\n+\trefs/heads/master\n+\trefs/remotes/origin/HEAD\n+\trefs/remotes/origin/master\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'clone -c remote..fetch= --origin=' '\n+\trm -rf child &&\n+\tgit clone --origin=upstream \\\n+\t\t-c \"remote.upstream.fetch=+refs/grab/*:refs/grab/*\" \\\n+\t\t-c \"remote.origin.fetch=+refs/leave/*:refs/leave/*\" \\\n+\t\t. child &&\n+\tgit -C child for-each-ref --format=\"%(refname)\" >actual &&\n+\tcat >expect <<-EOF &&\n+\trefs/grab/it\n+\trefs/heads/master\n+\trefs/remotes/upstream/HEAD\n+\trefs/remotes/upstream/master\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n # Tests for the hidden file attribute on windows\n is_hidden () {\n \t# Use the output of `attrib`, ignore the absolute path\n-- \n2.13.0.35.g14b6294b1\n\n"},{"id":"321007","messageId":"20170530071244.32257-2-szeder.dev@gmail.com","threadId":"45962","inReplyTo":"20170530071244.32257-1-szeder.dev@gmail.com","subject":"[PATCHv4 2/2] Documentation/clone: document ignored configuration variables","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-05-30T07:12:44Z","receivedAt":"2017-05-30T07:12:57Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Due to limitations/bugs in the current implementation, some\nconfiguration variables specified via 'git clone -c var=val' (or 'git\n-c var=val clone') are ignored during the initial fetch and checkout.\n\nLet the users know which configuration variables are known to be\nignored ('remote.origin.mirror' and 'remote.origin.tagOpt') under the\ndocumentation of 'git clone -c'.\n\nSigned-off-by: SZEDER Gábor \n---\n Documentation/git-clone.txt | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt\nindex ec41d3d69..4f1e7d4ba 100644\n--- a/Documentation/git-clone.txt\n+++ b/Documentation/git-clone.txt\n@@ -186,6 +186,10 @@ objects from the source repository into a pack in the cloned repository.\n \tvalues are given for the same key, each value will be written to\n \tthe config file. This makes it safe, for example, to add\n \tadditional fetch refspecs to the origin remote.\n+\tNote that due to limitations of the current implementation some\n+\tconfiguration variables don't take effect during the initial\n+\tfetch and checkout. Configuration variables known to not take\n+\teffect are: `remote..mirror` and `remote..tagOpt`.\n \n --depth ::\n \tCreate a 'shallow' clone with a history truncated to the\n-- \n2.13.0.35.g14b6294b1\n\n"},{"id":"321011","messageId":"CACBZZX740rcQKnfkRXgn0+fmeUDaWL-Kz5WzKeyUvBhXWPwPhg@mail.gmail.com","threadId":"45962","inReplyTo":"20170530071244.32257-2-szeder.dev@gmail.com","subject":"Re: [PATCHv4 2/2] Documentation/clone: document ignored configuration variables","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-30T09:01:56Z","receivedAt":"2017-05-30T09:02:26Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Tue, May 30, 2017 at 9:12 AM, SZEDER Gábor wrote:\n> Due to limitations/bugs in the current implementation, some\n> configuration variables specified via 'git clone -c var=val' (or 'git\n> -c var=val clone') are ignored during the initial fetch and checkout.\n>\n> Let the users know which configuration variables are known to be\n> ignored ('remote.origin.mirror' and 'remote.origin.tagOpt') under the\n> documentation of 'git clone -c'.\n>\n> Signed-off-by: SZEDER Gábor \n> ---\n> Documentation/git-clone.txt | 4 ++++\n> 1 file changed, 4 insertions(+)\n>\n> diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt\n> index ec41d3d69..4f1e7d4ba 100644\n> --- a/Documentation/git-clone.txt\n> +++ b/Documentation/git-clone.txt\n> @@ -186,6 +186,10 @@ objects from the source repository into a pack in the cloned repository.\n> values are given for the same key, each value will be written to\n> the config file. This makes it safe, for example, to add\n> additional fetch refspecs to the origin remote.\n> + Note that due to limitations of the current implementation some\n> + configuration variables don't take effect during the initial\n> + fetch and checkout. Configuration variables known to not take\n> + effect are: `remote..mirror` and `remote..tagOpt`.\n\nWe should add to that: \"Instead supply the --mirror and --no-tags\noptions, respectively\".\n\n> --depth ::\n> Create a 'shallow' clone with a history truncated to the\n> --\n> 2.13.0.35.g14b6294b1\n>\n"},{"id":"321107","messageId":"20170531042304.unkxkuw6s3tcpzkm@sigill.intra.peff.net","threadId":"45962","inReplyTo":"20170530071244.32257-1-szeder.dev@gmail.com","subject":"Re: [PATCHv4 1/2] clone: respect additional configured fetch refspecs during initial fetch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-31T04:23:05Z","receivedAt":"2017-05-31T04:23:14Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 30, 2017 at 09:12:43AM +0200, SZEDER Gábor wrote:\n\n> diff --git a/remote.c b/remote.c\n> index ad6c5424e..b8fd09dc9 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -626,6 +626,19 @@ struct refspec *parse_fetch_refspec(int nr_refspec, const char **refspec)\n> \treturn parse_refspec_internal(nr_refspec, refspec, 1, 0);\n> }\n> \n> +void add_and_parse_fetch_refspec(struct remote *remote, const char *refspec)\n> +{\n> +\tstruct refspec *rs;\n> +\n> +\tadd_fetch_refspec(remote, refspec);\n> +\trs = parse_fetch_refspec(1, &refspec);\n> +\tREALLOC_ARRAY(remote->fetch, remote->fetch_refspec_nr);\n> +\tremote->fetch[remote->fetch_refspec_nr - 1] = *rs;\n> +\n> +\t/* Not free_refspecs(), as we copied its pointers above */\n> +\tfree(rs);\n> +}\n\nWhat happens here if remote->fetch isn't already initialized? I think\nwe'd end up with a bunch of garbage values. That's what I was trying to\nprotect against in my original suggestion.\n\nI'm not sure if that's possible or not. We seem to initialize it in both\nremote_get() and for_each_remote(), and I don't think there are any\nother ways to get a remote. (In fact, I kind of wondered why we do this\nparsing lazily at all, but I think it's so that misconfigured remotes\ndon't cause us to die() if we aren't accessing them at all).\n\nIf we assume that the contract that remote.c provides is that the\nfetch fields are always filled in before a \"struct remote\" is returned\nto a caller, and that only such callers would use this function, it's\nOK. It just seems more dangerous than it needs to be.\n\n-Peff\n"},{"id":"321108","messageId":"20170531042747.7k2yfd7eml6ep5cd@sigill.intra.peff.net","threadId":"45962","inReplyTo":"CAM0VKjnOSxQg_VCBO2cgtbqesmNYx+e_H7m=36PsNWi9K9rQ1Q@mail.gmail.com","subject":"[PATCH] remote: drop free_refspecs() function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-31T04:27:47Z","receivedAt":"2017-05-31T04:27:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 30, 2017 at 09:11:28AM +0200, SZEDER Gábor wrote:\n\n> I also dropped Peff's two patches that were included in v3, because:\n> \n> - his last patch doesn't apply anymore, because the variable it frees\n> properly doesn't exist anymore, and\n> - with that patch gone his second patch is not tangled up with this\n> topic, so I think it should go on its own branch.\n\nMakes sense on both counts. Here's that stand-alone patch for reference,\nwhich can go onto its own topic.\n\n-- >8 --\nSubject: [PATCH] remote: drop free_refspecs() function\n\nWe already have free_refspec(), a public function which does\nthe same thing as the static free_refspecs(). Let's just\nkeep one. There are two minor differences between the\nfunctions:\n\n 1. free_refspecs() is a noop when the refspec argument is\n NULL. This probably doesn't matter in practice. The\n nr_refspec parameter would presumably be 0 in that\n case, skipping the loop. And free(NULL) is explicitly\n OK. But it doesn't hurt for us to port this extra\n safety to free_refspec(), as one of the callers passes\n a funny \"i+1\" count.\n\n 2. The order of arguments is reversed between the two\n functions. This patch uses the already-public order of\n free_refspec(), as it matches the argument order on the\n parsing side.\n\nSigned-off-by: Jeff King \n---\n remote.c | 28 ++++++----------------------\n 1 file changed, 6 insertions(+), 22 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex fdc52d802..2e39bf930 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -477,26 +477,6 @@ static void read_config(void)\n \talias_all_urls();\n }\n \n-/*\n- * This function frees a refspec array.\n- * Warning: code paths should be checked to ensure that the src\n- * and dst pointers are always freeable pointers as well\n- * as the refspec pointer itself.\n- */\n-static void free_refspecs(struct refspec *refspec, int nr_refspec)\n-{\n-\tint i;\n-\n-\tif (!refspec)\n-\t\treturn;\n-\n-\tfor (i = 0; i < nr_refspec; i++) {\n-\t\tfree(refspec[i].src);\n-\t\tfree(refspec[i].dst);\n-\t}\n-\tfree(refspec);\n-}\n-\n static struct refspec *parse_refspec_internal(int nr_refspec, const char **refspec, int fetch, int verify)\n {\n \tint i;\n@@ -610,7 +590,7 @@ static struct refspec *parse_refspec_internal(int nr_refspec, const char **refsp\n \t\t * since it is only possible to reach this point from within\n \t\t * the for loop above.\n \t\t */\n-\t\tfree_refspecs(rs, i+1);\n+\t\tfree_refspec(i+1, rs);\n \t\treturn NULL;\n \t}\n \tdie(\"Invalid refspec '%s'\", refspec[i]);\n@@ -621,7 +601,7 @@ int valid_fetch_refspec(const char *fetch_refspec_str)\n \tstruct refspec *refspec;\n \n \trefspec = parse_refspec_internal(1, &fetch_refspec_str, 1, 1);\n-\tfree_refspecs(refspec, 1);\n+\tfree_refspec(1, refspec);\n \treturn !!refspec;\n }\n \n@@ -638,6 +618,10 @@ struct refspec *parse_push_refspec(int nr_refspec, const char **refspec)\n void free_refspec(int nr_refspec, struct refspec *refspec)\n {\n \tint i;\n+\n+\tif (!refspec)\n+\t\treturn;\n+\n \tfor (i = 0; i < nr_refspec; i++) {\n \t\tfree(refspec[i].src);\n \t\tfree(refspec[i].dst);\n-- \n2.13.0.676.ge5a8f17ed\n\n"},{"id":"321119","messageId":"CAM0VKjmz7MpVt3oPBuwHiXNoLkZmdmrZ66ggk+aY5-4oVkE35A@mail.gmail.com","threadId":"45962","inReplyTo":"CACBZZX740rcQKnfkRXgn0+fmeUDaWL-Kz5WzKeyUvBhXWPwPhg@mail.gmail.com","subject":"Re: [PATCHv4 2/2] Documentation/clone: document ignored configuration variables","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-05-31T08:50:43Z","receivedAt":"2017-05-31T08:50:50Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, May 30, 2017 at 11:01 AM, Ævar Arnfjörð Bjarmason\n wrote:\n> On Tue, May 30, 2017 at 9:12 AM, SZEDER Gábor wrote:\n\n>> diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt\n>> index ec41d3d69..4f1e7d4ba 100644\n>> --- a/Documentation/git-clone.txt\n>> +++ b/Documentation/git-clone.txt\n>> @@ -186,6 +186,10 @@ objects from the source repository into a pack in the cloned repository.\n>> values are given for the same key, each value will be written to\n>> the config file. This makes it safe, for example, to add\n>> additional fetch refspecs to the origin remote.\n>> + Note that due to limitations of the current implementation some\n>> + configuration variables don't take effect during the initial\n>> + fetch and checkout. Configuration variables known to not take\n>> + effect are: `remote..mirror` and `remote..tagOpt`.\n>\n> We should add to that: \"Instead supply the --mirror and --no-tags\n> options, respectively\".\n\nI don't think we can do that in this patch right now. This topic\nbranches off from c3808ca69 (preparing for 2.10.3, 2016-12-05), I\nassume because, as a bugfix, it will be included in maintenance\nreleases for older releases, and those won't have the '--no-tags'\noption. Perhaps as a follow-up, only on master.\n"},{"id":"321120","messageId":"CAM0VKjkfuyBzZmpJL47xmK24EOh6X5OjrxtxVdsgGupAseT_wQ@mail.gmail.com","threadId":"45962","inReplyTo":"20170531042304.unkxkuw6s3tcpzkm@sigill.intra.peff.net","subject":"Re: [PATCHv4 1/2] clone: respect additional configured fetch refspecs during initial fetch","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-05-31T09:34:23Z","receivedAt":"2017-05-31T09:34:31Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Wed, May 31, 2017 at 6:23 AM, Jeff King wrote:\n> On Tue, May 30, 2017 at 09:12:43AM +0200, SZEDER Gábor wrote:\n>\n>> diff --git a/remote.c b/remote.c\n>> index ad6c5424e..b8fd09dc9 100644\n>> --- a/remote.c\n>> +++ b/remote.c\n>> @@ -626,6 +626,19 @@ struct refspec *parse_fetch_refspec(int nr_refspec, const char **refspec)\n>> return parse_refspec_internal(nr_refspec, refspec, 1, 0);\n>> }\n>>\n>> +void add_and_parse_fetch_refspec(struct remote *remote, const char *refspec)\n>> +{\n>> + struct refspec *rs;\n>> +\n>> + add_fetch_refspec(remote, refspec);\n>> + rs = parse_fetch_refspec(1, &refspec);\n>> + REALLOC_ARRAY(remote->fetch, remote->fetch_refspec_nr);\n>> + remote->fetch[remote->fetch_refspec_nr - 1] = *rs;\n>> +\n>> + /* Not free_refspecs(), as we copied its pointers above */\n>> + free(rs);\n>> +}\n>\n> What happens here if remote->fetch isn't already initialized? I think\n> we'd end up with a bunch of garbage values. That's what I was trying to\n> protect against in my original suggestion.\n>\n> I'm not sure if that's possible or not. We seem to initialize it in both\n> remote_get() and for_each_remote(), and I don't think there are any\n> other ways to get a remote.\n\nThe only place creating remotes is remote.c:make_remote(), which\ncalloc()s the required memory, making all of struct remote's fields\nzero-initialized. In case of clone the common case is that the user\ndoesn't specify any additional fetch refspecs, so remote->fetch will\nstill be NULL after full initialization and when\nadd_and_parse_fetch_refspec() is called with the default fetch\nrefspec, meaning we can't 'if (remote->fetch) { parse ... }'. OTOH,\nall functions involved can cope with the fetch-refspec-related fields\nbeing 0/NULL, and at the time remote->fetch_refspec_nr-1 is used for\narray indexing it's not 0 anymore.\n\n> (In fact, I kind of wondered why we do this\n> parsing lazily at all, but I think it's so that misconfigured remotes\n> don't cause us to die() if we aren't accessing them at all).\n>\n> If we assume that the contract that remote.c provides is that the\n> fetch fields are always filled in before a \"struct remote\" is returned\n> to a caller, and that only such callers would use this function, it's\n> OK. It just seems more dangerous than it needs to be.\n>\n> -Peff\n"},{"id":"321130","messageId":"CACBZZX6rczNtCLkxELZ0ETRhuKjf9Hma7AY420BXh0QwfopvgA@mail.gmail.com","threadId":"45962","inReplyTo":"CAM0VKjmz7MpVt3oPBuwHiXNoLkZmdmrZ66ggk+aY5-4oVkE35A@mail.gmail.com","subject":"Re: [PATCHv4 2/2] Documentation/clone: document ignored configuration variables","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-31T14:17:25Z","receivedAt":"2017-05-31T14:17:50Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Wed, May 31, 2017 at 10:50 AM, SZEDER Gábor wrote:\n> On Tue, May 30, 2017 at 11:01 AM, Ævar Arnfjörð Bjarmason\n> wrote:\n>> On Tue, May 30, 2017 at 9:12 AM, SZEDER Gábor wrote:\n>\n>>> diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt\n>>> index ec41d3d69..4f1e7d4ba 100644\n>>> --- a/Documentation/git-clone.txt\n>>> +++ b/Documentation/git-clone.txt\n>>> @@ -186,6 +186,10 @@ objects from the source repository into a pack in the cloned repository.\n>>> values are given for the same key, each value will be written to\n>>> the config file. This makes it safe, for example, to add\n>>> additional fetch refspecs to the origin remote.\n>>> + Note that due to limitations of the current implementation some\n>>> + configuration variables don't take effect during the initial\n>>> + fetch and checkout. Configuration variables known to not take\n>>> + effect are: `remote..mirror` and `remote..tagOpt`.\n>>\n>> We should add to that: \"Instead supply the --mirror and --no-tags\n>> options, respectively\".\n>\n> I don't think we can do that in this patch right now. This topic\n> branches off from c3808ca69 (preparing for 2.10.3, 2016-12-05), I\n> assume because, as a bugfix, it will be included in maintenance\n> releases for older releases, and those won't have the '--no-tags'\n> option. Perhaps as a follow-up, only on master.\n\nSorry, my misunderstanding. I didn't think we'd be updating docs like\nthis in maint releases, thought this was for to-be-2.14.\n"},{"id":"321524","messageId":"20170605081845.tvzidc5nblbnuner@sigill.intra.peff.net","threadId":"45962","inReplyTo":"CAM0VKjkfuyBzZmpJL47xmK24EOh6X5OjrxtxVdsgGupAseT_wQ@mail.gmail.com","subject":"Re: [PATCHv4 1/2] clone: respect additional configured fetch refspecs during initial fetch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-05T08:18:45Z","receivedAt":"2017-06-05T08:18:53Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 31, 2017 at 11:34:23AM +0200, SZEDER Gábor wrote:\n\n> >> +void add_and_parse_fetch_refspec(struct remote *remote, const char *refspec)\n> >> +{\n> >> + struct refspec *rs;\n> >> +\n> >> + add_fetch_refspec(remote, refspec);\n> >> + rs = parse_fetch_refspec(1, &refspec);\n> >> + REALLOC_ARRAY(remote->fetch, remote->fetch_refspec_nr);\n> >> + remote->fetch[remote->fetch_refspec_nr - 1] = *rs;\n> >> +\n> >> + /* Not free_refspecs(), as we copied its pointers above */\n> >> + free(rs);\n> >> +}\n> >\n> > What happens here if remote->fetch isn't already initialized? I think\n> > we'd end up with a bunch of garbage values. That's what I was trying to\n> > protect against in my original suggestion.\n> >\n> > I'm not sure if that's possible or not. We seem to initialize it in both\n> > remote_get() and for_each_remote(), and I don't think there are any\n> > other ways to get a remote.\n> \n> The only place creating remotes is remote.c:make_remote(), which\n> calloc()s the required memory, making all of struct remote's fields\n> zero-initialized. In case of clone the common case is that the user\n> doesn't specify any additional fetch refspecs, so remote->fetch will\n> still be NULL after full initialization and when\n> add_and_parse_fetch_refspec() is called with the default fetch\n> refspec, meaning we can't 'if (remote->fetch) { parse ... }'. OTOH,\n> all functions involved can cope with the fetch-refspec-related fields\n> being 0/NULL, and at the time remote->fetch_refspec_nr-1 is used for\n> array indexing it's not 0 anymore.\n\nYeah, I agree it is safe now. I'm just worried about some function in\nremote.c later doing:\n\n read_config();\n add_and_parse_fetch_refspec(remotes[0], whatever);\n\nwhich leaves the struct in an inconsistent state (we realloc NULL which\nallocates from scratch, and all of the other entries in remote->fetch\nend up uninitialized). Can we at least add an assertion like:\n\n if (!remote->fetch)\n\tBUG(\"cannot add refspec to an unparsed remote\");\n\n?\n\n-Peff\n"},{"id":"321630","messageId":"CAM0VKjngnRv6iAozvhY_c61CyWhQP2khcr0bs1=7G_-MDNu4kg@mail.gmail.com","threadId":"45962","inReplyTo":"20170605081845.tvzidc5nblbnuner@sigill.intra.peff.net","subject":"Re: [PATCHv4 1/2] clone: respect additional configured fetch refspecs during initial fetch","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-06-06T18:19:09Z","receivedAt":"2017-06-06T18:19:15Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Jun 5, 2017 at 10:18 AM, Jeff King wrote:\n> Yeah, I agree it is safe now. I'm just worried about some function in\n> remote.c later doing:\n>\n> read_config();\n> add_and_parse_fetch_refspec(remotes[0], whatever);\n>\n> which leaves the struct in an inconsistent state (we realloc NULL which\n> allocates from scratch, and all of the other entries in remote->fetch\n> end up uninitialized). Can we at least add an assertion like:\n>\n> if (!remote->fetch)\n> BUG(\"cannot add refspec to an unparsed remote\");\n>\n> ?\n\nBut as mentioned before, remote->fetch being NULL is not a bug in\nitself, it's a perfectly valid value even in a fully parsed remote\nwhen the remote has no fetch refspecs.\nTherefore, I think, the condition should instead be:\n\n remote->fetch_refspec_nr && !remote->fetch\n\nWe could even try to be extra helpful by checking this condition and\ncalling parse_fetch_refspec() to initialize remote->fetch instead of\nBUG()ing out. However, that would mask the real issue, namely not\nusing remote_get() to get the remote, so I don't actually think that's\na good thing to do.\n\nOTOH, having remote->fetch so closely related to, yet separate from\nremote->fetch_refspec{,_nr,_alloc} will always inherently be error\nprone. This assertion would catch one case where a less than careful\ndev could cause trouble, sure, but there will be still others left,\ne.g. he could still do:\n\n add_fetch_refspec(remote, ...); // this doesn't update remote->fetch\n for (i = 0; i < remote->fetch_refspec_nr; i++)\n func(remote->fetch[i]);\n\nand watch the array indexing blow up in the last iteration.\n\nOr a non-hypothetical one: when I first tried to use remote_get() for\nan earlier version of this patch, I ALLOC_GROW()-ed remote->fetch to\ncreate room for the default refspec, because in struct remote not\n**fetch_refspec but *fetch is listed right above\nfetch_refspec_{nr,alloc}, being way past my bedtime may be my only\nexcuse... It didn't work :) [1]\n\nTo put your worries to rest we should eliminate remote->fetch_refspec\naltogether and parse refspecs into remote->fetch right away, I'd\nthink. After all, that's what's used in most places anyway, and it\ncan be easily turned back to a single string where needed (I think in\nonly 3 places in builtin/remote.c).\n\n\n[1] - Though in the end this could be considered beneficial, because\n commits 53c5de29 (pickaxe: fix segfault with '-S<...>\n --pickaxe-regex', 2017-03-18), 59210dd56 (tests: make the\n 'test_pause' helper work in non-verbose mode, 2017-03-18), and\n 4ecae3c8c (tests: create an interactive gdb session with the\n 'debug' helper, 2017-03-18) were all fallouts from the ensuing\n debugging session :)\n"},{"id":"321633","messageId":"20170606183726.ycl4k2aoyurj5sfr@sigill.intra.peff.net","threadId":"45962","inReplyTo":"CAM0VKjngnRv6iAozvhY_c61CyWhQP2khcr0bs1=7G_-MDNu4kg@mail.gmail.com","subject":"Re: [PATCHv4 1/2] clone: respect additional configured fetch refspecs during initial fetch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-06T18:37:26Z","receivedAt":"2017-06-06T18:37:31Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 06, 2017 at 08:19:09PM +0200, SZEDER Gábor wrote:\n\n> > if (!remote->fetch)\n> > BUG(\"cannot add refspec to an unparsed remote\");\n> >\n> > ?\n> \n> But as mentioned before, remote->fetch being NULL is not a bug in\n> itself, it's a perfectly valid value even in a fully parsed remote\n> when the remote has no fetch refspecs.\n> Therefore, I think, the condition should instead be:\n> \n> remote->fetch_refspec_nr && !remote->fetch\n\nRight, that would be a better check.\n\n> We could even try to be extra helpful by checking this condition and\n> calling parse_fetch_refspec() to initialize remote->fetch instead of\n> BUG()ing out. However, that would mask the real issue, namely not\n> using remote_get() to get the remote, so I don't actually think that's\n> a good thing to do.\n\nOK.\n\n> To put your worries to rest we should eliminate remote->fetch_refspec\n> altogether and parse refspecs into remote->fetch right away, I'd\n> think. After all, that's what's used in most places anyway, and it\n> can be easily turned back to a single string where needed (I think in\n> only 3 places in builtin/remote.c).\n\nI don't think we can parse right away without regressing the error\nhandling. If I have two remotes, one with a bogus refspec, like:\n\n [remote \"one\"]\n url = ...\n fetch = refs/heads/*:refs/remotes/one/*\n [remote \"two\"]\n url = ...\n fetch = ***bogus***\n\nand I do:\n\n git fetch one\n\nthen read_config() will grab the data for _both_ of them, but only call\nremote_get() on the first one. If we parsed the refspecs during\nread_config(), we'd parse the bogus remote.two.fetch and die().\n\nI guess that's a minor case, but as far as I can tell that's the\nmotivation for the lazy parsing.\n\n-Peff\n"},{"id":"321674","messageId":"CAM0VKjmdTstqJVHZCQ4f+CxdQxnQP9C2ocCYepE5ihcfV7sTkA@mail.gmail.com","threadId":"45962","inReplyTo":"20170606183726.ycl4k2aoyurj5sfr@sigill.intra.peff.net","subject":"Re: [PATCHv4 1/2] clone: respect additional configured fetch refspecs during initial fetch","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-06-07T11:17:13Z","receivedAt":"2017-06-07T11:20:20Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Jun 6, 2017 at 8:37 PM, Jeff King wrote:\n\n>> To put your worries to rest we should eliminate remote->fetch_refspec\n>> altogether and parse refspecs into remote->fetch right away, I'd\n>> think. After all, that's what's used in most places anyway, and it\n>> can be easily turned back to a single string where needed (I think in\n>> only 3 places in builtin/remote.c).\n>\n> I don't think we can parse right away without regressing the error\n> handling. If I have two remotes, one with a bogus refspec, like:\n>\n> [remote \"one\"]\n> url = ...\n> fetch = refs/heads/*:refs/remotes/one/*\n> [remote \"two\"]\n> url = ...\n> fetch = ***bogus***\n>\n> and I do:\n>\n> git fetch one\n>\n> then read_config() will grab the data for _both_ of them, but only call\n> remote_get() on the first one. If we parsed the refspecs during\n> read_config(), we'd parse the bogus remote.two.fetch and die().\n>\n> I guess that's a minor case, but as far as I can tell that's the\n> motivation for the lazy parsing.\n\nYeah, I know, we'd need a parse_refspec_gently() or something.\nparse_refspec_internal() already has a 'verify' parameter which\nprevents it from die()ing while parsing a bogus refspec, but in its\ncurrent form it doesn't tell us which refspec was bogus, so we'd need\na bit more than that to let the user know what's wrong.\n"},{"id":"322169","messageId":"20170613232407.GQ133952@aiede.mtv.corp.google.com","threadId":"45962","inReplyTo":"20170530071244.32257-2-szeder.dev@gmail.com","subject":"Re: [PATCHv4 2/2] Documentation/clone: document ignored configuration variables","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-06-13T23:24:07Z","receivedAt":"2017-06-13T23:24:17Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nSZEDER Gábor wrote:\n\n> Due to limitations/bugs in the current implementation, some\n> configuration variables specified via 'git clone -c var=val' (or 'git\n> -c var=val clone') are ignored during the initial fetch and checkout.\n>\n> Let the users know which configuration variables are known to be\n> ignored ('remote.origin.mirror' and 'remote.origin.tagOpt') under the\n> documentation of 'git clone -c'.\n>\n> Signed-off-by: SZEDER Gábor \n> ---\n> Documentation/git-clone.txt | 4 ++++\n> 1 file changed, 4 insertions(+)\n\nMakes sense.\n\n> diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt\n> index ec41d3d69..4f1e7d4ba 100644\n> --- a/Documentation/git-clone.txt\n> +++ b/Documentation/git-clone.txt\n> @@ -186,6 +186,10 @@ objects from the source repository into a pack in the cloned repository.\n> \tvalues are given for the same key, each value will be written to\n> \tthe config file. This makes it safe, for example, to add\n> \tadditional fetch refspecs to the origin remote.\n> +\tNote that due to limitations of the current implementation some\n> +\tconfiguration variables don't take effect during the initial\n> +\tfetch and checkout. Configuration variables known to not take\n> +\teffect are: `remote..mirror` and `remote..tagOpt`.\n> \n\nTiny nit: the paragraph of --config description is already a bit\noverwhelming, and I think this additional note takes it over the edge\nwhere I give up and stop reading. Could it go in a separate\nparagraph?\n\n\t\tthe config file. This makes it safe, for example, to add\n\t\tadditional fetch refspecs to the origin remote.\n\t+\n\tDue to limitations in the current implementation, some\n\tconfiguration variables do not take effect until after the\n\tinitial fetch and checkout. Configuration variables known\n\tnot to take effect are `remote..mirror` and\n\t`remote..tagOpt`.\n\nThanks,\nJonathan\n"},{"id":"322175","messageId":"20170614004816.GR133952@aiede.mtv.corp.google.com","threadId":"45962","inReplyTo":"20170530071244.32257-1-szeder.dev@gmail.com","subject":"Re: [PATCHv4 1/2] clone: respect additional configured fetch refspecs during initial fetch","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-06-14T00:48:16Z","receivedAt":"2017-06-14T00:48:27Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nSZEDER Gábor wrote:\n\n> The initial fetch during a clone doesn't transfer refs matching\n> additional fetch refspecs given on the command line as configuration\n> variables. This contradicts the documentation stating that\n> configuration variables specified via 'git clone -c = ...'\n> \"take effect immediately after the repository is initialized, but\n> before the remote history is fetched\" and the given example\n[...]\n> The reason for this is that the initial fetch is not a fully fledged\n> 'git fetch' but a bunch of direct calls into the fetch/transport\n> machinery with clone's own refs-to-refspec matching logic, which\n> bypasses parts of 'git fetch' processing configured fetch refspecs.\n\nAgh, subtle.\n\nI'm hoping that longer term we can make fetch behave more like a\nlibrary and make the initial fetch into a fully fledged 'git fetch'\nlike thing again. But this smaller change is the logical fix in the\nmeantime.\n\n[...]\n> diff --git a/remote.c b/remote.c\n> index ad6c5424e..b8fd09dc9 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -626,6 +626,19 @@ struct refspec *parse_fetch_refspec(int nr_refspec, const char **refspec)\n> \treturn parse_refspec_internal(nr_refspec, refspec, 1, 0);\n> }\n> \n> +void add_and_parse_fetch_refspec(struct remote *remote, const char *refspec)\n> +{\n> +\tstruct refspec *rs;\n> +\n> +\tadd_fetch_refspec(remote, refspec);\n> +\trs = parse_fetch_refspec(1, &refspec);\n> +\tREALLOC_ARRAY(remote->fetch, remote->fetch_refspec_nr);\n> +\tremote->fetch[remote->fetch_refspec_nr - 1] = *rs;\n> +\n> +\t/* Not free_refspecs(), as we copied its pointers above */\n> +\tfree(rs);\n> +}\n> +\n> static struct refspec *parse_push_refspec(int nr_refspec, const char **refspec)\n> {\n> \treturn parse_refspec_internal(nr_refspec, refspec, 0, 0);\n> diff --git a/remote.h b/remote.h\n> index 924881169..9ad8c1085 100644\n> --- a/remote.h\n> +++ b/remote.h\n> @@ -164,6 +164,7 @@ struct ref *ref_remove_duplicates(struct ref *ref_map);\n> \n> int valid_fetch_refspec(const char *refspec);\n> struct refspec *parse_fetch_refspec(int nr_refspec, const char **refspec);\n> +void add_and_parse_fetch_refspec(struct remote *remote, const char *refspec);\n> \n> void free_refspec(int nr_refspec, struct refspec *refspec);\n\nI realize its neighbors don't have this, but can this function have a\nbrief comment explaining how it is meant to be used and what\nguarantees it makes?\n\nFor example:\n\n\t/** Adds a refspec to remote->fetch_refspec and remote->fetch. */\n\tvoid add_and_parse_fetch_refspec(struct remote *remote, const char *refspec);\n\nI'm tempted to say that this one should be named add_fetch_refspec (or\nsomething like remote_add_refspec) --- this is the only way to add a\nfetch refspec in the public remote API, and the fact that it parses is\nan implementation detail. The private add_fetch_refpsec that builds\nthe fetch_refspec as preparation for parsing them in a batch is not\npart of the exported API.\n\nAlso, now that the API is appending to remote->fetch instead of\nallocating it in one go, should it use the ALLOC_GROW heuristic /\nfetch_refspec_alloc size?\n\nThe caller adds one refspec right after calling remote_get. I'm\nstarting to wonder if this could be done more simply by having a\nvariant of remote_get that allows naming an additional refspec, so\nthat remote->fetch could be immutable after construction like it was\nbefore. What do you think?\n\n[...]\n> +\t/* Not free_refspecs(), as we copied its pointers above */\n> +\tfree(rs);\n\nAllocating an array to put the parsed refspec in and then freeing it\nseems wasteful. Should parse_refspec_internal be changed to take an\noutput parameter so it can put the refspec into remote->fetch\ndirectly?\n\n[...]\n> +++ b/builtin/clone.c\n[...]\n> @@ -848,16 +853,13 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n> \tconst struct ref *our_head_points_at;\n> \tstruct ref *mapped_refs;\n> \tconst struct ref *ref;\n> -\tstruct strbuf key = STRBUF_INIT, value = STRBUF_INIT;\n> +\tstruct strbuf key = STRBUF_INIT, default_refspec = STRBUF_INIT;\n\nnit: since it's not part of a key, value pair like value,\ndefault_refspec should probably go on its own line.\n\n[...]\n> --- a/t/t5611-clone-config.sh\n> +++ b/t/t5611-clone-config.sh\n> @@ -37,6 +37,50 @@ 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/leave/out refs/heads/master &&\n> +\tgit clone -c \"remote.origin.fetch=+refs/grab/*:refs/grab/*\" . child &&\n> +\tgit -C child for-each-ref --format=\"%(refname)\" >actual &&\n> +\tcat >expect <<-EOF &&\n> +\trefs/grab/it\n> +\trefs/heads/master\n> +\trefs/remotes/origin/HEAD\n> +\trefs/remotes/origin/master\n> +\tEOF\n> +\ttest_cmp expect actual\n> +'\n\nCan use <<-\\EOF to save the reviewer from having to look for variable\ninterpolations.\n\noptional nit: might be easier to read with a blank line before the\n\"cat >expect\" line or the for-each-ref line. That way, it's easier to\nseparate the validation of output from the commands being run at a\nglance and see what the test is about.\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> +\tgit -C child for-each-ref --format=\"%(refname)\" >actual &&\n> +\tcat >expect <<-EOF &&\n> +\trefs/grab/it\n> +\trefs/heads/master\n> +\trefs/remotes/origin/HEAD\n> +\trefs/remotes/origin/master\n> +\tEOF\n\nLikewise.\n\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'clone -c remote..fetch= --origin=' '\n> +\trm -rf child &&\n> +\tgit clone --origin=upstream \\\n> +\t\t-c \"remote.upstream.fetch=+refs/grab/*:refs/grab/*\" \\\n> +\t\t-c \"remote.origin.fetch=+refs/leave/*:refs/leave/*\" \\\n> +\t\t. child &&\n> +\tgit -C child for-each-ref --format=\"%(refname)\" >actual &&\n> +\tcat >expect <<-EOF &&\n> +\trefs/grab/it\n> +\trefs/heads/master\n> +\trefs/remotes/upstream/HEAD\n> +\trefs/remotes/upstream/master\n> +\tEOF\n\nLikewise. Nice.\n\n> +\ttest_cmp expect actual\n> +'\n> +\n> # Tests for the hidden file attribute on windows\n> is_hidden () {\n> \t# Use the output of `attrib`, ignore the absolute path\n\nThe rest looks good. Thanks for a pleasant read.\n\nSincerely,\nJonathan\n"},{"id":"322176","messageId":"20170614004951.GS133952@aiede.mtv.corp.google.com","threadId":"45962","inReplyTo":"20170531042747.7k2yfd7eml6ep5cd@sigill.intra.peff.net","subject":"Re: [PATCH] remote: drop free_refspecs() function","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-06-14T00:49:51Z","receivedAt":"2017-06-14T00:50:02Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> Subject: [PATCH] remote: drop free_refspecs() function\n>\n> We already have free_refspec(), a public function which does\n> the same thing as the static free_refspecs(). Let's just\n> keep one. There are two minor differences between the\n> functions:\n>\n> 1. free_refspecs() is a noop when the refspec argument is\n> NULL. This probably doesn't matter in practice. The\n> nr_refspec parameter would presumably be 0 in that\n> case, skipping the loop. And free(NULL) is explicitly\n> OK. But it doesn't hurt for us to port this extra\n> safety to free_refspec(), as one of the callers passes\n> a funny \"i+1\" count.\n>\n> 2. The order of arguments is reversed between the two\n> functions. This patch uses the already-public order of\n> free_refspec(), as it matches the argument order on the\n> parsing side.\n>\n> Signed-off-by: Jeff King \n> ---\n> remote.c | 28 ++++++----------------------\n> 1 file changed, 6 insertions(+), 22 deletions(-)\n\nReviewed-by: Jonathan Nieder \n\nThanks.\n"},{"id":"322214","messageId":"20170614095023.nzu535pv6cluzdz7@sigill.intra.peff.net","threadId":"45962","inReplyTo":"20170614004816.GR133952@aiede.mtv.corp.google.com","subject":"Re: [PATCHv4 1/2] clone: respect additional configured fetch refspecs during initial fetch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-14T09:50:23Z","receivedAt":"2017-06-14T09:51:50Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 13, 2017 at 05:48:16PM -0700, Jonathan Nieder wrote:\n\n> > The reason for this is that the initial fetch is not a fully fledged\n> > 'git fetch' but a bunch of direct calls into the fetch/transport\n> > machinery with clone's own refs-to-refspec matching logic, which\n> > bypasses parts of 'git fetch' processing configured fetch refspecs.\n> \n> Agh, subtle.\n> \n> I'm hoping that longer term we can make fetch behave more like a\n> library and make the initial fetch into a fully fledged 'git fetch'\n> like thing again. But this smaller change is the logical fix in the\n> meantime.\n\nWe talked about this earlier in the thread, but I doubt that will ever\nhappen. Things like --single-branch means that clone has to actually\nlook at what the remote has before it decides what to fetch.\n\nOf course we _could_ teach all that logic to fetch, too, but I don't\nthink you'll ever get the nice simple:\n\n 1. configure refspecs\n 2. fetch\n 3. checkout\n\nSo the best thing is probably to push any repeated logic down into the\ntransport layer, where it can be easily used by both commands.\n\n> > +\t/* Not free_refspecs(), as we copied its pointers above */\n> > +\tfree(rs);\n> \n> Allocating an array to put the parsed refspec in and then freeing it\n> seems wasteful. Should parse_refspec_internal be changed to take an\n> output parameter so it can put the refspec into remote->fetch\n> directly?\n\nIt's not quite trivial to make that change. parse_refspec_internal()\nactually handles an array of refspecs. So its callers would all have to\nstart allocating the correctly-sized array.\n\nI doubt that one malloc per clone is worth caring about. I'd worry more\nabout the trickiness that merited the comment above, but it's at least\ncontained in this one function.\n\n-Peff\n"},{"id":"322466","messageId":"CAM0VKjmxtqB2zrWOW8T9O1ReWNPTZA7V3-Dei7GecB3nxVh2Dg@mail.gmail.com","threadId":"45962","inReplyTo":"20170614004816.GR133952@aiede.mtv.corp.google.com","subject":"Re: [PATCHv4 1/2] clone: respect additional configured fetch refspecs during initial fetch","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-06-16T17:36:19Z","receivedAt":"2017-06-16T17:36:26Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Wed, Jun 14, 2017 at 2:48 AM, Jonathan Nieder wrote:\n\n>> diff --git a/remote.h b/remote.h\n>> index 924881169..9ad8c1085 100644\n>> --- a/remote.h\n>> +++ b/remote.h\n>> @@ -164,6 +164,7 @@ struct ref *ref_remove_duplicates(struct ref *ref_map);\n>>\n>> int valid_fetch_refspec(const char *refspec);\n>> struct refspec *parse_fetch_refspec(int nr_refspec, const char **refspec);\n>> +void add_and_parse_fetch_refspec(struct remote *remote, const char *refspec);\n\n> I'm tempted to say that this one should be named add_fetch_refspec (or\n> something like remote_add_refspec) --- this is the only way to add a\n> fetch refspec in the public remote API, and the fact that it parses is\n> an implementation detail. The private add_fetch_refpsec that builds\n> the fetch_refspec as preparation for parsing them in a batch is not\n> part of the exported API.\n\nI kind of agree, but ...\n\nFirst, there is an add_push_refspec() function as well, which, just\nlike its fetch counterpart, doesn't parse the given refspec, only\nappends it to remote->push_refspec. Changing add_fetch_refspec() to\nparse, too, would break this symmetry.\n\nFurthermore, at the moment we have both remote->fetch_refspec (for\nstrings) and remote->fetch (for parsed refspecs), and parsing a\nrefspec die()s if it's bogus, therefore I think that parsing is not an\nimplementation detail that should be hidden.\n\n> The caller adds one refspec right after calling remote_get. I'm\n> starting to wonder if this could be done more simply by having a\n> variant of remote_get that allows naming an additional refspec, so\n> that remote->fetch could be immutable after construction like it was\n> before. What do you think?\n\nThat's such a very specific and narrow use case that I don't think it\njustifies a dedicated function.\nI don't think remote->fetch should be immutable; I think\nremote->{fetch,push}_refspec and the lazy parsing of refspecs should\ngo away. Cleaning up this corner of the remote API is beyond the\nscope of this patch series.\n\n> [...]\n>> + /* Not free_refspecs(), as we copied its pointers above */\n>> + free(rs);\n>\n> Allocating an array to put the parsed refspec in and then freeing it\n> seems wasteful. Should parse_refspec_internal be changed to take an\n> output parameter so it can put the refspec into remote->fetch\n> directly?\n\nNo, I found that extracting the huge body of its loop into a helper\nfunction that fills an output parameter is much more useful.\n\n\n> [...]\n>> +++ b/builtin/clone.c\n> [...]\n>> @@ -848,16 +853,13 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n>> const struct ref *our_head_points_at;\n>> struct ref *mapped_refs;\n>> const struct ref *ref;\n>> - struct strbuf key = STRBUF_INIT, value = STRBUF_INIT;\n>> + struct strbuf key = STRBUF_INIT, default_refspec = STRBUF_INIT;\n>\n> nit: since it's not part of a key, value pair like value,\n> default_refspec should probably go on its own line.\n\nFun fact: they were never part of a key-value pair. While 'key' is\nindeed the name of a configuration variable, 'value' is not the value\nof that configuration variable, or any other configuration variable\nfor that matter.\n\n\nBest,\nGábor\n"},{"id":"322467","messageId":"20170616173849.8071-1-szeder.dev@gmail.com","threadId":"45962","inReplyTo":"CAM0VKjmxtqB2zrWOW8T9O1ReWNPTZA7V3-Dei7GecB3nxVh2Dg@mail.gmail.com","subject":"[PATCHv5 0/2] clone: respect additional configured fetch refspecs during initial fetch","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-06-16T17:38:47Z","receivedAt":"2017-06-16T17:39:10Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"\nOnly small, straightforward adjustments, mostly based on Jonathan's\ncomments.\n\nThe one exception is passing the default refspec using\nstrbuf_detach(), because add_fetch_refspec() doesn't make a copy of it\ninternally. It doesn't make any difference in practice, because\nstrbuf default_refspec remains unaltered til the very end, but this is\nthe right thing to do.\n\nSZEDER Gábor (2):\n clone: respect additional configured fetch refspecs during initial\n fetch\n Documentation/clone: document ignored configuration variables\n\n Documentation/git-clone.txt | 5 +++++\n builtin/clone.c | 36 ++++++++++++++++++----------------\n remote.c | 13 +++++++++++++\n remote.h | 1 +\n t/t5611-clone-config.sh | 47 +++++++++++++++++++++++++++++++++++++++++++++\n 5 files changed, 85 insertions(+), 17 deletions(-)\n\n-- \n2.13.1.505.g7cc9fcafb\n\n\n\ndiff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt\nindex 4f1e7d4ba..5ceccb258 100644\n--- a/Documentation/git-clone.txt\n+++ b/Documentation/git-clone.txt\n@@ -186,10 +186,11 @@ objects from the source repository into a pack in the cloned repository.\n \tvalues are given for the same key, each value will be written to\n \tthe config file. This makes it safe, for example, to add\n \tadditional fetch refspecs to the origin remote.\n-\tNote that due to limitations of the current implementation some\n-\tconfiguration variables don't take effect during the initial\n-\tfetch and checkout. Configuration variables known to not take\n-\teffect are: `remote..mirror` and `remote..tagOpt`.\n++\n+Due to limitations if the current implementation, some configuration\n+variables do not take effect until after the initial fetch and checkout.\n+Configuration variables known to not take effect are:\n+`remote..mirror` and `remote..tagOpt`.\n \n --depth ::\n \tCreate a 'shallow' clone with a history truncated to the\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 4157922d8..8a7edd4e5 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -853,7 +853,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tconst struct ref *our_head_points_at;\n \tstruct ref *mapped_refs;\n \tconst struct ref *ref;\n-\tstruct strbuf key = STRBUF_INIT, default_refspec = STRBUF_INIT;\n+\tstruct strbuf key = STRBUF_INIT;\n+\tstruct strbuf default_refspec = STRBUF_INIT;\n \tstruct strbuf branch_top = STRBUF_INIT, reflog_msg = STRBUF_INIT;\n \tstruct transport *transport = NULL;\n \tconst char *src_ref_prefix = \"refs/heads/\";\n@@ -987,7 +988,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tremote = remote_get(option_origin);\n \tstrbuf_addf(&default_refspec, \"+%s*:%s*\", src_ref_prefix,\n \t\t branch_top.buf);\n-\tadd_and_parse_fetch_refspec(remote, default_refspec.buf);\n+\tadd_and_parse_fetch_refspec(remote,\n+\t\t\t\t strbuf_detach(&default_refspec, NULL));\n \n \ttransport = transport_get(remote, remote->url[0]);\n \ttransport_set_verbosity(transport, option_verbosity, option_progress);\ndiff --git a/t/t5611-clone-config.sh b/t/t5611-clone-config.sh\nindex 114b53920..f240b22cc 100755\n--- a/t/t5611-clone-config.sh\n+++ b/t/t5611-clone-config.sh\n@@ -43,7 +43,8 @@ test_expect_success 'clone -c remote.origin.fetch= works' '\n \tgit update-ref refs/leave/out refs/heads/master &&\n \tgit clone -c \"remote.origin.fetch=+refs/grab/*:refs/grab/*\" . child &&\n \tgit -C child for-each-ref --format=\"%(refname)\" >actual &&\n-\tcat >expect <<-EOF &&\n+\n+\tcat >expect <<-\\EOF &&\n \trefs/grab/it\n \trefs/heads/master\n \trefs/remotes/origin/HEAD\n@@ -56,7 +57,8 @@ 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 \tgit -C child for-each-ref --format=\"%(refname)\" >actual &&\n-\tcat >expect <<-EOF &&\n+\n+\tcat >expect <<-\\EOF &&\n \trefs/grab/it\n \trefs/heads/master\n \trefs/remotes/origin/HEAD\n@@ -72,7 +74,8 @@ test_expect_success 'clone -c remote..fetch= --origin=' '\n \t\t-c \"remote.origin.fetch=+refs/leave/*:refs/leave/*\" \\\n \t\t. child &&\n \tgit -C child for-each-ref --format=\"%(refname)\" >actual &&\n-\tcat >expect <<-EOF &&\n+\n+\tcat >expect <<-\\EOF &&\n \trefs/grab/it\n \trefs/heads/master\n \trefs/remotes/upstream/HEAD\n"},{"id":"322468","messageId":"20170616173849.8071-3-szeder.dev@gmail.com","threadId":"45962","inReplyTo":"20170616173849.8071-1-szeder.dev@gmail.com","subject":"[PATCHv5 2/2] Documentation/clone: document ignored configuration variables","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-06-16T17:38:49Z","receivedAt":"2017-06-16T17:39:12Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Due to limitations/bugs in the current implementation, some\nconfiguration variables specified via 'git clone -c var=val' (or 'git\n-c var=val clone') are ignored during the initial fetch and checkout.\n\nLet the users know which configuration variables are known to be\nignored ('remote.origin.mirror' and 'remote.origin.tagOpt') under the\ndocumentation of 'git clone -c'.\n\nSigned-off-by: SZEDER Gábor \n---\n Documentation/git-clone.txt | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt\nindex ec41d3d69..5ceccb258 100644\n--- a/Documentation/git-clone.txt\n+++ b/Documentation/git-clone.txt\n@@ -186,6 +186,11 @@ objects from the source repository into a pack in the cloned repository.\n \tvalues are given for the same key, each value will be written to\n \tthe config file. This makes it safe, for example, to add\n \tadditional fetch refspecs to the origin remote.\n++\n+Due to limitations if the current implementation, some configuration\n+variables do not take effect until after the initial fetch and checkout.\n+Configuration variables known to not take effect are:\n+`remote..mirror` and `remote..tagOpt`.\n \n --depth ::\n \tCreate a 'shallow' clone with a history truncated to the\n-- \n2.13.1.505.g7cc9fcafb\n\n"},{"id":"322469","messageId":"20170616173849.8071-2-szeder.dev@gmail.com","threadId":"45962","inReplyTo":"20170616173849.8071-1-szeder.dev@gmail.com","subject":"[PATCHv5 1/2] clone: respect additional configured fetch refspecs during initial fetch","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-06-16T17:38:48Z","receivedAt":"2017-06-16T17:39:21Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The initial fetch during a clone doesn't transfer refs matching\nadditional fetch refspecs given on the command line as configuration\nvariables. This contradicts the documentation stating that\nconfiguration variables specified via 'git clone -c = ...'\n\"take effect immediately after the repository is initialized, but\nbefore the remote history is fetched\" and the given example\nspecifically mentions \"adding additional fetch refspecs to the origin\nremote\". Furthermore, one-shot configuration variables specified via\n'git -c = clone ...', though not written to the newly\ncreated repository's config file, live during the lifetime of the\n'clone' command, including the initial fetch. All this implies that\nany fetch refspecs specified this way should already be taken into\naccount during the initial fetch.\n\nThe reason for this is that the initial fetch is not a fully fledged\n'git fetch' but a bunch of direct calls into the fetch/transport\nmachinery with clone's own refs-to-refspec matching logic, which\nbypasses parts of 'git fetch' processing configured fetch refspecs.\nThis logic only considers a single default refspec, potentially\ninfluenced by options like '--single-branch' and '--mirror'. The\nconfigured refspecs are, however, already read and parsed properly\nwhen clone calls remote.c:remote_get(), but it never looks at the\nparsed refspecs in the resulting 'struct remote'.\n\nModify clone to take the configured fetch refspecs into account to\nretrieve all matching refs during the initial fetch. Note that the\nconfiguration at that point only includes the fetch refspecs specified\nby the user, but it doesn't include the default fetch refspec. Add a\nfunction to the remote API which encapsulates adding a fetch refspec\nto a 'struct remote' and parsing it, including all the necessary\nmemory management housekeeping as well. Use this new function to add\nclone's default fetch refspec to the other refspecs the user might\nhave specified.\n\nAdd tests to check that refspecs given both via 'git clone -c ...' and\n'git -c ... clone' retrieve all refs matching either the default or\nthe additional refspecs, and that it works even when the user\nspecifies an alternative remote name via '--origin='.\n\nHelped-by: Jeff King \nSigned-off-by: SZEDER Gábor \n---\n builtin/clone.c | 36 +++++++++++++++++++-----------------\n remote.c | 13 +++++++++++++\n remote.h | 1 +\n t/t5611-clone-config.sh | 47 +++++++++++++++++++++++++++++++++++++++++++++++\n 4 files changed, 80 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex a35d62293..8a7edd4e5 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -520,7 +520,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_nr)\n {\n \tstruct ref *head = copy_ref(find_ref_by_name(refs, \"HEAD\"));\n \tstruct ref *local_refs = head;\n@@ -541,13 +541,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_nr; 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_nr; 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@@ -848,16 +853,14 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tconst struct ref *our_head_points_at;\n \tstruct ref *mapped_refs;\n \tconst struct ref *ref;\n-\tstruct strbuf key = STRBUF_INIT, value = STRBUF_INIT;\n+\tstruct strbuf key = STRBUF_INIT;\n+\tstruct strbuf default_refspec = STRBUF_INIT;\n \tstruct strbuf branch_top = STRBUF_INIT, reflog_msg = STRBUF_INIT;\n \tstruct transport *transport = NULL;\n \tconst char *src_ref_prefix = \"refs/heads/\";\n \tstruct remote *remote;\n \tint err = 0, complete_refs_before_fetch = 1;\n \n-\tstruct refspec *refspec;\n-\tconst char *fetch_pattern;\n-\n \tpacket_trace_identity(\"clone\");\n \targc = parse_options(argc, argv, prefix, builtin_clone_options,\n \t\t\t builtin_clone_usage, 0);\n@@ -975,7 +978,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\tstrbuf_addf(&branch_top, \"refs/remotes/%s/\", option_origin);\n \t}\n \n-\tstrbuf_addf(&value, \"+%s*:%s*\", src_ref_prefix, branch_top.buf);\n \tstrbuf_addf(&key, \"remote.%s.url\", option_origin);\n \tgit_config_set(key.buf, repo);\n \tstrbuf_reset(&key);\n@@ -983,12 +985,12 @@ 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-\n-\tstrbuf_reset(&value);\n-\n \tremote = remote_get(option_origin);\n+\tstrbuf_addf(&default_refspec, \"+%s*:%s*\", src_ref_prefix,\n+\t\t branch_top.buf);\n+\tadd_and_parse_fetch_refspec(remote,\n+\t\t\t\t strbuf_detach(&default_refspec, NULL));\n+\n \ttransport = transport_get(remote, remote->url[0]);\n \ttransport_set_verbosity(transport, option_verbosity, option_progress);\n \ttransport->family = family;\n@@ -1029,7 +1031,8 @@ 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, remote->fetch,\n+\t\t\t\t\t remote->fetch_refspec_nr);\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@@ -1107,9 +1110,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tstrbuf_release(&reflog_msg);\n \tstrbuf_release(&branch_top);\n \tstrbuf_release(&key);\n-\tstrbuf_release(&value);\n+\tstrbuf_release(&default_refspec);\n \tjunk_mode = JUNK_LEAVE_ALL;\n \n-\tfree(refspec);\n \treturn err;\n }\ndiff --git a/remote.c b/remote.c\nindex ad6c5424e..b8fd09dc9 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -626,6 +626,19 @@ struct refspec *parse_fetch_refspec(int nr_refspec, const char **refspec)\n \treturn parse_refspec_internal(nr_refspec, refspec, 1, 0);\n }\n \n+void add_and_parse_fetch_refspec(struct remote *remote, const char *refspec)\n+{\n+\tstruct refspec *rs;\n+\n+\tadd_fetch_refspec(remote, refspec);\n+\trs = parse_fetch_refspec(1, &refspec);\n+\tREALLOC_ARRAY(remote->fetch, remote->fetch_refspec_nr);\n+\tremote->fetch[remote->fetch_refspec_nr - 1] = *rs;\n+\n+\t/* Not free_refspecs(), as we copied its pointers above */\n+\tfree(rs);\n+}\n+\n static struct refspec *parse_push_refspec(int nr_refspec, const char **refspec)\n {\n \treturn parse_refspec_internal(nr_refspec, refspec, 0, 0);\ndiff --git a/remote.h b/remote.h\nindex 924881169..9ad8c1085 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -164,6 +164,7 @@ struct ref *ref_remove_duplicates(struct ref *ref_map);\n \n int valid_fetch_refspec(const char *refspec);\n struct refspec *parse_fetch_refspec(int nr_refspec, const char **refspec);\n+void add_and_parse_fetch_refspec(struct remote *remote, const char *refspec);\n \n void free_refspec(int nr_refspec, struct refspec *refspec);\n \ndiff --git a/t/t5611-clone-config.sh b/t/t5611-clone-config.sh\nindex e4850b778..f240b22cc 100755\n--- a/t/t5611-clone-config.sh\n+++ b/t/t5611-clone-config.sh\n@@ -37,6 +37,53 @@ 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/leave/out refs/heads/master &&\n+\tgit clone -c \"remote.origin.fetch=+refs/grab/*:refs/grab/*\" . child &&\n+\tgit -C child for-each-ref --format=\"%(refname)\" >actual &&\n+\n+\tcat >expect <<-\\EOF &&\n+\trefs/grab/it\n+\trefs/heads/master\n+\trefs/remotes/origin/HEAD\n+\trefs/remotes/origin/master\n+\tEOF\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+\tgit -C child for-each-ref --format=\"%(refname)\" >actual &&\n+\n+\tcat >expect <<-\\EOF &&\n+\trefs/grab/it\n+\trefs/heads/master\n+\trefs/remotes/origin/HEAD\n+\trefs/remotes/origin/master\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'clone -c remote..fetch= --origin=' '\n+\trm -rf child &&\n+\tgit clone --origin=upstream \\\n+\t\t-c \"remote.upstream.fetch=+refs/grab/*:refs/grab/*\" \\\n+\t\t-c \"remote.origin.fetch=+refs/leave/*:refs/leave/*\" \\\n+\t\t. child &&\n+\tgit -C child for-each-ref --format=\"%(refname)\" >actual &&\n+\n+\tcat >expect <<-\\EOF &&\n+\trefs/grab/it\n+\trefs/heads/master\n+\trefs/remotes/upstream/HEAD\n+\trefs/remotes/upstream/master\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n # Tests for the hidden file attribute on windows\n is_hidden () {\n \t# Use the output of `attrib`, ignore the absolute path\n-- \n2.13.1.505.g7cc9fcafb\n\n"},{"id":"322471","messageId":"87wp8b94gb.fsf@gmail.com","threadId":"45962","inReplyTo":"20170616173849.8071-3-szeder.dev@gmail.com","subject":"Re: [PATCHv5 2/2] Documentation/clone: document ignored configuration variables","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-06-16T18:23:48Z","receivedAt":"2017-06-16T18:24:00Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Jun 16 2017, SZEDER Gábor jotted:\n\n> Due to limitations/bugs in the current implementation, some\n> configuration variables specified via 'git clone -c var=val' (or 'git\n> -c var=val clone') are ignored during the initial fetch and checkout.\n>\n> Let the users know which configuration variables are known to be\n> ignored ('remote.origin.mirror' and 'remote.origin.tagOpt') under the\n> documentation of 'git clone -c'.\n>\n> Signed-off-by: SZEDER Gábor \n> ---\n> Documentation/git-clone.txt | 5 +++++\n> 1 file changed, 5 insertions(+)\n>\n> diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt\n> index ec41d3d69..5ceccb258 100644\n> --- a/Documentation/git-clone.txt\n> +++ b/Documentation/git-clone.txt\n> @@ -186,6 +186,11 @@ objects from the source repository into a pack in the cloned repository.\n> \tvalues are given for the same key, each value will be written to\n> \tthe config file. This makes it safe, for example, to add\n> \tadditional fetch refspecs to the origin remote.\n> ++\n> +Due to limitations if the current implementation, some configuration\n> +variables do not take effect until after the initial fetch and checkout.\n> +Configuration variables known to not take effect are:\n> +`remote..mirror` and `remote..tagOpt`.\n>\n> --depth ::\n> \tCreate a 'shallow' clone with a history truncated to the\n\nJust so we're all on the same page, in\nCACBZZX740rcQKnfkRXgn0+fmeUDaWL-Kz5WzKeyUvBhXWPwPhg@mail.gmail.com I had\nthe feedback that this patch didn't make sense on top of 2.13.0 since we\nhave --no-tags now which should be documented here, but you replied in\nCAM0VKjmz7MpVt3oPBuwHiXNoLkZmdmrZ66ggk+aY5-4oVkE35A@mail.gmail.com in an\nanswer I understood to mean that this was a patch not meant for master,\nbut for the main track.\n\nBut this is now cooking in pu, Junio: is it clear that this patchu\nas-cooking ideally shouldn't land in next/master without the fix on top\nwhich I mentioned in my mail above? I can just submit that as a patch on\ntop, but I'm confused about the current state with this cooking in pu,\nso I thought I'd ask first how this should be handled.\n"},{"id":"322481","messageId":"20170616203745.GZ133952@aiede.mtv.corp.google.com","threadId":"45962","inReplyTo":"20170616173849.8071-2-szeder.dev@gmail.com","subject":"Re: [PATCHv5 1/2] clone: respect additional configured fetch refspecs during initial fetch","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-06-16T20:37:45Z","receivedAt":"2017-06-16T20:37:54Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"SZEDER Gábor wrote:\n\n> Helped-by: Jeff King \n> Signed-off-by: SZEDER Gábor \n> ---\n> builtin/clone.c | 36 +++++++++++++++++++-----------------\n> remote.c | 13 +++++++++++++\n> remote.h | 1 +\n> t/t5611-clone-config.sh | 47 +++++++++++++++++++++++++++++++++++++++++++++++\n> 4 files changed, 80 insertions(+), 17 deletions(-)\n\nReviewed-by: Jonathan Nieder \n\nThanks.\n"},{"id":"322482","messageId":"20170616203807.GA133952@aiede.mtv.corp.google.com","threadId":"45962","inReplyTo":"20170616173849.8071-3-szeder.dev@gmail.com","subject":"Re: [PATCHv5 2/2] Documentation/clone: document ignored configuration variables","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-06-16T20:38:07Z","receivedAt":"2017-06-16T20:38:15Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"SZEDER Gábor wrote:\n\n> Due to limitations/bugs in the current implementation, some\n> configuration variables specified via 'git clone -c var=val' (or 'git\n> -c var=val clone') are ignored during the initial fetch and checkout.\n>\n> Let the users know which configuration variables are known to be\n> ignored ('remote.origin.mirror' and 'remote.origin.tagOpt') under the\n> documentation of 'git clone -c'.\n>\n> Signed-off-by: SZEDER Gábor \n> ---\n> Documentation/git-clone.txt | 5 +++++\n> 1 file changed, 5 insertions(+)\n\nReviewed-by: Jonathan Nieder \n"},{"id":"322483","messageId":"20170616204109.GB133952@aiede.mtv.corp.google.com","threadId":"45962","inReplyTo":"87wp8b94gb.fsf@gmail.com","subject":"Re: [PATCHv5 2/2] Documentation/clone: document ignored configuration variables","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-06-16T20:41:09Z","receivedAt":"2017-06-16T20:41:18Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ævar Arnfjörð Bjarmason wrote:\n> On Fri, Jun 16 2017, SZEDER Gábor jotted:\n\n>> --- a/Documentation/git-clone.txt\n>> +++ b/Documentation/git-clone.txt\n>> @@ -186,6 +186,11 @@ objects from the source repository into a pack in the cloned repository.\n>> \tvalues are given for the same key, each value will be written to\n>> \tthe config file. This makes it safe, for example, to add\n>> \tadditional fetch refspecs to the origin remote.\n>> ++\n>> +Due to limitations if the current implementation, some configuration\n>> +variables do not take effect until after the initial fetch and checkout.\n>> +Configuration variables known to not take effect are:\n>> +`remote..mirror` and `remote..tagOpt`.\n>>\n>> --depth ::\n>> \tCreate a 'shallow' clone with a history truncated to the\n[...]\n> But this is now cooking in pu, Junio: is it clear that this patchu\n> as-cooking ideally shouldn't land in next/master without the fix on top\n> which I mentioned in my mail above? I can just submit that as a patch on\n> top, but I'm confused about the current state with this cooking in pu,\n> so I thought I'd ask first how this should be handled.\n\nI think it's simplest to write a patch on top that discusses --no-tags.\nThat way, Junio (and anyone else applying the patch) has the\nflexibility to apply or cherry-pick this change to old branches\nwithout the --no-tags discussion and newer branches with it.\n\nWould you like to write it (or suggest wording), or would you prefer\nif someone else does?\n\nThanks,\nJonathan\n"},{"id":"322487","messageId":"87vanv8wq4.fsf@gmail.com","threadId":"45962","inReplyTo":"20170616204109.GB133952@aiede.mtv.corp.google.com","subject":"Re: [PATCHv5 2/2] Documentation/clone: document ignored configuration variables","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-06-16T21:10:43Z","receivedAt":"2017-06-16T21:10:52Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Jun 16 2017, Jonathan Nieder jotted:\n\n> Ævar Arnfjörð Bjarmason wrote:\n>> On Fri, Jun 16 2017, SZEDER Gábor jotted:\n>\n>>> --- a/Documentation/git-clone.txt\n>>> +++ b/Documentation/git-clone.txt\n>>> @@ -186,6 +186,11 @@ objects from the source repository into a pack in the cloned repository.\n>>> \tvalues are given for the same key, each value will be written to\n>>> \tthe config file. This makes it safe, for example, to add\n>>> \tadditional fetch refspecs to the origin remote.\n>>> ++\n>>> +Due to limitations if the current implementation, some configuration\n>>> +variables do not take effect until after the initial fetch and checkout.\n>>> +Configuration variables known to not take effect are:\n>>> +`remote..mirror` and `remote..tagOpt`.\n>>>\n>>> --depth ::\n>>> \tCreate a 'shallow' clone with a history truncated to the\n> [...]\n>> But this is now cooking in pu, Junio: is it clear that this patchu\n>> as-cooking ideally shouldn't land in next/master without the fix on top\n>> which I mentioned in my mail above? I can just submit that as a patch on\n>> top, but I'm confused about the current state with this cooking in pu,\n>> so I thought I'd ask first how this should be handled.\n>\n> I think it's simplest to write a patch on top that discusses --no-tags.\n> That way, Junio (and anyone else applying the patch) has the\n> flexibility to apply or cherry-pick this change to old branches\n> without the --no-tags discussion and newer branches with it.\n>\n> Would you like to write it (or suggest wording), or would you prefer\n> if someone else does?\n\nI can do that no problem.\n\nI just first wanted to clarify what the status of this was, from\nSZEDER's comments in the referenced E-Mails I had the impression that\nthis was only meant for an old maintenance release:\n\n SZEDER: \"I assume because, as a bugfix, it will be included in\n maintenance releases for older releases, and those won't have the\n '--no-tags' option.\"\n\nBut thinking about it I don't see why we'd be doing such minor doc\nchanges in an old maintenance release. I haven't read the whole backlog\nof this topic though, so maybe I missed something.\n\nI initially suggested just adding \"Instead supply the --mirror and\n--no-tags options, respectively\" to the patch quoted above.\n\nBut actually, thinking about this again now, and being recently familiar\nwith this code after having implemented  on --no-tags, I think this\nwhole wording is just misleading. It makes it sound as though git clone\nis init + fetch, and that the initial fetch just ignores these two\nspecific options because of some quirk of the implementation.\n\nIn reality clone doesn't use fetch at all, they just share some of the\nunderlying fetch machinery.\n\nI think something like this as a replacement is better, assuming this\nreally needs to be applied to pre-2.13.0:\n\n diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt\n index 35cc34b2fb..2169e5c97f 100644\n --- a/Documentation/git-clone.txt\n +++ b/Documentation/git-clone.txt\n @@ -189,6 +189,14 @@ objects from the source repository into a pack in the cloned repository.\n values are given for the same key, each value will be written to\n the config file. This makes it safe, for example, to add\n additional fetch refspecs to the origin remote.\n ++\n +The underlying implementation of `git clone` isn't equivalent to `git\n +init` followed by a `git fetch`, but the two share some of the\n +underlying fetch machinery. Because of this, setting configuration\n +variables which would impact `git fetch` doesn't have any effect on\n +`git clone` at all. For example, setting `remote..mirror` and\n +`remote..tagOpt` will do to change how the initial fetch is\n +carried out.\n\n --depth ::\n Create a 'shallow' clone with a history truncated to the\n\nOr, in case this just needs to be applied on top of master we can\nmention --no-tags:\n\n diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt\n index 83c8e9b394..52a371176e 100644\n --- a/Documentation/git-clone.txt\n +++ b/Documentation/git-clone.txt\n @@ -189,6 +189,16 @@ objects from the source repository into a pack in the cloned repository.\n values are given for the same key, each value will be written to\n the config file. This makes it safe, for example, to add\n additional fetch refspecs to the origin remote.\n ++\n +The underlying implementation of `git clone` isn't equivalent to `git\n +init` followed by a `git fetch`, but the two share some of the\n +underlying fetch machinery. Because of this, setting configuration\n +variables which would impact `git fetch` doesn't have any effect on\n +`git clone` at all.\n ++\n +For example, setting `remote..mirror` and `remote..tagOpt`\n +will do to change how the initial fetch is carried out. Instead supply\n +the --mirror and --no-tags options, respectively.\n\n --depth ::\n Create a 'shallow' clone with a history truncated to the\n"},{"id":"322496","messageId":"xmqq1sqjwpn9.fsf@gitster.mtv.corp.google.com","threadId":"45962","inReplyTo":"20170616173849.8071-3-szeder.dev@gmail.com","subject":"Re: [PATCHv5 2/2] Documentation/clone: document ignored configuration variables","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-16T22:09:46Z","receivedAt":"2017-06-16T22:09:53Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> Due to limitations/bugs in the current implementation, some\n> configuration variables specified via 'git clone -c var=val' (or 'git\n> -c var=val clone') are ignored during the initial fetch and checkout.\n>\n> Let the users know which configuration variables are known to be\n> ignored ('remote.origin.mirror' and 'remote.origin.tagOpt') under the\n> documentation of 'git clone -c'.\n>\n> Signed-off-by: SZEDER Gábor \n> ---\n> Documentation/git-clone.txt | 5 +++++\n> 1 file changed, 5 insertions(+)\n>\n> diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt\n> index ec41d3d69..5ceccb258 100644\n> --- a/Documentation/git-clone.txt\n> +++ b/Documentation/git-clone.txt\n> @@ -186,6 +186,11 @@ objects from the source repository into a pack in the cloned repository.\n> \tvalues are given for the same key, each value will be written to\n> \tthe config file. This makes it safe, for example, to add\n> \tadditional fetch refspecs to the origin remote.\n> ++\n> +Due to limitations if the current implementation, some configuration\n\ns/if/of/ (will squash in while queuing).\n\n> +variables do not take effect until after the initial fetch and checkout.\n> +Configuration variables known to not take effect are:\n> +`remote..mirror` and `remote..tagOpt`.\n> \n> --depth ::\n> \tCreate a 'shallow' clone with a history truncated to the\n"},{"id":"322497","messageId":"CAM0VKjm0MY-DrOX+LuPqvk1L5od3g+SFA_k6dh0QT7ZT7HSh-A@mail.gmail.com","threadId":"45962","inReplyTo":"87vanv8wq4.fsf@gmail.com","subject":"Re: [PATCHv5 2/2] Documentation/clone: document ignored configuration variables","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-06-16T22:15:34Z","receivedAt":"2017-06-16T22:15:40Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Jun 16, 2017 at 11:10 PM, Ævar Arnfjörð Bjarmason\n wrote:\n\n> diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt\n> index 35cc34b2fb..2169e5c97f 100644\n> --- a/Documentation/git-clone.txt\n> +++ b/Documentation/git-clone.txt\n> @@ -189,6 +189,14 @@ objects from the source repository into a pack in the cloned repository.\n> values are given for the same key, each value will be written to\n> the config file. This makes it safe, for example, to add\n> additional fetch refspecs to the origin remote.\n> ++\n> +The underlying implementation of `git clone` isn't equivalent to `git\n> +init` followed by a `git fetch`, but the two share some of the\n> +underlying fetch machinery.\n\nI think this is unnecessarily technical and detailed to be included\nunder the docs of the '--config' option.\n\n> Because of this, setting configuration\n> +variables which would impact `git fetch` doesn't have any effect on\n> +`git clone` at all.\n\nThis is not true, several such configuration variables work even\nduring cloning, see:\n\n http://public-inbox.org/git/CAM0VKjkSMnemwPbFihXiQui3wm_wYmQeQmgYrFs5bfsH1jMg1A@mail.gmail.com/\n"},{"id":"322500","messageId":"20170617112228.vugswym4o4owf6wj@sigill.intra.peff.net","threadId":"45962","inReplyTo":"20170616173849.8071-2-szeder.dev@gmail.com","subject":"Re: [PATCHv5 1/2] clone: respect additional configured fetch refspecs during initial fetch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-17T11:22:28Z","receivedAt":"2017-06-17T11:22:35Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 16, 2017 at 07:38:48PM +0200, SZEDER Gábor wrote:\n\n> +void add_and_parse_fetch_refspec(struct remote *remote, const char *refspec)\n> +{\n> +\tstruct refspec *rs;\n> +\n> +\tadd_fetch_refspec(remote, refspec);\n> +\trs = parse_fetch_refspec(1, &refspec);\n> +\tREALLOC_ARRAY(remote->fetch, remote->fetch_refspec_nr);\n> +\tremote->fetch[remote->fetch_refspec_nr - 1] = *rs;\n> +\n> +\t/* Not free_refspecs(), as we copied its pointers above */\n> +\tfree(rs);\n> +}\n\nI'd still prefer this to have:\n\n if (!remote->fetch && remote->fetch_refspec_nr)\n\tBUG(\"attempt to add refspec to uninitialized list\");\n\nat the top, as otherwise this case writes garbage into remote->fetch[0].\n\nI see you have another series dealing with the lazy parsing, but I\nhaven't looked at it yet (hopefully this danger would just go away after\nthat).\n\nOther than that, the patch looks fine to me.\n\n-Peff\n"},{"id":"323011","messageId":"xmqqmv8zhdap.fsf@gitster.mtv.corp.google.com","threadId":"45962","inReplyTo":"20170617112228.vugswym4o4owf6wj@sigill.intra.peff.net","subject":"Re: [PATCHv5 1/2] clone: respect additional configured fetch refspecs during initial fetch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-22T22:23:42Z","receivedAt":"2017-06-22T22:23:50Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I'd still prefer this to have:\n>\n> if (!remote->fetch && remote->fetch_refspec_nr)\n> \tBUG(\"attempt to add refspec to uninitialized list\");\n>\n> at the top, as otherwise this case writes garbage into remote->fetch[0].\n>\n> I see you have another series dealing with the lazy parsing, but I\n> haven't looked at it yet (hopefully this danger would just go away after\n> that).\n>\n> Other than that, the patch looks fine to me.\n\nSZEDER? As long as the end result together with two series are\nsafe, I do not have a strong preference, but given that the other\none is a lot more invasive change [*1*], I think it is nicer to have\nthis two-patch series already safe without the other one.\n\nWhat's your take on Peff's point?\n\n\n[Footnote]\n\n*1* Especially the other branch does not merge cleanly into 'pu' and\n I haven't managed to include it in my tree yet.\n"},{"id":"326185","messageId":"xmqqbmnly5ja.fsf@gitster.mtv.corp.google.com","threadId":"45962","inReplyTo":"xmqqmv8zhdap.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv5 1/2] clone: respect additional configured fetch refspecs during initial fetch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-12T00:48:57Z","receivedAt":"2017-08-12T00:49:12Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jeff King writes:\n>\n>> I'd still prefer this to have:\n>>\n>> if (!remote->fetch && remote->fetch_refspec_nr)\n>> \tBUG(\"attempt to add refspec to uninitialized list\");\n>>\n>> at the top, as otherwise this case writes garbage into remote->fetch[0].\n>>\n>> I see you have another series dealing with the lazy parsing, but I\n>> haven't looked at it yet (hopefully this danger would just go away after\n>> that).\n>>\n>> Other than that, the patch looks fine to me.\n>\n> SZEDER? As long as the end result together with two series are\n> safe, I do not have a strong preference, but given that the other\n> one is a lot more invasive change [*1*], I think it is nicer to have\n> this two-patch series already safe without the other one.\n>\n> What's your take on Peff's point?\n>\n> [Footnote]\n>\n> *1* Especially the other branch does not merge cleanly into 'pu' and\n> I haven't managed to include it in my tree yet.\n\nAfter infinite amount of time has passed, I no longer even remember\nwhat \"the other\" topic was X-<. I'll tentatively eject these two\npatches from my tree in preparation for starting the cycle after Git\n2.14 early next week, but hopefully you can find time and energy to\nresurrect it.\n\nOr somebody else may be interested to take the topic over--which is\nalso fine by me, too.\n\nThanks.\n"}]}