[PATCH 0/2] remote: replace static variables with struct remote_state

[PATCH 0/2] remote: replace static variables with struct remote_state

[Glen Choo via GitGitGadget]

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

[PATCH 1/2] remote: move static variables into struct

[Glen Choo via GitGitGadget]

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

Re: [PATCH 1/2] remote: move static variables into struct

[Junio C Hamano]

"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.

[PATCH 2/2] remote: add remote_state to struct repository

[Glen Choo via GitGitGadget]

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

Re: [PATCH 2/2] remote: add remote_state to struct repository

[Junio C Hamano]

"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.

Re: [PATCH 2/2] remote: add remote_state to struct repository

[Glen Choo]

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.

[PATCH v2 0/3] remote: replace static variables with struct remote_state

[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

[PATCH v2 1/3] remote: move static variables into per-repository struct

[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

Re: [PATCH v2 1/3] remote: move static variables into per-repository struct

[Junio C Hamano]

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.

Re: [PATCH v2 1/3] remote: move static variables into per-repository struct

[Glen Choo]

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!

Re: [PATCH v2 1/3] remote: move static variables into per-repository struct

[Junio C Hamano]

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.

[PATCH v2 2/3] remote: use remote_state parameter internally

[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

Re: [PATCH v2 2/3] remote: use remote_state parameter internally

[Junio C Hamano]

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.

[PATCH v2 3/3] remote: add struct repository parameter to external functions

[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

Re: [PATCH v2 3/3] remote: add struct repository parameter to external functions

[Junio C Hamano]

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.

Re: [PATCH v2 0/3] remote: replace static variables with struct remote_state

[Junio C Hamano]

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.

Re: [PATCH v2 0/3] remote: replace static variables with struct remote_state

[Junio C Hamano]

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.

Re: [PATCH v2 0/3] remote: replace static variables with struct remote_state

[Glen Choo]

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 :)

Re: [PATCH v2 0/3] remote: replace static variables with struct remote_state

[Glen Choo]

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/

Re: [PATCH v2 0/3] remote: replace static variables with struct remote_state

[Junio C Hamano]

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.

Re: [PATCH v2 0/3] remote: replace static variables with struct remote_state

[Glen Choo]

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.

[PATCH v3 0/4] remote: replace static variables with struct remote_state

[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.

v3 tightens up the interface issues discussed previously ([2] and other v2
discussion) by adding a "struct remote_state" backpointer to "struct branch" and
"struct remote". The most notable improvement is that we no longer accept
"struct {remote_state,repository}" and "struct {branch,remote}" parameters at
the same time, thus it is impossible to pass the wrong (container, contained)
object pair. This is especially useful for branch_get_push_1(), where we
take a "struct branch" and need to get a related "struct remote".

The intention is that this backpointer is only meant to be used by the remotes
subsystem; it is meant to be an opaque implementation detail that no other
callers should touch.

I attempted the initial suggestion to include a backpointer from branch->repo
instead of branch->remote_state and uploaded it to a branch in my own fork [3].
However, this seems to be the wrong abstraction to me. I am not convinced that
the contained structs (branch, remote) need to (nor will ever need to) interact
with parts of the repository outside of remote_state. A symptom of this is that
internal code ends up using "struct repository" parameters instead of "struct
remote_state" only to use repo->remote_state repeatedly, violating some of the
nice layering that we have set up.

There is also a small YAGNI benefit of using a backpointer to the remote_state
instead of repository. If we eventually decide that branches should
point to their repository, we can make the change with
s/remote_state/repo->remote_state, even if external callers start to rely on
branch->remote_state (even though I don't think they shouldn't rely on it!).
This property does not hold for branch->repo because external callers might be
using members other than repository->remote_state.

[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/git/git/commit/9527d9106ff5a41530e7bfa50a316af296144c99

Changes since v2:
* Add .remote_state to struct branch and struct remote, changing the
  implementation appropriately.
* In patch 2, properly consider the initialized state of remote_state.
  In v2, I forgot to convert a static inside read_config() into a
  private member of struct remote_state. Fix this.
* In a new patch 3, add helper methods that get a remote via
  remote_state and the remote name.
* Move read_config(repo) calls to the external facing-functions. This keeps
  "struct repository" away from the remote.c internals.

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.

Glen Choo (4):
  remote: move static variables into per-repository struct
  remote: use remote_state parameter internally
  remote: remove the_repository->remote_state from static methods
  remote: add struct repository parameter to external functions

 remote.c     | 303 +++++++++++++++++++++++++++++++--------------------
 remote.h     |  72 +++++++++++-
 repository.c |   8 ++
 repository.h |   4 +
 4 files changed, 262 insertions(+), 125 deletions(-)

Range-diff against v2:
1:  6972ba4dcb ! 1:  1f712c22b4 remote: move static variables into per-repository struct
    @@ remote.h: enum {
     +	int instead_of_nr;
     +	int instead_of_alloc;
     +};
    ++
     +struct rewrites {
     +	struct rewrite **rewrite;
     +	int rewrite_alloc;
    @@ remote.h: enum {
     +};
     +
     +struct remote_state {
    -+	int config_loaded;
    -+
     +	struct remote **remotes;
     +	int remotes_alloc;
     +	int remotes_nr;
    @@ remote.h: enum {
     +	struct rewrites rewrites;
     +	struct rewrites rewrites_push;
     +};
    ++
     +void remote_state_clear(struct remote_state *remote_state);
     +struct remote_state *remote_state_new(void);
     +
2:  71b1da4389 ! 2:  467247fa9c remote: use remote_state parameter internally
    @@ Metadata
      ## Commit message ##
         remote: use remote_state parameter internally
     
    -    In internal-facing functions, replace the_repository->remote_state with
    -    a struct remote_state parameter, but do not change external-facing
    -    functions.
    +    Introduce a struct remote_state member to structs that need to
    +    'remember' their remote_state. Without changing external-facing
    +    functions, replace the_repository->remote_state internally by using the
    +    remote_state member where it is applicable i.e. when a function accepts
    +    a struct that depends on the remote_state. If it is not applicable, add
    +    a struct remote_state parameter instead.
     
    -    As a result, most static functions no longer reference
    +    As a result, external-facing functions are still tied to the_repository,
    +    but 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().
    @@ Commit message
     
      ## remote.c ##
     @@ remote.c: 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)
    + static void add_pushurl_alias(struct remote *remote, const char *url)
      {
    --	const char *pushurl =
    + 	const char *pushurl =
     -		alias_url(url, &the_repository->remote_state->rewrites_push);
    -+	const char *pushurl = alias_url(url, &remote_state->rewrites_push);
    ++		alias_url(url, &remote->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)
    + static void add_url_alias(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);
    ++	add_url(remote, alias_url(url, &remote->remote_state->rewrites));
    + 	add_pushurl_alias(remote, url);
      }
      
    - struct remotes_hash_key {
     @@ remote.c: static int remotes_hash_cmp(const void *unused_cmp_data,
      		return strcmp(a->name, b->name);
      }
    @@ remote.c: static struct remote *make_remote(const char *name, int len)
      		return container_of(e, struct remote, ent);
      
     @@ remote.c: static struct remote *make_remote(const char *name, int len)
    + 	ret->prune = -1;  /* unspecified */
    + 	ret->prune_tags = -1;  /* unspecified */
    + 	ret->name = xstrndup(name, len);
    ++	ret->remote_state = remote_state;
      	refspec_init(&ret->push, REFSPEC_PUSH);
      	refspec_init(&ret->fetch, REFSPEC_FETCH);
      
    @@ remote.c: static void add_merge(struct branch *branch, const char *name)
     +	remote_state->branches[remote_state->branches_nr++] = ret;
      	ret->name = xstrndup(name, len);
      	ret->refname = xstrfmt("refs/heads/%s", ret->name);
    ++	ret->remote_state = remote_state;
      
    -@@ remote.c: 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");
    -@@ remote.c: 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))
    -@@ remote.c: static void read_remotes_file(struct remote *remote)
    - 	fclose(f);
    + 	return ret;
      }
    - 
    --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;
    -@@ remote.c: 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);
    - 
     @@ remote.c: static int handle_config(const char *key, const char *value, void *cb)
      	const char *subkey;
      	struct remote *remote;
    @@ remote.c: static int handle_config(const char *key, const char *value, void *cb)
     -				->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],
     +					remote_state->remotes[i]->url[j]);
     +			remote_state->remotes[i]->url[j] =
     +				alias_url(remote_state->remotes[i]->url[j],
    @@ remote.c: static int handle_config(const char *key, const char *value, void *cb)
      }
      
     -static void read_config(void)
    -+static void read_config(struct remote_state *remote_state)
    ++static void read_config(struct repository *repo)
      {
    - 	static int loaded;
    +-	static int loaded;
      	int flag;
    -@@ remote.c: static void read_config(void)
    + 
    +-	if (loaded)
    ++	if (repo->remote_state->initialized)
      		return;
    - 	loaded = 1;
    +-	loaded = 1;
    ++	repo->remote_state->initialized = 1;
      
     -	the_repository->remote_state->current_branch = NULL;
    -+	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);
    +-		const char *head_ref = resolve_ref_unsafe("HEAD", 0, NULL, &flag);
    ++		const char *head_ref = refs_resolve_ref_unsafe(
    ++			get_main_ref_store(repo), "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));
    ++			repo->remote_state->current_branch = make_branch(
    ++				repo->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);
    ++	repo_config(repo, handle_config, repo->remote_state);
    ++	alias_all_urls(repo->remote_state);
      }
      
      static int valid_remote_nick(const char *name)
    +@@ remote.c: const char *pushremote_for_branch(struct branch *branch, int *explicit)
    + 			*explicit = 1;
    + 		return branch->pushremote_name;
    + 	}
    +-	if (the_repository->remote_state->pushremote_name) {
    ++	if (branch->remote_state->pushremote_name) {
    + 		if (explicit)
    + 			*explicit = 1;
    +-		return the_repository->remote_state->pushremote_name;
    ++		return branch->remote_state->pushremote_name;
    + 	}
    + 	return remote_for_branch(branch, explicit);
    + }
     @@ remote.c: static struct remote *remote_get_1(const char *name,
      	struct remote *ret;
      	int name_given = 0;
      
     -	read_config();
    -+	read_config(the_repository->remote_state);
    ++	read_config(the_repository);
      
      	if (name)
      		name_given = 1;
    @@ remote.c: static struct remote *remote_get_1(const char *name,
     +	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;
    + 			read_remotes_file(ret);
     @@ remote.c: 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);
    ++	read_config(the_repository);
      	for (i = 0; i < the_repository->remote_state->remotes_nr && !result;
      	     i++) {
      		struct remote *remote =
    @@ remote.c: struct branch *branch_get(const char *name)
      	struct branch *ret;
      
     -	read_config();
    -+	read_config(the_repository->remote_state);
    ++	read_config(the_repository);
      	if (!name || !*name || !strcmp(name, "HEAD"))
      		ret = the_repository->remote_state->current_branch;
      	else
    @@ remote.c: struct branch *branch_get(const char *name)
      	set_merge(ret);
      	return ret;
      }
    +
    + ## remote.h ##
    +@@ remote.h: struct remote_state {
    + 
    + 	struct rewrites rewrites;
    + 	struct rewrites rewrites_push;
    ++
    ++	int initialized;
    + };
    + 
    + void remote_state_clear(struct remote_state *remote_state);
    +@@ remote.h: struct remote {
    + 
    + 	/* The method used for authenticating against `http_proxy`. */
    + 	char *http_proxy_authmethod;
    ++
    ++	/** The remote_state that this remote belongs to. This is only meant to
    ++	 * be used by remote_* functions. */
    ++	struct remote_state *remote_state;
    + };
    + 
    + /**
    +@@ remote.h: struct branch {
    + 	int merge_alloc;
    + 
    + 	const char *push_tracking_ref;
    ++
    ++	/** The remote_state that this branch belongs to. This is only meant to
    ++	 * be used by branch_* functions. */
    ++	struct remote_state *remote_state;
    + };
    + 
    + struct branch *branch_get(const char *name);
