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

[Jonathan Tan <jonathantanmy@google.com>]

> On Tue, Jun 01, 2021 at 02:34:16PM -0700, Jonathan Tan wrote:
> > @@ -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.

Ah, good catch. I think you're right, but it's better to be defensive
here. I'll add that check and also say that this will be caught by
setup.c.

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

Yes, it's also a simplification that we go straight from config to
variable here, instead of going through the candidate struct. As for
freeing repository_format_partial_clone, this is run once and never
again (guarded by "initialized" check) so there is no prior value to
free, and I don't think this variable needs to be freed.

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

So I think things are fine here, but in a later patch,
repository_format_partial_clone is moved to the struct repository, which
can be cleared with repo_clear(). I'll need to add a free there :-)

> Thanks,
> Taylor

Thanks for looking at the patch set too.