Re: [RFC PATCH 3/5] pack-objects: add delta-islands support

On Mon, Jul 23, 2018 at 11:52:49AM -0700, Stefan Beller wrote:

quoted

+DELTA ISLANDS
+-------------

[...]

I had to read all of this [background information] to understand the
concept and I think it is misnamed, as my gut instinct first told me
to have deltas only "within an island and no island hopping is allowed".
(This message reads a bit like a commit message, not as documentation
as it is long winded, too).

I'm not sure if I'm parsing your sentence correctly, but the reason I
called them "islands" is exactly that you'd have deltas within an island
and want to forbid island hopping. So I wasn't sure if you were saying
"that's what I think, but not how the feature works".

There _is_ a tricky thing, which is that a given object is going to
appear in many islands. So the rule is really "you cannot hop to a base
that is not in all of your islands".

What about renaming this feature to

[pack]
    excludePartialReach = refs/virtual/[0-9]]+/tags/

  "By setting `pack.excludePartialReach`, object deltafication is
  prohibited for objects that are not reachable from all
  manifestations of the given regex"

Cryptic, but it explains it in my mind in a shorter, more concise way. ;-)

So I'm hopelessly biased at this point, having developed and worked with
the feature under the "island" name for several years. But I find your
name and explanation pretty confusing. :)

Worse, though, it does not have any noun to refer to the reachable set.
The regex capture and the island names are an important part of the
feature, because it lets you make a single island out of:

  refs/virtual/([0-9]+)/heads
  refs/virtual/([0-9]+)/tags

but exclude:

  refs/virtual/([0-9]+)/(foo)

which goes into its own island ("123-foo" instead of "123"). So what's
the equivalent nomenclature to "island name"?

quoted

@@ -3182,6 +3225,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
                  option_parse_missing_action },
                OPT_BOOL(0, "exclude-promisor-objects", &exclude_promisor_objects,
                         N_("do not pack objects in promisor packfiles")),
+               OPT_BOOL(0, "delta-islands", &use_delta_islands,
+                        N_("enable islands for delta compression")),

We enable this feature, but we disallow certain patterns to be used in packing,
so it sounds weird to me as we tell Git to *not* explore the full design space,
so we're not enabling it, but rather restricting it?

Yeah, perhaps "respect islands during delta compression" makes more sense.

-Peff