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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 8, 2026, 17:39 UTC
Message-ID: <xmqq7bkvy74h.fsf@gitster.g>
In-Reply-To: <20260908164129.560396-2-christian.couder@gmail.com>

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

Show 21 quoted lines

> In "promisor-remote.c:fetch_objects()", there is a check to disable
> lazy fetching when the `GIT_NO_LAZY_FETCH` environment variable is
> set. The fetch_objects() function is called once per promisor remote
> though. So the check might be performed more times than necessary.
>
> Also promisor_remote_get_direct() mixes up the logic deciding which
> promisor remotes to try with the logic checking that the objects
> that could not be fetched are promisor objects.
>
> Let's refactor the lazy fetching logic out of these two functions
> into a new lazy_fetch_objects() function.
>
> This is a pure refactoring with no intended behavior change. Two
> things shift in ways that are observably equivalent though:
>
>   - the `GIT_NO_LAZY_FETCH` check is now performed once up front,
>     instead of once per promisor remote, and
>
>   - promisor_remote_init() is no longer called when lazy fetching
>     is disabled, which is fine as nothing downstream of it, like
>     is_promisor_object(), needs it in that case.

Yeah, I too noticed these while reading the patch. The latter change may be a very good thing, in that the calling sequence around promisor_remote_init() seems to be anybody who needs to access the promisor remote information is expected to _init() the system beforehand. If it were "call _init() once at the very beginning and then do random things on promisor remotes", then moving its callsite may have to be done more carefully, but with the "user makes sure it is initialized beforehand" convention, the postimage of this patch follows the pattern exactly.

> While at it, let's also convert try_promisor_remotes() to return
> 'bool' instead of 'int', as it just returns whether all the objects
> could be fetched, and document its return value.

Meh.

> +/*
> + * Return 'true' if all the objects could be fetched from the
> + * (non-)accepted remotes, 'false' otherwise.
> + */

The comment was not quite understandable, at least to me, especially around "from the (non-)accepted" part of the sentence.

Also "could be fetched" made it sound as if this were dry-run but isn't this function actually doing the fetching and reporting if everything got fetched or there are still objects remaining to be fetched?

    /*
     * fetch remaining objects (given in remaining_oids) from
     * the known promisor remotes.  If accepted_only is true,
     * ignore promisor remotes with .accepted member unset.
     * return true when all requested objects have been fetched,
     * false otherwise.
     */

The above only mentions half of how the remaining_oids parameter is used (i.e., only on the input side), but if we are adding a comment, we should document how remaining_oids and to_free are used as well.

The semantics of to_free in the entire callchain is especially tricky to describe correctly, I am afraid.