Edgewall Software
Modify

Opened 18 years ago

Last modified 9 years ago

#4612 new enhancement

Changeset URL to contain the revision in the query string.

Reported by: Peter Dimov <peter.dimov@…> Owned by:
Priority: normal Milestone: next-major-releases
Component: version control Version: 0.10.3
Severity: normal Keywords: revision patch
Cc: Branch:
Release Notes:
API Changes:
Internal Changes:

Description

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.

Attachments (3)

trac-0.10.3-changeset.revision_in_querystring.patch (3.4 KB ) - added by Peter Dimov <peter.dimov@…> 18 years ago.
trac-0.10.3-changeset.revision_in_querystring.2.patch (3.3 KB ) - added by Peter Dimov <peter.dimov@…> 18 years ago.
Old version of the patch actually broke the parsing of [<revision>/<path>]. Fixed now.
trac-0.10.3-changeset.revision_in_querystring.3.patch (4.5 KB ) - added by Peter Dimov <peter.dimov@…> 18 years ago.
This patches also the changeset entries in the timeline to support revision_in_querystring.

Download all attachments as: .zip

Change History (10)

by Peter Dimov <peter.dimov@…>, 18 years ago

in reply to:  description ; comment:1 by Christian Boos, 18 years ago

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

in reply to:  1 comment:2 by Peter Dimov <peter.dimov@…>, 18 years ago

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?

by Peter Dimov <peter.dimov@…>, 18 years ago

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

by Peter Dimov <peter.dimov@…>, 18 years ago

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

comment:3 by Christian Boos, 18 years ago

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.

in reply to:  3 comment:4 by Peter Dimov <peter.dimov@…>, 18 years ago

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.

comment:5 by Christian Boos, 18 years ago

Keywords: revision added
Milestone: 0.110.12

Not for 0.11.

comment:6 by Ryan J Ollos, 10 years ago

Owner: Christian Boos removed

comment:7 by figaro, 9 years ago

Keywords: patch added

Modify Ticket

Change Properties
Set your email in Preferences
Action
as new The ticket will remain with no owner.
The ticket will be disowned.
as The resolution will be set. Next status will be 'closed'.
The owner will be changed from (none) to anonymous. Next status will be 'assigned'.

Add Comment


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