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

On Tue, Sep 8, 2026 at 7:40 PM Junio C Hamano [off-list ref] wrote:

Christian Couder [off-list ref] writes:

quoted

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.

Yeah, I have tried to explain this in the commit message of the v4 I just sent.

quoted

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.

try_promisor_remotes() is not converted to return 'bool' in v4 then.

quoted

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

The comment before try_promisor_remotes() is now the following in v4:

+/*
+ * Fetch the remaining objects (given in '*remaining_oids', which
+ * contains '*remaining_nr' object ids) from the known promisor
+ * remotes. If 'accepted_only' is true, ignore promisor remotes with
+ * their 'accepted' member unset.
+ *
+ * When a fetch from a remote fails, the objects that are still
+ * missing are computed, and '*remaining_oids' and '*remaining_nr' are
+ * updated accordingly before trying the next remote. In that case
+ * '*remaining_oids' points to a new array that this function
+ * allocated, and '*to_free' is set to 1 to tell the caller that it
+ * owns that array and should free it. '*to_free' should be 0 on the
+ * first call.
+ *
+ * Return 1 when all the requested objects have been fetched, 0
+ * otherwise.
+ */

I hope it's better.

Thanks.