3:  ff12771f06 < -:  ---------- remote: add struct repository parameter to external functions
-:  ---------- > 3:  10fbb84496 remote: remove the_repository->remote_state from static methods
-:  ---------- > 4:  4013f74fd9 remote: add struct repository parameter to external functions
-- 
2.33.GIT

[PATCH v3 1/4] remote: move static variables into per-repository struct

[Glen Choo]

remote.c does not works 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..d21c035f1b 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 {
+	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.GIT

[PATCH v3 2/4] remote: use remote_state parameter internally

[Glen Choo]

Introduce a struct remote_state member to structs that need to
'remember' their remote_state. Without changing external-facing
functions, replace the_repository->remote_state internally by using the
remote_state member where it is applicable i.e. when a function accepts
a struct that depends on the remote_state. If it is not applicable, add
a struct remote_state parameter instead.

As a result, external-facing functions are still tied to the_repository,
but 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 | 153 ++++++++++++++++++++++++++-----------------------------
 remote.h |  10 ++++
 2 files changed, 81 insertions(+), 82 deletions(-)

diff --git a/remote.c b/remote.c
index 29c29fcc3b..e3ca44f735 100644
--- a/remote.c
+++ b/remote.c
@@ -68,15 +68,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, &the_repository->remote_state->rewrites_push);
+		alias_url(url, &remote->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, &the_repository->remote_state->rewrites));
+	add_url(remote, alias_url(url, &remote->remote_state->rewrites));
 	add_pushurl_alias(remote, url);
 }
 
@@ -102,14 +101,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 +122,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);
 
@@ -132,18 +135,16 @@ static struct remote *make_remote(const char *name, int len)
 	ret->prune = -1;  /* unspecified */
 	ret->prune_tags = -1;  /* unspecified */
 	ret->name = xstrndup(name, len);
+	ret->remote_state = remote_state;
 	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,27 +178,25 @@ 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);
+	ret->remote_state = remote_state;
 
 	return ret;
 }
@@ -313,10 +312,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 +336,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 +353,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 +364,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,61 +434,51 @@ 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->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 repository *repo)
 {
-	static int loaded;
 	int flag;
 
-	if (loaded)
+	if (repo->remote_state->initialized)
 		return;
-	loaded = 1;
+	repo->remote_state->initialized = 1;
 
-	the_repository->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);
+		const char *head_ref = refs_resolve_ref_unsafe(
+			get_main_ref_store(repo), "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));
+			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)
@@ -524,10 +512,10 @@ const char *pushremote_for_branch(struct branch *branch, int *explicit)
 			*explicit = 1;
 		return branch->pushremote_name;
 	}
-	if (the_repository->remote_state->pushremote_name) {
+	if (branch->remote_state->pushremote_name) {
 		if (explicit)
 			*explicit = 1;
-		return the_repository->remote_state->pushremote_name;
+		return branch->remote_state->pushremote_name;
 	}
 	return remote_for_branch(branch, explicit);
 }
@@ -560,7 +548,7 @@ static struct remote *remote_get_1(const char *name,
 	struct remote *ret;
 	int name_given = 0;
 
-	read_config();
+	read_config(the_repository);
 
 	if (name)
 		name_given = 1;
@@ -568,7 +556,7 @@ 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);
@@ -604,7 +592,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);
 	for (i = 0; i < the_repository->remote_state->remotes_nr && !result;
 	     i++) {
 		struct remote *remote =
@@ -1717,11 +1705,12 @@ struct branch *branch_get(const char *name)
 {
 	struct branch *ret;
 
-	read_config();
+	read_config(the_repository);
 	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;
 }
diff --git a/remote.h b/remote.h
index d21c035f1b..d268a92ebb 100644
--- a/remote.h
+++ b/remote.h
@@ -52,6 +52,8 @@ struct remote_state {
 
 	struct rewrites rewrites;
 	struct rewrites rewrites_push;
+
+	int initialized;
 };
 
 void remote_state_clear(struct remote_state *remote_state);
@@ -110,6 +112,10 @@ struct remote {
 
 	/* The method used for authenticating against `http_proxy`. */
 	char *http_proxy_authmethod;
+
+	/** The remote_state that this remote belongs to. This is only meant to
+	 * be used by remote_* functions. */
+	struct remote_state *remote_state;
 };
 
 /**
@@ -318,6 +324,10 @@ struct branch {
 	int merge_alloc;
 
 	const char *push_tracking_ref;
+
+	/** The remote_state that this branch belongs to. This is only meant to
+	 * be used by branch_* functions. */
+	struct remote_state *remote_state;
 };
 
 struct branch *branch_get(const char *name);
-- 
2.33.GIT

Re: [PATCH v3 2/4] remote: use remote_state parameter internally

[Junio C Hamano]

Glen Choo <chooglen@google.com> writes:

> Introduce a struct remote_state member to structs that need to
> 'remember' their remote_state. Without changing external-facing
> functions, replace the_repository->remote_state internally by using the
> remote_state member where it is applicable i.e. when a function accepts
> a struct that depends on the remote_state. If it is not applicable, add
> a struct remote_state parameter instead.
>
> As a result, external-facing functions are still tied to the_repository,
> but 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 | 153 ++++++++++++++++++++++++++-----------------------------
>  remote.h |  10 ++++
>  2 files changed, 81 insertions(+), 82 deletions(-)

This made my "git push" to k.org and other places over ssh segfault
when their tip and what I am attempting to push are identical.  I
haven't spent more time than just to bisect the history down to
identify this commit as the possible culprit.

Re: [PATCH v3 2/4] remote: use remote_state parameter internally

[Junio C Hamano]

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

> This made my "git push" to k.org and other places over ssh segfault
> when their tip and what I am attempting to push are identical.  I
> haven't spent more time than just to bisect the history down to
> identify this commit as the possible culprit.

(gdb) bt
#0  0x000055555579a785 in pushremote_for_branch (branch=0x0, explicit=0x7fffffffcf84)
    at remote.c:564
#1  0x000055555579a5c2 in remotes_remote_get_1 (remote_state=0x5555559782a0, name=0x0,
    get_default=0x55555579a742 <pushremote_for_branch>) at remote.c:518
#2  0x000055555579a6d0 in remotes_pushremote_get (remote_state=0x5555559782a0, name=0x0)
    at remote.c:542
#3  0x000055555579a740 in repo_pushremote_get (repo=0x555555974b80 <the_repo>, name=0x0)
    at remote.c:554
#4  0x000055555560aa9d in pushremote_get (name=0x0) at ./remote.h:135
#5  0x000055555560c5ce in cmd_push (argc=0, argv=0x7fffffffdc70, prefix=0x0)
    at builtin/push.c:611
#6  0x000055555557396a in run_builtin (p=0x555555941f78 <commands+2136>, argc=3,
    argv=0x7fffffffdc70) at git.c:461
#7  0x0000555555573d79 in handle_builtin (argc=3, argv=0x7fffffffdc70) at git.c:713
#8  0x0000555555573fe6 in run_argv (argcp=0x7fffffffdafc, argv=0x7fffffffdaf0) at git.c:780
#9  0x000055555557448f in cmd_main (argc=3, argv=0x7fffffffdc70) at git.c:911
#10 0x000055555565b2ae in main (argc=6, argv=0x7fffffffdc58) at common-main.c:52

