Junio C Hamano [off-list ref] writes:
Jeff King [off-list ref] writes:
quoted
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
quoted
$ 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.
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
quoted
quoted
$ 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