git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH 0/2] git-config and large integers
Date: Tue, 20 Aug 2013 22:34:29 -0400	[thread overview]
Message-ID: <20130821023429.GA25296@sigill.intra.peff.net> (raw)
In-Reply-To: <xmqqli3wufmc.fsf@gitster.dls.corp.google.com>

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

> 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?

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

  parent reply	other threads:[~2013-08-21  2:34 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-08-20 22:39 [PATCH 0/2] git-config and large integers Jeff King
2013-08-20 22:42 ` [PATCH 1/2] config: properly range-check integer values Jeff King
2013-08-20 23:07   ` Jonathan Nieder
2013-08-21  2:55     ` Jeff King
2013-08-20 22:44 ` [PATCH 0/2] git-config and large integers Stefan Beller
2013-08-20 22:48   ` Jeff King
2013-08-20 22:47 ` [PATCH 2/2] teach git-config to output " Jeff King
2013-08-20 22:57   ` Jonathan Nieder
2013-08-21  3:00     ` Jeff King
2013-08-21  4:38       ` Jonathan Nieder
2013-08-21  5:00         ` Jeff King
2013-08-21  6:34           ` Jonathan Nieder
2013-08-20 23:06 ` [PATCH 0/2] git-config and " Junio C Hamano
2013-08-20 23:41   ` Junio C Hamano
2013-08-21  2:43     ` Jeff King
2013-08-21  2:34   ` Jeff King [this message]
2013-09-08  8:27 ` [PATCHv2 0/5] " Jeff King
2013-09-08  8:29   ` [PATCH 1/5] config: factor out integer parsing from range checks Jeff King
2013-09-08  8:33   ` [PATCH 2/5] config: properly range-check integer values Jeff King
2013-09-08  8:36   ` [PATCH 3/5] config: set errno in numeric git_parse_* functions Jeff King
2013-09-09  0:36     ` Eric Sunshine
2013-09-09 19:53       ` Jeff King
2013-09-08  8:38   ` [PATCH 4/5] config: make numeric parsing errors more clear Jeff King
2013-09-08  8:40   ` [PATCH 5/5] git-config: always treat --int as 64-bit internally Jeff King
2013-09-09 18:58   ` [PATCHv2 0/5] git-config and large integers Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20130821023429.GA25296@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).