The direct culprit is this part:

    const char *pushremote_for_branch(struct branch *branch, int *explicit)
    {
            if (branch && branch->pushremote_name) {
                    if (explicit)
                            *explicit = 1;
                    return branch->pushremote_name;
            }
            if (branch->remote_state->pushremote_name) {

where the second if() statement used to check "pushremote_name", but
now unconditionally dereferences "branch".

The caller is remote_get_1(); this funciton was called with
"current_branch", which can be NULL until you have a repository and
you've called read_config(), but otherwise shouldn't be.

I think somebody is not setting up the remote_state correctly?  When
the user wants to just use the repository-wide pushremote, not
having the current_branch would not matter, but if the pushremote
for the current branch is different from the repository-wide one,
the code would silently push to a wrong remote.

In any case, any public facing entry point, like pushremote_get()
that is directly called from cmd_push() with just a name, should
auto vivify an instance of struct remote_state and populate its
members as needed, I think, and in this particular case, I suspect
that it forgets to initialize the current_branch and other members
by calling read_config(), just like other entry points like
repo_remote_get() do.

Re: [PATCH v3 2/4] remote: use remote_state parameter internally

[Junio C Hamano]

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

> In any case, any public facing entry point, like pushremote_get()
> that is directly called from cmd_push() with just a name, should
> auto vivify an instance of struct remote_state and populate its
> members as needed, I think, and in this particular case, I suspect
> that it forgets to initialize the current_branch and other members
> by calling read_config(), just like other entry points like
> repo_remote_get() do.

I give up (for the day at least).  I am not quite sure where things
are going wrong.

There are a few bare repositories (that hold preformatted
documentation trees) next to my primary working tree, and I do

	for topic in htmldocs manpages
	do
		printf "%s: " "$topic"
		( cd ../git-$topic.git && git push "$@") || exit
	done

where "$@" is often nothing, and the backtrace I showed was from
such a push that uses "no remote specified?  push to the origin".
The configuration in these bare repositories do not have anything
particularly interesting.  It has three URLs, and pushes the same to
all three.

[core]
	repositoryformatversion = 0
	filemode = true
	bare = true

[remote "origin"]
        url = gitolite.kernel.org:/pub/scm/git/git-htmldocs.git
        url = github.com:gitster/git-htmldocs.git
	url = repo.or.cz:/srv/git/git-htmldocs.git
        push = refs/heads/master:refs/heads/master

Re: [PATCH v3 2/4] remote: use remote_state parameter internally

[Glen Choo]

Sorry I wasn't able to get to this sooner, this will be my first
priority.

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

>> This made my "git push" to k.org and other places over ssh segfault
>> when their tip and what I am attempting to push are identical.  I
>> haven't spent more time than just to bisect the history down to
>> identify this commit as the possible culprit.

Does this fail iff the tip and the attempted push are identical, or does
it also fail for others sorts of pushes?

> (gdb) bt
> #0  0x000055555579a785 in pushremote_for_branch (branch=0x0, explicit=0x7fffffffcf84)
>     at remote.c:564
> #1  0x000055555579a5c2 in remotes_remote_get_1 (remote_state=0x5555559782a0, name=0x0,
>     get_default=0x55555579a742 <pushremote_for_branch>) at remote.c:518
> #2  0x000055555579a6d0 in remotes_pushremote_get (remote_state=0x5555559782a0, name=0x0)
>     at remote.c:542
> #3  0x000055555579a740 in repo_pushremote_get (repo=0x555555974b80 <the_repo>, name=0x0)
>     at remote.c:554
> #4  0x000055555560aa9d in pushremote_get (name=0x0) at ./remote.h:135
> #5  0x000055555560c5ce in cmd_push (argc=0, argv=0x7fffffffdc70, prefix=0x0)
>     at builtin/push.c:611
> #6  0x000055555557396a in run_builtin (p=0x555555941f78 <commands+2136>, argc=3,
>     argv=0x7fffffffdc70) at git.c:461
> #7  0x0000555555573d79 in handle_builtin (argc=3, argv=0x7fffffffdc70) at git.c:713
> #8  0x0000555555573fe6 in run_argv (argcp=0x7fffffffdafc, argv=0x7fffffffdaf0) at git.c:780
> #9  0x000055555557448f in cmd_main (argc=3, argv=0x7fffffffdc70) at git.c:911
> #10 0x000055555565b2ae in main (argc=6, argv=0x7fffffffdc58) at common-main.c:52
>
> The direct culprit is this part:
>
>     const char *pushremote_for_branch(struct branch *branch, int *explicit)
>     {
>             if (branch && branch->pushremote_name) {
>                     if (explicit)
>                             *explicit = 1;
>                     return branch->pushremote_name;
>             }
>             if (branch->remote_state->pushremote_name) {
>
> where the second if() statement used to check "pushremote_name", but
> now unconditionally dereferences "branch".
>
> The caller is remote_get_1(); this funciton was called with
> "current_branch", which can be NULL until you have a repository and
> you've called read_config(), but otherwise shouldn't be.

Thanks for helping to narrow the scope of the search :)

> I think somebody is not setting up the remote_state correctly?  When
> the user wants to just use the repository-wide pushremote, not
> having the current_branch would not matter, but if the pushremote
> for the current branch is different from the repository-wide one,
> the code would silently push to a wrong remote.

For the_repository, remote_state should be initialized via
initialize_the_repository(), which is called in common-main.c. I assumed
that this would set up remote_state correctly. I will confirm whether or
not this is the cause.

> In any case, any public facing entry point, like pushremote_get()
> that is directly called from cmd_push() with just a name, should
> auto vivify an instance of struct remote_state and populate its
> members as needed, I think, and in this particular case, I suspect
> that it forgets to initialize the current_branch and other members
> by calling read_config(), just like other entry points like
> repo_remote_get() do.

I'm fairly certain that pushremote_get() calls read_config() correctly
via repo_pushremote_get():

  struct remote *repo_pushremote_get(struct repository *repo, const char *name)
  {
    read_config(repo);
    return remotes_pushremote_get(repo->remote_state, name);
  }

Perhaps the problem lies elsewhere, let me look into it further.

Re: [PATCH v3 2/4] remote: use remote_state parameter internally

[Glen Choo]

Glen Choo <chooglen@google.com> writes:

>> The caller is remote_get_1(); this funciton was called with
>> "current_branch", which can be NULL until you have a repository and
>> you've called read_config(), but otherwise shouldn't be.

One possible culprit is working with detached HEAD, are you pushing with
detached HEAD?

"current_branch" is allowed to be NULL when HEAD does not point to a
branch. From read_config():

	if (startup_info->have_repository) {
		const char *head_ref = refs_resolve_ref_unsafe(
			get_main_ref_store(repo), "HEAD", 0, NULL, &flag);
		if (head_ref && (flag & REF_ISSYMREF) &&
		    skip_prefix(head_ref, "refs/heads/", &head_ref)) {
			repo->remote_state->current_branch = make_branch(
				repo->remote_state, head_ref, strlen(head_ref));
		}
	}

This condition fails in detached head and "current_branch" is not set:

  head_ref && (flag & REF_ISSYMREF) && skip_prefix(head_ref, "refs/heads/", &head_ref)

>> The direct culprit is this part:
>>
>>     const char *pushremote_for_branch(struct branch *branch, int *explicit)
>>     {
>>             if (branch && branch->pushremote_name) {
>>                     if (explicit)
>>                             *explicit = 1;
>>                     return branch->pushremote_name;
>>             }
>>             if (branch->remote_state->pushremote_name) {
>>
>> where the second if() statement used to check "pushremote_name", but
>> now unconditionally dereferences "branch".
>>
To work with branch = NULL, it seems obvious that branch should be
conditionally dereferenced in pushremote_for_branch, i.e.

  - if (branch->remote_state->pushremote_name) {
  + if (brnach && branch->remote_state->pushremote_name) {

However, if you are not using detached HEAD, the problem might be
elsewhere..

Re: [PATCH v3 2/4] remote: use remote_state parameter internally

[Junio C Hamano]

Glen Choo <chooglen@google.com> writes:

> Glen Choo <chooglen@google.com> writes:
>
>>> The caller is remote_get_1(); this funciton was called with
>>> "current_branch", which can be NULL until you have a repository and
>>> you've called read_config(), but otherwise shouldn't be.
>
> One possible culprit is working with detached HEAD, are you pushing with
> detached HEAD?
>
> "current_branch" is allowed to be NULL when HEAD does not point to a
> branch.

Good point.  It is a good justification to make the remote_state
available to the function, as branch==NULL that signals "there is no
current branch in the repository" cannot be dereferenced to get to
either the repository or the remote_state, yet the function needs
access to remote_state even when branch==NULL.

What "branch" is pushremote_for_branch() meant to take?  Is there a
caller that asks a hypothetical "I know this is not a branch that is
the current branch in the repository, but to which remote would we
push if this branch _were_ the current one?" (and passes NULL to
mean "there is a checked out branch, but what happens if our HEAD
were detached?") question?  Even if there isn't currently, do we
want to add such callers in the future?

If the answer is yes, then the function need to take both branch and
remote_state as two separate parameters.  If the answer is no (i.e.
we never ask hypothetical questions, we just ask what we should do
in the current, real, state), then the function can just take the
remote_state and remote_state->branch being NULL would be used as a
signal that the HEAD is detached.  I suspect it is the former, as
this information is used in string-name-to-object translation for
"topic@{push}" in object-name.c::interpret_branch_mark() function.

> However, if you are not using detached HEAD, the problem might be
> elsewhere..

I just checked.  The repository the push is run in is bare and its
HEAD is detached, pointing at a commit directly.

Re: [PATCH v3 2/4] remote: use remote_state parameter internally

[Glen Choo]

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

> I just checked.  The repository the push is run in is bare and its
> HEAD is detached, pointing at a commit directly.

Thanks, I was able to reproduce the segfault using your config.

>> "current_branch" is allowed to be NULL when HEAD does not point to a
>> branch.
>
> Good point.  It is a good justification to make the remote_state
> available to the function, as branch==NULL that signals "there is no
> current branch in the repository" cannot be dereferenced to get to
> either the repository or the remote_state, yet the function needs
> access to remote_state even when branch==NULL.

Yes, I wish we had noticed this sooner in our discussion and the fault
is mine. It seems that pushremote_for_branch() is a prime example of
"get the settings from the branch if possible, but default to the
correct repository settings otherwise.", which is difficult to express
if remote_state is not available to the function.

> What "branch" is pushremote_for_branch() meant to take?  Is there a
> caller that asks a hypothetical "I know this is not a branch that is
> the current branch in the repository, but to which remote would we
> push if this branch _were_ the current one?" (and passes NULL to
> mean "there is a checked out branch, but what happens if our HEAD
> were detached?") question?  Even if there isn't currently, do we
> want to add such callers in the future?
>
> If the answer is yes, then the function need to take both branch and
> remote_state as two separate parameters.  If the answer is no (i.e.
> we never ask hypothetical questions, we just ask what we should do
> in the current, real, state), then the function can just take the
> remote_state and remote_state->branch being NULL would be used as a
> signal that the HEAD is detached.  I suspect it is the former, as
> this information is used in string-name-to-object translation for
> "topic@{push}" in object-name.c::interpret_branch_mark() function.

I agree that the need for hypothetical "what happens if HEAD were
detached?" questions may arise, though I'm not sure if there are any
right now. When we call branch_get(NULL), the expected return value is
the "current_branch". If there is no "current_branch" i.e. the return
value of branch_get() is the NULL branch. A NULL branch is not usable -
branch_get_push() and branch_get_upstream() return error messages
indicating "HEAD does not point to a branch". (these are the functions
used by object-name.c::interpret_branch_mark()).

Given the convention of "NULL branch == detached HEAD", how do we
proceed? Some options:

a) Represent detached HEAD with a non-NULL "struct branch". This will
   let us continue using the remote_state backpointer, which makes many
   interfaces clean, but is somewhat invasive, error-prone and it uses
   "struct branch" for something that is not a branch, which is itself
   an error-prone interface.

b) Keep the backpointer, but add remote_state as a parameter to
   pushremote_for_branch(). The least possible effort to fix the problem
   and might be 'easy' but is inconsistent with the other functions and
   is prone to misuse because the backpointer and parameter can be
   different.

c) Replace the backpointer with a remote_state parameter. Expressive and
   fits the paradigm of "defaulting to the repo when needed", but
   interfaces are repetitive and shifts the responsibility of
   correctness to the caller (see v2).

d) Default to the_repository in pushremote_for_branch(). Easy, but
   incorrect in general.

e) Some kind of reimagining of the remotes interfaces that doesn't
   have this problem. One possible approach is to remove branches from
   the remotes system altogether, since remotes are primarily concerned
   with _branch tracking information_ and not really _branches_ per se;
   perhaps we are being led astray by our terminology. If possible, this
   is probably the most elegant long term solution, but it's
   time-consuming and it's not clear how we will get there.

Currently, my preference is to go with (c). We can create a clear
expectation to callers that branch tracking information is not complete
without a repository, thus a repository is always supplied, explicitly
or not. If so, the remote_state parameter looks less like an
implementation detail, especially since a NULL branch is allowed.

I know we have already considered and abandoned (c) after v2, but has
your opinion changed after considering the new information?

Re: [PATCH v3 2/4] remote: use remote_state parameter internally

[Junio C Hamano]

Glen Choo <chooglen@google.com> writes:

>> If the answer is yes, then the function need to take both branch and
>> remote_state as two separate parameters.  If the answer is no (i.e.
>> we never ask hypothetical questions, we just ask what we should do
>> in the current, real, state), then the function can just take the
>> remote_state and remote_state->branch being NULL would be used as a
>> signal that the HEAD is detached.  I suspect it is the former, as
>> this information is used in string-name-to-object translation for
>> "topic@{push}" in object-name.c::interpret_branch_mark() function.
>
> I agree that the need for hypothetical "what happens if HEAD were
> detached?" questions may arise, though I'm not sure if there are any
> right now. When we call branch_get(NULL), the expected return value is
> the "current_branch". If there is no "current_branch" i.e. the return
> value of branch_get() is the NULL branch. A NULL branch is not usable -
> branch_get_push() and branch_get_upstream() return error messages
> indicating "HEAD does not point to a branch". (these are the functions
> used by object-name.c::interpret_branch_mark()).
>
> Given the convention of "NULL branch == detached HEAD", how do we
> proceed? Some options:
>
> a) Represent detached HEAD with a non-NULL "struct branch". This will
>    let us continue using the remote_state backpointer, which makes many
>    interfaces clean, but is somewhat invasive, error-prone and it uses
>    "struct branch" for something that is not a branch, which is itself
>    an error-prone interface.

