Patchwork [stable] graphmod: don't try to visit nullrev (issue3772)

login
register
mail settings
Submitter Sean Farley
Date Jan. 22, 2013, 7:05 p.m.
Message ID <CANH+MCiem0OHCw44WT6k561Dj3CoWq=Ogwr5Q4xJ1mW=5b7wSQ@mail.gmail.com>
Download mbox | patch
Permalink /patch/713/
State Superseded
Headers show

Comments

Sean Farley - Jan. 22, 2013, 7:05 p.m.
On Tue, Jan 22, 2013 at 1:01 PM, Matt Mackall <mpm@selenic.com> wrote:
> On Tue, 2013-01-22 at 13:47 -0500, Augie Fackler wrote:
>> On Tue, Jan 22, 2013 at 1:45 PM, Sean Farley
>> <sean.michael.farley@gmail.com> wrote:
>> > On IRC, Kevin and Pierre-Yves were thinking that this is a bug in
>> > 'revrange' and that ":null" should return [-1] instead of [0,-1].
>> > Thoughts?
>>
>>
>> Interesting. I'm somewhat inclined to agree, as nothing is an ancestor of null.
>
> Agreed. This result is nonsense:
>
> $ hg log -r :null
> changeset:   0:9117c6561b0b
> user:        mpm@selenic.com
> date:        Tue May 03 13:16:10 2005 -0800
> summary:     Add back links from file revisions to changeset revisions
>
> changeset:   -1:000000000000
> user:
> date:        Thu Jan 01 00:00:00 1970 +0000
>
> As is this one, which is wrong in a different way:
>
> $ hg log -r '(:null)'
> changeset:   0:9117c6561b0b
> user:        mpm@selenic.com
> date:        Tue May 03 13:16:10 2005 -0800
> summary:     Add back links from file revisions to changeset revisions

I have a simple patch for this but think I need to split it up (both
to be applied on top of Bryan's patch). I wanted to be sure that the
change to using "repo['tip']" was correct for filtering and also to
check to see if there was a better way than "if end == -1",
Pierre-Yves David - Jan. 22, 2013, 7:26 p.m.
On 22 janv. 2013, at 20:05, Sean Farley wrote:

> On Tue, Jan 22, 2013 at 1:01 PM, Matt Mackall <mpm@selenic.com> wrote:
>> On Tue, 2013-01-22 at 13:47 -0500, Augie Fackler wrote:
>>> On Tue, Jan 22, 2013 at 1:45 PM, Sean Farley
>>> <sean.michael.farley@gmail.com> wrote:
>>>> On IRC, Kevin and Pierre-Yves were thinking that this is a bug in
>>>> 'revrange' and that ":null" should return [-1] instead of [0,-1].
>>>> Thoughts?
>>> 
>>> 
>>> Interesting. I'm somewhat inclined to agree, as nothing is an ancestor of null.
>> 
>> Agreed. This result is nonsense:
>> 
>> $ hg log -r :null
>> changeset:   0:9117c6561b0b
>> user:        mpm@selenic.com
>> date:        Tue May 03 13:16:10 2005 -0800
>> summary:     Add back links from file revisions to changeset revisions
>> 
>> changeset:   -1:000000000000
>> user:
>> date:        Thu Jan 01 00:00:00 1970 +0000
>> 
>> As is this one, which is wrong in a different way:
>> 
>> $ hg log -r '(:null)'
>> changeset:   0:9117c6561b0b
>> user:        mpm@selenic.com
>> date:        Tue May 03 13:16:10 2005 -0800
>> summary:     Add back links from file revisions to changeset revisions
> 

All those error probably comes from the revlog.rev trying to be smarter than xrange.

xrange(start, end, step)
- generate from start
- in "step" order
- until end is reach (not included)

changelog.revs(start, stop)
- generate from start
- in the order that allow to reach end (reverse if stop start)
- end is included

As a result cl.revs is:
- unable to generate empty list
- can accidentally generate a non sensical reversed output instead of an empty list


> I have a simple patch for this but think I need to split it up (both
> to be applied on top of Bryan's patch). I wanted to be sure that the
> change to using "repo['tip']" was correct for filtering and also to
> check to see if there was a better way than "if end == -1",

repo['tip'] is correct regarding filtering (But I sent a fix about it today)

len(repo) - 1 is probably not wrong either as it will be filtered by changelog.rev

The best option is probably to leave end to None and leave changelog.revs do it's jobs

I do not see a better option than checking end==nullrev as changelog.rev is totally unaware that nullrev is valid iteration. As Matt pointed out, revset is also unaware of nullrev

Patch

diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
--- a/mercurial/scmutil.py
+++ b/mercurial/scmutil.py
@@ -644,11 +644,13 @@  def revrange(repo, revs):
                 continue

             if _revrangesep in spec:
                 start, end = spec.split(_revrangesep, 1)
                 start = revfix(repo, start, 0)
-                end = revfix(repo, end, len(repo) - 1)
+                end = revfix(repo, end, repo['tip'])
+                if end == -1:
+                    start = -1
                 rangeiter = repo.changelog.revs(start, end)
                 if not seen and not l:
                     # by far the most common case: revs = ["-1:0"]
                     l = list(rangeiter)
                     # defer syncing seen until next iteration