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

2013-08-20 Thread Jeff King
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

2013-08-20 Thread Stefan Beller
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

2013-08-20 Thread Jeff King
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

2013-08-20 Thread Junio C Hamano
Jeff King  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

2013-08-20 Thread Junio C Hamano
Junio C Hamano  writes:

> Jeff King  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

2013-08-20 Thread Jeff King
On Tue, Aug 20, 2013 at 04:06:19PM -0700, Junio C Hamano wrote:

> Jeff King  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

2013-08-20 Thread Jeff King
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