Re: [PATCH v3 1/5] repository: move global r_f_p_c to repo struct
From: Elijah Newren <hidden>
Date: 2021-06-10 20:47:20
On Thu, Jun 10, 2021 at 10:35 AM Jonathan Tan [off-list ref] wrote:
quoted hunk ↗ jump to hunk
Move repository_format_partial_clone, which is currently a global variable, into struct repository. (Full support for per-repository partial clone config will be done in a subsequent commit - this is split into its own commit because of the extent of the changes needed.) The new repo-specific variable cannot be set in check_repository_format_gently() (as is currently), because that function does not know which repo it is operating on (or even whether the value is important); therefore this responsibility is delegated to the outermost caller that knows. Of all the outermost callers that know (found by looking at all functions that call clear_repository_format()), I looked at those that either read from the main Git directory or write into a struct repository. These callers have been modified accordingly (write to the_repository in the former case and write to the given struct repository in the latter case). Signed-off-by: Jonathan Tan <redacted> --- promisor-remote.c | 13 +++---------- promisor-remote.h | 6 ------ repository.c | 3 +++ repository.h | 3 +++ setup.c | 16 +++++++++++----- 5 files changed, 20 insertions(+), 21 deletions(-)diff --git a/promisor-remote.c b/promisor-remote.c index da3f2ca261..d24081dc21 100644 --- a/promisor-remote.c +++ b/promisor-remote.c@@ -5,13 +5,6 @@ #include "transport.h" #include "strvec.h" -static char *repository_format_partial_clone; - -void set_repository_format_partial_clone(char *partial_clone) -{ - repository_format_partial_clone = xstrdup_or_null(partial_clone); -} - static int fetch_objects(const char *remote_name, const struct object_id *oids, int oid_nr)@@ -145,15 +138,15 @@ static void promisor_remote_init(void) git_config(promisor_remote_config, NULL); - if (repository_format_partial_clone) { + if (the_repository->repository_format_partial_clone) { struct promisor_remote *o, *previous; - o = promisor_remote_lookup(repository_format_partial_clone, + o = promisor_remote_lookup(the_repository->repository_format_partial_clone, &previous); if (o) promisor_remote_move_to_tail(o, previous); else - promisor_remote_new(repository_format_partial_clone); + promisor_remote_new(the_repository->repository_format_partial_clone); } }diff --git a/promisor-remote.h b/promisor-remote.h index c7a14063c5..687210ab87 100644 --- a/promisor-remote.h +++ b/promisor-remote.h@@ -32,10 +32,4 @@ int promisor_remote_get_direct(struct repository *repo, const struct object_id *oids, int oid_nr); -/* - * This should be used only once from setup.c to set the value we got - * from the extensions.partialclone config option. - */ -void set_repository_format_partial_clone(char *partial_clone); - #endif /* PROMISOR_REMOTE_H */diff --git a/repository.c b/repository.c index 448cd557d4..4878c297d9 100644 --- a/repository.c +++ b/repository.c@@ -172,6 +172,9 @@ int repo_init(struct repository *repo, repo_set_hash_algo(repo, format.hash_algo); + repo->repository_format_partial_clone = format.partial_clone; + format.partial_clone = NULL;
This was surprising to me, and I tried to dig around to find out why you set it to NULL. AFAICT, you're trying to avoid the need to do a xstrdup(), so you take over ownership in the first line and set to NULL in the second to avoid a double-free. So, it makes sense, but given how surprising it was and it took me a while to figure it out, perhaps it's worth adding a comment that this is what you're doing here? The same comment would also apply in a few other places in this patch...
quoted hunk ↗ jump to hunk
+ if (worktree) repo_set_worktree(repo, worktree);diff --git a/repository.h b/repository.h index a45f7520fd..6fb16ed336 100644 --- a/repository.h +++ b/repository.h@@ -139,6 +139,9 @@ struct repository { /* True if commit-graph has been disabled within this process. */ int commit_graph_disabled; + /* Configurations related to promisor remotes. */ + char *repository_format_partial_clone; + /* Configurations */ /* Indicate if a repository has a different 'commondir' from 'gitdir' */diff --git a/setup.c b/setup.c index 59e2facd9d..fbedfe8e03 100644 --- a/setup.c +++ b/setup.c@@ -468,8 +468,6 @@ static enum extension_result handle_extension_v0(const char *var, data->precious_objects = git_config_bool(var, value); return EXTENSION_OK; } else if (!strcmp(ext, "partialclone")) { - if (!value) - return config_error_nonbool(var); data->partial_clone = xstrdup(value); return EXTENSION_OK; } else if (!strcmp(ext, "worktreeconfig")) {@@ -566,7 +564,6 @@ static int check_repository_format_gently(const char *gitdir, struct repository_ } repository_format_precious_objects = candidate->precious_objects; - set_repository_format_partial_clone(candidate->partial_clone); repository_format_worktree_config = candidate->worktree_config; string_list_clear(&candidate->unknown_extensions, 0); string_list_clear(&candidate->v1_only_extensions, 0);@@ -1193,6 +1190,10 @@ int discover_git_directory(struct strbuf *commondir, return -1; } + the_repository->repository_format_partial_clone = + candidate.partial_clone; + candidate.partial_clone = NULL; +
comment would also be nice here
quoted hunk ↗ jump to hunk
clear_repository_format(&candidate); return 0; }@@ -1300,8 +1301,12 @@ const char *setup_git_directory_gently(int *nongit_ok) gitdir = DEFAULT_GIT_DIR_ENVIRONMENT; setup_git_env(gitdir); } - if (startup_info->have_repository) + if (startup_info->have_repository) { repo_set_hash_algo(the_repository, repo_fmt.hash_algo); + the_repository->repository_format_partial_clone = + repo_fmt.partial_clone; + repo_fmt.partial_clone = NULL;
and here
quoted hunk ↗ jump to hunk
+ } } /* * Since precompose_string_if_needed() needs to look at@@ -1319,7 +1324,6 @@ const char *setup_git_directory_gently(int *nongit_ok) setenv(GIT_PREFIX_ENVIRONMENT, "", 1); } -
why the stray whitespace change?
quoted hunk ↗ jump to hunk
strbuf_release(&dir); strbuf_release(&gitdir); clear_repository_format(&repo_fmt);@@ -1386,6 +1390,8 @@ void check_repository_format(struct repository_format *fmt) check_repository_format_gently(get_git_dir(), fmt, NULL); startup_info->have_repository = 1; repo_set_hash_algo(the_repository, fmt->hash_algo); + the_repository->repository_format_partial_clone = + xstrdup_or_null(fmt->partial_clone); clear_repository_format(&repo_fmt); } --2.32.0.rc1.229.g3e70b5a671-goog
Other than the minor nits above, this looks good.