Re: [PATCH 0/2] git-config and large integers

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 20, 2013, 23:41 UTC
Message-ID: <xmqqhaekudzp.fsf@gitster.dls.corp.google.com>
In-Reply-To: <xmqqli3wufmc.fsf@gitster.dls.corp.google.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 29 quoted lines

> Jeff King <peff@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.