Re: [PATCH v2 1/4] promisor-remote: read partialClone config here
flat view
From: Elijah Newren <hidden>
Date: 2021-06-08 17:29:23
On Mon, Jun 7, 2021 at 5:26 PM Jonathan Tan [off-list ref] wrote:
quoted hunk ↗ jump to hunk
Currently, the reading of config related to promisor remotes is done in two places: once in setup.c (which sets the global variable repository_format_partial_clone, to be read by the code in promisor-remote.c), and once in promisor-remote.c. This means that care must be taken to ensure that repository_format_partial_clone is set before any code in promisor-remote.c accesses it. To simplify the code, move all such config reading to promisor-remote.c. By doing this, it will be easier to see when repository_format_partial_clone is written and, thus, to reason about the code. This will be especially helpful in a subsequent commit, which modifies this code. Signed-off-by: Jonathan Tan <redacted> --- cache.h | 1 - promisor-remote.c | 14 +++++++++----- promisor-remote.h | 6 ------ setup.c | 10 +++++++--- 4 files changed, 16 insertions(+), 15 deletions(-)diff --git a/cache.h b/cache.h index ba04ff8bd3..dbdcec8601 100644 --- a/cache.h +++ b/cache.h@@ -1061,7 +1061,6 @@ extern int repository_format_worktree_config; struct repository_format { int version; int precious_objects; - char *partial_clone; /* value of extensions.partialclone */ int worktree_config; int is_bare; int hash_algo;diff --git a/promisor-remote.c b/promisor-remote.c index da3f2ca261..c0e5061dfe 100644 --- a/promisor-remote.c +++ b/promisor-remote.c@@ -7,11 +7,6 @@ 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)@@ -99,6 +94,15 @@ static int promisor_remote_config(const char *var, const char *value, void *data size_t namelen; const char *subkey; + if (!strcmp(var, "extensions.partialclone")) { + /* + * NULL value is handled in handle_extension_v0 in setup.c. + */ + if (value) + repository_format_partial_clone = xstrdup(value); + return 0; + }
This is actually slightly hard to parse out. I was trying to figure
out where repository_format_partial_clone was initialized, and it's
not handled when value is NULL in handle_extension_v0; it's the fact
that repository_format_partial_clone is declared a static global
variable.
But in the next patch you make it a member of struct
promisor_remote_config, and instead rely on the xcalloc call in
promisor_remote_init().
That means everything is properly initialized and you haven't made any
mistakes here, but the logic is a bit hard to follow. Perhaps it'd be
nicer to just write this as
+ if (!strcmp(var, "extensions.partialclone")) {
+ repository_format_partial_clone = xstrdup_or_null(value);
+ return 0;
+ }
which makes the code shorter and easier to follow, at least for me.