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

Christian Couder <[email protected]> writes:

> +/*
> + * 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.