Patchwork revset: rework 'filteredset.last'

login
register
mail settings
Submitter Pierre-Yves David
Date June 22, 2015, 9:57 p.m.
Message ID <5436ea241ec1c12294d7.1435010250@marginatus.alto.octopoid.net>
Download mbox | patch
Permalink /patch/9753/
State Accepted
Headers show

Comments

Pierre-Yves David - June 22, 2015, 9:57 p.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@fb.com>
# Date 1435006081 25200
#      Mon Jun 22 13:48:01 2015 -0700
# Node ID 5436ea241ec1c12294d7868de6c679d10d333060
# Parent  7fdd1782fc4ee9da87d8af13e806dc9055db2c38
revset: rework 'filteredset.last'

As 'isascending' and 'isdescending' are method, not attribute. This led 'last()'
to misbehave on some non-ascending filtered set.
Augie Fackler - June 23, 2015, 2:15 p.m.
On Mon, Jun 22, 2015 at 02:57:30PM -0700, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@fb.com>
> # Date 1435006081 25200
> #      Mon Jun 22 13:48:01 2015 -0700
> # Node ID 5436ea241ec1c12294d7868de6c679d10d333060
> # Parent  7fdd1782fc4ee9da87d8af13e806dc9055db2c38
> revset: rework 'filteredset.last'

queued, thanks

>
> As 'isascending' and 'isdescending' are method, not attribute. This led 'last()'
> to misbehave on some non-ascending filtered set.
>
> diff --git a/mercurial/revset.py b/mercurial/revset.py
> --- a/mercurial/revset.py
> +++ b/mercurial/revset.py
> @@ -3022,20 +3022,22 @@ class filteredset(abstractsmartset):
>              return x
>          return None
>
>      def last(self):
>          it = None
> -        if self._subset.isascending:
> +        if self.isascending():
>              it = self.fastdesc
> -        elif self._subset.isdescending:
> -            it = self.fastdesc
> -        if it is None:
> -            # slowly consume everything. This needs improvement
> -            it = lambda: reversed(list(self))
> -        for x in it():
> +        elif self.isdescending():
> +            it = self.fastasc
> +        if it is not None:
> +            for x in it():
> +                return x
> +        else:
> +            for x in self:
> +                pass
>              return x
> -        return None
> +        return None #empty case
>
>      def __repr__(self):
>          return '<%s %r>' % (type(self).__name__, self._subset)
>
>  # this function will be removed, or merged to addset or orset, when
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
Yuya Nishihara - June 23, 2015, 2:37 p.m.
On Mon, 22 Jun 2015 14:57:30 -0700, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@fb.com>
> # Date 1435006081 25200
> #      Mon Jun 22 13:48:01 2015 -0700
> # Node ID 5436ea241ec1c12294d7868de6c679d10d333060
> # Parent  7fdd1782fc4ee9da87d8af13e806dc9055db2c38
> revset: rework 'filteredset.last'
> 
> As 'isascending' and 'isdescending' are method, not attribute. This led 'last()'
> to misbehave on some non-ascending filtered set.
> 
> diff --git a/mercurial/revset.py b/mercurial/revset.py
> --- a/mercurial/revset.py
> +++ b/mercurial/revset.py
> @@ -3022,20 +3022,22 @@ class filteredset(abstractsmartset):
>              return x
>          return None
>  
>      def last(self):
>          it = None
> -        if self._subset.isascending:
> +        if self.isascending():
>              it = self.fastdesc
> -        elif self._subset.isdescending:
> -            it = self.fastdesc
> -        if it is None:
> -            # slowly consume everything. This needs improvement
> -            it = lambda: reversed(list(self))
> -        for x in it():
> +        elif self.isdescending():
> +            it = self.fastasc
> +        if it is not None:
> +            for x in it():
> +                return x
> +        else:

> +            for x in self:
> +                pass
>              return x

It will fail if self is empty.

