Re: [PATCH v2 1/4] promisor-remote: read partialClone config here

From: Elijah Newren <newren@gmail.com>
Date: Jun 8, 2021, 17:28 UTC
Message-ID: <CABPp-BEn+yksK1KhncL8jWAya+ikGPUtsWpoZhrJrgRH2ORajw@mail.gmail.com>
In-Reply-To: <07290cba86fda73ee329a47db8e524b32dba25af.1623111879.git.jonathantanmy@google.com>

On Mon, Jun 7, 2021 at 5:26 PM Jonathan Tan <jonathantanmy@google.com> wrote:

Show 62 quoted lines

>
> 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@google.com>
> ---
>  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.