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

From: Taylor Blau <me@ttaylorr.com>
Date: Jun 4, 2021, 19:56 UTC
Message-ID: <YLqFgjEV8JoLzZSL@nand.local>
In-Reply-To: <4a7ad9ffeb140c8b613c308280c2c234154ae2ab.1622580781.git.jonathantanmy@google.com>

On Tue, Jun 01, 2021 at 02:34:16PM -0700, Jonathan Tan wrote:

Show 6 quoted lines

> @@ -99,6 +94,11 @@ static int promisor_remote_config(const char *var, const char *value, void *data
>  	size_t namelen;
>  	const char *subkey;
>
> +	if (!strcmp(var, "extensions.partialclone")) {
> +		repository_format_partial_clone = xstrdup(value);

Can value ever be NULL here? I think the answer is "no, because we check earlier in setup.c:handle_extension_v0()", but there is an implicit conversion from xstrdup_or_null() to just xstrdup(), which would fault if value were to be NULL.

Looking deeper, this path is a little confusing to me, since (in the pre-image), handle_extension_v0() makes a copy of value and binds it to data->partial_clone. But then check_repository_format_gently() makes another copy of canidate->partial_clone (which is the same location as data->partial_clone).

So, the extra copy is a little strange to me, because even though the copy in handle_extension_v0() is definitely necessary, I'm not certain that the one in set_repository_format_partial_clone() is. And this patch removes the latter one, which I think is good. But we never free repository_format_partial_clone.

Maybe that is added in a later patch, let's see...

Thanks, Taylor