| 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
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.
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?
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.
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 ..