Changeset URL to contain the revision in the query string.

[Peter Dimov <peter.dimov@…>]

I’m using bzr as version control backend provided by the trac+bzr plugin. Since each branch has its own revision numbers, the revisions as seen by trac have the format:

path/to/branch,revno

ChangesetModule.match_request will match 'path' to the revision and 'to/branch,revno' as the path. trac+bzr works around this problem by URL escaping the revision string but the result is highly unreadable and not at all user-friendly.

Attached is a very simple patch I’m using to make trac gracefully handle the above scenario. I have added a configuration switch for the new format because I expect some people would like to keep the old URLs valid.

The solution is simply to change the changeset URLs from:

/changeset/revision/path

to:

/changeset/path?new=revision

The latter is also totally consistent with URLs of log and browser:

/log/path?rev=old_rev&stop_rev=new_rev
/browser/path?rev=revision

The patch also allows a new changeset notation:

changeset:path@revision

Clearly the changeset:revision/path notation cannot be used when revision can contain a slash.

[cboos]

Replying to Peter Dimov <peter.dimov@calitko.org>:

I’m using bzr as version control backend provided by the trac+bzr plugin. Since each branch has its own revision numbers, the revisions as seen by trac have the format: path/to/branch,revno

Hi Peter,

I'm not really familiar with bzr specifics, so I very much appreciate that you bring those additional requirements on the table.

The solution is simply to change the changeset URLs from: /changeset/revision/path to: /changeset/path?new=revision The latter is also totally consistent with URLs of log and browser:

Ok, I think this makes sense. I originally chose the changeset/rev/path format in an attempt to get URLs looking more user friendly than the log and browser ones. I think there will be no big impact in switching to the URL format you proposed (while staying backward compatible, of course).

The patch also allows a new changeset notation: changeset:path@revision Clearly the changeset:revision/path notation cannot be used when revision can contain a slash.

Well, you not only have that one, but also the r<rev>/<path> one, the [<rev>/<path>] one, the [<rev>:<rev2>/<path>] etc. So the problem is wider.

When you say bzr needs to have the branch path in the rev, doesn't the optional path restriction also need to be on that branch?

i.e. if you have rev=/stable/0.10,1232 and want to restrict to 'src', what do you write?

1. /stable/0.10/src@/stable/0.10,1232
2. src@/stable/0.10,1232

If it can be 1. and trac+bzr could determine where a branch stops, then we could simply use the normal syntax and write e.g. [1232/stable/0.10/src].

[Peter Dimov <peter.dimov@…>]

Replying to cboos:

Clearly the changeset:revision/path notation cannot be used when revision can contain a slash.

Well, you not only have that one, but also the r<rev>/<path> one, the [<rev>/<path>] one, the [<rev>:<rev2>/<path>] etc. So the problem is wider.

Yes, I knew about these but didn't touch them because of backwards compatibility. Actually these notations cannot be used with trac+bzr as it is now. I thought that if revisions have slashes, then changeset:path@revision should be used instead. I guess I could also add support for [path@revision] by patching get_wiki_syntax().

When you say bzr needs to have the branch path in the rev, doesn't the optional path restriction also need to be on that branch? i.e. if you have rev=/stable/0.10,1232 and want to restrict to 'src', what do you write? 1. /stable/0.10/src@/stable/0.10,1232 2. src@/stable/0.10,1232 If it can be 1. and trac+bzr could determine where a branch stops, then we could simply use the normal syntax and write e.g. [1232/stable/0.10/src].

Currently it is 1. and you are totally right that the normal syntax could also be used if trac+bzr was able to determine the branch base path. I was also thinking about this but the problem is that, for example, Repository.get_changeset() takes a string rev as parameter. trac+bzr needs both the path and the revno. What it would get in the case you suggest is only the revno and that is not enough. I already have some ideas how to solve that but they involve some refactoring in the api. I should better elaborate them in a separate ticket or in trac-dev, right?

[Peter Dimov <peter.dimov@…>]

Old version of the patch actually broke the parsing of [<revision>/<path>]. Fixed now.

[Peter Dimov <peter.dimov@…>]

This patches also the changeset entries in the timeline to support revision_in_querystring.

[cboos]

And what about using the double slash as a separator, if there must be a slash in the revision?

e.g. changeset:1232/stable/0.10//src ?

We would parse such a link the normal way, but there would be an extra step: we look into the path for "//" and if found, we append what's before to the revision (stable/0.10 - the branch part) and only keep what's after as the path (src).

If think that with this approach, we could keep the existing regexps and simply have the simple post-processing step describe above.

And when we're about to generate a changeset link, we would take an extra step: checking if '/' is present in the revision, and if yes, add an extra '/' between the revision and the path, e.g.

    href = formatter.href.changeset(revision, (None, '')['/' in revision], path)

This could be even simplified further using the Context API, as the logic above could be written in ChangesetContext.resource_href, instead of in every href.changeset(...) call.

[Peter Dimov <peter.dimov@…>]

Replying to cboos:

And what about using the double slash as a separator, if there must be a slash in the revision? e.g. changeset:1232/stable/0.10//src ? ... And when we're about to generate a changeset link, we would take an extra step: checking if '/' is present in the revision, and if yes, add an extra '/' between the revision and the path, e.g.

I think that would work! Revsions returned from the backend must therefore have the format:

[revno/path/to/branch//]
/changeset/revno/path/to/branch//

The above would display all changes in revno for branch. I don't like that the double slash in required in that case.

The log would create URLs like:

[revno/path/to/branch//path/to/branch/src/file.cpp]

What you suggested is nicer:

[revno/path/to/branch//src/file.cpp]

but it'll require additional pre and post processing.

I think the best solution would be not to change anything in changeset URL and Wiki format but instead pass both the path and the revno to the version control backend. Maybe let's wait and see what ideas we get from the trac-dev discussion on version control refactoring.

[cboos]

Not for 0.11.