I'd rather not to go there.

> b) Keep the backpointer, but add remote_state as a parameter to
>    pushremote_for_branch(). The least possible effort to fix the problem
>    and might be 'easy' but is inconsistent with the other functions and
>    is prone to misuse because the backpointer and parameter can be
>    different.

Make the function take a remote_state as a parameter (instead of,
not in addition to, the branch parameter), and use the remote_state
structure to find which branch's branch specific configuration we
want to use by checking its current_branch member.

I think that would be the best approach for "no, we won't ask
hypothetical question" case.

On the other hand,...

> c) Replace the backpointer with a remote_state parameter. Expressive and
>    fits the paradigm of "defaulting to the repo when needed", but
>    interfaces are repetitive and shifts the responsibility of
>    correctness to the caller (see v2).

... if we want to support the what-if callers, I
think the best approach would be a slight variant of c) above.

That is, pass branch and remote_state as two parameters, and when
branch is not NULL, barf if it is not among remote_state.branches[],
to protect against nonsense combinations.

Thanks.

Re: [PATCH v3 2/4] remote: use remote_state parameter internally

[Junio C Hamano]

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

>> a) Represent detached HEAD with a non-NULL "struct branch". This will
>>    let us continue using the remote_state backpointer, which makes many
>>    interfaces clean, but is somewhat invasive, error-prone and it uses
>>    "struct branch" for something that is not a branch, which is itself
>>    an error-prone interface.
>
> I'd rather not to go there.

Actually, I take half of that back, as I think this would be the
best direction in the longer term, and it conceptually is sound.
After all, detached HEAD is not all that special--- when we ask for
the "current branch" and the HEAD happens to be detached, we treat
it as a perfectly valid state and behave as if we are on that
unnamed branch.  The state probably deserves to be represented as a
"struct branch" instance.

I do agree with you that this approach would involve significant
short-term pain, as a lot of existing code may say "NULL is how we
represent detached HEAD" and they have to be identified and upgraded
before the above plan calls for.

So, I tend to think that in the shorter term, perhaps a safer
variant of (c) that allows us to ask "not-current@{push}" would be a
good way to go, but when things have stabilized, we should revisit
and see if it is feasible to represent a detached HEAD state with an
instance of "struct branch" and how simpler the interface would become
when we did so.

Thanks.

Re: [PATCH v3 2/4] remote: use remote_state parameter internally

[Glen Choo]

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

>> c) Replace the backpointer with a remote_state parameter. Expressive and
>>    fits the paradigm of "defaulting to the repo when needed", but
>>    interfaces are repetitive and shifts the responsibility of
>>    correctness to the caller (see v2).
> 
> ... if we want to support the what-if callers, I
> think the best approach would be a slight variant of c) above.
> 
> That is, pass branch and remote_state as two parameters, and when
> branch is not NULL, barf if it is not among remote_state.branches[],
> to protect against nonsense combinations.

Sounds reasonable to me. The resulting interface would look like the v2
one, but internally, this additional safety check will prevent misuse.

>>> a) Represent detached HEAD with a non-NULL "struct branch". This will
>>>    let us continue using the remote_state backpointer, which makes many
>>>    interfaces clean, but is somewhat invasive, error-prone and it uses
>>>    "struct branch" for something that is not a branch, which is itself
>>>    an error-prone interface.
>>
>> I'd rather not to go there.
>
> Actually, I take half of that back, as I think this would be the
> best direction in the longer term, and it conceptually is sound.
> After all, detached HEAD is not all that special--- when we ask for
> the "current branch" and the HEAD happens to be detached, we treat
> it as a perfectly valid state and behave as if we are on that
> unnamed branch.  The state probably deserves to be represented as a
> "struct branch" instance.
> [...] when things have stabilized, we should revisit
> and see if it is feasible to represent a detached HEAD state with an
> instance of "struct branch" and how simpler the interface would become
> when we did so.

This "longer term direction" sounds like what I envisioned with (e). I
agree that detached HEAD is a state that should be expressed with more
than just NULL, though I'm not sure that "struct branch" is the correct
abstraction. No point bikeshedding now of course, we'll cross that
bridge when we get there ;)

Re: [PATCH v3 2/4] remote: use remote_state parameter internally

[Junio C Hamano]

Glen Choo <chooglen@google.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>>> c) Replace the backpointer with a remote_state parameter. Expressive and
>>>    fits the paradigm of "defaulting to the repo when needed", but
>>>    interfaces are repetitive and shifts the responsibility of
>>>    correctness to the caller (see v2).
>> 
>> ... if we want to support the what-if callers, I
>> think the best approach would be a slight variant of c) above.
>> 
>> That is, pass branch and remote_state as two parameters, and when
>> branch is not NULL, barf if it is not among remote_state.branches[],
>> to protect against nonsense combinations.
>
> Sounds reasonable to me. The resulting interface would look like the v2
> one, but internally, this additional safety check will prevent misuse.

Hopefully.  Of course I think the implementation of the safety would
actually be done, not by iterationg over branches[] array, but just
checking branch->remote_state == remote_state pointer equality.

> This "longer term direction" sounds like what I envisioned with (e). I
> agree that detached HEAD is a state that should be expressed with more
> than just NULL, though I'm not sure that "struct branch" is the correct
> abstraction. No point bikeshedding now of course, we'll cross that
> bridge when we get there ;)

I actually was hoping that the time to cross the bridge was now,
though ;-)

Re: [PATCH v3 2/4] remote: use remote_state parameter internally

[Glen Choo]

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

> Glen Choo <chooglen@google.com> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>>> c) Replace the backpointer with a remote_state parameter. Expressive and
>>>>    fits the paradigm of "defaulting to the repo when needed", but
>>>>    interfaces are repetitive and shifts the responsibility of
>>>>    correctness to the caller (see v2).
> Hopefully.  Of course I think the implementation of the safety would
> actually be done, not by iterationg over branches[] array, but just
> checking branch->remote_state == remote_state pointer equality.

With (c), I plan to eliminate the backpointer altogether, so such a
check is not possible. However, I plan to add a branches_hash to
remote_state in the same way that we have remotes_hash for remote_state.

>> This "longer term direction" sounds like what I envisioned with (e). I
>> agree that detached HEAD is a state that should be expressed with more
>> than just NULL, though I'm not sure that "struct branch" is the correct
>> abstraction. No point bikeshedding now of course, we'll cross that
>> bridge when we get there ;)
>
> I actually was hoping that the time to cross the bridge was now,
> though ;-)

Hm, well I don't want to get too lost in the weeds here and lose sight
of the short-term objective. I have a few strands of thought, but
nothing concrete enough to propose a full alternative:

- It seems odd that the branches and the "current_branch" are handled by
  the remotes system, perhaps it would be clearer to move it elsewhere.
  Separating branches from "branch remote-tracking configuration" might
  clarify our thinking.
- branch_get(NULL) and branch_get("HEAD") are not entirely coherent with
  the idea of "getting a branch by name", perhaps we should just move
  this functionality into a different function.
- Similarly, variants of remote_for_branch() are misleading because they
  behave inconsistently when branch is NULL. 

I might not want to take action on these ideas though, e.g.
branch_get("HEAD") or remote_for_branch(NULL) are very convenient for
callers even though they behave slightly inconsisently. I'll propose a
longer term direction when I have a better grasp of the system and the
tradeoffs.

Re: [PATCH v3 2/4] remote: use remote_state parameter internally

[Junio C Hamano]

Glen Choo <chooglen@google.com> writes:

> I might not want to take action on these ideas though, e.g.
> branch_get("HEAD") or remote_for_branch(NULL) are very convenient
> for callers even though they behave slightly inconsisently. I'll
> propose a longer term direction when I have a better grasp of the
> system and the tradeoffs.

Yup, that sounds like a good plan.

[PATCH v3 3/4] remote: remove the_repository->remote_state from static methods

[Glen Choo]

Replace all remaining references of the_repository->remote_state in
static methods with a struct remote_state parameter. To do so, introduce
a family of helper functions, "remotes_*", that behave like "repo_*",
but accept struct remote_state instead of struct repository, and move
read_config() calls to non-static functions.

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

diff --git a/remote.c b/remote.c
index e3ca44f735..cf9cced5ed 100644
--- a/remote.c
+++ b/remote.c
@@ -505,6 +505,55 @@ const char *remote_for_branch(struct branch *branch, int *explicit)
 	return "origin";
 }
 
