{"thread":{"id":"34735","subject":"[PATCH 0/2] git-config and large integers","startedAt":"2013-08-20T22:39:54Z","lastAt":"2013-09-09T19:53:04Z","messageCount":25,"participants":["Jeff King","Stefan Beller","Jonathan Nieder","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"225570","messageId":"20130820223953.GA3429@sigill.intra.peff.net","threadId":"34735","inReplyTo":null,"subject":"[PATCH 0/2] git-config and large integers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-08-20T22:39:54Z","receivedAt":"2013-08-20T22:39:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I was playing with a hook for file size limits that wanted to store the\nlimit in git-config. It turns out we don't do a very good job of big\nintegers:\n\n  $ git config foo.size 2g\n  $ git config --int foo.size\n  -2147483648\n\nOops. After this series, we properly notice the error:\n\n  $ git config --int foo.size\n  fatal: bad config value for 'foo.size' in .git/config\n\nand even better, provide a way to access large values:\n\n  $ git config --ulong foo.size\n  2147483648\n\nThe patches are:\n\n  [1/2]: config: properly range-check integer values\n  [2/2]: teach git-config to output large integers\n\n-Peff\n"},{"id":"225571","messageId":"20130820224256.GA24766@sigill.intra.peff.net","threadId":"34735","inReplyTo":"20130820223953.GA3429@sigill.intra.peff.net","subject":"[PATCH 1/2] config: properly range-check integer values","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-08-20T22:42:57Z","receivedAt":"2013-08-20T22:42:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we look at a config value as an integer using the\ngit_config_int function, we carefully range-check the value\nwe get and complain if it is out of our range. But the range\nwe compare to is that of a \"long\", which we then cast to an\n\"int\" in the function's return value. This means that on\nsystems where \"int\" and \"long\" have different sizes (e.g.,\nLP64 systems), we may pass the range check, but then return\nnonsense by truncating the value as we cast it to an int.\n\nWe can solve this by converting git_parse_long into\ngit_parse_int, and range-checking the \"int\" range. Nobody\nactually cared that we used a \"long\" internally, since the\nresult was truncated anyway, and there were no other callers\nof git_parse_long.\n\nOf the existing callers, all of them expect small-ish values\nthat are fine for an \"int\", and should not be impacted\nexcept when nonsense is fed to them. The one arguable\nexception is http.lowSpeedLimit, which is given in bytes per\nsecond, and is fed to curl as a long. However, nobody is\nlikely to consider 2GB/sec as \"low speed\", and even if they\ndid, the new behavior is much better (it barfs and complains\nrather than considering the value negative and silently\nignoring it).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI added a test. It would not fail on existing 32-bit systems, but would\non existing LP64 systems. It will pass with the new code on both.\nHowever, it will fail on ILP64 systems (because their int is large, and\ncan represent 3GB). I'm not sure if any such systems are in wide use\n(SPARC64?), but we would want a prereq in that case, I guess. I'm\ninclined to wait to see if it actually fails for anybody.\n\n config.c               | 12 ++++++------\n t/t1300-repo-config.sh |  5 +++++\n 2 files changed, 11 insertions(+), 6 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex e13a7b6..9237d4b 100644\n--- a/config.c\n+++ b/config.c\n@@ -468,7 +468,7 @@ static int parse_unit_factor(const char *end, uintmax_t *val)\n \treturn 0;\n }\n \n-static int git_parse_long(const char *value, long *ret)\n+static int git_parse_int(const char *value, int *ret)\n {\n \tif (value && *value) {\n \t\tchar *end;\n@@ -484,7 +484,7 @@ static int git_parse_long(const char *value, long *ret)\n \t\t\treturn 0;\n \t\tuval = abs(val);\n \t\tuval *= factor;\n-\t\tif ((uval > maximum_signed_value_of_type(long)) ||\n+\t\tif ((uval > maximum_signed_value_of_type(int)) ||\n \t\t    (abs(val) > uval))\n \t\t\treturn 0;\n \t\tval *= factor;\n@@ -526,8 +526,8 @@ int git_config_int(const char *name, const char *value)\n \n int git_config_int(const char *name, const char *value)\n {\n-\tlong ret = 0;\n-\tif (!git_parse_long(value, &ret))\n+\tint ret;\n+\tif (!git_parse_int(value, &ret))\n \t\tdie_bad_config(name);\n \treturn ret;\n }\n@@ -559,10 +559,10 @@ int git_config_maybe_bool(const char *name, const char *value)\n \n int git_config_maybe_bool(const char *name, const char *value)\n {\n-\tlong v = git_config_maybe_bool_text(name, value);\n+\tint v = git_config_maybe_bool_text(name, value);\n \tif (0 <= v)\n \t\treturn v;\n-\tif (git_parse_long(value, &v))\n+\tif (git_parse_int(value, &v))\n \t\treturn !!v;\n \treturn -1;\n }\ndiff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\nindex c4a7d84..3c5ec4b 100755\n--- a/t/t1300-repo-config.sh\n+++ b/t/t1300-repo-config.sh\n@@ -652,6 +652,11 @@ test_expect_success numbers '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'out-of-range integers are detected' '\n+\tgit config giga.crash 3g &&\n+\ttest_must_fail git config --int giga.crash\n+'\n+\n test_expect_success 'invalid unit' '\n \tgit config aninvalid.unit \"1auto\" &&\n \techo 1auto >expect &&\n-- \n1.8.4.rc2.28.g6bb5f3f\n"},{"id":"225572","messageId":"5213F146.1010509@googlemail.com","threadId":"34735","inReplyTo":"20130820223953.GA3429@sigill.intra.peff.net","subject":"Re: [PATCH 0/2] git-config and large integers","fromName":"Stefan Beller","fromEmail":"stefanbeller@googlemail.com","sentAt":"2013-08-20T22:44:22Z","receivedAt":"2013-08-20T22:44:22Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On 08/21/2013 12:39 AM, Jeff King wrote:\n> I was playing with a hook for file size limits that wanted to store the\n> limit in git-config. It turns out we don't do a very good job of big\n> integers:\n> \n>   $ git config foo.size 2g\n>   $ git config --int foo.size\n>   -2147483648\n> \n> Oops. After this series, we properly notice the error:\n> \n>   $ git config --int foo.size\n>   fatal: bad config value for 'foo.size' in .git/config\n> \n> and even better, provide a way to access large values:\n> \n>   $ git config --ulong foo.size\n>   2147483648\n> \n\nint, ulong...\nHow large will those be, I'd guess they are machine dependent?\nSo int being 32 bits as usual, but not on all machines.\n(Those, which don't have 32 bits, are maybe not relevant anyways?)\n\nStefan\n\n\n\n"},{"id":"225574","messageId":"20130820224710.GB24766@sigill.intra.peff.net","threadId":"34735","inReplyTo":"20130820223953.GA3429@sigill.intra.peff.net","subject":"[PATCH 2/2] teach git-config to output large integers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-08-20T22:47:10Z","receivedAt":"2013-08-20T22:47:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Internally we use \"unsigned long\" to represent large config\nvalues like packed window memory sizes, packfile size\nlimits, etc.  On 32-bit systems, this limits these config\noptions to 4G (and we detect and complain about overflow).\nOn 64-bit systems, they get the full 64-bit range.\n\nHowever, there is no way for script callers of git-config to\nsee the numeric values (they cannot treat them as pure\nstrings, since they would want git-config to process \"3g\"\ninto \"3221225472\").\n\nThis patch teaches git-config a \"--ulong\" type that can\noutput these unsigned long values, allowing scripts to\nexamine the values as the internal code would.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI kind of hate the name \"--ulong\". I wanted to call it \"--size\" or\nsomething and abstract away the actual platform representation, and just\nmake it \"big enough for file sizes\". But internally, the git options\nfor dealing with such things use \"unsigned long\". We could change that\n(we are limiting sizes to 4GB on 32-bit systems, while most modern\n32-bit systems can handle >4GB files), but I don't know if anybody\nactually cares, and it would be a lot of error-prone work (we have to\nchange the type to off_t or similar, but then try to handle all of the\nfallout throughout the code where we use \"unsigned long\").\n\nSo rather than try for something abstract, I tried to keep it to what\ngit does internally, which works pretty well in practice. As with --int,\nyou'll get an error if you try to use too large an integer.\n\n Documentation/git-config.txt | 6 ++++++\n builtin/config.c             | 8 ++++++++\n t/t1300-repo-config.sh       | 7 +++++++\n 3 files changed, 21 insertions(+)\n\ndiff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\nindex 2dbe486..a8b6626 100644\n--- a/Documentation/git-config.txt\n+++ b/Documentation/git-config.txt\n@@ -165,6 +165,12 @@ See also <<FILES>>.\n \t'git config' will ensure that the output matches the format of\n \teither --bool or --int, as described above.\n \n+--ulong::\n+\tAs with `--int`, 'git config' will ensure that the output is a\n+\tsimple decimal number, and will respect value suffixes. Unlike\n+\t`--int`, the range of acceptable integers will be non-negative\n+\tand go higher (depending on your platform, but at least 2^32-1).\n+\n --path::\n \t'git-config' will expand leading '{tilde}' to the value of\n \t'$HOME', and '{tilde}user' to the home directory for the\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 4010c43..f7a46bf 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -47,6 +47,7 @@ static int respect_includes = -1;\n #define TYPE_INT (1<<1)\n #define TYPE_BOOL_OR_INT (1<<2)\n #define TYPE_PATH (1<<3)\n+#define TYPE_ULONG (1<<4)\n \n static struct option builtin_config_options[] = {\n \tOPT_GROUP(N_(\"Config file location\")),\n@@ -73,6 +74,7 @@ static struct option builtin_config_options[] = {\n \tOPT_BIT(0, \"bool\", &types, N_(\"value is \\\"true\\\" or \\\"false\\\"\"), TYPE_BOOL),\n \tOPT_BIT(0, \"int\", &types, N_(\"value is decimal number\"), TYPE_INT),\n \tOPT_BIT(0, \"bool-or-int\", &types, N_(\"value is --bool or --int\"), TYPE_BOOL_OR_INT),\n+\tOPT_BIT(0, \"ulong\", &types, N_(\"value is large unsigned decimal number\"), TYPE_ULONG),\n \tOPT_BIT(0, \"path\", &types, N_(\"value is a path (file or directory name)\"), TYPE_PATH),\n \tOPT_GROUP(N_(\"Other\")),\n \tOPT_BOOLEAN('z', \"null\", &end_null, N_(\"terminate values with NUL byte\")),\n@@ -129,6 +131,8 @@ static int collect_config(const char *key_, const char *value_, void *cb)\n \t}\n \tif (types == TYPE_INT)\n \t\tsprintf(value, \"%d\", git_config_int(key_, value_?value_:\"\"));\n+\telse if (types == TYPE_ULONG)\n+\t\tsprintf(value, \"%lu\", git_config_ulong(key_, value_ ? value_ : \"\"));\n \telse if (types == TYPE_BOOL)\n \t\tvptr = git_config_bool(key_, value_) ? \"true\" : \"false\";\n \telse if (types == TYPE_BOOL_OR_INT) {\n@@ -268,6 +272,10 @@ static char *normalize_value(const char *key, const char *value)\n \t\t\tint v = git_config_int(key, value);\n \t\t\tsprintf(normalized, \"%d\", v);\n \t\t}\n+\t\telse if (types == TYPE_ULONG)\n+\t\t\tsprintf(normalized, \"%lu\",\n+\t\t\t\tgit_config_ulong(key, value));\n+\n \t\telse if (types == TYPE_BOOL)\n \t\t\tsprintf(normalized, \"%s\",\n \t\t\t\tgit_config_bool(key, value) ? \"true\" : \"false\");\ndiff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\nindex 3c5ec4b..f57fb27 100755\n--- a/t/t1300-repo-config.sh\n+++ b/t/t1300-repo-config.sh\n@@ -669,6 +669,13 @@ test_expect_success 'invalid unit' '\n \ttest_cmp actual expect\n '\n \n+test_expect_success 'large numbers via --ulong' '\n+\tgit config big.file 3g &&\n+\techo 3221225472 >expect &&\n+\tgit config --ulong big.file >actual &&\n+\ttest_cmp expect actual\n+'\n+\n cat > expect << EOF\n true\n false\n-- \n1.8.4.rc2.28.g6bb5f3f\n"},{"id":"225575","messageId":"20130820224830.GC24766@sigill.intra.peff.net","threadId":"34735","inReplyTo":"5213F146.1010509@googlemail.com","subject":"Re: [PATCH 0/2] git-config and large integers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-08-20T22:48:30Z","receivedAt":"2013-08-20T22:48:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 21, 2013 at 12:44:22AM +0200, Stefan Beller wrote:\n\n> On 08/21/2013 12:39 AM, Jeff King wrote:\n> > I was playing with a hook for file size limits that wanted to store the\n> > limit in git-config. It turns out we don't do a very good job of big\n> > integers:\n> > \n> >   $ git config foo.size 2g\n> >   $ git config --int foo.size\n> >   -2147483648\n> > \n> > Oops. After this series, we properly notice the error:\n> > \n> >   $ git config --int foo.size\n> >   fatal: bad config value for 'foo.size' in .git/config\n> > \n> > and even better, provide a way to access large values:\n> > \n> >   $ git config --ulong foo.size\n> >   2147483648\n> > \n> \n> int, ulong...\n> How large will those be, I'd guess they are machine dependent?\n> So int being 32 bits as usual, but not on all machines.\n> (Those, which don't have 32 bits, are maybe not relevant anyways?)\n\nYes, machine dependent. See the discussion in the patches themselves.\nIt's kind of ugly, but it matches what git does internally (and we\nproperly detect range errors at runtime).\n\n-Peff\n"},{"id":"225576","messageId":"20130820225745.GL4110@google.com","threadId":"34735","inReplyTo":"20130820224710.GB24766@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] teach git-config to output large integers","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-08-20T22:57:45Z","receivedAt":"2013-08-20T22:57:45Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> I kind of hate the name \"--ulong\". I wanted to call it \"--size\" or\n> something and abstract away the actual platform representation, and just\n> make it \"big enough for file sizes\".\n\nYes, something like --size would be more pleasant.\n\nIt could still use unsigned long internally.  My only worry about\n--size is that it does not make it clear we are talking about file\nsizes and not in-memory sizes (size_t), and I'm not too worried about\nthat.\n\n[...]\n> --- a/builtin/config.c\n> +++ b/builtin/config.c\n[...]\n> @@ -268,6 +272,10 @@ static char *normalize_value(const char *key, const char *value)\n>  \t\t\tint v = git_config_int(key, value);\n>  \t\t\tsprintf(normalized, \"%d\", v);\n>  \t\t}\n> +\t\telse if (types == TYPE_ULONG)\n> +\t\t\tsprintf(normalized, \"%lu\",\n> +\t\t\t\tgit_config_ulong(key, value));\n> +\n>  \t\telse if (types == TYPE_BOOL)\n\nStyle: uncuddled \"else\", stray blank line.  (The former was already\nthere, but it still stands out.)  I think\n\n\t\tif (types == TYPE_INT) {\n\t\t\t...\n\t\t} else if (types == TYPE_ULONG) {\n\t\t\t...\n\t\t} else if (types == TYPE_BOOL) {\n\t\t\t...\n\t\t} else if (types == TYPE_BOOL_OR_INT) {\n\t\t\t...\n\t\t} else {\n\t\t\t...\n\t\t}\n\nwould be easiest to read.\n\nThanks for taking this on.\n\nSincerely,\nJonathan\n"},{"id":"225578","messageId":"xmqqli3wufmc.fsf@gitster.dls.corp.google.com","threadId":"34735","inReplyTo":"20130820223953.GA3429@sigill.intra.peff.net","subject":"Re: [PATCH 0/2] git-config and large integers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-20T23:06:19Z","receivedAt":"2013-08-20T23:06:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I was playing with a hook for file size limits that wanted to store the\n> limit in git-config. It turns out we don't do a very good job of big\n> integers:\n>\n>   $ git config foo.size 2g\n>   $ git config --int foo.size\n>   -2147483648\n>\n> Oops. After this series, we properly notice the error:\n>\n>   $ git config --int foo.size\n>   fatal: bad config value for 'foo.size' in .git/config\n>\n> and even better, provide a way to access large values:\n>\n>   $ git config --ulong foo.size\n>   2147483648\n\nI may be missing something, but why do we even need a new option for\nthe command that is known to always produce textual output?\n\nAs you said \"Oops\", the first example that shows a string of digits\nprefixed by a minus sign for input \"2g\" is buggy, and I think it is\nperfectly reasonable to fix it to show a stringified representation\nof 2*1024*1024*1024 when asked for \"--int\".\n\nWhat am I missing???\n"},{"id":"225579","messageId":"20130820230749.GM4110@google.com","threadId":"34735","inReplyTo":"20130820224256.GA24766@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] config: properly range-check integer values","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-08-20T23:07:49Z","receivedAt":"2013-08-20T23:07:49Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> I added a test. It would not fail on existing 32-bit systems, but would\n> on existing LP64 systems. It will pass with the new code on both.\n> However, it will fail on ILP64 systems (because their int is large, and\n> can represent 3GB). I'm not sure if any such systems are in wide use\n> (SPARC64?), but we would want a prereq in that case, I guess. I'm\n> inclined to wait to see if it actually fails for anybody.\n\nYuck.\n\nWhat will go wrong if \"git config --int\" starts returning numbers too\nlarge to fit in an 'int'?  That can already happen if \"git\" and a\ncommand that uses it are built for different ABIs (e.g., ILP64 git,\n32-bit custom tool that calls git).\n\nIt's possible that what the test should be checking for is \"either\nreturns a sane answer or fails\" (which would pass regardless of ABI).\nSomething like:\n\n\ttest_expect_success 'large integers do not confuse config --int' '\n\t\tgit config giga.crash 3g &&\n\t\ttest_might_fail git config --int giga.crash >actual &&\n\t\techo 3221225472 >expect &&\n\t\t{\n\t\t\ttest_cmp expect actual ||\n\t\t\ttest_must_fail git config --int giga.crash\n\t\t}\n\t'\n\nSensible?\nJonathan\n"},{"id":"225580","messageId":"xmqqhaekudzp.fsf@gitster.dls.corp.google.com","threadId":"34735","inReplyTo":"xmqqli3wufmc.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 0/2] git-config and large integers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-20T23:41:30Z","receivedAt":"2013-08-20T23:41:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Jeff King <peff@peff.net> writes:\n>\n>> I was playing with a hook for file size limits that wanted to store the\n>> limit in git-config. It turns out we don't do a very good job of big\n>> integers:\n>>\n>>   $ git config foo.size 2g\n>>   $ git config --int foo.size\n>>   -2147483648\n>>\n>> Oops. After this series, we properly notice the error:\n>>\n>>   $ git config --int foo.size\n>>   fatal: bad config value for 'foo.size' in .git/config\n>>\n>> and even better, provide a way to access large values:\n>>\n>>   $ git config --ulong foo.size\n>>   2147483648\n>\n> I may be missing something, but why do we even need a new option for\n> the command that is known to always produce textual output?\n>\n> As you said \"Oops\", the first example that shows a string of digits\n> prefixed by a minus sign for input \"2g\" is buggy, and I think it is\n> perfectly reasonable to fix it to show a stringified representation\n> of 2*1024*1024*1024 when asked for \"--int\".\n>\n> What am I missing???\n\nIf this applied on the writing side, I would understand it very\nmuch, i.e.\n\n\t$ git config --int32 foo.size 2g\n        fatal: \"2g\" is too large to be read as \"int32\".\n\nand as a complement it may make sense as a warning mechanism to also\nerror out when existing value does not fit on the \"platform\" int, so\nyour \n\n>>   $ git config --int foo.size\n>>   fatal: bad config value for 'foo.size' in .git/config\n\nmight make sense (even though I'd suggest being more explicit than\n\"bad value\" in this case---\"the value specified will not fit when\nused in a variable of type int on this platform\").  When .git/config\nis shared on two different boxes (think: NFS), the size of \"int\"\nmight be different between them, so the logic to produce such a\nwarning may have to explicitly check against int32_t, not platform\nint and say \"will not fit in 'int' on some machines\".\n\nI dunno.\n\n\n\t\n"},{"id":"225588","messageId":"20130821023429.GA25296@sigill.intra.peff.net","threadId":"34735","inReplyTo":"xmqqli3wufmc.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 0/2] git-config and large integers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-08-21T02:34:29Z","receivedAt":"2013-08-21T02:34:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 20, 2013 at 04:06:19PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > I was playing with a hook for file size limits that wanted to store the\n> > limit in git-config. It turns out we don't do a very good job of big\n> > integers:\n> >\n> >   $ git config foo.size 2g\n> >   $ git config --int foo.size\n> >   -2147483648\n> >\n> > Oops. After this series, we properly notice the error:\n> >\n> >   $ git config --int foo.size\n> >   fatal: bad config value for 'foo.size' in .git/config\n> >\n> > and even better, provide a way to access large values:\n> >\n> >   $ git config --ulong foo.size\n> >   2147483648\n> \n> I may be missing something, but why do we even need a new option for\n> the command that is known to always produce textual output?\n\nWe could do all math with int64_t (for example) and then print the\nstringified representation. But then we would not be matching the same\nchecks that git is doing internally.\n\nFor example, on a 32-bit system, setting core.packedGitLimit to 4G will\nproduce an error for \"git config --int core.packedgitlimit\", as we\ncannot represent the size internally. We could do the conversion in such\na way that we print the correct size, but it does not represent what\nhappens when \"git pack-objects\" is run (you get the same error).\n\n> As you said \"Oops\", the first example that shows a string of digits\n> prefixed by a minus sign for input \"2g\" is buggy, and I think it is\n> perfectly reasonable to fix it to show a stringified representation\n> of 2*1024*1024*1024 when asked for \"--int\".\n\nThe negative value is a little bit of a sidetrack. For both \"git config\n--int\" and for internal use, we do not correctly range-check integer\nvalues. And that's why we print the negative value, when we should say\n\"this is a bogus out-of-range value\". The latter is what we have always\ndone for values outside of range, both internal and external, and it is\nonly that our range check was bogus (and we fed negative crap to the\ncode instead of complaining). That is fixed by the first patch.\n\nAnd that leads to the second patch. The \"--int\" option provides a range\ncheck of (typically) -2^32 to 2^32-1. But many of our values internally\nuse a larger range. We can either drop that range check (which means we\nwill let you inspect values with config that git internally will barf\non, with no clue), or we need to add another option with a different\nrange to retrieve those values. I chose to add another option because I\nthink the range check has value.\n\nDoes that explain the problem more fully?\n\n-Peff\n"},{"id":"225589","messageId":"20130821024355.GB25296@sigill.intra.peff.net","threadId":"34735","inReplyTo":"xmqqhaekudzp.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 0/2] git-config and large integers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-08-21T02:43:55Z","receivedAt":"2013-08-21T02:43:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 20, 2013 at 04:41:30PM -0700, Junio C Hamano wrote:\n\n> If this applied on the writing side, I would understand it very\n> much, i.e.\n> \n> \t$ git config --int32 foo.size 2g\n>         fatal: \"2g\" is too large to be read as \"int32\".\n\nIt does, by the way. When you request a type on the writing side, we\nnormalize (and complain in the same way as we do when reading).\n\n> and as a complement it may make sense as a warning mechanism to also\n> error out when existing value does not fit on the \"platform\" int, so\n> your \n> \n> >>   $ git config --int foo.size\n> >>   fatal: bad config value for 'foo.size' in .git/config\n> \n> might make sense (even though I'd suggest being more explicit than\n> \"bad value\" in this case---\"the value specified will not fit when\n> used in a variable of type int on this platform\").\n\nYes, the error message is terrible, and I think an extra patch on top to\nimprove it is worth doing. But note that I am not introducing that error\nhere at all. On 32-bit systems, we already correctly range-checked and\nproduced that error. It is only on 64-bit systems that the range check\nwas flat out wrong. It checked against \"long\"'s precision, but then cast\nthe result to an int, losing bits. A possibly worse example than the\nnegative one is:\n\n  $ git config foo.bar 4g\n  $ git config --int foo.bar\n  0\n\nAgain, that is what git's internal code is seeing. And that is why\nkeeping the range check for git-config has value: it lets you see what\ngit would see internally.\n\n> When .git/config is shared on two different boxes (think: NFS), the\n> size of \"int\" might be different between them, so the logic to produce\n> such a warning may have to explicitly check against int32_t, not\n> platform int and say \"will not fit in 'int' on some machines\".\n\nI don't really see the value in that. You can always write whatever you\nlike in the config file. The reader is responsible during parsing for\nsaying \"Hey, I am 32-bit and I can't handle this\". And we already do\nthat, and it works fine. So if you have an NFS-shared .git/config, and\nyou set \"pack.deltacachesize\" to \"4g\", a 64-bit machine will do fine\nwith that, and a 32-bit machine will complain. Which seems like the only\nsane thing to do.\n\nThere are a few config options that use \"unsigned long\" that I would\nargue should be \"off_t\" or something (for example,\ncore.bigFileThreshold, which cannot be more than 4G on a 32-bit machine,\nsimply because we can't represent the size. On the other hand, there is\nprobably a ton of stuff that does not work with 4G files on such a\nsystem, because we use unsigned long all over the place inside the\ncode).\n-Peff\n"},{"id":"225590","messageId":"20130821025503.GC25296@sigill.intra.peff.net","threadId":"34735","inReplyTo":"20130820230749.GM4110@google.com","subject":"Re: [PATCH 1/2] config: properly range-check integer values","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-08-21T02:55:04Z","receivedAt":"2013-08-21T02:55:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 20, 2013 at 04:07:49PM -0700, Jonathan Nieder wrote:\n\n> > I added a test. It would not fail on existing 32-bit systems, but would\n> > on existing LP64 systems. It will pass with the new code on both.\n> > However, it will fail on ILP64 systems (because their int is large, and\n> > can represent 3GB). I'm not sure if any such systems are in wide use\n> > (SPARC64?), but we would want a prereq in that case, I guess. I'm\n> > inclined to wait to see if it actually fails for anybody.\n> \n> Yuck.\n> \n> What will go wrong if \"git config --int\" starts returning numbers too\n> large to fit in an 'int'?  That can already happen if \"git\" and a\n> command that uses it are built for different ABIs (e.g., ILP64 git,\n> 32-bit custom tool that calls git).\n\nI'm not sure I understand your question. \"git config --int\" cannot\nreturn numbers that do not fit in an \"int\", since we use an int as an\nintermediate representation type. The intent of the code is to\nrange-check the input and complain. But the code gets it wrong, and\nsometimes truncates the value instead. So we continue to never show\nanything that would not fit in an \"int\", but now our range-check\ncorrectly complains rather than truncating in some cases.\n\nIf you are worried about a 32-bit command calling an ILP64 git, then\nnothing is made worse by this patch. We return the same range of values\nas before.\n\nShort of adding \"--int32\", etc to git-config, I don't think git can be\nresponsible for this situation. It can only say \"This does not fit in my\ninternal representation, and I would barf on it\". If you do not tell it\nthat your int is smaller, then it cannot say \"you would barf on it\".\n\n> It's possible that what the test should be checking for is \"either\n> returns a sane answer or fails\" (which would pass regardless of ABI).\n> Something like:\n> \n> \ttest_expect_success 'large integers do not confuse config --int' '\n> \t\tgit config giga.crash 3g &&\n> \t\ttest_might_fail git config --int giga.crash >actual &&\n> \t\techo 3221225472 >expect &&\n> \t\t{\n> \t\t\ttest_cmp expect actual ||\n> \t\t\ttest_must_fail git config --int giga.crash\n> \t\t}\n> \t'\n> \n> Sensible?\n\nYes, that would cover an ILP64 system, though it very much muddies the\npoint of the test (which is to find a value that is represented by a\nlong, but not an int; such a value does not exist at all on ILP64).\n\nWhich is why I wondered about avoiding it with a prerequisite.\n\n-Peff\n"},{"id":"225591","messageId":"20130821030036.GD25296@sigill.intra.peff.net","threadId":"34735","inReplyTo":"20130820225745.GL4110@google.com","subject":"Re: [PATCH 2/2] teach git-config to output large integers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-08-21T03:00:36Z","receivedAt":"2013-08-21T03:00:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 20, 2013 at 03:57:45PM -0700, Jonathan Nieder wrote:\n\n> Jeff King wrote:\n> \n> > I kind of hate the name \"--ulong\". I wanted to call it \"--size\" or\n> > something and abstract away the actual platform representation, and just\n> > make it \"big enough for file sizes\".\n> \n> Yes, something like --size would be more pleasant.\n> \n> It could still use unsigned long internally.  My only worry about\n> --size is that it does not make it clear we are talking about file\n> sizes and not in-memory sizes (size_t), and I'm not too worried about\n> that.\n\nI almost sent it as \"--size\" with unsigned long internally. But try\nwriting the documentation for it. You want to say something like \"it's\nbig enough to handle file sizes\". Except that on 32-bit, it's _not_.\nIt's only 4G.\n\nYou really want something that uses off_t internally, so 32-bit systems\nwith largefile support do the sane thing. But now you have no way of\nemulating the way that git parses stuff internally. You cannot say \"git\nconfig --size core.bigFileThreshold\" and get the same results that git\nwill have internally when it looks at that file (because it uses\n\"unsigned long\" internally\").\n\nI think there is an argument to be made that git should be using off_t\ninternally for such things. But it is a lot of code to change and check,\nand I'm not sure that anybody even really cares that much.\n\n> Style: uncuddled \"else\", stray blank line.  (The former was already\n> there, but it still stands out.)  I think\n\nYes, I was trying to follow the existing style (but obviously the extra\nline was just a typo).\n\n> \t\tif (types == TYPE_INT) {\n> \t\t\t...\n> \t\t} else if (types == TYPE_ULONG) {\n> \t\t\t...\n> \t\t} else if (types == TYPE_BOOL) {\n> \t\t\t...\n> \t\t} else if (types == TYPE_BOOL_OR_INT) {\n> \t\t\t...\n> \t\t} else {\n> \t\t\t...\n> \t\t}\n> \n> would be easiest to read.\n\nBut that is adding brackets for one-liner conditional bodies that do not\nneed it. Which is more evil?\n\nMy usual method is to do what looks the most readable to me, but I admit\nI have a hard time using my intuition with the cuddled-elses, as I think\nthey look terrible (yes, I'm aware they are in our style guide and I am\nnot arguing to take them out, only that my personal sense of \"looks\ngood\" is helpless with them).\n\n-Peff\n"},{"id":"225597","messageId":"20130821043841.GA2802@elie.Belkin","threadId":"34735","inReplyTo":"20130821030036.GD25296@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] teach git-config to output large integers","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-08-21T04:38:41Z","receivedAt":"2013-08-21T04:38:41Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> I almost sent it as \"--size\" with unsigned long internally. But try\n> writing the documentation for it. You want to say something like \"it's\n> big enough to handle file sizes\". Except that on 32-bit, it's _not_.\n> It's only 4G.\n>\n> You really want something that uses off_t internally, so 32-bit systems\n> with largefile support do the sane thing. But now you have no way of\n> emulating the way that git parses stuff internally.\n\nLet's take a step back for a moment.  What problem is this patch\nsolving?\n\n>From the motivating example, I thought it was\n\n\tWhen reading or writing an integer config item, git sometimes\n\tencounters integer overflow and doesn't know how to deal with it.\n\tWorse, this means that some meaningful values are unrepresentable\n\tin config files.  Fix it in two steps:\n\n\t 1. Catch overflow, and error out instead of pretending to be\n\t    able to handle it.\n\n\t 2. Provide at least an option to use a wider integer type and\n\t    handle larger meaningful values.\n\n\tThis involves a new option --size instead of making --int use\n\tintmax_t for the following compatibility reason: ...\n\nFor example, the compatibility reason could be that some scripts\ncalling \"git config\" were not able to handle large integers and that\nwe do not want to expose them to unexpectedly large values.\n\nBut that reason doesn't sound realistic to me.  So what is the actual\nreason not to always use a wider range?\n\nThat is what I was trying to get at in discussing the test.  It is not\n\"We would like --int to reject values higher than this, but some\nplatforms do not allow us to\", but \"Either rejecting this value, or\neven better, computing the right size and printing it, is an\nacceptable behavior, and this test checks for those.\"\n\nHoping that clarifies,\nJonathan\n"},{"id":"225598","messageId":"20130821050053.GA875@sigill.intra.peff.net","threadId":"34735","inReplyTo":"20130821043841.GA2802@elie.Belkin","subject":"Re: [PATCH 2/2] teach git-config to output large integers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-08-21T05:00:53Z","receivedAt":"2013-08-21T05:00:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 20, 2013 at 09:38:41PM -0700, Jonathan Nieder wrote:\n\n> Jeff King wrote:\n> \n> > I almost sent it as \"--size\" with unsigned long internally. But try\n> > writing the documentation for it. You want to say something like \"it's\n> > big enough to handle file sizes\". Except that on 32-bit, it's _not_.\n> > It's only 4G.\n> >\n> > You really want something that uses off_t internally, so 32-bit systems\n> > with largefile support do the sane thing. But now you have no way of\n> > emulating the way that git parses stuff internally.\n> \n> Let's take a step back for a moment.  What problem is this patch\n> solving?\n\nThat you cannot currently ask git-config for a value larger than 2g.\n\n> From the motivating example, I thought it was\n> \n> \tWhen reading or writing an integer config item, git sometimes\n> \tencounters integer overflow and doesn't know how to deal with it.\n> \tWorse, this means that some meaningful values are unrepresentable\n> \tin config files.  Fix it in two steps:\n> \n> \t 1. Catch overflow, and error out instead of pretending to be\n> \t    able to handle it.\n\nNo, this first step is not being added. It is already the case that we\nerror out for overflow. The first patch is catching a case where we\nfailed to do that properly, and instead truncated. And it has nothing to\ndo with git-config itself; it is only that we must use git-config to\ntest it from the shell. The same problem may happen internally (but\ntends not to, because large things tend to use git_config_ulong instead\nof git_config_int).\n\n> \t 2. Provide at least an option to use a wider integer type and\n> \t    handle larger meaningful values.\n> \n> \tThis involves a new option --size instead of making --int use\n> \tintmax_t for the following compatibility reason: ...\n> \n> For example, the compatibility reason could be that some scripts\n> calling \"git config\" were not able to handle large integers and that\n> we do not want to expose them to unexpectedly large values.\n> \n> But that reason doesn't sound realistic to me.  So what is the actual\n> reason not to always use a wider range?\n\nMy reason is that it does not represent the same range checks that git\nis doing internally (and which vary from platform to platform). So you\nmight ask git-config \"what is the value of pack.deltacachelimit\"; I\nwould expect it to apply the same range checks there that would be used\nby git-pack-objects.\n\n> That is what I was trying to get at in discussing the test.  It is not\n> \"We would like --int to reject values higher than this, but some\n> platforms do not allow us to\", but \"Either rejecting this value, or\n> even better, computing the right size and printing it, is an\n> acceptable behavior, and this test checks for those.\"\n\nYou are conflating the two patches, I think. The test we were discussing\nis for the _first_ patch, which fixes a bug in the range check. It is\nnot meant to test git-config in particular, but to test that values\nhigher than INT_MAX and lower than LONG_MAX are properly range-checked.\n\nForget the second patch for a moment. I believe the first one is a bug\nfix that we would want even if we do not take the second patch at all.\n\n-Peff\n"},{"id":"225599","messageId":"20130821063458.GB2802@elie.Belkin","threadId":"34735","inReplyTo":"20130821050053.GA875@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] teach git-config to output large integers","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-08-21T06:34:58Z","receivedAt":"2013-08-21T06:34:58Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n> On Tue, Aug 20, 2013 at 09:38:41PM -0700, Jonathan Nieder wrote:\n\n>> That is what I was trying to get at in discussing the test.  It is not\n>> \"We would like --int to reject values higher than this, but some\n>> platforms do not allow us to\", but \"Either rejecting this value, or\n>> even better, computing the right size and printing it, is an\n>> acceptable behavior, and this test checks for those.\"\n>\n> You are conflating the two patches, I think. The test we were discussing\n> is for the _first_ patch, which fixes a bug in the range check. It is\n> not meant to test git-config in particular, but to test that values\n> higher than INT_MAX and lower than LONG_MAX are properly range-checked.\n>\n> Forget the second patch for a moment. I believe the first one is a bug\n> fix that we would want even if we do not take the second patch at all.\n\nSure.  I'm not conflating the patches.  What I mean is that tests are\nsupposed to test desirable behavior, whatever that is --- they are not\nabout preventing all behavior changes but only about preventing\nregressions.\n\nSo talking about tests is a (perhaps overly roundabout) way to figure\nout the desirable behavior.\n\nIn particular, at first glance I would think computing 3 * 2^20\ninstead of erroring out would be a *good* behavior, not a regression.\nIf that's right, it doesn't make sense to me to go to careful lengths\neither to test that git continues to error out on most platforms, or\nto introduce new options to ensure \"git config --int\" continues to\nerror out.\n\nThat is what I am trying to understand.  Everything about the first\npatch except for the test makes sense to me, but the test doesn't.  As\nyou noted, we know the test won't pass on some platforms.  Why is it\nsomething we should *want* to pass?\n\nJonathan\n"},{"id":"227106","messageId":"20130908082724.GA27184@sigill.intra.peff.net","threadId":"34735","inReplyTo":"20130820223953.GA3429@sigill.intra.peff.net","subject":"[PATCHv2 0/5] git-config and large integers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-08T08:27:24Z","receivedAt":"2013-09-08T08:27:24Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Here's a re-roll (and re-design) of my series to help with \"git config\"\nand large integers. It takes the different approach we discussed of\nsimply removing (well, increasing) the range limits for \"git config\n--int\".\n\nI also improved the error messages for bogus config (patches 3-4).\n\n  [1/5]: config: factor out integer parsing from range checks\n  [2/5]: config: properly range-check integer values\n  [3/5]: config: set errno in numeric git_parse_* functions\n  [4/5]: config: make numeric parsing errors more clear\n  [5/5]: git-config: always treat --int as 64-bit internally\n\n-Peff\n"},{"id":"227107","messageId":"20130908082927.GA29554@sigill.intra.peff.net","threadId":"34735","inReplyTo":"20130908082724.GA27184@sigill.intra.peff.net","subject":"[PATCH 1/5] config: factor out integer parsing from range checks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-08T08:29:27Z","receivedAt":"2013-09-08T08:29:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we are parsing integers for config, we use an intmax_t\n(or uintmax_t) internally, and then check against the size\nof our result type at the end. We can parameterize the\nmaximum representable value, which will let us re-use the\nparsing code for a variety of range checks.\n\nUnfortunately, we cannot combine the signed and unsigned\nparsing functions easily, as we have to rely on the signed\nand unsigned C types internally.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThe fact that we have two nearly-identical functions next to each other\nreally bugs me (especially since I'm going to make identical changes to\nthem later in this series). But I think trying to factor them out into a\nsingle function would end up being even less readable. I'd be happy if\nsomebody wants to prove me wrong.\n\n config.c | 28 ++++++++++++++++++++++------\n 1 file changed, 22 insertions(+), 6 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 9f9bf0c..ee7c1f8 100644\n--- a/config.c\n+++ b/config.c\n@@ -468,7 +468,7 @@ static int parse_unit_factor(const char *end, uintmax_t *val)\n \treturn 0;\n }\n \n-static int git_parse_long(const char *value, long *ret)\n+static int git_parse_signed(const char *value, intmax_t *ret, intmax_t max)\n {\n \tif (value && *value) {\n \t\tchar *end;\n@@ -484,8 +484,7 @@ static int git_parse_long(const char *value, long *ret)\n \t\t\treturn 0;\n \t\tuval = abs(val);\n \t\tuval *= factor;\n-\t\tif ((uval > maximum_signed_value_of_type(long)) ||\n-\t\t    (abs(val) > uval))\n+\t\tif (uval > max || abs(val) > uval)\n \t\t\treturn 0;\n \t\tval *= factor;\n \t\t*ret = val;\n@@ -494,7 +493,7 @@ static int git_parse_long(const char *value, long *ret)\n \treturn 0;\n }\n \n-int git_parse_ulong(const char *value, unsigned long *ret)\n+int git_parse_unsigned(const char *value, uintmax_t *ret, uintmax_t max)\n {\n \tif (value && *value) {\n \t\tchar *end;\n@@ -508,8 +507,7 @@ int git_parse_ulong(const char *value, unsigned long *ret)\n \t\toldval = val;\n \t\tif (!parse_unit_factor(end, &val))\n \t\t\treturn 0;\n-\t\tif ((val > maximum_unsigned_value_of_type(long)) ||\n-\t\t    (oldval > val))\n+\t\tif (val > max || oldval > val)\n \t\t\treturn 0;\n \t\t*ret = val;\n \t\treturn 1;\n@@ -517,6 +515,24 @@ int git_parse_ulong(const char *value, unsigned long *ret)\n \treturn 0;\n }\n \n+static int git_parse_long(const char *value, long *ret)\n+{\n+\tintmax_t tmp;\n+\tif (!git_parse_signed(value, &tmp, maximum_signed_value_of_type(long)))\n+\t\treturn 0;\n+\t*ret = tmp;\n+\treturn 1;\n+}\n+\n+int git_parse_ulong(const char *value, unsigned long *ret)\n+{\n+\tuintmax_t tmp;\n+\tif (!git_parse_unsigned(value, &tmp, maximum_unsigned_value_of_type(long)))\n+\t\treturn 0;\n+\t*ret = tmp;\n+\treturn 1;\n+}\n+\n static void die_bad_config(const char *name)\n {\n \tif (cf && cf->name)\n-- \n1.8.4.2.g87d4a77\n"},{"id":"227108","messageId":"20130908083308.GB29554@sigill.intra.peff.net","threadId":"34735","inReplyTo":"20130908082724.GA27184@sigill.intra.peff.net","subject":"[PATCH 2/5] config: properly range-check integer values","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-08T08:33:08Z","receivedAt":"2013-09-08T08:33:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we look at a config value as an integer using the\ngit_config_int function, we carefully range-check the value\nwe get and complain if it is out of our range. But the range\nwe compare to is that of a \"long\", which we then cast to an\n\"int\" in the function's return value. This means that on\nsystems where \"int\" and \"long\" have different sizes (e.g.,\nLP64 systems), we may pass the range check, but then return\nnonsense by truncating the value as we cast it to an int.\n\nWe can solve this by converting git_parse_long into\ngit_parse_int, and range-checking the \"int\" range. Nobody\nactually cared that we used a \"long\" internally, since the\nresult was truncated anyway. And the only other caller of\ngit_parse_long is git_config_maybe_bool, which should be\nfine to just use int (though we will now forbid out-of-range\nnonsense like setting \"merge.ff\" to \"10g\" to mean \"true\",\nwhich is probably a good thing).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n\nI tested this with:\n\n\tdiff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\n\tindex b1a6365..5444952 100755\n\t--- a/t/t6500-gc.sh\n\t+++ b/t/t6500-gc.sh\n\t@@ -25,4 +25,8 @@ test_expect_success 'gc -h with invalid configuration' '\n\t \ttest_i18ngrep \"[Uu]sage\" broken/usage\n\t '\n\t \n\t+test_expect_success 'gc complains about out-of-range integers' '\n\t+\ttest_must_fail git -c gc.auto=3g gc --auto\n\t+'\n\t+\n\t test_done\n\nbut that test is horrible for a few reasons:\n\n  1. It depends on knowing that gc.auto uses git_config_int internally.\n\n  2. It passes without the patch on 32-bit platforms.\n\n  3. It won't pass with the patch on ILP64 systems.\n\nWe can't use \"git config --int\" to test it, because it's going to stop\nusing git_config_int later in the series. So I think we should just go\nwithout a test.\n\n config.c | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex ee7c1f8..0a65ac2 100644\n--- a/config.c\n+++ b/config.c\n@@ -515,10 +515,10 @@ static int git_parse_long(const char *value, long *ret)\n \treturn 0;\n }\n \n-static int git_parse_long(const char *value, long *ret)\n+static int git_parse_int(const char *value, int *ret)\n {\n \tintmax_t tmp;\n-\tif (!git_parse_signed(value, &tmp, maximum_signed_value_of_type(long)))\n+\tif (!git_parse_signed(value, &tmp, maximum_signed_value_of_type(int)))\n \t\treturn 0;\n \t*ret = tmp;\n \treturn 1;\n@@ -542,8 +542,8 @@ int git_config_int(const char *name, const char *value)\n \n int git_config_int(const char *name, const char *value)\n {\n-\tlong ret = 0;\n-\tif (!git_parse_long(value, &ret))\n+\tint ret;\n+\tif (!git_parse_int(value, &ret))\n \t\tdie_bad_config(name);\n \treturn ret;\n }\n@@ -575,10 +575,10 @@ int git_config_maybe_bool(const char *name, const char *value)\n \n int git_config_maybe_bool(const char *name, const char *value)\n {\n-\tlong v = git_config_maybe_bool_text(name, value);\n+\tint v = git_config_maybe_bool_text(name, value);\n \tif (0 <= v)\n \t\treturn v;\n-\tif (git_parse_long(value, &v))\n+\tif (git_parse_int(value, &v))\n \t\treturn !!v;\n \treturn -1;\n }\n-- \n1.8.4.2.g87d4a77\n"},{"id":"227109","messageId":"20130908083642.GC29554@sigill.intra.peff.net","threadId":"34735","inReplyTo":"20130908082724.GA27184@sigill.intra.peff.net","subject":"[PATCH 3/5] config: set errno in numeric git_parse_* functions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-08T08:36:42Z","receivedAt":"2013-09-08T08:36:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we are parsing an integer or unsigned long, we use\nthe strto*max functions, which properly set errno to ERANGE\nif we get a large value. However, we also do further range\nchecks after applying our multiplication factor, but do not\nset ERANGE. This means that a caller cannot tell if an error\nwas caused by ERANGE or if the input was simply not a valid\nnumber.\n\nThis patch teaches git_parse_signed and git_parse_unsigned\nreliably set ERANGE for range errors, and EINVAL for other\nerrors.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI'm a little iffy on using errno like this. The \"normal\" way in git\nwould be to return an enum like:\n\n  enum config_parse_error {\n    CONFIG_PARSE_SUCCESS = 0,\n    CONFIG_PARSE_RANGE = -1,\n    CONFIG_PARSE_INVALID_UNIT = -2\n  };\n\nBut that would be changing the return semantics of git_parse_ulong\n(which currently is \"0\" for fail, \"1\" for success) without changing its\nsignature. I figured this was the path of least resistance since we\nalready get ERANGE out of strtoimax, but I'm happy to switch it, too.\n\n config.c | 18 ++++++++++++++----\n 1 file changed, 14 insertions(+), 4 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 0a65ac2..7332b06 100644\n--- a/config.c\n+++ b/config.c\n@@ -480,16 +480,21 @@ static int git_parse_signed(const char *value, intmax_t *ret, intmax_t max)\n \t\tval = strtoimax(value, &end, 0);\n \t\tif (errno == ERANGE)\n \t\t\treturn 0;\n-\t\tif (!parse_unit_factor(end, &factor))\n+\t\tif (!parse_unit_factor(end, &factor)) {\n+\t\t\terrno = EINVAL;\n \t\t\treturn 0;\n+\t\t}\n \t\tuval = abs(val);\n \t\tuval *= factor;\n-\t\tif (uval > max || abs(val) > uval)\n+\t\tif (uval > max || abs(val) > uval) {\n+\t\t\terrno = ERANGE;\n \t\t\treturn 0;\n+\t\t}\n \t\tval *= factor;\n \t\t*ret = val;\n \t\treturn 1;\n \t}\n+\terrno = EINVAL;\n \treturn 0;\n }\n \n@@ -505,13 +510,18 @@ int git_parse_unsigned(const char *value, uintmax_t *ret, uintmax_t max)\n \t\tif (errno == ERANGE)\n \t\t\treturn 0;\n \t\toldval = val;\n-\t\tif (!parse_unit_factor(end, &val))\n+\t\tif (!parse_unit_factor(end, &val)) {\n+\t\t\terrno = EINVAL;\n \t\t\treturn 0;\n-\t\tif (val > max || oldval > val)\n+\t\t}\n+\t\tif (val > max || oldval > val) {\n+\t\t\terrno = ERANGE;\n \t\t\treturn 0;\n+\t\t}\n \t\t*ret = val;\n \t\treturn 1;\n \t}\n+\terrno = EINVAL;\n \treturn 0;\n }\n \n-- \n1.8.4.2.g87d4a77\n"},{"id":"227110","messageId":"20130908083822.GD29554@sigill.intra.peff.net","threadId":"34735","inReplyTo":"20130908082724.GA27184@sigill.intra.peff.net","subject":"[PATCH 4/5] config: make numeric parsing errors more clear","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-08T08:38:22Z","receivedAt":"2013-09-08T08:38:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If we try to parse an integer config argument and get a\nnumber outside of the representable range, we die with the\ncryptic message: \"bad config value for '%s'\".\n\nWe can improve two things:\n\n  1. Show the value that produced the error (e.g., bad\n     config value '3g' for 'foo.bar').\n\n  2. Mention the reason the value was rejected (e.g.,\n     \"invalid unit\" versus \"out of range\").\n\nA few tests need to be updated with the new output, but that\nshould not be representative of real-world breakage, as\nscripts should not be depending on the exact text of our\nstderr output, which is subject to i18n anyway.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI was tempted to clean up the tortured string construction in\ndie_bad_number(), but my understanding is that translators prefer to see\nthe string in as whole a piece as we can get it, rather than building it\npiecemeal from \"if\"s. And though this isn't marked for translation, it\nprobably should be.\n\n config.c                | 17 ++++++++++++-----\n t/t1300-repo-config.sh  |  6 +++---\n t/t4055-diff-context.sh |  2 +-\n 3 files changed, 16 insertions(+), 9 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 7332b06..87f87ba 100644\n--- a/config.c\n+++ b/config.c\n@@ -543,18 +543,25 @@ int git_config_int(const char *name, const char *value)\n \treturn 1;\n }\n \n-static void die_bad_config(const char *name)\n+static void die_bad_number(const char *name, const char *value)\n {\n+\tconst char *reason = errno == ERANGE ?\n+\t\t\t     \"out of range\" :\n+\t\t\t     \"invalid unit\";\n+\tif (!value)\n+\t\tvalue = \"\";\n+\n \tif (cf && cf->name)\n-\t\tdie(\"bad config value for '%s' in %s\", name, cf->name);\n-\tdie(\"bad config value for '%s'\", name);\n+\t\tdie(\"bad numeric config value '%s' for '%s' in %s: %s\",\n+\t\t    value, name, cf->name, reason);\n+\tdie(\"bad numeric config value '%s' for '%s': %s\", value, name, reason);\n }\n \n int git_config_int(const char *name, const char *value)\n {\n \tint ret;\n \tif (!git_parse_int(value, &ret))\n-\t\tdie_bad_config(name);\n+\t\tdie_bad_number(name, value);\n \treturn ret;\n }\n \n@@ -562,7 +569,7 @@ unsigned long git_config_ulong(const char *name, const char *value)\n {\n \tunsigned long ret;\n \tif (!git_parse_ulong(value, &ret))\n-\t\tdie_bad_config(name);\n+\t\tdie_bad_number(name, value);\n \treturn ret;\n }\n \ndiff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\nindex c4a7d84..20aee6e 100755\n--- a/t/t1300-repo-config.sh\n+++ b/t/t1300-repo-config.sh\n@@ -657,11 +657,11 @@ test_expect_success 'invalid unit' '\n \techo 1auto >expect &&\n \tgit config aninvalid.unit >actual &&\n \ttest_cmp expect actual &&\n-\tcat > expect <<-\\EOF\n-\tfatal: bad config value for '\\''aninvalid.unit'\\'' in .git/config\n+\tcat >expect <<-\\EOF\n+\tfatal: bad numeric config value '\\''1auto'\\'' for '\\''aninvalid.unit'\\'' in .git/config: invalid unit\n \tEOF\n \ttest_must_fail git config --int --get aninvalid.unit 2>actual &&\n-\ttest_cmp actual expect\n+\ttest_i18ncmp expect actual\n '\n \n cat > expect << EOF\ndiff --git a/t/t4055-diff-context.sh b/t/t4055-diff-context.sh\nindex 97172b4..cd04543 100755\n--- a/t/t4055-diff-context.sh\n+++ b/t/t4055-diff-context.sh\n@@ -73,7 +73,7 @@ test_expect_success 'non-integer config parsing' '\n test_expect_success 'non-integer config parsing' '\n \tgit config diff.context no &&\n \ttest_must_fail git diff 2>output &&\n-\ttest_i18ngrep \"bad config value\" output\n+\ttest_i18ngrep \"bad numeric config value\" output\n '\n \n test_expect_success 'negative integer config parsing' '\n-- \n1.8.4.2.g87d4a77\n"},{"id":"227111","messageId":"20130908084002.GE29554@sigill.intra.peff.net","threadId":"34735","inReplyTo":"20130908082724.GA27184@sigill.intra.peff.net","subject":"[PATCH 5/5] git-config: always treat --int as 64-bit internally","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-08T08:40:02Z","receivedAt":"2013-09-08T08:40:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When you run \"git config --int\", the maximum size of integer\nyou get depends on how git was compiled, and what it\nconsiders to be an \"int\".\n\nThis is almost useful, because your scripts calling \"git\nconfig\" will behave similarly to git internally. But relying\non this is dubious; you have to actually know how git treats\neach value internally (e.g., int versus unsigned long),\nwhich is not documented and is subject to change. And even\nif you know it is \"unsigned long\", we do not have a\ngit-config option to match that behavior.\n\nFurthermore, you may simply be asking git to store a value\non your behalf (e.g., configuration for a hook). In that\ncase, the relevant range check has nothing at all to do with\ngit, but rather with whatever scripting tools you are using\n(and git has no way of knowing what the appropriate range is\nthere).\n\nNot only is the range check useless, but it is actively\nharmful, as there is no way at all for scripts to look\nat config variables with large values. For instance, one\ncannot reliably get the value of pack.packSizeLimit via\ngit-config. On an LP64 system, git happily uses a 64-bit\n\"unsigned long\" internally to represent the value, but the\nscript cannot read any value over 2G.\n\nIdeally, the \"--int\" option would simply represent an\narbitrarily large integer. For practical purposes, however,\na 64-bit integer is large enough, and is much easier to\nimplement (and if somebody overflows it, we will still\nnotice the problem, and not simply return garbage).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI kind of picked 64-bit as \"big enough\". But I suppose we could also go\nwith intmax_t. The only thing we can't do is an unsigned value.\n\nAlso, this is the first use of PRId64. I notice we have compatibility\nmacros for PRIu*, but I'm not sure what to put in one for PRId64.\n\n builtin/config.c       |  7 ++++---\n cache.h                |  1 +\n config.c               | 17 +++++++++++++++++\n t/t1300-repo-config.sh |  7 +++++++\n 4 files changed, 29 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 4010c43..8b182d2 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -128,7 +128,8 @@ static int collect_config(const char *key_, const char *value_, void *cb)\n \t\tmust_print_delim = 1;\n \t}\n \tif (types == TYPE_INT)\n-\t\tsprintf(value, \"%d\", git_config_int(key_, value_?value_:\"\"));\n+\t\tsprintf(value, \"%\"PRId64,\n+\t\t\tgit_config_int64(key_, value_ ? value_ : \"\"));\n \telse if (types == TYPE_BOOL)\n \t\tvptr = git_config_bool(key_, value_) ? \"true\" : \"false\";\n \telse if (types == TYPE_BOOL_OR_INT) {\n@@ -265,8 +266,8 @@ static char *normalize_value(const char *key, const char *value)\n \telse {\n \t\tnormalized = xmalloc(64);\n \t\tif (types == TYPE_INT) {\n-\t\t\tint v = git_config_int(key, value);\n-\t\t\tsprintf(normalized, \"%d\", v);\n+\t\t\tint64_t v = git_config_int64(key, value);\n+\t\t\tsprintf(normalized, \"%\"PRId64, v);\n \t\t}\n \t\telse if (types == TYPE_BOOL)\n \t\t\tsprintf(normalized, \"%s\",\ndiff --git a/cache.h b/cache.h\nindex 85b544f..ac4525a 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1190,6 +1190,7 @@ extern int git_config_int(const char *, const char *);\n extern int git_config_early(config_fn_t fn, void *, const char *repo_config);\n extern int git_parse_ulong(const char *, unsigned long *);\n extern int git_config_int(const char *, const char *);\n+extern int64_t git_config_int64(const char *, const char *);\n extern unsigned long git_config_ulong(const char *, const char *);\n extern int git_config_bool_or_int(const char *, const char *, int *);\n extern int git_config_bool(const char *, const char *);\ndiff --git a/config.c b/config.c\nindex 87f87ba..6588cf5 100644\n--- a/config.c\n+++ b/config.c\n@@ -534,6 +534,15 @@ static int git_parse_int(const char *value, int *ret)\n \treturn 1;\n }\n \n+static int git_parse_int64(const char *value, int64_t *ret)\n+{\n+\tintmax_t tmp;\n+\tif (!git_parse_signed(value, &tmp, maximum_signed_value_of_type(int64_t)))\n+\t\treturn 0;\n+\t*ret = tmp;\n+\treturn 1;\n+}\n+\n int git_parse_ulong(const char *value, unsigned long *ret)\n {\n \tuintmax_t tmp;\n@@ -565,6 +574,14 @@ int git_config_int(const char *name, const char *value)\n \treturn ret;\n }\n \n+int64_t git_config_int64(const char *name, const char *value)\n+{\n+\tint64_t ret;\n+\tif (!git_parse_int64(value, &ret))\n+\t\tdie_bad_number(name, value);\n+\treturn ret;\n+}\n+\n unsigned long git_config_ulong(const char *name, const char *value)\n {\n \tunsigned long ret;\ndiff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\nindex 20aee6e..b66c632 100755\n--- a/t/t1300-repo-config.sh\n+++ b/t/t1300-repo-config.sh\n@@ -652,6 +652,13 @@ test_expect_success numbers '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '--int is at least 64 bits' '\n+\tgit config giga.watts 121g &&\n+\techo 129922760704 >expect &&\n+\tgit config --int --get giga.watts >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'invalid unit' '\n \tgit config aninvalid.unit \"1auto\" &&\n \techo 1auto >expect &&\n-- \n1.8.4.2.g87d4a77\n"},{"id":"227178","messageId":"CAPig+cSPf0Cvk4Em71NX-ad46Ub7GysdhKUuqtjRU7fFTKOvSw@mail.gmail.com","threadId":"34735","inReplyTo":"20130908083642.GC29554@sigill.intra.peff.net","subject":"Re: [PATCH 3/5] config: set errno in numeric git_parse_* functions","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-09-09T00:36:35Z","receivedAt":"2013-09-09T00:36:35Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Sep 8, 2013 at 4:36 AM, Jeff King <peff@peff.net> wrote:\n> When we are parsing an integer or unsigned long, we use\n> the strto*max functions, which properly set errno to ERANGE\n> if we get a large value. However, we also do further range\n> checks after applying our multiplication factor, but do not\n> set ERANGE. This means that a caller cannot tell if an error\n> was caused by ERANGE or if the input was simply not a valid\n> number.\n>\n> This patch teaches git_parse_signed and git_parse_unsigned\n> reliably set ERANGE for range errors, and EINVAL for other\n\nMissing \"to\": s/reliably/to reliably/\n\nOr, if you don't like splitting the infinitive:\n\ns/reliably set ERANGE/to set ERANGE reliably/\n\n> errors.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n"},{"id":"227245","messageId":"xmqqsixd3jpz.fsf@gitster.dls.corp.google.com","threadId":"34735","inReplyTo":"20130908082724.GA27184@sigill.intra.peff.net","subject":"Re: [PATCHv2 0/5] git-config and large integers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-09T18:58:48Z","receivedAt":"2013-09-09T18:58:48Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Yikes. The updated series looks very sensible, but I already have\nthe previous one in 'next'.\n\nWill do the usual \"revert and requeue\" dance...\n"},{"id":"227252","messageId":"20130909195304.GB14021@sigill.intra.peff.net","threadId":"34735","inReplyTo":"CAPig+cSPf0Cvk4Em71NX-ad46Ub7GysdhKUuqtjRU7fFTKOvSw@mail.gmail.com","subject":"Re: [PATCH 3/5] config: set errno in numeric git_parse_* functions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-09T19:53:04Z","receivedAt":"2013-09-09T19:53:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Sep 08, 2013 at 08:36:35PM -0400, Eric Sunshine wrote:\n\n> On Sun, Sep 8, 2013 at 4:36 AM, Jeff King <peff@peff.net> wrote:\n> > When we are parsing an integer or unsigned long, we use\n> > the strto*max functions, which properly set errno to ERANGE\n> > if we get a large value. However, we also do further range\n> > checks after applying our multiplication factor, but do not\n> > set ERANGE. This means that a caller cannot tell if an error\n> > was caused by ERANGE or if the input was simply not a valid\n> > number.\n> >\n> > This patch teaches git_parse_signed and git_parse_unsigned\n> > reliably set ERANGE for range errors, and EINVAL for other\n> \n> Missing \"to\": s/reliably/to reliably/\n> \n> Or, if you don't like splitting the infinitive:\n> \n> s/reliably set ERANGE/to set ERANGE reliably/\n\nThanks. That is what I get for last-minute tweaking of the commit\nmessage (and I am OK with splitting the infinitive :) ).\n\n-Peff\n"}]}