Patchwork revset: added smartset attribute to new classes to test at mfunc and getset

login
register
mail settings
Submitter Lucas Moscovicz
Date Feb. 19, 2014, 12:13 a.m.
Message ID <d9813f85f80e36e413da.1392768790@dev1037.prn2.facebook.com>
Download mbox | patch
Permalink /patch/3693/
State Deferred
Headers show

Comments

Lucas Moscovicz - Feb. 19, 2014, 12:13 a.m.
# HG changeset patch
# User Lucas Moscovicz <lmoscovicz@fb.com>
# Date 1392767686 28800
#      Tue Feb 18 15:54:46 2014 -0800
# Node ID d9813f85f80e36e413da7bd1b5edd3a2118bc715
# Parent  c29948fed40a2d9755ecaa01ec05bfa542f65670
revset: added smartset attribute to new classes to test at mfunc and getset

Now extensions shouldn't break when adding new revsets.
Pierre-Yves David - Feb. 19, 2014, 12:25 a.m.
On 02/18/2014 04:13 PM, Lucas Moscovicz wrote:
> # HG changeset patch
> # User Lucas Moscovicz <lmoscovicz@fb.com>
> # Date 1392767686 28800
> #      Tue Feb 18 15:54:46 2014 -0800
> # Node ID d9813f85f80e36e413da7bd1b5edd3a2118bc715
> # Parent  c29948fed40a2d9755ecaa01ec05bfa542f65670
> revset: added smartset attribute to new classes to test at mfunc and getset

I believe this patch is important.  There is a lots of extension out 
there implementing new revset. We are adding smarter feature which are 
mostly a super set of the old behavior (using list). There is no good 
reason to break the compatibility for our user out there.

I know we do not have a public API but this does not mean we should 
gratuitously break widely used a trivial API. (I could compare that to 
breaking the changectx API for no good reason).

> Now extensions shouldn't break when adding new revsets.

Did you actually test it with old style code?

> diff --git a/mercurial/revset.py b/mercurial/revset.py
> --- a/mercurial/revset.py
> +++ b/mercurial/revset.py
> @@ -195,7 +195,10 @@
>   def getset(repo, subset, x):
>       if not x:
>           raise error.ParseError(_("missing argument"))
> -    return methods[x[0]](repo, subset, *x[1:])
> +    s = methods[x[0]](repo, subset, *x[1:])
> +    if util.safehasattr(s, 'smartset'):
> +        return s

We can probably just check for the presence of a `set` attribute. as it 
is the most visible addition of the new object.
Lucas Moscovicz - Feb. 19, 2014, 12:37 a.m.
On 2/18/14, 4:25 PM, "Pierre-Yves David" <pierre-yves.david@ens-lyon.org>
wrote:

>
>
>On 02/18/2014 04:13 PM, Lucas Moscovicz wrote:
>> # HG changeset patch
>> # User Lucas Moscovicz <lmoscovicz@fb.com>
>> # Date 1392767686 28800
>> #      Tue Feb 18 15:54:46 2014 -0800
>> # Node ID d9813f85f80e36e413da7bd1b5edd3a2118bc715
>> # Parent  c29948fed40a2d9755ecaa01ec05bfa542f65670
>> revset: added smartset attribute to new classes to test at mfunc and
>>getset
>
>I believe this patch is important.  There is a lots of extension out
>there implementing new revset. We are adding smarter feature which are
>mostly a super set of the old behavior (using list). There is no good
>reason to break the compatibility for our user out there.
>
>I know we do not have a public API but this does not mean we should
>gratuitously break widely used a trivial API. (I could compare that to
>breaking the changectx API for no good reason).
>
>> Now extensions shouldn't break when adding new revsets.
>
>Did you actually test it with old style code?

I just tested it with an extension that used to fail over this same
problem and I had fixed manually.

>
>> diff --git a/mercurial/revset.py b/mercurial/revset.py
>> --- a/mercurial/revset.py
>> +++ b/mercurial/revset.py
>> @@ -195,7 +195,10 @@
>>   def getset(repo, subset, x):
>>       if not x:
>>           raise error.ParseError(_("missing argument"))
>> -    return methods[x[0]](repo, subset, *x[1:])
>> +    s = methods[x[0]](repo, subset, *x[1:])
>> +    if util.safehasattr(s, 'smartset'):
>> +        return s
>
>We can probably just check for the presence of a `set` attribute. as it
>is the most visible addition of the new object.
>
>-- 
>Pierre-Yves
Pierre-Yves David - Feb. 19, 2014, 12:40 a.m.
On 02/18/2014 04:37 PM, Lucas Moscovicz wrote:
>
>
> On 2/18/14, 4:25 PM, "Pierre-Yves David" <pierre-yves.david@ens-lyon.org>
> wrote:
>
>>
>>
>> On 02/18/2014 04:13 PM, Lucas Moscovicz wrote:
>>> # HG changeset patch
>>> # User Lucas Moscovicz <lmoscovicz@fb.com>
>>> # Date 1392767686 28800
>>> #      Tue Feb 18 15:54:46 2014 -0800
>>> # Node ID d9813f85f80e36e413da7bd1b5edd3a2118bc715
>>> # Parent  c29948fed40a2d9755ecaa01ec05bfa542f65670
>>> revset: added smartset attribute to new classes to test at mfunc and
>>> getset
>>
>> I believe this patch is important.  There is a lots of extension out
>> there implementing new revset. We are adding smarter feature which are
>> mostly a super set of the old behavior (using list). There is no good
>> reason to break the compatibility for our user out there.
>>
>> I know we do not have a public API but this does not mean we should
>> gratuitously break widely used a trivial API. (I could compare that to
>> breaking the changectx API for no good reason).
>>
>>> Now extensions shouldn't break when adding new revsets.
>>
>> Did you actually test it with old style code?
>
> I just tested it with an extension that used to fail over this same
> problem and I had fixed manually.