>>> revset.filteredset(revset.generatorset([])).last()
...
UnboundLocalError: local variable 'x' referenced before assignment
Augie Fackler - June 23, 2015, 7:55 p.m.
On Tue, Jun 23, 2015 at 10:37 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> On Mon, 22 Jun 2015 14:57:30 -0700, Pierre-Yves David wrote:
>> # HG changeset patch
>> # User Pierre-Yves David <pierre-yves.david@fb.com>
>> # Date 1435006081 25200
>> #      Mon Jun 22 13:48:01 2015 -0700
>> # Node ID 5436ea241ec1c12294d7868de6c679d10d333060
>> # Parent  7fdd1782fc4ee9da87d8af13e806dc9055db2c38
>> revset: rework 'filteredset.last'
>>
>> As 'isascending' and 'isdescending' are method, not attribute. This led 'last()'
>> to misbehave on some non-ascending filtered set.
>>
>> diff --git a/mercurial/revset.py b/mercurial/revset.py
>> --- a/mercurial/revset.py
>> +++ b/mercurial/revset.py
>> @@ -3022,20 +3022,22 @@ class filteredset(abstractsmartset):

>>              return x
>
> It will fail if self is empty.
>
>>>> revset.filteredset(revset.generatorset([])).last()
> ...
> UnboundLocalError: local variable 'x' referenced before assignment

Nice catch. Should I drop this patch, or just await a followup?

> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
Pierre-Yves David - June 23, 2015, 8:06 p.m.
On 06/23/2015 12:55 PM, Augie Fackler wrote:
> On Tue, Jun 23, 2015 at 10:37 AM, Yuya Nishihara <yuya@tcha.org> wrote:
>> On Mon, 22 Jun 2015 14:57:30 -0700, Pierre-Yves David wrote:
>>> # HG changeset patch
>>> # User Pierre-Yves David <pierre-yves.david@fb.com>
>>> # Date 1435006081 25200
>>> #      Mon Jun 22 13:48:01 2015 -0700
>>> # Node ID 5436ea241ec1c12294d7868de6c679d10d333060
>>> # Parent  7fdd1782fc4ee9da87d8af13e806dc9055db2c38
>>> revset: rework 'filteredset.last'
>>>
>>> As 'isascending' and 'isdescending' are method, not attribute. This led 'last()'
>>> to misbehave on some non-ascending filtered set.
>>>
>>> diff --git a/mercurial/revset.py b/mercurial/revset.py
>>> --- a/mercurial/revset.py
>>> +++ b/mercurial/revset.py
>>> @@ -3022,20 +3022,22 @@ class filteredset(abstractsmartset):
>
>>>               return x
>>
>> It will fail if self is empty.
>>
>>>>> revset.filteredset(revset.generatorset([])).last()
>> ...
>> UnboundLocalError: local variable 'x' referenced before assignment
>
> Nice catch. Should I drop this patch, or just await a followup?

Just before the loop add

x = None

(please)
Augie Fackler - June 23, 2015, 8:07 p.m.
On Tue, Jun 23, 2015 at 4:06 PM, Pierre-Yves David
<pierre-yves.david@ens-lyon.org> wrote:
>> Nice catch. Should I drop this patch, or just await a followup?
>
>
> Just before the loop add
>
> x = None
>
> (please)


Please send me a diff, or amend the patch that's on the clowncopter?
Pierre-Yves David - June 23, 2015, 8:12 p.m.
On 06/23/2015 01:07 PM, Augie Fackler wrote:
> On Tue, Jun 23, 2015 at 4:06 PM, Pierre-Yves David
> <pierre-yves.david@ens-lyon.org> wrote:
>>> Nice catch. Should I drop this patch, or just await a followup?
>>
>>
>> Just before the loop add
>>
>> x = None
>>
>> (please)
>
>
> Please send me a diff, or amend the patch that's on the clowncopter?

I've amended it and will push back once the test are done running.

Patch

diff --git a/mercurial/revset.py b/mercurial/revset.py
--- a/mercurial/revset.py
+++ b/mercurial/revset.py
@@ -3022,20 +3022,22 @@  class filteredset(abstractsmartset):
             return x
         return None
 
     def last(self):
         it = None
-        if self._subset.isascending:
+        if self.isascending():
             it = self.fastdesc
-        elif self._subset.isdescending:
-            it = self.fastdesc
-        if it is None:
-            # slowly consume everything. This needs improvement
-            it = lambda: reversed(list(self))
-        for x in it():
+        elif self.isdescending():
+            it = self.fastasc
+        if it is not None:
+            for x in it():
+                return x
+        else:
+            for x in self:
+                pass
             return x
-        return None
+        return None #empty case
 
     def __repr__(self):
         return '<%s %r>' % (type(self).__name__, self._subset)
 
 # this function will be removed, or merged to addset or orset, when