git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jeff King <peff@peff.net>
To: "René Scharfe" <l.s.r@web.de>
Cc: Derrick Stolee <stolee@gmail.com>,
	git@vger.kernel.org, Yiyuan guo <yguoaz@gmail.com>
Subject: Re: [PATCH 3/5] pack-objects: clamp negative window size to 0
Date: Tue, 4 May 2021 17:38:14 -0400	[thread overview]
Message-ID: <YJG+xp2b1rUzBaws@coredump.intra.peff.net> (raw)
In-Reply-To: <02a66bfb-aac0-c05e-dab3-366bc312d900@web.de>

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

> >> 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

  reply	other threads:[~2021-05-04 21:38 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-01 14:02 [PATCH 0/5] pack-objects: better handling of negative options Jeff King
2021-05-01 14:02 ` [PATCH 1/5] t5300: modernize basic tests Jeff King
2021-05-03  5:27   ` Junio C Hamano
2021-05-03 14:53     ` Jeff King
2021-05-01 14:03 ` [PATCH 2/5] t5300: check that we produced expected number of deltas Jeff King
2021-05-01 14:03 ` [PATCH 3/5] pack-objects: clamp negative window size to 0 Jeff King
2021-05-03 12:10   ` Derrick Stolee
2021-05-03 14:55     ` Jeff King
2021-05-04 18:47       ` René Scharfe
2021-05-04 21:38         ` Jeff King [this message]
2021-05-05 11:53         ` Ævar Arnfjörð Bjarmason
2021-05-05 16:19           ` René Scharfe
2021-05-01 14:04 ` [PATCH 4/5] t5316: check behavior of pack-objects --depth=0 Jeff King
2021-05-01 14:04 ` [PATCH 5/5] pack-objects: clamp negative depth to 0 Jeff King
2021-05-03 12:11   ` Derrick Stolee

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YJG+xp2b1rUzBaws@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=l.s.r@web.de \
    --cc=stolee@gmail.com \
    --cc=yguoaz@gmail.com \
    --subject='Re: [PATCH 3/5] pack-objects: clamp negative window size to 0' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
on how to clone and mirror all data and code used for this inbox