Thread (59 messages) 59 messages, 5 authors, 2021-06-19

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.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help