Re: [PATCH 1/4] promisor-remote: read partialClone config here
- From
- Jonathan Tan <jonathantanmy@google.com>
- Date
- Jun 5, 2021, 01:38 UTC
- Message-ID
- <20210605013828.603432-1-jonathantanmy@google.com>
- In-Reply-To
- <YLqFgjEV8JoLzZSL@nand.local>
Show 12 quoted lines
> 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.
Show 11 quoted lines
> 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.