Latest revision number not written correctly to hdf

[bjorns@…]

It seems that when browsing the latest revision of the repository, the browser.revision attribute is not set. This results in the "View revision" input box being empty and, more importantly, the "View changes" button yielding an error.

Could it be that line 115 in trunk/trac/versioncontrol/web_ui/browser.py​ should read:

            'revision': rev_or_latest,

but not

            'revision': rev,

[Christian Boos]

When browsing the latest revision of the repository, the "View revision" input box is left empty on purpose, so that you keep browsing the latest revision, even if new revisions are created while you're browsing.

But the View changes button shouldn't trigger an error…

[Christian Boos]

… and for me, it doesn't.

Can you be more specific on how to trigger the error?

[anonymous]

I apologize, you are right. I assumed that the revision number should be shown in the "view revision" field since this is the behavior of Trac 0.9.5. I now see this is not the case (i.e. on trac.edgewall.com) and I assume this has been changed ? (( I don't see why, though, I found it quite informative. Even though the number is shown in the "view revision" you still follow the youngest branch as the "rev" argument is not included in the HTTP request unless you specifically put in a number and press enter. ))

I also found that the error wasn't tied to the revision number not being shown (although changing the HDF as shown in the Ticket description fixed the problem). The real problem is in versioncontrol/api.py - or p4trac.py, dependent on how you look at it.

Problem:

• Go to BROWSER
• Browse to you favorite path
• Click "View Changes"

This submits you to /trac/anydiff?new_path=xxx&old_path=xxx&new_rev=&old_rev=. The problem is that "new_rev" and "old_rev" are defined as zero length strings of type Unicode, but not as type None. The error is triggered in p4trac.py's implementation of normalize_rev when rev is casted to an integer.

I first though this was an error in p4trac.py but as it turns out, p4trac.py is according to Versioncontrol's API​ (see line 197):

def normalize_rev(self, rev):
    """
    Return a canonical representation of a revision in the repos.
    'None' is a valid revision value and represents the youngest revision.
    """

According to this, normalize_rev should be called with either a number (in a Unicode String that can be casted with int()) or None. The reason why this works with SVN is that versioncontrol/svn_fs.py catches the exception and set's rev=None if the casting failed. That is, does more extra checks then the API states should be necessary.

Not sure you agree, but I'd say that either normalize_rev shouldn't be called with u or that the API's documentation should be updated to state that both None and u are valid revision values.

[Christian Boos]

Maybe the documentation string is not clear enough about that, but the intent of this method is really to normalize the given "rev" argument, which comes from the user and therefore can be anything, to a value suitable for use by the backend. So it's up to the backend to decide how user input translate to revisions, the only "constraint" being that None should be translated to the latest revision.

The svn backend interprets a number to be a revision number (easy), but also "head" to be an alias to the latest revision (Ok, it will actually accept anything but a number as an alias to the latest revision ;) ). But the mercurial backend for example checks also whether the given input against the known tags and branch names.

A better doc would perhaps be:

def normalize_rev(self, rev):
    """Return a canonical representation of a revision.

    It's up to the backend to decide what string values of `rev` 
    (usually provided by the user) should be accepted, and how they 
    should be normalized. 

    In addition, if `rev` is `None`, the youngest revision should be returned.
    """

[Christian Boos]

Replying to cboos:

… checks also whether the given input against the known tags and …

sorry for my awful syntax…

[bjorns@…]

I agree that the backend should be able to implement what ever string→rev translation he pleases. As I haven't used anything but revision numbers in Trac I wasn't even aware of this being done ;)

Anyways, I'm +1 on the documentation change. How about (basically adding your explanations and one extra comment):

def normalize_rev(self, rev):
    """Return a canonical representation of a revision.

    It's up to the backend to decide which string values of `rev` 
    (usually provided by the user) should be accepted, and how they 
    should be normalized. Some backends may for instance want to match
    against known tags or branch names.

    In addition, if `rev` is `None` or '' (a zero length Unicode string),
    the youngest revision should be returned.
    """

The reason for me adding the "or ''" comment is that this is (if I'm not mistaken) a requirement from Trac (see process_request, line 781 in changeset.py​). If trac calls normalize_rev with an empty string it expects to get the latest revision, right ?

[Christian Boos]

Ok, things seems to be more robust the way you suggest, although I now see why you called it a bug ;)

[Christian Boos]

API doc for normalize_rev was improved in r3614.

Anyway, the method itself will probably go away in 0.12 in favor of a dedicated Revision class.