Strange code -- needs cleanup

[Toshio Kuratomi]

This code has been in the trac-bzr codebase for a long while:

Line 237:
         revid = urllib.unquote(rev)
         if revid in ('current:', 'null:'):
             return None, revid
        return None, revid
         if self.repo.has_revision(revid):
             return None, revid

        # Unsupported format.
        raise versioncontrol.NoSuchChangeset(rev)

Either this code should always return None, revid in which case, it should just look like this:

    revid = urllib.unquote(rev)
    return None, revid

Or the code should go through the whole sequence and raise the NoSuchChangeset exception if nothing else is found.

I talked to abentley a long time ago and he thought the latter was the correct case but he wasn't sure why the return was there in the first place. Attaching a patch to remove the return.

[Toshio Kuratomi (toshio)]

With bzr-1.7 this appears to cause a traceback (whereas it worked with bzr-1.3). However, the Exception is the same as:

  https://bugs.launchpad.net/trac-bzr/+bug/267700

so I think that we may be running into something that was deprecated and removed.

Traceback (most recent call last):
  File "/usr/lib/python2.5/site-packages/trac/web/main.py", line 406, in dispatch_request
    dispatcher.dispatch(req)
  File "/usr/lib/python2.5/site-packages/trac/web/main.py", line 237, in dispatch
    resp = chosen_handler.process_request(req)
  File "/usr/lib/python2.5/site-packages/trac/versioncontrol/web_ui/browser.py", line 148, in process_request
    self._render_directory(req, repos, node, rev)
  File "/usr/lib/python2.5/site-packages/trac/versioncontrol/web_ui/browser.py", line 173, in _render_directory
    changes = get_changes(self.env, repos, [i['rev'] for i in info])
  File "/usr/lib/python2.5/site-packages/trac/versioncontrol/web_ui/util.py", line 37, in get_changes
    changeset = repos.get_changeset(rev)
  File "/usr/lib/python2.5/site-packages/tracbzr/backend.py", line 312, in get_changeset
    branch, revid = self._parse_rev(rev)
  File "/usr/lib/python2.5/site-packages/tracbzr/backend.py", line 240, in _parse_rev
    if self.repo.has_revision(revid):
AttributeError: 'BzrRepository' object has no attribute 'repo'

[Toshio Kuratomi (toshio)]

Okay, here's a patch that fixes this issue including the traceback. I've also attached it to bug #267700 as I think it should fix that as well.

[Martin von Gagern (gagern)]

There is some more strange code in _parse_rev, causing a bunch of unreachable statements. The return Toshio mentioned is one of them, but here are two more, adding confusion.

            if len(split) != 2:
                raise versioncontrol.NoSuchChangeset(rev)
            ...
            if len(split) == 2:
                ...
            else:
                revid = branch.last_revision()

As the raise leaves the function when len(split) != 2, the else case will never happen. Maybe a check for empty string for one of the components was intended instead.

                        revid = cache.revid_from_dotted(rev_rev)
                        if revid is None:
                            raise repr(dotted)
                            revid = urllib.unquote(rev_rev)

I guess that the unquoted revid is intended as a kind of fallback: a string that looks like a dotted revno but is in fact a revid. In this case, The raise seems like a leftover from some debugging, especially as it raises a string and no proper exception object.
Btw: http://bazaar.launchpad.net/~trac-bzr-team/trac-bzr/trunk/revision/28.1.104 adds the last two lines in a single commit.

[Martin von Gagern (gagern)]

While writing a fix for this whole issue here, I had another look at the code and realized there might be a good reason for the return. It disables the inventory check in the (now unused and defunct) code following it. This is consistent with the case of "branch,revid", where the existence of the revid in question isn't checked, either.

When I tried to test this in practice, I found that I hadn't fully understood the intended semantics for revids without a branch. I assume they are somewhat unclear in the code as well. I filed bug #486795 about this. Until that gets addressed, I fear that the presence or absence of the return won't change much, except for the point where the exception gets raised and perhaps its error message.

[Martin von Gagern (gagern)]

Just committed a major rewrite of that _parse_rev function:
http://bazaar.launchpad.net/~trac-bzr-team/trac-bzr/trunk/revision/76