Add support for svn:externals "1.5" style

[Emmanuel Blot]

For now, Trac only supports the old-style syntax.

Subversion 1.5 URLs have the following special prefixes:

../

Relative to the URL of the directory on which the svn:externals property is set

^/

Relative to the root of the repository in which the svn:externals property is versioned

Relative to the scheme of the URL of the directory on which the svn:externals property is set

/

Relative to the root URL of the server on which the svn:externals property is versioned

Also, there's a new convention for svn:externals, instead of:

RELPATH [-r<rev>] URL

the mapping can now be written:

[-r<rev>] URL RELPATH

(taken from ​http://svnbook.red-bean.com/en/1.5/svn.advanced.externals.html)

[Emmanuel Blot]

Note: It's not only about the prefixes, it's also about the parameter order in the external info line. SVN book quoting is incomplete ;-)

[Christian Boos]

Oh brilliant, I missed that part… If I'm not mistaken, looks like we have to check for one of "-.^/" as the starting character, in order to detect the new mode.

[Emmanuel Blot]

Preliminary version

[Emmanuel Blot]

I've attached a patch for peer reviewing.

The patch is not yet complete, however the 4 new svn:externals syntaxes are supported:

1. relative to repository root (local external)
2. relative to direcotry (local external)
3. relative to server
4. relative to scheme

Things to do:

1. Validate error handling when property syntax is invalid
2. Better rendering

I'd like to discuss the property rendering: I think it would be better, from the user experience perspective, that svn:externals appears as special directory entries rather than properties. I'd also like to move property formatting from Python code up (svn_fs.py) to the Genshi template, to ease customization/rendering.

svn:external test cases:

/repos/branches/project-1.x/config           newstyle-server-norev
/repos/branches/project-1.x/config@3274      newstyle-server-at-3274
-r 3274 /repos/branches/project-1.x/config   newstyle-server-r3274

^/branches/project-1.x/config              newstyle-repos-norev
^/branches/project-1.x/config@3274         newstyle-repos-at-3274
-r 3274 ^/branches/project-1.x/config      newstyle-repos-r-3274

///Users/eblot/Sources/Svn/example.com/repos/repos/branches/project-1.x/config          newstyle-scheme-norev
///Users/eblot/Sources/Svn/example.com/repos/repos/branches/project-1.x/config@3274     newstyle-scheme-at-3274
-r 3274 ///Users/eblot/Sources/Svn/example.com/repos/repos/branches/project-1.x/config  newstyle-scheme-r-3274

../../branches/project-1.x/config              newstyle-relative-norev
../../branches/project-1.x/config@3274         newstyle-relative-at-3274
-r 3274 ../../branches/project-1.x/config      newstyle-relative-r-3274

oldstyle-server-norev          file:///Users/eblot/Sources/Svn/example.com/repos/repos/branches/project-1.x/config
oldstyle-server-r-3274 -r 3274 file:///Users/eblot/Sources/Svn/example.com/repos/repos/branches/project-1.x/config 

[Emmanuel Blot]

Note:

I've added a new configuration section to handle non-local, server-relative external definitions:

[svn:url]
1 = / http://svn.example.com/

In the above example, '/' match the server root

[Christian Boos]

Replying to eblot:

I've added a new configuration section to handle non-local, server-relative external definitions:

[svn:url]
1 = / http://svn.example.com/

But why not also use [svn:externals] for that?

[Remy Blank]

Replying to eblot:

I'd also like to move property formatting from Python code up (svn_fs.py) to the Genshi template, to ease customization/rendering.

I had the same thought today while implementing the svn:mergeinfo rendering. Using lots of nested tag.x() calls doesn't really make the code very clear.

[Christian Boos]

Replying to rblank:

Replying to eblot:

I'd also like to move property formatting from Python code up (svn_fs.py) to the Genshi template, to ease customization/rendering.

I had the same thought today while implementing the svn:mergeinfo rendering. Using lots of nested tag.x() calls doesn't really make the code very clear.

If you don't like the builder approach, it's also possible to use "inline" templates and call generate() directly on them (see ​genshi:wiki:Documentation/templates.html#python-api).

The browser templates (and even worse, the diff_div.html one) are quite generic and shouldn't know about svn related details…

[anonymous]

Replying to cboos:

But why not also use [svn:externals] for that?

Different semantic:

1. svn:externals map SVN URL to Trac URL
2. svn:url map SVN path to SVN URL

I'm not sure merging both semantics into a single definition is useful

Moreover, "local" externals are converted from SVN path to SVN URL using svn:url, then from SVN URL to Trac URL using svn:externals

[anonymous]

Replying to cboos:

Replying to rblank:

Replying to eblot: I had the same thought today while implementing the svn:mergeinfo rendering. Using lots of nested tag.x() calls doesn't really make the code very clear.

It is not much about the syntax itself, but where data are converted into rendered items. I'm not convinced that conversion from properties to sentences such as "path at revision rev in URL" is something that should take place in hardcoded Python code.

The browser templates (and even worse, the diff_div.html one) are quite generic and shouldn't know about svn related details…

On the other side, "svn:externals" have a semantic that is closer to directory entries than meta data such as the mime type of a file. I'd really like the site admin to be offered a way to represent externals as special directory entries rather than "properties".
Any idea on how to do that is welcomed.

[Emmanuel Blot]

anonymous was me.

[Christian Boos]

Replying to anonymous:

Replying to cboos:

But why not also use [svn:externals] for that?

Different semantic:

1. svn:externals map SVN URL to Trac URL
2. svn:url map SVN path to SVN URL

I'm not sure merging both semantics into a single definition is useful

But is this nuance really necessary? All we need is mapping an SVN path to a Trac-like URL.

I'll take an example.

Consider hypothetical svn:externals set at ​http://svn.edgewall.com/repos/trac/deps pointing to other repositories on s.e.o:

/repos/genshi/branches/experimental/advanced-i18n   genshi
/repos/babel/trunk/                                 babel

Of course we'd like to point to ​http://genshi.edgewall.org in the first case, ​http://babel.edgewall.org in the second.

With your proposal, this would be:

[svn:url]
1 = /repos/genshi http://svn.edgewall.org/repos/genshi
2 = /repos/babel  http://svn.edgewall.org/repos/babel

[svn:externals]
1 = http://svn.edgewall.org/repos/genshi  http://genshi.edgewall.org/browser/$path?rev=$rev
2 = http://svn.edgewall.org/repos/babel   http://babel.edgewall.org/browser/$path?rev=$rev

(IIUC)

But the fact that we go through ​http://svn.edgewall.org is not really relevant, as we could get the same result via the much simpler:

[svn:externals]
1 = /repos/genshi http://genshi.edgewall.org/browser/$path?rev=$rev
2 = /repos/babel  http://babel.edgewall.org/browser/$path?rev=$rev

What did I miss?

Even if you use [svn:url] / http://svn.edgewall.org, you'd still need the two entries in [svn:externals].

In a similar way, the scheme relative stuff could be interpreted like this:

[svn:externals]
3 = //svn.edgewall.org/repos/genshi  //genshi.edgewall.org/browser/$path?rev=$rev
4 = //svn.edgewall.org/repos/babel  http://babel.edgewall.org/browser/$path?rev=$rev

with (3) resolving to ​http://genshi.edgewall.org/… or ​https://genshi.edgewall.org/… depending on the current scheme used in the context, and (4) resolving always to ​http://babel.edgewall.org/…

[Christian Boos]

Replying to anonymous:

Replying to cboos:

Replying to rblank:

Replying to eblot: I had the same thought today while implementing the svn:mergeinfo rendering. Using lots of nested tag.x() calls doesn't really make the code very clear.

It is not much about the syntax itself, but where data are converted into rendered items. I'm not convinced that conversion from properties to sentences such as "path at revision rev in URL" is something that should take place in hardcoded Python code.

Ah, but that's a remaining of pre-i18n, this should actually be handled by a _('%(path)s at revision %(rev)s', path=..., rev=...).

The browser templates (and even worse, the diff_div.html one) are quite generic and shouldn't know about svn related details…

On the other side, "svn:externals" have a semantic that is closer to directory entries than meta data such as the mime type of a file. I'd really like the site admin to be offered a way to represent externals as special directory entries rather than "properties".
Any idea on how to do that is welcomed.

And that's a "different topic" (well not really, but at least a different ticket), see #6474 (and perhaps also #6615).

[Emmanuel Blot]

Replying to cboos:

What did I miss?

There may be a Trac environment for a given URL path, but this is not mandatory.
If no Trac environment backs up a SVN URL, there can still be a mapping from a SVN path to a SVN URL.

Mixing both mapping will end up into something hard to understand, where the first argument may be a URL (svn < 1.5) or a path (svn ≥ 1.5), and the second argument a Trac URL or a SVN URL.

I'm ok to merge both definitions into a single section, although I'm not sure it'll be easier to understand

BTW, there's still room for improvement with the current [svn:externals] syntax: a "project" variable would be useful

http://svn.example.com/$project http://trac.example.com/trac/$project/browser/$path?rev=$rev

[Emmanuel Blot]

Replying to cboos:

Ah, but that's a remaining of pre-i18n, this should actually be handled by a _('%(path)s at revision %(rev)s', path=..., rev=...).

Sure, but it does not address the actual issue: it's impossible to customize without hacking into Trac code. What if I want (and I do want it) to show the externals a different way? The current representation is hard to read. I'd prefer to have the actual data (path, rev, url, …) as template parameters that any hardcoded string that I will have to parse (!) if I want to render the externals strings another way.

But you're right, this enhancement should be tracked with #6474 (I did not know about this ticket).

[Remy Blank]

Replying to cboos:

The browser templates (and even worse, the diff_div.html one) are quite generic and shouldn't know about svn related details…

I didn't intend to put svn-specific stuff in any of the generic templates. But what if render_property() could either return a string (or Markup) as now, or a (template, data) tuple, and the template would be <xi:include>ed with the associated data in a py:with?

[Emmanuel Blot]

New version of the patch: svn:url removed (use svn:externals instead), simpler implementation

[Emmanuel Blot]

I've attached a new version of the patch. [svn:url] has been removed

Let me know.

[Emmanuel Blot]

Replace hard-to-read external list with a cleaner table

[Emmanuel Blot]

Rendering example

[Emmanuel Blot]

Rendering example (I should not have used a space char in the file name…)

[nattgris]

I just got a little disappointed. I've been waiting for svn:externals support to creep in (with repository-relative roots), since it's the main thing stopping our trac deployment. We're using svn:externals to link/group libraries and sub-projects (hardware, software, documentation) into main projects. The trac environments are linked to the main projects in SVN, since they are the deliverables and what tickets will be reported against.

I was hoping that the browser would display any externals definitions as regular subdirectories that you can browse into, just like what you get in your file system when such a main project is checked out. But from the rendering examples above it looks like it's only a fancier display of what the svn:externals property is set to. And I've just convinced my boss that upgrading to the latest trac will solve the problems… :(

Did I understand it correctly or can trac already be setup like I want it? Otherwise I'll have to open a new ticket about it. It can't be too hard to add the feature, at least not for repository local svn:externals (which is all I'm interested in).

[Christian Boos]

Replying to nattgris:

… And I've just convinced my boss that upgrading to the latest trac will solve the problems… :(

So you just lied to your boss?!?! Baaad boy…

We're just working on the first phase here, if you want to help out you can install trunk, apply the patch and test against your repository. That would be already great.

The second phase (showing the svn:externals inline) is the topic of another ticket (#6474), as already mentioned elsewhere in this ticket. You could add there that you wish to see the actual content of the folders in case the svn:externals are local ones (and I don't even want to think about the implications this would have for svn_authz…).

[Itamar O <itamarost@…>]

Ported patch to svn_prop.py and extended to support multirepos concepts

[Itamar O <itamarost@…>]

I really liked the idea of having the svn:externals show up as inline directories, and I understand that the first phase is getting this part (rendering of the externals) working well.

So I started with the patch provided by elbot and hacked at it until it worked for me. The patch I attached (svn-externals-7687-multirepos.patch​) contains comments and documentation for what I did.

Summary:

• Some use cases didn't work at all…
• Improved handling of scoped repositories.
• MultiRepos & 0.12 era brings extra complications and opportunities:
  • Repository URL may be defined as part of the repository, so when it is, I use it to extend the externals map, and to allow traversing outside the scope of scoped repositories.
  • When leaving the scope of the current repository, it is still possible that the resulting URL is in the scope of another repository. I use it to generate links to internal browser between different repositories.

Comments:

• I'm afraid I had difficulty understanding the externals-map-matching-mechanism is the original patch. Instead I do something much simpler, but maybe I'm missing something…
• I'm on Windows, so the patch contains CRLF eol's. sorry for that.
• I have tested my patch against trunk@9482​, manually. Really wanted to add unittests, but I'm afraid that for a first-time-hack I had no luck figuring out how to write the tests (see post on the issue @ dev mailing list).

[Remy Blank]

I have started using svn:externals recently, so I'm interested in this as well. Thanks for picking this up.

[Christian Boos]

A few preliminary observations, as I didn't yet test the patch not even read all of it…

Replying to Itamar O <itamarost@…>:

… So I started with the patch provided by elbot and hacked at it until it worked for me.

Summary:

• Some use cases didn't work at all…

So was that before you started rework the patch or are there still open issues with the attachment you posted?

• Improved handling of scoped repositories.
• MultiRepos & 0.12 era brings extra complications and opportunities:
  • Repository URL may be defined as part of the repository, so when it is, I use it to extend the externals map, and to allow traversing outside the scope of scoped repositories.

Good!

• When leaving the scope of the current repository, it is still possible that the resulting URL is in the scope of another repository. I use it to generate links to internal browser between different repositories.

Sounds reasonable.

Comments:

• I'm afraid I had difficulty understanding the externals-map-matching-mechanism is the original patch. Instead I do something much simpler, but maybe I'm missing something…

That would be welcomed, that code expanded into something complex and a fresh look at it might indeed result in a simplification. In order to be sure you didn't miss something (and well, because of the lack of tests for that part of code…), you could look at the "blame" view of those lines, and unfold its history for seeing the various corner cases I had to address.

• I'm on Windows, so the patch contains CRLF eol's. sorry for that.

No worries.

• I have tested my patch against trunk@9482​, manually. Really wanted to add unittests, but I'm afraid that for a first-time-hack I had no luck figuring out how to write the tests (see post on the issue @ dev mailing list).

Not your fault either. Adding tests for svn:externals would be a bit complex, but certainly much needed. The simpler approach would be to write a few functional tests to check we have the expected links, after two repositories with adequate svn:externals (and that would provide at the same time the first tests for MultiRepos as well!). See source:trunk/trac/versioncontrol/tests/functional.py​.

Now the usual code style rant: please have a look at TracDev/CodingStyle, in particular the advice for the maximal line length. Also, while it's good to document the code, it just feel like there's too much information in the docstring. Maybe it would be worth moving some of it in the changeset commit message, or even in some place in the wiki?

All in all, a very good first contribution on a much wanted feature, please keep up the good work and stick with us refining the patch until it gets ready for inclusion!

[Itamar Ostricher]

Replying to cboos:

A few preliminary observations, as I didn't yet test the patch not even read all of it…

Replying to Itamar O <itamarost@…>:

… Summary:

• Some use cases didn't work at all…

So was that before you started rework the patch or are there still open issues with the attachment you posted?

That was before the rework. When posted, all of (my) use-cases work.

Comments:

• I'm afraid I had difficulty understanding the externals-map-matching-mechanism is the original patch. Instead I do something much simpler, but maybe I'm missing something…

That would be welcomed, that code expanded into something complex and a fresh look at it might indeed result in a simplification. In order to be sure you didn't miss something (and well, because of the lack of tests for that part of code…), you could look at the "blame" view of those lines, and unfold its history for seeing the various corner cases I had to address.

will look into it.

• I have tested my patch against trunk@9482​, manually. Really wanted to add unittests, but I'm afraid that for a first-time-hack I had no luck figuring out how to write the tests (see post on the issue @ dev mailing list).

Not your fault either. Adding tests for svn:externals would be a bit complex, but certainly much needed. The simpler approach would be to write a few functional tests to check we have the expected links, after two repositories with adequate svn:externals (and that would provide at the same time the first tests for MultiRepos as well!). See source:trunk/trac/versioncontrol/tests/functional.py​.

Writing functional tests is simpler?

I automatically thought about unittests, the main reason being that they currently all pass on my setup, as opposed to the functional tests, that I didn't manage to get passing…

Now the usual code style rant: please have a look at TracDev/CodingStyle, in particular the advice for the maximal line length. Also, while it's good to document the code, it just feel like there's too much information in the docstring. Maybe it would be worth moving some of it in the changeset commit message, or even in some place in the wiki?

Already started working on improving the style. Will post a follow-up.

In fact, the follow-up I'm working on will be more than improving style, since I started looking into #6474 and realized some refactoring is needed here in order to use versioncontrol.svn_prop svn:externals processing there.

Just wondering, as long as I contribute via patches (as opposed to commits), how should I communicate the changeset commit message to the developer that will actually do the commit? Using a ticket comment perhaps?

All in all, a very good first contribution on a much wanted feature, please keep up the good work and stick with us refining the patch until it gets ready for inclusion!

Thanks!

[Itamar Ostricher]

New & improved patch, now with unit tests!

[Itamar Ostricher]

Forgot to include the added test module in init in the previous patch

[Itamar Ostricher]

Posted an updated patch (svn-externals-7687-multirepos.3.patch​), including style improvements, as well as code refactor that should be useful also for other features (e.g. #6474).

In addition, the patch also adds trac.versioncontrol.tests.svn_prop with unittests for the reusable functions added (not yet functional tests though).

(Further) feedback is appreciated.

[Christian Boos]

This looks very good - worth delaying a bit more 0.12beta1 to tomorrow (hey, I'm always looking for excuses ;-) ).

Some quick "style" tips:

• you don't need \ line continuations when you're inside an (...) expression.
• try avoiding variable names like lvl1, as l and 1 are very close in shape, makes it less readable (level_1, level_one, toplevel …)
• you used u'dir' etc. and the like constants. Well, that's not necessary and hurts readability. Remy recently suggested that we switched to that style for performance reasons, but nothing is decided yet (so eventually you'll need to change that ;-) )

More review tomorrow, but your code is a pleasure to work with, it looks like we'll manage to make svn_prop understandable at last ;-) Keep up the good work!

[Itamar Ostricher]

Fixed bug introduced by previous patch (rendered no-links) + cboos feedback

[Itamar Ostricher]

Replying to cboos:

Some quick "style" tips:

• you don't need \ line continuations when you're inside an (...) expression.

Fixed in svn-externals-7687-multirepos.4.patch​.

• try avoiding variable names like lvl1, as l and 1 are very close in shape, makes it less readable (level_1, level_one, toplevel …)

Changed "lvl" to "phase".

More review tomorrow, but your code is a pleasure to work with, it looks like we'll manage to make svn_prop understandable at last ;-) Keep up the good work!

Thanks for the positive feedback! :-)

The patch also fixes a bug I introduced that caused external items with unmatched reference to be rendered with links to the browser root.

[Itamar Ostricher]

some more refactoring, some bug fixes, some more tests

[Itamar Ostricher]

Yet another patch update (svn-externals-7687.patch​), that includes several fixes found with the added integration tests.

Also refactored the code that builds the final href in a way that should be useful for #6474 (with an intermediate dictionary that contains the link constituents).

[Christian Boos]

I'd suggest one more simplification:

• remove parse_externals
• rename externals_generator to parse_externals
  • the former calls to parse_externals(...) become list(parse_externals(...))
  • you can also remove the assert(..., isinstance(list)) that follows
• fold parse_ext_line into parse_externals

[Itamar Ostricher]

removed duplicate code in externals-string parsing

[Itamar Ostricher]

Replying to cboos:

I'd suggest one more simplification:

…

Done in svn-externals-7687.2.patch​

[Christian Boos]

I'm trying to test it now. Two initial remarks:

• svn_props.py, line 118, there's a Python 2.5ism (ternary ... if ... else ...); use scope += scope and '/' or '' for now
• I have an [svn:externals] set like that:

  [svn:externals]
  local = svn://server/repository /trac/browser/$path?rev=$rev
  

  The intent is that if you have svn:external prop saying:

  tools	svn://server/repository/branches/maintenance/common/v710_v1210/tools		
  

  then following the tools link should go in the Trac instance below /trac/browser.
  Details:
  • svn://server/repository/branches/maintenance/common/v710_v1210/tools starts with svn://server/repository, so we pick this mapping,
  • removing that prefix from the prop line gives path == /branches/maintenance/common/v710_v1210/tools
  • hence tools should have href==/trac/browser//branches/maintenance/common/v710_v1210/tools?rev=... according to the selected mapping (/trac/browser/$path?rev=$rev)
  Instead, with the patch, tools links to href=svn://server/repository/branches/maintenance/common/v710_v1210/tools

[Itamar Ostricher]

Fixed issues pointed by Christian

[Itamar Ostricher]

Attached another version of the patch (svn-externals-7687.3.patch​) to address to issues pointed by Christian in comment:37, including extended unittests to reproduce and verify the second issue.

[Christian Boos]

Thanks! The example I gave in comment:37 now works for me. However, I was talking about the first link, tools in the example above. It's fine (even a feature ;-) ) if the other links to the non-mapped target (in the example above, clicking on the svn:// link would launch TortoiseSvn on Windows).

But besides this, there are 4 failures in the tests:

python trac/versioncontrol/tests/svn_prop.py
......................
----------------------------------------------------------------------
Ran 22 tests in 0.021s

OK
...............F.F.F.F
======================================================================
FAIL: test_parse_ini_externals_map (__main__.SvnExternalsMapTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "trac/versioncontrol/tests/svn_prop.py", line 1446, in test_parse_ini_externals_map
    self.assertEqual(value, ext_map[key])
AssertionError: 'SubProjA/%(path)s?rev=%(rev)s' != 'browser/SubProjA/%(path)s?rev=%(rev)s'

======================================================================
FAIL: test_externals_on_one_scoped_repository (__main__.SvnExternalsIntegrationTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "trac/versioncontrol/tests/svn_prop.py", line 1620, in test_externals_on_one_scoped_repository
    self.req_href, self.ext_map))
AssertionError: {'phase': 1, 'href': 'http://old-server:8000/svn/myrep/devel/common/trunk', 'rev': None} != {'phase': 1, 'href': '/browser/myrep/devel/common/trunk?rev=', 'rev': None}

======================================================================
FAIL: test_externals_on_several_scoped_crossing_repositories (__main__.SvnExternalsIntegrationTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "trac/versioncontrol/tests/svn_prop.py", line 1670, in test_externals_on_several_scoped_crossing_repositories
    self.req_href, self.ext_map))
AssertionError: {'phase': 1, 'href': 'http://old-server:8000/svn/myrep/devel/common/trunk', 'rev': None} != {'phase': 1, 'href': '/browser/myrep/devel/common/trunk?rev=', 'rev': None}

======================================================================
FAIL: test_externals_on_urlless_repository_without_root (__main__.SvnExternalsIntegrationTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "trac/versioncontrol/tests/svn_prop.py", line 1770, in test_externals_on_urlless_repository_without_root
    self.req_href, self.ext_map))
AssertionError: {'phase': 1, 'href': 'http://old-server:8000/svn/myrep/devel/common/trunk', 'rev': None} != {'phase': 1, 'href': '/browser/myrep/devel/common/trunk?rev=', 'rev': None}

----------------------------------------------------------------------
Ran 22 tests in 0.016s

FAILED (failures=4)

[Itamar Ostricher]

New patch (see comment:40)

[Itamar Ostricher]

Regarding the failed tests: in my file I have only 927 lines, so I wonder where you got all these other lines from :-) Anyway, it seems that the failures you report are exactly the tests I needed to change in svn-externals-7687.3.patch​.. is it possible that you updated svn-prop.py but not tests/svn-prop.py to the latest patch?

If I understand you correctly, your intention is that the first column will link to the intra-Trac browser location (when available), and the second column (which shows the original URL from the property) will link to that location?

I thought such a behavior would be confusing, because the first item (which is usually a short name) will be the useful link, and the second item (which is usually a long URL) will be the not-so-useful link. You get different behaviors for links in the same row, and the "bigger" link is the less meaningful one…

So I posted svn-externals-7687.4.patch​, which includes:

• some major fixes to bugs I found while working on the last comment (wrong links were generated to internal browser when environment not served via server root)
• changed "linking policy" to externals-table items according the comment:39 (first item to internal browser, second item to original URL, revision not a link)
• improved support when default repository exists (was handled incorrectly)

[Christian Boos]

Replying to itamaro:

Regarding the failed tests: in my file I have only 927 lines, so I wonder where you got all these other lines from :-) Anyway, it seems that the failures you report are exactly the tests I needed to change in svn-externals-7687.3.patch​.. is it possible that you updated svn-prop.py but not tests/svn-prop.py to the latest patch?

You're right, I did a svn revert -R, but of course that didn't help in this situation as the file was not yet under version control… so the tests/svn_prop.py from the second patch was added to the the file already present from the first :-)

If I understand you correctly, your intention is that the first column will link to the intra-Trac browser location (when available), and the second column (which shows the original URL from the property) will link to that location?

