Patchwork changelog: only use filtering headrevs C extension when it is available

login
register
mail settings
Submitter Mads Kiilerich
Date Oct. 24, 2014, 12:39 a.m.
Message ID <c631a5ff9815a692b8b8.1414111184@localhost.localdomain>
Download mbox | patch
Permalink /patch/6458/
State Superseded
Headers show

Comments

Mads Kiilerich - Oct. 24, 2014, 12:39 a.m.
# HG changeset patch
# User Mads Kiilerich <madski@unity3d.com>
# Date 1414111164 -7200
#      Fri Oct 24 02:39:24 2014 +0200
# Branch stable
# Node ID c631a5ff9815a692b8b816bdbb71cd7adb60ded4
# Parent  eb763217152ab2b472416bcc57722451c317f282
changelog: only use filtering headrevs C extension when it is available

2b5940f64750 promised backwards compatibility with old C extensions that didn't
have the new filtering headrevs implementation. It did that by catching
TypeError to catch:
  TypeError: headrevs() takes no arguments (1 given)

TypeError can however also be thrown for other reasons, and 5715c93cb854 on
Python 2.4 showed that such Type errors shouldn't be ignored. They can leave
the system in an inconsistent state that cause wrong behaviour.

Instead, don't catch TypeErrors, but check that the C extension has the
asciilower function that was introduced after filtering headrevs was
introduced.

This change before 5715c93cb854 would have given a nice 'TypeError: unable to
check filter' in test-glog.t instead of a spurious failure. After 5715c93cb854
the test passes on 2.4.
Mads Kiilerich - Oct. 24, 2014, 12:40 a.m.
On 10/24/2014 02:39 AM, Mads Kiilerich wrote:
> # HG changeset patch
> # User Mads Kiilerich <madski@unity3d.com>
> # Date 1414111164 -7200
> #      Fri Oct 24 02:39:24 2014 +0200
> # Branch stable
> # Node ID c631a5ff9815a692b8b816bdbb71cd7adb60ded4
> # Parent  eb763217152ab2b472416bcc57722451c317f282
> changelog: only use filtering headrevs C extension when it is available

--flag stable

/Mads
Siddharth Agarwal - Oct. 24, 2014, 1:59 a.m.
On 10/23/2014 05:39 PM, Mads Kiilerich wrote:
> # HG changeset patch
> # User Mads Kiilerich <madski@unity3d.com>
> # Date 1414111164 -7200
> #      Fri Oct 24 02:39:24 2014 +0200
> # Branch stable
> # Node ID c631a5ff9815a692b8b816bdbb71cd7adb60ded4
> # Parent  eb763217152ab2b472416bcc57722451c317f282
> changelog: only use filtering headrevs C extension when it is available
>
> 2b5940f64750 promised backwards compatibility with old C extensions that didn't
> have the new filtering headrevs implementation. It did that by catching
> TypeError to catch:
>    TypeError: headrevs() takes no arguments (1 given)
>
> TypeError can however also be thrown for other reasons, and 5715c93cb854 on
> Python 2.4 showed that such Type errors shouldn't be ignored. They can leave
> the system in an inconsistent state that cause wrong behaviour.
>
> Instead, don't catch TypeErrors, but check that the C extension has the
> asciilower function that was introduced after filtering headrevs was
> introduced.

For layering reasons, the plan post 3.2 is to move asciilower out of 
parsers.so and into base85.so.
Pierre-Yves David - Oct. 24, 2014, 1:21 p.m.
On 10/24/2014 02:39 AM, Mads Kiilerich wrote:
> # HG changeset patch
> # User Mads Kiilerich <madski@unity3d.com>
> # Date 1414111164 -7200
> #      Fri Oct 24 02:39:24 2014 +0200
> # Branch stable
> # Node ID c631a5ff9815a692b8b816bdbb71cd7adb60ded4
> # Parent  eb763217152ab2b472416bcc57722451c317f282
> changelog: only use filtering headrevs C extension when it is available
>
> 2b5940f64750 promised backwards compatibility with old C extensions that didn't
> have the new filtering headrevs implementation. It did that by catching
> TypeError to catch:
>    TypeError: headrevs() takes no arguments (1 given)
>
> TypeError can however also be thrown for other reasons, and 5715c93cb854 on
> Python 2.4 showed that such Type errors shouldn't be ignored. They can leave
> the system in an inconsistent state that cause wrong behaviour.
>
> Instead, don't catch TypeErrors, but check that the C extension has the
> asciilower function that was introduced after filtering headrevs was
> introduced.
>
> This change before 5715c93cb854 would have given a nice 'TypeError: unable to
> check filter' in test-glog.t instead of a spurious failure. After 5715c93cb854
> the test passes on 2.4.
>
> diff --git a/mercurial/changelog.py b/mercurial/changelog.py
> --- a/mercurial/changelog.py
> +++ b/mercurial/changelog.py
> @@ -7,7 +7,7 @@
>
>   from node import bin, hex, nullid
>   from i18n import _
> -import util, error, revlog, encoding
> +import util, error, revlog, encoding, parsers
>
>   _defaultextra = {'branch': 'default'}
>
> @@ -172,11 +172,13 @@ class changelog(revlog.revlog):
>       def headrevs(self):
>           if self.filteredrevs:
>               try:
> -                return self.index.headrevs(self.filteredrevs)
> -            # AttributeError covers non-c-extension environments.
> -            # TypeError allows us work with old c extensions.
> -            except (AttributeError, TypeError):
> +                # Throw AttributeError in non-c-extension environments.
> +                f = self.index.headrevs
> +                # Throw AttributeError if C extension too old.
> +                parsers.asciilower
> +            except AttributeError:
>                   return self._headrevs()
> +            return f(self.filteredrevs)

