Re: [RFC PATCH 3/5] pack-objects: add delta-islands support
- From
Jeff King <peff@peff.net>
- Date
- Jul 24, 2018, 09:58 UTC
- Message-ID
- <20180724095843.GB3578@sigill.intra.peff.net>
- In-Reply-To
- <CAGZ79kbF7g3E4hBa0VqMqBoovbAb2dHaGFNRL=+f7Lce1AduVg@mail.gmail.com>
On Mon, Jul 23, 2018 at 11:52:49AM -0700, Stefan Beller wrote:
Show 9 quoted lines
> > +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".
Show 10 quoted lines
> 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"?
Show 10 quoted lines
> > @@ -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