On Tue, Jun 8, 2021 at 9:44 PM Jonathan Tan [off-list ref] wrote:

quoted

quoted

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

Hmm...is your concern about the case in which
repository_format_partial_clone is uninitialized, or about ignoring a
potential NULL value? If the former, I don't see how your suggestion
fixes things, since extensions.partialclone may never have been in the
config in the first place (and would thus leave
repository_format_partial_clone uninitialized, if it weren't for the
fact that it is in static storage and thus initialized to 0). If the
latter, I guess I should be more detailed about how it's being handled
in setup.c (or maybe just leave out the comment altogether - the code
here can handle a NULL repository_format_partial_clone for some reason).

My comment was about the latter; I was trying to understand what the
comment meant relative to that case, and how and where that case would
be handled in the code.  With that frame of reference, the comment
seemed misleading to me...though perhaps the comment was intended to
answer some other question entirely.