On Tue, Jun 01, 2021 at 02:34:17PM -0700, Jonathan Tan wrote:

quoted

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.

quoted

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

quoted

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