Re: [PATCH 2/4] promisor-remote: support per-repository config

> On Tue, Jun 01, 2021 at 02:34:17PM -0700, Jonathan Tan wrote:
> > Instead of using global variables to store promisor remote information,
> > store this config in struct repository instead, and add
> > repository-agnostic non-static functions corresponding to the existing
> > non-static functions that only work on the_repository.
> >
> > The actual lazy-fetching of missing objects currently does not work on
> > repositories other than the_repository, and will still not work after
> > this commit, so add a BUG message explaining this. A subsequent commit
> > will remove this limitation.
> 
> Makes sense to me. I found my answer to the question that I raised
> during my review of the previous patch, and I think it would make sense
> to address in an amended version of this patch.

> Other than that, the translation all looked very faithful to me.
> 
> > -void promisor_remote_reinit(void)
> > +void repo_promisor_remote_reinit(struct repository *r)
> >  {
> > -	initialized = 0;
> > -	promisor_remote_clear();
> > -	promisor_remote_init();
> > +	promisor_remote_clear(r->promisor_remote_config);
> 
> Ah, this is probably where I would have expected to see
> r->promisor_remote_config->repository_format_partial_clone freed as
> well.
> 
> I wondered whether or not that should have been freed, since on first
> read it seemed that this function was mostly concerned with the list of
> promisor remotes rather than the structure containing them. But on a
> closer look, we are re-initializing the whole structure with
> promisor_remote_init(), which runs the whole promisor_remote_config
> callback again.
> 
> So I do think we want to free that part of the structure, too, before
> reinitializing it. I would probably do it in promisor_remote_clear().

> > @@ -235,9 +244,11 @@ int promisor_remote_get_direct(struct repository *repo,
> >  	if (oid_nr == 0)
> >  		return 0;
> >
> > -	promisor_remote_init();
> > +	promisor_remote_init(repo);
> >
> > -	for (r = promisors; r; r = r->next) {
> > +	if (repo != the_repository)
> > +		BUG("only the_repository is supported for now");
> 
> I could go either way on whether this is worthy of a BUG() or not, but I
> don't really have much of a strong feeling about it.