git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: git@vger.kernel.org
Cc: Junio C Hamano <gitster@pobox.com>, Jonathan Nieder <jrnieder@gmail.com>
Subject: [PATCH 5/5] git-config: always treat --int as 64-bit internally
Date: Sun, 8 Sep 2013 04:40:02 -0400	[thread overview]
Message-ID: <20130908084002.GE29554@sigill.intra.peff.net> (raw)
In-Reply-To: <20130908082724.GA27184@sigill.intra.peff.net>

When you run "git config --int", the maximum size of integer
you get depends on how git was compiled, and what it
considers to be an "int".

This is almost useful, because your scripts calling "git
config" will behave similarly to git internally. But relying
on this is dubious; you have to actually know how git treats
each value internally (e.g., int versus unsigned long),
which is not documented and is subject to change. And even
if you know it is "unsigned long", we do not have a
git-config option to match that behavior.

Furthermore, you may simply be asking git to store a value
on your behalf (e.g., configuration for a hook). In that
case, the relevant range check has nothing at all to do with
git, but rather with whatever scripting tools you are using
(and git has no way of knowing what the appropriate range is
there).

Not only is the range check useless, but it is actively
harmful, as there is no way at all for scripts to look
at config variables with large values. For instance, one
cannot reliably get the value of pack.packSizeLimit via
git-config. On an LP64 system, git happily uses a 64-bit
"unsigned long" internally to represent the value, but the
script cannot read any value over 2G.

Ideally, the "--int" option would simply represent an
arbitrarily large integer. For practical purposes, however,
a 64-bit integer is large enough, and is much easier to
implement (and if somebody overflows it, we will still
notice the problem, and not simply return garbage).

Signed-off-by: Jeff King <peff@peff.net>
---
I kind of picked 64-bit as "big enough". But I suppose we could also go
with intmax_t. The only thing we can't do is an unsigned value.

Also, this is the first use of PRId64. I notice we have compatibility
macros for PRIu*, but I'm not sure what to put in one for PRId64.

 builtin/config.c       |  7 ++++---
 cache.h                |  1 +
 config.c               | 17 +++++++++++++++++
 t/t1300-repo-config.sh |  7 +++++++
 4 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index 4010c43..8b182d2 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -128,7 +128,8 @@ static int collect_config(const char *key_, const char *value_, void *cb)
 		must_print_delim = 1;
 	}
 	if (types == TYPE_INT)
-		sprintf(value, "%d", git_config_int(key_, value_?value_:""));
+		sprintf(value, "%"PRId64,
+			git_config_int64(key_, value_ ? value_ : ""));
 	else if (types == TYPE_BOOL)
 		vptr = git_config_bool(key_, value_) ? "true" : "false";
 	else if (types == TYPE_BOOL_OR_INT) {
@@ -265,8 +266,8 @@ static char *normalize_value(const char *key, const char *value)
 	else {
 		normalized = xmalloc(64);
 		if (types == TYPE_INT) {
-			int v = git_config_int(key, value);
-			sprintf(normalized, "%d", v);
+			int64_t v = git_config_int64(key, value);
+			sprintf(normalized, "%"PRId64, v);
 		}
 		else if (types == TYPE_BOOL)
 			sprintf(normalized, "%s",
diff --git a/cache.h b/cache.h
index 85b544f..ac4525a 100644
--- a/cache.h
+++ b/cache.h
@@ -1190,6 +1190,7 @@ extern int git_config_int(const char *, const char *);
 extern int git_config_early(config_fn_t fn, void *, const char *repo_config);
 extern int git_parse_ulong(const char *, unsigned long *);
 extern int git_config_int(const char *, const char *);
+extern int64_t git_config_int64(const char *, const char *);
 extern unsigned long git_config_ulong(const char *, const char *);
 extern int git_config_bool_or_int(const char *, const char *, int *);
 extern int git_config_bool(const char *, const char *);
diff --git a/config.c b/config.c
index 87f87ba..6588cf5 100644
--- a/config.c
+++ b/config.c
@@ -534,6 +534,15 @@ static int git_parse_int(const char *value, int *ret)
 	return 1;
 }
 
+static int git_parse_int64(const char *value, int64_t *ret)
+{
+	intmax_t tmp;
+	if (!git_parse_signed(value, &tmp, maximum_signed_value_of_type(int64_t)))
+		return 0;
+	*ret = tmp;
+	return 1;
+}
+
 int git_parse_ulong(const char *value, unsigned long *ret)
 {
 	uintmax_t tmp;
@@ -565,6 +574,14 @@ int git_config_int(const char *name, const char *value)
 	return ret;
 }
 
+int64_t git_config_int64(const char *name, const char *value)
+{
+	int64_t ret;
+	if (!git_parse_int64(value, &ret))
+		die_bad_number(name, value);
+	return ret;
+}
+
 unsigned long git_config_ulong(const char *name, const char *value)
 {
 	unsigned long ret;
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 20aee6e..b66c632 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -652,6 +652,13 @@ test_expect_success numbers '
 	test_cmp expect actual
 '
 
+test_expect_success '--int is at least 64 bits' '
+	git config giga.watts 121g &&
+	echo 129922760704 >expect &&
+	git config --int --get giga.watts >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'invalid unit' '
 	git config aninvalid.unit "1auto" &&
 	echo 1auto >expect &&
-- 
1.8.4.2.g87d4a77

  parent reply	other threads:[~2013-09-08  8:40 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
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   ` Jeff King [this message]
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

  List information: http://vger.kernel.org/majordomo-info.html

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

  git send-email \
    --in-reply-to=20130908084002.GE29554@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jrnieder@gmail.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.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

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 read-only IMAP folder(s) and NNTP newsgroup(s).