--- v5
+++ v4
@@ -3,19 +3,36 @@
through child processes. This is accomplished by creating a struct remote_state
and adding it to struct repository.
-Thanks for the kind review, Jonathan. I've incorporated most of the feedback
-from v4, except squashing patches 2-4. My reasoning is that those changes are
-mechanical and keeping them separate makes it more obvious whether or not each
-step has been done correctly. However, I agree that they are just intermediate
-steps that touch the same lines, so I am happy to squash them if other reviewers
-prefer that.
-
-Changes since v4:
-* Fixed an incorrect description of 'git push' without explicit refspecs
-* In patch 5, remove the branches array and keep the hashmap (note that I did
- not make a similar change to remotes)
-* Drop patch 6 because it is untested in this series (will incorporate into a
- future series)
+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
@@ -49,107 +66,293 @@
static variables > the_repository->remote_state.
* Add more instances of repo_* that were missed.
-Glen Choo (5):
+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.c | 368 +++++++++++++++++++++++++++++-------------
- remote.h | 35 ++++
+ 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, 311 insertions(+), 113 deletions(-)
-
-Range-diff against v4:
-1: 9b29ec27c6 ! 1: 7e60457e11 t5516: add test case for pushing remote refspecs
+ 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 ##
- t5516: add test case for pushing remote refspecs
+ remote: use remote_state parameter internally
- - 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.
- + "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.
+ - 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.
- Signed-off-by: Glen Choo <chooglen@google.com>
-
- @@ t/t5516-fetch-push.sh: do
-
- done
-
- -+test_expect_success "push to remote with detached HEAD and config remote.*.push = src:dest" '
- ++test_expect_success "push to remote with no explicit refspec and config remote.*.push = src:dest" '
- + mk_test testrepo heads/main &&
- + git checkout $the_first_commit &&
- + test_config remote.there.url testrepo &&
-2: ca9b5ab66a = 2: ecc637ee74 remote: move static variables into per-repository struct
-3: 5d6a245cae = 3: a915155979 remote: use remote_state parameter internally
-4: 53f2e31f72 = 4: 8ef43570e9 remote: remove the_repository->remote_state from static methods
-5: d3281c14eb ! 5: 8bb7bddda4 remote: die if branch is not found in repository
+ As a result, external-facing functions are still tied to the_repository,
+ but most static functions no longer reference
@@ Commit message
- 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().
- + existence check by replacing the branches list with a branches_hash
- + hashmap.
-
- Like read_config(), die_on_missing_branch() is only called from
- non-static functions; static functions are less prone to misuse because
+ ## 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)
- + if (ret)
- + return ret;
-
- - ALLOC_GROW(remote_state->branches, remote_state->branches_nr + 1,
- - remote_state->branches_alloc);
- -@@ remote.c: static struct branch *make_branch(struct remote_state *remote_state,
- +- ALLOC_GROW(remote_state->branches, remote_state->branches_nr + 1,
- +- remote_state->branches_alloc);
- + CALLOC_ARRAY(ret, 1);
- +- remote_state->branches[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);
-
- @@ remote.c: struct remote_state *remote_state_new(void)
- return r;
- }
-
- +@@ remote.c: void remote_state_clear(struct remote_state *remote_state)
- + 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;
- ++ hashmap_clear_and_free(&remote_state->branches_hash, struct remote, ent);
+ -+ 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);
+ }
-
- ## remote.h ##
- @@ remote.h: struct remote_state {
- - struct branch **branches;
- - int branches_alloc;
- - int branches_nr;
- + int remotes_nr;
- + struct hashmap remotes_hash;
- +
- +- struct branch **branches;
- +- int branches_alloc;
- +- int branches_nr;
- + struct hashmap branches_hash;
-
- struct branch *current_branch;
-6: 0974994cc6 < -: ---------- remote: add struct repository parameter to external functions
+ +
+ +-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