Re: [PATCH 2/2] teach git-config to output large integers
- From
Jeff King <peff@peff.net>
- Date
- Aug 21, 2013, 05:00 UTC
- Message-ID
- <20130821050053.GA875@sigill.intra.peff.net>
- In-Reply-To
- <20130821043841.GA2802@elie.Belkin>
On Tue, Aug 20, 2013 at 09:38:41PM -0700, Jonathan Nieder wrote:
Show 13 quoted lines
> Jeff King wrote: > > > I almost sent it as "--size" with unsigned long internally. But try > > writing the documentation for it. You want to say something like "it's > > big enough to handle file sizes". Except that on 32-bit, it's _not_. > > It's only 4G. > > > > You really want something that uses off_t internally, so 32-bit systems > > with largefile support do the sane thing. But now you have no way of > > emulating the way that git parses stuff internally. > > Let's take a step back for a moment. What problem is this patch > solving?
That you cannot currently ask git-config for a value larger than 2g.
Show 9 quoted lines
> From the motivating example, I thought it was > > When reading or writing an integer config item, git sometimes > encounters integer overflow and doesn't know how to deal with it. > Worse, this means that some meaningful values are unrepresentable > in config files. Fix it in two steps: > > 1. Catch overflow, and error out instead of pretending to be > able to handle it.
No, this first step is not being added. It is already the case that we error out for overflow. The first patch is catching a case where we failed to do that properly, and instead truncated. And it has nothing to do with git-config itself; it is only that we must use git-config to test it from the shell. The same problem may happen internally (but tends not to, because large things tend to use git_config_ulong instead of git_config_int).
Show 12 quoted lines
> 2. Provide at least an option to use a wider integer type and > handle larger meaningful values. > > This involves a new option --size instead of making --int use > intmax_t for the following compatibility reason: ... > > For example, the compatibility reason could be that some scripts > calling "git config" were not able to handle large integers and that > we do not want to expose them to unexpectedly large values. > > But that reason doesn't sound realistic to me. So what is the actual > reason not to always use a wider range?
My reason is that it does not represent the same range checks that git is doing internally (and which vary from platform to platform). So you might ask git-config "what is the value of pack.deltacachelimit"; I would expect it to apply the same range checks there that would be used by git-pack-objects.
Show 5 quoted lines
> That is what I was trying to get at in discussing the test. It is not > "We would like --int to reject values higher than this, but some > platforms do not allow us to", but "Either rejecting this value, or > even better, computing the right size and printing it, is an > acceptable behavior, and this test checks for those."
You are conflating the two patches, I think. The test we were discussing is for the _first_ patch, which fixes a bug in the range check. It is not meant to test git-config in particular, but to test that values higher than INT_MAX and lower than LONG_MAX are properly range-checked.
Forget the second patch for a moment. I believe the first one is a bug fix that we would want even if we do not take the second patch at all.
-Peff