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.
Thanks,
Taylor
