Edgewall Software
Modify

Opened 10 years ago

Last modified 9 months ago

#7687 assigned defect

Add support for svn:externals "1.5" style

Reported by: Emmanuel Blot Owned by: Jun Omae
Priority: high Milestone: next-stable-1.2.x
Component: version control/browser Version: 0.11.1
Severity: normal Keywords: svn15 svn:externals patch svn
Cc: Emmanuel Blot, itamarost@…, mpotter@…, joel@…, rverchere@…, lists@…, jon.kowal@…, Ryan J Ollos
Release Notes:
API Changes:

Description (last modified by Christian Boos)

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)

Attachments (13)

svn-externals-7687-1.patch (6.1 KB ) - added by Emmanuel Blot 9 years ago.
Preliminary version
svn-externals-7687-2.patch (6.2 KB ) - added by Emmanuel Blot 9 years ago.
New version of the patch: svn:url removed (use svn:externals instead), simpler implementation
svn-external-7687-3.patch (6.5 KB ) - added by Emmanuel Blot 9 years ago.
Replace hard-to-read external list with a cleaner table
Picture 1.png (12.9 KB ) - added by Emmanuel Blot 9 years ago.
Rendering example
patch3.png (12.9 KB ) - added by Emmanuel Blot 9 years ago.
Rendering example (I should not have used a space char in the file name…)
svn-externals-7687-multirepos.patch (19.4 KB ) - added by Itamar O <itamarost@…> 8 years ago.
Ported patch to svn_prop.py and extended to support multirepos concepts
svn-externals-7687-multirepos.2.patch (47.0 KB ) - added by Itamar Ostricher 8 years ago.
New & improved patch, now with unit tests!
svn-externals-7687-multirepos.3.patch (47.8 KB ) - added by Itamar Ostricher 8 years ago.
Forgot to include the added test module in init in the previous patch
svn-externals-7687-multirepos.4.patch (47.8 KB ) - added by Itamar Ostricher 8 years ago.
Fixed bug introduced by previous patch (rendered no-links) + cboos feedback
svn-externals-7687.patch (67.4 KB ) - added by Itamar Ostricher 8 years ago.
some more refactoring, some bug fixes, some more tests
svn-externals-7687.2.patch (66.5 KB ) - added by Itamar Ostricher 8 years ago.
removed duplicate code in externals-string parsing
svn-externals-7687.3.patch (69.3 KB ) - added by Itamar Ostricher 8 years ago.
Fixed issues pointed by Christian
svn-externals-7687.4.patch (69.9 KB ) - added by Itamar Ostricher 8 years ago.
New patch (see comment:40)

Download all attachments as: .zip

Change History (76)

comment:1 Changed 10 years ago by Christian Boos

Description: modified (diff)
Keywords: svn15 added

comment:2 Changed 10 years ago by 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 ;-)

comment:3 Changed 10 years ago by Christian Boos

Description: modified (diff)

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.

comment:4 Changed 10 years ago by Christian Boos

Milestone: 0.13

comment:5 Changed 9 years ago by Emmanuel Blot

Owner: set to Emmanuel Blot
Status: newassigned

Changed 9 years ago by Emmanuel Blot

Attachment: svn-externals-7687-1.patch added

Preliminary version

comment:6 Changed 9 years ago by 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 

comment:7 Changed 9 years ago by 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

comment:8 in reply to:  7 ; Changed 9 years ago by 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?

comment:9 in reply to:  6 ; Changed 9 years ago by 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.

comment:10 in reply to:  9 ; Changed 9 years ago by 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…

comment:11 in reply to:  8 ; Changed 9 years ago by 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

comment:12 in reply to:  10 ; Changed 9 years ago by 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.

comment:13 Changed 9 years ago by Emmanuel Blot

anonymous was me.

comment:14 in reply to:  11 ; Changed 9 years ago by 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/

comment:15 in reply to:  12 ; Changed 9 years ago by 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).

comment:16 in reply to:  14 Changed 9 years ago by 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

comment:17 in reply to:  15 Changed 9 years ago by 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).

comment:18 in reply to:  10 Changed 9 years ago by 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?

Changed 9 years ago by Emmanuel Blot

Attachment: svn-externals-7687-2.patch added

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

comment:19 Changed 9 years ago by Emmanuel Blot

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

Let me know.

comment:20 Changed 9 years ago by Emmanuel Blot

Cc: Emmanuel Blot added

Changed 9 years ago by Emmanuel Blot

Attachment: svn-external-7687-3.patch added

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

Changed 9 years ago by Emmanuel Blot

Attachment: Picture 1.png added

Rendering example

Changed 9 years ago by Emmanuel Blot

Attachment: patch3.png added

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

comment:21 Changed 9 years ago by Emmanuel Blot

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

