From: Jeff King <firstname.lastname@example.org> To: "René Scharfe" <email@example.com> Cc: Derrick Stolee <firstname.lastname@example.org>, email@example.com, Yiyuan guo <firstname.lastname@example.org> 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: <email@example.com> 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
next prev parent 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 List information: http://vger.kernel.org/majordomo-info.html * 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 \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --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
Code repositories for project(s) associated with this inbox: https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).