Re: [PATCH 1/2 v3] revision.c: args starting with "-" might be a revision

[Junio C Hamano <gitster@pobox.com>]

Siddharth Kannan <kannan.siddharth12@gmail.com> writes:

> @@ -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) {