> 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.
Just to clarify, what is the question and what is the answer?
> 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().
I'll add that in the next version.
> > @@ -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.
This BUG() will be removed in patch 4. I'm OK either way (adding BUG()
here and removing it in patch 4, or never adding BUG() in the first
place).
