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

[Glen Choo <chooglen@google.com>]

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