+static struct remote *remotes_remote_get_1(
+	struct remote_state *remote_state, const char *name,
+	const char *(*get_default)(struct branch *, int *))
+{
+	struct remote *ret;
+	int name_given = 0;
+
+	if (name)
+		name_given = 1;
+	else
+		name = get_default(remote_state->current_branch,
+				   &name_given);
+
+	ret = make_remote(remote_state, name, 0);
+	if (valid_remote_nick(name) && have_git_dir()) {
+		if (!valid_remote(ret))
+			read_remotes_file(ret);
+		if (!valid_remote(ret))
+			read_branches_file(ret);
+	}
+	if (name_given && !valid_remote(ret))
+		add_url_alias(ret, name);
+	if (!valid_remote(ret))
+		return NULL;
+	return ret;
+}
+
+static inline struct remote *remotes_remote_get(struct remote_state *remote_state, const char *name)
+{
+	return remotes_remote_get_1(remote_state, name, remote_for_branch);
+}
+
+static inline struct remote *remotes_pushremote_get(struct remote_state *remote_state, const char *name)
+{
+	return remotes_remote_get_1(remote_state, name, pushremote_for_branch);
+}
+
+struct remote *remote_get(const char *name)
+{
+	read_config(the_repository);
+	return remotes_remote_get(the_repository->remote_state, name);
+}
+
+struct remote *pushremote_get(const char *name)
+{
+	read_config(the_repository);
+	return remotes_pushremote_get(the_repository->remote_state, name);
+}
+
 const char *pushremote_for_branch(struct branch *branch, int *explicit)
 {
 	if (branch && branch->pushremote_name) {
@@ -530,7 +579,8 @@ const char *remote_ref_for_branch(struct branch *branch, int for_push)
 		} else {
 			const char *dst, *remote_name =
 				pushremote_for_branch(branch, NULL);
-			struct remote *remote = remote_get(remote_name);
+			struct remote *remote =
+				remotes_remote_get(branch->remote_state, remote_name);
 
 			if (remote && remote->push.nr &&
 			    (dst = apply_refspecs(&remote->push,
@@ -542,44 +592,6 @@ 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 *))
-{
-	struct remote *ret;
-	int name_given = 0;
-
-	read_config(the_repository);
-
-	if (name)
-		name_given = 1;
-	else
-		name = get_default(the_repository->remote_state->current_branch,
-				   &name_given);
-
-	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);
-		if (!valid_remote(ret))
-			read_branches_file(ret);
-	}
-	if (name_given && !valid_remote(ret))
-		add_url_alias(ret, name);
-	if (!valid_remote(ret))
-		return NULL;
-	return ret;
-}
-
-struct remote *remote_get(const char *name)
-{
-	return remote_get_1(name, remote_for_branch);
-}
-
-struct remote *pushremote_get(const char *name)
-{
-	return remote_get_1(name, pushremote_for_branch);
-}
-
 int remote_is_configured(struct remote *remote, int in_repo)
 {
 	if (!remote)
@@ -1684,7 +1696,7 @@ static void set_merge(struct branch *ret)
 		return;
 	}
 
-	remote = remote_get(ret->remote_name);
+	remote = remotes_remote_get(ret->remote_state, ret->remote_name);
 
 	CALLOC_ARRAY(ret->merge, ret->merge_nr);
 	for (i = 0; i < ret->merge_nr; i++) {
@@ -1786,7 +1798,7 @@ static const char *branch_get_push_1(struct branch *branch, struct strbuf *err)
 {
 	struct remote *remote;
 
-	remote = remote_get(pushremote_for_branch(branch, NULL));
+	remote = remotes_remote_get(branch->remote_state, pushremote_for_branch(branch, NULL));
 	if (!remote)
 		return error_buf(err,
 				 _("branch '%s' has no remote for pushing"),
-- 
2.33.GIT

[PATCH v3 4/4] remote: add struct repository parameter to external functions

[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 | 29 ++++++++++++++---------------
 remote.h | 28 +++++++++++++++++++++++-----
 2 files changed, 37 insertions(+), 20 deletions(-)

diff --git a/remote.c b/remote.c
index cf9cced5ed..179f0ff1ab 100644
--- a/remote.c
+++ b/remote.c
@@ -542,16 +542,16 @@ static inline struct remote *remotes_pushremote_get(struct remote_state *remote_
 	return remotes_remote_get_1(remote_state, name, pushremote_for_branch);
 }
 
-struct remote *remote_get(const char *name)
+struct remote *repo_remote_get(struct repository *repo, const char *name)
 {
-	read_config(the_repository);
-	return remotes_remote_get(the_repository->remote_state, name);
+	read_config(repo);
+	return remotes_remote_get(repo->remote_state, name);
 }
 
-struct remote *pushremote_get(const char *name)
+struct remote *repo_pushremote_get(struct repository *repo, const char *name)
 {
-	read_config(the_repository);
-	return remotes_pushremote_get(the_repository->remote_state, name);
+	read_config(repo);
+	return remotes_pushremote_get(repo->remote_state, name);
 }
 
 const char *pushremote_for_branch(struct branch *branch, int *explicit)
@@ -601,14 +601,14 @@ 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);
-	for (i = 0; i < the_repository->remote_state->remotes_nr && !result;
+	read_config(repo);
+	for (i = 0; i < repo->remote_state->remotes_nr && !result;
 	     i++) {
 		struct remote *remote =
-			the_repository->remote_state->remotes[i];
+			repo->remote_state->remotes[i];
 		if (!remote)
 			continue;
 		result = fn(remote, priv);
@@ -1713,16 +1713,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(the_repository);
+	read_config(repo);
 	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));
+		ret = make_branch(repo->remote_state, name, strlen(name));
 	set_merge(ret);
 	return ret;
 }
diff --git a/remote.h b/remote.h
index d268a92ebb..e19cae2718 100644
--- a/remote.h
+++ b/remote.h
@@ -123,15 +123,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);
 
@@ -330,7 +343,12 @@ struct branch {
 	struct remote_state *remote_state;
 };
 
-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 *remote_ref_for_branch(struct branch *branch, int for_push);
-- 
2.33.GIT

[PATCH v4 0/6] remote: replace static variables with struct remote_state

[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.

v4 reverts the backpointer introduced in v3. In authoring v3, I had
overlooked the fact that branch == NULL (representing detached HEAD) is
a valid argument to some branch_* functions and as a result, we cannot
always rely on branch->remote_state to tell us the remote_state of a
branch. This was not discovered because of a coding mistake in v3 where
branch was unconditionally dereferenced, even when it was null [2]. v4
adds a test that checks the relevant behavior.

The resulting interface is similar to v2, but with Junio's proposed
safety check [3] - when branch + repository are passed as a pair we
check that the branch belongs to the repository (i.e. it is in the
repository's remote_state struct). This check is only implemented for
non-static functions because the probability of misuse is much higher.
In static functions, this check is wasteful because we frequently
operate on the remote_state + {branch, remote} pair in order to maintain
data consistency and the correct remote_state is often obvious from
context.

In the long run, I believe that there is room for refactoring/interface changes
as to avoid these internal correctness issues, but I think this is a good enough
starting point.

[1] https://lore.kernel.org/git/20210921232529.81811-1-chooglen@google.com/
[2] https://lore.kernel.org/git/xmqqtuhbo2tn.fsf@gitster.g
[2] https://lore.kernel.org/git/xmqqfssozk8r.fsf@gitster.g

Changes since v3:
* Add a test case for pushing to a remote in detached HEAD. This test
  would have caught the segfault that resulted in this reroll.
* Remove the NEEDSWORK saying that init_remotes_hash() should be moved
  into remote_state_new() and just do it.
* Remove the backpointer to remote_state and add a remote_state
  parameter instead.
* In patch 4, add more remotes_* functions. These functions were not
  needed in v3 because of the backpointer.
* In patch 5, add a function that checks if a branch is in a repo. Add a
  branch hashmap that makes this operation fast.
* In patch 6, add more repo_* functions. These functions were not needed
  in v3 because of the backpointer.

Changes since v2:
* Add .remote_state to struct branch and struct remote, changing the
  implementation appropriately.
* In patch 2, properly consider the initialized state of remote_state.
  In v2, I forgot to convert a static inside read_config() into a
  private member of struct remote_state. Fix this.
* In a new patch 3, add helper methods that get a remote via
  remote_state and the remote name.
* Move read_config(repo) calls to the external facing-functions. This keeps
  "struct repository" away from the remote.c internals.

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.

Glen Choo (6):
  t5516: add test case for pushing remote refspecs
  remote: move static variables into per-repository struct
  remote: use remote_state parameter internally
  remote: remove the_repository->remote_state from static methods
  remote: die if branch is not found in repository
  remote: add struct repository parameter to external functions

 remote.c              | 406 +++++++++++++++++++++++++++++-------------
 remote.h              | 118 ++++++++++--
 repository.c          |   8 +
 repository.h          |   4 +
 t/t5516-fetch-push.sh |   9 +
 5 files changed, 404 insertions(+), 141 deletions(-)

Range-diff against v3:
-:  ---------- > 1:  9b29ec27c6 t5516: add test case for pushing remote refspecs
1:  1f712c22b4 ! 2:  ca9b5ab66a remote: move static variables into per-repository struct
    @@ remote.c: static void add_pushurl(struct remote *remote, const char *pushurl)
      }
      
     @@ remote.c: 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(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)
    + {
    + 	struct remote *ret;
     @@ remote.c: static struct remote *make_remote(const char *name, int len)
    + 	if (!len)
    + 		len = strlen(name);
    + 
    +-	init_remotes_hash();
    + 	lookup.str = name;
      	lookup.len = len;
      	hashmap_entry_init(&lookup_entry, memhash(name, len));
      
    @@ remote.c: void apply_push_cas(struct push_cas_option *cas,
     +	struct remote_state *r = xmalloc(sizeof(*r));
     +
     +	memset(r, 0, sizeof(*r));
    ++
    ++	hashmap_init(&r->remotes_hash, remotes_hash_cmp, NULL, 0);
     +	return r;
     +}
     +
2:  467247fa9c ! 3:  5d6a245cae remote: use remote_state parameter internally
    @@ Metadata
      ## Commit message ##
         remote: use remote_state parameter internally
     
    -    Introduce a struct remote_state member to structs that need to
    -    'remember' their remote_state. Without changing external-facing
    -    functions, replace the_repository->remote_state internally by using the
    -    remote_state member where it is applicable i.e. when a function accepts
    -    a struct that depends on the remote_state. If it is not applicable, add
    -    a struct remote_state parameter instead.
    +    Without changing external-facing functions, replace
    +    the_repository->remote_state internally by adding a struct remote_state
    +    parameter.
     
         As a result, external-facing functions are still tied to the_repository,
         but most static functions no longer reference
    @@ Commit message
     
      ## remote.c ##
     @@ remote.c: static void add_pushurl(struct remote *remote, const char *pushurl)
    - static void add_pushurl_alias(struct remote *remote, const char *url)
    + 	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 =
    +-	const char *pushurl =
     -		alias_url(url, &the_repository->remote_state->rewrites_push);
    -+		alias_url(url, &remote->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 *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_url(remote, alias_url(url, &remote->remote_state->rewrites));
    - 	add_pushurl_alias(remote, url);
    +-	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 {
     @@ remote.c: 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)
    @@ remote.c: static int remotes_hash_cmp(const void *unused_cmp_data,
      	struct remote *ret;
      	struct remotes_hash_key lookup;
     @@ remote.c: 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));
      
    @@ remote.c: static struct remote *make_remote(const char *name, int len)
      		return container_of(e, struct remote, ent);
      
     @@ remote.c: static struct remote *make_remote(const char *name, int len)
    - 	ret->prune = -1;  /* unspecified */
    - 	ret->prune_tags = -1;  /* unspecified */
    - 	ret->name = xstrndup(name, len);
    -+	ret->remote_state = remote_state;
      	refspec_init(&ret->push, REFSPEC_PUSH);
      	refspec_init(&ret->fetch, REFSPEC_FETCH);
      
    @@ remote.c: static void add_merge(struct branch *branch, const char *name)
     +	remote_state->branches[remote_state->branches_nr++] = ret;
      	ret->name = xstrndup(name, len);
      	ret->refname = xstrfmt("refs/heads/%s", ret->name);
    -+	ret->remote_state = remote_state;
      
    - 	return ret;
    +@@ remote.c: 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");
    +@@ remote.c: 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))
    +@@ remote.c: 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;
    +@@ remote.c: 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);
    + 
     @@ remote.c: static int handle_config(const char *key, const char *value, void *cb)
      	const char *subkey;
      	struct remote *remote;
    @@ remote.c: static int handle_config(const char *key, const char *value, void *cb)
     -				->url[j] = alias_url(
     -				the_repository->remote_state->remotes[i]->url[j],
     -				&the_repository->remote_state->rewrites);
    -+					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],
    @@ remote.c: static int handle_config(const char *key, const char *value, void *cb)
      }
      
      static int valid_remote_nick(const char *name)
    -@@ remote.c: const char *pushremote_for_branch(struct branch *branch, int *explicit)
    - 			*explicit = 1;
    - 		return branch->pushremote_name;
    - 	}
    --	if (the_repository->remote_state->pushremote_name) {
    -+	if (branch->remote_state->pushremote_name) {
    - 		if (explicit)
    - 			*explicit = 1;
    --		return the_repository->remote_state->pushremote_name;
    -+		return branch->remote_state->pushremote_name;
    - 	}
    - 	return remote_for_branch(branch, explicit);
    - }
     @@ remote.c: static struct remote *remote_get_1(const char *name,
      	struct remote *ret;
      	int name_given = 0;
    @@ remote.c: static struct remote *remote_get_1(const char *name,
     +	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(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;
     @@ remote.c: int remote_is_configured(struct remote *remote, int in_repo)
      int for_each_remote(each_remote_fn fn, void *priv)
      {
    @@ remote.h: struct remote_state {
      };
      
      void remote_state_clear(struct remote_state *remote_state);
    -@@ remote.h: struct remote {
    - 
    - 	/* The method used for authenticating against `http_proxy`. */
    - 	char *http_proxy_authmethod;
    -+
    -+	/** The remote_state that this remote belongs to. This is only meant to
    -+	 * be used by remote_* functions. */
    -+	struct remote_state *remote_state;
    - };
    - 
    - /**
    -@@ remote.h: struct branch {
    - 	int merge_alloc;
    - 
    - 	const char *push_tracking_ref;
    -+
    -+	/** The remote_state that this branch belongs to. This is only meant to
    -+	 * be used by branch_* functions. */
    -+	struct remote_state *remote_state;
    - };
    - 
    - struct branch *branch_get(const char *name);
3:  10fbb84496 < -:  ---------- remote: remove the_repository->remote_state from static methods
4:  4013f74fd9 < -:  ---------- remote: add struct repository parameter to external functions
-:  ---------- > 4:  53f2e31f72 remote: remove the_repository->remote_state from static methods
-:  ---------- > 5:  d3281c14eb remote: die if branch is not found in repository
-:  ---------- > 6:  0974994cc6 remote: add struct repository parameter to external functions
-- 
2.33.GIT

[PATCH v4 1/6] t5516: add test case for pushing remote refspecs

[Glen Choo]

In detached HEAD, "git push remote-name" should push the refspecs in
remote.remote-name.push. Since there is no test case that checks this
behavior, add one.

Signed-off-by: Glen Choo <chooglen@google.com>
---
 t/t5516-fetch-push.sh | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 8212ca56dc..d4c2f1b68f 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -541,6 +541,15 @@ do
 
 done
 
+test_expect_success "push to remote with detached HEAD and config remote.*.push = src:dest" '
+	mk_test testrepo heads/main &&
+	git checkout $the_first_commit &&
+	test_config remote.there.url testrepo &&
+	test_config remote.there.push refs/heads/main:refs/heads/main &&
+	git push there &&
+	check_push_result testrepo $the_commit heads/main
+'
+
 test_expect_success 'push with remote.pushdefault' '
 	mk_test up_repo heads/main &&
 	mk_test down_repo heads/main &&
-- 
2.33.GIT

Re: [PATCH v4 1/6] t5516: add test case for pushing remote refspecs

[Junio C Hamano]

Glen Choo <chooglen@google.com> writes:

> In detached HEAD, "git push remote-name" should push the refspecs in
> remote.remote-name.push. Since there is no test case that checks this
> behavior, add one.

Makes sense.

>
> Signed-off-by: Glen Choo <chooglen@google.com>
> ---
>  t/t5516-fetch-push.sh | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index 8212ca56dc..d4c2f1b68f 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -541,6 +541,15 @@ do
>  
>  done
>  
> +test_expect_success "push to remote with detached HEAD and config remote.*.push = src:dest" '
> +	mk_test testrepo heads/main &&
> +	git checkout $the_first_commit &&
> +	test_config remote.there.url testrepo &&
> +	test_config remote.there.push refs/heads/main:refs/heads/main &&
> +	git push there &&
> +	check_push_result testrepo $the_commit heads/main
> +'
> +
>  test_expect_success 'push with remote.pushdefault' '
>  	mk_test up_repo heads/main &&
>  	mk_test down_repo heads/main &&

Re: [PATCH v4 1/6] t5516: add test case for pushing remote refspecs

[Jonathan Tan]

> In detached HEAD, "git push remote-name" should push the refspecs in
> remote.remote-name.push. Since there is no test case that checks this
> behavior, add one.

This is technically true, but reading the documentation, it seems that
this should also happen when we're not in detached HEAD. Maybe write:

  "git push remote-name" (that is, with no refspec given on the command
  line) should push the refspecs in remote.remote-name.push. There is no
  test case that checks this behavior in detached HEAD, so add one.

The test itself looks OK.

Re: [PATCH v4 1/6] t5516: add test case for pushing remote refspecs

[Glen Choo]

Jonathan Tan <jonathantanmy@google.com> writes:

> This is technically true, but reading the documentation, it seems that
> this should also happen when we're not in detached HEAD. Maybe write:
>
>   "git push remote-name" (that is, with no refspec given on the command
>   line) should push the refspecs in remote.remote-name.push. There is no
>   test case that checks this behavior in detached HEAD, so add one.

Ah, you are right. I was under the wrong impression that refspec
defaults to HEAD if HEAD points to a branch.

I will adopt your wording, thanks.

[PATCH v4 2/6] remote: move static variables into per-repository struct

[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     | 191 +++++++++++++++++++++++++++++++++------------------
 remote.h     |  34 +++++++++
 repository.c |   8 +++
 repository.h |   4 ++
 4 files changed, 170 insertions(+), 67 deletions(-)

diff --git a/remote.c b/remote.c
index f958543d70..c2bb11242b 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);
 }
 
@@ -127,12 +102,6 @@ static int remotes_hash_cmp(const void *unused_cmp_data,
 		return strcmp(a->name, b->name);
 }
 
-static inline void init_remotes_hash(void)
-{
-	if (!remotes_hash.cmpfn)
-		hashmap_init(&remotes_hash, remotes_hash_cmp, NULL, 0);
-}
-
 static struct remote *make_remote(const char *name, int len)
 {
 	struct remote *ret;
@@ -142,12 +111,12 @@ static struct remote *make_remote(const char *name, int len)
 	if (!len)
 		len = strlen(name);
 
-	init_remotes_hash();
 	lookup.str = name;
 	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 +127,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 +174,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 +327,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 +346,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 +431,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 +472,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 +516,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 +557,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 +597,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 +1711,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 +2611,34 @@ 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));
+
+	hashmap_init(&r->remotes_hash, remotes_hash_cmp, NULL, 0);
+	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..d21c035f1b 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 {
+	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.GIT

[PATCH v4 3/6] remote: use remote_state parameter internally

[Glen Choo]

Without changing external-facing functions, replace
the_repository->remote_state internally by adding a struct remote_state
parameter.

As a result, external-facing functions are still tied to the_repository,
but 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 | 159 +++++++++++++++++++++++++------------------------------
 remote.h |   2 +
 2 files changed, 75 insertions(+), 86 deletions(-)

diff --git a/remote.c b/remote.c
index c2bb11242b..48374cc0e2 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,7 +102,8 @@ static int remotes_hash_cmp(const void *unused_cmp_data,
 		return strcmp(a->name, b->name);
 }
 
-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;
@@ -115,8 +116,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(&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);
 
@@ -127,15 +127,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;
 }
@@ -169,25 +166,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);
 
