git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH 0/2] remote: replace static variables with struct remote_state
@ 2021-10-07 19:07 Glen Choo via GitGitGadget
  2021-10-07 19:07 ` [PATCH 1/2] remote: move static variables into struct Glen Choo via GitGitGadget
                   ` (2 more replies)
  0 siblings, 3 replies; 21+ messages in thread
From: Glen Choo via GitGitGadget @ 2021-10-07 19:07 UTC (permalink / raw)
  To: git; +Cc: Glen Choo

This series aims to make the remotes subsystem work with non-the_repository,
which will allow submodule remotes to be accessed in-process, rather than
through child processes. This is accomplished by creating a struct
remote_state and adding it to struct repository.

One motivation for this is that it allows future submodule commands to run
in-process. An example is an RFC series of mine [1], where I tried to
implement "git branch --recurse-submodules" in-process but couldn't figure
out how to read the remotes of a submodule.

For the most part, this was a mechanical process of taking static variables
and putting them in a struct. As such, I think this series might benefit
from thoughtful review, especially from people with more insight into this
area. Some areas I am extra concerned about are:

 * Naming, especially the renaming of variables in patch 1 and the field
   names in patch 2. I don't really like the name "remote_state", but I
   can't think of a better one.
 * Memory leaks - I am not confident that remote_state_clear() frees all of
   the memory that it should. I tried to eliminate as many leaks as I could
   with "make SANITIZE=address,leak", but I'm not confident that I've caught
   them all.

[1] https://lore.kernel.org/git/20210921232529.81811-1-chooglen@google.com/

Glen Choo (2):
  remote: move static variables into struct
  remote: add remote_state to struct repository

 remote.c     | 245 ++++++++++++++++++++++++++++++---------------------
 remote.h     |  69 +++++++++++++--
 repository.c |   8 ++
 repository.h |   4 +
 4 files changed, 222 insertions(+), 104 deletions(-)


base-commit: 0785eb769886ae81e346df10e88bc49ffc0ac64e
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1103%2Fchooglen%2Fremote%2Fstruct-no-global-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1103/chooglen/remote/struct-no-global-v1
Pull-Request: https://github.com/git/git/pull/1103
-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 1/2] remote: move static variables into struct
  2021-10-07 19:07 [PATCH 0/2] remote: replace static variables with struct remote_state Glen Choo via GitGitGadget
@ 2021-10-07 19:07 ` Glen Choo via GitGitGadget
  2021-10-07 23:36   ` Junio C Hamano
  2021-10-07 19:07 ` [PATCH 2/2] remote: add remote_state to struct repository Glen Choo via GitGitGadget
  2021-10-13 19:31 ` [PATCH v2 0/3] remote: replace static variables with struct remote_state Glen Choo
  2 siblings, 1 reply; 21+ messages in thread
From: Glen Choo via GitGitGadget @ 2021-10-07 19:07 UTC (permalink / raw)
  To: git; +Cc: Glen Choo, Glen Choo

From: Glen Choo <chooglen@google.com>

remote.c only works with the_repository because it stores its state as
static variables. To support non-the_repository, we can use a
per-repository struct for the remotes subsystem.

Prepare for this change by moving the static variables of remote.c into
a static struct remote_state.

This introduces no behavioral or API changes.

Signed-off-by: Glen Choo <chooglen@google.com>
---
 remote.c | 109 ++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 63 insertions(+), 46 deletions(-)

diff --git a/remote.c b/remote.c
index 31e141b01fa..55997210060 100644
--- a/remote.c
+++ b/remote.c
@@ -34,20 +34,24 @@ struct rewrites {
 	int rewrite_nr;
 };
 
