Re: [PATCH v2 2/2] diff: batch fetching of missing blobs

[Junio C Hamano <gitster@xxxxxxxxx>]

Christian Couder <christian.couder@xxxxxxxxx> writes:

> Patch 3f82acbca2 (Use promisor_remote_get_direct() and
> has_promisor_remote(), 2019-04-01) in cc/multi-promisor starts with:
>
>     Use promisor_remote_get_direct() and has_promisor_remote()
>
>     Instead of using the repository_format_partial_clone global
>     and fetch_objects() directly, let's use has_promisor_remote()
>     and promisor_remote_get_direct().
>
>     This way all the configured promisor remotes will be taken
>     into account, not only the one specified by
>     extensions.partialClone.
>
> I will at least add something telling that in most cases
> "repository_format_partial_clone" and fetch_objects() shouldn't be
> used directly anymore.

Yes, that would be necessary not in the log, but in-code comments
next to now "by-exception-only" API functions and variables.

>> > @@ -6506,8 +6507,7 @@ void diffcore_std(struct diff_options *options)
>> >              /*
>> >               * NEEDSWORK: Consider deduplicating the OIDs sent.
>> >               */
>> > -            fetch_objects(repository_format_partial_clone,
>> > -                      to_fetch.oid, to_fetch.nr);
>> > +            promisor_remote_get_direct(to_fetch.oid, to_fetch.nr);
>>
>> Likewise between fetch_objects() and promisor_remote_get_direct().
>> Shouldn't the underlying fetch_objects be hidden from general
>> callers?
>
> I will see if I can do that, though it has to be used in promisor-remote.c.

Does fetch-objects.[ch] even need to exist after multi-promisor
world as an external interface?  

Wouldn't it become an implementation detail of promisor-remote.[ch]
API?