Which extension? (out of curiosity)
Lucas Moscovicz - Feb. 19, 2014, 1:23 a.m.
On 2/18/14, 4:40 PM, "Pierre-Yves David" <pierre-yves.david@ens-lyon.org>
wrote:

>
>
>On 02/18/2014 04:37 PM, Lucas Moscovicz wrote:
>>
>>
>> On 2/18/14, 4:25 PM, "Pierre-Yves David"
>><pierre-yves.david@ens-lyon.org>
>> wrote:
>>
>>>
>>>
>>> On 02/18/2014 04:13 PM, Lucas Moscovicz wrote:
>>>> # HG changeset patch
>>>> # User Lucas Moscovicz <lmoscovicz@fb.com>
>>>> # Date 1392767686 28800
>>>> #      Tue Feb 18 15:54:46 2014 -0800
>>>> # Node ID d9813f85f80e36e413da7bd1b5edd3a2118bc715
>>>> # Parent  c29948fed40a2d9755ecaa01ec05bfa542f65670
>>>> revset: added smartset attribute to new classes to test at mfunc and
>>>> getset
>>>
>>> I believe this patch is important.  There is a lots of extension out
>>> there implementing new revset. We are adding smarter feature which are
>>> mostly a super set of the old behavior (using list). There is no good
>>> reason to break the compatibility for our user out there.
>>>
>>> I know we do not have a public API but this does not mean we should
>>> gratuitously break widely used a trivial API. (I could compare that to
>>> breaking the changectx API for no good reason).
>>>
>>>> Now extensions shouldn't break when adding new revsets.
>>>
>>> Did you actually test it with old style code?
>>
>> I just tested it with an extension that used to fail over this same
>> problem and I had fixed manually.
>
>Which extension? (out of curiosity)

Largefiles

In hgext/largefiles/overrides.py there was a revset defined which had that
same problem.

>
>-- 
>Pierre-Yves
Matt Mackall - Feb. 19, 2014, 8:32 p.m.
On Tue, 2014-02-18 at 16:13 -0800, Lucas Moscovicz wrote:
> # HG changeset patch
> # User Lucas Moscovicz <lmoscovicz@fb.com>
> # Date 1392767686 28800
> #      Tue Feb 18 15:54:46 2014 -0800
> # Node ID d9813f85f80e36e413da7bd1b5edd3a2118bc715
> # Parent  c29948fed40a2d9755ecaa01ec05bfa542f65670
> revset: added smartset attribute to new classes to test at mfunc and getset
> 
> Now extensions shouldn't break when adding new revsets.

Am I right in thinking all revset classes already implement a set()
method while legacy revset lists won't? Perhaps we should distinguish on
that instead.

Patch

diff --git a/mercurial/revset.py b/mercurial/revset.py
--- a/mercurial/revset.py
+++ b/mercurial/revset.py
@@ -195,7 +195,10 @@ 
 def getset(repo, subset, x):
     if not x:
         raise error.ParseError(_("missing argument"))
-    return methods[x[0]](repo, subset, *x[1:])
+    s = methods[x[0]](repo, subset, *x[1:])
+    if util.safehasattr(s, 'smartset'):
+        return s
+    return baseset(s)
 
 def _getrevsource(repo, r):
     extra = repo[r].extra()
@@ -1919,7 +1922,9 @@ 
         tree = findaliases(ui, tree)
     weight, tree = optimize(tree, True)
     def mfunc(repo, subset):
-        return getset(repo, subset, tree)
+        if util.safehasattr(subset, 'smartset'):
+            return getset(repo, subset, tree)
+        return getset(repo, baseset(subset), tree)
     return mfunc
 
 def formatspec(expr, *args):
@@ -2052,6 +2057,8 @@ 
     """Basic data structure that represents a revset and contains the basic
     operation that it should be able to perform.
     """
+    smartset = True
+
     def __init__(self, data):
         super(baseset, self).__init__(data)
         self._set = None
@@ -2083,6 +2090,8 @@ 
     the subset and contains a function which tests for membership in the
     revset
     """
+    smartset = True
+
     def __init__(self, subset, condition):
         self._subset = subset
         self._condition = condition
@@ -2135,6 +2144,8 @@ 
     """Duck type for baseset class which represents a range of revisions and
     can work lazily and without having all the range in memory
     """
+    smartset = True
+
     def __init__(self, repo, start=0, end=None):
         self._start = start
         if end is not None: