[PATCH] revision: parse integer arguments to --max-count, --skip, etc., more carefully

[Junio C Hamano <gitster@pobox.com>]

The "rev-list" and other commands in the "log" family, being the
oldest part of the system, use their own custom argument parsers,
and integer values of some options are parsed with atoi(), which
allows a non-digit after the number (e.g., "1q") to be silently
ignored.  As a natural consequence, an argument that does not begin
with a digit (e.g., "q") silently becomes zero, too.

Switch to use strtol_i() and parse_timestamp() appropriately to
catch bogus input.

Note that one may naïvely expect that --max-count, --skip, etc., to
only take non-negative values, but we must allow them to also take
negative values, as an escape hatch to countermand a limit set by an
earlier option on the command line; the underlying variables are
initialized to (-1) and "--max-count=-1", for example, is a
legitimate way to reinitialize the limit.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 revision.c                 | 41 ++++++++++++++++++++++++++++----------
 t/t6005-rev-list-count.sh  | 15 +++++++++++++-
 t/t6009-rev-list-parent.sh | 11 ++++++++++
 3 files changed, 55 insertions(+), 12 deletions(-)

diff --git a/revision.c b/revision.c
index 0ae1c76db3..8cda6e43d4 100644
--- a/revision.c
+++ b/revision.c
@@ -2214,6 +2214,27 @@ static void add_message_grep(struct rev_info *revs, const char *pattern)
 	add_grep(revs, pattern, GREP_PATTERN_BODY);
 }
 
+static int parse_count(const char *arg)
+{
+	int count;
+
+	if (strtol_i(arg, 10, &count) < 0)
+		die("'%s': not an integer", arg);
+	return count;
+}
+
+static timestamp_t parse_age(const char *arg)
+{
+	timestamp_t num;
+	char *p;
+
+	errno = 0;
+	num = parse_timestamp(arg, &p, 10);
+	if (errno || *p || p == arg)
+		die("'%s': not a number of seconds since epoch", arg);
+	return num;
+}
+
 static int handle_revision_opt(struct rev_info *revs, int argc, const char **argv,
 			       int *unkc, const char **unkv,
 			       const struct setup_revision_opt* opt)