@@ -229,7 +223,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");
@@ -244,7 +239,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))
@@ -254,7 +250,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;
@@ -286,7 +283,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);
 
@@ -305,10 +302,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")) {
@@ -327,16 +326,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));
 		}
 	}
@@ -346,9 +343,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;
@@ -358,7 +354,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)
@@ -428,61 +424,51 @@ 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 repository *repo)
 {
-	static int loaded;
 	int flag;
 
-	if (loaded)
+	if (repo->remote_state->initialized)
 		return;
-	loaded = 1;
+	repo->remote_state->initialized = 1;
 
-	the_repository->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);
+		const char *head_ref = refs_resolve_ref_unsafe(
+			get_main_ref_store(repo), "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));
+			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)
@@ -552,7 +538,7 @@ static struct remote *remote_get_1(const char *name,
 	struct remote *ret;
 	int name_given = 0;
 
-	read_config();
+	read_config(the_repository);
 
 	if (name)
 		name_given = 1;
@@ -560,15 +546,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;
@@ -596,7 +582,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);
 	for (i = 0; i < the_repository->remote_state->remotes_nr && !result;
 	     i++) {
 		struct remote *remote =
@@ -1709,11 +1695,12 @@ struct branch *branch_get(const char *name)
 {
 	struct branch *ret;
 
-	read_config();
+	read_config(the_repository);
 	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;
 }
diff --git a/remote.h b/remote.h
index d21c035f1b..85a730b8ef 100644
--- a/remote.h
+++ b/remote.h
@@ -52,6 +52,8 @@ struct remote_state {
 
 	struct rewrites rewrites;
 	struct rewrites rewrites_push;
+
+	int initialized;
 };
 
 void remote_state_clear(struct remote_state *remote_state);
-- 
2.33.GIT

[PATCH v4 4/6] remote: remove the_repository->remote_state from static methods

[Glen Choo]

Replace all remaining references of the_repository->remote_state in
static functions with a struct remote_state parameter.

To do so, move read_config() calls to non-static functions and create a
family of static functions, "remotes_*", that behave like "repo_*", but
accept struct remote_state instead of struct repository. In the case
where a static function calls a non-static function, replace the
non-static function with its "remotes_*" equivalent.

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

diff --git a/remote.c b/remote.c
index 48374cc0e2..88697e7e6d 100644
--- a/remote.c
+++ b/remote.c
@@ -483,7 +483,9 @@ static int valid_remote_nick(const char *name)
 	return 1;
 }
 
