Re: [RFC PATCH] diff: only prefetch for certain output formats

From: Jeff King <peff@peff.net>
Date: Feb 1, 2020, 11:29 UTC
Message-ID: <20200201112920.GD1864948@coredump.intra.peff.net>
In-Reply-To: <xmqqeevfbisb.fsf@gitster-ct.c.googlers.com>

On Fri, Jan 31, 2020 at 10:08:36AM -0800, Junio C Hamano wrote:

Show 20 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > fetches anything at all). I.e., for "diff -M -p", you'd want:
> >
> >   1. diffcore_std() sees "-p" and fetches everything
> >
> >   2. diffcore_rename() sees there's nothing we don't already have
> >
> > rather than:
> >
> >   1. diffcore_rename() fetches a few blobs to do rename detection
> >
> >   2. diffcore_std() fetches a few more blobs that weren't rename
> >      candidates, but we need for "-p"
> 
> Hmph, a pure rename only change will cause no blobs transferred with
> the latter (because there is no content change for "-p" to report,
> and the rename detection for R100 paths would be done at the object
> name level), but all blobs in filepairs (before rename matches A/D
> up) with the former, no?

Hmm, good point that an exact rename won't show the blobs. I don't think there's a way that covers all cases in a single fetch at the granularity of the functions I listed above. But we could if we break it down a bit:

  1. look for exact renames

  2. queue for fetching any inexact rename candidates leftover

  3. if we're going to show a diff that requires contents, then:

     3a. if marked as an exact rename/copy, don't queue (I guess this is
         really checking p->one->oid versus p->two->oid)

     3b. likewise, deletions with --irreversible-delete don't need queued

     3c. otherwise, queue for fetch

  4. fetch whatever was queued

  5. proceed with inexact rename detection, then showing the diffs

So that logic has to go in the middle of diffcore_rename(). And if we're not doing renames, then the logic from (3) needs to get triggered by diffcore_std(). So it probably needs to be hoisted out into a helper, and made idempotent (it already should be, since after the first prefetch we'd have all of the objects, but we might want a flag to avoid unnecessarily checking again).

-Peff