Patchwork [1,of,2,stable] WHAT: PyExc_TypeError in parsers.c is lost - use PyExc_ValueError instead

login
register
mail settings
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

Mads Kiilerich - Oct. 20, 2014, 2:56 a.m.
# 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.
Pierre-Yves David - Oct. 23, 2014, 1:30 a.m.
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.
Mads Kiilerich - Oct. 24, 2014, 12:37 a.m.
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;
 		}