Martin von Zweigbergk <martinvonz@xxxxxxxxx> 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.
> 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)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html