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.
next prev 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).