From: Junio C Hamano <gitster@pobox•com>
To: Kenny Lee Sin Cheong <kenny.lee28@gmail•com>
Cc: git@vger•kernel.org
Subject: Re: [PATCH 2/2] Add revision range support on "-" and "@{-1}"
Date: Tue, 17 Mar 2015 15:16:38 -0700 [thread overview]
Message-ID: <xmqqpp87mfqx.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <87egons4du.fsf@gmail.com> (Kenny Lee Sin Cheong's message of "Tue, 17 Mar 2015 17:25:33 -0400")
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.
next prev parent reply other threads:[~2015-03-17 22:16 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-03-16 15:11 [PATCH/RFC 0/2][GSoC] revision.c: Allow "-" as stand-in for "@{-1}" everywhere a branch is allowed Kenny Lee Sin Cheong
2015-03-16 15:11 ` [PATCH 1/2] "-" and "@{-1}" on various programs Kenny Lee Sin Cheong
2015-03-16 15:11 ` [PATCH 2/2] Add revision range support on "-" and "@{-1}" Kenny Lee Sin Cheong
2015-03-16 18:22 ` Junio C Hamano
2015-03-17 6:49 ` Junio C Hamano
2015-03-17 21:25 ` Kenny Lee Sin Cheong
2015-03-17 22:16 ` Junio C Hamano [this message]
2015-03-24 0:09 ` Kenny Lee Sin Cheong
2015-03-25 22:24 ` Junio C Hamano
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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqpp87mfqx.fsf@gitster.dls.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=kenny.lee28@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox