On Mon, Jun 7, 2021 at 5:26 PM Jonathan Tan <jonathantanmy@xxxxxxxxxx> wrote:
>
> 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 <jonathantanmy@xxxxxxxxxx>
> ---
> 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.