Actually, I now remind that the TypeError is here to catch the fact we 
(read, durham) added a the filtered revs argument to headrevs. I assume 
that your parser.asciilower access try to detect this but it is far from 
obvious.
Mads Kiilerich - Oct. 24, 2014, 1:31 p.m.
On 10/24/2014 03:21 PM, Pierre-Yves David wrote:
> On 10/24/2014 02:39 AM, Mads Kiilerich wrote:
>> @@ -172,11 +172,13 @@ class changelog(revlog.revlog):
>>       def headrevs(self):
>>           if self.filteredrevs:
>>               try:
>> -                return self.index.headrevs(self.filteredrevs)
>> -            # AttributeError covers non-c-extension environments.
>> -            # TypeError allows us work with old c extensions.
>> -            except (AttributeError, TypeError):
>> +                # Throw AttributeError in non-c-extension environments.
>> +                f = self.index.headrevs
>> +                # Throw AttributeError if C extension too old.
>> +                parsers.asciilower
>> +            except AttributeError:
>>                   return self._headrevs()
>> +            return f(self.filteredrevs)
>
> Actually, I now remind that the TypeError is here to catch the fact we 
> (read, durham) added a the filtered revs argument to headrevs. I 
> assume that your parser.asciilower access try to detect this but it is 
> far from obvious.

It is not obvious that we use asciilower to catch the situation that the 
extension is too old?

/Mads
Pierre-Yves David - Oct. 24, 2014, 1:35 p.m.
On 10/24/2014 03:31 PM, Mads Kiilerich wrote:
> On 10/24/2014 03:21 PM, Pierre-Yves David wrote:
>> On 10/24/2014 02:39 AM, Mads Kiilerich wrote:
>>> @@ -172,11 +172,13 @@ class changelog(revlog.revlog):
>>>       def headrevs(self):
>>>           if self.filteredrevs:
>>>               try:
>>> -                return self.index.headrevs(self.filteredrevs)
>>> -            # AttributeError covers non-c-extension environments.
>>> -            # TypeError allows us work with old c extensions.
>>> -            except (AttributeError, TypeError):
>>> +                # Throw AttributeError in non-c-extension environments.
>>> +                f = self.index.headrevs
>>> +                # Throw AttributeError if C extension too old.
>>> +                parsers.asciilower
>>> +            except AttributeError:
>>>                   return self._headrevs()
>>> +            return f(self.filteredrevs)
>>
>> Actually, I now remind that the TypeError is here to catch the fact we
>> (read, durham) added a the filtered revs argument to headrevs. I
>> assume that your parser.asciilower access try to detect this but it is
>> far from obvious.
>
> It is not obvious that we use asciilower to catch the situation that the
> extension is too old?

It is not obvious that "too old" means:

  "headrevs does not take argument".
Siddharth Agarwal - Oct. 24, 2014, 11:23 p.m.
On 10/23/2014 05:39 PM, Mads Kiilerich wrote:
> # HG changeset patch
> # User Mads Kiilerich <madski@unity3d.com>
> # Date 1414111164 -7200
> #      Fri Oct 24 02:39:24 2014 +0200
> # Branch stable
> # Node ID c631a5ff9815a692b8b816bdbb71cd7adb60ded4
> # Parent  eb763217152ab2b472416bcc57722451c317f282
> changelog: only use filtering headrevs C extension when it is available
>
> 2b5940f64750 promised backwards compatibility with old C extensions that didn't
> have the new filtering headrevs implementation. It did that by catching
> TypeError to catch:
>    TypeError: headrevs() takes no arguments (1 given)
>
> TypeError can however also be thrown for other reasons, and 5715c93cb854 on
> Python 2.4 showed that such Type errors shouldn't be ignored. They can leave
> the system in an inconsistent state that cause wrong behaviour.
>
> Instead, don't catch TypeErrors, but check that the C extension has the
> asciilower function that was introduced after filtering headrevs was
> introduced.

So as I said earlier, asciilower is scheduled to move out of parsers, 
therefore it isn't a reliable indicator of 'are these C bindings too old'.

I'd rather rename the headrevs function.

- Siddharth


