[PATCH/RFC 0/2][GSoC] revision.c: Allow "-" as stand-in for "@{-1}" everywhere a branch is allowed

[PATCH/RFC 0/2][GSoC] revision.c: Allow "-" as stand-in for "@{-1}" everywhere a branch is allowed

[Kenny Lee Sin Cheong]

This is an attempt at a microproject for GSoC

An attempt to add revision range support to Junio's JFF patch sent a few days ago. The first patch is the a copy of the one he posted.

I was wondering if it was a good idea to add support for commands like "<rev>..-". Files that starts with "-" requires "--" or a "./" format but what if we have a file named "next..-" and call "git log next..-" ?

Junio C Hamano (1):
  "-" and "@{-1}" on various programs

Kenny Lee Sin Cheong (1):
  Add revision range support on "-" and "@{-1}"

 builtin/checkout.c |  3 ---
 builtin/merge.c    |  3 +--
 builtin/revert.c   |  2 --
 revision.c         | 16 +++++++++++++--
 sha1_name.c        | 57 +++++++++++++++++++++++++++++++++---------------------
 5 files changed, 50 insertions(+), 31 deletions(-)

-- 
2.3.2.225.gebdc58a

[PATCH 1/2] "-" and "@{-1}" on various programs

[Kenny Lee Sin Cheong]

From: Junio C Hamano <gitster@pobox.com>

JFF stands for just for fun.

This is not meant to give out a model answer and is known to be
incomplete, but I was wondering if it would be a better direction to
allow "-" as a stand-in for "@{-1}" everywhere we allow a branch
name, losing workarounds at the surface level we have for checkout,
merge and revert.

The first three paths are to remove the surface workarounds that
become unnecessary.  The one in sha1_name.c is the central change.

The change in revision.c is to allow a single "-" to be recognized
as a potential revision name (without this change, what begins with
"-" is either an option or an unknown option).

So you could do things like "git reset - $path" but also things like
"git log -" after switching out of a branch.

What does not work are what needs further tweaking in revision.c
parser.  "git checkout master && git checkout next && git log -.."
should show what next has on top of master but I didn't touch the
range notation so it does not work, for example.

 builtin/checkout.c |  3 ---
 builtin/merge.c    |  3 +--
 builtin/revert.c   |  2 --
 revision.c         |  2 +-
 sha1_name.c        | 57 +++++++++++++++++++++++++++++++++---------------------
 5 files changed, 37 insertions(+), 30 deletions(-)

