{"thread":{"id":"46200","subject":"[PATCH 2/5] remote.c: don't pass copies of refspecs to add_{fetch,push}_refspec()","startedAt":"2017-06-16T19:28:59Z","lastAt":"2017-06-19T20:07:43Z","messageCount":12,"participants":["SZEDER Gábor","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"322474","messageId":"20170616192837.11035-3-szeder.dev@gmail.com","threadId":"46200","inReplyTo":"20170616192837.11035-1-szeder.dev@gmail.com","subject":"[PATCH 2/5] remote.c: don't pass copies of refspecs to add_{fetch,push}_refspec()","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-06-16T19:28:34Z","receivedAt":"2017-06-16T19:28:59Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Currently neither add_fetch_refspec() nor add_push_refspec() duplicate\nthe refspecs before appending them to an array of refspecs, therefore\nall their callers pass them copies of refspecs.  Soon we won't store\nrefspecs as strings, therefore passing duplicated strings to these\nfunctions will not be necessary.\n\nPerform the copying inside these functions and modify their callers to\neither not pass duplicates or free() them to avoid leaks.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n builtin/clone.c |  3 +--\n remote.c        | 21 ++++++++++++++-------\n 2 files changed, 15 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 869e093ff..5b72d853f 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -1054,8 +1054,7 @@ 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,\n-\t\t\t\t    strbuf_detach(&default_refspec, NULL));\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);\ndiff --git a/remote.c b/remote.c\nindex a021decee..d23518afd 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -91,7 +91,7 @@ static void add_push_refspec(struct remote *remote, const char *ref)\n \tALLOC_GROW(remote->push_refspec,\n \t\t   remote->push_refspec_nr + 1,\n \t\t   remote->push_refspec_alloc);\n-\tremote->push_refspec[remote->push_refspec_nr++] = ref;\n+\tremote->push_refspec[remote->push_refspec_nr++] = strdup(ref);\n }\n \n static void add_fetch_refspec(struct remote *remote, const char *ref)\n@@ -99,7 +99,7 @@ static void add_fetch_refspec(struct remote *remote, const char *ref)\n \tALLOC_GROW(remote->fetch_refspec,\n \t\t   remote->fetch_refspec_nr + 1,\n \t\t   remote->fetch_refspec_alloc);\n-\tremote->fetch_refspec[remote->fetch_refspec_nr++] = ref;\n+\tremote->fetch_refspec[remote->fetch_refspec_nr++] = strdup(ref);\n }\n \n static void add_url(struct remote *remote, const char *url)\n@@ -265,9 +265,9 @@ static void read_remotes_file(struct remote *remote)\n \t\tif (skip_prefix(buf.buf, \"URL:\", &v))\n \t\t\tadd_url_alias(remote, xstrdup(skip_spaces(v)));\n \t\telse if (skip_prefix(buf.buf, \"Push:\", &v))\n-\t\t\tadd_push_refspec(remote, xstrdup(skip_spaces(v)));\n+\t\t\tadd_push_refspec(remote, skip_spaces(v));\n \t\telse if (skip_prefix(buf.buf, \"Pull:\", &v))\n-\t\t\tadd_fetch_refspec(remote, xstrdup(skip_spaces(v)));\n+\t\t\tadd_fetch_refspec(remote, skip_spaces(v));\n \t}\n \tstrbuf_release(&buf);\n \tfclose(f);\n@@ -306,15 +306,20 @@ static void read_branches_file(struct remote *remote)\n \t\tfrag = \"master\";\n \n \tadd_url_alias(remote, strbuf_detach(&buf, NULL));\n-\tadd_fetch_refspec(remote, xstrfmt(\"refs/heads/%s:refs/heads/%s\",\n-\t\t\t\t\t  frag, remote->name));\n+\n+\tstrbuf_addf(&buf, \"refs/heads/%s:refs/heads/%s\", frag, remote->name);\n+\tadd_fetch_refspec(remote, buf.buf);\n+\tstrbuf_reset(&buf);\n \n \t/*\n \t * Cogito compatible push: push current HEAD to remote #branch\n \t * (master if missing)\n \t */\n-\tadd_push_refspec(remote, xstrfmt(\"HEAD:refs/heads/%s\", frag));\n+\tstrbuf_addf(&buf, \"HEAD:refs/heads/%s\", frag);\n+\tadd_push_refspec(remote, buf.buf);\n \tremote->fetch_tags = 1; /* always auto-follow */\n+\n+\tstrbuf_release(&buf);\n }\n \n static int handle_config(const char *key, const char *value, void *cb)\n@@ -398,11 +403,13 @@ static int handle_config(const char *key, const char *value, void *cb)\n \t\tif (git_config_string(&v, key, value))\n \t\t\treturn -1;\n \t\tadd_push_refspec(remote, v);\n+\t\tfree((char*)v);\n \t} else if (!strcmp(subkey, \"fetch\")) {\n \t\tconst char *v;\n \t\tif (git_config_string(&v, key, value))\n \t\t\treturn -1;\n \t\tadd_fetch_refspec(remote, v);\n+\t\tfree((char*)v);\n \t} else if (!strcmp(subkey, \"receivepack\")) {\n \t\tconst char *v;\n \t\tif (git_config_string(&v, key, value))\n-- \n2.13.1.505.g7cc9fcafb\n\n"},{"id":"322475","messageId":"20170616192837.11035-5-szeder.dev@gmail.com","threadId":"46200","inReplyTo":"20170616192837.11035-1-szeder.dev@gmail.com","subject":"[PATCH 4/5] remote.c: eliminate remote->fetch_refspec","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-06-16T19:28:36Z","receivedAt":"2017-06-16T19:29:01Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"'struct remote' stores fetch refspecs twice: once in their original\nstring form in remote->fetch_refspecs and once in their parsed form in\nremote->fetch.\n\nThe main reason for this is that we want to read the configuration\nonly once, but we don't want to error out while doing so because of a\nbogus refspec in a remote that we aren't accessing at all.  Therefore,\nwhen the configuration is read all refspecs are simply stored in\nstring arrays for each remote, and then refspecs of a particular\nremote are parsed lazily, only when that remote is actually accessed.\n\nHowever, storing refspecs in both forms has some drawbacks:\n\n  - The same information is stored twice, wasting memory.\n  - remote->fetch_refspecs, i.e. the string array is conveniently\n    ALLOC_GROW()-able with associated 'fetch_refspec_{nr,alloc}'\n    fields, but remote->fetch is not.\n  - Wherever remote->fetch are accessed, the number of parsed refspecs\n    in there is specified by remote->fetch_refspec_nr.  This requires\n    us to keep the two arrays in sync and makes adding additional\n    refspecs cumbersome and error prone.\n\nSo eliminate remote->fetch_refspec and parse fetch refspecs right away\nwhile they are being read from the configuration.  However, to avoid\nerroring out on bogus refspecs in \"uninteresting\" remotes, parse them\ngently: instead of die()ing, store the problematic refspec in\nremote->bogus_refspec, so it will be available later when that remote\nis actually accessed and we can use it in the error message.  Make\nremote->fetch ALLOC_GROW()-able.  Add a new add_fetch_refspec()\nfunction to the remote API, replacing an old function with the same\nname and add_and_parse_fetch_refspec(), to encompass memory\nmanagement of remote->fetch and parsing the new refspec (gently or\notherwise).  Update all sites accessing fetch-refspec-related fields\nto use the new field names.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n builtin/clone.c  |  6 +++---\n builtin/fetch.c  | 20 ++++++++++----------\n builtin/remote.c | 18 +++++++++---------\n remote.c         | 55 +++++++++++++++++++++++++++++--------------------------\n remote.h         | 23 ++++++++++++++++++-----\n 5 files changed, 69 insertions(+), 53 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 5b72d853f..05cc57f79 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -1054,7 +1054,7 @@ 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_fetch_refspec(remote, default_refspec.buf, 0);\n \n \ttransport = transport_get(remote, remote->url[0]);\n \ttransport_set_verbosity(transport, option_verbosity, option_progress);\n@@ -1106,8 +1106,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, remote->fetch,\n-\t\t\t\t\t       remote->fetch_refspec_nr);\n+\t\tmapped_refs = wanted_peer_refs(refs, remote->fetch.rs,\n+\t\t\t\t\t       remote->fetch.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\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 100248c5a..0f791a12f 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -360,8 +360,8 @@ static struct ref *get_ref_map(struct transport *transport,\n \t\t\tfetch_refspec = parse_fetch_refspec(refmap_nr, refmap_array);\n \t\t\tfetch_refspec_nr = refmap_nr;\n \t\t} else {\n-\t\t\tfetch_refspec = transport->remote->fetch;\n-\t\t\tfetch_refspec_nr = transport->remote->fetch_refspec_nr;\n+\t\t\tfetch_refspec = transport->remote->fetch.rs;\n+\t\t\tfetch_refspec_nr = transport->remote->fetch.nr;\n \t\t}\n \n \t\tfor (i = 0; i < fetch_refspec_nr; i++)\n@@ -374,16 +374,16 @@ static struct ref *get_ref_map(struct transport *transport,\n \t\tstruct branch *branch = branch_get(NULL);\n \t\tint has_merge = branch_has_merge_config(branch);\n \t\tif (remote &&\n-\t\t    (remote->fetch_refspec_nr ||\n+\t\t    (remote->fetch.nr ||\n \t\t     /* Note: has_merge implies non-NULL branch->remote_name */\n \t\t     (has_merge && !strcmp(branch->remote_name, remote->name)))) {\n-\t\t\tfor (i = 0; i < remote->fetch_refspec_nr; i++) {\n-\t\t\t\tget_fetch_map(remote_refs, &remote->fetch[i], &tail, 0);\n-\t\t\t\tif (remote->fetch[i].dst &&\n-\t\t\t\t    remote->fetch[i].dst[0])\n+\t\t\tfor (i = 0; i < remote->fetch.nr; i++) {\n+\t\t\t\tget_fetch_map(remote_refs, &remote->fetch.rs[i], &tail, 0);\n+\t\t\t\tif (remote->fetch.rs[i].dst &&\n+\t\t\t\t    remote->fetch.rs[i].dst[0])\n \t\t\t\t\t*autotags = 1;\n \t\t\t\tif (!i && !has_merge && ref_map &&\n-\t\t\t\t    !remote->fetch[0].pattern)\n+\t\t\t\t    !remote->fetch.rs[0].pattern)\n \t\t\t\t\tref_map->fetch_head_status = FETCH_HEAD_MERGE;\n \t\t\t}\n \t\t\t/*\n@@ -1117,8 +1117,8 @@ static int do_fetch(struct transport *transport,\n \t\tif (ref_count) {\n \t\t\tprune_refs(refs, ref_count, ref_map, transport->url);\n \t\t} else {\n-\t\t\tprune_refs(transport->remote->fetch,\n-\t\t\t\t   transport->remote->fetch_refspec_nr,\n+\t\t\tprune_refs(transport->remote->fetch.rs,\n+\t\t\t\t   transport->remote->fetch.nr,\n \t\t\t\t   ref_map,\n \t\t\t\t   transport->url);\n \t\t}\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 7f0072fe5..d61daa5e8 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -331,10 +331,10 @@ static int get_ref_states(const struct ref *remote_refs, struct ref_states *stat\n \tstruct ref *ref, *stale_refs;\n \tint i;\n \n-\tfor (i = 0; i < states->remote->fetch_refspec_nr; i++)\n-\t\tif (get_fetch_map(remote_refs, states->remote->fetch + i, &tail, 1))\n+\tfor (i = 0; i < states->remote->fetch.nr; i++)\n+\t\tif (get_fetch_map(remote_refs, states->remote->fetch.rs + i, &tail, 1))\n \t\t\tdie(_(\"Could not get fetch map for refspec %s\"),\n-\t\t\t      refspec_to_string(&states->remote->fetch[i]));\n+\t\t\t      refspec_to_string(&states->remote->fetch.rs[i]));\n \n \tstates->new.strdup_strings = 1;\n \tstates->tracked.strdup_strings = 1;\n@@ -345,8 +345,8 @@ static int get_ref_states(const struct ref *remote_refs, struct ref_states *stat\n \t\telse\n \t\t\tstring_list_append(&states->tracked, abbrev_branch(ref->name));\n \t}\n-\tstale_refs = get_stale_heads(states->remote->fetch,\n-\t\t\t\t     states->remote->fetch_refspec_nr, fetch_map);\n+\tstale_refs = get_stale_heads(states->remote->fetch.rs,\n+\t\t\t\t     states->remote->fetch.nr, fetch_map);\n \tfor (ref = stale_refs; ref; ref = ref->next) {\n \t\tstruct string_list_item *item =\n \t\t\tstring_list_append(&states->stale, abbrev_branch(ref->name));\n@@ -591,8 +591,8 @@ static int migrate_file(struct remote *remote)\n \t}\n \tstrbuf_reset(&buf);\n \tstrbuf_addf(&buf, \"remote.%s.fetch\", remote->name);\n-\tfor (i = 0; i < remote->fetch_refspec_nr; i++) {\n-\t\tstrbuf_add_refspec(&refspec, &remote->fetch[i]);\n+\tfor (i = 0; i < remote->fetch.nr; i++) {\n+\t\tstrbuf_add_refspec(&refspec, &remote->fetch.rs[i]);\n \t\tgit_config_set_multivar(buf.buf, refspec.buf, \"^$\", 0);\n \t\tstrbuf_reset(&refspec);\n \t}\n@@ -651,11 +651,11 @@ static int mv(int argc, const char **argv)\n \tstrbuf_addf(&buf, \"remote.%s.fetch\", rename.new);\n \tgit_config_set_multivar(buf.buf, NULL, NULL, 1);\n \tstrbuf_addf(&old_remote_context, \":refs/remotes/%s/\", rename.old);\n-\tfor (i = 0; i < oldremote->fetch_refspec_nr; i++) {\n+\tfor (i = 0; i < oldremote->fetch.nr; i++) {\n \t\tchar *ptr;\n \n \t\tstrbuf_reset(&buf2);\n-\t\tstrbuf_add_refspec(&buf2, &oldremote->fetch[i]);\n+\t\tstrbuf_add_refspec(&buf2, &oldremote->fetch.rs[i]);\n \t\tptr = strstr(buf2.buf, old_remote_context.buf);\n \t\tif (ptr) {\n \t\t\trefspec_updated = 1;\ndiff --git a/remote.c b/remote.c\nindex fc1d3cf7a..952000b61 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -94,14 +94,6 @@ static void add_push_refspec(struct remote *remote, const char *ref)\n \tremote->push_refspec[remote->push_refspec_nr++] = strdup(ref);\n }\n \n-static void add_fetch_refspec(struct remote *remote, const char *ref)\n-{\n-\tALLOC_GROW(remote->fetch_refspec,\n-\t\t   remote->fetch_refspec_nr + 1,\n-\t\t   remote->fetch_refspec_alloc);\n-\tremote->fetch_refspec[remote->fetch_refspec_nr++] = strdup(ref);\n-}\n-\n static void add_url(struct remote *remote, const char *url)\n {\n \tALLOC_GROW(remote->url, remote->url_nr + 1, remote->url_alloc);\n@@ -267,7 +259,7 @@ static void read_remotes_file(struct remote *remote)\n \t\telse if (skip_prefix(buf.buf, \"Push:\", &v))\n \t\t\tadd_push_refspec(remote, skip_spaces(v));\n \t\telse if (skip_prefix(buf.buf, \"Pull:\", &v))\n-\t\t\tadd_fetch_refspec(remote, skip_spaces(v));\n+\t\t\tadd_fetch_refspec(remote, skip_spaces(v), 1);\n \t}\n \tstrbuf_release(&buf);\n \tfclose(f);\n@@ -308,7 +300,7 @@ static void read_branches_file(struct remote *remote)\n \tadd_url_alias(remote, strbuf_detach(&buf, NULL));\n \n \tstrbuf_addf(&buf, \"refs/heads/%s:refs/heads/%s\", frag, remote->name);\n-\tadd_fetch_refspec(remote, buf.buf);\n+\tadd_fetch_refspec(remote, buf.buf, 1);\n \tstrbuf_reset(&buf);\n \n \t/*\n@@ -408,7 +400,7 @@ static int handle_config(const char *key, const char *value, void *cb)\n \t\tconst char *v;\n \t\tif (git_config_string(&v, key, value))\n \t\t\treturn -1;\n-\t\tadd_fetch_refspec(remote, v);\n+\t\tadd_fetch_refspec(remote, v, 1);\n \t\tfree((char*)v);\n \t} else if (!strcmp(subkey, \"receivepack\")) {\n \t\tconst char *v;\n@@ -630,17 +622,28 @@ 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+static int add_refspec(struct remote *remote, const char *refspec,\n+\t\t       int fetch, int gently)\n {\n-\tstruct refspec *rs;\n+\tstruct refspec_array *rsa = fetch ? &remote->fetch : NULL;\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+\tALLOC_GROW(rsa->rs, rsa->nr + 1, rsa->alloc);\n \n-\t/* Not free_refspecs(), as we copied its pointers above */\n-\tfree(rs);\n+\tif (parse_one_refspec(&rsa->rs[rsa->nr], refspec, fetch, gently) < 0) {\n+\t\tif (gently) {\n+\t\t\tif (!remote->bogus_refspec)\n+\t\t\t\tremote->bogus_refspec = strdup(refspec);\n+\t\t\treturn -1;\n+\t\t} else\n+\t\t\tdie(\"Invalid refspec: '%s'\", refspec);\n+\t}\n+\trsa->nr++;\n+\treturn 0;\n+}\n+\n+int add_fetch_refspec(struct remote *remote, const char *refspec, int gently)\n+{\n+\treturn add_refspec(remote, refspec, 1, gently);\n }\n \n struct refspec *parse_push_refspec(int nr_refspec, const char **refspec)\n@@ -725,7 +728,8 @@ static struct remote *remote_get_1(const char *name,\n \t\tadd_url_alias(ret, name);\n \tif (!valid_remote(ret))\n \t\treturn NULL;\n-\tret->fetch = parse_fetch_refspec(ret->fetch_refspec_nr, ret->fetch_refspec);\n+\tif (ret->bogus_refspec)\n+\t\tdie(\"Invalid refspec '%s'\", ret->bogus_refspec);\n \tret->push = parse_push_refspec(ret->push_refspec_nr, ret->push_refspec);\n \treturn ret;\n }\n@@ -757,9 +761,8 @@ int for_each_remote(each_remote_fn fn, void *priv)\n \t\tstruct remote *r = remotes[i];\n \t\tif (!r)\n \t\t\tcontinue;\n-\t\tif (!r->fetch)\n-\t\t\tr->fetch = parse_fetch_refspec(r->fetch_refspec_nr,\n-\t\t\t\t\t\t       r->fetch_refspec);\n+\t\tif (r->bogus_refspec)\n+\t\t\tdie(\"Invalid refspec '%s'\", r->bogus_refspec);\n \t\tif (!r->push)\n \t\t\tr->push = parse_push_refspec(r->push_refspec_nr,\n \t\t\t\t\t\t     r->push_refspec);\n@@ -961,7 +964,7 @@ char *refspec_to_string(const struct refspec *refspec)\n \n int remote_find_tracking(struct remote *remote, struct refspec *refspec)\n {\n-\treturn query_refspecs(remote->fetch, remote->fetch_refspec_nr, refspec);\n+\treturn query_refspecs(remote->fetch.rs, remote->fetch.nr, refspec);\n }\n \n static struct ref *alloc_ref_with_prefix(const char *prefix, size_t prefixlen,\n@@ -1756,7 +1759,7 @@ static const char *tracking_for_push_dest(struct remote *remote,\n {\n \tchar *ret;\n \n-\tret = apply_refspecs(remote->fetch, remote->fetch_refspec_nr, refname);\n+\tret = apply_refspecs(remote->fetch.rs, remote->fetch.nr, refname);\n \tif (!ret)\n \t\treturn error_buf(err,\n \t\t\t\t _(\"push destination '%s' on remote '%s' has no local tracking branch\"),\n@@ -2373,7 +2376,7 @@ static int remote_tracking(struct remote *remote, const char *refname,\n {\n \tchar *dst;\n \n-\tdst = apply_refspecs(remote->fetch, remote->fetch_refspec_nr, refname);\n+\tdst = apply_refspecs(remote->fetch.rs, remote->fetch.nr, refname);\n \tif (!dst)\n \t\treturn -1; /* no tracking ref for refname at remote */\n \tif (read_ref(dst, oid->hash))\ndiff --git a/remote.h b/remote.h\nindex ee6c432d0..416a08501 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -11,6 +11,11 @@ enum {\n \tREMOTE_BRANCHES\n };\n \n+struct refspec_array {\n+\tstruct refspec *rs;\n+\tint nr, alloc;\n+};\n+\n struct remote {\n \tstruct hashmap_entry ent;  /* must be first */\n \n@@ -32,10 +37,10 @@ struct remote {\n \tint push_refspec_nr;\n \tint push_refspec_alloc;\n \n-\tconst char **fetch_refspec;\n-\tstruct refspec *fetch;\n-\tint fetch_refspec_nr;\n-\tint fetch_refspec_alloc;\n+\tstruct refspec_array fetch;\n+\n+\t/* Copy of the first bogus fetch refspec we couldn't parse */\n+\tconst char *bogus_refspec;\n \n \t/*\n \t * -1 to never fetch tags\n@@ -169,7 +174,15 @@ 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+ * Add a fetch refspec to a remote.\n+ * If the refspec cannot be parsed successfully and\n+ *  - gently=0: die().\n+ *  - gently=1: store the refspec in remote->bogus_refspec and return with -1.\n+ *              If there are more than one bogus refspecs in the same remote,\n+ *              then only the first one will be stored.\n+ */\n+int add_fetch_refspec(struct remote *remote, const char *refspec, int gently);\n extern struct refspec *parse_push_refspec(int nr_refspec, const char **refspec);\n \n void free_refspec(int nr_refspec, struct refspec *refspec);\n-- \n2.13.1.505.g7cc9fcafb\n\n"},{"id":"322476","messageId":"20170616192837.11035-2-szeder.dev@gmail.com","threadId":"46200","inReplyTo":"20170616192837.11035-1-szeder.dev@gmail.com","subject":"[PATCH 1/5] remote: don't use remote->{fetch,push}_refspec","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-06-16T19:28:33Z","receivedAt":"2017-06-16T19:29:03Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"builtin/remote.c uses remote->fetch_refspec and remote->push_refspec,\ni.e. refspecs as strings, in a few places, e.g. in an error message or\nto set configuration variables.\n\nSince we are about to eliminate remote->{fetch,push}_refspec, recreate\nthose strings from the corresponding remote->{fetch,push} entries.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n builtin/remote.c | 22 +++++++++++++++-------\n remote.c         | 20 ++++++++++++++++++++\n remote.h         |  2 ++\n 3 files changed, 37 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex f1a88fe26..7f0072fe5 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -334,7 +334,7 @@ static int get_ref_states(const struct ref *remote_refs, struct ref_states *stat\n \tfor (i = 0; i < states->remote->fetch_refspec_nr; i++)\n \t\tif (get_fetch_map(remote_refs, states->remote->fetch + i, &tail, 1))\n \t\t\tdie(_(\"Could not get fetch map for refspec %s\"),\n-\t\t\t\tstates->remote->fetch_refspec[i]);\n+\t\t\t      refspec_to_string(&states->remote->fetch[i]));\n \n \tstates->new.strdup_strings = 1;\n \tstates->tracked.strdup_strings = 1;\n@@ -576,7 +576,7 @@ static int read_remote_branches(const char *refname,\n \n static int migrate_file(struct remote *remote)\n {\n-\tstruct strbuf buf = STRBUF_INIT;\n+\tstruct strbuf buf = STRBUF_INIT, refspec = STRBUF_INIT;\n \tint i;\n \n \tstrbuf_addf(&buf, \"remote.%s.url\", remote->name);\n@@ -584,17 +584,25 @@ static int migrate_file(struct remote *remote)\n \t\tgit_config_set_multivar(buf.buf, remote->url[i], \"^$\", 0);\n \tstrbuf_reset(&buf);\n \tstrbuf_addf(&buf, \"remote.%s.push\", remote->name);\n-\tfor (i = 0; i < remote->push_refspec_nr; i++)\n-\t\tgit_config_set_multivar(buf.buf, remote->push_refspec[i], \"^$\", 0);\n+\tfor (i = 0; i < remote->push_refspec_nr; i++) {\n+\t\tstrbuf_add_refspec(&refspec, &remote->push[i]);\n+\t\tgit_config_set_multivar(buf.buf, refspec.buf, \"^$\", 0);\n+\t\tstrbuf_reset(&refspec);\n+\t}\n \tstrbuf_reset(&buf);\n \tstrbuf_addf(&buf, \"remote.%s.fetch\", remote->name);\n-\tfor (i = 0; i < remote->fetch_refspec_nr; i++)\n-\t\tgit_config_set_multivar(buf.buf, remote->fetch_refspec[i], \"^$\", 0);\n+\tfor (i = 0; i < remote->fetch_refspec_nr; i++) {\n+\t\tstrbuf_add_refspec(&refspec, &remote->fetch[i]);\n+\t\tgit_config_set_multivar(buf.buf, refspec.buf, \"^$\", 0);\n+\t\tstrbuf_reset(&refspec);\n+\t}\n \tif (remote->origin == REMOTE_REMOTES)\n \t\tunlink_or_warn(git_path(\"remotes/%s\", remote->name));\n \telse if (remote->origin == REMOTE_BRANCHES)\n \t\tunlink_or_warn(git_path(\"branches/%s\", remote->name));\n \n+\tstrbuf_release(&buf);\n+\tstrbuf_release(&refspec);\n \treturn 0;\n }\n \n@@ -647,7 +655,7 @@ static int mv(int argc, const char **argv)\n \t\tchar *ptr;\n \n \t\tstrbuf_reset(&buf2);\n-\t\tstrbuf_addstr(&buf2, oldremote->fetch_refspec[i]);\n+\t\tstrbuf_add_refspec(&buf2, &oldremote->fetch[i]);\n \t\tptr = strstr(buf2.buf, old_remote_context.buf);\n \t\tif (ptr) {\n \t\t\trefspec_updated = 1;\ndiff --git a/remote.c b/remote.c\nindex 336db8298..a021decee 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -919,6 +919,26 @@ char *apply_refspecs(struct refspec *refspecs, int nr_refspec,\n \treturn query.dst;\n }\n \n+void strbuf_add_refspec(struct strbuf *sb, const struct refspec *refspec)\n+{\n+\tif (refspec->force)\n+\t\tstrbuf_addch(sb, '+');\n+\tif (refspec->src)\n+\t\tstrbuf_addstr(sb, refspec->src);\n+\tif (refspec->dst) {\n+\t\tstrbuf_addch(sb, ':');\n+\t\tstrbuf_addstr(sb, refspec->dst);\n+\t} else if (!refspec->src)\n+\t\tstrbuf_addch(sb, ':');\n+}\n+\n+char *refspec_to_string(const struct refspec *refspec)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tstrbuf_add_refspec(&sb, refspec);\n+\treturn strbuf_detach(&sb, NULL);\n+}\n+\n int remote_find_tracking(struct remote *remote, struct refspec *refspec)\n {\n \treturn query_refspecs(remote->fetch, remote->fetch_refspec_nr, refspec);\ndiff --git a/remote.h b/remote.h\nindex 9619f94dd..ee6c432d0 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -177,6 +177,8 @@ void free_refspec(int nr_refspec, struct refspec *refspec);\n extern int query_refspecs(struct refspec *specs, int nr, struct refspec *query);\n char *apply_refspecs(struct refspec *refspecs, int nr_refspec,\n \t\t     const char *name);\n+void strbuf_add_refspec(struct strbuf *sb, const struct refspec *refspec);\n+char *refspec_to_string(const struct refspec *refspec);\n \n int check_push_refs(struct ref *src, int nr_refspec, const char **refspec);\n int match_push_refs(struct ref *src, struct ref **dst,\n-- \n2.13.1.505.g7cc9fcafb\n\n"},{"id":"322477","messageId":"20170616192837.11035-4-szeder.dev@gmail.com","threadId":"46200","inReplyTo":"20170616192837.11035-1-szeder.dev@gmail.com","subject":"[PATCH 3/5] remote.c: extract a helper function to parse a single refspec","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-06-16T19:28:35Z","receivedAt":"2017-06-16T19:29:06Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Fetch refspecs read from the configuration are currently parsed\nlazily: they are collected into a string array for each remote while\nreading the configuration and then refspecs of a particular remote are\nparsed together later when the remote is accessed by remote_get() or\nfor_each_remote().\n\nWe are about to parse refspecs _while_ reading them from the\nconfiguration, one by one, and a function parsing a single refspec\ninto a memory location provided by the caller will be quite useful\nthen.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n remote.c | 231 +++++++++++++++++++++++++++++++++------------------------------\n 1 file changed, 122 insertions(+), 109 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex d23518afd..fc1d3cf7a 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -484,123 +484,136 @@ static void read_config(void)\n \talias_all_urls();\n }\n \n+static int parse_one_refspec(struct refspec *rs, const char *refspec,\n+\t\t\t     int fetch, int gently)\n+{\n+\tsize_t llen;\n+\tint is_glob;\n+\tconst char *lhs, *rhs;\n+\tint flags;\n+\n+\tmemset(rs, 0, sizeof(*rs));\n+\n+\tis_glob = 0;\n+\n+\tlhs = refspec;\n+\tif (*lhs == '+') {\n+\t\trs->force = 1;\n+\t\tlhs++;\n+\t}\n+\n+\trhs = strrchr(lhs, ':');\n+\n+\t/*\n+\t * Before going on, special case \":\" (or \"+:\") as a refspec\n+\t * for pushing matching refs.\n+\t */\n+\tif (!fetch && rhs == lhs && rhs[1] == '\\0') {\n+\t\trs->matching = 1;\n+\t\treturn 0;\n+\t}\n+\n+\tif (rhs) {\n+\t\tsize_t rlen = strlen(++rhs);\n+\t\tis_glob = (1 <= rlen && strchr(rhs, '*'));\n+\t\trs->dst = xstrndup(rhs, rlen);\n+\t}\n+\n+\tllen = (rhs ? (rhs - lhs - 1) : strlen(lhs));\n+\tif (1 <= llen && memchr(lhs, '*', llen)) {\n+\t\tif ((rhs && !is_glob) || (!rhs && fetch))\n+\t\t\tgoto invalid;\n+\t\tis_glob = 1;\n+\t} else if (rhs && is_glob) {\n+\t\tgoto invalid;\n+\t}\n+\n+\trs->pattern = is_glob;\n+\trs->src = xstrndup(lhs, llen);\n+\tflags = REFNAME_ALLOW_ONELEVEL | (is_glob ? REFNAME_REFSPEC_PATTERN : 0);\n+\n+\tif (fetch) {\n+\t\tstruct object_id unused;\n+\n+\t\t/* LHS */\n+\t\tif (!*rs->src)\n+\t\t\t; /* empty is ok; it means \"HEAD\" */\n+\t\telse if (llen == GIT_SHA1_HEXSZ && !get_oid_hex(rs->src, &unused))\n+\t\t\trs->exact_sha1 = 1; /* ok */\n+\t\telse if (!check_refname_format(rs->src, flags))\n+\t\t\t; /* valid looking ref is ok */\n+\t\telse\n+\t\t\tgoto invalid;\n+\t\t/* RHS */\n+\t\tif (!rs->dst)\n+\t\t\t; /* missing is ok; it is the same as empty */\n+\t\telse if (!*rs->dst)\n+\t\t\t; /* empty is ok; it means \"do not store\" */\n+\t\telse if (!check_refname_format(rs->dst, flags))\n+\t\t\t; /* valid looking ref is ok */\n+\t\telse\n+\t\t\tgoto invalid;\n+\t} else {\n+\t\t/*\n+\t\t * LHS\n+\t\t * - empty is allowed; it means delete.\n+\t\t * - when wildcarded, it must be a valid looking ref.\n+\t\t * - otherwise, it must be an extended SHA-1, but\n+\t\t *   there is no existing way to validate this.\n+\t\t */\n+\t\tif (!*rs->src)\n+\t\t\t; /* empty is ok */\n+\t\telse if (is_glob) {\n+\t\t\tif (check_refname_format(rs->src, flags))\n+\t\t\t\tgoto invalid;\n+\t\t}\n+\t\telse\n+\t\t\t; /* anything goes, for now */\n+\t\t/*\n+\t\t * RHS\n+\t\t * - missing is allowed, but LHS then must be a\n+\t\t *   valid looking ref.\n+\t\t * - empty is not allowed.\n+\t\t * - otherwise it must be a valid looking ref.\n+\t\t */\n+\t\tif (!rs->dst) {\n+\t\t\tif (check_refname_format(rs->src, flags))\n+\t\t\t\tgoto invalid;\n+\t\t} else if (!*rs->dst) {\n+\t\t\tgoto invalid;\n+\t\t} else {\n+\t\t\tif (check_refname_format(rs->dst, flags))\n+\t\t\t\tgoto invalid;\n+\t\t}\n+\t}\n+\n+\treturn 0;\n+\n+ invalid:\n+\tif (gently) {\n+\t\tfree(rs->src);\n+\t\tfree(rs->dst);\n+\t\treturn -1;\n+\t}\n+\tdie(\"Invalid refspec '%s'\", 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-\tstruct refspec *rs = xcalloc(nr_refspec, sizeof(*rs));\n+\tstruct refspec *rs;\n+\n+\tALLOC_ARRAY(rs, nr_refspec);\n \n \tfor (i = 0; i < nr_refspec; i++) {\n-\t\tsize_t llen;\n-\t\tint is_glob;\n-\t\tconst char *lhs, *rhs;\n-\t\tint flags;\n-\n-\t\tis_glob = 0;\n-\n-\t\tlhs = refspec[i];\n-\t\tif (*lhs == '+') {\n-\t\t\trs[i].force = 1;\n-\t\t\tlhs++;\n-\t\t}\n-\n-\t\trhs = strrchr(lhs, ':');\n-\n-\t\t/*\n-\t\t * Before going on, special case \":\" (or \"+:\") as a refspec\n-\t\t * for pushing matching refs.\n-\t\t */\n-\t\tif (!fetch && rhs == lhs && rhs[1] == '\\0') {\n-\t\t\trs[i].matching = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\n-\t\tif (rhs) {\n-\t\t\tsize_t rlen = strlen(++rhs);\n-\t\t\tis_glob = (1 <= rlen && strchr(rhs, '*'));\n-\t\t\trs[i].dst = xstrndup(rhs, rlen);\n-\t\t}\n-\n-\t\tllen = (rhs ? (rhs - lhs - 1) : strlen(lhs));\n-\t\tif (1 <= llen && memchr(lhs, '*', llen)) {\n-\t\t\tif ((rhs && !is_glob) || (!rhs && fetch))\n-\t\t\t\tgoto invalid;\n-\t\t\tis_glob = 1;\n-\t\t} else if (rhs && is_glob) {\n-\t\t\tgoto invalid;\n-\t\t}\n-\n-\t\trs[i].pattern = is_glob;\n-\t\trs[i].src = xstrndup(lhs, llen);\n-\t\tflags = REFNAME_ALLOW_ONELEVEL | (is_glob ? REFNAME_REFSPEC_PATTERN : 0);\n-\n-\t\tif (fetch) {\n-\t\t\tstruct object_id unused;\n-\n-\t\t\t/* LHS */\n-\t\t\tif (!*rs[i].src)\n-\t\t\t\t; /* empty is ok; it means \"HEAD\" */\n-\t\t\telse if (llen == GIT_SHA1_HEXSZ && !get_oid_hex(rs[i].src, &unused))\n-\t\t\t\trs[i].exact_sha1 = 1; /* ok */\n-\t\t\telse if (!check_refname_format(rs[i].src, flags))\n-\t\t\t\t; /* valid looking ref is ok */\n-\t\t\telse\n-\t\t\t\tgoto invalid;\n-\t\t\t/* RHS */\n-\t\t\tif (!rs[i].dst)\n-\t\t\t\t; /* missing is ok; it is the same as empty */\n-\t\t\telse if (!*rs[i].dst)\n-\t\t\t\t; /* empty is ok; it means \"do not store\" */\n-\t\t\telse if (!check_refname_format(rs[i].dst, flags))\n-\t\t\t\t; /* valid looking ref is ok */\n-\t\t\telse\n-\t\t\t\tgoto invalid;\n-\t\t} else {\n-\t\t\t/*\n-\t\t\t * LHS\n-\t\t\t * - empty is allowed; it means delete.\n-\t\t\t * - when wildcarded, it must be a valid looking ref.\n-\t\t\t * - otherwise, it must be an extended SHA-1, but\n-\t\t\t *   there is no existing way to validate this.\n-\t\t\t */\n-\t\t\tif (!*rs[i].src)\n-\t\t\t\t; /* empty is ok */\n-\t\t\telse if (is_glob) {\n-\t\t\t\tif (check_refname_format(rs[i].src, flags))\n-\t\t\t\t\tgoto invalid;\n-\t\t\t}\n-\t\t\telse\n-\t\t\t\t; /* anything goes, for now */\n-\t\t\t/*\n-\t\t\t * RHS\n-\t\t\t * - missing is allowed, but LHS then must be a\n-\t\t\t *   valid looking ref.\n-\t\t\t * - empty is not allowed.\n-\t\t\t * - otherwise it must be a valid looking ref.\n-\t\t\t */\n-\t\t\tif (!rs[i].dst) {\n-\t\t\t\tif (check_refname_format(rs[i].src, flags))\n-\t\t\t\t\tgoto invalid;\n-\t\t\t} else if (!*rs[i].dst) {\n-\t\t\t\tgoto invalid;\n-\t\t\t} else {\n-\t\t\t\tif (check_refname_format(rs[i].dst, flags))\n-\t\t\t\t\tgoto invalid;\n-\t\t\t}\n+\t\tif (parse_one_refspec(&rs[i], refspec[i], fetch, verify) < 0) {\n+\t\t\t/* verify != 0 here, because parse_one_refspec()\n+\t\t\t * would have already die()d otherwise. */\n+\t\t\tfree_refspec(i, rs);\n+\t\t\treturn NULL;\n \t\t}\n \t}\n \treturn rs;\n-\n- invalid:\n-\tif (verify) {\n-\t\t/*\n-\t\t * nr_refspec must be greater than zero and i must be valid\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_refspec(i+1, rs);\n-\t\treturn NULL;\n-\t}\n-\tdie(\"Invalid refspec '%s'\", refspec[i]);\n }\n \n int valid_fetch_refspec(const char *fetch_refspec_str)\n-- \n2.13.1.505.g7cc9fcafb\n\n"},{"id":"322478","messageId":"20170616192837.11035-1-szeder.dev@gmail.com","threadId":"46200","inReplyTo":null,"subject":"[RFC/PATCH 0/5] remote: eliminate remote->{fetch,push}_refspec and lazy parsing of refspecs","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-06-16T19:28:32Z","receivedAt":"2017-06-16T19:29:08Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"\n'struct remote' stores refspecs twice: once in their original string\nform in remote->{fetch,push}_refspecs and once in their parsed form in\nremote->{fetch,push}.  This is necessary, because we need the refspecs\nfor lazy parsing after we finished reading the configuration: we don't\nwant to die() on a bogus refspec while reading the configuration of a\nremote we are not going to access at all.\n\nHowever, storing refspecs in both forms has some drawbacks:\n\n  - The same information is stored twice, wasting memory.\n  - remote->{fetch,push}_refspecs, i.e. the string arrays are\n    conveniently ALLOC_GROW()-able with associated\n    {fetch,push}_refspec_{nr,alloc} fields, but remote->{fetch,push}\n    are not.\n  - Wherever remote->{fetch,push} are accessed, the number of parsed\n    refspecs in there is specified by remote->{fetch,push}_refspec_nr.\n    This requires us to keep the two arrays in sync and makes adding\n    additional refspecs cumbersome and error prone.\n\n  - And worst of all, it pissed me off while working on\n    sg/clone-refspec-from-command-line-config ;)\n\n\nSo here is my crack at getting rid of them.\n\nThe idea is to parse refspecs gently while reading the configuration:\nthis way we won't need to store all refspecs as strings, and won't\ndie() on a bogus refspec right away.  A bogus refspec, if there's one,\nwill be stored in the remote it belongs to, so it will be available\nlater when that remote is accessed and can be used in the error\nmessage.\n\nAt the end of the series the remote API will have public functions\nadd_{fetch,push}_refspec() for, well, adding refspecs, including\nparsing, to those that were already present in the configuration.  I'm\nnot sure what the use case for adding extra push refspecs could be,\nbut sg/clone-refspec-from-command-line-config has shown that there is\na need for adding fetch refspecs, and this way they are at least\nsymmetrical.  Though I don't see any other use case for adding extra\nfetch refspecs, either.  Anyway, there's less subtlety without the\nneed to keep the two arrays ->fetch and ->fetch_refspec in sync.\n\n\nThe first three patches are preparation.\nPatch 3 was done with --patience, otherwise it's unreadable.  -w will\nhelp even more to see what's going on.\nThe last patch's commit message is rather terse... but I deemed it\nsufficient for an initial RFC; will expand it later, if there's an\nagreement that this approach is worth pursuing.\n\n\nThis applies on top of a merge of master and the fresh reroll (v5) of\nsg/clone-refspec-from-command-line-config:\n\n  http://public-inbox.org/git/20170616173849.8071-1-szeder.dev@gmail.com/T/#m7f9c81fb415920acc2af93f432aecfa52477e6b8\n\nIt's also available at:\n\n  https://github.com/szeder/git remote-no-string-refspecs\n\n\nBest,\nGábor\n\n\nSZEDER Gábor (5):\n  remote: don't use remote->{fetch,push}_refspec\n  remote.c: don't pass copies of refspecs to add_{fetch,push}_refspec()\n  remote.c: extract a helper function to parse a single refspec\n  remote.c: eliminate remote->fetch_refspec\n  remote.c: eliminate remote->push_refspec\n\n builtin/clone.c  |   7 +-\n builtin/fetch.c  |  20 ++--\n builtin/push.c   |  12 ++-\n builtin/remote.c |  47 ++++++---\n remote.c         | 316 +++++++++++++++++++++++++++++++------------------------\n remote.h         |  31 ++++--\n 6 files changed, 248 insertions(+), 185 deletions(-)\n\n-- \n2.13.1.505.g7cc9fcafb\n\n"},{"id":"322479","messageId":"20170616192837.11035-6-szeder.dev@gmail.com","threadId":"46200","inReplyTo":"20170616192837.11035-1-szeder.dev@gmail.com","subject":"[PATCH 5/5] remote.c: eliminate remote->push_refspec","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-06-16T19:28:37Z","receivedAt":"2017-06-16T19:29:11Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Do the same as in the previous patch, but for push refspecs, i.e. with\nremote->push_refspec, remote->push and add_push_refspec().\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n builtin/push.c   | 12 +++++++-----\n builtin/remote.c | 19 ++++++++++++-------\n remote.c         | 29 +++++++++++------------------\n remote.h         | 10 ++++------\n 4 files changed, 34 insertions(+), 36 deletions(-)\n\ndiff --git a/builtin/push.c b/builtin/push.c\nindex 258648d5f..9a721fe8a 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -47,11 +47,11 @@ static const char *map_refspec(const char *ref,\n \tif (count_refspec_match(ref, local_refs, &matched) != 1)\n \t\treturn ref;\n \n-\tif (remote->push) {\n+\tif (remote->push.rs) {\n \t\tstruct refspec query;\n \t\tmemset(&query, 0, sizeof(struct refspec));\n \t\tquery.src = matched->name;\n-\t\tif (!query_refspecs(remote->push, remote->push_refspec_nr, &query) &&\n+\t\tif (!query_refspecs(remote->push.rs, remote->push.nr, &query) &&\n \t\t    query.dst) {\n \t\t\tstruct strbuf buf = STRBUF_INIT;\n \t\t\tstrbuf_addf(&buf, \"%s%s:%s\",\n@@ -401,9 +401,11 @@ static int do_push(const char *repo, int flags,\n \t}\n \n \tif (!refspec && !(flags & TRANSPORT_PUSH_ALL)) {\n-\t\tif (remote->push_refspec_nr) {\n-\t\t\trefspec = remote->push_refspec;\n-\t\t\trefspec_nr = remote->push_refspec_nr;\n+\t\tif (remote->push.nr) {\n+\t\t\tALLOC_ARRAY(refspec, remote->push.nr);\n+\t\t\tfor (i = 0; i < remote->push.nr; i++)\n+\t\t\t\trefspec[i] = refspec_to_string(&remote->push.rs[i]);\n+\t\t\trefspec_nr = remote->push.nr;\n \t\t} else if (!(flags & TRANSPORT_PUSH_MIRROR))\n \t\t\tsetup_default_push_refspecs(remote);\n \t}\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex d61daa5e8..c04dec0e1 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -380,14 +380,19 @@ static int get_push_ref_states(const struct ref *remote_refs,\n {\n \tstruct remote *remote = states->remote;\n \tstruct ref *ref, *local_refs, *push_map;\n+\tconst char **refspec;\n+\tint i;\n \tif (remote->mirror)\n \t\treturn 0;\n \n \tlocal_refs = get_local_heads();\n \tpush_map = copy_ref_list(remote_refs);\n \n-\tmatch_push_refs(local_refs, &push_map, remote->push_refspec_nr,\n-\t\t\tremote->push_refspec, MATCH_REFS_NONE);\n+\tALLOC_ARRAY(refspec, remote->push.nr);\n+\tfor (i = 0; i < remote->push.nr; i++)\n+\t\trefspec[i] = refspec_to_string(&remote->push.rs[i]);\n+\tmatch_push_refs(local_refs, &push_map, remote->push.nr,\n+\t\t\trefspec, MATCH_REFS_NONE);\n \n \tstates->push.strdup_strings = 1;\n \tfor (ref = push_map; ref; ref = ref->next) {\n@@ -433,14 +438,14 @@ static int get_push_ref_states_noquery(struct ref_states *states)\n \t\treturn 0;\n \n \tstates->push.strdup_strings = 1;\n-\tif (!remote->push_refspec_nr) {\n+\tif (!remote->push.nr) {\n \t\titem = string_list_append(&states->push, _(\"(matching)\"));\n \t\tinfo = item->util = xcalloc(1, sizeof(struct push_info));\n \t\tinfo->status = PUSH_STATUS_NOTQUERIED;\n \t\tinfo->dest = xstrdup(item->string);\n \t}\n-\tfor (i = 0; i < remote->push_refspec_nr; i++) {\n-\t\tstruct refspec *spec = remote->push + i;\n+\tfor (i = 0; i < remote->push.nr; i++) {\n+\t\tstruct refspec *spec = remote->push.rs + i;\n \t\tif (spec->matching)\n \t\t\titem = string_list_append(&states->push, _(\"(matching)\"));\n \t\telse if (strlen(spec->src))\n@@ -584,8 +589,8 @@ static int migrate_file(struct remote *remote)\n \t\tgit_config_set_multivar(buf.buf, remote->url[i], \"^$\", 0);\n \tstrbuf_reset(&buf);\n \tstrbuf_addf(&buf, \"remote.%s.push\", remote->name);\n-\tfor (i = 0; i < remote->push_refspec_nr; i++) {\n-\t\tstrbuf_add_refspec(&refspec, &remote->push[i]);\n+\tfor (i = 0; i < remote->push.nr; i++) {\n+\t\tstrbuf_add_refspec(&refspec, &remote->push.rs[i]);\n \t\tgit_config_set_multivar(buf.buf, refspec.buf, \"^$\", 0);\n \t\tstrbuf_reset(&refspec);\n \t}\ndiff --git a/remote.c b/remote.c\nindex 952000b61..9dfe3e9a6 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -86,14 +86,6 @@ static const char *alias_url(const char *url, struct rewrites *r)\n \treturn xstrfmt(\"%s%s\", r->rewrite[longest_i]->base, url + longest->len);\n }\n \n-static void add_push_refspec(struct remote *remote, const char *ref)\n-{\n-\tALLOC_GROW(remote->push_refspec,\n-\t\t   remote->push_refspec_nr + 1,\n-\t\t   remote->push_refspec_alloc);\n-\tremote->push_refspec[remote->push_refspec_nr++] = strdup(ref);\n-}\n-\n static void add_url(struct remote *remote, const char *url)\n {\n \tALLOC_GROW(remote->url, remote->url_nr + 1, remote->url_alloc);\n@@ -257,7 +249,7 @@ static void read_remotes_file(struct remote *remote)\n \t\tif (skip_prefix(buf.buf, \"URL:\", &v))\n \t\t\tadd_url_alias(remote, xstrdup(skip_spaces(v)));\n \t\telse if (skip_prefix(buf.buf, \"Push:\", &v))\n-\t\t\tadd_push_refspec(remote, skip_spaces(v));\n+\t\t\tadd_push_refspec(remote, skip_spaces(v), 1);\n \t\telse if (skip_prefix(buf.buf, \"Pull:\", &v))\n \t\t\tadd_fetch_refspec(remote, skip_spaces(v), 1);\n \t}\n@@ -308,7 +300,7 @@ static void read_branches_file(struct remote *remote)\n \t * (master if missing)\n \t */\n \tstrbuf_addf(&buf, \"HEAD:refs/heads/%s\", frag);\n-\tadd_push_refspec(remote, buf.buf);\n+\tadd_push_refspec(remote, buf.buf, 1);\n \tremote->fetch_tags = 1; /* always auto-follow */\n \n \tstrbuf_release(&buf);\n@@ -394,7 +386,7 @@ static int handle_config(const char *key, const char *value, void *cb)\n \t\tconst char *v;\n \t\tif (git_config_string(&v, key, value))\n \t\t\treturn -1;\n-\t\tadd_push_refspec(remote, v);\n+\t\tadd_push_refspec(remote, v, 1);\n \t\tfree((char*)v);\n \t} else if (!strcmp(subkey, \"fetch\")) {\n \t\tconst char *v;\n@@ -625,7 +617,7 @@ struct refspec *parse_fetch_refspec(int nr_refspec, const char **refspec)\n static int add_refspec(struct remote *remote, const char *refspec,\n \t\t       int fetch, int gently)\n {\n-\tstruct refspec_array *rsa = fetch ? &remote->fetch : NULL;\n+\tstruct refspec_array *rsa = fetch ? &remote->fetch : &remote->push;\n \n \tALLOC_GROW(rsa->rs, rsa->nr + 1, rsa->alloc);\n \n@@ -651,6 +643,11 @@ struct refspec *parse_push_refspec(int nr_refspec, const char **refspec)\n \treturn parse_refspec_internal(nr_refspec, refspec, 0, 0);\n }\n \n+int add_push_refspec(struct remote *remote, const char *refspec, int gently)\n+{\n+\treturn add_refspec(remote, refspec, 0, gently);\n+}\n+\n void free_refspec(int nr_refspec, struct refspec *refspec)\n {\n \tint i;\n@@ -730,7 +727,6 @@ static struct remote *remote_get_1(const char *name,\n \t\treturn NULL;\n \tif (ret->bogus_refspec)\n \t\tdie(\"Invalid refspec '%s'\", ret->bogus_refspec);\n-\tret->push = parse_push_refspec(ret->push_refspec_nr, ret->push_refspec);\n \treturn ret;\n }\n \n@@ -763,9 +759,6 @@ int for_each_remote(each_remote_fn fn, void *priv)\n \t\t\tcontinue;\n \t\tif (r->bogus_refspec)\n \t\t\tdie(\"Invalid refspec '%s'\", r->bogus_refspec);\n-\t\tif (!r->push)\n-\t\t\tr->push = parse_push_refspec(r->push_refspec_nr,\n-\t\t\t\t\t\t     r->push_refspec);\n \t\tresult = fn(r, priv);\n \t}\n \treturn result;\n@@ -1777,11 +1770,11 @@ static const char *branch_get_push_1(struct branch *branch, struct strbuf *err)\n \t\t\t\t _(\"branch '%s' has no remote for pushing\"),\n \t\t\t\t branch->name);\n \n-\tif (remote->push_refspec_nr) {\n+\tif (remote->push.nr) {\n \t\tchar *dst;\n \t\tconst char *ret;\n \n-\t\tdst = apply_refspecs(remote->push, remote->push_refspec_nr,\n+\t\tdst = apply_refspecs(remote->push.rs, remote->push.nr,\n \t\t\t\t     branch->refname);\n \t\tif (!dst)\n \t\t\treturn error_buf(err,\ndiff --git a/remote.h b/remote.h\nindex 416a08501..eba06bacb 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -32,14 +32,10 @@ struct remote {\n \tint pushurl_nr;\n \tint pushurl_alloc;\n \n-\tconst char **push_refspec;\n-\tstruct refspec *push;\n-\tint push_refspec_nr;\n-\tint push_refspec_alloc;\n-\n+\tstruct refspec_array push;\n \tstruct refspec_array fetch;\n \n-\t/* Copy of the first bogus fetch refspec we couldn't parse */\n+\t/* Copy of the first bogus refspec we couldn't parse */\n \tconst char *bogus_refspec;\n \n \t/*\n@@ -184,6 +180,8 @@ struct refspec *parse_fetch_refspec(int nr_refspec, const char **refspec);\n  */\n int add_fetch_refspec(struct remote *remote, const char *refspec, int gently);\n extern struct refspec *parse_push_refspec(int nr_refspec, const char **refspec);\n+/* The same as add_fetch_refspec() above, but for push refspecs. */\n+int add_push_refspec(struct remote *remote, const char *refspec, int gently);\n \n void free_refspec(int nr_refspec, struct refspec *refspec);\n \n-- \n2.13.1.505.g7cc9fcafb\n\n"},{"id":"322494","messageId":"xmqqa857wqay.fsf@gitster.mtv.corp.google.com","threadId":"46200","inReplyTo":"20170616192837.11035-1-szeder.dev@gmail.com","subject":"Re: [RFC/PATCH 0/5] remote: eliminate remote->{fetch,push}_refspec and lazy parsing of refspecs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-16T21:55:33Z","receivedAt":"2017-06-16T21:55:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor <szeder.dev@gmail.com> writes:\n\n> 'struct remote' stores refspecs twice: once in their original string\n> form in remote->{fetch,push}_refspecs and once in their parsed form in\n> remote->{fetch,push}.  This is necessary, because we need the refspecs\n> for lazy parsing after we finished reading the configuration: we don't\n> want to die() on a bogus refspec while reading the configuration of a\n> remote we are not going to access at all.\n>\n> However, storing refspecs in both forms has some drawbacks:\n>\n>   - The same information is stored twice, wasting memory.\n\nTrue (but a few hundred bytes is nothing among friends ;-)\n\n>   - remote->{fetch,push}_refspecs, i.e. the string arrays are\n>     conveniently ALLOC_GROW()-able with associated\n>     {fetch,push}_refspec_{nr,alloc} fields, but remote->{fetch,push}\n>     are not.\n\nThis is a more real issue.\n\n>   - Wherever remote->{fetch,push} are accessed, the number of parsed\n>     refspecs in there is specified by remote->{fetch,push}_refspec_nr.\n>     This requires us to keep the two arrays in sync and makes adding\n>     additional refspecs cumbersome and error prone.\n\nYou haven't told us which way you want to dedup.  Are you keeping\nthe original and removing the pre-parsed?  or are you only keeping\nthe pre-parsed ones?  As long as you want ALLOC_GROW() ability, you\nneed to maintain the invariants in three-tuple (foo, foo_alloc,\nfoo_nr).\n\n>   - And worst of all, it pissed me off while working on\n>     sg/clone-refspec-from-command-line-config ;)\n\nYour feelings (or mine) do not count ;-).\n\nI do not think we would terribly mind if you only kept a list of\npre-parsed form, with some mechanism to keep an \"error\" entry in\nthat list with its original, so that an error can be reported with\nthe refspec as the user originally gave us (which may mean the\n\"error\" entry may have to keep the original form, since it wasn't\ncorrectly parsable in the first place for it to trigger an error).\n\n> So here is my crack at getting rid of them.\n\nYou still haven't told us what \"them\" are.  Parsed form, or the\noriginal?  Let's find out by reading on....\n\n> The idea is to parse refspecs gently while reading the configuration:\n> this way we won't need to store all refspecs as strings, and won't\n> die() on a bogus refspec right away.  A bogus refspec, if there's one,\n> will be stored in the remote it belongs to, so it will be available\n> later when that remote is accessed and can be used in the error\n> message.\n\nSo normally we only have a list of parsed ones, but optionally there\nis a list of malformed originals that are before attempted (and\nfailed) parsing used for error reporting?  That sounds sensible,\nespecially given that we can recreate the original textual form from\ncorrectly parsed result (which allows us to report on other kinds\nof errors as necessary).\n\n> This applies on top of a merge of master and the fresh reroll (v5) of\n> sg/clone-refspec-from-command-line-config:\n\nThanks.  Will take a look (but not immediately).\n"},{"id":"322495","messageId":"xmqq60fvwq24.fsf@gitster.mtv.corp.google.com","threadId":"46200","inReplyTo":"20170616192837.11035-2-szeder.dev@gmail.com","subject":"Re: [PATCH 1/5] remote: don't use remote->{fetch,push}_refspec","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-16T22:00:51Z","receivedAt":"2017-06-16T22:00:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor <szeder.dev@gmail.com> writes:\n\n> diff --git a/remote.c b/remote.c\n> index 336db8298..a021decee 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -919,6 +919,26 @@ char *apply_refspecs(struct refspec *refspecs, int nr_refspec,\n>  \treturn query.dst;\n>  }\n>  \n> +void strbuf_add_refspec(struct strbuf *sb, const struct refspec *refspec)\n> +{\n> +\tif (refspec->force)\n> +\t\tstrbuf_addch(sb, '+');\n> +\tif (refspec->src)\n> +\t\tstrbuf_addstr(sb, refspec->src);\n> +\tif (refspec->dst) {\n> +\t\tstrbuf_addch(sb, ':');\n> +\t\tstrbuf_addstr(sb, refspec->dst);\n> +\t} else if (!refspec->src)\n> +\t\tstrbuf_addch(sb, ':');\n> +}\n\nHmph, don't we support wildcarding (aka refspec->pattern)?\n\n    ... goes and looks ...\n\nAh, even when we set the .pattern bit, we retain '*' in the string,\nso the above is correct.\n\nThe last one looks a bit sloppy; shouldn't it be inspecting the\n.matching bit?\n\n"},{"id":"322501","messageId":"20170617115522.lbke5wnle5irbunj@sigill.intra.peff.net","threadId":"46200","inReplyTo":"20170616192837.11035-1-szeder.dev@gmail.com","subject":"Re: [RFC/PATCH 0/5] remote: eliminate remote->{fetch,push}_refspec and lazy parsing of refspecs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-17T11:55:22Z","receivedAt":"2017-06-17T11:55:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 16, 2017 at 09:28:32PM +0200, SZEDER Gábor wrote:\n\n> 'struct remote' stores refspecs twice: once in their original string\n> form in remote->{fetch,push}_refspecs and once in their parsed form in\n> remote->{fetch,push}.  This is necessary, because we need the refspecs\n> for lazy parsing after we finished reading the configuration: we don't\n> want to die() on a bogus refspec while reading the configuration of a\n> remote we are not going to access at all.\n> \n> However, storing refspecs in both forms has some drawbacks:\n> \n>   - The same information is stored twice, wasting memory.\n\nI don't think this is really that important. It's rare to have more than\na handful of refspecs, and they're short strings.\n\n>   - remote->{fetch,push}_refspecs, i.e. the string arrays are\n>     conveniently ALLOC_GROW()-able with associated\n>     {fetch,push}_refspec_{nr,alloc} fields, but remote->{fetch,push}\n>     are not.\n>   - Wherever remote->{fetch,push} are accessed, the number of parsed\n>     refspecs in there is specified by remote->{fetch,push}_refspec_nr.\n>     This requires us to keep the two arrays in sync and makes adding\n>     additional refspecs cumbersome and error prone.\n\nI think the two-arrays things is more important, because keeping them in\nsync is error-prone. On the other hand, if the array are manipulated by\naccessors, that keeps the complexity in one spot.\n\nThe lazy parsing is an additional headache on top of that, but that's\northogonal. Without the lazy parsing, then a single add_refspec() would\nbe simple and keep everything in sync.\n\n>   - And worst of all, it pissed me off while working on\n>     sg/clone-refspec-from-command-line-config ;)\n\nThat I can sympathize with. ;)\n\n> The idea is to parse refspecs gently while reading the configuration:\n> this way we won't need to store all refspecs as strings, and won't\n> die() on a bogus refspec right away.  A bogus refspec, if there's one,\n> will be stored in the remote it belongs to, so it will be available\n> later when that remote is accessed and can be used in the error\n> message.\n\nThe \"turn a parsed refspec struct back into a refspec string\" makes me a\nlittle nervous, just because it needs to be the inverse of the parsing\nfunction. So now we have two new things that need to be kept in sync.\n\nIf we forget the \"storing it twice\" argument, would it make sense to\nconvert the parallel arrays of items into a single array-of-struct?\nI.e.:\n\n  struct configured_refspec {\n\tconst char *string;\n\tstruct refspec refspec;\n\tunsigned parsed:1;\n  }\n\nI guess that may run into problems where we really need an\narray-of-refspec to pass into sub-functions. So going further, could we\njust have \"struct refspec\" store the text form it was parsed from?\n\n-Peff\n"},{"id":"322503","messageId":"CAM0VKj=1DiPZzGot34Ar9SQR8VWV8ugvzFhAMibnZ8d-4F+1QA@mail.gmail.com","threadId":"46200","inReplyTo":"xmqqa857wqay.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/PATCH 0/5] remote: eliminate remote->{fetch,push}_refspec and lazy parsing of refspecs","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-06-17T12:25:39Z","receivedAt":"2017-06-17T12:25:45Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Jun 16, 2017 at 11:55 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> SZEDER Gábor <szeder.dev@gmail.com> writes:\n>\n>> 'struct remote' stores refspecs twice: once in their original string\n>> form in remote->{fetch,push}_refspecs and once in their parsed form in\n>> remote->{fetch,push}.  This is necessary, because we need the refspecs\n>> for lazy parsing after we finished reading the configuration: we don't\n>> want to die() on a bogus refspec while reading the configuration of a\n>> remote we are not going to access at all.\n>>\n>> However, storing refspecs in both forms has some drawbacks:\n>>\n>>   - The same information is stored twice, wasting memory.\n>\n> True (but a few hundred bytes is nothing among friends ;-)\n\nIndeed.  Even in my repos with close to 10k remotes the amount of\nmemory wasted by the duplicated refspecs is not an problem, there are\nmore pressing issues there.\n\n>>   - remote->{fetch,push}_refspecs, i.e. the string arrays are\n>>     conveniently ALLOC_GROW()-able with associated\n>>     {fetch,push}_refspec_{nr,alloc} fields, but remote->{fetch,push}\n>>     are not.\n>\n> This is a more real issue.\n>\n>>   - Wherever remote->{fetch,push} are accessed, the number of parsed\n>>     refspecs in there is specified by remote->{fetch,push}_refspec_nr.\n>>     This requires us to keep the two arrays in sync and makes adding\n>>     additional refspecs cumbersome and error prone.\n>\n> You haven't told us which way you want to dedup.\n\nWell, I actually did, right at the beginning.  The Subject:\nspecifically mentions which fields will be removed, and the first one\nand a half line says in more usual terms what their roles are.\n\nAnyway, made a note to use more natural language in the subjects (and\nelsewhere) when we get there, maybe \"remote.c: don't store refspecs as\nstrings in 'struct remote'\" or something.\n\n>  Are you keeping\n> the original and removing the pre-parsed?  or are you only keeping\n> the pre-parsed ones?  As long as you want ALLOC_GROW() ability, you\n> need to maintain the invariants in three-tuple (foo, foo_alloc,\n> foo_nr).\n>\n>>   - And worst of all, it pissed me off while working on\n>>     sg/clone-refspec-from-command-line-config ;)\n>\n> Your feelings (or mine) do not count ;-).\n\nFeelings have nothing to do with it, \"it pissed me off\" is a concise\nway of saying \"it made me waste quite some time debugging segfaults\nand other nonsense resulting from ALLOC_GROW()ing remote->fetch,\nmisled by the previous point and the confusing order in which these\nfields are listed in 'struct remote's definition\".\n\n> I do not think we would terribly mind if you only kept a list of\n> pre-parsed form, with some mechanism to keep an \"error\" entry in\n> that list with its original, so that an error can be reported with\n> the refspec as the user originally gave us (which may mean the\n> \"error\" entry may have to keep the original form, since it wasn't\n> correctly parsable in the first place for it to trigger an error).\n>\n>> So here is my crack at getting rid of them.\n>\n> You still haven't told us what \"them\" are.  Parsed form, or the\n> original?  Let's find out by reading on....\n>\n>> The idea is to parse refspecs gently while reading the configuration:\n>> this way we won't need to store all refspecs as strings, and won't\n>> die() on a bogus refspec right away.  A bogus refspec, if there's one,\n>> will be stored in the remote it belongs to, so it will be available\n>> later when that remote is accessed and can be used in the error\n>> message.\n>\n> So normally we only have a list of parsed ones, but optionally there\n> is a list of malformed originals that are before attempted (and\n> failed) parsing used for error reporting?\n\nFor each remote there are two arrays of parsed refspecs, one for fetch\nand one for push, and a single malformed original as string.\n\nThe reason for storing only a single malformed refspec per remote is\nthat I didn't want to noteworthily change the behaviour: the current\nimplementation die()s on the first malformed refspec it encounters\nwhile parsing and reports only that one malformed refspec in the error\nmessage.  This series essentially does the same as far as observable\nbehaviour goes, though it might happen that a different malformed\nrefspec is reported in the error message (if there are more than one,\ndepending on their order in the configuration).\n\nOf course, if we want to, then this could be extended to record all\nmalformed refspecs while reading the configuration and report all of\nthem in the error message.  But that's a behaviour change which I\nthink should come on top as a separate patch.\n\n>  That sounds sensible,\n> especially given that we can recreate the original textual form from\n> correctly parsed result (which allows us to report on other kinds\n> of errors as necessary).\n>\n>> This applies on top of a merge of master and the fresh reroll (v5) of\n>> sg/clone-refspec-from-command-line-config:\n>\n> Thanks.  Will take a look (but not immediately).\n"},{"id":"322504","messageId":"20170617123354.xolqjqdvofjtksll@sigill.intra.peff.net","threadId":"46200","inReplyTo":"CAM0VKj=1DiPZzGot34Ar9SQR8VWV8ugvzFhAMibnZ8d-4F+1QA@mail.gmail.com","subject":"Re: [RFC/PATCH 0/5] remote: eliminate remote->{fetch,push}_refspec and lazy parsing of refspecs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-17T12:33:54Z","receivedAt":"2017-06-17T12:34:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jun 17, 2017 at 02:25:39PM +0200, SZEDER Gábor wrote:\n\n> >>   - The same information is stored twice, wasting memory.\n> >\n> > True (but a few hundred bytes is nothing among friends ;-)\n> \n> Indeed.  Even in my repos with close to 10k remotes the amount of\n> memory wasted by the duplicated refspecs is not an problem, there are\n> more pressing issues there.\n\nThis is sort of a side note, but I'm curious why you need 10k remotes.\n\nWe used to do this at GitHub as part of our fork storage (the shared\nrepository had each fork as a remote). We fixed the quadratic issues\nlike d0da003d5 (use a hashmap to make remotes faster, 2014-07-29). But\neven the linear work to read the config is noticeable (and hits you on\nevery command, even ones that don't care about remotes).  Now instead we\npass the refspecs directly to fetch whenever move objects between the\nstorage repos. They were the same for every remote anyway (and I'd guess\nthat is true, too, of your 10k remotes).\n\n-Peff\n"},{"id":"322606","messageId":"xmqqmv93spvb.fsf@gitster.mtv.corp.google.com","threadId":"46200","inReplyTo":"20170617115522.lbke5wnle5irbunj@sigill.intra.peff.net","subject":"Re: [RFC/PATCH 0/5] remote: eliminate remote->{fetch,push}_refspec and lazy parsing of refspecs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-19T20:07:36Z","receivedAt":"2017-06-19T20:07:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> If we forget the \"storing it twice\" argument, would it make sense to\n> convert the parallel arrays of items into a single array-of-struct?\n> I.e.:\n>\n>   struct configured_refspec {\n> \tconst char *string;\n> \tstruct refspec refspec;\n> \tunsigned parsed:1;\n>   }\n>\n> I guess that may run into problems where we really need an\n> array-of-refspec to pass into sub-functions. So going further, could we\n> just have \"struct refspec\" store the text form it was parsed from?\n\nI find this a sensible suggestion.  \n\nI think the original \"parallel\" structure was anticipating that a\ntextual input we get from the user (either from the command line or\nfrom the configuration) could expand to multiple parsed refspecs for\neasier use by the code, but it appears that we hadn't seen a need\nfor that, so I think it is safe to unify them into a single struct.\n\nOf course, that still anticipates that a parsed refspec may not be\nunambiguously turned back to textual original input form; if that\nwill never be the case, then the approach taken by the posted\npatches should also be OK.\n"}]}