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

[Junio C Hamano <gitster@pobox.com>]

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

> 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.