-static struct remote **remotes;
-static int remotes_alloc;
-static int remotes_nr;
-static struct hashmap remotes_hash;
+static struct remote_state {
+	struct remote **remotes;
+	int remotes_alloc;
+	int remotes_nr;
+	struct hashmap remotes_hash;
 
-static struct branch **branches;
-static int branches_alloc;
-static int branches_nr;
+	struct branch **branches;
+	int branches_alloc;
+	int branches_nr;
 
-static struct branch *current_branch;
-static const char *pushremote_name;
+	struct branch *current_branch;
+	const char *pushremote_name;
 
-static struct rewrites rewrites;
-static struct rewrites rewrites_push;
+	struct rewrites rewrites;
+	struct rewrites rewrites_push;
+} remotes;
+
+struct remote_state *remote_state = &remotes;
 
 static int valid_remote(const struct remote *remote)
 {
@@ -94,14 +98,14 @@ static void add_pushurl(struct remote *remote, const char *pushurl)
 
 static void add_pushurl_alias(struct remote *remote, const char *url)
 {
-	const char *pushurl = alias_url(url, &rewrites_push);
+	const char *pushurl = alias_url(url, &remote_state->rewrites_push);
 	if (pushurl != url)
 		add_pushurl(remote, pushurl);
 }
 
 static void add_url_alias(struct remote *remote, const char *url)
 {
-	add_url(remote, alias_url(url, &rewrites));
+	add_url(remote, alias_url(url, &remote_state->rewrites));
 	add_pushurl_alias(remote, url);
 }
 
@@ -129,8 +133,9 @@ static int remotes_hash_cmp(const void *unused_cmp_data,
 
 static inline void init_remotes_hash(void)
 {
-	if (!remotes_hash.cmpfn)
-		hashmap_init(&remotes_hash, remotes_hash_cmp, NULL, 0);
+	if (!remote_state->remotes_hash.cmpfn)
+		hashmap_init(&remote_state->remotes_hash, remotes_hash_cmp,
+			     NULL, 0);
 }
 
 static struct remote *make_remote(const char *name, int len)
@@ -147,7 +152,7 @@ static struct remote *make_remote(const char *name, int len)
 	lookup.len = len;
 	hashmap_entry_init(&lookup_entry, memhash(name, len));
 
-	e = hashmap_get(&remotes_hash, &lookup_entry, &lookup);
+	e = hashmap_get(&remote_state->remotes_hash, &lookup_entry, &lookup);
 	if (e)
 		return container_of(e, struct remote, ent);
 
@@ -158,11 +163,12 @@ static struct remote *make_remote(const char *name, int len)
 	refspec_init(&ret->push, REFSPEC_PUSH);
 	refspec_init(&ret->fetch, REFSPEC_FETCH);
 
-	ALLOC_GROW(remotes, remotes_nr + 1, remotes_alloc);
-	remotes[remotes_nr++] = ret;
+	ALLOC_GROW(remote_state->remotes, remote_state->remotes_nr + 1,
+		   remote_state->remotes_alloc);
+	remote_state->remotes[remote_state->remotes_nr++] = ret;
 
 	hashmap_entry_init(&ret->ent, lookup_entry.hash);
-	if (hashmap_put_entry(&remotes_hash, ret, ent))
+	if (hashmap_put_entry(&remote_state->remotes_hash, ret, ent))
 		BUG("hashmap_put overwrote entry after hashmap_get returned NULL");
 	return ret;
 }
@@ -179,15 +185,16 @@ static struct branch *make_branch(const char *name, size_t len)
 	struct branch *ret;
 	int i;
 
-	for (i = 0; i < branches_nr; i++) {
-		if (!strncmp(name, branches[i]->name, len) &&
-		    !branches[i]->name[len])
-			return branches[i];
+	for (i = 0; i < remote_state->branches_nr; i++) {
+		if (!strncmp(name, remote_state->branches[i]->name, len) &&
+		    !remote_state->branches[i]->name[len])
+			return remote_state->branches[i];
 	}
 
-	ALLOC_GROW(branches, branches_nr + 1, branches_alloc);
+	ALLOC_GROW(remote_state->branches, remote_state->branches_nr + 1,
+		   remote_state->branches_alloc);
 	CALLOC_ARRAY(ret, 1);
-	branches[branches_nr++] = ret;
+	remote_state->branches[remote_state->branches_nr++] = ret;
 	ret->name = xstrndup(name, len);
 	ret->refname = xstrfmt("refs/heads/%s", ret->name);
 
@@ -327,12 +334,14 @@ static int handle_config(const char *key, const char *value, void *cb)
 		if (!strcmp(subkey, "insteadof")) {
 			if (!value)
 				return config_error_nonbool(key);
-			rewrite = make_rewrite(&rewrites, name, namelen);
+			rewrite = make_rewrite(&remote_state->rewrites, name,
+					       namelen);
 			add_instead_of(rewrite, xstrdup(value));
 		} else if (!strcmp(subkey, "pushinsteadof")) {
 			if (!value)
 				return config_error_nonbool(key);
-			rewrite = make_rewrite(&rewrites_push, name, namelen);
+			rewrite = make_rewrite(&remote_state->rewrites_push,
+					       name, namelen);
 			add_instead_of(rewrite, xstrdup(value));
 		}
 	}
@@ -342,7 +351,8 @@ static int handle_config(const char *key, const char *value, void *cb)
 
 	/* Handle remote.* variables */
 	if (!name && !strcmp(subkey, "pushdefault"))
-		return git_config_string(&pushremote_name, key, value);
+		return git_config_string(&remote_state->pushremote_name, key,
+					 value);
 
 	if (!name)
 		return 0;
@@ -425,18 +435,24 @@ static int handle_config(const char *key, const char *value, void *cb)
 static void alias_all_urls(void)
 {
 	int i, j;
-	for (i = 0; i < remotes_nr; i++) {
+	for (i = 0; i < remote_state->remotes_nr; i++) {
 		int add_pushurl_aliases;
-		if (!remotes[i])
+		if (!remote_state->remotes[i])
 			continue;
-		for (j = 0; j < remotes[i]->pushurl_nr; j++) {
-			remotes[i]->pushurl[j] = alias_url(remotes[i]->pushurl[j], &rewrites);
+		for (j = 0; j < remote_state->remotes[i]->pushurl_nr; j++) {
+			remote_state->remotes[i]->pushurl[j] =
+				alias_url(remote_state->remotes[i]->pushurl[j],
+					  &remote_state->rewrites);
 		}
-		add_pushurl_aliases = remotes[i]->pushurl_nr == 0;
-		for (j = 0; j < remotes[i]->url_nr; j++) {
+		add_pushurl_aliases = remote_state->remotes[i]->pushurl_nr == 0;
+		for (j = 0; j < remote_state->remotes[i]->url_nr; j++) {
 			if (add_pushurl_aliases)
-				add_pushurl_alias(remotes[i], remotes[i]->url[j]);
-			remotes[i]->url[j] = alias_url(remotes[i]->url[j], &rewrites);
+				add_pushurl_alias(
+					remote_state->remotes[i],
+					remote_state->remotes[i]->url[j]);
+			remote_state->remotes[i]->url[j] =
+				alias_url(remote_state->remotes[i]->url[j],
+					  &remote_state->rewrites);
 		}
 	}
 }
@@ -450,12 +466,13 @@ static void read_config(void)
 		return;
 	loaded = 1;
 
-	current_branch = NULL;
+	remote_state->current_branch = NULL;
 	if (startup_info->have_repository) {
 		const char *head_ref = resolve_ref_unsafe("HEAD", 0, NULL, &flag);
 		if (head_ref && (flag & REF_ISSYMREF) &&
 		    skip_prefix(head_ref, "refs/heads/", &head_ref)) {
-			current_branch = make_branch(head_ref, strlen(head_ref));
+			remote_state->current_branch =
+				make_branch(head_ref, strlen(head_ref));
 		}
 	}
 	git_config(handle_config, NULL);
@@ -493,10 +510,10 @@ const char *pushremote_for_branch(struct branch *branch, int *explicit)
 			*explicit = 1;
 		return branch->pushremote_name;
 	}
-	if (pushremote_name) {
+	if (remote_state->pushremote_name) {
 		if (explicit)
 			*explicit = 1;
-		return pushremote_name;
+		return remote_state->pushremote_name;
 	}
 	return remote_for_branch(branch, explicit);
 }
@@ -534,7 +551,7 @@ static struct remote *remote_get_1(const char *name,
 	if (name)
 		name_given = 1;
 	else
-		name = get_default(current_branch, &name_given);
+		name = get_default(remote_state->current_branch, &name_given);
 
 	ret = make_remote(name, 0);
 	if (valid_remote_nick(name) && have_git_dir()) {
@@ -573,11 +590,11 @@ int for_each_remote(each_remote_fn fn, void *priv)
 {
 	int i, result = 0;
 	read_config();
-	for (i = 0; i < remotes_nr && !result; i++) {
-		struct remote *r = remotes[i];
-		if (!r)
+	for (i = 0; i < remote_state->remotes_nr && !result; i++) {
+		struct remote *remote = remote_state->remotes[i];
+		if (!remote)
 			continue;
-		result = fn(r, priv);
+		result = fn(remote, priv);
 	}
 	return result;
 }
@@ -1685,7 +1702,7 @@ struct branch *branch_get(const char *name)
 
 	read_config();
 	if (!name || !*name || !strcmp(name, "HEAD"))
-		ret = current_branch;
+		ret = remote_state->current_branch;
 	else
 		ret = make_branch(name, strlen(name));
 	set_merge(ret);
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 2/2] remote: add remote_state to struct repository
  2021-10-07 19:07 [PATCH 0/2] remote: replace static variables with struct remote_state Glen Choo via GitGitGadget
  2021-10-07 19:07 ` [PATCH 1/2] remote: move static variables into struct Glen Choo via GitGitGadget
@ 2021-10-07 19:07 ` Glen Choo via GitGitGadget
  2021-10-07 23:39   ` Junio C Hamano
  2021-10-13 19:31 ` [PATCH v2 0/3] remote: replace static variables with struct remote_state Glen Choo
  2 siblings, 1 reply; 21+ messages in thread
From: Glen Choo via GitGitGadget @ 2021-10-07 19:07 UTC (permalink / raw)
  To: git; +Cc: Glen Choo, Glen Choo

From: Glen Choo <chooglen@google.com>

Add struct remote_state to struct repository and remove the global
struct remote_state from remote.c. In order for remote.c to receive
struct repository, introduce a family of repo_* functions that
accept a struct repository argument.

The existing functions in remote.h that use the_repository are
reimplemented as thin wrappers around their repo_* counterparts.

Signed-off-by: Glen Choo <chooglen@google.com>
---
 remote.c     | 186 ++++++++++++++++++++++++++++++---------------------
 remote.h     |  69 +++++++++++++++++--
 repository.c |   8 +++
 repository.h |   4 ++
 4 files changed, 184 insertions(+), 83 deletions(-)

diff --git a/remote.c b/remote.c
index 55997210060..3a0a130ad2a 100644
--- a/remote.c
+++ b/remote.c
@@ -21,37 +21,6 @@ struct counted_string {
 	size_t len;
 	const char *s;
 };
-struct rewrite {
-	const char *base;
-	size_t baselen;
-	struct counted_string *instead_of;
-	int instead_of_nr;
-	int instead_of_alloc;
-};
-struct rewrites {
-	struct rewrite **rewrite;
-	int rewrite_alloc;
-	int rewrite_nr;
-};
-
-static struct remote_state {
-	struct remote **remotes;
-	int remotes_alloc;
-	int remotes_nr;
-	struct hashmap remotes_hash;
-
-	struct branch **branches;
-	int branches_alloc;
-	int branches_nr;
-
-	struct branch *current_branch;
-	const char *pushremote_name;
-
-	struct rewrites rewrites;
-	struct rewrites rewrites_push;
-} remotes;
-
-struct remote_state *remote_state = &remotes;
 
 static int valid_remote(const struct remote *remote)
 {
@@ -96,17 +65,19 @@ static void add_pushurl(struct remote *remote, const char *pushurl)
 	remote->pushurl[remote->pushurl_nr++] = pushurl;
 }
 
-static void add_pushurl_alias(struct remote *remote, const char *url)
+static void add_pushurl_alias(struct remote_state *remote_state,
+			      struct remote *remote, const char *url)
 {
 	const char *pushurl = alias_url(url, &remote_state->rewrites_push);
 	if (pushurl != url)
 		add_pushurl(remote, pushurl);
 }
 
-static void add_url_alias(struct remote *remote, const char *url)
+static void add_url_alias(struct remote_state *remote_state,
+			  struct remote *remote, const char *url)
 {
 	add_url(remote, alias_url(url, &remote_state->rewrites));
-	add_pushurl_alias(remote, url);
+	add_pushurl_alias(remote_state, remote, url);
 }
 
 struct remotes_hash_key {
@@ -131,14 +102,15 @@ static int remotes_hash_cmp(const void *unused_cmp_data,
 		return strcmp(a->name, b->name);
 }
 
-static inline void init_remotes_hash(void)
+static inline void init_remotes_hash(struct remote_state *remote_state)
 {
 	if (!remote_state->remotes_hash.cmpfn)
 		hashmap_init(&remote_state->remotes_hash, remotes_hash_cmp,
 			     NULL, 0);
 }
 
-static struct remote *make_remote(const char *name, int len)
+static struct remote *make_remote(struct remote_state *remote_state,
+				  const char *name, int len)
 {
 	struct remote *ret;
 	struct remotes_hash_key lookup;
@@ -147,7 +119,7 @@ static struct remote *make_remote(const char *name, int len)
 	if (!len)
 		len = strlen(name);
 
-	init_remotes_hash();
+	init_remotes_hash(remote_state);
 	lookup.str = name;
 	lookup.len = len;
 	hashmap_entry_init(&lookup_entry, memhash(name, len));
@@ -173,6 +145,28 @@ static struct remote *make_remote(const char *name, int len)
 	return ret;
 }
 
+static void remote_clear(struct remote *remote)
+{
+	int i;
+
+	free((char *)remote->name);
+	free((char *)remote->foreign_vcs);
+
+	for (i = 0; i < remote->url_nr; i++) {
+		free((char *)remote->url[i]);
+	}
+	FREE_AND_NULL(remote->pushurl);
+
+	for (i = 0; i < remote->pushurl_nr; i++) {
+		free((char *)remote->pushurl[i]);
+	}
+	FREE_AND_NULL(remote->pushurl);
+	free((char *)remote->receivepack);
+	free((char *)remote->uploadpack);
+	FREE_AND_NULL(remote->http_proxy);
+	FREE_AND_NULL(remote->http_proxy_authmethod);
+}
+
 static void add_merge(struct branch *branch, const char *name)
 {
 	ALLOC_GROW(branch->merge_name, branch->merge_nr + 1,
@@ -180,7 +174,8 @@ static void add_merge(struct branch *branch, const char *name)
 	branch->merge_name[branch->merge_nr++] = name;
 }
 
-static struct branch *make_branch(const char *name, size_t len)
+static struct branch *make_branch(struct remote_state *remote_state,
+				  const char *name, size_t len)
 {
 	struct branch *ret;
 	int i;
@@ -236,7 +231,8 @@ static const char *skip_spaces(const char *s)
 	return s;
 }
 
-static void read_remotes_file(struct remote *remote)
+static void read_remotes_file(struct remote_state *remote_state,
+			      struct remote *remote)
 {
 	struct strbuf buf = STRBUF_INIT;
 	FILE *f = fopen_or_warn(git_path("remotes/%s", remote->name), "r");
@@ -251,7 +247,8 @@ static void read_remotes_file(struct remote *remote)
 		strbuf_rtrim(&buf);
 
 		if (skip_prefix(buf.buf, "URL:", &v))
-			add_url_alias(remote, xstrdup(skip_spaces(v)));
+			add_url_alias(remote_state, remote,
+				      xstrdup(skip_spaces(v)));
 		else if (skip_prefix(buf.buf, "Push:", &v))
 			refspec_append(&remote->push, skip_spaces(v));
 		else if (skip_prefix(buf.buf, "Pull:", &v))
@@ -261,7 +258,8 @@ static void read_remotes_file(struct remote *remote)
 	fclose(f);
 }
 
-static void read_branches_file(struct remote *remote)
+static void read_branches_file(struct remote_state *remote_state,
+			       struct remote *remote)
 {
 	char *frag;
 	struct strbuf buf = STRBUF_INIT;
@@ -293,7 +291,7 @@ static void read_branches_file(struct remote *remote)
 	else
 		frag = (char *)git_default_branch_name(0);
 
-	add_url_alias(remote, strbuf_detach(&buf, NULL));
+	add_url_alias(remote_state, remote, strbuf_detach(&buf, NULL));
 	refspec_appendf(&remote->fetch, "refs/heads/%s:refs/heads/%s",
 			frag, remote->name);
 
@@ -312,10 +310,11 @@ static int handle_config(const char *key, const char *value, void *cb)
 	const char *subkey;
 	struct remote *remote;
 	struct branch *branch;
+	struct remote_state *remote_state = cb;
 	if (parse_config_key(key, "branch", &name, &namelen, &subkey) >= 0) {
 		if (!name)
 			return 0;
-		branch = make_branch(name, namelen);
+		branch = make_branch(remote_state, name, namelen);
 		if (!strcmp(subkey, "remote")) {
 			return git_config_string(&branch->remote_name, key, value);
 		} else if (!strcmp(subkey, "pushremote")) {
@@ -362,7 +361,7 @@ static int handle_config(const char *key, const char *value, void *cb)
 			name);
 		return 0;
 	}
-	remote = make_remote(name, namelen);
+	remote = make_remote(remote_state, name, namelen);
 	remote->origin = REMOTE_CONFIG;
 	if (current_config_scope() == CONFIG_SCOPE_LOCAL ||
 	    current_config_scope() == CONFIG_SCOPE_WORKTREE)
@@ -432,7 +431,7 @@ static int handle_config(const char *key, const char *value, void *cb)
 	return 0;
 }
 
-static void alias_all_urls(void)
+static void alias_all_urls(struct remote_state *remote_state)
 {
 	int i, j;
 	for (i = 0; i < remote_state->remotes_nr; i++) {
@@ -448,7 +447,7 @@ static void alias_all_urls(void)
 		for (j = 0; j < remote_state->remotes[i]->url_nr; j++) {
 			if (add_pushurl_aliases)
 				add_pushurl_alias(
-					remote_state->remotes[i],
+					remote_state, remote_state->remotes[i],
 					remote_state->remotes[i]->url[j]);
 			remote_state->remotes[i]->url[j] =
 				alias_url(remote_state->remotes[i]->url[j],
@@ -457,26 +456,25 @@ static void alias_all_urls(void)
 	}
 }
 
-static void read_config(void)
+static void read_config(struct repository *repo)
 {
-	static int loaded;
 	int flag;
 
-	if (loaded)
+	if (repo->remote_state->config_loaded)
 		return;
-	loaded = 1;
+	repo->remote_state->config_loaded = 1;
 
-	remote_state->current_branch = NULL;
+	repo->remote_state->current_branch = NULL;
 	if (startup_info->have_repository) {
 		const char *head_ref = resolve_ref_unsafe("HEAD", 0, NULL, &flag);
 		if (head_ref && (flag & REF_ISSYMREF) &&
 		    skip_prefix(head_ref, "refs/heads/", &head_ref)) {
-			remote_state->current_branch =
-				make_branch(head_ref, strlen(head_ref));
+			repo->remote_state->current_branch = make_branch(
+				repo->remote_state, head_ref, strlen(head_ref));
 		}
 	}
-	git_config(handle_config, NULL);
-	alias_all_urls();
+	repo_config(repo, handle_config, repo->remote_state);
+	alias_all_urls(repo->remote_state);
 }
 
 static int valid_remote_nick(const char *name)
@@ -503,17 +501,18 @@ const char *remote_for_branch(struct branch *branch, int *explicit)
 	return "origin";
 }
 
-const char *pushremote_for_branch(struct branch *branch, int *explicit)
+const char *repo_pushremote_for_branch(struct repository *repo,
+				       struct branch *branch, int *explicit)
 {
 	if (branch && branch->pushremote_name) {
 		if (explicit)
 			*explicit = 1;
 		return branch->pushremote_name;
 	}
-	if (remote_state->pushremote_name) {
+	if (repo->remote_state->pushremote_name) {
 		if (explicit)
 			*explicit = 1;
-		return remote_state->pushremote_name;
+		return repo->remote_state->pushremote_name;
 	}
 	return remote_for_branch(branch, explicit);
 }
@@ -540,41 +539,43 @@ const char *remote_ref_for_branch(struct branch *branch, int for_push)
 	return NULL;
 }
 
-static struct remote *remote_get_1(const char *name,
-				   const char *(*get_default)(struct branch *, int *))
+static struct remote *remote_get_1(struct repository *repo, const char *name,
+				   const char *(*get_default)(struct branch *,
+							      int *))
 {
 	struct remote *ret;
 	int name_given = 0;
 
-	read_config();
+	read_config(repo);
 
 	if (name)
 		name_given = 1;
 	else
-		name = get_default(remote_state->current_branch, &name_given);
+		name = get_default(repo->remote_state->current_branch,
+				   &name_given);
 
-	ret = make_remote(name, 0);
+	ret = make_remote(repo->remote_state, name, 0);
 	if (valid_remote_nick(name) && have_git_dir()) {
 		if (!valid_remote(ret))
-			read_remotes_file(ret);
+			read_remotes_file(repo->remote_state, ret);
 		if (!valid_remote(ret))
-			read_branches_file(ret);
+			read_branches_file(repo->remote_state, ret);
 	}
 	if (name_given && !valid_remote(ret))
-		add_url_alias(ret, name);
+		add_url_alias(repo->remote_state, ret, name);
 	if (!valid_remote(ret))
 		return NULL;
 	return ret;
 }
 
-struct remote *remote_get(const char *name)
+struct remote *repo_remote_get(struct repository *repo, const char *name)
 {
-	return remote_get_1(name, remote_for_branch);
+	return remote_get_1(repo, name, remote_for_branch);
 }
 
-struct remote *pushremote_get(const char *name)
+struct remote *repo_pushremote_get(struct repository *repo, const char *name)
 {
-	return remote_get_1(name, pushremote_for_branch);
+	return remote_get_1(repo, name, pushremote_for_branch);
 }
 
 int remote_is_configured(struct remote *remote, int in_repo)
@@ -586,12 +587,12 @@ int remote_is_configured(struct remote *remote, int in_repo)
 	return !!remote->origin;
 }
 
-int for_each_remote(each_remote_fn fn, void *priv)
+int repo_for_each_remote(struct repository *repo, each_remote_fn fn, void *priv)
 {
 	int i, result = 0;
-	read_config();
-	for (i = 0; i < remote_state->remotes_nr && !result; i++) {
-		struct remote *remote = remote_state->remotes[i];
+	read_config(repo);
+	for (i = 0; i < repo->remote_state->remotes_nr && !result; i++) {
+		struct remote *remote = repo->remote_state->remotes[i];
 		if (!remote)
 			continue;
 		result = fn(remote, priv);
@@ -1696,15 +1697,15 @@ static void set_merge(struct branch *ret)
 	}
 }
 
-struct branch *branch_get(const char *name)
+struct branch *repo_branch_get(struct repository *repo, const char *name)
 {
 	struct branch *ret;
 
-	read_config();
+	read_config(repo);
 	if (!name || !*name || !strcmp(name, "HEAD"))
-		ret = remote_state->current_branch;
+		ret = repo->remote_state->current_branch;
 	else
-		ret = make_branch(name, strlen(name));
+		ret = make_branch(repo->remote_state, name, strlen(name));
 	set_merge(ret);
 	return ret;
 }
@@ -2602,3 +2603,32 @@ void apply_push_cas(struct push_cas_option *cas,
 			check_if_includes_upstream(ref);
 	}
 }
+
+struct remote_state *remote_state_new(void)
+{
+	struct remote_state *r = xmalloc(sizeof(*r));
+
+	memset(r, 0, sizeof(*r));
+	return r;
+}
+
+void remote_state_clear(struct remote_state *remote_state)
+{
+	int i;
+
+	for (i = 0; i < remote_state->remotes_nr; i++) {
+		remote_clear(remote_state->remotes[i]);
+	}
+	FREE_AND_NULL(remote_state->remotes);
+	remote_state->remotes_alloc = 0;
+	remote_state->remotes_nr = 0;
+
+	hashmap_clear_and_free(&remote_state->remotes_hash, struct remote, ent);
+
+	for (i = 0; i < remote_state->branches_nr; i++) {
+		FREE_AND_NULL(remote_state->branches[i]);
+	}
+	FREE_AND_NULL(remote_state->branches);
+	remote_state->branches_alloc = 0;
+	remote_state->branches_nr = 0;
+}
diff --git a/remote.h b/remote.h
index 5a591982528..3cba8eb8cb6 100644
--- a/remote.h
+++ b/remote.h
@@ -23,6 +23,40 @@ enum {
 	REMOTE_BRANCHES
 };
 
+struct rewrite {
+	const char *base;
+	size_t baselen;
+	struct counted_string *instead_of;
+	int instead_of_nr;
+	int instead_of_alloc;
+};
+struct rewrites {
+	struct rewrite **rewrite;
+	int rewrite_alloc;
+	int rewrite_nr;
+};
+
+struct remote_state {
+	int config_loaded;
+
+	struct remote **remotes;
+	int remotes_alloc;
+	int remotes_nr;
+	struct hashmap remotes_hash;
+
+	struct branch **branches;
+	int branches_alloc;
+	int branches_nr;
+
+	struct branch *current_branch;
+	const char *pushremote_name;
+
+	struct rewrites rewrites;
+	struct rewrites rewrites_push;
+};
+void remote_state_clear(struct remote_state *remote_state);
+struct remote_state *remote_state_new(void);
+
 struct remote {
 	struct hashmap_entry ent;
 
@@ -83,15 +117,29 @@ struct remote {
  * remote_get(NULL) will return the default remote, given the current branch
  * and configuration.
  */
-struct remote *remote_get(const char *name);
+struct remote *repo_remote_get(struct repository *repo, const char *name);
+static inline struct remote *remote_get(const char *name)
+{
+	return repo_remote_get(the_repository, name);
+}
+
+struct remote *repo_pushremote_get(struct repository *repo, const char *name);
+static inline struct remote *pushremote_get(const char *name)
+{
+	return repo_pushremote_get(the_repository, name);
+}
 
-struct remote *pushremote_get(const char *name);
 int remote_is_configured(struct remote *remote, int in_repo);
 
 typedef int each_remote_fn(struct remote *remote, void *priv);
 
 /* iterate through struct remotes */
-int for_each_remote(each_remote_fn fn, void *priv);
+int repo_for_each_remote(struct repository *repo, each_remote_fn fn,
+			 void *priv);
+static inline int for_each_remote(each_remote_fn fn, void *priv)
+{
+	return repo_for_each_remote(the_repository, fn, priv);
+}
 
 int remote_has_url(struct remote *remote, const char *url);
 
@@ -286,9 +334,20 @@ struct branch {
 	const char *push_tracking_ref;
 };
 
-struct branch *branch_get(const char *name);
+struct branch *repo_branch_get(struct repository *repo, const char *name);
+static inline struct branch *branch_get(const char *name)
+{
+	return repo_branch_get(the_repository, name);
+}
 const char *remote_for_branch(struct branch *branch, int *explicit);
-const char *pushremote_for_branch(struct branch *branch, int *explicit);
+const char *repo_pushremote_for_branch(struct repository *repo,
+				       struct branch *branch, int *explicit);
+static inline const char *pushremote_for_branch(struct branch *branch,
+						int *explicit)
+{
+	return repo_pushremote_for_branch(the_repository, branch, explicit);
+}
+
 const char *remote_ref_for_branch(struct branch *branch, int for_push);
 
 /* returns true if the given branch has merge configuration given. */
diff --git a/repository.c b/repository.c
index 710a3b4bf87..47a27c8efb3 100644
--- a/repository.c
+++ b/repository.c
@@ -9,6 +9,7 @@
 #include "config.h"
 #include "object.h"
 #include "lockfile.h"
+#include "remote.h"
 #include "submodule-config.h"
 #include "sparse-index.h"
 #include "promisor-remote.h"
@@ -24,6 +25,7 @@ void initialize_the_repository(void)
 
 	the_repo.index = &the_index;
 	the_repo.objects = raw_object_store_new();
+	the_repo.remote_state = remote_state_new();
 	the_repo.parsed_objects = parsed_object_pool_new();
 
 	repo_set_hash_algo(&the_repo, GIT_HASH_SHA1);
@@ -164,6 +166,7 @@ int repo_init(struct repository *repo,
 
 	repo->objects = raw_object_store_new();
 	repo->parsed_objects = parsed_object_pool_new();
+	repo->remote_state = remote_state_new();
 
 	if (repo_init_gitdir(repo, gitdir))
 		goto error;
@@ -267,6 +270,11 @@ void repo_clear(struct repository *repo)
 		promisor_remote_clear(repo->promisor_remote_config);
 		FREE_AND_NULL(repo->promisor_remote_config);
 	}
+
+	if (repo->remote_state) {
+		remote_state_clear(repo->remote_state);
+		FREE_AND_NULL(repo->remote_state);
+	}
 }
 
 int repo_read_index(struct repository *repo)
diff --git a/repository.h b/repository.h
index 3740c93bc0f..4bcdbc22ed4 100644
--- a/repository.h
+++ b/repository.h
@@ -11,6 +11,7 @@ struct pathspec;
 struct raw_object_store;
 struct submodule_cache;
 struct promisor_remote_config;
+struct remote_state;
 
 enum untracked_cache_setting {
 	UNTRACKED_CACHE_UNSET = -1,
@@ -131,6 +132,9 @@ struct repository {
 	 */
 	struct index_state *index;
 
+	/* Repository's remotes and associated structures. */
+	struct remote_state *remote_state;
+
 	/* Repository's current hash algorithm, as serialized on disk. */
 	const struct git_hash_algo *hash_algo;
 
-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 1/2] remote: move static variables into struct
  2021-10-07 19:07 ` [PATCH 1/2] remote: move static variables into struct Glen Choo via GitGitGadget
@ 2021-10-07 23:36   ` Junio C Hamano
  0 siblings, 0 replies; 21+ messages in thread
From: Junio C Hamano @ 2021-10-07 23:36 UTC (permalink / raw)
  To: Glen Choo via GitGitGadget; +Cc: git, Glen Choo

"Glen Choo via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Glen Choo <chooglen@google.com>
>
> remote.c only works with the_repository because it stores its state as
> static variables. To support non-the_repository, we can use a
> per-repository struct for the remotes subsystem.

OK, so eventually we can hook the struct this patch introduces as a
member in the repository structure, so that we can have one such
struct per in-core repository instances?

Will some of the component of this new structure have to be aware of
the containing structure, even the repository instance the
containing structure belongs to?

For example, the apply_push_cas() function (there is nothing special
about it---just a random example near the end of file) takes "struct
remote *" which will be passed to underlying apply_cas() and the
control flow eventually need to reach read_ref() via
remote_tracking().  In a future world where we keep multiple in-core
repositories and interact with multiple repositories in-process,
this read_ref() must know in which repository it is asked to read
the value of the ref.  Do we expect to plumb "struct repository *"
throughout the callchain, or will some of the struct types gain a
backpointer to "struct remote_state", and "struct remote_state" will
in turn have a backpointer to the containing repository, so that
remote_tracking() can learn the ref_store it needs to use when
making a call to refs_read_ref_full() to replace the current call it
makes to the read_ref() function?

As to the name, I do not think "remote state" is not too bad, but
the word "remote" there certainly does not mean "state of a remote
repository", and in that sense, it might be confusing.

But It is still a seemingly random "collection of data that we need
to interact with remote repositories", so in that sense, the word
"remote" fits its purpose.  There are pieces of information needed
for a branch to interact with a remote repository, and the branches
variable serves as a caching mechanism for that, for example.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/2] remote: add remote_state to struct repository
  2021-10-07 19:07 ` [PATCH 2/2] remote: add remote_state to struct repository Glen Choo via GitGitGadget
@ 2021-10-07 23:39   ` Junio C Hamano
  2021-10-08 17:30     ` Glen Choo
  0 siblings, 1 reply; 21+ messages in thread
From: Junio C Hamano @ 2021-10-07 23:39 UTC (permalink / raw)
  To: Glen Choo via GitGitGadget; +Cc: git, Glen Choo

"Glen Choo via GitGitGadget" <gitgitgadget@gmail.com> writes:

> -static void add_pushurl_alias(struct remote *remote, const char *url)
> +static void add_pushurl_alias(struct remote_state *remote_state,
> +			      struct remote *remote, const char *url)
>  {

I am not sure if this is a good interface.  It allows a caller to
obtain "struct remote *" instance from somewhere, and feed it with
an instance of "struct remote_state *" that has nothing to do with
the "struct remote *", no?

> -static struct remote *make_remote(const char *name, int len)
> +static struct remote *make_remote(struct remote_state *remote_state,
> +				  const char *name, int len)
>  {
>  	struct remote *ret;
>  	struct remotes_hash_key lookup;
> @@ -147,7 +119,7 @@ static struct remote *make_remote(const char *name, int len)
>  	if (!len)
>  		len = strlen(name);
>  
> -	init_remotes_hash();
> +	init_remotes_hash(remote_state);
>  	lookup.str = name;
>  	lookup.len = len;
>  	hashmap_entry_init(&lookup_entry, memhash(name, len));
> @@ -173,6 +145,28 @@ static struct remote *make_remote(const char *name, int len)
>  	return ret;

Instead, shouldn't "struct remote *" _know_ which remote-state it
came from?

I didn't look, but I suspect that there may be similar problems with
other structures like "branch" in this change.

Thanks.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/2] remote: add remote_state to struct repository
  2021-10-07 23:39   ` Junio C Hamano
@ 2021-10-08 17:30     ` Glen Choo
  0 siblings, 0 replies; 21+ messages in thread
From: Glen Choo @ 2021-10-08 17:30 UTC (permalink / raw)
  To: Junio C Hamano, Glen Choo via GitGitGadget; +Cc: git

Junio C Hamano <gitster@pobox.com> writes:

>> -static void add_pushurl_alias(struct remote *remote, const char *url)
>> +static void add_pushurl_alias(struct remote_state *remote_state,
>> +			      struct remote *remote, const char *url)
>>  {
>
> I am not sure if this is a good interface.  It allows a caller to
> obtain "struct remote *" instance from somewhere, and feed it with
> an instance of "struct remote_state *" that has nothing to do with
> the "struct remote *", no?

Valid point, this interface should not be so easy to misuse.

> Instead, shouldn't "struct remote *" _know_ which remote-state it
> came from?

I am less certain about this. The pattern of
container->contained->container is convenient for callers, but requires
very deliberate maintenance and the reduced separation might promote
thoughtless use of the interfaces e.g. "Should I use struct remote_state
+ name or struct remote? Eh doesn't matter, they're equivalent."

Instead, we could converge on a pattern of:

* struct remote_state + name when the caller does something in the
  context of the remote's repository.
* struct remote when the caller doesn't need the remote's repository

I think we might do this in slightly different ways for branches vs
remotes.

> I didn't look, but I suspect that there may be similar problems with
> other structures like "branch" in this change.

Looking into it, it appears that only static functions pass struct
remote_state and struct remote at the same time. That gives us a lot of
leeway to clean things up inside of remote.c.

The same cannot be said for struct branch, that will be much harder to
clean up. In fact, rereading patch 2, I missed many implicit references
to the_repository in the branch functions, so the pattern of struct
remote_state + struct branch would actually more pervasive than it seems.

However, I suspect that we don't need to pass around struct branch very
often. We may be able to maintain referential integrity by passing the
branch name instead.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH v2 0/3] remote: replace static variables with struct remote_state
  2021-10-07 19:07 [PATCH 0/2] remote: replace static variables with struct remote_state Glen Choo via GitGitGadget
  2021-10-07 19:07 ` [PATCH 1/2] remote: move static variables into struct Glen Choo via GitGitGadget
  2021-10-07 19:07 ` [PATCH 2/2] remote: add remote_state to struct repository Glen Choo via GitGitGadget
@ 2021-10-13 19:31 ` Glen Choo
  2021-10-13 19:31   ` [PATCH v2 1/3] remote: move static variables into per-repository struct Glen Choo
                     ` (3 more replies)
  2 siblings, 4 replies; 21+ messages in thread
From: Glen Choo @ 2021-10-13 19:31 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Glen Choo

This series aims to make the remotes subsystem work with
non-the_repository, which will allow submodule remotes to be accessed
in-process, rather than through child processes. This is accomplished by
creating a struct remote_state and adding it to struct repository.

One motivation for this is that it allows future submodule commands to run
in-process. An example is an RFC series of mine [1], where I tried to implement
"git branch --recurse-submodules" in-process but couldn't figure out how to
read the remotes of a submodule.

v2 aims to catch all of the instances of struct remote_state that I had
missed in v1, particularly with the struct branch functions. Because
more repo_* functions were added, the preparatory patches have been
reorganized.

In this version, my biggest uncertainty is in the interfaces of the
repo_* functions. Some functions expect a "struct repository" and one
of its contained members (e.g. "struct branch"). This interface allows
for incorrect behavior if the caller supplies a "struct repository"
instance that is unrelated from the "struct branch". This concern was
raised by Junio in [2].

While this concern is valid, I decided to keep this interface for a few
reasons:

1. The correct way of calling the function is 'obvious'.
2. It is relatively easy to get the contained object (struct branch/remote)
   and its containing struct repository/remote_state (e.g. we don't pass
   struct branch or remote through long call chains). For "struct
   branch", callers usually get the branch from the repo and use it
   immediately. For "struct remote", we don't use container objects
   outside of static functions. If you are interested in seeing all of
   the call sites, you can see a sample commit in [3].
3. The separation between container/contained objects allows us to
   reason better about what the correct interface is. e.g. we might be
   tempted to include a backpointer from struct branch to struct
   remote_state so that we can pass around struct branch and be
   confident that struct branch has all of the information it needs.

   However, I believe that some questions *shouldn't* be answered by
   just struct branch. The prime example in this series is
   branch_get_push() - it conceptually answers 'What is the pushremote
   of this branch', but the behavior we want is closer to 'If
   configured, give me the pushremote for the branch. Otherwise, give me
   the default pushremote of the repo.'. This is arguably a relevant
   detail that should be exposed to callers.

If we want the interface to prevent misuse, we can check that the
correct repository is passed at runtime. Alternatively, if we are
convinced that some questions can only be answered in the context of a
repository, we can take things one step further by using (struct
repository, branch_name) instead of (struct repository, struct branch).

A different concern is that struct repository is added to some callback
signatures even though the function body doesn't use it e.g.
repo_remote_for_branch(). However, this might be more of an accident of
history than anything else - the difference between remote and
pushremote is that remote always defaults to 'origin', whereas
the default value of pushremote is configurable.

Changes since v1:

* In v1, we moved static variables into the_repository->remote_state in
  two steps: static variables > static remote_state >
  the_repository->remote_state. In v2, make this change in one step:
  static variables > the_repository->remote_state.
* Add more instances of repo_* that were missed.

[1] https://lore.kernel.org/git/20210921232529.81811-1-chooglen@google.com/
[2] https://lore.kernel.org/git/xmqq4k9so15i.fsf@gitster.g/
[3] https://github.com/chooglen/git/commit/173e0268fd076044dd6b3cae893eedfa33965942

Glen Choo (3):
  remote: move static variables into per-repository struct
  remote: use remote_state parameter internally
  remote: add struct repository parameter to external functions

 remote.c     | 305 +++++++++++++++++++++++++++++++--------------------
 remote.h     | 130 +++++++++++++++++++---
 repository.c |   8 ++
 repository.h |   4 +
 4 files changed, 312 insertions(+), 135 deletions(-)

Range-diff against v1:
1:  6972ba4dcb = 1:  6972ba4dcb remote: move static variables into per-repository struct
2:  71b1da4389 = 2:  71b1da4389 remote: use remote_state parameter internally
3:  ff12771f06 = 3:  ff12771f06 remote: add struct repository parameter to external functions
-- 
2.33.0.882.g93a45727a2-goog


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH v2 1/3] remote: move static variables into per-repository struct
  2021-10-13 19:31 ` [PATCH v2 0/3] remote: replace static variables with struct remote_state Glen Choo
@ 2021-10-13 19:31   ` Glen Choo
  2021-10-13 20:21     ` Junio C Hamano
  2021-10-13 19:31   ` [PATCH v2 2/3] remote: use remote_state parameter internally Glen Choo
                     ` (2 subsequent siblings)
  3 siblings, 1 reply; 21+ messages in thread
From: Glen Choo @ 2021-10-13 19:31 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Glen Choo

remote.c does not work with non-the_repository because it stores its
state as static variables. To support non-the_repository, we can use a
per-repository struct for the remotes subsystem.

Prepare for this change by defining a struct remote_state that holds
the remotes subsystem state and move the static variables of remote.c
into the_repository->remote_state.

This introduces no behavioral or API changes.

Signed-off-by: Glen Choo <chooglen@google.com>
---
 remote.c     | 187 ++++++++++++++++++++++++++++++++++-----------------
 remote.h     |  34 ++++++++++
 repository.c |   8 +++
 repository.h |   4 ++
 4 files changed, 171 insertions(+), 62 deletions(-)

diff --git a/remote.c b/remote.c
index f958543d70..29c29fcc3b 100644
--- a/remote.c
+++ b/remote.c
@@ -21,33 +21,6 @@ struct counted_string {
 	size_t len;
 	const char *s;
 };
-struct rewrite {
-	const char *base;
-	size_t baselen;
-	struct counted_string *instead_of;
-	int instead_of_nr;
-	int instead_of_alloc;
-};
-struct rewrites {
-	struct rewrite **rewrite;
-	int rewrite_alloc;
-	int rewrite_nr;
-};
-
-static struct remote **remotes;
-static int remotes_alloc;
-static int remotes_nr;
-static struct hashmap remotes_hash;
-
-static struct branch **branches;
-static int branches_alloc;
-static int branches_nr;
-
-static struct branch *current_branch;
-static const char *pushremote_name;
-
-static struct rewrites rewrites;
-static struct rewrites rewrites_push;
 
 static int valid_remote(const struct remote *remote)
 {
@@ -94,14 +67,16 @@ static void add_pushurl(struct remote *remote, const char *pushurl)
 
 static void add_pushurl_alias(struct remote *remote, const char *url)
 {
-	const char *pushurl = alias_url(url, &rewrites_push);
+	const char *pushurl =
+		alias_url(url, &the_repository->remote_state->rewrites_push);
 	if (pushurl != url)
 		add_pushurl(remote, pushurl);
 }
 
 static void add_url_alias(struct remote *remote, const char *url)
 {
-	add_url(remote, alias_url(url, &rewrites));
+	add_url(remote,
+		alias_url(url, &the_repository->remote_state->rewrites));
 	add_pushurl_alias(remote, url);
 }
 
@@ -129,8 +104,9 @@ static int remotes_hash_cmp(const void *unused_cmp_data,
 
 static inline void init_remotes_hash(void)
 {
-	if (!remotes_hash.cmpfn)
-		hashmap_init(&remotes_hash, remotes_hash_cmp, NULL, 0);
+	if (!the_repository->remote_state->remotes_hash.cmpfn)
+		hashmap_init(&the_repository->remote_state->remotes_hash,
+			     remotes_hash_cmp, NULL, 0);
 }
 
 static struct remote *make_remote(const char *name, int len)
@@ -147,7 +123,8 @@ static struct remote *make_remote(const char *name, int len)
 	lookup.len = len;
 	hashmap_entry_init(&lookup_entry, memhash(name, len));
 
-	e = hashmap_get(&remotes_hash, &lookup_entry, &lookup);
+	e = hashmap_get(&the_repository->remote_state->remotes_hash,
+			&lookup_entry, &lookup);
 	if (e)
 		return container_of(e, struct remote, ent);
 
@@ -158,15 +135,41 @@ static struct remote *make_remote(const char *name, int len)
 	refspec_init(&ret->push, REFSPEC_PUSH);
 	refspec_init(&ret->fetch, REFSPEC_FETCH);
 
-	ALLOC_GROW(remotes, remotes_nr + 1, remotes_alloc);
-	remotes[remotes_nr++] = ret;
+	ALLOC_GROW(the_repository->remote_state->remotes,
+		   the_repository->remote_state->remotes_nr + 1,
+		   the_repository->remote_state->remotes_alloc);
+	the_repository->remote_state
+		->remotes[the_repository->remote_state->remotes_nr++] = ret;
 
 	hashmap_entry_init(&ret->ent, lookup_entry.hash);
-	if (hashmap_put_entry(&remotes_hash, ret, ent))
+	if (hashmap_put_entry(&the_repository->remote_state->remotes_hash, ret,
+			      ent))
 		BUG("hashmap_put overwrote entry after hashmap_get returned NULL");
 	return ret;
 }
 
+static void remote_clear(struct remote *remote)
+{
+	int i;
+
+	free((char *)remote->name);
+	free((char *)remote->foreign_vcs);
+
+	for (i = 0; i < remote->url_nr; i++) {
+		free((char *)remote->url[i]);
+	}
+	FREE_AND_NULL(remote->pushurl);
+
+	for (i = 0; i < remote->pushurl_nr; i++) {
+		free((char *)remote->pushurl[i]);
+	}
+	FREE_AND_NULL(remote->pushurl);
+	free((char *)remote->receivepack);
+	free((char *)remote->uploadpack);
+	FREE_AND_NULL(remote->http_proxy);
+	FREE_AND_NULL(remote->http_proxy_authmethod);
+}
+
 static void add_merge(struct branch *branch, const char *name)
 {
 	ALLOC_GROW(branch->merge_name, branch->merge_nr + 1,
@@ -179,15 +182,20 @@ static struct branch *make_branch(const char *name, size_t len)
 	struct branch *ret;
 	int i;
 
-	for (i = 0; i < branches_nr; i++) {
-		if (!strncmp(name, branches[i]->name, len) &&
-		    !branches[i]->name[len])
-			return branches[i];
+	for (i = 0; i < the_repository->remote_state->branches_nr; i++) {
+		if (!strncmp(name,
+			     the_repository->remote_state->branches[i]->name,
+			     len) &&
+		    !the_repository->remote_state->branches[i]->name[len])
+			return the_repository->remote_state->branches[i];
 	}
 
-	ALLOC_GROW(branches, branches_nr + 1, branches_alloc);
+	ALLOC_GROW(the_repository->remote_state->branches,
+		   the_repository->remote_state->branches_nr + 1,
+		   the_repository->remote_state->branches_alloc);
 	CALLOC_ARRAY(ret, 1);
-	branches[branches_nr++] = ret;
+	the_repository->remote_state
+		->branches[the_repository->remote_state->branches_nr++] = ret;
 	ret->name = xstrndup(name, len);
 	ret->refname = xstrfmt("refs/heads/%s", ret->name);
 
@@ -327,12 +335,16 @@ static int handle_config(const char *key, const char *value, void *cb)
 		if (!strcmp(subkey, "insteadof")) {
 			if (!value)
 				return config_error_nonbool(key);
-			rewrite = make_rewrite(&rewrites, name, namelen);
+			rewrite = make_rewrite(
+				&the_repository->remote_state->rewrites, name,
+				namelen);
 			add_instead_of(rewrite, xstrdup(value));
 		} else if (!strcmp(subkey, "pushinsteadof")) {
 			if (!value)
 				return config_error_nonbool(key);
-			rewrite = make_rewrite(&rewrites_push, name, namelen);
+			rewrite = make_rewrite(
+				&the_repository->remote_state->rewrites_push,
+				name, namelen);
 			add_instead_of(rewrite, xstrdup(value));
 		}
 	}
@@ -342,7 +354,9 @@ static int handle_config(const char *key, const char *value, void *cb)
 
 	/* Handle remote.* variables */
 	if (!name && !strcmp(subkey, "pushdefault"))
-		return git_config_string(&pushremote_name, key, value);
+		return git_config_string(
+			&the_repository->remote_state->pushremote_name, key,
+			value);
 
 	if (!name)
 		return 0;
@@ -425,18 +439,34 @@ static int handle_config(const char *key, const char *value, void *cb)
 static void alias_all_urls(void)
 {
 	int i, j;
-	for (i = 0; i < remotes_nr; i++) {
+	for (i = 0; i < the_repository->remote_state->remotes_nr; i++) {
 		int add_pushurl_aliases;
-		if (!remotes[i])
+		if (!the_repository->remote_state->remotes[i])
 			continue;
-		for (j = 0; j < remotes[i]->pushurl_nr; j++) {
-			remotes[i]->pushurl[j] = alias_url(remotes[i]->pushurl[j], &rewrites);
+		for (j = 0;
+		     j < the_repository->remote_state->remotes[i]->pushurl_nr;
+		     j++) {
+			the_repository->remote_state->remotes[i]->pushurl[j] =
+				alias_url(
+					the_repository->remote_state->remotes[i]
+						->pushurl[j],
+					&the_repository->remote_state->rewrites);
 		}
-		add_pushurl_aliases = remotes[i]->pushurl_nr == 0;
-		for (j = 0; j < remotes[i]->url_nr; j++) {
+		add_pushurl_aliases =
+			the_repository->remote_state->remotes[i]->pushurl_nr ==
+			0;
+		for (j = 0;
+		     j < the_repository->remote_state->remotes[i]->url_nr;
+		     j++) {
 			if (add_pushurl_aliases)
-				add_pushurl_alias(remotes[i], remotes[i]->url[j]);
-			remotes[i]->url[j] = alias_url(remotes[i]->url[j], &rewrites);
+				add_pushurl_alias(
+					the_repository->remote_state->remotes[i],
+					the_repository->remote_state->remotes[i]
+						->url[j]);
+			the_repository->remote_state->remotes[i]
+				->url[j] = alias_url(
+				the_repository->remote_state->remotes[i]->url[j],
+				&the_repository->remote_state->rewrites);
 		}
 	}
 }
@@ -450,12 +480,13 @@ static void read_config(void)
 		return;
 	loaded = 1;
 
-	current_branch = NULL;
+	the_repository->remote_state->current_branch = NULL;
 	if (startup_info->have_repository) {
 		const char *head_ref = resolve_ref_unsafe("HEAD", 0, NULL, &flag);
 		if (head_ref && (flag & REF_ISSYMREF) &&
 		    skip_prefix(head_ref, "refs/heads/", &head_ref)) {
-			current_branch = make_branch(head_ref, strlen(head_ref));
+			the_repository->remote_state->current_branch =
+				make_branch(head_ref, strlen(head_ref));
 		}
 	}
 	git_config(handle_config, NULL);
@@ -493,10 +524,10 @@ const char *pushremote_for_branch(struct branch *branch, int *explicit)
 			*explicit = 1;
 		return branch->pushremote_name;
 	}
-	if (pushremote_name) {
+	if (the_repository->remote_state->pushremote_name) {
 		if (explicit)
 			*explicit = 1;
-		return pushremote_name;
+		return the_repository->remote_state->pushremote_name;
 	}
 	return remote_for_branch(branch, explicit);
 }
@@ -534,7 +565,8 @@ static struct remote *remote_get_1(const char *name,
 	if (name)
 		name_given = 1;
 	else
-		name = get_default(current_branch, &name_given);
+		name = get_default(the_repository->remote_state->current_branch,
+				   &name_given);
 
 	ret = make_remote(name, 0);
 	if (valid_remote_nick(name) && have_git_dir()) {
@@ -573,11 +605,13 @@ int for_each_remote(each_remote_fn fn, void *priv)
 {
 	int i, result = 0;
 	read_config();
-	for (i = 0; i < remotes_nr && !result; i++) {
-		struct remote *r = remotes[i];
-		if (!r)
+	for (i = 0; i < the_repository->remote_state->remotes_nr && !result;
+	     i++) {
+		struct remote *remote =
+			the_repository->remote_state->remotes[i];
+		if (!remote)
 			continue;
-		result = fn(r, priv);
+		result = fn(remote, priv);
 	}
 	return result;
 }
@@ -1685,7 +1719,7 @@ struct branch *branch_get(const char *name)
 
 	read_config();
 	if (!name || !*name || !strcmp(name, "HEAD"))
-		ret = current_branch;
+		ret = the_repository->remote_state->current_branch;
 	else
 		ret = make_branch(name, strlen(name));
 	set_merge(ret);
@@ -2585,3 +2619,32 @@ void apply_push_cas(struct push_cas_option *cas,
 			check_if_includes_upstream(ref);
 	}
 }
+
+struct remote_state *remote_state_new(void)
+{
+	struct remote_state *r = xmalloc(sizeof(*r));
+
+	memset(r, 0, sizeof(*r));
+	return r;
+}
+
+void remote_state_clear(struct remote_state *remote_state)
+{
+	int i;
+
+	for (i = 0; i < remote_state->remotes_nr; i++) {
+		remote_clear(remote_state->remotes[i]);
+	}
+	FREE_AND_NULL(remote_state->remotes);
+	remote_state->remotes_alloc = 0;
+	remote_state->remotes_nr = 0;
+
+	hashmap_clear_and_free(&remote_state->remotes_hash, struct remote, ent);
+
+	for (i = 0; i < remote_state->branches_nr; i++) {
+		FREE_AND_NULL(remote_state->branches[i]);
+	}
+	FREE_AND_NULL(remote_state->branches);
+	remote_state->branches_alloc = 0;
+	remote_state->branches_nr = 0;
+}
diff --git a/remote.h b/remote.h
index 5a59198252..184d14af3d 100644
--- a/remote.h
+++ b/remote.h
@@ -23,6 +23,40 @@ enum {
 	REMOTE_BRANCHES
 };
 
+struct rewrite {
+	const char *base;
+	size_t baselen;
+	struct counted_string *instead_of;
+	int instead_of_nr;
+	int instead_of_alloc;
+};
+struct rewrites {
+	struct rewrite **rewrite;
+	int rewrite_alloc;
+	int rewrite_nr;
+};
+
+struct remote_state {
+	int config_loaded;
+
+	struct remote **remotes;
+	int remotes_alloc;
+	int remotes_nr;
+	struct hashmap remotes_hash;
+
+	struct branch **branches;
+	int branches_alloc;
+	int branches_nr;
+
+	struct branch *current_branch;
+	const char *pushremote_name;
+
+	struct rewrites rewrites;
+	struct rewrites rewrites_push;
+};
+void remote_state_clear(struct remote_state *remote_state);
+struct remote_state *remote_state_new(void);
+
 struct remote {
 	struct hashmap_entry ent;
 
diff --git a/repository.c b/repository.c
index c5b90ba93e..c7ea706c20 100644
--- a/repository.c
+++ b/repository.c
@@ -9,6 +9,7 @@
 #include "config.h"
 #include "object.h"
 #include "lockfile.h"
+#include "remote.h"
 #include "submodule-config.h"
 #include "sparse-index.h"
 #include "promisor-remote.h"
@@ -24,6 +25,7 @@ void initialize_the_repository(void)
 
 	the_repo.index = &the_index;
 	the_repo.objects = raw_object_store_new();
+	the_repo.remote_state = remote_state_new();
 	the_repo.parsed_objects = parsed_object_pool_new();
 
 	repo_set_hash_algo(&the_repo, GIT_HASH_SHA1);
@@ -164,6 +166,7 @@ int repo_init(struct repository *repo,
 
 	repo->objects = raw_object_store_new();
 	repo->parsed_objects = parsed_object_pool_new();
+	repo->remote_state = remote_state_new();
 
 	if (repo_init_gitdir(repo, gitdir))
 		goto error;
@@ -270,6 +273,11 @@ void repo_clear(struct repository *repo)
 		promisor_remote_clear(repo->promisor_remote_config);
 		FREE_AND_NULL(repo->promisor_remote_config);
 	}
+
+	if (repo->remote_state) {
+		remote_state_clear(repo->remote_state);
+		FREE_AND_NULL(repo->remote_state);
+	}
 }
 
 int repo_read_index(struct repository *repo)
diff --git a/repository.h b/repository.h
index a057653981..98f9583470 100644
--- a/repository.h
+++ b/repository.h
@@ -11,6 +11,7 @@ struct pathspec;
 struct raw_object_store;
 struct submodule_cache;
 struct promisor_remote_config;
+struct remote_state;
 
 enum untracked_cache_setting {
 	UNTRACKED_CACHE_KEEP,
@@ -127,6 +128,9 @@ struct repository {
 	 */
 	struct index_state *index;
 
+	/* Repository's remotes and associated structures. */
+	struct remote_state *remote_state;
+
 	/* Repository's current hash algorithm, as serialized on disk. */
 	const struct git_hash_algo *hash_algo;
 
-- 
2.33.0.882.g93a45727a2-goog


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH v2 2/3] remote: use remote_state parameter internally
  2021-10-13 19:31 ` [PATCH v2 0/3] remote: replace static variables with struct remote_state Glen Choo
  2021-10-13 19:31   ` [PATCH v2 1/3] remote: move static variables into per-repository struct Glen Choo
@ 2021-10-13 19:31   ` Glen Choo
  2021-10-13 20:23     ` Junio C Hamano
  2021-10-13 19:31   ` [PATCH v2 3/3] remote: add struct repository parameter to external functions Glen Choo
  2021-10-13 20:11   ` [PATCH v2 0/3] remote: replace static variables with struct remote_state Junio C Hamano
  3 siblings, 1 reply; 21+ messages in thread
From: Glen Choo @ 2021-10-13 19:31 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Glen Choo

In internal-facing functions, replace the_repository->remote_state with
a struct remote_state parameter, but do not change external-facing
functions.

As a result, most static functions no longer reference
the_repository->remote_state. The exceptions are those that are used in
a way that depends on external-facing functions e.g. the callbacks to
remote_get_1().

Signed-off-by: Glen Choo <chooglen@google.com>
---
 remote.c | 165 ++++++++++++++++++++++++++-----------------------------
 1 file changed, 78 insertions(+), 87 deletions(-)

diff --git a/remote.c b/remote.c
index 29c29fcc3b..850ccd8eff 100644
--- a/remote.c
+++ b/remote.c
@@ -65,19 +65,19 @@ static void add_pushurl(struct remote *remote, const char *pushurl)
 	remote->pushurl[remote->pushurl_nr++] = pushurl;
 }
 
-static void add_pushurl_alias(struct remote *remote, const char *url)
+static void add_pushurl_alias(struct remote_state *remote_state,
+			      struct remote *remote, const char *url)
 {
-	const char *pushurl =
-		alias_url(url, &the_repository->remote_state->rewrites_push);
+	const char *pushurl = alias_url(url, &remote_state->rewrites_push);
 	if (pushurl != url)
 		add_pushurl(remote, pushurl);
 }
 
-static void add_url_alias(struct remote *remote, const char *url)
+static void add_url_alias(struct remote_state *remote_state,
+			  struct remote *remote, const char *url)
 {
-	add_url(remote,
-		alias_url(url, &the_repository->remote_state->rewrites));
-	add_pushurl_alias(remote, url);
+	add_url(remote, alias_url(url, &remote_state->rewrites));
+	add_pushurl_alias(remote_state, remote, url);
 }
 
 struct remotes_hash_key {
@@ -102,14 +102,19 @@ static int remotes_hash_cmp(const void *unused_cmp_data,
 		return strcmp(a->name, b->name);
 }
 
-static inline void init_remotes_hash(void)
+/**
+ * NEEDSWORK: Now that the hashmap is in a struct, this should probably
+ * just be moved into remote_state_new().
+ */
+static inline void init_remotes_hash(struct remote_state *remote_state)
 {
-	if (!the_repository->remote_state->remotes_hash.cmpfn)
-		hashmap_init(&the_repository->remote_state->remotes_hash,
-			     remotes_hash_cmp, NULL, 0);
+	if (!remote_state->remotes_hash.cmpfn)
+		hashmap_init(&remote_state->remotes_hash, remotes_hash_cmp,
+			     NULL, 0);
 }
 
-static struct remote *make_remote(const char *name, int len)
+static struct remote *make_remote(struct remote_state *remote_state,
+				  const char *name, int len)
 {
 	struct remote *ret;
 	struct remotes_hash_key lookup;
@@ -118,13 +123,12 @@ static struct remote *make_remote(const char *name, int len)
 	if (!len)
 		len = strlen(name);
 
-	init_remotes_hash();
+	init_remotes_hash(remote_state);
 	lookup.str = name;
 	lookup.len = len;
 	hashmap_entry_init(&lookup_entry, memhash(name, len));
 
-	e = hashmap_get(&the_repository->remote_state->remotes_hash,
-			&lookup_entry, &lookup);
+	e = hashmap_get(&remote_state->remotes_hash, &lookup_entry, &lookup);
 	if (e)
 		return container_of(e, struct remote, ent);
 
@@ -135,15 +139,12 @@ static struct remote *make_remote(const char *name, int len)
 	refspec_init(&ret->push, REFSPEC_PUSH);
 	refspec_init(&ret->fetch, REFSPEC_FETCH);
 
-	ALLOC_GROW(the_repository->remote_state->remotes,
-		   the_repository->remote_state->remotes_nr + 1,
-		   the_repository->remote_state->remotes_alloc);
-	the_repository->remote_state
-		->remotes[the_repository->remote_state->remotes_nr++] = ret;
+	ALLOC_GROW(remote_state->remotes, remote_state->remotes_nr + 1,
+		   remote_state->remotes_alloc);
+	remote_state->remotes[remote_state->remotes_nr++] = ret;
 
 	hashmap_entry_init(&ret->ent, lookup_entry.hash);
-	if (hashmap_put_entry(&the_repository->remote_state->remotes_hash, ret,
-			      ent))
+	if (hashmap_put_entry(&remote_state->remotes_hash, ret, ent))
 		BUG("hashmap_put overwrote entry after hashmap_get returned NULL");
 	return ret;
 }
@@ -177,25 +178,22 @@ static void add_merge(struct branch *branch, const char *name)
 	branch->merge_name[branch->merge_nr++] = name;
 }
 
-static struct branch *make_branch(const char *name, size_t len)
+static struct branch *make_branch(struct remote_state *remote_state,
+				  const char *name, size_t len)
 {
 	struct branch *ret;
 	int i;
 
-	for (i = 0; i < the_repository->remote_state->branches_nr; i++) {
-		if (!strncmp(name,
-			     the_repository->remote_state->branches[i]->name,
-			     len) &&
-		    !the_repository->remote_state->branches[i]->name[len])
-			return the_repository->remote_state->branches[i];
+	for (i = 0; i < remote_state->branches_nr; i++) {
+		if (!strncmp(name, remote_state->branches[i]->name, len) &&
+		    !remote_state->branches[i]->name[len])
+			return remote_state->branches[i];
 	}
 
-	ALLOC_GROW(the_repository->remote_state->branches,
-		   the_repository->remote_state->branches_nr + 1,
-		   the_repository->remote_state->branches_alloc);
+	ALLOC_GROW(remote_state->branches, remote_state->branches_nr + 1,
+		   remote_state->branches_alloc);
 	CALLOC_ARRAY(ret, 1);
-	the_repository->remote_state
-		->branches[the_repository->remote_state->branches_nr++] = ret;
+	remote_state->branches[remote_state->branches_nr++] = ret;
 	ret->name = xstrndup(name, len);
 	ret->refname = xstrfmt("refs/heads/%s", ret->name);
 
@@ -237,7 +235,8 @@ static const char *skip_spaces(const char *s)
 	return s;
 }
 
-static void read_remotes_file(struct remote *remote)
+static void read_remotes_file(struct remote_state *remote_state,
+			      struct remote *remote)
 {
 	struct strbuf buf = STRBUF_INIT;
 	FILE *f = fopen_or_warn(git_path("remotes/%s", remote->name), "r");
@@ -252,7 +251,8 @@ static void read_remotes_file(struct remote *remote)
 		strbuf_rtrim(&buf);
 
 		if (skip_prefix(buf.buf, "URL:", &v))
-			add_url_alias(remote, xstrdup(skip_spaces(v)));
+			add_url_alias(remote_state, remote,
+				      xstrdup(skip_spaces(v)));
 		else if (skip_prefix(buf.buf, "Push:", &v))
 			refspec_append(&remote->push, skip_spaces(v));
 		else if (skip_prefix(buf.buf, "Pull:", &v))
@@ -262,7 +262,8 @@ static void read_remotes_file(struct remote *remote)
 	fclose(f);
 }
 
-static void read_branches_file(struct remote *remote)
+static void read_branches_file(struct remote_state *remote_state,
+			       struct remote *remote)
 {
 	char *frag;
 	struct strbuf buf = STRBUF_INIT;
@@ -294,7 +295,7 @@ static void read_branches_file(struct remote *remote)
 	else
 		frag = (char *)git_default_branch_name(0);
 
-	add_url_alias(remote, strbuf_detach(&buf, NULL));
+	add_url_alias(remote_state, remote, strbuf_detach(&buf, NULL));
 	refspec_appendf(&remote->fetch, "refs/heads/%s:refs/heads/%s",
 			frag, remote->name);
 
@@ -313,10 +314,12 @@ static int handle_config(const char *key, const char *value, void *cb)
 	const char *subkey;
 	struct remote *remote;
 	struct branch *branch;
+	struct remote_state *remote_state = cb;
+
 	if (parse_config_key(key, "branch", &name, &namelen, &subkey) >= 0) {
 		if (!name)
 			return 0;
-		branch = make_branch(name, namelen);
+		branch = make_branch(remote_state, name, namelen);
 		if (!strcmp(subkey, "remote")) {
 			return git_config_string(&branch->remote_name, key, value);
 		} else if (!strcmp(subkey, "pushremote")) {
@@ -335,16 +338,14 @@ static int handle_config(const char *key, const char *value, void *cb)
 		if (!strcmp(subkey, "insteadof")) {
 			if (!value)
 				return config_error_nonbool(key);
-			rewrite = make_rewrite(
-				&the_repository->remote_state->rewrites, name,
-				namelen);
+			rewrite = make_rewrite(&remote_state->rewrites, name,
+					       namelen);
 			add_instead_of(rewrite, xstrdup(value));
 		} else if (!strcmp(subkey, "pushinsteadof")) {
 			if (!value)
 				return config_error_nonbool(key);
-			rewrite = make_rewrite(
-				&the_repository->remote_state->rewrites_push,
-				name, namelen);
+			rewrite = make_rewrite(&remote_state->rewrites_push,
+					       name, namelen);
 			add_instead_of(rewrite, xstrdup(value));
 		}
 	}
@@ -354,9 +355,8 @@ static int handle_config(const char *key, const char *value, void *cb)
 
 	/* Handle remote.* variables */
 	if (!name && !strcmp(subkey, "pushdefault"))
-		return git_config_string(
-			&the_repository->remote_state->pushremote_name, key,
-			value);
+		return git_config_string(&remote_state->pushremote_name, key,
+					 value);
 
 	if (!name)
 		return 0;
@@ -366,7 +366,7 @@ static int handle_config(const char *key, const char *value, void *cb)
 			name);
 		return 0;
 	}
-	remote = make_remote(name, namelen);
+	remote = make_remote(remote_state, name, namelen);
 	remote->origin = REMOTE_CONFIG;
 	if (current_config_scope() == CONFIG_SCOPE_LOCAL ||
 	    current_config_scope() == CONFIG_SCOPE_WORKTREE)
@@ -436,42 +436,32 @@ static int handle_config(const char *key, const char *value, void *cb)
 	return 0;
 }
 
-static void alias_all_urls(void)
+static void alias_all_urls(struct remote_state *remote_state)
 {
 	int i, j;
-	for (i = 0; i < the_repository->remote_state->remotes_nr; i++) {
+	for (i = 0; i < remote_state->remotes_nr; i++) {
 		int add_pushurl_aliases;
-		if (!the_repository->remote_state->remotes[i])
+		if (!remote_state->remotes[i])
 			continue;
-		for (j = 0;
-		     j < the_repository->remote_state->remotes[i]->pushurl_nr;
-		     j++) {
-			the_repository->remote_state->remotes[i]->pushurl[j] =
-				alias_url(
-					the_repository->remote_state->remotes[i]
-						->pushurl[j],
-					&the_repository->remote_state->rewrites);
+		for (j = 0; j < remote_state->remotes[i]->pushurl_nr; j++) {
+			remote_state->remotes[i]->pushurl[j] =
+				alias_url(remote_state->remotes[i]->pushurl[j],
+					  &remote_state->rewrites);
 		}
-		add_pushurl_aliases =
-			the_repository->remote_state->remotes[i]->pushurl_nr ==
-			0;
-		for (j = 0;
-		     j < the_repository->remote_state->remotes[i]->url_nr;
-		     j++) {
+		add_pushurl_aliases = remote_state->remotes[i]->pushurl_nr == 0;
+		for (j = 0; j < remote_state->remotes[i]->url_nr; j++) {
 			if (add_pushurl_aliases)
 				add_pushurl_alias(
-					the_repository->remote_state->remotes[i],
-					the_repository->remote_state->remotes[i]
-						->url[j]);
-			the_repository->remote_state->remotes[i]
-				->url[j] = alias_url(
-				the_repository->remote_state->remotes[i]->url[j],
-				&the_repository->remote_state->rewrites);
+					remote_state, remote_state->remotes[i],
+					remote_state->remotes[i]->url[j]);
+			remote_state->remotes[i]->url[j] =
+				alias_url(remote_state->remotes[i]->url[j],
+					  &remote_state->rewrites);
 		}
 	}
 }
 
-static void read_config(void)
+static void read_config(struct remote_state *remote_state)
 {
 	static int loaded;
 	int flag;
@@ -480,17 +470,17 @@ static void read_config(void)
 		return;
 	loaded = 1;
 
-	the_repository->remote_state->current_branch = NULL;
+	remote_state->current_branch = NULL;
 	if (startup_info->have_repository) {
 		const char *head_ref = resolve_ref_unsafe("HEAD", 0, NULL, &flag);
 		if (head_ref && (flag & REF_ISSYMREF) &&
 		    skip_prefix(head_ref, "refs/heads/", &head_ref)) {
-			the_repository->remote_state->current_branch =
-				make_branch(head_ref, strlen(head_ref));
+			remote_state->current_branch = make_branch(
+				remote_state, head_ref, strlen(head_ref));
 		}
 	}
-	git_config(handle_config, NULL);
-	alias_all_urls();
+	git_config(handle_config, remote_state);
+	alias_all_urls(remote_state);
 }
 
 static int valid_remote_nick(const char *name)
@@ -560,7 +550,7 @@ static struct remote *remote_get_1(const char *name,
 	struct remote *ret;
 	int name_given = 0;
 
-	read_config();
+	read_config(the_repository->remote_state);
 
 	if (name)
 		name_given = 1;
@@ -568,15 +558,15 @@ static struct remote *remote_get_1(const char *name,
 		name = get_default(the_repository->remote_state->current_branch,
 				   &name_given);
 
-	ret = make_remote(name, 0);
+	ret = make_remote(the_repository->remote_state, name, 0);
 	if (valid_remote_nick(name) && have_git_dir()) {
 		if (!valid_remote(ret))
-			read_remotes_file(ret);
+			read_remotes_file(the_repository->remote_state, ret);
 		if (!valid_remote(ret))
-			read_branches_file(ret);
+			read_branches_file(the_repository->remote_state, ret);
 	}
 	if (name_given && !valid_remote(ret))
-		add_url_alias(ret, name);
+		add_url_alias(the_repository->remote_state, ret, name);
 	if (!valid_remote(ret))
 		return NULL;
 	return ret;
@@ -604,7 +594,7 @@ int remote_is_configured(struct remote *remote, int in_repo)
 int for_each_remote(each_remote_fn fn, void *priv)
 {
 	int i, result = 0;
-	read_config();
+	read_config(the_repository->remote_state);
 	for (i = 0; i < the_repository->remote_state->remotes_nr && !result;
 	     i++) {
 		struct remote *remote =
@@ -1717,11 +1707,12 @@ struct branch *branch_get(const char *name)
 {
 	struct branch *ret;
 
-	read_config();
+	read_config(the_repository->remote_state);
 	if (!name || !*name || !strcmp(name, "HEAD"))
 		ret = the_repository->remote_state->current_branch;
 	else
-		ret = make_branch(name, strlen(name));
+		ret = make_branch(the_repository->remote_state, name,
+				  strlen(name));
 	set_merge(ret);
 	return ret;
 }
-- 
2.33.0.882.g93a45727a2-goog


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH v2 3/3] remote: add struct repository parameter to external functions
  2021-10-13 19:31 ` [PATCH v2 0/3] remote: replace static variables with struct remote_state Glen Choo
  2021-10-13 19:31   ` [PATCH v2 1/3] remote: move static variables into per-repository struct Glen Choo
  2021-10-13 19:31   ` [PATCH v2 2/3] remote: use remote_state parameter internally Glen Choo
@ 2021-10-13 19:31   ` Glen Choo
  2021-10-13 20:24     ` Junio C Hamano
  2021-10-13 20:11   ` [PATCH v2 0/3] remote: replace static variables with struct remote_state Junio C Hamano
  3 siblings, 1 reply; 21+ messages in thread
From: Glen Choo @ 2021-10-13 19:31 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Glen Choo

Finish plumbing remote_state by adding a struct repository
parameter to repo_* functions. While this removes all references to
the_repository->remote_state, certain functions still use the_repository
to parse oids.

Signed-off-by: Glen Choo <chooglen@google.com>
---
 remote.c | 111 ++++++++++++++++++++++++++++++-------------------------
 remote.h |  96 +++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 142 insertions(+), 65 deletions(-)

diff --git a/remote.c b/remote.c
index 850ccd8eff..be0889c53b 100644
--- a/remote.c
+++ b/remote.c
@@ -495,7 +495,8 @@ static int valid_remote_nick(const char *name)
 	return 1;
 }
 
-const char *remote_for_branch(struct branch *branch, int *explicit)
+const char *repo_remote_for_branch(struct repository *repo,
+				   struct branch *branch, int *explicit)
 {
 	if (branch && branch->remote_name) {
 		if (explicit)
@@ -507,22 +508,24 @@ const char *remote_for_branch(struct branch *branch, int *explicit)
 	return "origin";
 }
 
-const char *pushremote_for_branch(struct branch *branch, int *explicit)
+const char *repo_pushremote_for_branch(struct repository *repo,
+				       struct branch *branch, int *explicit)
 {
 	if (branch && branch->pushremote_name) {
 		if (explicit)
 			*explicit = 1;
 		return branch->pushremote_name;
 	}
-	if (the_repository->remote_state->pushremote_name) {
+	if (repo->remote_state->pushremote_name) {
 		if (explicit)
 			*explicit = 1;
-		return the_repository->remote_state->pushremote_name;
+		return repo->remote_state->pushremote_name;
 	}
-	return remote_for_branch(branch, explicit);
+	return repo_remote_for_branch(repo, branch, explicit);
 }
 
-const char *remote_ref_for_branch(struct branch *branch, int for_push)
+const char *repo_remote_ref_for_branch(struct repository *repo,
+				       struct branch *branch, int for_push)
 {
 	if (branch) {
 		if (!for_push) {
@@ -530,9 +533,11 @@ const char *remote_ref_for_branch(struct branch *branch, int for_push)
 				return branch->merge_name[0];
 			}
 		} else {
-			const char *dst, *remote_name =
-				pushremote_for_branch(branch, NULL);
-			struct remote *remote = remote_get(remote_name);
+			const char *dst,
+				*remote_name = repo_pushremote_for_branch(
+					repo, branch, NULL);
+			struct remote *remote =
+				repo_remote_get(repo, remote_name);
 
 			if (remote && remote->push.nr &&
 			    (dst = apply_refspecs(&remote->push,
@@ -544,42 +549,43 @@ const char *remote_ref_for_branch(struct branch *branch, int for_push)
 	return NULL;
 }
 
-static struct remote *remote_get_1(const char *name,
-				   const char *(*get_default)(struct branch *, int *))
+static struct remote *repo_remote_get_1(
+	struct repository *repo, const char *name,
+	const char *(*get_default)(struct repository *, struct branch *, int *))
 {
 	struct remote *ret;
 	int name_given = 0;
 
-	read_config(the_repository->remote_state);
+	read_config(repo->remote_state);
 
 	if (name)
 		name_given = 1;
 	else
-		name = get_default(the_repository->remote_state->current_branch,
+		name = get_default(repo, repo->remote_state->current_branch,
 				   &name_given);
 
-	ret = make_remote(the_repository->remote_state, name, 0);
+	ret = make_remote(repo->remote_state, name, 0);
 	if (valid_remote_nick(name) && have_git_dir()) {
 		if (!valid_remote(ret))
-			read_remotes_file(the_repository->remote_state, ret);
+			read_remotes_file(repo->remote_state, ret);
 		if (!valid_remote(ret))
-			read_branches_file(the_repository->remote_state, ret);
+			read_branches_file(repo->remote_state, ret);
 	}
 	if (name_given && !valid_remote(ret))
-		add_url_alias(the_repository->remote_state, ret, name);
+		add_url_alias(repo->remote_state, ret, name);
 	if (!valid_remote(ret))
 		return NULL;
 	return ret;
 }
 
-struct remote *remote_get(const char *name)
+struct remote *repo_remote_get(struct repository *repo, const char *name)
 {
-	return remote_get_1(name, remote_for_branch);
+	return repo_remote_get_1(repo, name, repo_remote_for_branch);
 }
 
-struct remote *pushremote_get(const char *name)
+struct remote *repo_pushremote_get(struct repository *repo, const char *name)
 {
-	return remote_get_1(name, pushremote_for_branch);
+	return repo_remote_get_1(repo, name, repo_pushremote_for_branch);
 }
 
 int remote_is_configured(struct remote *remote, int in_repo)
@@ -591,14 +597,12 @@ int remote_is_configured(struct remote *remote, int in_repo)
 	return !!remote->origin;
 }
 
-int for_each_remote(each_remote_fn fn, void *priv)
+int repo_for_each_remote(struct repository *repo, each_remote_fn fn, void *priv)
 {
 	int i, result = 0;
-	read_config(the_repository->remote_state);
-	for (i = 0; i < the_repository->remote_state->remotes_nr && !result;
-	     i++) {
-		struct remote *remote =
-			the_repository->remote_state->remotes[i];
+	read_config(repo->remote_state);
+	for (i = 0; i < repo->remote_state->remotes_nr && !result; i++) {
+		struct remote *remote = repo->remote_state->remotes[i];
 		if (!remote)
 			continue;
 		result = fn(remote, priv);
@@ -1666,7 +1670,7 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
 	}
 }
 
-static void set_merge(struct branch *ret)
+static void set_merge(struct repository *repository, struct branch *ret)
 {
 	struct remote *remote;
 	char *ref;
@@ -1686,7 +1690,7 @@ static void set_merge(struct branch *ret)
 		return;
 	}
 
-	remote = remote_get(ret->remote_name);
+	remote = repo_remote_get(repository, ret->remote_name);
 
 	CALLOC_ARRAY(ret->merge, ret->merge_nr);
 	for (i = 0; i < ret->merge_nr; i++) {
@@ -1703,17 +1707,16 @@ static void set_merge(struct branch *ret)
 	}
 }
 
-struct branch *branch_get(const char *name)
+struct branch *repo_branch_get(struct repository *repo, const char *name)
 {
 	struct branch *ret;
 
-	read_config(the_repository->remote_state);
+	read_config(repo->remote_state);
 	if (!name || !*name || !strcmp(name, "HEAD"))
-		ret = the_repository->remote_state->current_branch;
+		ret = repo->remote_state->current_branch;
 	else
-		ret = make_branch(the_repository->remote_state, name,
-				  strlen(name));
-	set_merge(ret);
+		ret = make_branch(repo->remote_state, name, strlen(name));
+	set_merge(repo, ret);
 	return ret;
 }
 
@@ -1743,7 +1746,8 @@ static const char *error_buf(struct strbuf *err, const char *fmt, ...)
 	return NULL;
 }
 
-const char *branch_get_upstream(struct branch *branch, struct strbuf *err)
+const char *repo_branch_get_upstream(struct repository *repo,
+				     struct branch *branch, struct strbuf *err)
 {
 	if (!branch)
 		return error_buf(err, _("HEAD does not point to a branch"));
@@ -1784,11 +1788,14 @@ static const char *tracking_for_push_dest(struct remote *remote,
 	return ret;
 }
 
-static const char *branch_get_push_1(struct branch *branch, struct strbuf *err)
+static const char *repo_branch_get_push_1(struct repository *repo,
+					  struct branch *branch,
+					  struct strbuf *err)
 {
 	struct remote *remote;
 
-	remote = remote_get(pushremote_for_branch(branch, NULL));
+	remote = repo_remote_get(repo, repo_pushremote_for_branch(repo, branch,
+								  NULL));
 	if (!remote)
 		return error_buf(err,
 				 _("branch '%s' has no remote for pushing"),
@@ -1821,14 +1828,14 @@ static const char *branch_get_push_1(struct branch *branch, struct strbuf *err)
 		return tracking_for_push_dest(remote, branch->refname, err);
 
 	case PUSH_DEFAULT_UPSTREAM:
-		return branch_get_upstream(branch, err);
+		return repo_branch_get_upstream(repo, branch, err);
 
 	case PUSH_DEFAULT_UNSPECIFIED:
 	case PUSH_DEFAULT_SIMPLE:
 		{
 			const char *up, *cur;
 
-			up = branch_get_upstream(branch, err);
+			up = repo_branch_get_upstream(repo, branch, err);
 			if (!up)
 				return NULL;
 			cur = tracking_for_push_dest(remote, branch->refname, err);
@@ -1844,13 +1851,15 @@ static const char *branch_get_push_1(struct branch *branch, struct strbuf *err)
 	BUG("unhandled push situation");
 }
 
-const char *branch_get_push(struct branch *branch, struct strbuf *err)
+const char *repo_branch_get_push(struct repository *repo, struct branch *branch,
+				 struct strbuf *err)
 {
 	if (!branch)
 		return error_buf(err, _("HEAD does not point to a branch"));
 
 	if (!branch->push_tracking_ref)
-		branch->push_tracking_ref = branch_get_push_1(branch, err);
+		branch->push_tracking_ref =
+			repo_branch_get_push_1(repo, branch, err);
 	return branch->push_tracking_ref;
 }
 
@@ -2103,15 +2112,16 @@ static int stat_branch_pair(const char *branch_name, const char *base,
  * upstream defined, or ref does not exist).  Returns 0 if the commits are
  * identical.  Returns 1 if commits are different.
  */
-int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs,
-		       const char **tracking_name, int for_push,
-		       enum ahead_behind_flags abf)
+int repo_stat_tracking_info(struct repository *repo, struct branch *branch,
+			    int *num_ours, int *num_theirs,
+			    const char **tracking_name, int for_push,
+			    enum ahead_behind_flags abf)
 {
 	const char *base;
 
 	/* Cannot stat unless we are marked to build on top of somebody else. */
-	base = for_push ? branch_get_push(branch, NULL) :
-		branch_get_upstream(branch, NULL);
+	base = for_push ? repo_branch_get_push(repo, branch, NULL) :
+				repo_branch_get_upstream(repo, branch, NULL);
 	if (tracking_name)
 		*tracking_name = base;
 	if (!base)
@@ -2123,15 +2133,16 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs,
 /*
  * Return true when there is anything to report, otherwise false.
  */
-int format_tracking_info(struct branch *branch, struct strbuf *sb,
-			 enum ahead_behind_flags abf)
+int repo_format_tracking_info(struct repository *repo, struct branch *branch,
+			      struct strbuf *sb, enum ahead_behind_flags abf)
 {
 	int ours, theirs, sti;
 	const char *full_base;
 	char *base;
 	int upstream_is_gone = 0;
 
-	sti = stat_tracking_info(branch, &ours, &theirs, &full_base, 0, abf);
+	sti = repo_stat_tracking_info(repo, branch, &ours, &theirs, &full_base,
+				      0, abf);
 	if (sti < 0) {
 		if (!full_base)
 			return 0;
diff --git a/remote.h b/remote.h
index 184d14af3d..a1cf86f973 100644
--- a/remote.h
+++ b/remote.h
@@ -117,15 +117,28 @@ struct remote {
  * remote_get(NULL) will return the default remote, given the current branch
  * and configuration.
  */
-struct remote *remote_get(const char *name);
-
-struct remote *pushremote_get(const char *name);
+struct remote *repo_remote_get(struct repository *repo, const char *name);
+static inline struct remote *remote_get(const char *name)
+{
+	return repo_remote_get(the_repository, name);
+}
+
+struct remote *repo_pushremote_get(struct repository *repo, const char *name);
+static inline struct remote *pushremote_get(const char *name)
+{
+	return repo_pushremote_get(the_repository, name);
+}
 int remote_is_configured(struct remote *remote, int in_repo);
 
 typedef int each_remote_fn(struct remote *remote, void *priv);
 
 /* iterate through struct remotes */
-int for_each_remote(each_remote_fn fn, void *priv);
+int repo_for_each_remote(struct repository *repo, each_remote_fn fn,
+			 void *priv);
+static inline int for_each_remote(each_remote_fn fn, void *priv)
+{
+	return repo_for_each_remote(the_repository, fn, priv);
+}
 
 int remote_has_url(struct remote *remote, const char *url);
 
@@ -320,10 +333,35 @@ struct branch {
 	const char *push_tracking_ref;
 };
 
-struct branch *branch_get(const char *name);
-const char *remote_for_branch(struct branch *branch, int *explicit);
-const char *pushremote_for_branch(struct branch *branch, int *explicit);
-const char *remote_ref_for_branch(struct branch *branch, int for_push);
+struct branch *repo_branch_get(struct repository *repo, const char *name);
+static inline struct branch *branch_get(const char *name)
+{
+	return repo_branch_get(the_repository, name);
+}
+
+const char *repo_remote_for_branch(struct repository *repo,
+				   struct branch *branch, int *explicit);
+static inline const char *remote_for_branch(struct branch *branch,
+					    int *explicit)
+{
+	return repo_remote_for_branch(the_repository, branch, explicit);
+}
+
+const char *repo_pushremote_for_branch(struct repository *repo,
+				       struct branch *branch, int *explicit);
+static inline const char *pushremote_for_branch(struct branch *branch,
+						int *explicit)
+{
+	return repo_pushremote_for_branch(the_repository, branch, explicit);
+}
+
+const char *repo_remote_ref_for_branch(struct repository *repo,
+				       struct branch *branch, int for_push);
+static inline const char *remote_ref_for_branch(struct branch *branch,
+						int for_push)
+{
+	return repo_remote_ref_for_branch(the_repository, branch, for_push);
+}
 
 /* returns true if the given branch has merge configuration given. */
 int branch_has_merge_config(struct branch *branch);
@@ -339,7 +377,13 @@ int branch_merge_matches(struct branch *, int n, const char *);
  * message is recorded there (if the function does not return NULL, then
  * `err` is not touched).
  */
-const char *branch_get_upstream(struct branch *branch, struct strbuf *err);
+const char *repo_branch_get_upstream(struct repository *repo,
+				     struct branch *branch, struct strbuf *err);
+static inline const char *branch_get_upstream(struct branch *branch,
+					      struct strbuf *err)
+{
+	return repo_branch_get_upstream(the_repository, branch, err);
+}
 
 /**
  * Return the tracking branch that corresponds to the ref we would push to
@@ -347,7 +391,13 @@ const char *branch_get_upstream(struct branch *branch, struct strbuf *err);
  *
  * The return value and `err` conventions match those of `branch_get_upstream`.
  */
-const char *branch_get_push(struct branch *branch, struct strbuf *err);
+const char *repo_branch_get_push(struct repository *repo, struct branch *branch,
+				 struct strbuf *err);
+static inline const char *branch_get_push(struct branch *branch,
+					  struct strbuf *err)
+{
+	return repo_branch_get_push(the_repository, branch, err);
+}
 
 /* Flags to match_refs. */
 enum match_refs_flags {
@@ -366,11 +416,27 @@ enum ahead_behind_flags {
 };
 
 /* Reporting of tracking info */
-int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs,
-		       const char **upstream_name, int for_push,
-		       enum ahead_behind_flags abf);
-int format_tracking_info(struct branch *branch, struct strbuf *sb,
-			 enum ahead_behind_flags abf);
+int repo_stat_tracking_info(struct repository *repo, struct branch *branch,
+			    int *num_ours, int *num_theirs,
+			    const char **upstream_name, int for_push,
+			    enum ahead_behind_flags abf);
+static inline int stat_tracking_info(struct branch *branch, int *num_ours,
+				     int *num_theirs,
+				     const char **upstream_name, int for_push,
+				     enum ahead_behind_flags abf)
+{
+	return repo_stat_tracking_info(the_repository, branch, num_ours,
+				       num_theirs, upstream_name, for_push,
+				       abf);
+}
+
+int repo_format_tracking_info(struct repository *repo, struct branch *branch,
+			      struct strbuf *sb, enum ahead_behind_flags abf);
+static inline int format_tracking_info(struct branch *branch, struct strbuf *sb,
+				       enum ahead_behind_flags abf)
+{
+	return repo_format_tracking_info(the_repository, branch, sb, abf);
+}
 
 struct ref *get_local_heads(void);
 /*
-- 
2.33.0.882.g93a45727a2-goog


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 0/3] remote: replace static variables with struct remote_state
  2021-10-13 19:31 ` [PATCH v2 0/3] remote: replace static variables with struct remote_state Glen Choo
                     ` (2 preceding siblings ...)
  2021-10-13 19:31   ` [PATCH v2 3/3] remote: add struct repository parameter to external functions Glen Choo
@ 2021-10-13 20:11   ` Junio C Hamano
  2021-10-13 20:27     ` Junio C Hamano
  2021-10-13 21:56     ` Glen Choo
  3 siblings, 2 replies; 21+ messages in thread
From: Junio C Hamano @ 2021-10-13 20:11 UTC (permalink / raw)
  To: Glen Choo; +Cc: git

Glen Choo <chooglen@google.com> writes:

> While this concern is valid, I decided to keep this interface for a few
> reasons:
>
> 1. The correct way of calling the function is 'obvious'.
> 2. It is relatively easy to get the contained object (struct branch/remote)
>    and its containing struct repository/remote_state (e.g. we don't pass
>    struct branch or remote through long call chains). For "struct
>    branch", callers usually get the branch from the repo and use it
>    immediately. For "struct remote", we don't use container objects
>    outside of static functions. If you are interested in seeing all of
>    the call sites, you can see a sample commit in [3].
> 3. The separation between container/contained objects allows us to
>    reason better about what the correct interface is. e.g. we might be
>    tempted to include a backpointer from struct branch to struct
>    remote_state so that we can pass around struct branch and be
>    confident that struct branch has all of the information it needs.

I am not following.  None of the above reasons argue for forcing the
functions that take contained object to also take its container.

>    However, I believe that some questions *shouldn't* be answered by
>    just struct branch. The prime example in this series is
>    branch_get_push() - it conceptually answers 'What is the pushremote
>    of this branch', but the behavior we want is closer to 'If
>    configured, give me the pushremote for the branch. Otherwise, give me
>    the default pushremote of the repo.'. This is arguably a relevant
>    detail that should be exposed to callers.

It is a good example why such a function can just take an instance
of branch, and the caller,

 (1) who does care about the fallback, can be assured that the logic
     falls back to the correct repository; and

 (2) who does not care about the fallback and sees it a mere
     implementation detail of "I am on this branch; give me the
     remote to push to", do not have to know what, other than the
     branch object, needs to be passed.

if we explicitly record a branch object which repository it was
taken from.

There may be some other (real) reason where the resistance comes
from, that you may not be telling us, though.  But in what was
described in the message I am responding to, I didn't see much
convincing reason to argue _for_ keeping the contained objects
ignorant of the container and forcing callers to pass both to
functions that use both the container and contained to compute
something.

Thanks.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 1/3] remote: move static variables into per-repository struct
  2021-10-13 19:31   ` [PATCH v2 1/3] remote: move static variables into per-repository struct Glen Choo
@ 2021-10-13 20:21     ` Junio C Hamano
  2021-10-14 17:25       ` Glen Choo
  0 siblings, 1 reply; 21+ messages in thread
From: Junio C Hamano @ 2021-10-13 20:21 UTC (permalink / raw)
  To: Glen Choo; +Cc: git

Glen Choo <chooglen@google.com> writes:

> remote.c does not work with non-the_repository because it stores its
> state as static variables. To support non-the_repository, we can use a
> per-repository struct for the remotes subsystem.
>
> Prepare for this change by defining a struct remote_state that holds
> the remotes subsystem state and move the static variables of remote.c
> into the_repository->remote_state.

That all sounds very sensible, but ...

> diff --git a/remote.h b/remote.h
> index 5a59198252..184d14af3d 100644
> --- a/remote.h
> +++ b/remote.h
> @@ -23,6 +23,40 @@ enum {
>  	REMOTE_BRANCHES
>  };
>  
> +struct rewrite {
> +	const char *base;
> +	size_t baselen;
> +	struct counted_string *instead_of;
> +	int instead_of_nr;
> +	int instead_of_alloc;
> +};
> +struct rewrites {

Missing a blank line between two type decls.

> +	struct rewrite **rewrite;
> +	int rewrite_alloc;
> +	int rewrite_nr;
> +};

It is a bit sad that we still have to keep "struct rewrites"; if we
see how .remotes and .branches are handled, we probably should have
left the <array, alloc, nr> 3-tuple for "struct rewrite" without an
extra layer.  But this step is about moving things around and
wrapping the set of existing static file-scope-global variables into
a structure, so such a restructuring would not belong here.  OK.

> +struct remote_state {
> +	int config_loaded;
> +
> +	struct remote **remotes;
> +	int remotes_alloc;
> +	int remotes_nr;
> +	struct hashmap remotes_hash;
> +
> +	struct branch **branches;
> +	int branches_alloc;
> +	int branches_nr;
> +
> +	struct branch *current_branch;
> +	const char *pushremote_name;
> +
> +	struct rewrites rewrites;
> +	struct rewrites rewrites_push;
> +};
> +void remote_state_clear(struct remote_state *remote_state);

Missing a blank line after "struct" decl.

Thanks.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 2/3] remote: use remote_state parameter internally
  2021-10-13 19:31   ` [PATCH v2 2/3] remote: use remote_state parameter internally Glen Choo
@ 2021-10-13 20:23     ` Junio C Hamano
  0 siblings, 0 replies; 21+ messages in thread
From: Junio C Hamano @ 2021-10-13 20:23 UTC (permalink / raw)
  To: Glen Choo; +Cc: git

Glen Choo <chooglen@google.com> writes:

> In internal-facing functions, replace the_repository->remote_state with
> a struct remote_state parameter, but do not change external-facing
> functions.

OK.  Quite nice.


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 3/3] remote: add struct repository parameter to external functions
  2021-10-13 19:31   ` [PATCH v2 3/3] remote: add struct repository parameter to external functions Glen Choo
@ 2021-10-13 20:24     ` Junio C Hamano
  0 siblings, 0 replies; 21+ messages in thread
From: Junio C Hamano @ 2021-10-13 20:24 UTC (permalink / raw)
  To: Glen Choo; +Cc: git

Glen Choo <chooglen@google.com> writes:

> Finish plumbing remote_state by adding a struct repository
> parameter to repo_* functions. While this removes all references to
> the_repository->remote_state, certain functions still use the_repository
> to parse oids.

OK.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 0/3] remote: replace static variables with struct remote_state
  2021-10-13 20:11   ` [PATCH v2 0/3] remote: replace static variables with struct remote_state Junio C Hamano
@ 2021-10-13 20:27     ` Junio C Hamano
  2021-10-13 22:00       ` Glen Choo
  2021-10-13 21:56     ` Glen Choo
  1 sibling, 1 reply; 21+ messages in thread
From: Junio C Hamano @ 2021-10-13 20:27 UTC (permalink / raw)
  To: Glen Choo; +Cc: git

Junio C Hamano <gitster@pobox.com> writes:

> There may be some other (real) reason where the resistance comes
> from, that you may not be telling us, though.  But in what was
> described in the message I am responding to, I didn't see much
> convincing reason to argue _for_ keeping the contained objects
> ignorant of the container and forcing callers to pass both to
> functions that use both the container and contained to compute
> something.

I am not you, so I can only speculate, but the real reason _could_
be that it makes it simpler to formulate steps 2 and 3 mechanically.
After adding "repo" parameter to a function that used to take, say,
a "branch", in step 3, a future clean-up series could add a .repo
member to branch objects and remove the "repo" parameter from such
function.

I think that approach would make more work to get to the final
state, though.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 0/3] remote: replace static variables with struct remote_state
  2021-10-13 20:11   ` [PATCH v2 0/3] remote: replace static variables with struct remote_state Junio C Hamano
  2021-10-13 20:27     ` Junio C Hamano
@ 2021-10-13 21:56     ` Glen Choo
  2021-10-13 23:37       ` Junio C Hamano
  1 sibling, 1 reply; 21+ messages in thread
From: Glen Choo @ 2021-10-13 21:56 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

Junio C Hamano <gitster@pobox.com> writes:

>>    However, I believe that some questions *shouldn't* be answered by
>>    just struct branch. The prime example in this series is
>>    branch_get_push() - it conceptually answers 'What is the pushremote
>>    of this branch', but the behavior we want is closer to 'If
>>    configured, give me the pushremote for the branch. Otherwise, give me
>>    the default pushremote of the repo.'. This is arguably a relevant
>>    detail that should be exposed to callers.
>
> It is a good example why such a function can just take an instance
> of branch, and the caller,
>
>  (1) who does care about the fallback, can be assured that the logic
>      falls back to the correct repository; and
>
>  (2) who does not care about the fallback and sees it a mere
>      implementation detail of "I am on this branch; give me the
>      remote to push to", do not have to know what, other than the
>      branch object, needs to be passed.
>
> if we explicitly record a branch object which repository it was
> taken from.
>
> There may be some other (real) reason where the resistance comes
> from, that you may not be telling us, though.  But in what was
> described in the message I am responding to, I didn't see much
> convincing reason to argue _for_ keeping the contained objects
> ignorant of the container and forcing callers to pass both to
> functions that use both the container and contained to compute
> something.

My primary line of thinking is that adding the backpointer from struct
branch to struct repository adds "unnecessary" coupling between the two
objects, which causes the more concrete problems of:

* Inconsistency between other functions that use struct repository as a
  "context object". We now find ourselves having to justify why branch
  functions can bypass the context object using a special pointer,
  whereas other functions and structs (say, struct object) cannot.
* Interface misuse, where callers can now dereference branch->repository
  even though it is meant to be internal. This is also a possible source
  of inconsistency.
* Extra memory consumption.

A counterpoint to what I said is [1], where Jonathan eventually added
the backpointer from struct ref_store to struct repository, which does
give the nice benefits of referential integrity and abstraction that you
cited. However in most of that series, struct ref_store is meant to be
the context object, but due to poor internal abstraction, ref_store ends
up depending on repository in some form or another and thus the
backpointer is more defensible (as opposed to passing two context
objects). I do not think we are in the same position with struct branch;
struct branch seems sufficiently separated to me.

I'm not opposed to adding a backpointer if it helps us get to a good
final state, but I currently suspect that the final state still involves
passing around struct repository as a context object.

[1] https://lore.kernel.org/git/xmqqlf3gib0p.fsf@gitster.g/

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 0/3] remote: replace static variables with struct remote_state
  2021-10-13 20:27     ` Junio C Hamano
@ 2021-10-13 22:00       ` Glen Choo
  0 siblings, 0 replies; 21+ messages in thread
From: Glen Choo @ 2021-10-13 22:00 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

Junio C Hamano <gitster@pobox.com> writes:

> I am not you, so I can only speculate, but the real reason _could_
> be that it makes it simpler to formulate steps 2 and 3 mechanically.

This is somewhat true, however...

> I think that approach would make more work to get to the final
> state, though.

I found that if I were to add the backpointer, it would have been better
to add it early and avoid this extra work that you mention here :)

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 0/3] remote: replace static variables with struct remote_state
  2021-10-13 21:56     ` Glen Choo
@ 2021-10-13 23:37       ` Junio C Hamano
  2021-10-14  1:25         ` Glen Choo
  0 siblings, 1 reply; 21+ messages in thread
From: Junio C Hamano @ 2021-10-13 23:37 UTC (permalink / raw)
  To: Glen Choo; +Cc: git

Glen Choo <chooglen@google.com> writes:

> My primary line of thinking is that adding the backpointer from struct
> branch to struct repository adds "unnecessary" coupling between the two
> objects,...

meaning, that the paragraphs below will explain why the reference is
unnecessary?

> ...which causes the more concrete problems of:

> * Inconsistency between other functions that use struct repository as a
>   "context object". We now find ourselves having to justify why branch
>   functions can bypass the context object using a special pointer,
>   whereas other functions and structs (say, struct object) cannot.

Given that the API has been evolved over time from a "we deal only
with a single repository and everybody else can just fork into a
separate repository" to "now this function can work in the specified
repository", it is understandable that some still do take repository
as a separate parameter, even though the original counterpart that
did not take a repository pointer took an object that clearly can
belong to only one repository at a time.  They would need to be
cleaned up, and they do not make a good excuse to add more of them,
when we know we are dealing with objects that belong only to one
repository, like the "remote.c" contents we have been discussing.

IOW, we need to start somewhere to clean things up, so either we
teach multi-repository support to remote.c from day one, or we pass
a repository as a separate and redundant parameter, with the
intention to later clean up.  I suspect that this piece is isolated
enough that it is simpler to use as a starting point, and then the
callers into remote.c API can then be cleaned up, gradually extending
the circle.

The original counerpart may not have been about "the other object"
to begin with (e.g. "we have this string (not an object); resolve it
to an object name in the context of the given repository"), in which
case they are fine as they are, but I think most of the contents of
"remote.c" do not fall into this category.

> * Interface misuse, where callers can now dereference branch->repository
>   even though it is meant to be internal. This is also a possible source
>   of inconsistency.

I do not think it is meant to be internal to begin with.  If we
declare, after careful consideration, that an in-core branch object
given by remote.c API always belongs to one and only one repository,
then following the branch->repository pointer should be the
documented and kosher way to find out the repository by anybody who
is given the branch object.  A function that takes repo and branch
to pretend as if it can work with an inconsistent pair is an
invidation for the interface misuse, but I do not think it is a
misuse of the API for a codepath that obtained a branch instance
inspecting what repository it came from.

A function that takes a pair of remote and a branch object for
whatever reason may want to have an assert() that makes sure they
came from the same repository, by the way.

Also, some of the functions involved in this topic may need to say
"I have this string, please resolve it as an object name in the
context of this repository", and call a function that takes a
repository as "context".  The repository they use must be given by
their caller somehow---is there any valid case where it must be
different from the other parameter (i.e. contained object) they are
dealing with?  I do not think so.  So such a function would have to
say "resolve this string in the context of the repository
branch->repo" because the function was given an in-core branch
instance.

> * Extra memory consumption.

This is true.  It however is the only valid excuse I can fully agree
with for being hesitant to have "where did I come from" information.
We do grow each in-core branch and remote instance by an 8-byte
pointer (but we reduce the stackframe consumption by 8-byte along
each step in the callchain as we do not have to pass a repository as
a separate pointer parameter).

> objects). I do not think we are in the same position with struct branch;
> struct branch seems sufficiently separated to me.

Sufficiently separated in the sense that you take a branch from the
parent repository and throwing it at a function with a pointer to an
in-core repository for a submodule would make some sort of sense?  

Sorry, but I do not follow this part of your argument, at all.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 0/3] remote: replace static variables with struct remote_state
  2021-10-13 23:37       ` Junio C Hamano
