Re: [PATCH v2 2/2] diff: restrict when prefetching occurs

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 2, 2020, 23:54 UTC
Message-ID: <xmqqsghl1m0p.fsf@gitster.c.googlers.com>
In-Reply-To: <20200402230937.47323-1-jonathantanmy@google.com>

Jonathan Tan <jonathantanmy@google.com> writes:

> My idea is that this prefetch is a superset of what diffcore_rebase()
> wants to prefetch, so if we have already done the necessary logic here
> (even if nothing gets prefetched - which might be the case if we have
> all objects), we do not need to do it in diffcore_rebase().

s/rebase/rename/ I presume, but the above reasoning, while it may happen to hold true right now, feels brittle. In other words

 - how do we know it would stay to be "a superset"?

 - would it change the picture if we later added a prefetch in
   diffcore_break(), just like you are doing so to diffcore_rename()
   in this patch?

So the revised version of my earlier "wondering" is if it would be more future-proof (easier to teach different steps to prefetch for their own needs, without having to make an assumption like "what this step needs is sufficient for the other step") to arrange the codepath from diffcore_std() to its helpers like so:

    - prepare an empty to_fetch OID array in the caller,

    - if the output format is one of the ones that wants prefetch,
      add object names to to_fetch in the caller, but not fetch as
      long as the caller does not yet need the contents of the
      blobs.

    - pass &to_fetch from diffcore_std() to the helper functions in
      the diffcore family like diffcore_{break,rename}() have them
      also batch what they (may) want to prefetch in there.  Delay
      fetching until they actually need to look at the blobs, and
      when they fetch, clear &to_fetch for the next helper.

    - diffcore_std() also would need to look at the blob eventually,
      perhaps after all the helpers it may call returns.  Do the
      final prefetch if to_fetch is still not empty before it has to
      look at the blobs.

Thanks.