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

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

Not really. If we absolutely need to have one variable that is meant to escape the "if it begins with a dash" block and to affect what comes next, I think the variable should mean "we know we saw a revision and you do not have to call it again". IOW the code that needs to do "handle_rev_arg_called && arg_not_rev" is just being silly. At that point in the codeflow, I do not see why the code needs to take two bits of information and combine them; the one that sets these two variables should have done the work for it.

And that would make the if statement slightly easier to read compared to the original. I am however not suggesting to do that; read on.

The function follows the convention to return 0 for success, -1 for error/unexpected, by the way.

Yes, but it is no more confusing than your original "left--".

If we want to make the flow of logic easier to follow, we need to step back and view what the codepath _wants_ to do at the higher level, which is:

 * If it is an option known to us, handle it and go to the next arg.

 * If it is an option that we do not understand, stuff it in
   argv[left++] and go to the next arg.

 * If it is a rev, handle it, and note that fact in got_rev_arg.

 * If it is not a rev and we haven't seen dashdash, verify that it
   and everything that follows it are pathnames (which is an inexact
   but a cheap way to avoid ambiguity), make all them the prune_data
   and conclude.

Because the second step currently is implemented by calling handle_opt(), which not just tells if it is an option we understand or not, but also mucks with argv[left++], you need to undo it once you start making it possible for a valid "rev" to begin with a dash. That is what your left-- was, and that is what "decrement and then increment when it turns out it was an unknown option after all" is.

The first step to a saner flow _could_ be to stop passing the unkv and unkc to handle_revision_opt() and instead make the caller responsible for doing that. That would express what your patch wanted to do in the most natural way, i.e.

 * If it is an option known to us, handle it and go to the next arg.

 * If it is a rev, handle it, and note that fact in got_rev_arg
   (this change of order enables you to allow a rev that begins with
   a dash, which would have been misrecognised as a possible unknown
   option).

 * If it looks like an option (i.e. "begins with a dash"), then we
   already know that it is not something we understand, because the
   first step would have caught it already.  Stuff it in
   argv[left++] and go to the next arg.

 * If it is not a rev and we haven't seen dashdash, verify that it
   and everything that follows it are pathnames (which is an inexact
   but a cheap way to avoid ambiguity), make all them the prune_data
   and conclude.

Such a change to handle_revision_opt() unfortunately affects other callers of the function, so it may not be worth it, but I think "decrement and then increment, because this codepath wants to check to see something that may ordinarily be clasified as an unknown option if it is a rev" is an ugly workaround, just like your left-- was. But I think the resulting code flow is much closer to the above ideal.