| Submitter | Mads Kiilerich |
|---|---|
| Date | Oct. 20, 2014, 2:56 a.m. |
| Message ID | <2ee3d9d0bc7be9b67f5f.1413773809@localhost.localdomain> |
| Download | mbox | patch |
| Permalink | /patch/6414/ |
| State | Rejected |
| Headers | show |
Comments
On 10/19/2014 07:56 PM, Mads Kiilerich wrote: > # HG changeset patch > # User Mads Kiilerich <madski@unity3d.com> > # Date 1413773777 -7200 > # Mon Oct 20 04:56:17 2014 +0200 > # Branch stable > # Node ID 2ee3d9d0bc7be9b67f5f5f05db6faa2a9d927e2a > # Parent c1ae0b2c1719f56b906472efea8b20ca0774c968 > WHAT: PyExc_TypeError in parsers.c is lost - use PyExc_ValueError instead > > This gives a reasonable and helpful error on Python 2.4: > + File "$HGTMP/install/lib/python/mercurial/changelog.py", line 175, in headrevs > + return self.index.headrevs(self.filteredrevs) > + ValueError: unable to check filter > + [1] > With TypeError the exception is lost and the code will continue but misbehave. > In the tests it shows as test-glog.t failing in weird ways. Python 2.7.8 also > seems to ignore TypeError here. > > This doesn't make sense and we should figure out what really is going on, but > it helps pointing out why test-glog.t is failing on Python 2.4. > > No other tests are failing even though index.headrevs terminates with an error > that is ignored. That suggests that we have insufficient test coverage of the > headrevs code. I believe that the changelog is catxching the TypeError: http://selenic.com/hg/file/d583f1cfca96/mercurial/changelog.py#l172 We should fix that code to explicitly check for the C version attribute and stick on type error.
On 10/23/2014 03:30 AM, Pierre-Yves David wrote: > I believe that the changelog is catxching the TypeError: > > http://selenic.com/hg/file/d583f1cfca96/mercurial/changelog.py#l172 Right, thanks. I was sure changelog was calling the C extension directly. I assumed I was right. Wrong assumptions can invalidate even the best reasoning. :-( > We should fix that code to explicitly check for the C version > attribute and stick on type error. Which C version attribute? AFAIK the C extension (and Mercurial in general) don't have version numbers, only features. Unfortunately, in this case the C extension got a new feature, and the absence of that feature can only be detected as TypeError when calling with a parameter ... which unfortunately is indistinguishable from other runtime type errors. inspect only works on Python code. (Ok, not completely indistinguishable. We could check if TypeError .args == ('headrevs() takes no arguments (1 given)',) ... but that is too ugly.) AFAICS, the only options are to check on something existing, or to add something we easily can check. AFAICS, the only existing thing we can check is asciilower which was introduced after the filtering headrevs. Alternatively, we could introduce a version number or rename headrevs or add a filteringheadrevs method or do something else we can detect. 3.2 is a new major version and there has been significant improvements to the C extension. It could perhaps make sense to require new extensions for this version. /Mads
Patch
diff --git a/mercurial/parsers.c b/mercurial/parsers.c --- a/mercurial/parsers.c +++ b/mercurial/parsers.c @@ -950,7 +950,7 @@ static PyObject *index_headrevs(indexObj isfiltered = check_filter(filter, i); if (isfiltered == -1) { - PyErr_SetString(PyExc_TypeError, + PyErr_SetString(PyExc_ValueError, "unable to check filter"); goto bail; }