Yes, that was the intent.

I thought such a behavior would be confusing, because the first item (which is usually a short name) will be the useful link, and the second item (which is usually a long URL) will be the not-so-useful link. You get different behaviors for links in the same row, and the "bigger" link is the less meaningful one…

But you see the bigger link in full, no? If you see svn://server/repository/branches/maintenance/..., or "​http://…", then it's not surprising that it takes you there.

So I posted svn-externals-7687.4.patch​, which includes:

• some major fixes to bugs I found while working on the last comment (wrong links were generated to internal browser when environment not served via server root)
• changed "linking policy" to externals-table items according the comment:39 (first item to internal browser, second item to original URL, revision not a link)
• improved support when default repository exists (was handled incorrectly)

Great, I've now tested the patch on a few 1.5 examples, and it works already quite well.

Now I'm doing further changes myself while trying to get into the code. It's taking a bit more time than expected…

[Itamar Ostricher]

Replying to cboos:

Replying to itamaro: …

I thought such a behavior would be confusing, because the first item (which is usually a short name) will be the useful link, and the second item (which is usually a long URL) will be the not-so-useful link. You get different behaviors for links in the same row, and the "bigger" link is the less meaningful one…

But you see the bigger link in full, no? If you see svn://server/repository/branches/maintenance/..., or "​http://…", then it's not surprising that it takes you there.

