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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]