[PATCH 0/2] git-config and large integers
I was playing with a hook for file size limits that wanted to store the limit in git-config. It turns out we don't do a very good job of big integers: $ git config foo.size 2g $ git config --int foo.size -2147483648 Oops. After this series, we properly notice the error: $ git config --int foo.size fatal: bad config value for 'foo.size' in .git/config and even better, provide a way to access large values: $ git config --ulong foo.size 2147483648 The patches are: [1/2]: config: properly range-check integer values [2/2]: teach git-config to output large integers -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] git-config and large integers
On 08/21/2013 12:39 AM, Jeff King wrote: I was playing with a hook for file size limits that wanted to store the limit in git-config. It turns out we don't do a very good job of big integers: $ git config foo.size 2g $ git config --int foo.size -2147483648 Oops. After this series, we properly notice the error: $ git config --int foo.size fatal: bad config value for 'foo.size' in .git/config and even better, provide a way to access large values: $ git config --ulong foo.size 2147483648 int, ulong... How large will those be, I'd guess they are machine dependent? So int being 32 bits as usual, but not on all machines. (Those, which don't have 32 bits, are maybe not relevant anyways?) Stefan signature.asc Description: OpenPGP digital signature
Re: [PATCH 0/2] git-config and large integers
On Wed, Aug 21, 2013 at 12:44:22AM +0200, Stefan Beller wrote: On 08/21/2013 12:39 AM, Jeff King wrote: I was playing with a hook for file size limits that wanted to store the limit in git-config. It turns out we don't do a very good job of big integers: $ git config foo.size 2g $ git config --int foo.size -2147483648 Oops. After this series, we properly notice the error: $ git config --int foo.size fatal: bad config value for 'foo.size' in .git/config and even better, provide a way to access large values: $ git config --ulong foo.size 2147483648 int, ulong... How large will those be, I'd guess they are machine dependent? So int being 32 bits as usual, but not on all machines. (Those, which don't have 32 bits, are maybe not relevant anyways?) Yes, machine dependent. See the discussion in the patches themselves. It's kind of ugly, but it matches what git does internally (and we properly detect range errors at runtime). -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] git-config and large integers
Jeff King p...@peff.net writes: I was playing with a hook for file size limits that wanted to store the limit in git-config. It turns out we don't do a very good job of big integers: $ git config foo.size 2g $ git config --int foo.size -2147483648 Oops. After this series, we properly notice the error: $ git config --int foo.size fatal: bad config value for 'foo.size' in .git/config and even better, provide a way to access large values: $ git config --ulong foo.size 2147483648 I may be missing something, but why do we even need a new option for the command that is known to always produce textual output? As you said Oops, the first example that shows a string of digits prefixed by a minus sign for input 2g is buggy, and I think it is perfectly reasonable to fix it to show a stringified representation of 2*1024*1024*1024 when asked for --int. What am I missing??? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] git-config and large integers
Junio C Hamano gits...@pobox.com writes: Jeff King p...@peff.net writes: I was playing with a hook for file size limits that wanted to store the limit in git-config. It turns out we don't do a very good job of big integers: $ git config foo.size 2g $ git config --int foo.size -2147483648 Oops. After this series, we properly notice the error: $ git config --int foo.size fatal: bad config value for 'foo.size' in .git/config and even better, provide a way to access large values: $ git config --ulong foo.size 2147483648 I may be missing something, but why do we even need a new option for the command that is known to always produce textual output? As you said Oops, the first example that shows a string of digits prefixed by a minus sign for input 2g is buggy, and I think it is perfectly reasonable to fix it to show a stringified representation of 2*1024*1024*1024 when asked for --int. What am I missing??? If this applied on the writing side, I would understand it very much, i.e. $ git config --int32 foo.size 2g fatal: 2g is too large to be read as int32. and as a complement it may make sense as a warning mechanism to also error out when existing value does not fit on the platform int, so your $ git config --int foo.size fatal: bad config value for 'foo.size' in .git/config might make sense (even though I'd suggest being more explicit than bad value in this case---the value specified will not fit when used in a variable of type int on this platform). When .git/config is shared on two different boxes (think: NFS), the size of int might be different between them, so the logic to produce such a warning may have to explicitly check against int32_t, not platform int and say will not fit in 'int' on some machines. I dunno. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] git-config and large integers
On Tue, Aug 20, 2013 at 04:06:19PM -0700, Junio C Hamano wrote: Jeff King p...@peff.net writes: I was playing with a hook for file size limits that wanted to store the limit in git-config. It turns out we don't do a very good job of big integers: $ git config foo.size 2g $ git config --int foo.size -2147483648 Oops. After this series, we properly notice the error: $ git config --int foo.size fatal: bad config value for 'foo.size' in .git/config and even better, provide a way to access large values: $ git config --ulong foo.size 2147483648 I may be missing something, but why do we even need a new option for the command that is known to always produce textual output? We could do all math with int64_t (for example) and then print the stringified representation. But then we would not be matching the same checks that git is doing internally. For example, on a 32-bit system, setting core.packedGitLimit to 4G will produce an error for git config --int core.packedgitlimit, as we cannot represent the size internally. We could do the conversion in such a way that we print the correct size, but it does not represent what happens when git pack-objects is run (you get the same error). As you said Oops, the first example that shows a string of digits prefixed by a minus sign for input 2g is buggy, and I think it is perfectly reasonable to fix it to show a stringified representation of 2*1024*1024*1024 when asked for --int. The negative value is a little bit of a sidetrack. For both git config --int and for internal use, we do not correctly range-check integer values. And that's why we print the negative value, when we should say this is a bogus out-of-range value. The latter is what we have always done for values outside of range, both internal and external, and it is only that our range check was bogus (and we fed negative crap to the code instead of complaining). That is fixed by the first patch. And that leads to the second patch. The --int option provides a range check of (typically) -2^32 to 2^32-1. But many of our values internally use a larger range. We can either drop that range check (which means we will let you inspect values with config that git internally will barf on, with no clue), or we need to add another option with a different range to retrieve those values. I chose to add another option because I think the range check has value. Does that explain the problem more fully? -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] git-config and large integers
On Tue, Aug 20, 2013 at 04:41:30PM -0700, Junio C Hamano wrote: If this applied on the writing side, I would understand it very much, i.e. $ git config --int32 foo.size 2g fatal: 2g is too large to be read as int32. It does, by the way. When you request a type on the writing side, we normalize (and complain in the same way as we do when reading). and as a complement it may make sense as a warning mechanism to also error out when existing value does not fit on the platform int, so your $ git config --int foo.size fatal: bad config value for 'foo.size' in .git/config might make sense (even though I'd suggest being more explicit than bad value in this case---the value specified will not fit when used in a variable of type int on this platform). Yes, the error message is terrible, and I think an extra patch on top to improve it is worth doing. But note that I am not introducing that error here at all. On 32-bit systems, we already correctly range-checked and produced that error. It is only on 64-bit systems that the range check was flat out wrong. It checked against long's precision, but then cast the result to an int, losing bits. A possibly worse example than the negative one is: $ git config foo.bar 4g $ git config --int foo.bar 0 Again, that is what git's internal code is seeing. And that is why keeping the range check for git-config has value: it lets you see what git would see internally. When .git/config is shared on two different boxes (think: NFS), the size of int might be different between them, so the logic to produce such a warning may have to explicitly check against int32_t, not platform int and say will not fit in 'int' on some machines. I don't really see the value in that. You can always write whatever you like in the config file. The reader is responsible during parsing for saying Hey, I am 32-bit and I can't handle this. And we already do that, and it works fine. So if you have an NFS-shared .git/config, and you set pack.deltacachesize to 4g, a 64-bit machine will do fine with that, and a 32-bit machine will complain. Which seems like the only sane thing to do. There are a few config options that use unsigned long that I would argue should be off_t or something (for example, core.bigFileThreshold, which cannot be more than 4G on a 32-bit machine, simply because we can't represent the size. On the other hand, there is probably a ton of stuff that does not work with 4G files on such a system, because we use unsigned long all over the place inside the code). -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