Agreed. Then the latest patch does that :-)

I wonder - what does that mean that I am the ticket owner?

[Christian Boos]

Replying to itamaro:

Agreed. Then the latest patch does that :-)

Right, verified.

I wonder - what does that mean that I am the ticket owner?

That you did most of the work ;-)

[rverchere@…]

Hi,

I was using trac0.11 plus old svn:externals patch provided by eblot. Last week I upgraded to trac 0.12beta, and applied svn:externals patch. I did not modify my trac.ini since update.

In trac.ini, I get these parameters:

[svn:externals]
1 = /sdk http://www.domain.com/trac/sdk/browser/$path?rev=$rev

My svn:externals look likes :

-r11948 /sdk/branches/hoverla-1.x/sdk sdk

and

/sdk/branches/hoverla-1.x/sdk@11948 sdk

In both cases, svn:externals link is incorrect in trac browser. Instead of redirect to http://www.domain.com/trac/sdk/browser/branches/hoverla-1.x/sdk, it redirects to http://www.domain.com/sdk/branches/hoverla-1.x/sdk

is there an issue in my config, or in patch ? ;)

If help needed, I can test it as we use a lot of svn:externals here.

[Christian Boos]

I'll resume work on this after the 0.12 release. I have a small set of follow-up patches and still need to understand better one or two things in the code.

