Siddharth Kannan <kannan.siddhart...@gmail.com> writes: > On Fri, Feb 10, 2017 at 03:35:47PM -0800, Junio C Hamano wrote: > >> 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"?
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. > 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 function follows the convention to return 0 for success, -1 for error/unexpected, by the way. > Um, I am sorry, but I feel that decrementing left, and incrementing it again > is > also confusing. 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.
