[PATCH 0 of 1 stable 3] accept long as revs

Sune Foldager cryo at cyanite.org
Sun Jun 24 07:36:52 CDT 2012


On 2012-05-21 10:19, Matt Mackall wrote:
>On Mon, 2012-05-21 at 16:36 +0200, Sune Foldager wrote:
>> On 2012-05-21 09:28, Matt Mackall wrote:
>> >On Mon, 2012-05-21 at 15:23 +0200, Sune Foldager wrote:
>> >> Since the index is no longer an array (is there a document about the design of the
>> >> new ADT somewhere?), I had to change the C code to make this work. Note that if we
>> >> don't take this patch, since some places do accept long, I bet it will fail in the
>> >> C code (with the weird message "expected string or Unicode. Got long").
>> >
>> >Whoa whoa whoa. I thought we were talking about a one-liner in
>> >context.py.
>> >
>> >Again, using a long here is utterly bogus and misguided. Catching it in
>> >one central place is acceptable, letting that nonsense ripple down into
>> >the C code is not.
>>
>> I'm sorry, but I don't agree. Just because Mercurial doesn't support more than
>> some-number-less-than-intmax revisions, doesn't mean long shouldn't work. No need
>> for all the superlatives, really.
>>
>> It's already handled in several places in our code, and Python in general accepts ints
>> and longs throughout. It's only because we have special isinstance code which really
>> isn't optimal anyway, and because the C code has been introduced that doesn't use
>> PyLong_AsLong (in order to work with both types).
>>
>> If we don't take this or a similar patch, we have a situation where longs work in
>> some places, and not in others, and where it might fail in C code if you use them,
>> with a somewhat obscure error message.
>
>So far, you've only succeeded in convincing me that all the other places
>(one by my count) that currently accept long revisions are latent API
>bugs that should be ripped out.

What about all the python lists? They accept long indexes. Surely you're not suggesting adding
special code to *prevent* long from working with them? But if not, it'll still end up being inconsistent.

>Explain to me why it ever makes sense to cast a revision number to long
>in the first place.

I have no idea; it's not my code, it's in hgtk.

>And remember that "it used to work in my third-party
>app" has never been a valid argument. Thus, when I said you get to catch
>this in context.py, I was being generous.

Well either it works everywhere it did before the new C-module, or not. Catching it in one place isn't gonna help me. The C-code made it regress from accepting longs to only doing it in some places.

I know we don't have a stable API, but generally we don't regress needlessly.

>I've just queued this fix:
>
>diff -r 2403f64b20c0 mercurial/context.py
>--- a/mercurial/context.py	Mon May 21 10:07:52 2012 -0500
>+++ b/mercurial/context.py	Mon May 21 10:11:27 2012 -0500
>@@ -26,6 +26,8 @@
>             self._rev = changeid
>             self._node = repo.changelog.node(changeid)
>             return
>+        if isinstance(changeid, long):
>+            changeid = str(changeid)
>         if changeid == '.':
>             self._node = repo.dirstate.p1()
>             self._rev = repo.changelog.rev(self._node)
>
>Unlike ints, which we always expect to be sane, in-bounds values
>generated internally and need to be as fast as possible, longs obviously
>can't be trusted to be sane and are thus sent through the carefully
>bounds-checked path that command line args go through.

Isn't -42 also a valid int that isn't sane or in bounds? Why are longs worse in this respect?

>Simply doing:
>
>    changeid = int(changeid)
>
>wouldn't help here, as int(BIGNUM) will still give a long.

Right.

-Sune


More information about the Mercurial-devel mailing list