@ 2021-10-14  1:25         ` Glen Choo
  0 siblings, 0 replies; 21+ messages in thread
From: Glen Choo @ 2021-10-14  1:25 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

Junio C Hamano <gitster@pobox.com> writes:

> IOW, we need to start somewhere to clean things up, so either we
> teach multi-repository support to remote.c from day one, or we pass
> a repository as a separate and redundant parameter, with the
> intention to later clean up.

I authored this topic with the assumption that passing a 'redundant'
context object was the path forward, not something to clean up. I think
this explains most of the divergence in opinion.

> ...I suspect that this piece is isolated
> enough that it is simpler to use as a starting point, and then the
> callers into remote.c API can then be cleaned up, gradually extending
> the circle.

Agree that this piece is a good place to start such work.

> I do not think it is meant to be internal to begin with.  If we
> declare, after careful consideration, that an in-core branch object
> given by remote.c API always belongs to one and only one repository,
> then following the branch->repository pointer should be the
> documented and kosher way to find out the repository by anybody who
> is given the branch object.

In the specific case of an in-core object that can refer only to one
repository, I think this is a fairly straightforward way of maintaining
referential integrity. However I don't think that the approach of
"contained object pointing to container" generalizes well - if I were
passed a struct remote_state and I needed to access its repository
later, is the ideal interface a backpointer, or is there a deeper
problem with how things are structured?

IOW, whether or not we should use the backpointer seems to depend on the
specifics of each use case. It might be productive to decide on
guidelines, especially with regards to repository.

> ...A function that takes repo and branch
> to pretend as if it can work with an inconsistent pair is an
> invidation for the interface misuse.

Fair.

>> objects). I do not think we are in the same position with struct branch;
>> struct branch seems sufficiently separated to me.
>
> Sufficiently separated in the sense that you take a branch from the
> parent repository and throwing it at a function with a pointer to an
> in-core repository for a submodule would make some sort of sense?  

I meant "sufficiently separated" in the sense that a struct branch is
meaningful to callers, even in the absence of a repository. Even in the
case where we *might* care about the repository e.g. getting a default
for remote_for_branch(), there are callers that make do with just
branch->remote_name.

This was meant to contrast to ref_store, which relies on its specific
repository for much of its internals and is difficult to separate. I am
worried about the knock-on effects of taking a _relatively_ clean
abstraction layer in struct branch and tying it back to the repository
layer.

That said, you have rightly noted that we should not pretend that a
branch from repo A can be thrown together with a pointer to repo B in
any kind of meaningful way (or at least, not yet). A backpointer can
prevent this.

I'm not fully convinced either way so I'll take time to mull over it; I
would also like to start on the right foot with this subsystem.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 1/3] remote: move static variables into per-repository struct
  2021-10-13 20:21     ` Junio C Hamano
