Re: [PATCH 06/19] reset.c: remove unnecessary variable 'i'
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 9, 2013, 19:39 UTC
- Message-ID
- <7vpq1e5ent.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <1357719376-16406-7-git-send-email-martinvonz@gmail.com>
Martin von Zweigbergk <martinvonz@gmail.com> writes:
Show 6 quoted lines
> 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.
Show 64 quoted lines
> 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)