-const char *remote_for_branch(struct branch *branch, int *explicit)
+static const char *remotes_remote_for_branch(struct remote_state *remote_state,
+					     struct branch *branch,
+					     int *explicit)
 {
 	if (branch && branch->remote_name) {
 		if (explicit)
@@ -495,32 +497,55 @@ const char *remote_for_branch(struct branch *branch, int *explicit)
 	return "origin";
 }
 
-const char *pushremote_for_branch(struct branch *branch, int *explicit)
+const char *remote_for_branch(struct branch *branch, int *explicit)
+{
+	read_config(the_repository);
+	return remotes_remote_for_branch(the_repository->remote_state, branch,
+					 explicit);
+}
+
+static const char *
+remotes_pushremote_for_branch(struct remote_state *remote_state,
+			      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 (remote_state->pushremote_name) {
 		if (explicit)
 			*explicit = 1;
-		return the_repository->remote_state->pushremote_name;
+		return remote_state->pushremote_name;
 	}
-	return remote_for_branch(branch, explicit);
+	return remotes_remote_for_branch(remote_state, branch, explicit);
 }
 
+const char *pushremote_for_branch(struct branch *branch, int *explicit)
+{
+	read_config(the_repository);
+	return remotes_pushremote_for_branch(the_repository->remote_state,
+					     branch, explicit);
+}
+
+static struct remote *remotes_remote_get(struct remote_state *remote_state,
+					 const char *name);
+
 const char *remote_ref_for_branch(struct branch *branch, int for_push)
 {
+	read_config(the_repository);
 	if (branch) {
 		if (!for_push) {
 			if (branch->merge_nr) {
 				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 = remotes_pushremote_for_branch(
+					the_repository->remote_state, branch,
+					NULL);
+			struct remote *remote = remotes_remote_get(
+				the_repository->remote_state, remote_name);
 
 			if (remote && remote->push.nr &&
 			    (dst = apply_refspecs(&remote->push,
@@ -532,42 +557,58 @@ 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 *
+remotes_remote_get_1(struct remote_state *remote_state, const char *name,
+		     const char *(*get_default)(struct remote_state *,
+						struct branch *, int *))
 {
 	struct remote *ret;
 	int name_given = 0;
 
-	read_config(the_repository);
-
 	if (name)
 		name_given = 1;
 	else
-		name = get_default(the_repository->remote_state->current_branch,
+		name = get_default(remote_state, remote_state->current_branch,
 				   &name_given);
 
-	ret = make_remote(the_repository->remote_state, name, 0);
+	ret = make_remote(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(remote_state, ret);
 		if (!valid_remote(ret))
-			read_branches_file(the_repository->remote_state, ret);
+			read_branches_file(remote_state, ret);
 	}
 	if (name_given && !valid_remote(ret))
-		add_url_alias(the_repository->remote_state, ret, name);
+		add_url_alias(remote_state, ret, name);
 	if (!valid_remote(ret))
 		return NULL;
 	return ret;
 }
 
+static inline struct remote *
+remotes_remote_get(struct remote_state *remote_state, const char *name)
+{
+	return remotes_remote_get_1(remote_state, name,
+				    remotes_remote_for_branch);
+}
+
 struct remote *remote_get(const char *name)
 {
-	return remote_get_1(name, remote_for_branch);
+	read_config(the_repository);
+	return remotes_remote_get(the_repository->remote_state, name);
+}
+
+static inline struct remote *
+remotes_pushremote_get(struct remote_state *remote_state, const char *name)
+{
+	return remotes_remote_get_1(remote_state, name,
+				    remotes_pushremote_for_branch);
 }
 
 struct remote *pushremote_get(const char *name)
 {
-	return remote_get_1(name, pushremote_for_branch);
+	read_config(the_repository);
+	return remotes_pushremote_get(the_repository->remote_state, name);
 }
 
 int remote_is_configured(struct remote *remote, int in_repo)
@@ -1654,7 +1695,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 remote_state *remote_state, struct branch *ret)
 {
 	struct remote *remote;
 	char *ref;
@@ -1674,7 +1715,7 @@ static void set_merge(struct branch *ret)
 		return;
 	}
 
-	remote = remote_get(ret->remote_name);
+	remote = remotes_remote_get(remote_state, ret->remote_name);
 
 	CALLOC_ARRAY(ret->merge, ret->merge_nr);
 	for (i = 0; i < ret->merge_nr; i++) {
@@ -1701,7 +1742,7 @@ struct branch *branch_get(const char *name)
 	else
 		ret = make_branch(the_repository->remote_state, name,
 				  strlen(name));
-	set_merge(ret);
+	set_merge(the_repository->remote_state, ret);
 	return ret;
 }
 
@@ -1772,11 +1813,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 *branch_get_push_1(struct remote_state *remote_state,
+				     struct branch *branch, struct strbuf *err)
 {
 	struct remote *remote;
 
-	remote = remote_get(pushremote_for_branch(branch, NULL));
+	remote = remotes_remote_get(
+		remote_state,
+		remotes_pushremote_for_branch(remote_state, branch, NULL));
 	if (!remote)
 		return error_buf(err,
 				 _("branch '%s' has no remote for pushing"),
@@ -1834,11 +1878,13 @@ static const char *branch_get_push_1(struct branch *branch, struct strbuf *err)
 
 const char *branch_get_push(struct branch *branch, struct strbuf *err)
 {
+	read_config(the_repository);
 	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 = branch_get_push_1(
+			the_repository->remote_state, branch, err);
 	return branch->push_tracking_ref;
 }
 
-- 
2.33.GIT

Re: [PATCH v4 4/6] remote: remove the_repository->remote_state from static methods

[Jonathan Tan]

> Replace all remaining references of the_repository->remote_state in
> static functions with a struct remote_state parameter.
> 
> To do so, move read_config() calls to non-static functions and create a
> family of static functions, "remotes_*", that behave like "repo_*", but
> accept struct remote_state instead of struct repository. In the case
> where a static function calls a non-static function, replace the
> non-static function with its "remotes_*" equivalent.

In patches 2-4, the same lines have been changed a few times, so much so
that I found it easier to review if I squashed 2-4 together. I'm not
sure what the opinions of other reviewers are, but I think it's better
if patches 2-4 were combined into one patch.

The only confusing part is the moving of read_config(), but that is
described in this commit message - it's moved to the outer (non-static)
functions so that the inner functions do not need to have a pointer to
the repo, only to remote_state.

So, patches 2-4 look good. I think they should be combined, but I'm OK
if they're left separate too.

[PATCH v4 5/6] remote: die if branch is not found in repository

[Glen Choo]

In a subsequent commit, we would like external-facing functions to be
able to accept "struct repository" and "struct branch" as a pair. This
is useful for functions like pushremote_for_branch(), which need to take
values from the remote_state and branch, even if branch == NULL.
However, a caller may supply an unrelated repository and branch, which
is not supported behavior.

To prevent misuse, add a die_on_missing_branch() helper function that
dies if a given branch is not from a given repository. Speed up the
existence check by using a new branches_hash hashmap to remote_state,
and use the hashmap to remove the branch array iteration in
make_branch().

Like read_config(), die_on_missing_branch() is only called from
non-static functions; static functions are less prone to misuse because
they have strong conventions for keeping remote_state and branch in
sync.

Signed-off-by: Glen Choo <chooglen@google.com>
---
 remote.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++-----
 remote.h |  2 ++
 2 files changed, 69 insertions(+), 6 deletions(-)

diff --git a/remote.c b/remote.c
index 88697e7e6d..2b5166dee0 100644
--- a/remote.c
+++ b/remote.c
@@ -166,17 +166,66 @@ static void add_merge(struct branch *branch, const char *name)
 	branch->merge_name[branch->merge_nr++] = name;
 }
 
+struct branches_hash_key {
+	const char *str;
+	int len;
+};
+
+static int branches_hash_cmp(const void *unused_cmp_data,
+			     const struct hashmap_entry *eptr,
+			     const struct hashmap_entry *entry_or_key,
+			     const void *keydata)
+{
+	const struct branch *a, *b;
+	const struct branches_hash_key *key = keydata;
+
+	a = container_of(eptr, const struct branch, ent);
+	b = container_of(entry_or_key, const struct branch, ent);
+
+	if (key)
+		return strncmp(a->name, key->str, key->len) ||
+		       a->name[key->len];
+	else
+		return strcmp(a->name, b->name);
+}
+
+static struct branch *find_branch(struct remote_state *remote_state,
+				  const char *name, size_t len)
+{
+	struct branches_hash_key lookup;
+	struct hashmap_entry lookup_entry, *e;
+
+	if (!len)
+		len = strlen(name);
+
+	lookup.str = name;
+	lookup.len = len;
+	hashmap_entry_init(&lookup_entry, memhash(name, len));
+
+	e = hashmap_get(&remote_state->branches_hash, &lookup_entry, &lookup);
+	if (e)
+		return container_of(e, struct branch, ent);
+
+	return NULL;
+}
+
+static void die_on_missing_branch(struct repository *repo,
+				  struct branch *branch)
+{
+	/* branch == NULL is always valid because it represents detached HEAD. */
+	if (branch &&
+	    branch != find_branch(repo->remote_state, branch->name, 0))
+		die("branch %s was not found in the repository", branch->name);
+}
+
 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 < 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];
-	}
+	ret = find_branch(remote_state, name, len);
+	if (ret)
+		return ret;
 
 	ALLOC_GROW(remote_state->branches, remote_state->branches_nr + 1,
 		   remote_state->branches_alloc);
@@ -185,6 +234,9 @@ static struct branch *make_branch(struct remote_state *remote_state,
 	ret->name = xstrndup(name, len);
 	ret->refname = xstrfmt("refs/heads/%s", ret->name);
 
+	hashmap_entry_init(&ret->ent, memhash(name, len));
+	if (hashmap_put_entry(&remote_state->branches_hash, ret, ent))
+		BUG("hashmap_put overwrote entry after hashmap_get returned NULL");
 	return ret;
 }
 
@@ -500,6 +552,8 @@ static const char *remotes_remote_for_branch(struct remote_state *remote_state,
 const char *remote_for_branch(struct branch *branch, int *explicit)
 {
 	read_config(the_repository);
+	die_on_missing_branch(the_repository, branch);
+
 	return remotes_remote_for_branch(the_repository->remote_state, branch,
 					 explicit);
 }
@@ -524,6 +578,8 @@ remotes_pushremote_for_branch(struct remote_state *remote_state,
 const char *pushremote_for_branch(struct branch *branch, int *explicit)
 {
 	read_config(the_repository);
+	die_on_missing_branch(the_repository, branch);
+
 	return remotes_pushremote_for_branch(the_repository->remote_state,
 					     branch, explicit);
 }
@@ -534,6 +590,8 @@ static struct remote *remotes_remote_get(struct remote_state *remote_state,
 const char *remote_ref_for_branch(struct branch *branch, int for_push)
 {
 	read_config(the_repository);
+	die_on_missing_branch(the_repository, branch);
+
 	if (branch) {
 		if (!for_push) {
 			if (branch->merge_nr) {
@@ -1879,6 +1937,8 @@ static const char *branch_get_push_1(struct remote_state *remote_state,
 const char *branch_get_push(struct branch *branch, struct strbuf *err)
 {
 	read_config(the_repository);
+	die_on_missing_branch(the_repository, branch);
+
 	if (!branch)
 		return error_buf(err, _("HEAD does not point to a branch"));
 
@@ -2652,6 +2712,7 @@ struct remote_state *remote_state_new(void)
 	memset(r, 0, sizeof(*r));
 
 	hashmap_init(&r->remotes_hash, remotes_hash_cmp, NULL, 0);
+	hashmap_init(&r->branches_hash, branches_hash_cmp, NULL, 0);
 	return r;
 }
 
diff --git a/remote.h b/remote.h
index 85a730b8ef..b205830ac3 100644
--- a/remote.h
+++ b/remote.h
@@ -46,6 +46,7 @@ struct remote_state {
 	struct branch **branches;
 	int branches_alloc;
 	int branches_nr;
+	struct hashmap branches_hash;
 
 	struct branch *current_branch;
 	const char *pushremote_name;
@@ -292,6 +293,7 @@ int remote_find_tracking(struct remote *remote, struct refspec_item *refspec);
  * branch_get(name) for "refs/heads/{name}", or with branch_get(NULL) for HEAD.
  */
 struct branch {
+	struct hashmap_entry ent;
 
 	/* The short name of the branch. */
 	const char *name;
-- 
2.33.GIT

Re: [PATCH v4 5/6] remote: die if branch is not found in repository

[Jonathan Tan]

> In a subsequent commit, we would like external-facing functions to be
> able to accept "struct repository" and "struct branch" as a pair. This
> is useful for functions like pushremote_for_branch(), which need to take
> values from the remote_state and branch, even if branch == NULL.
> However, a caller may supply an unrelated repository and branch, which
> is not supported behavior.
> 
> To prevent misuse, add a die_on_missing_branch() helper function that
> dies if a given branch is not from a given repository. Speed up the
> existence check by using a new branches_hash hashmap to remote_state,
> and use the hashmap to remove the branch array iteration in
> make_branch().
> 
> Like read_config(), die_on_missing_branch() is only called from
> non-static functions; static functions are less prone to misuse because
> they have strong conventions for keeping remote_state and branch in
> sync.

This makes sense, but how often would this be called? Couldn't we just
iterate over the array (instead of making a hashmap)? If speed is
important, I think we could just sort the array and do a binary search.

Re: [PATCH v4 5/6] remote: die if branch is not found in repository

[Glen Choo]

Jonathan Tan <jonathantanmy@google.com> writes:

>> To prevent misuse, add a die_on_missing_branch() helper function that
>> dies if a given branch is not from a given repository. Speed up the
>> existence check by using a new branches_hash hashmap to remote_state,
>> and use the hashmap to remove the branch array iteration in
>> make_branch().
> This makes sense, but how often would this be called?

This affects most of remote.h branches API (branch_get(),
branch_get_upstream(), branch_get_push()). From what I can tell, I don't
think these are called very frequently.

> Couldn't we just iterate over the array (instead of making a hashmap)?
> If speed is important, I think we could just sort the array and do a
> binary search.

The primary reason I used a hashmap is to be consistent with struct
remote (which also uses a hashmap). One possible argument in your favor
is that remotes are often looked up by name often (and justify the
hashmap), whereas branches are not looked up by name as often (and don't
justify a hashmap).

I say _justify_, but I don't see significant drawbacks to using a
hashmap here. I suspect that there is an advantage to binary search that
you haven't made explicit yet? Could you share your thought process to
help inform the decision?

Re: [PATCH v4 5/6] remote: die if branch is not found in repository

[Jonathan Tan]

Glen Choo <chooglen@google.com> writes:
> > Couldn't we just iterate over the array (instead of making a hashmap)?
> > If speed is important, I think we could just sort the array and do a
> > binary search.
> 
> The primary reason I used a hashmap is to be consistent with struct
> remote (which also uses a hashmap). One possible argument in your favor
> is that remotes are often looked up by name often (and justify the
> hashmap), whereas branches are not looked up by name as often (and don't
> justify a hashmap).
> 
> I say _justify_, but I don't see significant drawbacks to using a
> hashmap here. I suspect that there is an advantage to binary search that
> you haven't made explicit yet? Could you share your thought process to
> help inform the decision?

The main drawback is that branches are now stored in 2 ways - as the
"branches" array in struct remote_state and as this new "branches_hash".
I think we should avoid storing the same data twice unless we really
need to, and I don't think there is a need here.

As for hashmap vs array (say, if we were thinking of removing the array
and putting in a hashmap instead), I would still prefer the array
(sorted if needed) just for the simplicity, but I wouldn't feel as
strongly about this since there is no duplication here.

[PATCH v4 6/6] remote: add struct repository parameter to external functions

[Glen Choo]

Finish plumbing remote_state by adding a struct repository
parameter to repo_* functions. This removes all references to
the_repository->remote_state in remote.c - explicit references in the
function bodies and implicit references via calls to non-repo_*
functions. However, certain functions still use the_repository to parse
oids.

Where a non-static function has the option to call either the static
"remotes_*" variant or the non-static "repo_*" variant, use "remotes_*"
in order to be consistent with the static functions. A desirable
side-effect of this is that it minimizes the number of calls to
read_config().

Signed-off-by: Glen Choo <chooglen@google.com>
---
 remote.c | 85 ++++++++++++++++++++++++++++----------------------------
 remote.h | 80 ++++++++++++++++++++++++++++++++++++++++++----------
 2 files changed, 109 insertions(+), 56 deletions(-)

diff --git a/remote.c b/remote.c
index 2b5166dee0..f7d9a47faa 100644
--- a/remote.c
+++ b/remote.c
@@ -549,13 +549,13 @@ static const char *remotes_remote_for_branch(struct remote_state *remote_state,
 	return "origin";
 }
 
-const char *remote_for_branch(struct branch *branch, int *explicit)
+const char *repo_remote_for_branch(struct repository *repo, struct branch *branch, int *explicit)
 {
-	read_config(the_repository);
-	die_on_missing_branch(the_repository, branch);
+	read_config(repo);
+	die_on_missing_branch(repo, branch);
 
-	return remotes_remote_for_branch(the_repository->remote_state, branch,
-					 explicit);
+	return remotes_remote_for_branch(repo->remote_state, branch,
+					     explicit);
 }
 
 static const char *
@@ -575,22 +575,22 @@ remotes_pushremote_for_branch(struct remote_state *remote_state,
 	return remotes_remote_for_branch(remote_state, branch, 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)
 {
-	read_config(the_repository);
-	die_on_missing_branch(the_repository, branch);
+	read_config(repo);
+	die_on_missing_branch(repo, branch);
 
-	return remotes_pushremote_for_branch(the_repository->remote_state,
+	return remotes_pushremote_for_branch(repo->remote_state,
 					     branch, explicit);
 }
 
 static struct remote *remotes_remote_get(struct remote_state *remote_state,
 					 const char *name);
 
-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)
 {
-	read_config(the_repository);
-	die_on_missing_branch(the_repository, branch);
+	read_config(repo);
+	die_on_missing_branch(repo, branch);
 
 	if (branch) {
 		if (!for_push) {
@@ -600,10 +600,10 @@ const char *remote_ref_for_branch(struct branch *branch, int for_push)
 		} else {
 			const char *dst,
 				*remote_name = remotes_pushremote_for_branch(
-					the_repository->remote_state, branch,
+					repo->remote_state, branch,
 					NULL);
 			struct remote *remote = remotes_remote_get(
-				the_repository->remote_state, remote_name);
+				repo->remote_state, remote_name);
 
 			if (remote && remote->push.nr &&
 			    (dst = apply_refspecs(&remote->push,
@@ -650,10 +650,10 @@ remotes_remote_get(struct remote_state *remote_state, const char *name)
 				    remotes_remote_for_branch);
 }
 
-struct remote *remote_get(const char *name)
+struct remote *repo_remote_get(struct repository *repo, const char *name)
 {
-	read_config(the_repository);
-	return remotes_remote_get(the_repository->remote_state, name);
+	read_config(repo);
+	return remotes_remote_get(repo->remote_state, name);
 }
 
 static inline struct remote *
@@ -663,10 +663,10 @@ remotes_pushremote_get(struct remote_state *remote_state, const char *name)
 				    remotes_pushremote_for_branch);
 }
 
-struct remote *pushremote_get(const char *name)
+struct remote *repo_pushremote_get(struct repository *repo, const char *name)
 {
-	read_config(the_repository);
-	return remotes_pushremote_get(the_repository->remote_state, name);
+	read_config(repo);
+	return remotes_pushremote_get(repo->remote_state, name);
 }
 
 int remote_is_configured(struct remote *remote, int in_repo)
@@ -678,14 +678,14 @@ 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);
-	for (i = 0; i < the_repository->remote_state->remotes_nr && !result;
+	read_config(repo);
+	for (i = 0; i < repo->remote_state->remotes_nr && !result;
 	     i++) {
 		struct remote *remote =
-			the_repository->remote_state->remotes[i];
+			repo->remote_state->remotes[i];
 		if (!remote)
 			continue;
 		result = fn(remote, priv);
@@ -1790,17 +1790,16 @@ static void set_merge(struct remote_state *remote_state, 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);
+	read_config(repo);
 	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(the_repository->remote_state, ret);
+		ret = make_branch(repo->remote_state, name, strlen(name));
+	set_merge(repo->remote_state, ret);
 	return ret;
 }
 
@@ -1934,17 +1933,17 @@ static const char *branch_get_push_1(struct remote_state *remote_state,
 	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)
 {
-	read_config(the_repository);
-	die_on_missing_branch(the_repository, branch);
+	read_config(repo);
+	die_on_missing_branch(repo, branch);
 
 	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(
-			the_repository->remote_state, branch, err);
+			repo->remote_state, branch, err);
 	return branch->push_tracking_ref;
 }
 
@@ -2197,15 +2196,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) :
+				branch_get_upstream(branch, NULL);
 	if (tracking_name)
 		*tracking_name = base;
 	if (!base)
@@ -2217,15 +2217,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 b205830ac3..9b06afd285 100644
--- a/remote.h
+++ b/remote.h
@@ -120,15 +120,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);
 
@@ -324,10 +337,29 @@ 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);
@@ -351,7 +383,11 @@ 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 {
@@ -370,11 +406,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.GIT

Re: [PATCH v4 6/6] remote: add struct repository parameter to external functions

[Jonathan Tan]

> Finish plumbing remote_state by adding a struct repository
> parameter to repo_* functions. This removes all references to
> the_repository->remote_state in remote.c - explicit references in the
> function bodies and implicit references via calls to non-repo_*
> functions. However, certain functions still use the_repository to parse
> oids.
> 
> Where a non-static function has the option to call either the static
> "remotes_*" variant or the non-static "repo_*" variant, use "remotes_*"
> in order to be consistent with the static functions. A desirable
> side-effect of this is that it minimizes the number of calls to
> read_config().

I don't feel as confident about this patch as I do about the patches
that remove the global variables - as you said, some functions might use
the_repository to parse OIDs (or do other things). I think that part of
this patch should go into the next series you have planned (as you
describe in the original cover letter [1], when you make future
submodule commands be run in-process), so that they can be tested.
(Removing the global variables was more of a mechanical code change, so
it was easier to see what's going on by just looking at "before" and
"after".)

Overall this patch series looks good with only minor comments from me,
except that I think that this patch (patch 6) should be dropped.

[1] https://lore.kernel.org/git/pull.1103.git.git.1633633635.gitgitgadget@gmail.com/

Re: [PATCH v4 6/6] remote: add struct repository parameter to external functions

[Glen Choo]

Jonathan Tan <jonathantanmy@google.com> writes:

> I don't feel as confident about this patch as I do about the patches
> that remove the global variables - as you said, some functions might use
> the_repository to parse OIDs (or do other things). I think that part of
> this patch should go into the next series you have planned (as you
> describe in the original cover letter [1], when you make future
> submodule commands be run in-process), so that they can be tested.
> (Removing the global variables was more of a mechanical code change, so
> it was easier to see what's going on by just looking at "before" and
> "after".)

Thanks! I think that thematically, this patch belongs in this series.
But that is not worth leaving the code untested, so I will move this
patch into my future series.

Re: [PATCH v4 0/6] remote: replace static variables with struct remote_state

[Glen Choo]

Hi everyone! I'd appreciate thoughts on the design presented here. I
think Junio and I have explored a good part of the short-term design
space, but it would be great for others to chime in - as Junio mentioned
in [1], this series isn't exactly sufficiently reviewed yet.

For whoever is taking a look, here are some pieces that may be worth
paying special attention to:

* [2] Junio expresses concern that struct branch depends on struct
  repository and accepting both of them as parameters might be unsafe. A
  back pointer from branch->repository might solve this.
  
* [3] Glen and Junio discover that branch_get() can accept NULL and return
  NULL. accepting NULL means "give me the current branch", returning
  NULL means "detached HEAD".
  
* [4] Glen questions the design of the current API and wonders what an
  optimal API might look like. Junio and Glen agree that we can remove
  global variables now and rethink the API later.
  

[1] https://lore.kernel.org/git/xmqqlf1ujo4g.fsf@gitster.g/
[2] https://lore.kernel.org/git/xmqqr1cnbgmw.fsf@gitster.g/
[3] https://lore.kernel.org/git/xmqqzgqwzvwx.fsf@gitster.g/
[4] https://lore.kernel.org/git/kl6l35om9wn9.fsf@chooglen-macbookpro.roam.corp.google.com/