@ 2021-10-14 17:25       ` Glen Choo
  2021-10-14 18:33         ` Junio C Hamano
  0 siblings, 1 reply; 21+ messages in thread
From: Glen Choo @ 2021-10-14 17:25 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

Junio C Hamano <gitster@pobox.com> writes:

>> +struct rewrite {
>> +	const char *base;
>> +	size_t baselen;
>> +	struct counted_string *instead_of;
>> +	int instead_of_nr;
>> +	int instead_of_alloc;
>> +};
>> +struct rewrites {
>
> Missing a blank line between two type decls.

Thanks!
>
>> +	struct rewrite **rewrite;
>> +	int rewrite_alloc;
>> +	int rewrite_nr;
>> +};
>
> It is a bit sad that we still have to keep "struct rewrites"; if we
> see how .remotes and .branches are handled, we probably should have
> left the <array, alloc, nr> 3-tuple for "struct rewrite" without an
> extra layer.

I considered doing this, but I found it non-trivial because some
functions use "struct rewrites" without consideration for whether it is
remote_state->rewrites or remote_state->rewrites_push i.e. the "struct
rewrites" abstraction is doing its job.

> ...such a restructuring would not belong here.  OK.

By "here", I assume you mean "the restructuring doesn't belong in
this topic" vs "the restructuring belongs in this topic but not this
patch".