>
> This change before 5715c93cb854 would have given a nice 'TypeError: unable to
> check filter' in test-glog.t instead of a spurious failure. After 5715c93cb854
> the test passes on 2.4.
>
> diff --git a/mercurial/changelog.py b/mercurial/changelog.py
> --- a/mercurial/changelog.py
> +++ b/mercurial/changelog.py
> @@ -7,7 +7,7 @@
>   
>   from node import bin, hex, nullid
>   from i18n import _
> -import util, error, revlog, encoding
> +import util, error, revlog, encoding, parsers
>   
>   _defaultextra = {'branch': 'default'}
>   
> @@ -172,11 +172,13 @@ class changelog(revlog.revlog):
>       def headrevs(self):
>           if self.filteredrevs:
>               try:
> -                return self.index.headrevs(self.filteredrevs)
> -            # AttributeError covers non-c-extension environments.
> -            # TypeError allows us work with old c extensions.
> -            except (AttributeError, TypeError):
> +                # Throw AttributeError in non-c-extension environments.
> +                f = self.index.headrevs
> +                # Throw AttributeError if C extension too old.
> +                parsers.asciilower
> +            except AttributeError:
>                   return self._headrevs()
> +            return f(self.filteredrevs)
>   
>           return super(changelog, self).headrevs()
>   
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Pierre-Yves David - Oct. 26, 2014, 10:58 a.m.
On 10/25/2014 01:23 AM, Siddharth Agarwal wrote:
> On 10/23/2014 05:39 PM, Mads Kiilerich wrote:
>> # HG changeset patch
>> # User Mads Kiilerich <madski@unity3d.com>
>> # Date 1414111164 -7200
>> #      Fri Oct 24 02:39:24 2014 +0200
>> # Branch stable
>> # Node ID c631a5ff9815a692b8b816bdbb71cd7adb60ded4
>> # Parent  eb763217152ab2b472416bcc57722451c317f282
>> changelog: only use filtering headrevs C extension when it is available
>>
>> 2b5940f64750 promised backwards compatibility with old C extensions
>> that didn't
>> have the new filtering headrevs implementation. It did that by catching
>> TypeError to catch:
>>    TypeError: headrevs() takes no arguments (1 given)
>>
>> TypeError can however also be thrown for other reasons, and
>> 5715c93cb854 on
>> Python 2.4 showed that such Type errors shouldn't be ignored. They can
>> leave
>> the system in an inconsistent state that cause wrong behaviour.
>>
>> Instead, don't catch TypeErrors, but check that the C extension has the
>> asciilower function that was introduced after filtering headrevs was
>> introduced.
>
> So as I said earlier, asciilower is scheduled to move out of parsers,
> therefore it isn't a reliable indicator of 'are these C bindings too old'.
>
> I'd rather rename the headrevs function.

Unfortunatly, I think Siddharth is right on this.
Mads Kiilerich - Oct. 26, 2014, 11:35 a.m.
On 10/25/2014 01:23 AM, Siddharth Agarwal wrote:
> So as I said earlier, asciilower is scheduled to move out of parsers, 
> therefore it isn't a reliable indicator of 'are these C bindings too 
> old'.

I'm sure references to the old asciilower location will be fixed up if 
moving the function. Not having it is still a reliable indicator.

> I'd rather rename the headrevs function.

Fine with me.

/Mads
Matt Mackall - Oct. 28, 2014, 8:36 p.m.
On Fri, 2014-10-24 at 02:39 +0200, Mads Kiilerich wrote:
> # HG changeset patch
> # User Mads Kiilerich <madski@unity3d.com>
> # Date 1414111164 -7200
> #      Fri Oct 24 02:39:24 2014 +0200
> # Branch stable
> # Node ID c631a5ff9815a692b8b816bdbb71cd7adb60ded4
> # Parent  eb763217152ab2b472416bcc57722451c317f282
> changelog: only use filtering headrevs C extension when it is available

As far as I can tell, this appears to be sorted, so I'm dropping this.

Patch

diff --git a/mercurial/changelog.py b/mercurial/changelog.py
--- a/mercurial/changelog.py
+++ b/mercurial/changelog.py
@@ -7,7 +7,7 @@ 
 
 from node import bin, hex, nullid
 from i18n import _
-import util, error, revlog, encoding
+import util, error, revlog, encoding, parsers
 
 _defaultextra = {'branch': 'default'}
 
@@ -172,11 +172,13 @@  class changelog(revlog.revlog):
     def headrevs(self):
         if self.filteredrevs:
             try:
-                return self.index.headrevs(self.filteredrevs)
-            # AttributeError covers non-c-extension environments.
-            # TypeError allows us work with old c extensions.
-            except (AttributeError, TypeError):
+                # Throw AttributeError in non-c-extension environments.
+                f = self.index.headrevs
+                # Throw AttributeError if C extension too old.
+                parsers.asciilower
+            except AttributeError:
                 return self._headrevs()
+            return f(self.filteredrevs)
 
         return super(changelog, self).headrevs()