Britton Kerin <britton.kerin@xxxxxxxxx> writes: > It tolerates non-numeric arguments and garbage after a number: > > For example: > > $ # -n 1 means same as -n 0: > $ git rev-list -n q newest_commit > $ git rev-list -n 0 newest_commit > $ # Garbage after number is tolerated: > $ git rev-list -n 1q newest_commit > 3be33f83695088d968cf084a1a08bdcde25a8d7a > $ git rev-list -n 2q newest_commit > 3be33f83695088d968cf084a1a08bdcde25a8d7a > 286e62e1b68e39334978e6222cbff187ecc17bcf Indeed, unlike the option parsing for most commands, "rev-list" and "log" family of commands in the oldest part of the system still use hand-crafted option parsing and most notably use atoi() instead of a more robust strtol() family of functions. The same issue is present for parsing things like "--max-count=1q" and "--skip=1q". Perhaps a change along this line, plus a few new tests, would suffice. There are others (like "--max-age" we see in the post context of the patch) that use atoi() but they probably should not share the same machinery (most importantly, the type of the value) as "--max-count", so I didn't touch it in the below. revision.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git c/revision.c w/revision.c index 00d5c29bfc..99e838bbd1 100644 --- c/revision.c +++ w/revision.c @@ -2223,6 +2223,15 @@ 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 || count < 0) + die("'%s': not a non-negative integer", arg); + return count; +} + 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) @@ -2249,26 +2258,24 @@ 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);