On Sat, Feb 11, 2017 at 01:08:09PM -0800, Junio C Hamano wrote:
> Siddharth Kannan <kannan.siddharth12@xxxxxxxxx> 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.