remote: replace static variables with struct remote_state
cover remote: replace static variables with struct remote_state
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/[email protected]/
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
Signature · 1 line
gitgitgadget
PATCH 1/2 remote: move static variables into struct
From: Glen Choo <[email protected]>
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.
--- remote.c | 109 ++++++++++++++++++++++++++++++++----------------------- 1 file changed, 63 insertions(+), 46 deletions(-)
remote.c +63 -46
@@ -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);
Signature · 1 line
gitgitgadget
PATCH 2/2 remote: add remote_state to struct repository
In reply to Glen Choo via GitGitGadgetFrom: Glen Choo <[email protected]>
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.
--- remote.c | 186 ++++++++++++++++++++++++++++++--------------------- remote.h | 69 +++++++++++++++++-- repository.c | 8 +++ repository.h | 4 ++ 4 files changed, 184 insertions(+), 83 deletions(-)
remote.c +108 -78
@@ -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 ; +}
remote.h +64 -5
94 lines not on this page · raw message
repository.c +8 -0
@@ -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)
repository.h +4 -0
@@ -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;
Signature · 1 line
gitgitgadget
Re: [PATCH 1/2] remote: move static variables into struct
In reply to Glen Choo via GitGitGadgetQuoted 5 lines · Glen Choo via GitGitGadget
From: Glen Choo <[email protected]>
remote.conly works withthe_repositorybecause 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.
Re: [PATCH 2/2] remote: add remote_state to struct repository
In reply to Glen Choo via GitGitGadgetremote.c static void add_pushurl(struct remote *remote, const char *pushurl) · line 70
Quoted 4 lines · Glen Choo via GitGitGadget
remote.c +2
-1-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?
Quoted 16 lines
remote.c +3
-2-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));
remote.c static struct remote *make_remote(const char *name, int len) · line 145
Quoted 1 line
remote.c +0
-0@@ -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
Quoted 9 lines · Junio C Hamano
remote.c static void add_pushurl(struct remote *remote, const char *pushurl) · line 70
Quoted 4 lines
remote.c +2
-1-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.
Quoted 2 lines
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.
Quoted 2 lines
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.
v2 cover remote: replace static variables with struct remote_state
In reply to Glen Choo via GitGitGadgetThis 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/[email protected]/
[2] https://lore.kernel.org/git/[email protected]/
[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
Signature · 1 line
2.33.0.882.g93a45727a2-goog
PATCH v2 1/3 remote: move static variables into per-repository struct
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.
--- remote.c | 187 ++++++++++++++++++++++++++++++++++----------------- remote.h | 34 ++++++++++ repository.c | 8 +++ repository.h | 4 ++ 4 files changed, 171 insertions(+), 62 deletions(-)
remote.c +125 -62
@@ -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 ; +}
remote.h +34 -0
@@ -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;
repository.c +8 -0
@@ -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)
repository.h +4 -0
@@ -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;
Signature · 1 line
2.33.0.882.g93a45727a2-goog
PATCH v2 2/3 remote: use remote_state parameter internally
In reply to Glen ChooIn 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().
--- remote.c | 165 ++++++++++++++++++++++++++----------------------------- 1 file changed, 78 insertions(+), 87 deletions(-)
remote.c +78 -87
@@ -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; }
Signature · 1 line
2.33.0.882.g93a45727a2-goog
PATCH v2 3/3 remote: add struct repository parameter to external functions
In reply to Glen ChooFinish 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.
--- remote.c | 111 ++++++++++++++++++++++++++++++------------------------- remote.h | 96 +++++++++++++++++++++++++++++++++++++++-------- 2 files changed, 142 insertions(+), 65 deletions(-)
remote.c +61 -50
@@ -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 ;
remote.h +81 -15
131 lines not on this page · raw message
Signature · 1 line
2.33.0.882.g93a45727a2-goog
Quoted 16 lines · Glen Choo
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 structremote_stateso 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.
Quoted 7 lines
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 1/3] remote: move static variables into per-repository struct
In reply to Glen ChooQuoted 7 lines · Glen Choo
remote.cdoes 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_statethat holds the remotes subsystem state and move the static variables ofremote.cintothe_repository->remote_state.
That all sounds very sensible, but ...
Quoted 6 lines
remote.h +0
-0@@ -23,6 +23,40 @@ enum {REMOTE_BRANCHES
remote.h enum { · line 33
Quoted 10 lines
remote.h +8
-0}; +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.
remote.h enum { · line 37
Quoted 4 lines
remote.h +4
-0+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.
Quoted 9 lines
remote.h +9
-0+struct remote_state { +int config_loaded; + +struct remote **remotes; +int remotes_alloc; +int remotes_nr; +struct hashmap remotes_hash; + +struct branch **branches;
remote.h enum { · line 57
Quoted 10 lines
remote.h +10
-0+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 2/3] remote: use remote_state parameter internally
In reply to Glen ChooQuoted 3 lines · Glen Choo
In internal-facing functions, replace
the_repository->remote_statewith a structremote_stateparameter, but do not change external-facing functions.
OK. Quite nice.
Re: [PATCH v2 3/3] remote: add struct repository parameter to external functions
In reply to Glen ChooQuoted 4 lines · Glen Choo
Finish plumbing
remote_stateby adding a struct repository parameter to repo_* functions. While this removes all references tothe_repository->remote_state, certain functions still usethe_repositoryto parse oids.
OK.
Quoted 7 lines · Junio C Hamano
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.
Quoted 22 lines · Junio C Hamano
Quoted 7 lines
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.
Quoted 7 lines
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.
Quoted 2 lines · Junio C Hamano
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...
Quoted 2 lines
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 :)
Quoted 3 lines · Glen Choo
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?
Quoted 6 lines
...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.
Quoted 3 lines
* Interface misuse, where callers can now dereference
branch->repositoryeven 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.
Quoted 1 line
* 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).
Quoted 2 lines
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.
Quoted 4 lines · Junio C Hamano
IOW, we need to start somewhere to clean things up, so either we teach multi-repository support to
remote.cfrom 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.
Quoted 4 lines
...I suspect that this piece is isolated
enough that it is simpler to use as a starting point, and then the callers intoremote.cAPI can then be cleaned up, gradually extending the circle.
Agree that this piece is a good place to start such work.
Quoted 6 lines
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.cAPI always belongs to one and only one repository, then following thebranch->repositorypointer 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.
Quoted 3 lines
...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.
Quoted 6 lines
Quoted 2 lines
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.
Re: [PATCH v2 1/3] remote: move static variables into per-repository struct
In reply to Junio C HamanoQuoted 10 lines · Junio C Hamano
remote.h enum { · line 33
Quoted 8 lines
remote.h +8
-0+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!
Quoted 10 lines
remote.h enum { · line 37
Quoted 5 lines
remote.h +4
-0+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.
Quoted 1 line
...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".
Quoted 13 lines
Quoted 13 lines
remote.h +13
-0+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;
Quoted 8 lines
remote.h enum { · line 57
Quoted 6 lines
remote.h +6
-0+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
Quoted 3 lines · Glen Choo
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.
v3 cover remote: replace static variables with struct remote_state
In reply to Glen ChooThis 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/[email protected]/
[2] https://lore.kernel.org/git/[email protected]/
[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 functionsSignature · 1 line
2.33.GIT
PATCH v3 1/4 remote: move static variables into per-repository struct
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.
--- remote.c | 187 ++++++++++++++++++++++++++++++++++----------------- remote.h | 34 ++++++++++ repository.c | 8 +++ repository.h | 4 ++ 4 files changed, 171 insertions(+), 62 deletions(-)
remote.c +125 -62
@@ -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 ; +}
remote.h +34 -0
@@ -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;
repository.c +8 -0
@@ -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)
repository.h +4 -0
@@ -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;
Signature · 1 line
2.33.GIT
PATCH v3 2/4 remote: use remote_state parameter internally
In reply to Glen ChooIntroduce 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().
--- remote.c | 153 ++++++++++++++++++++++++++----------------------------- remote.h | 10 ++++ 2 files changed, 81 insertions(+), 82 deletions(-)
remote.c +71 -82
@@ -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; }
remote.h +10 -0
@@ -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);
Signature · 1 line
2.33.GIT
PATCH v3 3/4 remote: remove the_repository->remote_state from static methods
In reply to Glen ChooReplace 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.
--- remote.c | 94 ++++++++++++++++++++++++++++++++------------------------ 1 file changed, 53 insertions(+), 41 deletions(-)
remote.c +53 -41
@@ -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"),
Signature · 1 line
2.33.GIT
PATCH v3 4/4 remote: add struct repository parameter to external functions
In reply to Glen ChooFinish 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.
--- remote.c | 29 ++++++++++++++--------------- remote.h | 28 +++++++++++++++++++++++----- 2 files changed, 37 insertions(+), 20 deletions(-)
remote.c +14 -15
@@ -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; }
remote.h +23 -5
@@ -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);
Signature · 1 line
2.33.GIT
Re: [PATCH v3 2/4] remote: use remote_state parameter internally
In reply to Glen ChooQuoted 13 lines · Glen Choo
Introduce a struct
remote_statemember to structs that need to 'remember' theirremote_state. Without changing external-facing functions, replacethe_repository->remote_stateinternally by using theremote_statemember where it is applicable i.e. when a function accepts a struct that depends on theremote_state. If it is not applicable, add a structremote_stateparameter instead.As a result, external-facing functions are still tied to
the_repository, but most static functions no longer referencethe_repository->remote_state. The exceptions are those that are used in a way that depends on external-facing functions e.g. the callbacks toremote_get_1().
Quoted 5 lines
Signed-off-by Glen Choo <[email protected]>--- 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
Quoted 4 lines · Junio C Hamano
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
Quoted 7 lines · Junio C Hamano
In any case, any public facing entry point, like
pushremote_get()that is directly called fromcmd_push()with just a name, should auto vivify an instance of structremote_stateand populate its members as needed, I think, and in this particular case, I suspect that it forgets to initialize thecurrent_branchand other members by callingread_config(), just like other entry points likerepo_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
In reply to Junio C HamanoSorry I wasn't able to get to this sooner, this will be my first priority.
Quoted 4 lines · Junio C Hamano
Quoted 4 lines
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?
Quoted 30 lines
(gdb) bt
#00x000055555579a785inpushremote_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
0x000055555565b2aein main (argc=6, argv=0x7fffffffdc58) at common-main.c:52The 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) {
Quoted 6 lines
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 calledread_config(), but otherwise shouldn't be.
Thanks for helping to narrow the scope of the search :)
Quoted 5 lines
I think somebody is not setting up the
remote_statecorrectly? When the user wants to just use the repository-wide pushremote, not having thecurrent_branchwould 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.
Quoted 7 lines
In any case, any public facing entry point, like
pushremote_get()that is directly called fromcmd_push()with just a name, should auto vivify an instance of structremote_stateand populate its members as needed, I think, and in this particular case, I suspect that it forgets to initialize thecurrent_branchand other members by callingread_config(), just like other entry points likerepo_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
Quoted 3 lines · Glen Choo
Quoted 3 lines
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 calledread_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)
Quoted 11 lines
Quoted 11 lines
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) {
Quoted 3 lines
Quoted 3 lines
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
Quoted 11 lines · Glen Choo
Quoted 3 lines · Glen Choo
Quoted 3 lines
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 calledread_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.
Quoted 2 lines
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
Quoted 2 lines · Junio C Hamano
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.
Quoted 8 lines
Quoted 2 lines
"
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_stateavailable 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 theremote_state, yet the function needs access toremote_stateeven 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.
Quoted 8 lines
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?
Quoted 8 lines
If the answer is yes, then the function need to take both branch and
remote_stateas 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 theremote_stateandremote_state->branchbeing 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
Quoted 18 lines · Glen Choo
Quoted 8 lines
If the answer is yes, then the function need to take both branch and
remote_stateas 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 theremote_stateandremote_state->branchbeing 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 ofbranch_get()is the NULL branch. A NULL branch is not usable -branch_get_push()andbranch_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()).
Quoted 8 lines
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_statebackpointer, 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.
Quoted 5 lines
b) Keep the backpointer, but add
remote_stateas a parameter topushremote_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,...
Quoted 4 lines
c) Replace the backpointer with a
remote_stateparameter. 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
Quoted 7 lines · Junio C Hamano
Quoted 5 lines
a) Represent detached HEAD with a non-NULL "struct branch". This will let us continue using the
remote_statebackpointer, 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
Quoted 11 lines · Junio C Hamano
Quoted 4 lines
c) Replace the backpointer with a
remote_stateparameter. 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_stateas 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.
Quoted 8 lines
Quoted 7 lines
Quoted 5 lines
a) Represent detached HEAD with a non-NULL "struct branch". This will let us continue using the
remote_statebackpointer, 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.
Quoted 11 lines
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
Quoted 10 lines · Glen Choo
Quoted 8 lines · Junio C Hamano
Quoted 4 lines
c) Replace the backpointer with a
remote_stateparameter. 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.
Quoted 6 lines
Quoted 3 lines
That is, pass branch and
remote_stateas 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.
Quoted 5 lines
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
Quoted 11 lines · Junio C Hamano
Quoted 6 lines · Glen Choo
Quoted 4 lines · Junio C Hamano
Quoted 4 lines
c) Replace the backpointer with a
remote_stateparameter. 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_statepointer 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.
Quoted 8 lines
Quoted 5 lines
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
Quoted 5 lines · Glen Choo
I might not want to take action on these ideas though, e.g.
branch_get("HEAD")orremote_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.
v4 cover remote: replace static variables with struct remote_state
In reply to Glen ChooThis 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/[email protected]/
[2] https://lore.kernel.org/git/[email protected]
[2] https://lore.kernel.org/git/[email protected]
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
Signature · 1 line
2.33.GIT
PATCH v4 1/6 t5516: add test case for pushing remote refspecs
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.
--- t/t5516-fetch-push.sh | 9 +++++++++ 1 file changed, 9 insertions(+)
t/t5516-fetch-push.sh +9 -0
@@ -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 &&
Signature · 1 line
2.33.GIT
PATCH v4 2/6 remote: move static variables into per-repository struct
In reply to Glen Chooremote.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.
--- remote.c | 191 +++++++++++++++++++++++++++++++++------------------ remote.h | 34 +++++++++ repository.c | 8 +++ repository.h | 4 ++ 4 files changed, 170 insertions(+), 67 deletions(-)
remote.c +124 -67
@@ -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 ; +}
remote.h +34 -0
@@ -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;
repository.c +8 -0
@@ -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)
repository.h +4 -0
@@ -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;
Signature · 1 line
2.33.GIT
PATCH v4 3/6 remote: use remote_state parameter internally
In reply to Glen ChooWithout 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().
--- remote.c | 159 +++++++++++++++++++++++++------------------------------ remote.h | 2 + 2 files changed, 75 insertions(+), 86 deletions(-)
remote.c +73 -86
@@ -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; }
remote.h +2 -0
@@ -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);
Signature · 1 line
2.33.GIT
PATCH v4 4/6 remote: remove the_repository->remote_state from static methods
In reply to Glen ChooReplace 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.
--- remote.c | 96 +++++++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 71 insertions(+), 25 deletions(-)
remote.c +71 -25
@@ -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; }
Signature · 1 line
2.33.GIT
PATCH v4 5/6 remote: die if branch is not found in repository
In reply to Glen ChooIn 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.
--- remote.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++----- remote.h | 2 ++ 2 files changed, 69 insertions(+), 6 deletions(-)
remote.c +67 -6
@@ -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; }
remote.h +2 -0
@@ -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;
Signature · 1 line
2.33.GIT
PATCH v4 6/6 remote: add struct repository parameter to external functions
In reply to Glen ChooFinish 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().
--- remote.c | 85 ++++++++++++++++++++++++++++---------------------------- remote.h | 80 ++++++++++++++++++++++++++++++++++++++++++---------- 2 files changed, 109 insertions(+), 56 deletions(-)
remote.c +43 -42
@@ -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 ;
remote.h +66 -14
109 lines not on this page · raw message
Signature · 1 line
2.33.GIT
Re: [PATCH v4 1/6] t5516: add test case for pushing remote refspecs
In reply to Glen ChooQuoted 3 lines · 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.
Makes sense.
Quoted 26 lines
Signed-off-by Glen Choo <[email protected]>--- t/t5516-fetch-push.sh | 9 +++++++++ 1 file changed, 9 insertions(+)t/t5516-fetch-push.sh +9
-0@@ -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 &&
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/[email protected]/
[2] https://lore.kernel.org/git/[email protected]/
[3] https://lore.kernel.org/git/[email protected]/
[4] https://lore.kernel.org/git/[email protected]/
Re: [PATCH v4 1/6] t5516: add test case for pushing remote refspecs
In reply to Glen ChooQuoted 3 lines
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 4/6] remote: remove the_repository->remote_state from static methods
In reply to Glen ChooQuoted 8 lines
Replace all remaining references of
the_repository->remote_statein static functions with a structremote_stateparameter.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 structremote_stateinstead 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.
Re: [PATCH v4 1/6] t5516: add test case for pushing remote refspecs
In reply to Jonathan TanQuoted 6 lines · Jonathan Tan
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.
Re: [PATCH v4 6/6] remote: add struct repository parameter to external functions
In reply to Jonathan TanQuoted 9 lines · Jonathan Tan
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_repositoryto 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 5/6] remote: die if branch is not found in repository
In reply to Jonathan TanQuoted 6 lines · Jonathan Tan
Quoted 5 lines
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 newbranches_hashhashmap toremote_state, and use the hashmap to remove the branch array iteration inmake_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.
Quoted 3 lines
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 6/6] remote: add struct repository parameter to external functions
In reply to Glen ChooQuoted 7 lines
Finish plumbing
remote_stateby adding a struct repository parameter to repo_* functions. This removes all references tothe_repository->remote_stateinremote.c- explicit references in the function bodies and implicit references via calls to non-repo_* functions. However, certain functions still usethe_repositoryto parse oids.
Quoted 5 lines
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.
Re: [PATCH v4 5/6] remote: die if branch is not found in repository
In reply to Glen ChooQuoted 13 lines
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 theremote_stateand 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 newbranches_hashhashmap toremote_state, and use the hashmap to remove the branch array iteration inmake_branch().
Quoted 4 lines
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 keepingremote_stateand 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
In reply to Glen ChooQuoted 10 lines · Glen Choo
Quoted 3 lines
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).
Quoted 4 lines
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.