{"thread":{"id":"56931","subject":"[PATCH v5 0/5] remote: replace static variables with struct remote_state","startedAt":"2021-11-18T00:53:45Z","lastAt":"2021-11-18T17:52:00Z","messageCount":7,"participants":["Glen Choo","Jonathan Tan"],"isPatch":true,"patchVersion":5,"patchTotal":5},"messages":[{"id":"441545","messageId":"20211118005325.64971-1-chooglen@google.com","threadId":"56931","inReplyTo":"id:20211028183101.41013-1-chooglen@google.com","subject":"[PATCH v5 0/5] remote: replace static variables with struct remote_state","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-11-18T00:53:20Z","receivedAt":"2021-11-18T00:53:45Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"This series aims to make the remotes subsystem work with non-the_repository,\nwhich will allow submodule remotes to be accessed in-process, rather than\nthrough child processes. This is accomplished by creating a struct remote_state\nand adding it to struct repository.\n\nThanks for the kind review, Jonathan. I've incorporated most of the feedback\nfrom v4, except squashing patches 2-4. My reasoning is that those changes are\nmechanical and keeping them separate makes it more obvious whether or not each\nstep has been done correctly. However, I agree that they are just intermediate\nsteps that touch the same lines, so I am happy to squash them if other reviewers\nprefer that.\n\nChanges since v4:\n* Fixed an incorrect description of 'git push' without explicit refspecs\n* In patch 5, remove the branches array and keep the hashmap (note that I did\n  not make a similar change to remotes)\n* Drop patch 6 because it is untested in this series (will incorporate into a\n  future series)\n\nChanges since v3:\n* Add a test case for pushing to a remote in detached HEAD. This test\n  would have caught the segfault that resulted in this reroll.\n* Remove the NEEDSWORK saying that init_remotes_hash() should be moved\n  into remote_state_new() and just do it.\n* Remove the backpointer to remote_state and add a remote_state\n  parameter instead.\n* In patch 4, add more remotes_* functions. These functions were not\n  needed in v3 because of the backpointer.\n* In patch 5, add a function that checks if a branch is in a repo. Add a\n  branch hashmap that makes this operation fast.\n* In patch 6, add more repo_* functions. These functions were not needed\n  in v3 because of the backpointer.\n\nChanges since v2:\n* Add .remote_state to struct branch and struct remote, changing the\n  implementation appropriately.\n* In patch 2, properly consider the initialized state of remote_state.\n  In v2, I forgot to convert a static inside read_config() into a\n  private member of struct remote_state. Fix this.\n* In a new patch 3, add helper methods that get a remote via\n  remote_state and the remote name.\n* Move read_config(repo) calls to the external facing-functions. This keeps\n  \"struct repository\" away from the remote.c internals.\n\nChanges since v1:\n* In v1, we moved static variables into the_repository->remote_state in\n  two steps: static variables > static remote_state >\n  the_repository->remote_state. In v2, make this change in one step:\n  static variables > the_repository->remote_state.\n* Add more instances of repo_* that were missed.\n\nGlen Choo (5):\n  t5516: add test case for pushing remote refspecs\n  remote: move static variables into per-repository struct\n  remote: use remote_state parameter internally\n  remote: remove the_repository->remote_state from static methods\n  remote: die if branch is not found in repository\n\n remote.c              | 368 +++++++++++++++++++++++++++++-------------\n remote.h              |  35 ++++\n repository.c          |   8 +\n repository.h          |   4 +\n t/t5516-fetch-push.sh |   9 ++\n 5 files changed, 311 insertions(+), 113 deletions(-)\n\nRange-diff against v4:\n1:  9b29ec27c6 ! 1:  7e60457e11 t5516: add test case for pushing remote refspecs\n    @@ Metadata\n      ## Commit message ##\n         t5516: add test case for pushing remote refspecs\n     \n    -    In detached HEAD, \"git push remote-name\" should push the refspecs in\n    -    remote.remote-name.push. Since there is no test case that checks this\n    -    behavior, add one.\n    +    \"git push remote-name\" (that is, with no refspec given on the command\n    +    line) should push the refspecs in remote.remote-name.push. There is no\n    +    test case that checks this behavior in detached HEAD, so add one.\n     \n         Signed-off-by: Glen Choo <chooglen@google.com>\n     \n    @@ t/t5516-fetch-push.sh: do\n      \n      done\n      \n    -+test_expect_success \"push to remote with detached HEAD and config remote.*.push = src:dest\" '\n    ++test_expect_success \"push to remote with no explicit refspec and config remote.*.push = src:dest\" '\n     +\tmk_test testrepo heads/main &&\n     +\tgit checkout $the_first_commit &&\n     +\ttest_config remote.there.url testrepo &&\n2:  ca9b5ab66a = 2:  ecc637ee74 remote: move static variables into per-repository struct\n3:  5d6a245cae = 3:  a915155979 remote: use remote_state parameter internally\n4:  53f2e31f72 = 4:  8ef43570e9 remote: remove the_repository->remote_state from static methods\n5:  d3281c14eb ! 5:  8bb7bddda4 remote: die if branch is not found in repository\n    @@ Commit message\n     \n         To prevent misuse, add a die_on_missing_branch() helper function that\n         dies if a given branch is not from a given repository. Speed up the\n    -    existence check by using a new branches_hash hashmap to remote_state,\n    -    and use the hashmap to remove the branch array iteration in\n    -    make_branch().\n    +    existence check by replacing the branches list with a branches_hash\n    +    hashmap.\n     \n         Like read_config(), die_on_missing_branch() is only called from\n         non-static functions; static functions are less prone to misuse because\n    @@ remote.c: static void add_merge(struct branch *branch, const char *name)\n     +\tif (ret)\n     +\t\treturn ret;\n      \n    - \tALLOC_GROW(remote_state->branches, remote_state->branches_nr + 1,\n    - \t\t   remote_state->branches_alloc);\n    -@@ remote.c: static struct branch *make_branch(struct remote_state *remote_state,\n    +-\tALLOC_GROW(remote_state->branches, remote_state->branches_nr + 1,\n    +-\t\t   remote_state->branches_alloc);\n    + \tCALLOC_ARRAY(ret, 1);\n    +-\tremote_state->branches[remote_state->branches_nr++] = ret;\n      \tret->name = xstrndup(name, len);\n      \tret->refname = xstrfmt(\"refs/heads/%s\", ret->name);\n      \n    @@ remote.c: struct remote_state *remote_state_new(void)\n      \treturn r;\n      }\n      \n    +@@ remote.c: void remote_state_clear(struct remote_state *remote_state)\n    + \tremote_state->remotes_nr = 0;\n    + \n    + \thashmap_clear_and_free(&remote_state->remotes_hash, struct remote, ent);\n    +-\n    +-\tfor (i = 0; i < remote_state->branches_nr; i++) {\n    +-\t\tFREE_AND_NULL(remote_state->branches[i]);\n    +-\t}\n    +-\tFREE_AND_NULL(remote_state->branches);\n    +-\tremote_state->branches_alloc = 0;\n    +-\tremote_state->branches_nr = 0;\n    ++\thashmap_clear_and_free(&remote_state->branches_hash, struct remote, ent);\n    + }\n     \n      ## remote.h ##\n     @@ remote.h: struct remote_state {\n    - \tstruct branch **branches;\n    - \tint branches_alloc;\n    - \tint branches_nr;\n    + \tint remotes_nr;\n    + \tstruct hashmap remotes_hash;\n    + \n    +-\tstruct branch **branches;\n    +-\tint branches_alloc;\n    +-\tint branches_nr;\n     +\tstruct hashmap branches_hash;\n      \n      \tstruct branch *current_branch;\n6:  0974994cc6 < -:  ---------- remote: add struct repository parameter to external functions\n-- \n2.33.GIT\n\n"},{"id":"441546","messageId":"20211118005325.64971-2-chooglen@google.com","threadId":"56931","inReplyTo":"20211118005325.64971-1-chooglen@google.com","subject":"[PATCH v5 1/5] t5516: add test case for pushing remote refspecs","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-11-18T00:53:21Z","receivedAt":"2021-11-18T00:53:47Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"\"git push remote-name\" (that is, with no refspec given on the command\nline) should push the refspecs in remote.remote-name.push. There is no\ntest case that checks this behavior in detached HEAD, so add one.\n\nSigned-off-by: Glen Choo <chooglen@google.com>\n---\n t/t5516-fetch-push.sh | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 8212ca56dc..7831a38dde 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -541,6 +541,15 @@ do\n \n done\n \n+test_expect_success \"push to remote with no explicit refspec and config remote.*.push = src:dest\" '\n+\tmk_test testrepo heads/main &&\n+\tgit checkout $the_first_commit &&\n+\ttest_config remote.there.url testrepo &&\n+\ttest_config remote.there.push refs/heads/main:refs/heads/main &&\n+\tgit push there &&\n+\tcheck_push_result testrepo $the_commit heads/main\n+'\n+\n test_expect_success 'push with remote.pushdefault' '\n \tmk_test up_repo heads/main &&\n \tmk_test down_repo heads/main &&\n-- \n2.33.GIT\n\n"},{"id":"441547","messageId":"20211118005325.64971-3-chooglen@google.com","threadId":"56931","inReplyTo":"20211118005325.64971-1-chooglen@google.com","subject":"[PATCH v5 2/5] remote: move static variables into per-repository struct","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-11-18T00:53:22Z","receivedAt":"2021-11-18T00:53:50Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"remote.c does not works with non-the_repository because it stores its\nstate as static variables. To support non-the_repository, we can use a\nper-repository struct for the remotes subsystem.\n\nPrepare for this change by defining a struct remote_state that holds\nthe remotes subsystem state and move the static variables of remote.c\ninto the_repository->remote_state.\n\nThis introduces no behavioral or API changes.\n\nSigned-off-by: Glen Choo <chooglen@google.com>\n---\n remote.c     | 191 +++++++++++++++++++++++++++++++++------------------\n remote.h     |  34 +++++++++\n repository.c |   8 +++\n repository.h |   4 ++\n 4 files changed, 170 insertions(+), 67 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex f958543d70..c2bb11242b 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -21,33 +21,6 @@ struct counted_string {\n \tsize_t len;\n \tconst char *s;\n };\n-struct rewrite {\n-\tconst char *base;\n-\tsize_t baselen;\n-\tstruct counted_string *instead_of;\n-\tint instead_of_nr;\n-\tint instead_of_alloc;\n-};\n-struct rewrites {\n-\tstruct rewrite **rewrite;\n-\tint rewrite_alloc;\n-\tint rewrite_nr;\n-};\n-\n-static struct remote **remotes;\n-static int remotes_alloc;\n-static int remotes_nr;\n-static struct hashmap remotes_hash;\n-\n-static struct branch **branches;\n-static int branches_alloc;\n-static int branches_nr;\n-\n-static struct branch *current_branch;\n-static const char *pushremote_name;\n-\n-static struct rewrites rewrites;\n-static struct rewrites rewrites_push;\n \n static int valid_remote(const struct remote *remote)\n {\n@@ -94,14 +67,16 @@ static void add_pushurl(struct remote *remote, const char *pushurl)\n \n static void add_pushurl_alias(struct remote *remote, const char *url)\n {\n-\tconst char *pushurl = alias_url(url, &rewrites_push);\n+\tconst char *pushurl =\n+\t\talias_url(url, &the_repository->remote_state->rewrites_push);\n \tif (pushurl != url)\n \t\tadd_pushurl(remote, pushurl);\n }\n \n static void add_url_alias(struct remote *remote, const char *url)\n {\n-\tadd_url(remote, alias_url(url, &rewrites));\n+\tadd_url(remote,\n+\t\talias_url(url, &the_repository->remote_state->rewrites));\n \tadd_pushurl_alias(remote, url);\n }\n \n@@ -127,12 +102,6 @@ static int remotes_hash_cmp(const void *unused_cmp_data,\n \t\treturn strcmp(a->name, b->name);\n }\n \n-static inline void init_remotes_hash(void)\n-{\n-\tif (!remotes_hash.cmpfn)\n-\t\thashmap_init(&remotes_hash, remotes_hash_cmp, NULL, 0);\n-}\n-\n static struct remote *make_remote(const char *name, int len)\n {\n \tstruct remote *ret;\n@@ -142,12 +111,12 @@ static struct remote *make_remote(const char *name, int len)\n \tif (!len)\n \t\tlen = strlen(name);\n \n-\tinit_remotes_hash();\n \tlookup.str = name;\n \tlookup.len = len;\n \thashmap_entry_init(&lookup_entry, memhash(name, len));\n \n-\te = hashmap_get(&remotes_hash, &lookup_entry, &lookup);\n+\te = hashmap_get(&the_repository->remote_state->remotes_hash,\n+\t\t\t&lookup_entry, &lookup);\n \tif (e)\n \t\treturn container_of(e, struct remote, ent);\n \n@@ -158,15 +127,41 @@ static struct remote *make_remote(const char *name, int len)\n \trefspec_init(&ret->push, REFSPEC_PUSH);\n \trefspec_init(&ret->fetch, REFSPEC_FETCH);\n \n-\tALLOC_GROW(remotes, remotes_nr + 1, remotes_alloc);\n-\tremotes[remotes_nr++] = ret;\n+\tALLOC_GROW(the_repository->remote_state->remotes,\n+\t\t   the_repository->remote_state->remotes_nr + 1,\n+\t\t   the_repository->remote_state->remotes_alloc);\n+\tthe_repository->remote_state\n+\t\t->remotes[the_repository->remote_state->remotes_nr++] = ret;\n \n \thashmap_entry_init(&ret->ent, lookup_entry.hash);\n-\tif (hashmap_put_entry(&remotes_hash, ret, ent))\n+\tif (hashmap_put_entry(&the_repository->remote_state->remotes_hash, ret,\n+\t\t\t      ent))\n \t\tBUG(\"hashmap_put overwrote entry after hashmap_get returned NULL\");\n \treturn ret;\n }\n \n+static void remote_clear(struct remote *remote)\n+{\n+\tint i;\n+\n+\tfree((char *)remote->name);\n+\tfree((char *)remote->foreign_vcs);\n+\n+\tfor (i = 0; i < remote->url_nr; i++) {\n+\t\tfree((char *)remote->url[i]);\n+\t}\n+\tFREE_AND_NULL(remote->pushurl);\n+\n+\tfor (i = 0; i < remote->pushurl_nr; i++) {\n+\t\tfree((char *)remote->pushurl[i]);\n+\t}\n+\tFREE_AND_NULL(remote->pushurl);\n+\tfree((char *)remote->receivepack);\n+\tfree((char *)remote->uploadpack);\n+\tFREE_AND_NULL(remote->http_proxy);\n+\tFREE_AND_NULL(remote->http_proxy_authmethod);\n+}\n+\n static void add_merge(struct branch *branch, const char *name)\n {\n \tALLOC_GROW(branch->merge_name, branch->merge_nr + 1,\n@@ -179,15 +174,20 @@ static struct branch *make_branch(const char *name, size_t len)\n \tstruct branch *ret;\n \tint i;\n \n-\tfor (i = 0; i < branches_nr; i++) {\n-\t\tif (!strncmp(name, branches[i]->name, len) &&\n-\t\t    !branches[i]->name[len])\n-\t\t\treturn branches[i];\n+\tfor (i = 0; i < the_repository->remote_state->branches_nr; i++) {\n+\t\tif (!strncmp(name,\n+\t\t\t     the_repository->remote_state->branches[i]->name,\n+\t\t\t     len) &&\n+\t\t    !the_repository->remote_state->branches[i]->name[len])\n+\t\t\treturn the_repository->remote_state->branches[i];\n \t}\n \n-\tALLOC_GROW(branches, branches_nr + 1, branches_alloc);\n+\tALLOC_GROW(the_repository->remote_state->branches,\n+\t\t   the_repository->remote_state->branches_nr + 1,\n+\t\t   the_repository->remote_state->branches_alloc);\n \tCALLOC_ARRAY(ret, 1);\n-\tbranches[branches_nr++] = ret;\n+\tthe_repository->remote_state\n+\t\t->branches[the_repository->remote_state->branches_nr++] = ret;\n \tret->name = xstrndup(name, len);\n \tret->refname = xstrfmt(\"refs/heads/%s\", ret->name);\n \n@@ -327,12 +327,16 @@ static int handle_config(const char *key, const char *value, void *cb)\n \t\tif (!strcmp(subkey, \"insteadof\")) {\n \t\t\tif (!value)\n \t\t\t\treturn config_error_nonbool(key);\n-\t\t\trewrite = make_rewrite(&rewrites, name, namelen);\n+\t\t\trewrite = make_rewrite(\n+\t\t\t\t&the_repository->remote_state->rewrites, name,\n+\t\t\t\tnamelen);\n \t\t\tadd_instead_of(rewrite, xstrdup(value));\n \t\t} else if (!strcmp(subkey, \"pushinsteadof\")) {\n \t\t\tif (!value)\n \t\t\t\treturn config_error_nonbool(key);\n-\t\t\trewrite = make_rewrite(&rewrites_push, name, namelen);\n+\t\t\trewrite = make_rewrite(\n+\t\t\t\t&the_repository->remote_state->rewrites_push,\n+\t\t\t\tname, namelen);\n \t\t\tadd_instead_of(rewrite, xstrdup(value));\n \t\t}\n \t}\n@@ -342,7 +346,9 @@ static int handle_config(const char *key, const char *value, void *cb)\n \n \t/* Handle remote.* variables */\n \tif (!name && !strcmp(subkey, \"pushdefault\"))\n-\t\treturn git_config_string(&pushremote_name, key, value);\n+\t\treturn git_config_string(\n+\t\t\t&the_repository->remote_state->pushremote_name, key,\n+\t\t\tvalue);\n \n \tif (!name)\n \t\treturn 0;\n@@ -425,18 +431,34 @@ static int handle_config(const char *key, const char *value, void *cb)\n static void alias_all_urls(void)\n {\n \tint i, j;\n-\tfor (i = 0; i < remotes_nr; i++) {\n+\tfor (i = 0; i < the_repository->remote_state->remotes_nr; i++) {\n \t\tint add_pushurl_aliases;\n-\t\tif (!remotes[i])\n+\t\tif (!the_repository->remote_state->remotes[i])\n \t\t\tcontinue;\n-\t\tfor (j = 0; j < remotes[i]->pushurl_nr; j++) {\n-\t\t\tremotes[i]->pushurl[j] = alias_url(remotes[i]->pushurl[j], &rewrites);\n+\t\tfor (j = 0;\n+\t\t     j < the_repository->remote_state->remotes[i]->pushurl_nr;\n+\t\t     j++) {\n+\t\t\tthe_repository->remote_state->remotes[i]->pushurl[j] =\n+\t\t\t\talias_url(\n+\t\t\t\t\tthe_repository->remote_state->remotes[i]\n+\t\t\t\t\t\t->pushurl[j],\n+\t\t\t\t\t&the_repository->remote_state->rewrites);\n \t\t}\n-\t\tadd_pushurl_aliases = remotes[i]->pushurl_nr == 0;\n-\t\tfor (j = 0; j < remotes[i]->url_nr; j++) {\n+\t\tadd_pushurl_aliases =\n+\t\t\tthe_repository->remote_state->remotes[i]->pushurl_nr ==\n+\t\t\t0;\n+\t\tfor (j = 0;\n+\t\t     j < the_repository->remote_state->remotes[i]->url_nr;\n+\t\t     j++) {\n \t\t\tif (add_pushurl_aliases)\n-\t\t\t\tadd_pushurl_alias(remotes[i], remotes[i]->url[j]);\n-\t\t\tremotes[i]->url[j] = alias_url(remotes[i]->url[j], &rewrites);\n+\t\t\t\tadd_pushurl_alias(\n+\t\t\t\t\tthe_repository->remote_state->remotes[i],\n+\t\t\t\t\tthe_repository->remote_state->remotes[i]\n+\t\t\t\t\t\t->url[j]);\n+\t\t\tthe_repository->remote_state->remotes[i]\n+\t\t\t\t->url[j] = alias_url(\n+\t\t\t\tthe_repository->remote_state->remotes[i]->url[j],\n+\t\t\t\t&the_repository->remote_state->rewrites);\n \t\t}\n \t}\n }\n@@ -450,12 +472,13 @@ static void read_config(void)\n \t\treturn;\n \tloaded = 1;\n \n-\tcurrent_branch = NULL;\n+\tthe_repository->remote_state->current_branch = NULL;\n \tif (startup_info->have_repository) {\n \t\tconst char *head_ref = resolve_ref_unsafe(\"HEAD\", 0, NULL, &flag);\n \t\tif (head_ref && (flag & REF_ISSYMREF) &&\n \t\t    skip_prefix(head_ref, \"refs/heads/\", &head_ref)) {\n-\t\t\tcurrent_branch = make_branch(head_ref, strlen(head_ref));\n+\t\t\tthe_repository->remote_state->current_branch =\n+\t\t\t\tmake_branch(head_ref, strlen(head_ref));\n \t\t}\n \t}\n \tgit_config(handle_config, NULL);\n@@ -493,10 +516,10 @@ const char *pushremote_for_branch(struct branch *branch, int *explicit)\n \t\t\t*explicit = 1;\n \t\treturn branch->pushremote_name;\n \t}\n-\tif (pushremote_name) {\n+\tif (the_repository->remote_state->pushremote_name) {\n \t\tif (explicit)\n \t\t\t*explicit = 1;\n-\t\treturn pushremote_name;\n+\t\treturn the_repository->remote_state->pushremote_name;\n \t}\n \treturn remote_for_branch(branch, explicit);\n }\n@@ -534,7 +557,8 @@ static struct remote *remote_get_1(const char *name,\n \tif (name)\n \t\tname_given = 1;\n \telse\n-\t\tname = get_default(current_branch, &name_given);\n+\t\tname = get_default(the_repository->remote_state->current_branch,\n+\t\t\t\t   &name_given);\n \n \tret = make_remote(name, 0);\n \tif (valid_remote_nick(name) && have_git_dir()) {\n@@ -573,11 +597,13 @@ int for_each_remote(each_remote_fn fn, void *priv)\n {\n \tint i, result = 0;\n \tread_config();\n-\tfor (i = 0; i < remotes_nr && !result; i++) {\n-\t\tstruct remote *r = remotes[i];\n-\t\tif (!r)\n+\tfor (i = 0; i < the_repository->remote_state->remotes_nr && !result;\n+\t     i++) {\n+\t\tstruct remote *remote =\n+\t\t\tthe_repository->remote_state->remotes[i];\n+\t\tif (!remote)\n \t\t\tcontinue;\n-\t\tresult = fn(r, priv);\n+\t\tresult = fn(remote, priv);\n \t}\n \treturn result;\n }\n@@ -1685,7 +1711,7 @@ struct branch *branch_get(const char *name)\n \n \tread_config();\n \tif (!name || !*name || !strcmp(name, \"HEAD\"))\n-\t\tret = current_branch;\n+\t\tret = the_repository->remote_state->current_branch;\n \telse\n \t\tret = make_branch(name, strlen(name));\n \tset_merge(ret);\n@@ -2585,3 +2611,34 @@ void apply_push_cas(struct push_cas_option *cas,\n \t\t\tcheck_if_includes_upstream(ref);\n \t}\n }\n+\n+struct remote_state *remote_state_new(void)\n+{\n+\tstruct remote_state *r = xmalloc(sizeof(*r));\n+\n+\tmemset(r, 0, sizeof(*r));\n+\n+\thashmap_init(&r->remotes_hash, remotes_hash_cmp, NULL, 0);\n+\treturn r;\n+}\n+\n+void remote_state_clear(struct remote_state *remote_state)\n+{\n+\tint i;\n+\n+\tfor (i = 0; i < remote_state->remotes_nr; i++) {\n+\t\tremote_clear(remote_state->remotes[i]);\n+\t}\n+\tFREE_AND_NULL(remote_state->remotes);\n+\tremote_state->remotes_alloc = 0;\n+\tremote_state->remotes_nr = 0;\n+\n+\thashmap_clear_and_free(&remote_state->remotes_hash, struct remote, ent);\n+\n+\tfor (i = 0; i < remote_state->branches_nr; i++) {\n+\t\tFREE_AND_NULL(remote_state->branches[i]);\n+\t}\n+\tFREE_AND_NULL(remote_state->branches);\n+\tremote_state->branches_alloc = 0;\n+\tremote_state->branches_nr = 0;\n+}\ndiff --git a/remote.h b/remote.h\nindex 5a59198252..d21c035f1b 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -23,6 +23,40 @@ enum {\n \tREMOTE_BRANCHES\n };\n \n+struct rewrite {\n+\tconst char *base;\n+\tsize_t baselen;\n+\tstruct counted_string *instead_of;\n+\tint instead_of_nr;\n+\tint instead_of_alloc;\n+};\n+\n+struct rewrites {\n+\tstruct rewrite **rewrite;\n+\tint rewrite_alloc;\n+\tint rewrite_nr;\n+};\n+\n+struct remote_state {\n+\tstruct remote **remotes;\n+\tint remotes_alloc;\n+\tint remotes_nr;\n+\tstruct hashmap remotes_hash;\n+\n+\tstruct branch **branches;\n+\tint branches_alloc;\n+\tint branches_nr;\n+\n+\tstruct branch *current_branch;\n+\tconst char *pushremote_name;\n+\n+\tstruct rewrites rewrites;\n+\tstruct rewrites rewrites_push;\n+};\n+\n+void remote_state_clear(struct remote_state *remote_state);\n+struct remote_state *remote_state_new(void);\n+\n struct remote {\n \tstruct hashmap_entry ent;\n \ndiff --git a/repository.c b/repository.c\nindex c5b90ba93e..c7ea706c20 100644\n--- a/repository.c\n+++ b/repository.c\n@@ -9,6 +9,7 @@\n #include \"config.h\"\n #include \"object.h\"\n #include \"lockfile.h\"\n+#include \"remote.h\"\n #include \"submodule-config.h\"\n #include \"sparse-index.h\"\n #include \"promisor-remote.h\"\n@@ -24,6 +25,7 @@ void initialize_the_repository(void)\n \n \tthe_repo.index = &the_index;\n \tthe_repo.objects = raw_object_store_new();\n+\tthe_repo.remote_state = remote_state_new();\n \tthe_repo.parsed_objects = parsed_object_pool_new();\n \n \trepo_set_hash_algo(&the_repo, GIT_HASH_SHA1);\n@@ -164,6 +166,7 @@ int repo_init(struct repository *repo,\n \n \trepo->objects = raw_object_store_new();\n \trepo->parsed_objects = parsed_object_pool_new();\n+\trepo->remote_state = remote_state_new();\n \n \tif (repo_init_gitdir(repo, gitdir))\n \t\tgoto error;\n@@ -270,6 +273,11 @@ void repo_clear(struct repository *repo)\n \t\tpromisor_remote_clear(repo->promisor_remote_config);\n \t\tFREE_AND_NULL(repo->promisor_remote_config);\n \t}\n+\n+\tif (repo->remote_state) {\n+\t\tremote_state_clear(repo->remote_state);\n+\t\tFREE_AND_NULL(repo->remote_state);\n+\t}\n }\n \n int repo_read_index(struct repository *repo)\ndiff --git a/repository.h b/repository.h\nindex a057653981..98f9583470 100644\n--- a/repository.h\n+++ b/repository.h\n@@ -11,6 +11,7 @@ struct pathspec;\n struct raw_object_store;\n struct submodule_cache;\n struct promisor_remote_config;\n+struct remote_state;\n \n enum untracked_cache_setting {\n \tUNTRACKED_CACHE_KEEP,\n@@ -127,6 +128,9 @@ struct repository {\n \t */\n \tstruct index_state *index;\n \n+\t/* Repository's remotes and associated structures. */\n+\tstruct remote_state *remote_state;\n+\n \t/* Repository's current hash algorithm, as serialized on disk. */\n \tconst struct git_hash_algo *hash_algo;\n \n-- \n2.33.GIT\n\n"},{"id":"441548","messageId":"20211118005325.64971-4-chooglen@google.com","threadId":"56931","inReplyTo":"20211118005325.64971-1-chooglen@google.com","subject":"[PATCH v5 3/5] remote: use remote_state parameter internally","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-11-18T00:53:23Z","receivedAt":"2021-11-18T00:53:51Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Without changing external-facing functions, replace\nthe_repository->remote_state internally by adding a struct remote_state\nparameter.\n\nAs a result, external-facing functions are still tied to the_repository,\nbut most static functions no longer reference\nthe_repository->remote_state. The exceptions are those that are used in\na way that depends on external-facing functions e.g. the callbacks to\nremote_get_1().\n\nSigned-off-by: Glen Choo <chooglen@google.com>\n---\n remote.c | 159 +++++++++++++++++++++++++------------------------------\n remote.h |   2 +\n 2 files changed, 75 insertions(+), 86 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex c2bb11242b..48374cc0e2 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -65,19 +65,19 @@ static void add_pushurl(struct remote *remote, const char *pushurl)\n \tremote->pushurl[remote->pushurl_nr++] = pushurl;\n }\n \n-static void add_pushurl_alias(struct remote *remote, const char *url)\n+static void add_pushurl_alias(struct remote_state *remote_state,\n+\t\t\t      struct remote *remote, const char *url)\n {\n-\tconst char *pushurl =\n-\t\talias_url(url, &the_repository->remote_state->rewrites_push);\n+\tconst char *pushurl = alias_url(url, &remote_state->rewrites_push);\n \tif (pushurl != url)\n \t\tadd_pushurl(remote, pushurl);\n }\n \n-static void add_url_alias(struct remote *remote, const char *url)\n+static void add_url_alias(struct remote_state *remote_state,\n+\t\t\t  struct remote *remote, const char *url)\n {\n-\tadd_url(remote,\n-\t\talias_url(url, &the_repository->remote_state->rewrites));\n-\tadd_pushurl_alias(remote, url);\n+\tadd_url(remote, alias_url(url, &remote_state->rewrites));\n+\tadd_pushurl_alias(remote_state, remote, url);\n }\n \n struct remotes_hash_key {\n@@ -102,7 +102,8 @@ static int remotes_hash_cmp(const void *unused_cmp_data,\n \t\treturn strcmp(a->name, b->name);\n }\n \n-static struct remote *make_remote(const char *name, int len)\n+static struct remote *make_remote(struct remote_state *remote_state,\n+\t\t\t\t  const char *name, int len)\n {\n \tstruct remote *ret;\n \tstruct remotes_hash_key lookup;\n@@ -115,8 +116,7 @@ static struct remote *make_remote(const char *name, int len)\n \tlookup.len = len;\n \thashmap_entry_init(&lookup_entry, memhash(name, len));\n \n-\te = hashmap_get(&the_repository->remote_state->remotes_hash,\n-\t\t\t&lookup_entry, &lookup);\n+\te = hashmap_get(&remote_state->remotes_hash, &lookup_entry, &lookup);\n \tif (e)\n \t\treturn container_of(e, struct remote, ent);\n \n@@ -127,15 +127,12 @@ static struct remote *make_remote(const char *name, int len)\n \trefspec_init(&ret->push, REFSPEC_PUSH);\n \trefspec_init(&ret->fetch, REFSPEC_FETCH);\n \n-\tALLOC_GROW(the_repository->remote_state->remotes,\n-\t\t   the_repository->remote_state->remotes_nr + 1,\n-\t\t   the_repository->remote_state->remotes_alloc);\n-\tthe_repository->remote_state\n-\t\t->remotes[the_repository->remote_state->remotes_nr++] = ret;\n+\tALLOC_GROW(remote_state->remotes, remote_state->remotes_nr + 1,\n+\t\t   remote_state->remotes_alloc);\n+\tremote_state->remotes[remote_state->remotes_nr++] = ret;\n \n \thashmap_entry_init(&ret->ent, lookup_entry.hash);\n-\tif (hashmap_put_entry(&the_repository->remote_state->remotes_hash, ret,\n-\t\t\t      ent))\n+\tif (hashmap_put_entry(&remote_state->remotes_hash, ret, ent))\n \t\tBUG(\"hashmap_put overwrote entry after hashmap_get returned NULL\");\n \treturn ret;\n }\n@@ -169,25 +166,22 @@ static void add_merge(struct branch *branch, const char *name)\n \tbranch->merge_name[branch->merge_nr++] = name;\n }\n \n-static struct branch *make_branch(const char *name, size_t len)\n+static struct branch *make_branch(struct remote_state *remote_state,\n+\t\t\t\t  const char *name, size_t len)\n {\n \tstruct branch *ret;\n \tint i;\n \n-\tfor (i = 0; i < the_repository->remote_state->branches_nr; i++) {\n-\t\tif (!strncmp(name,\n-\t\t\t     the_repository->remote_state->branches[i]->name,\n-\t\t\t     len) &&\n-\t\t    !the_repository->remote_state->branches[i]->name[len])\n-\t\t\treturn the_repository->remote_state->branches[i];\n+\tfor (i = 0; i < remote_state->branches_nr; i++) {\n+\t\tif (!strncmp(name, remote_state->branches[i]->name, len) &&\n+\t\t    !remote_state->branches[i]->name[len])\n+\t\t\treturn remote_state->branches[i];\n \t}\n \n-\tALLOC_GROW(the_repository->remote_state->branches,\n-\t\t   the_repository->remote_state->branches_nr + 1,\n-\t\t   the_repository->remote_state->branches_alloc);\n+\tALLOC_GROW(remote_state->branches, remote_state->branches_nr + 1,\n+\t\t   remote_state->branches_alloc);\n \tCALLOC_ARRAY(ret, 1);\n-\tthe_repository->remote_state\n-\t\t->branches[the_repository->remote_state->branches_nr++] = ret;\n+\tremote_state->branches[remote_state->branches_nr++] = ret;\n \tret->name = xstrndup(name, len);\n \tret->refname = xstrfmt(\"refs/heads/%s\", ret->name);\n \n@@ -229,7 +223,8 @@ static const char *skip_spaces(const char *s)\n \treturn s;\n }\n \n-static void read_remotes_file(struct remote *remote)\n+static void read_remotes_file(struct remote_state *remote_state,\n+\t\t\t      struct remote *remote)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \tFILE *f = fopen_or_warn(git_path(\"remotes/%s\", remote->name), \"r\");\n@@ -244,7 +239,8 @@ static void read_remotes_file(struct remote *remote)\n \t\tstrbuf_rtrim(&buf);\n \n \t\tif (skip_prefix(buf.buf, \"URL:\", &v))\n-\t\t\tadd_url_alias(remote, xstrdup(skip_spaces(v)));\n+\t\t\tadd_url_alias(remote_state, remote,\n+\t\t\t\t      xstrdup(skip_spaces(v)));\n \t\telse if (skip_prefix(buf.buf, \"Push:\", &v))\n \t\t\trefspec_append(&remote->push, skip_spaces(v));\n \t\telse if (skip_prefix(buf.buf, \"Pull:\", &v))\n@@ -254,7 +250,8 @@ static void read_remotes_file(struct remote *remote)\n \tfclose(f);\n }\n \n-static void read_branches_file(struct remote *remote)\n+static void read_branches_file(struct remote_state *remote_state,\n+\t\t\t       struct remote *remote)\n {\n \tchar *frag;\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -286,7 +283,7 @@ static void read_branches_file(struct remote *remote)\n \telse\n \t\tfrag = (char *)git_default_branch_name(0);\n \n-\tadd_url_alias(remote, strbuf_detach(&buf, NULL));\n+\tadd_url_alias(remote_state, remote, strbuf_detach(&buf, NULL));\n \trefspec_appendf(&remote->fetch, \"refs/heads/%s:refs/heads/%s\",\n \t\t\tfrag, remote->name);\n \n@@ -305,10 +302,12 @@ static int handle_config(const char *key, const char *value, void *cb)\n \tconst char *subkey;\n \tstruct remote *remote;\n \tstruct branch *branch;\n+\tstruct remote_state *remote_state = cb;\n+\n \tif (parse_config_key(key, \"branch\", &name, &namelen, &subkey) >= 0) {\n \t\tif (!name)\n \t\t\treturn 0;\n-\t\tbranch = make_branch(name, namelen);\n+\t\tbranch = make_branch(remote_state, name, namelen);\n \t\tif (!strcmp(subkey, \"remote\")) {\n \t\t\treturn git_config_string(&branch->remote_name, key, value);\n \t\t} else if (!strcmp(subkey, \"pushremote\")) {\n@@ -327,16 +326,14 @@ static int handle_config(const char *key, const char *value, void *cb)\n \t\tif (!strcmp(subkey, \"insteadof\")) {\n \t\t\tif (!value)\n \t\t\t\treturn config_error_nonbool(key);\n-\t\t\trewrite = make_rewrite(\n-\t\t\t\t&the_repository->remote_state->rewrites, name,\n-\t\t\t\tnamelen);\n+\t\t\trewrite = make_rewrite(&remote_state->rewrites, name,\n+\t\t\t\t\t       namelen);\n \t\t\tadd_instead_of(rewrite, xstrdup(value));\n \t\t} else if (!strcmp(subkey, \"pushinsteadof\")) {\n \t\t\tif (!value)\n \t\t\t\treturn config_error_nonbool(key);\n-\t\t\trewrite = make_rewrite(\n-\t\t\t\t&the_repository->remote_state->rewrites_push,\n-\t\t\t\tname, namelen);\n+\t\t\trewrite = make_rewrite(&remote_state->rewrites_push,\n+\t\t\t\t\t       name, namelen);\n \t\t\tadd_instead_of(rewrite, xstrdup(value));\n \t\t}\n \t}\n@@ -346,9 +343,8 @@ static int handle_config(const char *key, const char *value, void *cb)\n \n \t/* Handle remote.* variables */\n \tif (!name && !strcmp(subkey, \"pushdefault\"))\n-\t\treturn git_config_string(\n-\t\t\t&the_repository->remote_state->pushremote_name, key,\n-\t\t\tvalue);\n+\t\treturn git_config_string(&remote_state->pushremote_name, key,\n+\t\t\t\t\t value);\n \n \tif (!name)\n \t\treturn 0;\n@@ -358,7 +354,7 @@ static int handle_config(const char *key, const char *value, void *cb)\n \t\t\tname);\n \t\treturn 0;\n \t}\n-\tremote = make_remote(name, namelen);\n+\tremote = make_remote(remote_state, name, namelen);\n \tremote->origin = REMOTE_CONFIG;\n \tif (current_config_scope() == CONFIG_SCOPE_LOCAL ||\n \t    current_config_scope() == CONFIG_SCOPE_WORKTREE)\n@@ -428,61 +424,51 @@ static int handle_config(const char *key, const char *value, void *cb)\n \treturn 0;\n }\n \n-static void alias_all_urls(void)\n+static void alias_all_urls(struct remote_state *remote_state)\n {\n \tint i, j;\n-\tfor (i = 0; i < the_repository->remote_state->remotes_nr; i++) {\n+\tfor (i = 0; i < remote_state->remotes_nr; i++) {\n \t\tint add_pushurl_aliases;\n-\t\tif (!the_repository->remote_state->remotes[i])\n+\t\tif (!remote_state->remotes[i])\n \t\t\tcontinue;\n-\t\tfor (j = 0;\n-\t\t     j < the_repository->remote_state->remotes[i]->pushurl_nr;\n-\t\t     j++) {\n-\t\t\tthe_repository->remote_state->remotes[i]->pushurl[j] =\n-\t\t\t\talias_url(\n-\t\t\t\t\tthe_repository->remote_state->remotes[i]\n-\t\t\t\t\t\t->pushurl[j],\n-\t\t\t\t\t&the_repository->remote_state->rewrites);\n+\t\tfor (j = 0; j < remote_state->remotes[i]->pushurl_nr; j++) {\n+\t\t\tremote_state->remotes[i]->pushurl[j] =\n+\t\t\t\talias_url(remote_state->remotes[i]->pushurl[j],\n+\t\t\t\t\t  &remote_state->rewrites);\n \t\t}\n-\t\tadd_pushurl_aliases =\n-\t\t\tthe_repository->remote_state->remotes[i]->pushurl_nr ==\n-\t\t\t0;\n-\t\tfor (j = 0;\n-\t\t     j < the_repository->remote_state->remotes[i]->url_nr;\n-\t\t     j++) {\n+\t\tadd_pushurl_aliases = remote_state->remotes[i]->pushurl_nr == 0;\n+\t\tfor (j = 0; j < remote_state->remotes[i]->url_nr; j++) {\n \t\t\tif (add_pushurl_aliases)\n \t\t\t\tadd_pushurl_alias(\n-\t\t\t\t\tthe_repository->remote_state->remotes[i],\n-\t\t\t\t\tthe_repository->remote_state->remotes[i]\n-\t\t\t\t\t\t->url[j]);\n-\t\t\tthe_repository->remote_state->remotes[i]\n-\t\t\t\t->url[j] = alias_url(\n-\t\t\t\tthe_repository->remote_state->remotes[i]->url[j],\n-\t\t\t\t&the_repository->remote_state->rewrites);\n+\t\t\t\t\tremote_state, remote_state->remotes[i],\n+\t\t\t\t\tremote_state->remotes[i]->url[j]);\n+\t\t\tremote_state->remotes[i]->url[j] =\n+\t\t\t\talias_url(remote_state->remotes[i]->url[j],\n+\t\t\t\t\t  &remote_state->rewrites);\n \t\t}\n \t}\n }\n \n-static void read_config(void)\n+static void read_config(struct repository *repo)\n {\n-\tstatic int loaded;\n \tint flag;\n \n-\tif (loaded)\n+\tif (repo->remote_state->initialized)\n \t\treturn;\n-\tloaded = 1;\n+\trepo->remote_state->initialized = 1;\n \n-\tthe_repository->remote_state->current_branch = NULL;\n+\trepo->remote_state->current_branch = NULL;\n \tif (startup_info->have_repository) {\n-\t\tconst char *head_ref = resolve_ref_unsafe(\"HEAD\", 0, NULL, &flag);\n+\t\tconst char *head_ref = refs_resolve_ref_unsafe(\n+\t\t\tget_main_ref_store(repo), \"HEAD\", 0, NULL, &flag);\n \t\tif (head_ref && (flag & REF_ISSYMREF) &&\n \t\t    skip_prefix(head_ref, \"refs/heads/\", &head_ref)) {\n-\t\t\tthe_repository->remote_state->current_branch =\n-\t\t\t\tmake_branch(head_ref, strlen(head_ref));\n+\t\t\trepo->remote_state->current_branch = make_branch(\n+\t\t\t\trepo->remote_state, head_ref, strlen(head_ref));\n \t\t}\n \t}\n-\tgit_config(handle_config, NULL);\n-\talias_all_urls();\n+\trepo_config(repo, handle_config, repo->remote_state);\n+\talias_all_urls(repo->remote_state);\n }\n \n static int valid_remote_nick(const char *name)\n@@ -552,7 +538,7 @@ static struct remote *remote_get_1(const char *name,\n \tstruct remote *ret;\n \tint name_given = 0;\n \n-\tread_config();\n+\tread_config(the_repository);\n \n \tif (name)\n \t\tname_given = 1;\n@@ -560,15 +546,15 @@ static struct remote *remote_get_1(const char *name,\n \t\tname = get_default(the_repository->remote_state->current_branch,\n \t\t\t\t   &name_given);\n \n-\tret = make_remote(name, 0);\n+\tret = make_remote(the_repository->remote_state, name, 0);\n \tif (valid_remote_nick(name) && have_git_dir()) {\n \t\tif (!valid_remote(ret))\n-\t\t\tread_remotes_file(ret);\n+\t\t\tread_remotes_file(the_repository->remote_state, ret);\n \t\tif (!valid_remote(ret))\n-\t\t\tread_branches_file(ret);\n+\t\t\tread_branches_file(the_repository->remote_state, ret);\n \t}\n \tif (name_given && !valid_remote(ret))\n-\t\tadd_url_alias(ret, name);\n+\t\tadd_url_alias(the_repository->remote_state, ret, name);\n \tif (!valid_remote(ret))\n \t\treturn NULL;\n \treturn ret;\n@@ -596,7 +582,7 @@ int remote_is_configured(struct remote *remote, int in_repo)\n int for_each_remote(each_remote_fn fn, void *priv)\n {\n \tint i, result = 0;\n-\tread_config();\n+\tread_config(the_repository);\n \tfor (i = 0; i < the_repository->remote_state->remotes_nr && !result;\n \t     i++) {\n \t\tstruct remote *remote =\n@@ -1709,11 +1695,12 @@ struct branch *branch_get(const char *name)\n {\n \tstruct branch *ret;\n \n-\tread_config();\n+\tread_config(the_repository);\n \tif (!name || !*name || !strcmp(name, \"HEAD\"))\n \t\tret = the_repository->remote_state->current_branch;\n \telse\n-\t\tret = make_branch(name, strlen(name));\n+\t\tret = make_branch(the_repository->remote_state, name,\n+\t\t\t\t  strlen(name));\n \tset_merge(ret);\n \treturn ret;\n }\ndiff --git a/remote.h b/remote.h\nindex d21c035f1b..85a730b8ef 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -52,6 +52,8 @@ struct remote_state {\n \n \tstruct rewrites rewrites;\n \tstruct rewrites rewrites_push;\n+\n+\tint initialized;\n };\n \n void remote_state_clear(struct remote_state *remote_state);\n-- \n2.33.GIT\n\n"},{"id":"441549","messageId":"20211118005325.64971-5-chooglen@google.com","threadId":"56931","inReplyTo":"20211118005325.64971-1-chooglen@google.com","subject":"[PATCH v5 4/5] remote: remove the_repository->remote_state from static methods","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-11-18T00:53:24Z","receivedAt":"2021-11-18T00:53:55Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Replace all remaining references of the_repository->remote_state in\nstatic functions with a struct remote_state parameter.\n\nTo do so, move read_config() calls to non-static functions and create a\nfamily of static functions, \"remotes_*\", that behave like \"repo_*\", but\naccept struct remote_state instead of struct repository. In the case\nwhere a static function calls a non-static function, replace the\nnon-static function with its \"remotes_*\" equivalent.\n\nSigned-off-by: Glen Choo <chooglen@google.com>\n---\n remote.c | 96 +++++++++++++++++++++++++++++++++++++++++---------------\n 1 file changed, 71 insertions(+), 25 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex 48374cc0e2..88697e7e6d 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -483,7 +483,9 @@ static int valid_remote_nick(const char *name)\n \treturn 1;\n }\n \n-const char *remote_for_branch(struct branch *branch, int *explicit)\n+static const char *remotes_remote_for_branch(struct remote_state *remote_state,\n+\t\t\t\t\t     struct branch *branch,\n+\t\t\t\t\t     int *explicit)\n {\n \tif (branch && branch->remote_name) {\n \t\tif (explicit)\n@@ -495,32 +497,55 @@ const char *remote_for_branch(struct branch *branch, int *explicit)\n \treturn \"origin\";\n }\n \n-const char *pushremote_for_branch(struct branch *branch, int *explicit)\n+const char *remote_for_branch(struct branch *branch, int *explicit)\n+{\n+\tread_config(the_repository);\n+\treturn remotes_remote_for_branch(the_repository->remote_state, branch,\n+\t\t\t\t\t explicit);\n+}\n+\n+static const char *\n+remotes_pushremote_for_branch(struct remote_state *remote_state,\n+\t\t\t      struct branch *branch, int *explicit)\n {\n \tif (branch && branch->pushremote_name) {\n \t\tif (explicit)\n \t\t\t*explicit = 1;\n \t\treturn branch->pushremote_name;\n \t}\n-\tif (the_repository->remote_state->pushremote_name) {\n+\tif (remote_state->pushremote_name) {\n \t\tif (explicit)\n \t\t\t*explicit = 1;\n-\t\treturn the_repository->remote_state->pushremote_name;\n+\t\treturn remote_state->pushremote_name;\n \t}\n-\treturn remote_for_branch(branch, explicit);\n+\treturn remotes_remote_for_branch(remote_state, branch, explicit);\n }\n \n+const char *pushremote_for_branch(struct branch *branch, int *explicit)\n+{\n+\tread_config(the_repository);\n+\treturn remotes_pushremote_for_branch(the_repository->remote_state,\n+\t\t\t\t\t     branch, explicit);\n+}\n+\n+static struct remote *remotes_remote_get(struct remote_state *remote_state,\n+\t\t\t\t\t const char *name);\n+\n const char *remote_ref_for_branch(struct branch *branch, int for_push)\n {\n+\tread_config(the_repository);\n \tif (branch) {\n \t\tif (!for_push) {\n \t\t\tif (branch->merge_nr) {\n \t\t\t\treturn branch->merge_name[0];\n \t\t\t}\n \t\t} else {\n-\t\t\tconst char *dst, *remote_name =\n-\t\t\t\tpushremote_for_branch(branch, NULL);\n-\t\t\tstruct remote *remote = remote_get(remote_name);\n+\t\t\tconst char *dst,\n+\t\t\t\t*remote_name = remotes_pushremote_for_branch(\n+\t\t\t\t\tthe_repository->remote_state, branch,\n+\t\t\t\t\tNULL);\n+\t\t\tstruct remote *remote = remotes_remote_get(\n+\t\t\t\tthe_repository->remote_state, remote_name);\n \n \t\t\tif (remote && remote->push.nr &&\n \t\t\t    (dst = apply_refspecs(&remote->push,\n@@ -532,42 +557,58 @@ const char *remote_ref_for_branch(struct branch *branch, int for_push)\n \treturn NULL;\n }\n \n-static struct remote *remote_get_1(const char *name,\n-\t\t\t\t   const char *(*get_default)(struct branch *, int *))\n+static struct remote *\n+remotes_remote_get_1(struct remote_state *remote_state, const char *name,\n+\t\t     const char *(*get_default)(struct remote_state *,\n+\t\t\t\t\t\tstruct branch *, int *))\n {\n \tstruct remote *ret;\n \tint name_given = 0;\n \n-\tread_config(the_repository);\n-\n \tif (name)\n \t\tname_given = 1;\n \telse\n-\t\tname = get_default(the_repository->remote_state->current_branch,\n+\t\tname = get_default(remote_state, remote_state->current_branch,\n \t\t\t\t   &name_given);\n \n-\tret = make_remote(the_repository->remote_state, name, 0);\n+\tret = make_remote(remote_state, name, 0);\n \tif (valid_remote_nick(name) && have_git_dir()) {\n \t\tif (!valid_remote(ret))\n-\t\t\tread_remotes_file(the_repository->remote_state, ret);\n+\t\t\tread_remotes_file(remote_state, ret);\n \t\tif (!valid_remote(ret))\n-\t\t\tread_branches_file(the_repository->remote_state, ret);\n+\t\t\tread_branches_file(remote_state, ret);\n \t}\n \tif (name_given && !valid_remote(ret))\n-\t\tadd_url_alias(the_repository->remote_state, ret, name);\n+\t\tadd_url_alias(remote_state, ret, name);\n \tif (!valid_remote(ret))\n \t\treturn NULL;\n \treturn ret;\n }\n \n+static inline struct remote *\n+remotes_remote_get(struct remote_state *remote_state, const char *name)\n+{\n+\treturn remotes_remote_get_1(remote_state, name,\n+\t\t\t\t    remotes_remote_for_branch);\n+}\n+\n struct remote *remote_get(const char *name)\n {\n-\treturn remote_get_1(name, remote_for_branch);\n+\tread_config(the_repository);\n+\treturn remotes_remote_get(the_repository->remote_state, name);\n+}\n+\n+static inline struct remote *\n+remotes_pushremote_get(struct remote_state *remote_state, const char *name)\n+{\n+\treturn remotes_remote_get_1(remote_state, name,\n+\t\t\t\t    remotes_pushremote_for_branch);\n }\n \n struct remote *pushremote_get(const char *name)\n {\n-\treturn remote_get_1(name, pushremote_for_branch);\n+\tread_config(the_repository);\n+\treturn remotes_pushremote_get(the_repository->remote_state, name);\n }\n \n int remote_is_configured(struct remote *remote, int in_repo)\n@@ -1654,7 +1695,7 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,\n \t}\n }\n \n-static void set_merge(struct branch *ret)\n+static void set_merge(struct remote_state *remote_state, struct branch *ret)\n {\n \tstruct remote *remote;\n \tchar *ref;\n@@ -1674,7 +1715,7 @@ static void set_merge(struct branch *ret)\n \t\treturn;\n \t}\n \n-\tremote = remote_get(ret->remote_name);\n+\tremote = remotes_remote_get(remote_state, ret->remote_name);\n \n \tCALLOC_ARRAY(ret->merge, ret->merge_nr);\n \tfor (i = 0; i < ret->merge_nr; i++) {\n@@ -1701,7 +1742,7 @@ struct branch *branch_get(const char *name)\n \telse\n \t\tret = make_branch(the_repository->remote_state, name,\n \t\t\t\t  strlen(name));\n-\tset_merge(ret);\n+\tset_merge(the_repository->remote_state, ret);\n \treturn ret;\n }\n \n@@ -1772,11 +1813,14 @@ static const char *tracking_for_push_dest(struct remote *remote,\n \treturn ret;\n }\n \n-static const char *branch_get_push_1(struct branch *branch, struct strbuf *err)\n+static const char *branch_get_push_1(struct remote_state *remote_state,\n+\t\t\t\t     struct branch *branch, struct strbuf *err)\n {\n \tstruct remote *remote;\n \n-\tremote = remote_get(pushremote_for_branch(branch, NULL));\n+\tremote = remotes_remote_get(\n+\t\tremote_state,\n+\t\tremotes_pushremote_for_branch(remote_state, branch, NULL));\n \tif (!remote)\n \t\treturn error_buf(err,\n \t\t\t\t _(\"branch '%s' has no remote for pushing\"),\n@@ -1834,11 +1878,13 @@ static const char *branch_get_push_1(struct branch *branch, struct strbuf *err)\n \n const char *branch_get_push(struct branch *branch, struct strbuf *err)\n {\n+\tread_config(the_repository);\n \tif (!branch)\n \t\treturn error_buf(err, _(\"HEAD does not point to a branch\"));\n \n \tif (!branch->push_tracking_ref)\n-\t\tbranch->push_tracking_ref = branch_get_push_1(branch, err);\n+\t\tbranch->push_tracking_ref = branch_get_push_1(\n+\t\t\tthe_repository->remote_state, branch, err);\n \treturn branch->push_tracking_ref;\n }\n \n-- \n2.33.GIT\n\n"},{"id":"441550","messageId":"20211118005325.64971-6-chooglen@google.com","threadId":"56931","inReplyTo":"20211118005325.64971-1-chooglen@google.com","subject":"[PATCH v5 5/5] remote: die if branch is not found in repository","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-11-18T00:53:25Z","receivedAt":"2021-11-18T00:53:56Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"In a subsequent commit, we would like external-facing functions to be\nable to accept \"struct repository\" and \"struct branch\" as a pair. This\nis useful for functions like pushremote_for_branch(), which need to take\nvalues from the remote_state and branch, even if branch == NULL.\nHowever, a caller may supply an unrelated repository and branch, which\nis not supported behavior.\n\nTo prevent misuse, add a die_on_missing_branch() helper function that\ndies if a given branch is not from a given repository. Speed up the\nexistence check by replacing the branches list with a branches_hash\nhashmap.\n\nLike read_config(), die_on_missing_branch() is only called from\nnon-static functions; static functions are less prone to misuse because\nthey have strong conventions for keeping remote_state and branch in\nsync.\n\nSigned-off-by: Glen Choo <chooglen@google.com>\n---\n remote.c | 84 +++++++++++++++++++++++++++++++++++++++++++++-----------\n remote.h |  5 ++--\n 2 files changed, 70 insertions(+), 19 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex 88697e7e6d..c97c626eaa 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -166,25 +166,74 @@ static void add_merge(struct branch *branch, const char *name)\n \tbranch->merge_name[branch->merge_nr++] = name;\n }\n \n+struct branches_hash_key {\n+\tconst char *str;\n+\tint len;\n+};\n+\n+static int branches_hash_cmp(const void *unused_cmp_data,\n+\t\t\t     const struct hashmap_entry *eptr,\n+\t\t\t     const struct hashmap_entry *entry_or_key,\n+\t\t\t     const void *keydata)\n+{\n+\tconst struct branch *a, *b;\n+\tconst struct branches_hash_key *key = keydata;\n+\n+\ta = container_of(eptr, const struct branch, ent);\n+\tb = container_of(entry_or_key, const struct branch, ent);\n+\n+\tif (key)\n+\t\treturn strncmp(a->name, key->str, key->len) ||\n+\t\t       a->name[key->len];\n+\telse\n+\t\treturn strcmp(a->name, b->name);\n+}\n+\n+static struct branch *find_branch(struct remote_state *remote_state,\n+\t\t\t\t  const char *name, size_t len)\n+{\n+\tstruct branches_hash_key lookup;\n+\tstruct hashmap_entry lookup_entry, *e;\n+\n+\tif (!len)\n+\t\tlen = strlen(name);\n+\n+\tlookup.str = name;\n+\tlookup.len = len;\n+\thashmap_entry_init(&lookup_entry, memhash(name, len));\n+\n+\te = hashmap_get(&remote_state->branches_hash, &lookup_entry, &lookup);\n+\tif (e)\n+\t\treturn container_of(e, struct branch, ent);\n+\n+\treturn NULL;\n+}\n+\n+static void die_on_missing_branch(struct repository *repo,\n+\t\t\t\t  struct branch *branch)\n+{\n+\t/* branch == NULL is always valid because it represents detached HEAD. */\n+\tif (branch &&\n+\t    branch != find_branch(repo->remote_state, branch->name, 0))\n+\t\tdie(\"branch %s was not found in the repository\", branch->name);\n+}\n+\n static struct branch *make_branch(struct remote_state *remote_state,\n \t\t\t\t  const char *name, size_t len)\n {\n \tstruct branch *ret;\n-\tint i;\n \n-\tfor (i = 0; i < remote_state->branches_nr; i++) {\n-\t\tif (!strncmp(name, remote_state->branches[i]->name, len) &&\n-\t\t    !remote_state->branches[i]->name[len])\n-\t\t\treturn remote_state->branches[i];\n-\t}\n+\tret = find_branch(remote_state, name, len);\n+\tif (ret)\n+\t\treturn ret;\n \n-\tALLOC_GROW(remote_state->branches, remote_state->branches_nr + 1,\n-\t\t   remote_state->branches_alloc);\n \tCALLOC_ARRAY(ret, 1);\n-\tremote_state->branches[remote_state->branches_nr++] = ret;\n \tret->name = xstrndup(name, len);\n \tret->refname = xstrfmt(\"refs/heads/%s\", ret->name);\n \n+\thashmap_entry_init(&ret->ent, memhash(name, len));\n+\tif (hashmap_put_entry(&remote_state->branches_hash, ret, ent))\n+\t\tBUG(\"hashmap_put overwrote entry after hashmap_get returned NULL\");\n \treturn ret;\n }\n \n@@ -500,6 +549,8 @@ static const char *remotes_remote_for_branch(struct remote_state *remote_state,\n const char *remote_for_branch(struct branch *branch, int *explicit)\n {\n \tread_config(the_repository);\n+\tdie_on_missing_branch(the_repository, branch);\n+\n \treturn remotes_remote_for_branch(the_repository->remote_state, branch,\n \t\t\t\t\t explicit);\n }\n@@ -524,6 +575,8 @@ remotes_pushremote_for_branch(struct remote_state *remote_state,\n const char *pushremote_for_branch(struct branch *branch, int *explicit)\n {\n \tread_config(the_repository);\n+\tdie_on_missing_branch(the_repository, branch);\n+\n \treturn remotes_pushremote_for_branch(the_repository->remote_state,\n \t\t\t\t\t     branch, explicit);\n }\n@@ -534,6 +587,8 @@ static struct remote *remotes_remote_get(struct remote_state *remote_state,\n const char *remote_ref_for_branch(struct branch *branch, int for_push)\n {\n \tread_config(the_repository);\n+\tdie_on_missing_branch(the_repository, branch);\n+\n \tif (branch) {\n \t\tif (!for_push) {\n \t\t\tif (branch->merge_nr) {\n@@ -1879,6 +1934,8 @@ static const char *branch_get_push_1(struct remote_state *remote_state,\n const char *branch_get_push(struct branch *branch, struct strbuf *err)\n {\n \tread_config(the_repository);\n+\tdie_on_missing_branch(the_repository, branch);\n+\n \tif (!branch)\n \t\treturn error_buf(err, _(\"HEAD does not point to a branch\"));\n \n@@ -2652,6 +2709,7 @@ struct remote_state *remote_state_new(void)\n \tmemset(r, 0, sizeof(*r));\n \n \thashmap_init(&r->remotes_hash, remotes_hash_cmp, NULL, 0);\n+\thashmap_init(&r->branches_hash, branches_hash_cmp, NULL, 0);\n \treturn r;\n }\n \n@@ -2667,11 +2725,5 @@ void remote_state_clear(struct remote_state *remote_state)\n \tremote_state->remotes_nr = 0;\n \n \thashmap_clear_and_free(&remote_state->remotes_hash, struct remote, ent);\n-\n-\tfor (i = 0; i < remote_state->branches_nr; i++) {\n-\t\tFREE_AND_NULL(remote_state->branches[i]);\n-\t}\n-\tFREE_AND_NULL(remote_state->branches);\n-\tremote_state->branches_alloc = 0;\n-\tremote_state->branches_nr = 0;\n+\thashmap_clear_and_free(&remote_state->branches_hash, struct remote, ent);\n }\ndiff --git a/remote.h b/remote.h\nindex 85a730b8ef..4a1209ae2c 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -43,9 +43,7 @@ struct remote_state {\n \tint remotes_nr;\n \tstruct hashmap remotes_hash;\n \n-\tstruct branch **branches;\n-\tint branches_alloc;\n-\tint branches_nr;\n+\tstruct hashmap branches_hash;\n \n \tstruct branch *current_branch;\n \tconst char *pushremote_name;\n@@ -292,6 +290,7 @@ int remote_find_tracking(struct remote *remote, struct refspec_item *refspec);\n  * branch_get(name) for \"refs/heads/{name}\", or with branch_get(NULL) for HEAD.\n  */\n struct branch {\n+\tstruct hashmap_entry ent;\n \n \t/* The short name of the branch. */\n \tconst char *name;\n-- \n2.33.GIT\n\n"},{"id":"441648","messageId":"20211118175152.2716229-1-jonathantanmy@google.com","threadId":"56931","inReplyTo":"20211118005325.64971-1-chooglen@google.com","subject":"Re: [PATCH v5 0/5] remote: replace static variables with struct remote_state","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-11-18T17:51:52Z","receivedAt":"2021-11-18T17:52:00Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Glen Choo <chooglen@google.com> writes:\n> This series aims to make the remotes subsystem work with non-the_repository,\n> which will allow submodule remotes to be accessed in-process, rather than\n> through child processes. This is accomplished by creating a struct remote_state\n> and adding it to struct repository.\n> \n> Thanks for the kind review, Jonathan. I've incorporated most of the feedback\n> from v4, except squashing patches 2-4. My reasoning is that those changes are\n> mechanical and keeping them separate makes it more obvious whether or not each\n> step has been done correctly. However, I agree that they are just intermediate\n> steps that touch the same lines, so I am happy to squash them if other reviewers\n> prefer that.\n\nThanks. This patch series looks good to me.\nReviewed-by: Jonathan Tan <jonathantanmy@google.com>\n"}]}