git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Siddharth Kannan <kannan.siddharth12@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, Matthieu.Moy@imag.fr,
	pranit.bauva@gmail.com, peff@peff.net, pclouds@gmail.com,
	sandals@crustytoothpaste.ath.cx
Subject: Re: [PATCH 1/2 v3] revision.c: args starting with "-" might be a revision
Date: Sun, 12 Feb 2017 12:36:30 +0000	[thread overview]
Message-ID: <20170212123630.GA20872@ubuntu-512mb-blr1-01.localdomain> (raw)
In-Reply-To: <xmqqefz4h1vq.fsf@gitster.mtv.corp.google.com>

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

  parent reply	other threads:[~2017-02-12 12:36 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-02-10 18:55 [PATCH 0/2 v3] WIP: allow "-" as a shorthand for "previous branch" Siddharth Kannan
2017-02-10 18:55 ` [PATCH 1/2 v3] revision.c: args starting with "-" might be a revision Siddharth Kannan
2017-02-10 18:55   ` [PATCH 2/2 v3] sha1_name: teach get_sha1_1 "-" shorthand for "@{-1}" Siddharth Kannan
2017-02-12  9:48     ` Matthieu Moy
2017-02-12 10:42       ` Siddharth Kannan
2017-02-13 19:51       ` Junio C Hamano
2017-02-13 20:03         ` Junio C Hamano
2017-02-10 23:35   ` [PATCH 1/2 v3] revision.c: args starting with "-" might be a revision Junio C Hamano
2017-02-11  7:52     ` Siddharth Kannan
2017-02-11 21:08       ` Junio C Hamano
2017-02-11 23:40         ` Junio C Hamano
2017-02-12 18:41           ` [PATCH 0/3] prepare for a rev/range that begins with a dash Junio C Hamano
2017-02-12 18:41             ` [PATCH 1/3] handle_revision_opt(): do not update argv[left++] with an unknown arg Junio C Hamano
2017-02-12 18:41             ` [PATCH 2/3] setup_revisions(): swap if/else bodies to make the next step more readable Junio C Hamano
2017-02-12 18:41             ` [PATCH 3/3] setup_revisions(): allow a rev that begins with a dash Junio C Hamano
2017-02-12 12:36         ` Siddharth Kannan [this message]
2017-02-12 18:56           ` [PATCH 1/2 v3] revision.c: args starting with "-" might be a revision Junio C Hamano
2017-02-14  4:23             ` Siddharth Kannan

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170212123630.GA20872@ubuntu-512mb-blr1-01.localdomain \
    --to=kannan.siddharth12@gmail.com \
    --cc=Matthieu.Moy@imag.fr \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=pclouds@gmail.com \
    --cc=peff@peff.net \
    --cc=pranit.bauva@gmail.com \
    --cc=sandals@crustytoothpaste.ath.cx \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).