Edgewall Software
Modify

Opened 14 years ago

Closed 14 years ago

#9230 closed defect (fixed)

OverflowError when viewing ticket

Reported by: gbarberi Owned by: Remy Blank
Priority: normal Milestone: 0.12
Component: ticket system Version: 0.12dev
Severity: major Keywords:
Cc: Christian Boos Branch:
Release Notes:
API Changes:
Internal Changes:

Description

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

Attachments (2)

9230-display-rev-r9490.patch (25.5 KB ) - added by Remy Blank 14 years ago.
Introduce and use Repository.display_rev().
9230-display-rev-hg-r9248.patch (6.3 KB ) - added by Remy Blank 14 years ago.
Move configurable rev display to display_rev().

Download all attachments as: .zip

Change History (20)

comment:1 by gbarberi, 14 years ago

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.

comment:2 by Remy Blank, 14 years ago

Owner: set to 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.

comment:3 by gbarberi, 14 years ago

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.

comment:4 by Remy Blank, 14 years ago

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.

comment:5 by Remy Blank, 14 years ago

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.

comment:6 by Remy Blank, 14 years ago

Cc: Christian Boos added

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?

comment:7 by Christian Boos, 14 years ago

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).

comment:8 by Remy Blank, 14 years ago

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().

in reply to:  8 ; comment:9 by Christian Boos, 14 years ago

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?)

in reply to:  9 ; comment:10 by Remy Blank, 14 years ago

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)?

in reply to:  10 comment:11 by Christian Boos, 14 years ago

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.

by Remy Blank, 14 years ago

Introduce and use Repository.display_rev().

by Remy Blank, 14 years ago

Move configurable rev display to display_rev().

comment:12 by Remy Blank, 14 years ago

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?

comment:13 by Christian Boos, 14 years ago

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

comment:14 by Christian Boos, 14 years ago

Works fine for me, please apply.

comment:15 by Remy Blank, 14 years ago

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.

comment:16 by Christian Boos, 14 years ago

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.

in reply to:  16 comment:17 by Remy Blank, 14 years ago

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].

comment:18 by Remy Blank, 14 years ago

Resolution: fixed
Status: newclosed

Change documented in TracDev/ApiChanges/0.12@31.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Remy Blank.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Remy Blank to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.