OverflowError when viewing ticket

[gbarberi]

How to Reproduce

I got this error while viewing a ticket right after doing a push on Mercurial and having an automated script issue a "trac-admin changeset added…" wit the CommitTicketUpdater.

This was the "target" variable in the Local Variable table: u'/gameclient@92:569251789268' which looks suspicious to me.

Please let me know if I can provide any more information. I can not view the ticket page at all due to this error but I'm not 100% sure how this can be reproduced at will.

System Information

Trac	0.12dev-r9476
Genshi	0.6dev-r1096
Mercurial	1.5
mod_wsgi	2.7 (WSGIProcessGroup WSGIApplicationGroup %{GLOBAL})
pysqlite	2.4.1
Python	2.6.5 (r265:79096, Mar 19 2010, 21:48:26) [MSC v.1500 32 bit (Intel)]
setuptools	0.6c11
SQLite	3.5.9
Subversion	1.6.6 (r40053)
jQuery	1.4.2

Enabled Plugins

tracaccountmanager	0.2.1dev-r7737
tracmercurial	0.12.0.21dev-r9248

Python Traceback

Traceback (most recent call last):
  File "build\bdist.win32\egg\trac\web\main.py", line 512, in _dispatch_request
    dispatcher.dispatch(req)
  File "build\bdist.win32\egg\trac\web\main.py", line 255, in dispatch
    content_type)
  File "build\bdist.win32\egg\trac\web\chrome.py", line 864, in render_template
    stream.render(method, doctype=doctype, out=buffer)
  File "build\bdist.win32\egg\genshi\core.py", line 183, in render
    return encode(generator, method=method, encoding=encoding, out=out)
  File "build\bdist.win32\egg\genshi\output.py", line 58, in encode
    for chunk in iterator:
  File "build\bdist.win32\egg\genshi\output.py", line 339, in __call__
    for kind, data, pos in stream:
  File "build\bdist.win32\egg\genshi\output.py", line 826, in __call__
    for kind, data, pos in stream:
  File "build\bdist.win32\egg\genshi\output.py", line 670, in __call__
    for kind, data, pos in stream:
  File "build\bdist.win32\egg\genshi\output.py", line 771, in __call__
    for kind, data, pos in chain(stream, [(None, None, None)]):
  File "build\bdist.win32\egg\genshi\output.py", line 586, in __call__
    for ev in stream:
  File "build\bdist.win32\egg\genshi\core.py", line 288, in _ensure
    for event in stream:
  File "build\bdist.win32\egg\genshi\core.py", line 288, in _ensure
    for event in stream:
  File "build\bdist.win32\egg\trac\web\chrome.py", line 968, in _strip_accesskeys
    for kind, data, pos in stream:
  File "build\bdist.win32\egg\genshi\core.py", line 288, in _ensure
    for event in stream:
  File "build\bdist.win32\egg\trac\web\chrome.py", line 957, in _generate
    for kind, data, pos in stream:
  File "build\bdist.win32\egg\genshi\template\base.py", line 592, in _include
    for event in stream:
  File "build\bdist.win32\egg\genshi\template\markup.py", line 381, in _match
    ctxt, start=idx + 1, **vars):
  File "build\bdist.win32\egg\genshi\template\markup.py", line 381, in _match
    ctxt, start=idx + 1, **vars):
  File "build\bdist.win32\egg\genshi\template\markup.py", line 330, in _match
    for event in stream:
  File "build\bdist.win32\egg\genshi\template\base.py", line 532, in _flatten
    for kind, data, pos in stream:
  File "build\bdist.win32\egg\genshi\core.py", line 288, in _ensure
    for event in stream:
  File "build\bdist.win32\egg\genshi\path.py", line 588, in _generate
    subevent = next()
  File "build\bdist.win32\egg\genshi\template\base.py", line 592, in _include
    for event in stream:
  File "build\bdist.win32\egg\genshi\template\markup.py", line 319, in _strip
    event = next()
  File "build\bdist.win32\egg\genshi\template\base.py", line 532, in _flatten
    for kind, data, pos in stream:
  File "build\bdist.win32\egg\genshi\core.py", line 288, in _ensure
    for event in stream:
  File "build\bdist.win32\egg\genshi\path.py", line 588, in _generate
    subevent = next()
  File "build\bdist.win32\egg\genshi\template\base.py", line 592, in _include
    for event in stream:
  File "build\bdist.win32\egg\genshi\template\markup.py", line 319, in _strip
    event = next()
  File "build\bdist.win32\egg\genshi\template\base.py", line 552, in _flatten
    result = _eval_expr(data, ctxt, vars)
  File "build\bdist.win32\egg\genshi\template\base.py", line 277, in _eval_expr
    retval = expr.evaluate(ctxt)
  File "build\bdist.win32\egg\genshi\template\eval.py", line 178, in evaluate
    return eval(self.code, _globals, {'__data__': data})
  File "c:/sites/projects/share/trac/egg-cache\trac-0.12dev_r9476-py2.6.egg-tmp\trac\ticket\templates\ticket_change.html", line 61, in <Expression u'wiki_to_html(context, change.comment, escape_newlines=preserve_newlines)'>
    <py:otherwise>${wiki_to_html(context, change.comment, escape_newlines=preserve_newlines)}</py:otherwise>
  File "build\bdist.win32\egg\trac\wiki\formatter.py", line 1490, in format_to_html
    return HtmlFormatter(env, context, wikidom).generate(escape_newlines)
  File "build\bdist.win32\egg\trac\wiki\formatter.py", line 1445, in generate
    escape_newlines)
  File "build\bdist.win32\egg\trac\wiki\formatter.py", line 1226, in format
    result = re.sub(self.wikiparser.rules, self.replace, line)
  File "C:\Python26\Lib\re.py", line 151, in sub
    return _compile(pattern, 0).sub(repl, string, count)
  File "build\bdist.win32\egg\trac\wiki\formatter.py", line 1149, in replace
    replacement = self.handle_match(fullmatch)
  File "build\bdist.win32\egg\trac\wiki\formatter.py", line 1142, in handle_match
    return external_handler(self, match, fullmatch)
  File "build\bdist.win32\egg\trac\versioncontrol\web_ui\log.py", line 330, in <lambda>
    lambda x, y, z: self._format_link(x, 'log1', y[1:-1], y, z))
  File "build\bdist.win32\egg\trac\versioncontrol\web_ui\log.py", line 377, in _format_link
    if revranges:
