Re: [PATCH 3/5] pack-objects: clamp negative window size to 0

From: Jeff King <peff@peff.net>
Date: May 4, 2021, 21:38 UTC
Message-ID: <YJG+xp2b1rUzBaws@coredump.intra.peff.net>
In-Reply-To: <02a66bfb-aac0-c05e-dab3-366bc312d900@web.de>

On Tue, May 04, 2021 at 08:47:56PM +0200, René Scharfe wrote:

Show 33 quoted lines

> >> This seems like a reasonable approach. It takes existing "undefined"
> >> behavior and turns it into well-understood, "defined" behavior.
> >
> > I was on the fence on doing that, or just:
> >
> >   if (window < 0)
> > 	die("sorry dude, negative windows are nonsense");
> >
> > So if anybody has a strong preference, I could be easily persuaded. Part
> > of what led me to being forgiving was that we similarly clamp too-large
> > depth values (with a warning; I didn't think it was really necessary
> > here, though).
> 
> There's another option: Mapping -1 or all negative values to the
> maximum:
> 
> 	if (window < 0)
> 		window = INT_MAX;
> 	if (depth < 0)
> 		depth = (1 << OE_DEPTH_BITS) - 1;
> 
> That's allows saying "gimme all you got" without knowing or being
> willing to type out the exact maximum value, which may change between
> versions.  Not all that useful for --window, I guess.  That convention
> has been used elsewhere I'm sure, but can't point out a concrete
> example.  $arr[-1] get the last item of the array $arr in PowerShell,
> though, which is kind of similar.
> 
> Sure, you get the same effect in both cases by typing 2147483647, but
> -1 is more convenient.
> 
> Not a strong preference, but I thought it was at least worth
> mentioning that particular bike shed color. :)

Thanks for thinking about this. In general, yeah, allowing "-1" as "unlimited" is a sensible thing for many options. But for these two variables, I don't think "unlimited" is ever a good idea:

  - for --window, it makes the amount of work quadratic in the number of
    objects in the repository (and likewise, memory usage equivalent to
    the uncompressed contents of the repo). Going beyond about 250 or so
    gives diminishing returns.

  - for --depth, going beyond about 50 provides diminishing space
    returns, and makes the run-time performance of accessing the deltas
    horrible.

So certainly INT_MAX or the max allowable by OE_DEPTH_BITS is a very bad idea in both cases, and the use would probably be happier if we just hit a die() instead. :) We _could_ make "-1" mean "the maximum sensible valuable", but I think there's a lot of wiggle room for "sensible" there. I much prefer using and advertising the actual values there (as we do for "gc --aggressive").

-Peff