>> +struct remote_state {
>> +	int config_loaded;
>> +
>> +	struct remote **remotes;
>> +	int remotes_alloc;
>> +	int remotes_nr;
>> +	struct hashmap remotes_hash;
>> +
>> +	struct branch **branches;
>> +	int branches_alloc;
>> +	int branches_nr;
>> +
>> +	struct branch *current_branch;
>> +	const char *pushremote_name;
>> +
>> +	struct rewrites rewrites;
>> +	struct rewrites rewrites_push;
>> +};
>> +void remote_state_clear(struct remote_state *remote_state);
>
> Missing a blank line after "struct" decl.

Thanks!

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 1/3] remote: move static variables into per-repository struct
  2021-10-14 17:25       ` Glen Choo
@ 2021-10-14 18:33         ` Junio C Hamano
  0 siblings, 0 replies; 21+ messages in thread
From: Junio C Hamano @ 2021-10-14 18:33 UTC (permalink / raw)
  To: Glen Choo; +Cc: git

Glen Choo <chooglen@google.com> writes:

> By "here", I assume you mean "the restructuring doesn't belong in
> this topic" vs "the restructuring belongs in this topic but not this
> patch".

Hmph, it is unclear which one you assumed ;-), but anyway I do not
think it would help to make such a clean-up as a part of this
series.