OverflowError: long int too large to convert to int

[gbarberi]

Something else I noticed. It appears the revision came out to be "569251789268" while usually a revision will have letters such as "6de7f54ca23f."

What I'm guessing is that the formatter is looking at the value and thinking it's an integer while in reality it should be a string.

[Remy Blank]

That's highly suspicious indeed. I'll try to reproduce the issue. Could you please provide the exact revision hash for which this issue happened (taken directly from Mercurial)? Thanks.

[gbarberi]

hg log -r 92
changeset: 92:569251789268
branch: xxxxxxxxx
user: xxxxxxxxx
date: Wed Apr 14 21:10:39 2010 -0400
summary: Test 2. Refs #109.

The following entry was in the ticket_change table that caused the error:

In [92:569251789268/gameclient]:
{{{
#!CommitTicketReference repository="gameclient" revision="92:569251789268"
Test 2. Refs #109.
}}}

I was able to change the first line to (notice I replaced the last digit with an "a"):

In [92:56925178926a/gameclient]:

and the problem went away.

[Remy Blank]

Funny, it seems that all-numerical hashes make trouble.

Also, I'm not sure if it's a good idea that the ticket updater component includes the revision number. It should probably rather use [569251789268/gameclient] as a revision link.

[Remy Blank]

Heh, actually that's exactly the problem: [92:569251789268] is interpreted as a revision range, not a single revision. This must be fixed in the commit updater.

[Remy Blank]

This is tricky: the value contained in changeset.rev in the case of TracMercurial is dependent on the node_format and show_rev settings. Using normalize_rev() on that doesn't change anything, as it also depends on those settings.

So there are two issues:

• I don't know how I'm supposed to get the full revision hash for a changeset, to be used in the revision= argument of the CommitTicketReference macro.
• The "composite" {num}:{hash} revisions as generated by TracMercurial are ambiguous when put between [], and can be confused with revision ranges in the case of all-numerical hashes. For reference, the ​th:GitPlugin doesn't use the revision numbers and only ever displays hashes.

Thoughts?

[Christian Boos]

Maybe we could use shortrev here, or introduce yet another method (canonicalrev?), with a fallback implementation that would work for Subversion and Git? (Git has no concept of repository local revision numbers, btw).

[Remy Blank]

Not sure I understand:

