On Sat, Feb 11, 2017 at 01:08:09PM -0800, Junio C Hamano wrote: > Siddharth Kannan <kannan.siddhart...@gmail.com> writes: > > > 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 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. > > 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.
So, our problem here is that the function handle_revision_opt is opaquely also incrementing "left", which we need to decrement somehow. Or: we could change the flow of the code so that this incrementing will happen only when we have decided that the argument is not a revision. > > * 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. This "changing the order" gave me the idea to change the flow. I tried to implement the above steps without touching the function handle_revision_opt. By inserting the handle_revision_arg call just before calling handle_revision_opt. The decrementing then incrementing or "left--" things have now been removed. (But there is still one thing which doesn't look good) diff --git a/revision.c b/revision.c index b37dbec..8c0acea 100644 --- a/revision.c +++ b/revision.c @@ -2203,11 +2203,11 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s if (seen_dashdash) revarg_opt |= REVARG_CANNOT_BE_FILENAME; read_from_stdin = 0; + for (left = i = 1; i < argc; i++) { const char *arg = argv[i]; + int opts; if (*arg == '-') { - int opts; - opts = handle_revision_pseudo_opt(submodule, revs, argc - i, argv + i, &flags); @@ -2226,7 +2226,11 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s read_revisions_from_stdin(revs, &prune_data); continue; } + } + if (!handle_revision_arg(arg, revs, flags, revarg_opt)) + got_rev_arg = 1; + else if (*arg == '-') { opts = handle_revision_opt(revs, argc - i, argv + i, &left, argv); if (opts > 0) { i += opts - 1; @@ -2234,11 +2238,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s } if (opts < 0) exit(128); - continue; - } - - - if (handle_revision_arg(arg, revs, flags, revarg_opt)) { + } else { int j; if (seen_dashdash || *arg == '^') die("bad revision '%s'", arg); @@ -2255,8 +2255,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) { The "if (*arg =='-')" line is repeated. On analysing the resulting revision.c:setup_revisions function, I feel that the codepath is still as easily followable as it was earlier, and there is definitely no confusion because of a mysterious decrement. Also, the repeated condition doesn't make it any harder (it looks like a useful check because we already know that every option would start with a "-"). But that's only my opinion, and you definitely know better. now the flow is very close to the ideal flow that we prefer: 1. If it is a pseudo_opt or --stdin, handle and go to the next arg 2. If it is a revision, note that in "got_rev_arg", and go to the next arg 3. If it starts with a "-" and is a known option, handle and go to the next arg 4. If it is none of {revision, known-option} 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. > But I think the resulting code flow is much closer to the > above ideal. (about Junio's version of the patch): Yes, I agree with you on this. It's like the ideal, but the argv has already been populated, so the only remaining step is "left++". > > Such a change to handle_revision_opt() unfortunately affects other > callers of the function, so it may not be worth it. handle_revision_opt is called once apart from within setup_revisions, from within revision.c:parse_revision_opt. If this version is not acceptable, we should either revert back to your version of the patch with the fixed variable name OR consider re-writing handle_revision_opt, as per your suggested flow. Note that this will put the code for "Stuff it in argv[left++]" in every caller. Thank you for the time you have spent on analysing each version of the patch! -- Best Regards, Siddharth Kannan.