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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 17, 2015, 22:16 UTC
Message-ID: <xmqqpp87mfqx.fsf@gitster.dls.corp.google.com>
In-Reply-To: <87egons4du.fsf@gmail.com>

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

Show 21 quoted lines

> 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.