Re: [PATCH v3 1/5] promisor-remote: factor out lazy_fetch_objects()
On Tue, Sep 8, 2026 at 7:40 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Christian Couder <christian.couder@gmail.com> writes:
Show 19 quoted lines
> > 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.
Show 5 quoted lines
> > 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.
Show 27 quoted lines
> > +/*
> > + * 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.
Christian Couder <christian.couder@gmail.com>