Patchwork [2,of,2,STABLE] revset: handle old-style subset input by getset() function (issue4515)

login
register
mail settings
Submitter Yuya Nishihara
Date Jan. 30, 2015, 2:08 p.m.
Message ID <7fe084aeb4226dfe0f15.1422626882@mimosa>
Download mbox | patch
Permalink /patch/7577/
State Deferred
Headers show

Comments

Yuya Nishihara - Jan. 30, 2015, 2:08 p.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1422624046 -32400
#      Fri Jan 30 22:20:46 2015 +0900
# Branch stable
# Node ID 7fe084aeb4226dfe0f15a81cb36304f5194640ec
# Parent  03b6a2472e4e713f62e91549ee166c04e547adb2
revset: handle old-style subset input by getset() function (issue4515)

Old extensions may call revset.getset() with range(len(repo)) to evaluate
the argument set.
Matt Mackall - Jan. 30, 2015, 8:14 p.m.
On Fri, 2015-01-30 at 23:08 +0900, Yuya Nishihara wrote:
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1422624046 -32400
> #      Fri Jan 30 22:20:46 2015 +0900
> # Branch stable
> # Node ID 7fe084aeb4226dfe0f15a81cb36304f5194640ec
> # Parent  03b6a2472e4e713f62e91549ee166c04e547adb2
> revset: handle old-style subset input by getset() function (issue4515)
> 
> Old extensions may call revset.getset() with range(len(repo)) to evaluate
> the argument set.

Not super-excited about this. We probably should have changed the name
of getset to forcibly broken affected extensions instead... 9 months
ago.
Pierre-Yves David - Jan. 30, 2015, 8:17 p.m.
On 01/30/2015 08:14 PM, Matt Mackall wrote:
> On Fri, 2015-01-30 at 23:08 +0900, Yuya Nishihara wrote:
>> # HG changeset patch
>> # User Yuya Nishihara <yuya@tcha.org>
>> # Date 1422624046 -32400
>> #      Fri Jan 30 22:20:46 2015 +0900
>> # Branch stable
>> # Node ID 7fe084aeb4226dfe0f15a81cb36304f5194640ec
>> # Parent  03b6a2472e4e713f62e91549ee166c04e547adb2
>> revset: handle old-style subset input by getset() function (issue4515)
>>
>> Old extensions may call revset.getset() with range(len(repo)) to evaluate
>> the argument set.
>
> Not super-excited about this. We probably should have changed the name
> of getset to forcibly broken affected extensions instead... 9 months
> ago.

I'm a bit reluctant to gratuitously break unsuspicious extension. What 
about adding a developer warning in 3.4?
Matt Mackall - Jan. 30, 2015, 9:53 p.m.
On Fri, 2015-01-30 at 20:17 +0000, Pierre-Yves David wrote:
> 
> On 01/30/2015 08:14 PM, Matt Mackall wrote:
> > On Fri, 2015-01-30 at 23:08 +0900, Yuya Nishihara wrote:
> >> # HG changeset patch
> >> # User Yuya Nishihara <yuya@tcha.org>
> >> # Date 1422624046 -32400
> >> #      Fri Jan 30 22:20:46 2015 +0900
> >> # Branch stable
> >> # Node ID 7fe084aeb4226dfe0f15a81cb36304f5194640ec
> >> # Parent  03b6a2472e4e713f62e91549ee166c04e547adb2
> >> revset: handle old-style subset input by getset() function (issue4515)
> >>
> >> Old extensions may call revset.getset() with range(len(repo)) to evaluate
> >> the argument set.
> >
> > Not super-excited about this. We probably should have changed the name
> > of getset to forcibly broken affected extensions instead... 9 months
> > ago.
> 
> I'm a bit reluctant to gratuitously break unsuspicious extension.

Let me recap:

- we changed the getset API
- you said "not breaking extensions is nice"
- we put in a hack to quietly make the getset function backward
compatible
- that hack didn't work
- an extension broke
- but the extension author didn't notice
- for 9 months
- because we tried to be nice
- and the extension in question is **yours**

