Re: [PATCH 6/6] pack-objects: reuse on-disk deltas for thin "have" objects

On Fri, Aug 17, 2018 at 03:57:18PM -0700, Stefan Beller wrote:

On Fri, Aug 17, 2018 at 2:06 PM Jeff King [off-list ref] wrote:

quoted

When we serve a fetch, we pass the "wants" and "haves" from
the fetch negotiation to pack-objects. That tells us not
only which objects we need to send, but we also use the
boundary commits as "preferred bases": their trees and blobs
are candidates for delta bases, both for reusing on-disk
deltas and for finding new ones.

However, this misses some opportunities. Modulo some special
cases like shallow or partial clones, we know that every
object reachable from the "haves" could be a preferred base.
We don't use them all for two reasons:

s/all/at all/ ?

No, I meant "we don't use all of them". As in the Pokemon "gotta catch
'em all" slogan. ;)

Probably writing out "all of them" is better.

quoted

The first is that check_object() needs access to the
relevant information (the thin flag and bitmap result). We
can do this by pushing these into program-lifetime globals.

I discussed internally if extending the fetch protocol to include
submodule packs would be a good idea, as then you can get all
the superproject+submodule updates via one connection. This
gives some benefits, such as a more consistent view from the
superproject as well as already knowing the have/wants for
the submodule.

With this background story, moving things into globals
makes me sad, but I guess we can flip this decision once
we actually move towards "submodule packs in the
main connection".

I don't think it significantly changes the existing code, which is
already relying on a ton of globals (most notably to_pack). The first
step in doing multiple packs in the same process is going to be to shove
all of that into a "struct pack_objects_context" or similar, and these
can just follow the rest.

quoted

The second is that the rest of the code assumes that any
reused delta will point to another "struct object_entry" as
its base. But by definition, we don't have such an entry!

I got lost here by the definition (which def?).

  The delta that we look up from the bitmap, doesn't may
  not be in the pack, but it could be based off of an object
  the client already has in its object store and for that
  there is no struct object_entry in memory.

Is that correct?

Right, we are interested in objects that we _couldn't_ find a struct
for.  I agree this could be more clear.

-Peff