mbox series

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

Message ID 20211019224339.61881-1-chooglen@google.com (mailing list archive)
Headers show
Series remote: replace static variables with struct remote_state | expand

Message

Glen Choo Oct. 19, 2021, 10:43 p.m. UTC 
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