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

[Junio C Hamano <gitster@pobox.com>]

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.