Re: [PATCH v2 1/5] promisor-remote: factor out lazy_fetch_objects()

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 14, 2026, 17:49 UTC
Message-ID: <xmqqjypsoami.fsf@gitster.g>
In-Reply-To: <20260813154748.2378747-2-christian.couder@gmail.com>

Christian Couder <christian.couder@gmail.com> writes:

Show 27 quoted lines

> +/*
> + * Return 'true' if all the objects could be fetched, 'false' otherwise.
> + */
> +static bool lazy_fetch_objects(struct repository *repo,
> +			       struct object_id **remaining_oids,
> +			       int *remaining_nr,
> +			       int *to_free)
> +{
> +	if (git_env_bool(NO_LAZY_FETCH_ENVIRONMENT, 0)) {
> +		static int warning_shown;
> +		if (!warning_shown) {
> +			warning_shown = 1;
> +			warning(_("lazy fetching disabled; some objects may not be available"));
> +		}
> +		return false;
> +	}
> +
> +	promisor_remote_init(repo);
> +
> +	/* Try accepted remotes first (those the server told us to use) */
> +	if (try_promisor_remotes(repo, remaining_oids, remaining_nr,
> +				 to_free, true))
> +		return true;
> +
> +	return try_promisor_remotes(repo, remaining_oids, remaining_nr,
> +				    to_free, false);
>  }

Perhaps writing it this way would make it easier to tell what is going on. We try the preferred ones first, and then fall back to the other ones.

        return (try_promisor_remotes(..., true) ||
                try_promisor_remotes(..., false));

But more importantly, I wonder if keeping the list of missing object names in memory will later turn out to be problematic in real-life applications. Without knowing much about how the current code for bulk dehydrating promisor objects is structured, I expected an API that looks more like:

 - bulk_download_begin(): performs the early part of
   fetch_objects(), sets up connections to the promisor remote(s),
   and calls start_command() on the child process.

 - bulk_download_this(): after calling the _begin() function above,
   it runs around and collects missing objects that it needs to do
   its work.  For each such missing object it discovers, this
   function is called, which sends the object name down the
   '--stdin' file descriptor.

 - bulk_download_done(): tells the child process that we are done
   feeding object names.

but that is not what I am seeing. I guess the current arrangement cannot be avoided, because we are going to fetch from more than one promisor remote. Under such constraints, the way to deal with a massive number of missing objects will not be "streaming" like I imagined above, but needs to be done differently, like spooling to a file or something silly like that.

In any case, except that this avoids checking the environment variable multiple times, I can see that it is a no-op refactoring of the existing code.

Nice and cleanly done.

Thanks.