Signed-off-by: Kenny Lee Sin Cheong <kenny.lee28@gmail.com>
---
 builtin/checkout.c |  3 ---
 builtin/merge.c    |  3 +--
 builtin/revert.c   |  2 --
 revision.c         |  2 +-
 sha1_name.c        | 57 +++++++++++++++++++++++++++++++++---------------------
 5 files changed, 37 insertions(+), 30 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 3e141fc..f86bad7 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -951,9 +951,6 @@ static int parse_branchname_arg(int argc, const char **argv,
 	else if (dash_dash_pos >= 2)
 		die(_("only one reference expected, %d given."), dash_dash_pos);
 
-	if (!strcmp(arg, "-"))
-		arg = "@{-1}";
-
 	if (get_sha1_mb(arg, rev)) {
 		/*
 		 * Either case (3) or (4), with <something> not being
diff --git a/builtin/merge.c b/builtin/merge.c
index 3b0f8f9..03b260f 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1164,8 +1164,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 				argc = setup_with_upstream(&argv);
 			else
 				die(_("No commit specified and merge.defaultToUpstream not set."));
-		} else if (argc == 1 && !strcmp(argv[0], "-"))
-			argv[0] = "@{-1}";
+		}
 	}
 	if (!argc)
 		usage_with_options(builtin_merge_usage,
diff --git a/builtin/revert.c b/builtin/revert.c
index 56a2c36..dc98b4e 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -170,8 +170,6 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 		opts->revs->no_walk = REVISION_WALK_NO_WALK_UNSORTED;
 		if (argc < 2)
 			usage_with_options(usage_str, options);
-		if (!strcmp(argv[1], "-"))
-			argv[1] = "@{-1}";
 		memset(&s_r_opt, 0, sizeof(s_r_opt));
 		s_r_opt.assume_dashdash = 1;
 		argc = setup_revisions(argc, argv, opts->revs, &s_r_opt);
diff --git a/revision.c b/revision.c
index 66520c6..7778bbd 100644
--- a/revision.c
+++ b/revision.c
@@ -2198,7 +2198,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 	read_from_stdin = 0;
 	for (left = i = 1; i < argc; i++) {
 		const char *arg = argv[i];
-		if (*arg == '-') {
+		if (arg[0] == '-' && arg[1]) {
 			int opts;
 
 			opts = handle_revision_pseudo_opt(submodule,
diff --git a/sha1_name.c b/sha1_name.c
index 95f9f8f..7a621ba 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -483,6 +483,8 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1,
 				break;
 			}
 		}
+	} else if (len == 1 && str[0] == '-') {
+		nth_prior = 1;
 	}
 
 	/* Accept only unambiguous ref paths. */
@@ -491,13 +493,16 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1,
 
 	if (nth_prior) {
 		struct strbuf buf = STRBUF_INIT;
-		int detached;
+		int status;
 
 		if (interpret_nth_prior_checkout(str, len, &buf) > 0) {
-			detached = (buf.len == 40 && !get_sha1_hex(buf.buf, sha1));
+			if (get_sha1(buf.buf, sha1))
+				/* bad---the previous branch no longer exists? */
+				status = -1;
+			else
+				status = 0; /* detached */
 			strbuf_release(&buf);
-			if (detached)
-				return 0;
+			return status;
 		}
 	}
 
@@ -931,35 +936,43 @@ static int interpret_nth_prior_checkout(const char *name, int namelen,
 					struct strbuf *buf)
 {
 	long nth;
-	int retval;
+	int consumed;
 	struct grab_nth_branch_switch_cbdata cb;
-	const char *brace;
-	char *num_end;
 
-	if (namelen < 4)
-		return -1;
-	if (name[0] != '@' || name[1] != '{' || name[2] != '-')
-		return -1;
-	brace = memchr(name, '}', namelen);
-	if (!brace)
-		return -1;
-	nth = strtol(name + 3, &num_end, 10);
-	if (num_end != brace)
-		return -1;
-	if (nth <= 0)
-		return -1;
+	if (namelen == 1 && name[0] == '-') {
+		nth = 1;
+		consumed = 1;
+	} else {
+		const char *brace;
+		char *num_end;
+
+		if (namelen < 4)
+			return -1;
+		if (name[0] != '@' || name[1] != '{' || name[2] != '-')
+			return -1;
+		brace = memchr(name, '}', namelen);
+		if (!brace)
+			return -1;
+		nth = strtol(name + 3, &num_end, 10);
+		if (num_end != brace)
+			return -1;
+		if (nth <= 0)
+			return -1;
+		consumed = brace - name + 1;
+	}
+
 	cb.remaining = nth;
 	strbuf_init(&cb.buf, 20);
 
-	retval = 0;
 	if (0 < for_each_reflog_ent_reverse("HEAD", grab_nth_branch_switch, &cb)) {
 		strbuf_reset(buf);
 		strbuf_addbuf(buf, &cb.buf);
-		retval = brace - name + 1;
+	} else {
+		consumed = 0;
 	}
 
 	strbuf_release(&cb.buf);
-	return retval;
+	return consumed;
 }
 
 int get_sha1_mb(const char *name, unsigned char *sha1)
-- 
2.3.2.225.gebdc58a

[PATCH 2/2] Add revision range support on "-" and "@{-1}"

[Kenny Lee Sin Cheong]

Currently it is not be possible to do something like "git checkout
master && git checkout next && git log -.." to see what master has on
top of master.

Allows use of the revision range such as <rev>..- or -..<rev> to see
what HEAD has on top of <rev> or vice versa, respectively.

Also allows use of symmetric differences such as <rev>...- and -...<rev>

This is written on top of Junio's "Just For Fun" patch ($Gmane/265260).

Signed-off-by: Kenny Lee Sin Cheong <kenny.lee28@gmail.com>
---
 revision.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/revision.c b/revision.c
index 7778bbd..a79b443 100644
--- a/revision.c
+++ b/revision.c
@@ -1490,6 +1490,7 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi
 		int symmetric = *next == '.';
 		unsigned int flags_exclude = flags ^ (UNINTERESTING | BOTTOM);
 		static const char head_by_default[] = "HEAD";
+		static const char prev_rev[] = "@{-1}";
 		unsigned int a_flags;
 
 		*dotdot = 0;
@@ -1499,6 +1500,13 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi
 			next = head_by_default;
 		if (dotdot == arg)
 			this = head_by_default;
+		/*  Allows -..<rev> and <rev>..- */
+		if (!strcmp(this, "-")) {
+			this = prev_rev;
+		}
+		if (!strcmp(next, "-")) {
+			next = prev_rev;
+		}
 		if (this == head_by_default && next == head_by_default &&
 		    !symmetric) {
 			/*
@@ -2198,7 +2206,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 	read_from_stdin = 0;
 	for (left = i = 1; i < argc; i++) {
 		const char *arg = argv[i];
-		if (arg[0] == '-' && arg[1]) {
+		if (arg[0] == '-' && !strstr(arg, "..")) {
 			int opts;
 
 			opts = handle_revision_pseudo_opt(submodule,
@@ -2220,6 +2228,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 				continue;
 			}
 
+
 			opts = handle_revision_opt(revs, argc - i, argv + i, &left, argv);
 			if (opts > 0) {
 				i += opts - 1;
@@ -2229,7 +2238,10 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 				exit(128);
 			continue;
 		}
-
+		if (strstr(arg, "..")) {
+			handle_revision_arg(arg, revs, flags, revarg_opt);
+			continue;
+		}
 
 		if (handle_revision_arg(arg, revs, flags, revarg_opt)) {
 			int j;
-- 
2.3.2.225.gebdc58a

Re: [PATCH 2/2] Add revision range support on "-" and "@{-1}"

[Junio C Hamano]

Kenny Lee Sin Cheong <kenny.lee28@gmail.com> writes:

> diff --git a/revision.c b/revision.c
> index 7778bbd..a79b443 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -1490,6 +1490,7 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi
>  		int symmetric = *next == '.';
>  		unsigned int flags_exclude = flags ^ (UNINTERESTING | BOTTOM);
>  		static const char head_by_default[] = "HEAD";
> +		static const char prev_rev[] = "@{-1}";
>  		unsigned int a_flags;
>  
>  		*dotdot = 0;
> @@ -1499,6 +1500,13 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi
>  			next = head_by_default;
>  		if (dotdot == arg)
>  			this = head_by_default;
> +		/*  Allows -..<rev> and <rev>..- */
> +		if (!strcmp(this, "-")) {
> +			this = prev_rev;
> +		}
> +		if (!strcmp(next, "-")) {
> +			next = prev_rev;
> +		}

The above two hunks are disappointing.  "this" and "next" are passed
to get_sha1_committish() and the point of the [1/2] patch was to
allow "-" to be just as usable as "@{-1}" anywhere a branch name can
be used.

>  		if (this == head_by_default && next == head_by_default &&
>  		    !symmetric) {
>  			/*
> @@ -2198,7 +2206,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
>  	read_from_stdin = 0;
>  	for (left = i = 1; i < argc; i++) {
>  		const char *arg = argv[i];
> -		if (arg[0] == '-' && arg[1]) {
> +		if (arg[0] == '-' && !strstr(arg, "..")) {

Isn't this way too loose?  "--some-opt=I.wish..." would have ".."
in it, and we would want to leave room to add new options that may
take arbitrary string as an argument.

I would have expected it would be more like

		if (arg[0] == '-' && arg[1] && !starts_with(arg + 1, "..")) {

That is, "anything that begins with '-', if it is to be taken as an
option, must not begin with '-..'", which I think should be strict
enough.

> @@ -2220,6 +2228,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
>  				continue;
>  			}
>  
> +
>  			opts = handle_revision_opt(revs, argc - i, argv + i, &left, argv);
>  			if (opts > 0) {
>  				i += opts - 1;

Noise.

> @@ -2229,7 +2238,10 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
>  				exit(128);
>  			continue;
>  		}
> -
> +		if (strstr(arg, "..")) {
> +			handle_revision_arg(arg, revs, flags, revarg_opt);
> +			continue;
> +		}

What is this for?  We will call handle_revision_arg() whether arg
has ".." or not immediately after this one.

>  		if (handle_revision_arg(arg, revs, flags, revarg_opt)) {
>  			int j;

Re: [PATCH 2/2] Add revision range support on "-" and "@{-1}"

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>> @@ -1499,6 +1500,13 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi
>>  			next = head_by_default;
>>  		if (dotdot == arg)
>>  			this = head_by_default;
>> +		/*  Allows -..<rev> and <rev>..- */
>> +		if (!strcmp(this, "-")) {
>> +			this = prev_rev;
>> +		}
>> +		if (!strcmp(next, "-")) {
>> +			next = prev_rev;
>> +		}
>
> The above two hunks are disappointing.  "this" and "next" are passed
> to get_sha1_committish() and the point of the [1/2] patch was to
> allow "-" to be just as usable as "@{-1}" anywhere a branch name can
> be used.
>
>>  		if (this == head_by_default && next == head_by_default &&
>>  		    !symmetric) {
>>  			/*
>> @@ -2198,7 +2206,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
>>  	read_from_stdin = 0;
>>  	for (left = i = 1; i < argc; i++) {
>>  		const char *arg = argv[i];
>> -		if (arg[0] == '-' && arg[1]) {
>> +		if (arg[0] == '-' && !strstr(arg, "..")) {
>
> Isn't this way too loose?  "--some-opt=I.wish..." would have ".."
> in it, and we would want to leave room to add new options that may
> take arbitrary string as an argument.
>
> I would have expected it would be more like
>
> 		if (arg[0] == '-' && arg[1] && !starts_with(arg + 1, "..")) {
>
> That is, "anything that begins with '-', if it is to be taken as an
> option, must not begin with '-..'", which I think should be strict
> enough.

I have an updated version to handle the simplest forms of range
notations on 'pu' as d40f108d ("-" and "@{-1}" on various programs,
2015-03-16).  I do not think either your !strstr(arg, "..") or my
!starts_with(arg + 1, "..")  is correct, if we really wanted to make
"-" a true stand-in for @{-1}, as we would need to stop ourselves
fall into this "This begins with a dash, so it has to be a dashed
option" block when things like these are given:

    "-^"
    "-~10"
    "-^{/^### match next}"

I have a suspicion that it might be a matter of swapping the if
clauses, that is, instead of the current way

	if (starts with '-') {
        	do the option thing;
                continue;
	}
	if (try to see if it is a revision or revision range) {
        	/* if failed, args must be pathspecs from here on */
		check the '--' disambiguation;
                add pathspec to prune-data;
	} else {
		got_rev_arg = 1;
	}

which tries "the option thing" first, do something like this:

	if (try to see if it is a revision or regvision range) {
        	/* if failed ... */
		if (starts with '-') {
                	do the option thing;
                        continue;
		}
		/* args must be pathspecs from here on */
                check the  '--' disambiguation;
                add pathspec to prune-data;
	} else {
		got_rev_arg = 1;
	}

but I didn't trace the logic myself to see if that would work.

Re: [PATCH 2/2] Add revision range support on "-" and "@{-1}"

[Kenny Lee Sin Cheong]

On Tue, Mar 17 2015 at 02:49:48 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> 	if (try to see if it is a revision or regvision range) {
>         	/* if failed ... */
> 		if (starts with '-') {
>                 	do the option thing;
>                         continue;
> 		}
> 		/* args must be pathspecs from here on */
>                 check the  '--' disambiguation;
>                 add pathspec to prune-data;
> 	} else {
> 		got_rev_arg = 1;
> 	}
>
> but I didn't trace the logic myself to see if that would work.

You're right. I was actually going to try and check all possible
suffixes of "-" but your solution saves us from doing that, and it
didn't break any tests.

On a similar note, would it be relevant to add similar changes to
rev-parse? While trying to write some test, I noticed that rev-parse
doesn't support "-". If I'm not mistaking it assumes everything that starts with "-"
must be an option. But since it is a plumbing tool I don't know if it
would be worth it or even an improvement.

Re: [PATCH 2/2] Add revision range support on "-" and "@{-1}"

[Junio C Hamano]

Kenny Lee Sin Cheong <kenny.lee28@gmail.com> writes:

> On Tue, Mar 17 2015 at 02:49:48 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>> 	if (try to see if it is a revision or regvision range) {
>>         	/* if failed ... */
>> 		if (starts with '-') {
>>                 	do the option thing;
>>                         continue;
>> 		}
>> 		/* args must be pathspecs from here on */
>>                 check the  '--' disambiguation;
>>                 add pathspec to prune-data;
>> 	} else {
>> 		got_rev_arg = 1;
>> 	}
>>
>> but I didn't trace the logic myself to see if that would work.
>
> You're right. I was actually going to try and check all possible
> suffixes of "-" but your solution saves us from doing that, and it
> didn't break any tests.

"It didn't break any tests" does not tell us much, though.

I also notice that handle_revision_arg() would die() by calling it
directly or indirectly via verify_non_filename(), etc., but the
caller actually is expecting it to silently return non-zero when it
finds an argument that cannot be interpreted as a revision or as a
revision range.  

If we feed the function a string that has ".." in it, with
cant_be_filename unset, and if that string _can_ be parsed as a
valid range (e.g. "master..next"), we would check if a file whose
name is that string and die, e.g.

    $ >master..next ; git log master..next
    fatal: ambigous argument 'master..next': both revision and filename

If we swap the order to do the "revision" first before "option",
however, we would end up getting the same for a name that begins
with "-" and has ".." in it.  I see no guarantee that future
possible option name cannot be misinterpreted as a range to trigger
this check.

But "git cmd -$option" for any value of $option does not have to be
disambiguated when there is a file whose name is "-$option".  The
existing die()'s in the handle_revision_arg() function _will_ break
that promise.  Currently, because we check the options first,
handle_revision_arg() does not cause us any problem, but swapping
the order will have fallouts.

If we want to really do the swapping (and I think that is the only
sensible way if we wanted to allow "-" and any extended SHA-1 that
begins with "-" as "the previous branch"), I think the "OK, it looks
like a revision (or revision range); as we didn't see dashdash, it
must not be a filename" check has to be moved to the caller, perhaps
like this:

	if (try to see if it is a revision or a revision range) {
        	/* failed */
                ...
	} else {
        	/* it can be read as a revision or a revision range */
                if (!seen_dashdash)
			verify_non_filename(arg);
		got_rev_arg = 1;
	}

The "missing" cases should also silently return failure and have the
caller deal with that.

> On a similar note, would it be relevant to add similar changes to
> rev-parse?

If the goal is "to allow '-' everywhere '@{-1}' is allowed, and used
as such", then yes, of course, such an update is needed.

But I am not sure if that is a worthwhile goal to aim for in the
first place, though.  You would need to accept -@{two.days.ago} as a
"short-hand" for @{-1}@{two.days.ago}, etc., which does not look
very readable way in the first place.

Re: [PATCH 2/2] Add revision range support on "-" and "@{-1}"

[Kenny Lee Sin Cheong]

On Tue, Mar 17 2015 at 06:16:38 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> I also notice that handle_revision_arg() would die() by calling it
> directly or indirectly via verify_non_filename(), etc., but the
> caller actually is expecting it to silently return non-zero when it
> finds an argument that cannot be interpreted as a revision or as a
> revision range.  
>
> If we feed the function a string that has ".." in it, with
> cant_be_filename unset, and if that string _can_ be parsed as a
> valid range (e.g. "master..next"), we would check if a file whose
> name is that string and die, e.g.
>
>     $ >master..next ; git log master..next
>     fatal: ambigous argument 'master..next': both revision and filename
>
> If we swap the order to do the "revision" first before "option",
> however, we would end up getting the same for a name that begins
> with "-" and has ".." in it.  I see no guarantee that future
> possible option name cannot be misinterpreted as a range to trigger
> this check.
>
If I'm understanding correctly, the problem of checking revisions before
arg is that an option fed to handle_revision_arg() might die() before getting
checked as an option in cases where a file with the same name exists?

But doesn't verify_non_filename() already return silently if arg begins
with "-"? It die() only after making that check.

If an option with ".." in it such as -$opt..ion is really given to
handle_revision_arg() then verify_non_filename should not be a problem.

> But "git cmd -$option" for any value of $option does not have to be
> disambiguated when there is a file whose name is "-$option".  The
> existing die()'s in the handle_revision_arg() function _will_ break
> that promise.  Currently, because we check the options first,
> handle_revision_arg() does not cause us any problem, but swapping
> the order will have fallouts.
>

The only other way handle_revision_arg() can die() is if given a ".."
range, either revisions return null when passed their sha1 to
parse_object().

So something like you proposed earlier:

      if(try to see if it is a revision or a revision range) {
              /* if failed ... */
              if (starts with '-') {
                      do the option thing;
                      continue;
              }
              /*
               * args must be pathspecs from here on.
               * We already checked that rev arg cannot be
               * interpreted as a filename at this point
               */
              if(dashdash)
                      verify_filename()
                     
      } else {
              got_rev_arg = 1;
      }

should work. I'm still getting familiar to how it works so I might be missing
something but shouldn't this be fine? At least concerning the possible fallouts
that you've raised.

> If we want to really do the swapping (and I think that is the only
> sensible way if we wanted to allow "-" and any extended SHA-1 that
> begins with "-" as "the previous branch"), I think the "OK, it looks
> like a revision (or revision range); as we didn't see dashdash, it
> must not be a filename" check has to be moved to the caller, perhaps
> like this:
>
> 	if (try to see if it is a revision or a revision range) {
>         	/* failed */
>                 ...
> 	} else {
>         	/* it can be read as a revision or a revision range */
>                 if (!seen_dashdash)
> 			verify_non_filename(arg);
> 		got_rev_arg = 1;
> 	}
>
If what I'm saying makes sense, then verify_non_filename(arg) would be
already working as intended in handle_revision_arg(), so moving it to
the caller wouldn't be necessary.

> The "missing" cases should also silently return failure and have the
> caller deal with that.

Re: [PATCH 2/2] Add revision range support on "-" and "@{-1}"

[Junio C Hamano]

Kenny Lee Sin Cheong <kenny.lee28@gmail.com> writes:

> If I'm understanding correctly, the problem of checking revisions before
> arg is that an option fed to handle_revision_arg() might die() before getting
> checked as an option in cases where a file with the same name exists?
>
> But doesn't verify_non_filename() already return silently if arg begins
> with "-"? It die() only after making that check.
>
> If an option with ".." in it such as -$opt..ion is really given to
> handle_revision_arg() then verify_non_filename should not be a problem.

Yes, but should we be relying on that behaviour?  The special casing
to assume that no sane person would name a file starting with a dash
is what I find somewhat disturbing.