Re: [PATCH 1/2 v3] revision.c: args starting with "-" might be a revision
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 10, 2017, 23:35 UTC
- Message-ID
- <xmqqh941ippo.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <1486752926-12020-2-git-send-email-kannan.siddharth12@gmail.com>
Siddharth Kannan <kannan.siddharth12@gmail.com> writes:
Show 18 quoted lines
> @@ -2234,11 +2235,18 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
> }
> if (opts < 0)
> exit(128);
> - continue;
> +
> + args = handle_revision_arg(arg, revs, flags, revarg_opt);
> + handle_rev_arg_called = 1;
> + if (args)
> + continue;
> + else
> + --left;
> }
>
>
> - if (handle_revision_arg(arg, revs, flags, revarg_opt)) {
> + if ((handle_rev_arg_called && args) ||
> + handle_revision_arg(arg, revs, flags, revarg_opt)) {Naively I would have expected that removing the "continue" at the end and letting the control go to the existing
if (handle_revision_arg(arg, revs, flags, revarg_opt)) {would be all that is needed. The latter half of the patch is an artifact of having ane xtra "handle_revision_arg() calls inside the "if it begins with dash" block to avoid calling it twice.
So the difference is just "--left" (by the way, our codebase seem to prefer "left--" when there is no difference between pre- or post- decrement/increment) that adjusts the slot in argv[] where the next unknown argument is stuffed to.
The adjustment is needed as the call to handle_revision_opt() that is before the pre-context of this hunk stuffed the unknown thing that begins with "-" into argv[left++]; if that thing turns out to be a valid rev, then you would need to take it back, because after all, that is not an unknown command line argument.
I am wondering if writing it like the following is easier to understand. I had a hard time figuring out what you are trying to do, partly because "args" is quite a misnomer---implying "how many arguments did we see" that is similar to opts that does mean "how many options did handle_revision_opts() see?" The variable means means "yes we saw a valid rev" when it is zero. The rewrite below may avoid such a confusion. I dunno.
revision.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/revision.c b/revision.c index b37dbec378..e238430948 100644 --- a/revision.c +++ b/revision.c @@ -2204,6 +2204,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s revarg_opt |= REVARG_CANNOT_BE_FILENAME; read_from_stdin = 0; for (left = i = 1; i < argc; i++) { + int maybe_rev = 0; const char *arg = argv[i]; if (*arg == '-') { int opts; @@ -2234,11 +2235,16 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s } if (opts < 0) exit(128); - continue; + maybe_rev = 1; + left--; /* tentatively cancel "unknown opt" */ } - - if (handle_revision_arg(arg, revs, flags, revarg_opt)) { + if (!handle_revision_arg(arg, revs, flags, revarg_opt)) { + got_rev_arg = 1; + } else if (maybe_rev) { + left++; /* it turns out that it was "unknown opt" */ + continue; + } else { int j; if (seen_dashdash || *arg == '^') die("bad revision '%s'", arg); @@ -2255,8 +2261,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s append_prune_data(&prune_data, argv + i); break; } - else - got_rev_arg = 1; } if (prune_data.nr) {