Adding a pointer to the container to contained types, if done from
the beginning without disrupting the function signature (i.e. a
function that takes a branch instance can now work on a branch that
was taken from any repository, by simply following branch->repo and
using it as a parameter to repo_foo_blah() calls it would make
instead of existing foo_blah() calls), may help reduce the size of
the series greatly, because you wouldn't have to touch may existing
callers.  That would make a great part of this topic.

But if done as a post clean-up step (i.e. first we sprinkle "struct
repo *" pointer as an additional parameter all over the functions
that take an object that clearly belongs to a repository, and then
later shrink the API to remove the extra pointer parameter by
teaching the contained object which repository it belongs to), it
would probably not a good idea to have it as part of this series.
We'd instead stop at "we add a new parameter to functions to tell
in which repository it now should work" in the topic itself.

The "post clean-up" series would be like "the previous series made
it 'working', now let's clean-up the API" follow-up that is done
separately.  One benefit of going that route would be that it would
allow us notice there are cases where contained object may have to
be used in the context of the container that is different from the
container it belongs to, before locking ourselves out of the
possibility by limiting the API.  I personally do not think we'd
reap that benefit as I do not think of a case where it makes sense
to take a branch or a remote from a repository and use it in the
context of another repository (hence my suggestion to teach objects
the repository they belong to), but if others think it is easier to
proceed, then...

