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

[Siddharth Kannan <kannan.siddharth12@gmail.com>]

Hey Junio,
On Fri, Feb 10, 2017 at 03:35:47PM -0800, Junio C Hamano wrote:
> 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.

Understood, I will use post decrement.

> 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?"  

Um, okay, I see that "args" is very confusing. Would it help if this variable
was called "arg_not_rev"? Because the value that is returned from
handle_revision_arg is 1 when it is not a revision, and 0 when it is a
revision. The changed block of code would look like this:

---
 revision.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/revision.c b/revision.c
index b37dbec..4131ad5 100644
--- a/revision.c
+++ b/revision.c
@@ -2205,6 +2205,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 	read_from_stdin = 0;
 	for (left = i = 1; i < argc; i++) {
 		const char *arg = argv[i];
+		int handle_rev_arg_called = 0, arg_not_rev;
 		if (*arg == '-') {
 			int opts;
@@ -2234,11 +2235,18 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
                    }
                    if (opts < 0)
                            exit(128);
-                   continue;
+
+                   arg_not_rev = handle_revision_arg(arg, revs, flags, revarg_opt);
+                   handle_rev_arg_called = 1;
+                   if (arg_not_rev)
+                           continue; /* arg is neither an option nor a revision */
+                   else
+                           left--; /* arg is a revision! */
            }
 
 
-           if (handle_revision_arg(arg, revs, flags, revarg_opt)) {
+           if ((handle_rev_arg_called && arg_not_rev) ||
+                           handle_revision_arg(arg, revs, flags, revarg_opt)) {

> The rewrite below may avoid such a confusion.  I dunno.

Um, I am sorry, but I feel that decrementing left, and incrementing it again is
also confusing. I think that with a better name for the return value from
handle_revision_arg, the earlier confusion should be resolved.

I base this on my previous experience following the codepath. It was easy for
me to understand with the previous code that "continue" will be executed from
within the first if block whenever arg begins with a "-" and it is determined
that it is not an option. 

going by that, now, "continue" will be executed whenever it's not an option and
_also_ not an argument. Otherwise, the further parts of the code will execute
as before, and there are no continue statements there. I hope this argument
makes sense.

Also worth noting, The two `if` lines look better now:

1. If arg is not a revision, go to the next arg (because we have already
determined that it is not an option)

2. If handle_rev_arg was called AND the argument was not a revision, OR
if handle_revision_arg says that arg is not a rev, execute the following block.

Perhaps, someone else could please have a look at the changes in the block
above and the block below and give some feedback on which one is easier to
understand and the reason that they feel so. Thanks a lot!

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

Thanks Junio, for the time you spent analysing and writing the above version of
the patch!

Regards,

- Siddharth Kannan