From: Christian Couder Date: Mon, 28 Sep 2026 13:39:58 GMT Subject: Re: [PATCH v3 1/5] promisor-remote: factor out lazy_fetch_objects() Message-ID: In-Reply-To: On Tue, Sep 8, 2026 at 7:40 PM Junio C Hamano wrote: > > Christian Couder writes: > > 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. > > 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. > > +/* > > + * 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.