Kenny Lee Sin Cheong <kenny.lee28@xxxxxxxxx> writes:
> On Tue, Mar 17 2015 at 02:49:48 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>> Junio C Hamano <gitster@xxxxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html