In other words... we introduced unintentional invisible breakage... in
code you maintain... for an extension you also maintain... because you
were not keen on instead introducing intentional visible breakage for
third-party developers.

I gotta say, you're not really selling me on this one.
Pierre-Yves David - Jan. 30, 2015, 9:56 p.m.
On 01/30/2015 09:53 PM, Matt Mackall wrote:
> On Fri, 2015-01-30 at 20:17 +0000, Pierre-Yves David wrote:
>>
>> On 01/30/2015 08:14 PM, Matt Mackall wrote:
>>> On Fri, 2015-01-30 at 23:08 +0900, Yuya Nishihara wrote:
>>>> # HG changeset patch
>>>> # User Yuya Nishihara <yuya@tcha.org>
>>>> # Date 1422624046 -32400
>>>> #      Fri Jan 30 22:20:46 2015 +0900
>>>> # Branch stable
>>>> # Node ID 7fe084aeb4226dfe0f15a81cb36304f5194640ec
>>>> # Parent  03b6a2472e4e713f62e91549ee166c04e547adb2
>>>> revset: handle old-style subset input by getset() function (issue4515)
>>>>
>>>> Old extensions may call revset.getset() with range(len(repo)) to evaluate
>>>> the argument set.
>>>
>>> Not super-excited about this. We probably should have changed the name
>>> of getset to forcibly broken affected extensions instead... 9 months
>>> ago.
>>
>> I'm a bit reluctant to gratuitously break unsuspicious extension.
>
> Let me recap:
>
> - we changed the getset API
> - you said "not breaking extensions is nice"
> - we put in a hack to quietly make the getset function backward
> compatible
> - that hack didn't work
> - an extension broke
> - but the extension author didn't notice
> - for 9 months
> - because we tried to be nice
> - and the extension in question is **yours**
>
> In other words... we introduced unintentional invisible breakage... in
> code you maintain... for an extension you also maintain... because you
> were not keen on instead introducing intentional visible breakage for
> third-party developers.
>
> I gotta say, you're not really selling me on this one.

This is why I advertise for and explicit developer warning (as discussed 
multiple time) so that such people notice it.

Patch

diff --git a/mercurial/revset.py b/mercurial/revset.py
--- a/mercurial/revset.py
+++ b/mercurial/revset.py
@@ -304,6 +304,8 @@  def getfuncargs(tree):
 def getset(repo, subset, x):
     if not x:
         raise error.ParseError(_("missing argument"))
+    if not util.safehasattr(subset, 'isascending'):
+        subset = baseset(subset)
     s = methods[x[0]](repo, subset, *x[1:])
     if util.safehasattr(s, 'isascending'):
         return s
@@ -2389,11 +2391,7 @@  def match(ui, spec, repo=None):
     tree = foldconcat(tree)
     weight, tree = optimize(tree, True)
     def mfunc(repo, subset):
-        if util.safehasattr(subset, 'isascending'):
-            result = getset(repo, subset, tree)
-        else:
-            result = getset(repo, baseset(subset), tree)
-        return result
+        return getset(repo, subset, tree)
     return mfunc
 
 def formatspec(expr, *args):
diff --git a/tests/test-revset.t b/tests/test-revset.t
--- a/tests/test-revset.t
+++ b/tests/test-revset.t
@@ -1284,8 +1284,12 @@  compatibility layer for old extension th
   > def aslist(repo, subset, x):
   >     revset.getargs(x, 0, 0, 'aslist takes no argument')
   >     return list(subset)
+  > def xlist(repo, subset, x):
+  >     s = revset.getset(repo, range(len(repo)), x)
+  >     return [r for r in subset if r in s]
   > def uisetup(ui):
   >     revset.symbols['aslist'] = aslist
+  >     revset.symbols['xlist'] = xlist
   > EOF
 
   $ cd repo
@@ -1294,5 +1298,7 @@  compatibility layer for old extension th
   9
   $ hg --config extensions.lset=../lset.py debugrevspec 'min(aslist())'
   0
+  $ hg --config extensions.lset=../lset.py debugrevspec 'xlist(p1())'
+  9
 
   $ cd ..