Thanks.


^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2021-10-14 18:33 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-07 19:07 [PATCH 0/2] remote: replace static variables with struct remote_state Glen Choo via GitGitGadget
2021-10-07 19:07 ` [PATCH 1/2] remote: move static variables into struct Glen Choo via GitGitGadget
2021-10-07 23:36   ` Junio C Hamano
2021-10-07 19:07 ` [PATCH 2/2] remote: add remote_state to struct repository Glen Choo via GitGitGadget
2021-10-07 23:39   ` Junio C Hamano
2021-10-08 17:30     ` Glen Choo
2021-10-13 19:31 ` [PATCH v2 0/3] remote: replace static variables with struct remote_state Glen Choo
2021-10-13 19:31   ` [PATCH v2 1/3] remote: move static variables into per-repository struct Glen Choo
2021-10-13 20:21     ` Junio C Hamano
2021-10-14 17:25       ` Glen Choo
2021-10-14 18:33         ` Junio C Hamano
2021-10-13 19:31   ` [PATCH v2 2/3] remote: use remote_state parameter internally Glen Choo
2021-10-13 20:23     ` Junio C Hamano
2021-10-13 19:31   ` [PATCH v2 3/3] remote: add struct repository parameter to external functions Glen Choo
2021-10-13 20:24     ` Junio C Hamano
2021-10-13 20:11   ` [PATCH v2 0/3] remote: replace static variables with struct remote_state Junio C Hamano
2021-10-13 20:27     ` Junio C Hamano
2021-10-13 22:00       ` Glen Choo
2021-10-13 21:56     ` Glen Choo
2021-10-13 23:37       ` Junio C Hamano
2021-10-14  1:25         ` Glen Choo

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).