@@ -2240,29 +2261,27 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 	}
 
 	if ((argcount = parse_long_opt("max-count", argv, &optarg))) {
-		revs->max_count = atoi(optarg);
+		revs->max_count = parse_count(optarg);
 		revs->no_walk = 0;
 		return argcount;
 	} else if ((argcount = parse_long_opt("skip", argv, &optarg))) {
-		revs->skip_count = atoi(optarg);
+		revs->skip_count = parse_count(optarg);
 		return argcount;
 	} else if ((*arg == '-') && isdigit(arg[1])) {
 		/* accept -<digit>, like traditional "head" */
-		if (strtol_i(arg + 1, 10, &revs->max_count) < 0 ||
-		    revs->max_count < 0)
-			die("'%s': not a non-negative integer", arg + 1);
+		revs->max_count = parse_count(arg + 1);
 		revs->no_walk = 0;
 	} else if (!strcmp(arg, "-n")) {
 		if (argc <= 1)
 			return error("-n requires an argument");
-		revs->max_count = atoi(argv[1]);
+		revs->max_count = parse_count(argv[1]);
 		revs->no_walk = 0;
 		return 2;
 	} else if (skip_prefix(arg, "-n", &optarg)) {
-		revs->max_count = atoi(optarg);
+		revs->max_count = parse_count(optarg);
 		revs->no_walk = 0;
 	} else if ((argcount = parse_long_opt("max-age", argv, &optarg))) {
-		revs->max_age = atoi(optarg);
+		revs->max_age = parse_age(optarg);
 		return argcount;
 	} else if ((argcount = parse_long_opt("since", argv, &optarg))) {
 		revs->max_age = approxidate(optarg);
@@ -2274,7 +2293,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->max_age = approxidate(optarg);
 		return argcount;
 	} else if ((argcount = parse_long_opt("min-age", argv, &optarg))) {
-		revs->min_age = atoi(optarg);
+		revs->min_age = parse_age(optarg);
 		return argcount;
 	} else if ((argcount = parse_long_opt("before", argv, &optarg))) {
 		revs->min_age = approxidate(optarg);
@@ -2362,11 +2381,11 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 	} else if (!strcmp(arg, "--no-merges")) {
 		revs->max_parents = 1;
 	} else if (skip_prefix(arg, "--min-parents=", &optarg)) {
-		revs->min_parents = atoi(optarg);
+		revs->min_parents = parse_count(optarg);
 	} else if (!strcmp(arg, "--no-min-parents")) {
 		revs->min_parents = 0;
 	} else if (skip_prefix(arg, "--max-parents=", &optarg)) {
-		revs->max_parents = atoi(optarg);
+		revs->max_parents = parse_count(optarg);
 	} else if (!strcmp(arg, "--no-max-parents")) {
 		revs->max_parents = -1;
 	} else if (!strcmp(arg, "--boundary")) {
diff --git a/t/t6005-rev-list-count.sh b/t/t6005-rev-list-count.sh
index 0729f800c3..62538f5a02 100755
--- a/t/t6005-rev-list-count.sh
+++ b/t/t6005-rev-list-count.sh
@@ -18,13 +18,23 @@ test_expect_success 'no options' '
 '
 
 test_expect_success '--max-count' '
+	test_must_fail git rev-list --max-count=1q HEAD 2>error &&
+	grep "not an integer" error &&
+
 	test_stdout_line_count = 0 git rev-list HEAD --max-count=0 &&
 	test_stdout_line_count = 3 git rev-list HEAD --max-count=3 &&
 	test_stdout_line_count = 5 git rev-list HEAD --max-count=5 &&
-	test_stdout_line_count = 5 git rev-list HEAD --max-count=10
+	test_stdout_line_count = 5 git rev-list HEAD --max-count=10 &&
+	test_stdout_line_count = 5 git rev-list HEAD --max-count=-1
 '
 
 test_expect_success '--max-count all forms' '
+	test_must_fail git rev-list -1q HEAD 2>error &&
+	grep "not an integer" error &&
+	test_must_fail git rev-list --1 HEAD &&
+	test_must_fail git rev-list -n 1q HEAD 2>error &&
+	grep "not an integer" error &&
+
 	test_stdout_line_count = 1 git rev-list HEAD --max-count=1 &&
 	test_stdout_line_count = 1 git rev-list HEAD -1 &&
 	test_stdout_line_count = 1 git rev-list HEAD -n1 &&
@@ -32,6 +42,9 @@ test_expect_success '--max-count all forms' '
 '
 
 test_expect_success '--skip' '
+	test_must_fail git rev-list --skip 1q HEAD 2>error &&
+	grep "not an integer" error &&
+
 	test_stdout_line_count = 5 git rev-list HEAD --skip=0 &&
 	test_stdout_line_count = 2 git rev-list HEAD --skip=3 &&
 	test_stdout_line_count = 0 git rev-list HEAD --skip=5 &&
diff --git a/t/t6009-rev-list-parent.sh b/t/t6009-rev-list-parent.sh
index 5a67bbc760..9c9a8459af 100755
--- a/t/t6009-rev-list-parent.sh
+++ b/t/t6009-rev-list-parent.sh
@@ -62,6 +62,17 @@ test_expect_success 'setup roots, merges and octopuses' '
 	git checkout main
 '
 
+test_expect_success 'parse --max-parents & --min-parents' '
+	test_must_fail git rev-list --max-parents=1q HEAD 2>error &&
+	grep "not an integer" error &&
+
+	test_must_fail git rev-list --min-parents=1q HEAD 2>error &&
+	grep "not an integer" error &&
+
+	git rev-list --max-parents=1 --min-parents=1 HEAD &&
+	git rev-list --max-parents=-1 --min-parents=-1 HEAD
+'
+
 test_expect_success 'rev-list roots' '
 
 	check_revlist "--max-parents=0" one five
-- 
2.43.0