comment:22 Changed 9 years ago by 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).

comment:23 in reply to:  22 Changed 9 years ago by 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…).

comment:24 Changed 9 years ago by Ryan Ollos <ryano@…>

Cc: ryano@… added

Changed 8 years ago by Itamar O <itamarost@…>

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

comment:25 Changed 8 years ago by Itamar O <itamarost@…>

Cc: itamarost@… added

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

comment:26 Changed 8 years ago by Remy Blank

Milestone: next-major-0.1X0.12.1
Owner: changed from Emmanuel Blot to Remy Blank
Status: assignednew

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

comment:27 in reply to:  25 ; Changed 8 years ago by 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!

comment:28 in reply to:  27 Changed 8 years ago by 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!

Changed 8 years ago by Itamar Ostricher

New & improved patch, now with unit tests!

Changed 8 years ago by Itamar Ostricher

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

comment:29 Changed 8 years ago by 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.

comment:30 Changed 8 years ago by Christian Boos

Milestone: 0.12.10.12

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!

Changed 8 years ago by Itamar Ostricher

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

comment:31 in reply to:  30 Changed 8 years ago by 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.

comment:32 Changed 8 years ago by Remy Blank

Owner: changed from Remy Blank to Christian Boos

Changed 8 years ago by Itamar Ostricher

Attachment: svn-externals-7687.patch added

some more refactoring, some bug fixes, some more tests

comment:33 Changed 8 years ago by 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).

comment:34 Changed 8 years ago by 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

Changed 8 years ago by Itamar Ostricher

Attachment: svn-externals-7687.2.patch added

removed duplicate code in externals-string parsing

comment:35 in reply to:  34 Changed 8 years ago by Itamar Ostricher

Replying to cboos:

I'd suggest one more simplification:

Done in svn-externals-7687.2.patch

comment:36 Changed 8 years ago by mpotter@…

Cc: mpotter@… added

comment:37 Changed 8 years ago by 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

Changed 8 years ago by Itamar Ostricher

Attachment: svn-externals-7687.3.patch added

Fixed issues pointed by Christian

comment:38 Changed 8 years ago by 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.

comment:39 Changed 8 years ago by 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)

Changed 8 years ago by Itamar Ostricher

Attachment: svn-externals-7687.4.patch added

New patch (see comment:40)

comment:40 Changed 8 years ago by 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)

comment:41 in reply to:  40 ; Changed 8 years ago by Christian Boos

Owner: changed from Christian Boos to Itamar Ostricher

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…

comment:42 in reply to:  41 ; Changed 8 years ago by 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?

comment:43 in reply to:  42 Changed 8 years ago by 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 ;-)

comment:44 Changed 8 years ago by joel@…

Cc: joel@… added

comment:45 Changed 8 years ago by 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.

comment:46 Changed 8 years ago by rverchere@…

Cc: rverchere@… added

comment:47 Changed 8 years ago by Christian Boos

Milestone: 0.120.12.1
Priority: normalhigh

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.

comment:48 Changed 8 years ago by Christian Boos

Milestone: 0.12.10.13

comment:49 in reply to:  48 Changed 8 years ago by Itamar Ostricher

Replying to cboos:

Why wait for 0.13, with an existing patch?

comment:50 Changed 8 years ago by 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.

comment:51 Changed 8 years ago by 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?

comment:52 Changed 8 years ago by Thijs Triemstra <lists@…>

Cc: lists@… added

comment:53 in reply to:  51 Changed 8 years ago by 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`."

comment:54 Changed 7 years ago by jon.kowal@…

Cc: jon.kowal@… added

comment:55 Changed 6 years ago by Remy Blank

Milestone: 1.01.0-triage

Preparing for 1.0.

comment:56 Changed 4 years ago by Jun Omae

Component: version controlversion control/browser
Keywords: svn:externals added

comment:58 Changed 4 years ago by Ryan J Ollos

Cc: Ryan J Ollos added; ryano@… removed

comment:59 Changed 4 years ago by Jun Omae

#11880 was closed as duplicate.

comment:60 Changed 3 years ago by Ryan J Ollos

Owner: Itamar Ostricher deleted

comment:61 Changed 2 years ago by figaro

Keywords: patch added

comment:62 Changed 2 years ago by Jun Omae

Owner: set to Jun Omae
Status: newassigned

comment:63 Changed 19 months ago by Ryan J Ollos

Milestone: next-stable-1.0.xnext-stable-1.2.x

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.

comment:64 Changed 16 months ago by Ryan J Ollos

Keywords: svn added

Modify Ticket

Change Properties
Set your email in Preferences
Action
as assigned The owner will remain Jun Omae.
The ticket will be disowned.
as The resolution will be set.
to The owner will be changed from Jun Omae to the specified user.

Add Comment


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