Re: [PATCH 2/2] index-pack: prefetch missing REF_DELTA bases

From: Jeff King <peff@peff.net>
Date: May 17, 2019, 08:55 UTC
Message-ID: <20190517085509.GA20039@sigill.intra.peff.net>
In-Reply-To: <CACsJy8CNyug3wvZ+6ts1nzgWyPF1JqC0LceP-HzMHjqvCr2Ugw@mail.gmail.com>

On Fri, May 17, 2019 at 02:20:42PM +0700, Duy Nguyen wrote:

Show 12 quoted lines

> On Fri, May 17, 2019 at 12:35 PM Jeff King <peff@peff.net> wrote:
> > As it turns out, index-pack does not handle these complicated cases at
> > all! In the final fix_unresolved_deltas(), we are only looking for thin
> > deltas, and anything that was not yet resolved is assumed to be a thin
> > object. In many of these cases we _could_ resolve them if we tried
> > harder. But that is good news for us because it means that these
> > expectations about delta relationships are already there, and the
> > pre-fetch done by your patch should always be 100% correct and
> > efficient.
> 
> Is it worth keeping some of these notes in the "third pass" comment
> block in index-pack.c to help future readers?

Perhaps. I started on the patch below, but I had trouble in the commit message. I couldn't find the part of the code that explains why we would never produce this combination, though empirically we do not.

-- >8 --
Subject: [PATCH] index-pack: describe an implication of our thin resolving

After digging into the delta resolution code, I discovered a surprising (to me, anyway) implication of our strategy: we could never find a non-thin delta with a thin delta as its base. This is OK because pack-objects will never produce such a combination, because....?

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/index-pack.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index ccf4eb7e9b..f40f4560d4 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1224,6 +1224,13 @@ static void resolve_deltas(void)
  * Third pass:
  * - append objects to convert thin pack to full pack if required
  * - write the final pack hash
+ *
+ * Note that we assume all deltas at this phase are thin. We take only a
+ * single pass over the unresolved objects, and we look for bases only
+ * in our set of already-existing objects, _not_ other objects within this
+ * pack. This means that we would never find an object A stored as a delta
+ * against another object B in this pack, when B is a thin delta against a base
+ * not in the pack.
  */
 static void fix_unresolved_deltas(struct hashfile *f);
 static void conclude_pack(int fix_thin_pack, const char *curr_pack, unsigned char *pack_hash)

-- 
2.22.0.rc0.544.g1eb4087842