Re: [PATCH 06/19] reset.c: remove unnecessary variable 'i'
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:55:42
Martin von Zweigbergk [off-list ref] writes:
Throughout most of parse_args(), the variable 'i' remains at 0. In the remaining few cases, we can do pointer arithmentic on argv itself instead. --- This is clearly mostly a matter of taste. The remainder of the series does not depend on it in any way.
I agree that it indeed is a matter of taste between
(1) look at av[i], check with (i < ac) for the end, and increment i to
iterate over the arguments; and
(2) look at av[0], check with (0 < ac) for the end, and increment
av and decrement ac at the same time to iterate over the
arguments.
When (ac, av) appear as a pair, however, adjusting only av without
adjusting ac is asking for future trouble. It violates a common
expectation that av[ac] points at the NULL at the end of the list.
If a code chooses to use !av[0] as the terminating condition and
never looks at ac, then incrementing only av is fine, but in such a
case, the function probably should lose ac altogether.
quoted hunk
builtin/reset.c | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-)diff --git a/builtin/reset.c b/builtin/reset.c index 9473725..68be05c 100644 --- a/builtin/reset.c +++ b/builtin/reset.c@@ -199,7 +199,6 @@ static void die_if_unmerged_cache(int reset_type) } const char **parse_args(int argc, const char **argv, const char *prefix, const char **rev_ret) { - int i = 0; const char *rev = "HEAD"; unsigned char unused[20]; /*@@ -210,34 +209,34 @@ const char **parse_args(int argc, const char **argv, const char *prefix, const c * git reset [-opts] -- <paths>... * git reset [-opts] <paths>... * - * At this point, argv[i] points immediately after [-opts]. + * At this point, argv points immediately after [-opts]. */ - if (i < argc) { - if (!strcmp(argv[i], "--")) { - i++; /* reset to HEAD, possibly with paths */ - } else if (i + 1 < argc && !strcmp(argv[i+1], "--")) { - rev = argv[i]; - i += 2; + if (argc) { + if (!strcmp(argv[0], "--")) { + argv++; /* reset to HEAD, possibly with paths */ + } else if (argc > 1 && !strcmp(argv[1], "--")) { + rev = argv[0]; + argv += 2; } /* - * Otherwise, argv[i] could be either <rev> or <paths> and + * Otherwise, argv[0] could be either <rev> or <paths> and * has to be unambiguous. */ - else if (!get_sha1_committish(argv[i], unused)) { + else if (!get_sha1_committish(argv[0], unused)) { /* - * Ok, argv[i] looks like a rev; it should not + * Ok, argv[0] looks like a rev; it should not * be a filename. */ - verify_non_filename(prefix, argv[i]); - rev = argv[i++]; + verify_non_filename(prefix, argv[0]); + rev = *argv++; } else { /* Otherwise we treat this as a filename */ - verify_filename(prefix, argv[i], 1); + verify_filename(prefix, argv[0], 1); } } *rev_ret = rev; - return i < argc ? get_pathspec(prefix, argv + i) : NULL; + return *argv ? get_pathspec(prefix, argv) : NULL; } int cmd_reset(int argc, const char **argv, const char *prefix)