• normalize_rev() would return a "readable" revision number (for human consumption), including the local revision number for Mercurial (if configured).
• canonicalrev() would return the full hash or revision number.

Or did you mean to say the reverse?

What I'm worried about is that normalize_rev() is used throughout Trac for both making "readable" revisions and for making internal revisions for e.g. storing in the cache. So how about the following:

• normalize_rev() always returns the full hash.
• readable_rev() returns a (configurable) readable revision for display.

The default for readable_rev() would be to simply call normalize_rev().

[Christian Boos]

Replying to rblank:

Not sure I understand:

• normalize_rev() would return a "readable" revision number (for human consumption), including the local revision number for Mercurial (if configured).
• canonicalrev() would return the full hash or revision number.

Yes, that was the idea, as at first I didn't want to change the way normalize_rev was currently used.

What I'm worried about is that normalize_rev() is used throughout Trac for both making "readable" revisions and for making internal revisions for e.g. storing in the cache.

If you see a way out, for cleaning-up the current API without breaking things to much, I'm all ears :-)

So how about the following:

• normalize_rev() always returns the full hash.
• readable_rev() returns a (configurable) readable revision for display.

The default for readable_rev() would be to simply call normalize_rev().

This could work, yes. Too bad for the naming convention that I picked "shortrev" instead of "short_rev" (or we could fix that as well, for the sake of consistency?)

[Remy Blank]

Replying to cboos:

This could work, yes. Too bad for the naming convention that I picked "shortrev" instead of "short_rev" (or we could fix that as well, for the sake of consistency?)

It's already short_rev() in api.py :-)

But wait, if we already have short_rev(), we don't need another readable_rev(), do we? We can make normalize_rev() always return the complete hash (which shouldn't break anything, as the full hash can always be used to get a changeset), and short_rev() return a configurable short revision for display.

So in the end, maybe this should be fixed in TracMercurial (with maybe adding a few short_rev() calls in core for display)?

[Christian Boos]

Replying to rblank:

Replying to cboos:

This could work, yes. Too bad for the naming convention that I picked "shortrev" instead of "short_rev" (or we could fix that as well, for the sake of consistency?)

It's already short_rev() in api.py :-)

Ah, "bien Christian !" ;-)

But wait, if we already have short_rev(), we don't need another readable_rev(), do we?

I think we still need it, short_rev is used in place we're really tight of space, typically diff column headings. So in Mercurial we always use the local revision number, in Git they might use a very short hash prefix, etc.

We can make normalize_rev() always return the complete hash (which shouldn't break anything, as the full hash can always be used to get a changeset),

Other suggestions: long_rev for symmetry with short_rev, display_rev for consistency with hg_display in TracMercurial… but readable_rev is also OK for me.

[Remy Blank]

Introduce and use Repository.display_rev().

[Remy Blank]

Move configurable rev display to display_rev().

[Remy Blank]

9230-display-rev-r9490.patch​ introduces Repository.display_rev() and uses it every time a revision must be displayed. Internally, and in URLs as well, full revision hashes are used consistently. It also replaces a few occurrences of r%(rev)s with [%(rev)s], as the former looks quite ugly when rev is a hash.

9230-display-rev-hg-r9248.patch​ modifies mercurial-plugin so that normalize_rev() always returns a full hash, and full hashes are used consistently internally. It also implements display_rev() to shorten the hashes according to the configured preferences.

Thoughts?

[Christian Boos]

I like the patches, let me find some time to test them.

[Christian Boos]

Works fine for me, please apply.

[Remy Blank]

Patches applied in [9559] and [9560]. Documentation update follows later today.

The first changeset resulted in a few message changes. Should I commit a new extraction? I have the French translation fixed already, so I could commit that, too.

[Christian Boos]

I wonder if some of the changes could then be applied on all the translations, like s/new_rev/new_drev/?

Anyway, it looks like we're going to have to shift 0.12b1 a bit more, see follow-up on TracDev/ToDo.

[Remy Blank]

Replying to cboos:

I wonder if some of the changes could then be applied on all the translations, like s/new_rev/new_drev/?

Actually, those didn't change, I have kept new_rev. There were two missing markers, and some changes in how to display the revision (r%(rev)s -> [%(rev)s]).

Anyway, it looks like we're going to have to shift 0.12b1 a bit more, see follow-up on TracDev/ToDo.

Ok, so I committed the new extraction and the update to the French translation in [9562].

[Remy Blank]

Change documented in TracDev/ApiChanges/0.12@31.