From: Junio C Hamano <gitster@pobox.com>
To: Siddharth Kannan <kannan.siddharth12@gmail.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: Fri, 10 Feb 2017 15:35:47 -0800 [thread overview]
Message-ID: <xmqqh941ippo.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <1486752926-12020-2-git-send-email-kannan.siddharth12@gmail.com> (Siddharth Kannan's message of "Fri, 10 Feb 2017 18:55:25 +0000")
Siddharth Kannan <kannan.siddharth12@gmail.com> writes:
> @@ -2234,11 +2235,18 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
> }
> if (opts < 0)
> exit(128);
> - continue;
> +
> + args = handle_revision_arg(arg, revs, flags, revarg_opt);
> + handle_rev_arg_called = 1;
> + if (args)
> + continue;
> + else
> + --left;
> }
>
>
> - if (handle_revision_arg(arg, revs, flags, revarg_opt)) {
> + if ((handle_rev_arg_called && args) ||
> + handle_revision_arg(arg, revs, flags, revarg_opt)) {
Naively I would have expected that removing the "continue" at the
end and letting the control go to the existing
if (handle_revision_arg(arg, revs, flags, revarg_opt)) {
would be all that is needed. The latter half of the patch is an
artifact of having ane xtra "handle_revision_arg() calls inside the
"if it begins with dash" block to avoid calling it twice.
So the difference is just "--left" (by the way, our codebase seem to
prefer "left--" when there is no difference between pre- or post-
decrement/increment) that adjusts the slot in argv[] where the next
unknown argument is stuffed to.
The adjustment is needed as the call to handle_revision_opt() that
is before the pre-context of this hunk stuffed the unknown thing
that begins with "-" into argv[left++]; if that thing turns out to
be a valid rev, then you would need to take it back, because after
all, that is not an unknown command line argument.
I am wondering if writing it like the following is easier to
understand. I had a hard time figuring out what you are trying to
do, partly because "args" is quite a misnomer---implying "how many
arguments did we see" that is similar to opts that does mean "how
many options did handle_revision_opts() see?" The variable means
means "yes we saw a valid rev" when it is zero. The rewrite
below may avoid such a confusion. I dunno.
revision.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/revision.c b/revision.c
index b37dbec378..e238430948 100644
--- a/revision.c
+++ b/revision.c
@@ -2204,6 +2204,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
revarg_opt |= REVARG_CANNOT_BE_FILENAME;
read_from_stdin = 0;
for (left = i = 1; i < argc; i++) {
+ int maybe_rev = 0;
const char *arg = argv[i];
if (*arg == '-') {
int opts;
@@ -2234,11 +2235,16 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
}
if (opts < 0)
exit(128);
- continue;
+ maybe_rev = 1;
+ left--; /* tentatively cancel "unknown opt" */
}
-
- if (handle_revision_arg(arg, revs, flags, revarg_opt)) {
+ if (!handle_revision_arg(arg, revs, flags, revarg_opt)) {
+ got_rev_arg = 1;
+ } else if (maybe_rev) {
+ left++; /* it turns out that it was "unknown opt" */
+ continue;
+ } else {
int j;
if (seen_dashdash || *arg == '^')
die("bad revision '%s'", arg);
@@ -2255,8 +2261,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) {
next prev parent reply other threads:[~2017-02-10 23:37 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 ` Junio C Hamano [this message]
2017-02-11 7:52 ` [PATCH 1/2 v3] revision.c: args starting with "-" might be a revision 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 ` [PATCH 1/2 v3] revision.c: args starting with "-" might be a revision Siddharth Kannan
2017-02-12 18:56 ` 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=xmqqh941ippo.fsf@gitster.mtv.corp.google.com \
--to=gitster@pobox.com \
--cc=Matthieu.Moy@imag.fr \
--cc=git@vger.kernel.org \
--cc=kannan.siddharth12@gmail.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).