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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 8, 2019, 02:36 UTC
Message-ID: <xmqqy34lb4vb.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <CAP8UFD1qK61MtJ-XeMuwY4W2V0CdHeZQC9rQap1M+nEeaLmBJQ@mail.gmail.com>

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

Show 13 quoted lines

> Thanks for investigating! Yeah, that's part of the problem.
>
> The fix I would suggest is:
>
> diff --git a/diff.c b/diff.c
> index f685ab10b5..a2b1241f83 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -26,6 +26,7 @@
>  #include "parse-options.h"
>  #include "help.h"
>  #include "fetch-object.h"
> +#include "promisor-remote.h"

Thanks.

Show 9 quoted lines

>
>  #ifdef NO_FAST_WORKING_DIRECTORY
>  #define FAST_WORKING_DIRECTORY 0
> @@ -6489,7 +6490,7 @@ static void add_if_missing(struct oid_array *to_fetch,
>
>  void diffcore_std(struct diff_options *options)
>  {
> -    if (repository_format_partial_clone) {
> +    if (has_promisor_remote()) {

Hmph, I see quite a few references to the variable disappears between next and pu. Is it that in the new world order, nobody outside the low-level object-access layer should look at the variable directly, but instead ask the has_promisor_remote() function? If so, can we at least document that? Making it static (or at least renaming it) would have helped the compiler to notice this semantic merge conflict better.

Show 7 quoted lines

> @@ -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?

Show 5 quoted lines

>          oid_array_clear(&to_fetch);
>      }
>
> I will send a new version with the above soon based on top of
> jt/batch-fetch-blobs-in-diff in pu.