[Itamar Ostricher]

Replying to cboos:

Why wait for 0.13, with an existing patch?

[Christian Boos]

Well, back then in June I tried to work on this, and I did a few further changes (some fixes, some simplifications), some more tests), but I remember also that I couldn't fully understand some parts of it. So I have the feeling that either the patch is not fully ready for integration or that I need further explanations. My intent was to create a branch starting with your patches, then mine, then refine the whole thing once more with your help.

[Itamar Ostricher]

I will gladly explain any misunderstood parts of my code, if anyone asks :-) and, of course, willing to refactor and refine the patch until it is fit for integration.

It's just that I hoped this will go into 0.12.x, since I have started further work on other stuff that depend on this (e.g. #6474), and are defined as "enhancements", so if this happens only in 0.13 then the other stuff will have to wait for 0.14…

Maybe, in the spirit of the dedicated branch you mention, it might be productive to group several browser-related tickets (from the top of my head: this one, #6474, #2695) and give write-access to interested parties (such as myself :-) ). what do you think?

[Thijs Triemstra <lists@…>]

Replying to itamaro:

I will gladly explain any misunderstood parts of my code, if anyone asks :-) and, of course, willing to refactor and refine the patch until it is fit for integration.

I guess more docstrings are always welcome (for new public methods that are currently missing them) and I don't know if trac likes pep-8 but I prefer:

"""
Removes `prefix` from `str` if `prefix` prefixes `str`.
"""

over

"Removes `prefix` from `str` if `prefix` prefixes `str`."

[Remy Blank]

Preparing for 1.0.

[Jun Omae]

#11880 was closed as duplicate.

[Ryan J Ollos]

Moved ticket assigned to next-stable-1.0.x since maintenance of 1.0.x is coming to a close. Please move the ticket back if it's critical to fix on 1.0.x.

[Ryan J Ollos]

Milestone renamed