svn:mergeinfo should link to changeset view

[edgewall.org@…]

When showing the svn:mergeinfo property, it would be useful if each line was linked to the changeset view. e.g if the mergeinfo property is set to:

/branches/featurebranch/:500-523

You could click that link and be taken to

changeset?old_path=branches/featurebranch&old=500&new_path=branches/featurebranch&new=523

[Christian Boos]

Something like that, yes. I think it would be better to show the corresponding log range, as it's cheaper to generate and it's easy from there to go to the corresponding changeset if that's really needed, using the View changes button.

Even better would be to support showing the Merged/Eligible ranges that way, using ​svn_client_mergeinfo_log_merged() and ​svn_client_mergeinfo_log_eligible().

[Javier Sanz <jsceballos@…>]

Proposed patch

[Javier Sanz <jsceballos@…>]

That patch parses a bit the property. It links the branches names to their browser URL, and for the ranges, I also liked better the approach that cboos suggested, rather than showing the whole diff.

It doesn't show any eligible -yet to merge- revisions. Maybe in the future. In that case we must take into account that svn:mergeinfo is generated for every merge operation, even when the intention isn't synchronizing branches.

[Remy Blank]

This is a good first step. A few comments:

• I would try to keep the textual representation of the svn:mergeinfo property intact, and just "linkify" the revisions and ranges. That would mean joining the individual revisions and ranges with a ',' followed by the zero-width space as in the original version.

• You should use self.env.href instead of self.env.abs_href for generating the URLs. Also, by convention we write the first part of the URL as an attribute, like so: self.env.href.changeset(rev, path) instead of self.env.href('changeset', rev, path).

• Couldn't you use the splitlines() method of string objects to split the paths, instead of a regexp?

• Two stylistic details, please add a space after commas in function arguments lists, and trim all lines to a maximum of 80 characters.

• Finally, shouldn't the links to the changelog and changesets be restricted to destination path?

[Javier Sanz <jsceballos@…>]

I applied your suggestions, except the last one. What do you mean by "restricted to destination path"? They already have the form ​http://myrepo/changeset/999/mybranch, instead of just ​http://myrepo/changeset/999.

[Remy Blank]

Replying to Javier Sanz <jsceballos@…>:

I applied your suggestions, except the last one. What do you mean by "restricted to destination path"? They already have the form ​http://myrepo/changeset/999/mybranch, instead of just ​http://myrepo/changeset/999.

Sorry, my bad. I haven't had time to actually test the patch yet, only had a look at the code. Looks pretty good now! I'll test it tomorrow (bed time here).

[osimons]

Great initative - I want to see this included too. I haven't tried the patch either - just reading the code as well :-)

A quick scan of latest patch, and I see two issues:

1. Using env.href is not good. Anything rendering links needs to be passed the context and use context.href, or else all kinds of wrong links can be made. Search for env.href in all Trac code shows it is just used a couple of places in test code in mock objects for simplicity. The same page can often be accessed by many url's (prime example is http vs https), and it needs to be aware of the current context.
2. Passing context would also let us check the paths and/or changesets against user permissions, and only render links when the user can actually see the the 'otherbranch' information.

If we get a tested and working patch, I don't see any reason not to schedule it for 0.11.x?

[Remy Blank]

Replying to osimons:

1. Using env.href is not good. Anything rendering links needs to be passed the context and use context.href

Yes, you're right. I was only reading the patch, and I hadn't seen that the context was passed to render_property(). I'll fix that.

1. Passing context would also let us check the paths and/or changesets against user permissions, and only render links when the user can actually see the the 'otherbranch' information.

We could do that, but it would result in lots of permission checks (one for the path, and one for every revision / range). As we see on t.e.o, the mergeinfo properties tend to get very large, so I'm worried about the performance impact. If you are using fine-grained permissions, checking the permissions on a changeset means retrieving that changeset from the repository and checking every single modified path.

Maybe we could simplify the check by only verifying permissions on the merge path, and not linkify if the user doesn't have access?

If we get a tested and working patch, I don't see any reason not to schedule it for 0.11.x?

The reason why I scheduled it for 0.12 is that svn:mergeinfo rendering is currently only implemented there. We could backport it, though.

[Christian Boos]

Replying to rblank:

Replying to osimons:

1. Passing context would also let us check the paths and/or changesets against user permissions, and only render links when the user can actually see the the 'otherbranch' information.

We could do that, but it would result in lots of permission checks (one for the path, and one for every revision / range). As we see on t.e.o, the mergeinfo properties tend to get very large, so I'm worried about the performance impact. If you are using fine-grained permissions, checking the permissions on a changeset means retrieving that changeset from the repository and checking every single modified path.

Maybe we could simplify the check by only verifying permissions on the merge path, and not linkify if the user doesn't have access?

I think that would be enough, yes.

Another point: the log module supports displaying ranges directly, so it would be better to generate a single log link for each path. That way one sees the "Merged" changesets all at once.

e.g. something like:

        ul = tag.ul()
        for line in prop.splitlines(): 
            path, revs = line.split(':')
            if 'LOG_VIEW' in context.perm('source', path):
                ul.append(tag.li(tag.a(path, href=context.href.browser(path)), 
                                 ': ', 
                                 tag.a(_('merged'), title='r' + revs,
                                       href=context.href.log(path, revs))))

(untested)

[osimons]

Replying to rblank:

Maybe we could simplify the check by only verifying permissions on the merge path, and not linkify if the user doesn't have access?

I'm fine with that - I said 'and/or' in my request as I see the potential cost of full ckecks (and complexity of certain ranges within ranges are not permitted and so on…).

The reason why I scheduled it for 0.12 is that svn:mergeinfo rendering is currently only implemented there. We could backport it, though.

Ah. Not tracking the versioncontrol code all that closesly… :-) Subversion 1.5+ is everywhere now, so likely it would be a welcome addition (including here at the edgewall sites).

[osimons]

Replying to cboos:

Another point: the log module supports displaying ranges directly, so it would be better to generate a single log link for each path. That way one sees the "Merged" changesets all at once.

Yes and no. Sometimes that would be nice, and sometimes you just want to see the latest. Couldn't we do both? Linkify the ranges and individual changesets as the patch does + provide an additional 'All' link?

[Christian Boos]

Replying to osimons:

Replying to cboos:

Another point: the log module supports displaying ranges directly, so it would be better to generate a single log link for each path. That way one sees the "Merged" changesets all at once.

Yes and no. Sometimes that would be nice, and sometimes you just want to see the latest. Couldn't we do both? Linkify the ranges and individual changesets as the patch does + provide an additional 'All' link?

I think that showing links for all changesets would quickly become cumbersome. For example, in the situation described by eblot ​here.

We could show the latest merged changeset (or latest range), though:

• path merged: all, latest | eligible: next, all

Eligible info is mainly interesting when browsing at HEAD, so we wouldn't have to compute it when browsing on past revisions.

[Remy Blank]

Updated patch with permission checks and error handling

[Remy Blank]

The patch above is an update on the initial idea fixing the following issues:

• Added permission checks on the merge paths.
• Added error handling for the case where the property content is invalid.
• Added some padding to avoid Firefox displaying scrollbars due to overflow: auto.

Please test and let me know what you think. I tend to agree with Christian that the result is hardly readable and doesn't really make sense. I'll attach a second patch shortly that implements his idea, at least for displaying the merged / blocked revisions.

I also agree that displaying the eligible revisions is the most useful functionality, but also the most difficult. Should we replicate the algorithm used by SVN for calculating them, or do we use svn_client_mergeinfo_log_eligible()?

[Christian Boos]

Replying to rblank:

I'll attach a second patch shortly that implements his idea, at least for displaying the merged / blocked revisions.

And about the "latest"/"all" part, maybe it would be even more interesting to show it like that:

• path1: last merge r7342 (more)
• path2: last merge r7222-7334 (more)

i.e. show directly the last merge number or last merge range.

I also agree that displaying the eligible revisions is the most useful functionality, but also the most difficult. Should we replicate the algorithm used by SVN for calculating them, or do we use svn_client_mergeinfo_log_eligible()?

He, to be 100% bug compatible we should use svn_client_mergeinfo_log_eligible() ;-)

One problem is that if we're using svn's own algorithm, that might lead to different results than to what the command-line svn mergeinfo would show to the user in its working copy (as the version of Subversion might well be different). The second is the performance impact - I have no idea if its cost yet, but assume it should be fast enough as everything will be done using the ​file:// protocol (see our usage of client.blame2).

OTOH, showing simply the changesets available on the merge source and not yet in the merged range shouldn't be hard…

[Remy Blank]

Replying to cboos:

And about the "latest"/"all" part, maybe it would be even more interesting to show it like that:

• path1: last merge r7342 (more)
• path2: last merge r7222-7334 (more)

i.e. show directly the last merge number or last merge range.

I wonder if even that makes sense. The last revision is not necessarily the last merge operation that was done. Currently, I'm being minimalistic and only display:

• path1: show revisions
• path2: show revisions

with the list of revisions in the link title. So if you want to see the last revision, it's two clicks away.

OTOH, showing simply the changesets available on the merge source and not yet in the merged range shouldn't be hard…

That's what I was thinking as well. It should also allow displaying the svnmerge-* properties properly.

[Remy Blank]

Don't linkify all revisions, just show a single link to the log.

[Remy Blank]

The patch above shows only a single "show revisions" link next to each path. It's much cleaner IMO.

Now on to calculating eligible revisions…

[Remy Blank]

mergeinfo display with eligible revisions

[Remy Blank]

The patch above also calculates the eligible revisions. The calculated list is the same as returned by svnmerge.py avail on trunk.

• For each path, there is a "revisions" link that points to the log with all the revisions contained in the property. Additionally, an "eligible" link points to the log with the revisions not yet merged and not blocked.

• The tooltips for "revisions" and "eligible" show the list of revisions and ranges.

I find it pretty cool, and it will be very useful for merging to trunk, as you can immediately see in the log which revisions are yours.

Let me know what you think! I'll then apply to trunk and backport to 0.11-stable as requested (there are a few set() and generator expression instances in there, so I'll have to be careful for Python 2.3).

[Christian Boos]

Looks great! I still would prefer to see merged instead of revisions, but that's a detail.

One thing which needs to be improved is the performance of the initial gathering of potential revs, as the loop on node.get_history() is not really affordable. On a s.e.o/trac mirror, viewing /trunk is okay as the merge source don't have a long history, but when viewing any other branch, we get trunk as a merge source and so we currently unfold the whole history up to r1, which takes approx. 11 seconds (and for each property!).

I've also got a KeyError: svnmerge-blocked when browsing 0.11-stable, easy to fix:

trac/versioncontrol/svn_fs.py

@@ -394 +394 @@
         name and path.
         """
         if name == 'svnmerge-integrated':
-            prop = props['svnmerge-blocked']
+            prop = props.get('svnmerge-blocked', '')
         else:
             return ""
         for line in prop.splitlines():

[Christian Boos]

Also, there can be a single revision in the merged set, in which case the log module will show all revisions starting from that rev. Would require some fixes in the log module that will expand to the log: wiki formatter, but could also be worked around by directly linking to the changeset in this situation:

trac/versioncontrol/svn_fs.py

@@ -424 +424 @@
                                  href=context.href.browser(spath,
                                                 rev=context.resource.version))]
                     if revs:
-                        log_href = context.href.log(spath, revs=revs)
-                        row.append(tag.a(_('revisions'),
+                        if ',' in revs or '-' in revs:
+                            merged_href = context.href.log(spath, revs=revs)
+                        else:
+                            merged_href = context.href.changeset(revs, spath)
+                        row.append(tag.a(_('merged'), # try it! :-)
                                          title=revs.replace(',', ', '),
-                                         href=log_href))
+                                         href=merged_href))
                     else:
                         row.append(tag.span(_('revisions'),
                                             title=_('No revisions')))

[Remy Blank]

Replying to cboos:

I still would prefer to see merged instead of revisions, but that's a detail.

I don't really mind. Does "merged" also make sense for svnmerge-blocked?

One thing which needs to be improved is the performance of the initial gathering of potential revs

Any ideas how I could improve this? I don't think in-memory caching is adequate here. For cached repositories, we could probably use some SQL mojo, but for direct-svnfs, I'm at a loss.

I've also got a KeyError: svnmerge-blocked when browsing 0.11-stable, easy to fix:

Good catch, thanks!

Also, there can be a single revision in the merged set

Right, I think linking directly to the changeset makes sense here.

[Christian Boos]

The changeset view actually used the renderer as well, and it didn't look that pretty there. So I also added an implementation of IPropertyDiffRenderer. It's probably not complete, the layout is not that pretty (but you get the idea). I figured I'll add it anyway, in case you'd had the idea to work in the some area ;-)

It also contains my other updates and "fixes" the svnmerge-blocked case! (s/merged/blocked there…)

As for the performance issue, I also have some ideas (like simply assuming a start of range(1,rev)), but this needs some refining.

[Christian Boos]

my updates to the previous patch + added an implementation of IPropertyDiffRenderer

[Emmanuel Blot]

It would be nice that the above patch provides a mean to discard source branches that do not exist anymore, such as in

        #line 419
        repos = self.env.get_repository()
        for line in props[name].splitlines():
            try:
                path, revs = line.split(':', 1)
                spath = path.strip('/')
                revs = revs.strip()
                try:
                    cnode = repos.get_node(spath, context.resource.version)
                except NoSuchNode:
                    continue
                if 'LOG_VIEW' in context.perm('source', spath):

This code snippet discards all the deleted source branches, although it might be nice to have some hidden <div> than could be expanded on user request, and that would contain those deleted source branches.

[Christian Boos]

Replying to eblot:

It would be nice that the above patch provides a mean to discard source branches that do not exist anymore

I'm wondering… wouldn't it be better to take this as an incentive to fix the svn:mergeinfo and prune the no longer needed sources?

The information about what got merged is not lost, it would still be present in the revision just before the deletion. Even in the rare case the branch would be resurrected, I don't think the old mergeinfo for that source would still be relevant (or even used, as the path is re-created).

[Emmanuel Blot]

cboos' last patch with deleted branches show/hide feature

[Emmanuel Blot]

Replying to cboos:

I'm wondering… wouldn't it be better to take this as an incentive to fix the svn:mergeinfo and prune the no longer needed sources?

Not sure to understand your point… What do you want to fix?
It's important (at least for us) to be able to track which of the developers' branches/sandboxes has been merged into a "main" branch.

I've attached a modified version of your patch, so that deleted branches do not get in the way, although it is still possible to show them on request. This is a "proof-of-concept" patch: it does not support browsers w/o Javascript, and I'm still not convinced that creating Markup code in Python source code is a good idea ;-)

Just to let you know: I've installed this patch on our production server, so that we can start using your patch with a real environment - and a lot of merge operations, several ops a day.

[Christian Boos]

Replying to eblot:

Replying to cboos:

I'm wondering… wouldn't it be better to take this as an incentive to fix the svn:mergeinfo and prune the no longer needed sources?

Not sure to understand your point… What do you want to fix?

If you remove a branch, then the svn:mergeinfo about that source won't be needed again for subsequent merges …

It's important (at least for us) to be able to track which of the developers' branches/sandboxes has been merged into a "main" branch.

… except for this.

I've attached a modified version of your patch, so that deleted branches do not get in the way, although it is still possible to show them on request.

OK, fine, I'll test that!

This is a "proof-of-concept" patch: it does not support browsers w/o Javascript, and I'm still not convinced that creating Markup code in Python source code is a good idea ;-)

The suggestion from Remy on ticket:7687#comment:18 is fine by me, we'll eventually do it that way.

Just to let you know: I've installed this patch on our production server, so that we can start using your patch with a real environment - and a lot of merge operations, several ops a day.

Ugh, that's a bit too early I'd say, as the performance impact must be quite important (see comment:18). The good news is that I'm working on this right now, so expect a fix soon ;-)

[Remy Blank]

Replying to cboos:

The good news is that I'm working on this right now, so expect a fix soon ;-)

Ah, good thing that you wrote that, because I was going to work on it as well. I guess I better wait a bit…

[Christian Boos]

Back to the crucial performance issue for eligible changesets, here are my working notes.

… simply assuming a start of range(1,rev), but this needs some refining.

We assume that a merge source will have eligible changesets corresponding to changesets which were created on that branch. So as a first approximation, we have at most [source_origin_rev, rev], excluding the changesets not actually on the branch.

The problem is that if this source is not a branch, i.e. it's a path that has never been copied from some other path (like trunk, in most cases), then using the above [branch_origin_rev, rev] would translate to [1, rev], and that would be overkill to discover and tidy up. Even if it's a real branch with a long history (say > 1000 changesets), it will take some time to tidy up the range.

OTOH, if the target path is a branch, we wouldn't have to consider all the changesets that are part of the history of the branch (through the follow copy information).

Example:

                4 -- 7 (other-post)
               /
 -- 1 --(2)-- 3 -- 21 -- 37 (trunk)
          \
           5 --(8)-- 12 (stable)
            \    \
             \    10 -- 20 (exp) <= TARGET
              \
               6 -- 9 -- 13 (other-pre)

If we're browsing exp, we have the following initial eligible sets from the other branches considered as merge sources:

• [3-37] from trunk (instead of [1-37])
• [12] from stable (instead of [5-12])
• but [4-7] from other-post
• and [6-13] from other-pre

So what is needed here is to be able to quickly gather the "branch nodes" ((2) and (8) in the above examples). If no branch node is found, then we would also know that we're not dealing with a branch.

Fortunately, the SVN API has a primitive to do that efficiently: ​svn_fs_closest_copy.

This will enable us to build more optimal ranges for the merge sources that are ancestor paths. For the others, we would at least know the [branch_origin_rev, rev] instantaneously and decide if it's worth tidying up the range. If we don't tidy up, then risk to get spurious data in the tooltip and at worst lead to empty lines in the revision log. Those inconvenients are much mitigated if the svn:mergeinfo itself is accurate.

Everything is not clear cut at this point, in particular I found out that the optimization using the ancestry can actually be slower, as it might lead to smaller ranges that get below the threshold below which we just use the approximation…

Maybe it would be worth to try to see how efficient the SVN API mergeinfo primitives are…

[Christian Boos]

First try at optimizing the eligible information

[Christian Boos]

The attachment:7715-mergeinfo-optimized-eligible-r8263.patch​ implements the ideas in comment:28. It's "fast" now, but perhaps not 100% satisfying, due to the fact that we approximate ranges bigger than 1000 changesets (for example, on a t.e.o mirror, the interval for 0.11-stable gets approximated, leading to non-optimal display of the mergeinfo for this source on /trunk).

So maybe a different approach is needed, like acknowledging that accurate display of this information takes time. So we could only display the eligible link, and do the actual computation when one clicks on it.

The patch also doesn't produce the Toggle deleted branches table from attachment:svn-mergeinfo-7715-2.patch​ when there are no deleted branches and fixes the display of the row when an exception occurs.

[Christian Boos]

Cumulative patch, 7715-mergeinfo-optimized-eligible-r8263.patch​ + improve the rendering of mergeinfo diffs on Changeset view

[Christian Boos]

Thanks to some help from Manu, I improved the rendering of mergeinfo property differences in 7715-mergeinfo-improved-mergeinfo-diffs-r8263.patch​.

Please test, comment and refine further if needed!

[Remy Blank]

I've been playing with the patch above, and have a few comments:

• In the property diff viewer, we have to take into account the actual revisions on the path, in the same way as in the property viewer. For example, if you view [8254], you see that the svn:mergeinfo and svnmerge-integrated are not rendered the same, because the latter stores ranges containing revisions not affecting the path.

• The table below each property in the diff viewer has a partial border, which looks ugly. This is due to its inheriting from the .diff table CSS selector. We will probably have to use more specific selectors.

• In the diff viewer, the path and revisions are rendered in monospace, whereas the paths shown above are in a proportional typeface. This is due to the .diff table td CSS entry.

• I'm not sure about using approximations for calculating the revisions in the property viewer. I'm working on an alternate, exact solution. Stay tuned.

• I don't like using a normal, textual link for toggling the visibility of the deleted branches, as it is inconsistent with other parts of Trac. I'll experiment with using the same mechanism as for ticket queries. And yes, as noted in the code, the current implementation generates duplicate ids.

I'll work on this some more this afternoon.

[Remy Blank]

Faster retrieval of eligible revisions

[Remy Blank]

The patch above is an update with the following changes:

• Fixed the bugs from comment:18 and comment:19.

• Added a proof-of-concept for a faster exact calculation of eligible revisions, by using the database cache. If we keep it, it will be moved into CachedRepository, and the reference to SubversionRepository. I get a typical performance factor of 10 when viewing the current HEAD of 0.11-stable.

• In the current state, both the reference implementation (using node.get_history()) and the fast implementation are used when finding eligible revisions, and the time for each is displayed, along with any differences found (should normally be empty). On my machine, the reference implementation takes around 3s per property on 0.11-stable, whereas the fast one takes 0.3s per property.

• Integrated the IPropertyDiffRenderer and tried to improve the display somewhat (including displaying "blocked" instead of "merged" for svnmerge-blocked).

Still to be done:

• Re-add hiding of paths that don't exist in the repository anymore. I have intentionally left it out for now, to keep the code simpler. I intend to add it back once the performance issues are solved.

• Fix the display inconsistency in the changeset viewer (first point in comment:31).

• Fix the CSS issues from comment:31.

I'll work on it more in the next days, so you may want to wait before testing.

[Christian Boos]

Replying to rblank:

• The table below each property in the diff viewer has a partial border, which looks ugly. This is due to its inheriting from the .diff table CSS selector. We will probably have to use more specific selectors.

Are you sure you didn't miss the CSS changes in trac/htdocs/css/diff.css, in the attachment:7715-mergeinfo-improved-mergeinfo-diffs-r8263.patch​? I ask because for me this suppresses the table border (and I don't see those changes propagated to your attachment:7715-mergeinfo-fast-r8263.patch​).

[Christian Boos]

Replying to rblank:

The patch above is an update with the following changes:

• In the current state, both the reference implementation (using node.get_history()) and the fast implementation are used when finding eligible revisions, and the time for each is displayed, along with any differences found (should normally be empty). On my machine, the reference implementation takes around 3s per property on 0.11-stable, whereas the fast one takes 0.3s per property.

What would be also interesting is to compare with the timings and results obtained using svn_client_mergeinfo_log_eligible().

[Remy Blank]

Replying to cboos:

Are you sure you didn't miss the CSS changes in trac/htdocs/css/diff.css, in the attachment:7715-mergeinfo-improved-mergeinfo-diffs-r8263.patch​?

Indeed, that was the case. I must have missed that change while I was trying to merge the various approaches. This highlights the difficulty of working together on a patch basis (and the usefulness of revision control systems).

What would be also interesting is to compare with the timings and results obtained using svn_client_mergeinfo_log_eligible().

Yes, good idea. I'll do that.

[Remy Blank]

I have committed a complete version in [8271]. Some comments:

• Moved the retrieval of revisions affecting a given path into SubversionRepository (using the slow version) and CachedRepository (using the database). Performance here is quite good on the Trac repository displaying 0.11-stable (0.3s per property, quite a slow machine).

• Fixed the display of property diffs by taking into account the revisions affecting the given paths. This way the display of svn:mergeinfo and svnmerge-integrated is consistent.

• Fixed the display of the property diffs to be consistent with the property display (proportional font, no border).

• Hiding of deleted branches was re-added, and should work with JavaScript disabled (i.e. the link "(toggle deleted branches)" should not appear and the deleted branches should be visible if JavaScript is disabled).

I haven't tested svn_client_mergeinfo_log_eligible() yet, but I'm not sure it's worth using it, as we need to implement the algorithm for svnmerge-integrated anyway.

I'm quite happy with the result. Please test and report any issues here. If nothing pops up in the next few days, I'll backport the feature to 0.11-stable, as requested in comment:7.

[Christian Boos]

Great job, I looked through the patch and it seems very good. I'll just mess up a bit the t.e.o repository before testing it … (r8272).

One thing though: the way you changed the diff.css and the corresponding HTML by adding a trac-diff class doesn't seem backward compatible. That's fine for trunk but maybe we need to do it differently when backporting on 0.11-stable?

[Remy Blank]

Replying to cboos:

Great job, I looked through the patch and it seems very good. I'll just mess up a bit the t.e.o repository before testing it … (r8272).

I have already found a glitch, [8199/trunk] doesn't render correctly anymore. I'm working on a fix.

One thing though: the way you changed the diff.css and the corresponding HTML by adding a trac-diff class doesn't seem backward compatible.

You mean it will break custom CSS styling? Probably true, though I wonder how many people customize the appearance to that extent.

That's fine for trunk but maybe we need to do it differently when backporting on 0.11-stable?

A general comment on Trac's CSS is that selectors are often much too general. For example, styling .diff table will automatically apply the style to all tables below a <div class="diff">. That's what happened in the property diff view. To fix that, I could either make the selector more specific (that's what [8271] does) or "undo" the styling with a more specific selector, which is cumbersome.

So I would argue that using more specific selectors is the way to go (also for future changes), but yes, it does break backward compatibility. Any suggestions how we could cleanly handle that in 0.11-stable?

[Remy Blank]

BTW, we will need to fix the merge to multirepos in [8274], as the new functionality currently uses env.get_repository(), whereas in multirepos, we will need to find the right repository first.

[Remy Blank]

[8275] fixes the property diff renderer for the case where the source path doesn't exist.

[Remy Blank]

While working on this ticket, I noticed that an exception raised during rendering of a property (or property diff) will trigger an internal error. This is not very user-friendly, so [8276] changes this behavior by trying all renderers by decreasing quality until a renderer doesn't raise an exception. This should ensure that even ill-formatted input doesn't prevent a path or changeset from being shown.

[Christian Boos]

Replying to rblank:

So I would argue that using more specific selectors is the way to go (also for future changes),

Agreed.

but yes, it does break backward compatibility. Any suggestions how we could cleanly handle that in 0.11-stable?

Well, as you said:

"undo" the styling with a more specific selector, which is cumbersome.

… but should work:

trac/htdocs/css/diff.css

@@ -186 +186 @@
 .diff pre { background: #fff; border: 1px solid #ddd; font-size: 85%;
   margin: 0;
 }
+
+/* Override .diff table for property tables */
+.diff table.props {
+ border: 0px;
+}
+.diff table.props td {
+ background: inherit;
+}

(taken from attachment:7715-mergeinfo-improved-mergeinfo-diffs-r8263.patch​; I haven't tested if it renders the same as the current trunk, more tweaking probably necessary)

Besides, what do you think about moving the SubversionPropertyRenderer in its own file (e.g. trac/versioncontrol/svn_properties.py)? It's already quite big right now and we also have #7687 coming along…

[Remy Blank]

Replying to cboos:

… but should work:

That's not enough. I had tested that change, and:

• The font was still monospace
• The table fitted the whole width, so spacing between columns looked silly

Basically, you would have to undo all the changes done by the .diff table * selectors, which is why I think this is cumbersome.

Besides, what do you think about moving the SubversionPropertyRenderer in its own file (e.g. trac/versioncontrol/svn_properties.py)? It's already quite big right now and we also have #7687 coming along…

I'd wait until #7687 is done, if at all.

On another note, I don't think I like the message from [8279] to avoid "empty" property changes, it just clutters the display with no real information. I had noticed the special case of [8206], but technically, the display was correct:

• The property has actually changed, but
• The change has no real effect

So I prefer the "Property svn:mergeinfo changed" message with no content (or maybe a single line saying that the change has no effect, which has the added benefit that the XHTML would validate again). If this seems suspect to the user, he can easily check what happened with his SVN client.

If you don't mind, I'll revert that change and implement the message if both modified_and_added_sources and removed_sources are empty.

[Christian Boos]

Replying to rblank:

Replying to cboos:

… but should work:

That's not enough. I had tested that change, and:

• The font was still monospace
• The table fitted the whole width, so spacing between columns looked silly

Basically, you would have to undo all the changes done by the .diff table * selectors, which is why I think this is cumbersome.

No question about that, it's cumbersome. But doing it "the right way" like on trunk possibly breaks backward compatibility, so if we can't do it in another way, I'd still be in favor of the "undo" approach (with a big fat warning saying this is 0.11 specific).

Besides, what do you think about moving the SubversionPropertyRenderer in its own file (e.g. trac/versioncontrol/svn_properties.py)? It's already quite big right now and we also have #7687 coming along…

I'd wait until #7687 is done, if at all.

I think it's useful to separate them. Think about having an alternate svn backend (the one using the ctypes bindings), it would then be necessary to have that split. We could wait until such a backend is implemented and integrated, but nevertheless I think it's conceptually cleaner to put it in its own file (also given the size it takes now).

On another note, I don't think I like the message from [8279] to avoid "empty" property changes, it just clutters the display with no real information. I had noticed the special case of [8206], but technically, the display was correct:

• The property has actually changed, but
• The change has no real effect

Yes, but the above proves my point: if you say The property has actually changed alone, it's confusing (changed, but how?). Adding The change has no real effect or anything else explaining why nothing is shown seems necessary to me.

So I prefer the "Property svn:mergeinfo changed" message with no content (or maybe a single line saying that the change has no effect, which has the added benefit that the XHTML would validate again). If this seems suspect to the user, he can easily check what happened with his SVN client.

OK, with that added line.

If you don't mind, I'll revert that change and implement the message if both modified_and_added_sources and removed_sources are empty.

No problem with that.

[Remy Blank]

Replying to cboos:

… if we can't do it in another way, I'd still be in favor of the "undo" approach (with a big fat warning saying this is 0.11 specific).

Ok, I'll do that for the 0.11-stable backport.

We could wait until such a backend is implemented and integrated, but nevertheless I think it's conceptually cleaner to put it in its own file (also given the size it takes now).

I was going the ​YAGNI route. But as you think it's necessary, I'll split the file.

OK, with that added line.

Ok, will do. Thanks for the feedback!

[Remy Blank]

Final fixes done in [8280]. Next steps are:

• Move property renderers to a separate module.
• Backport to 0.11-stable.

[Remy Blank]

The property renderer has been moved to its own module in [8282], and a few minor details have been fixed in [8283-8284].

The renderer has also been backported to 0.11-stable in [8285]. It should be 2.3-compatible, and the CSS has been adapted as suggested in comment:42.

Please test (especially the backport) and report any issues so that I can fix them in time for 0.11.5.

[Christian Boos]

One nit: on 0.11-stable, the new diff.css rules seem to work for some changesets, and not for others. Looking closer, it seems that when it doesn't work the <table> has no "props" class. It's the "set to" vs. "changed", i.e. the first case is using the IPropertyRenderer, the second the IPropertyDiffRenderer.

Ok with the following change?

trac/versioncontrol/svn_prop.py

@@ -155 +155 @@
                                          href='#'),
                    tag.table(tag.tbody(
                        [tag.tr(row, class_=deleted and 'trac-deleted' or None)
-                        for deleted, p, row in rows])))
+                        for deleted, p, row in rows]), class_='props'))
 
     def _get_blocked_revs(self, props, name, path):
         """Return the revisions blocked from merging for the given property

[Remy Blank]

Replying to cboos:

Ok with the following change?

Yes, and I have found a few more minor issues, fixed in 0.11-stable in [8288].

[Remy Blank]

Mmh, [8288] fixed the display of property diffs to be grey instead of black, to be consistent with the browser display. In retrospect, this wasn't such a great idea, as the text of property diffs rendered as "diffs" was then grey as well. Reverted in [8291].

[Christian Boos]

With [8294], I fixed the source:sandbox/multirepos svn:mergeinfo property by copying the recorded revisions for /branches/0.11-stable and /sandbox/rework-testing from svn:mergeinfo on source:trunk​. The eligible revisions for those two sources is now correct (same as on trunk).

I've also reset the revision range for the /trunk source to be [6446-8292]. Using the start of a branch as the initial revision for the merge range is also what svn does when you do the first svn merge ^/source on the branch. But here, the eligible information is not correct, it shows also trunk revisions before r6446. As expected, svn mergeinfo --show-revs eligible ^/trunk does the right thing here.

Btw, same goes for the source:/sandbox/rework-testing branch.

This is related to what I wrote in comment:28. I look what I can get back from attachment:7715-mergeinfo-optimized-eligible-r8263.patch​ (i.e. keep the "optimize" parts, but not the "approximate" parts).

[Remy Blank]

Replying to cboos:

But here, the eligible information is not correct, it shows also trunk revisions before r6446.

Ah, yes, you also have to stop at the creation of the target branch, not only of the source branch. You can probably use the same kind of query as the second part of CachedRepository._get_node_revs() to get the branch point for cached repositories.

I see you have fixed the implementation on multirepos, thanks!

[Christian Boos]

fix start of eligible ranges, on top of r8299 (tested on 0.11-stable and trunk)

[Christian Boos]

Eligible ranges for a source shouldn't contain revisions prior to the branch point of the target path (or one of its ancestor) on that source (see comment:28 for a sample, (8) is the branch point of the target path /exp on its /stable source and (2) is the branch point of an ancestor of the target path /exp (here /stable) on its /trunk source).

The attachment:t7715-fix-eligible-start-r8299.patch​ computes all those branch points at once, using the svn API (which is fast in this case).

[Christian Boos]

Improved version of the previous patch (smarter CachedRepository._get_node_revs)

[Remy Blank]

Patch looks good at first glance, I'll test it tomorrow morning. You can probably remove the revs.sort() in CachedRepository._get_node_revs(), as the returned list is put in a set anyway.

Heh, in retrospect, that idea with bisect looks pretty ridiculous now… Must have been in a weird state of mind :-)

[Remy Blank]

Works great, applied in [8310].

But I wonder if this is enough already. Currently, we compute eligible revisions by starting with the list of revisions on the source since the target has forked off the source, and then subtracting merged and blocked revisions. What if the target has not forked from the source directly? Something like the following situation:

            9 -- 10 (branch-1.1) (SOURCE)
           /
     2 -- 5 -- 6 (branch-1)
    /
-- 1 -- 3 -- 7 (trunk)
         \
          4 -- 8 (branch-2) (TARGET)

Shouldn't we actually be finding the common ancestor of SOURCE and TARGET instead?

[Christian Boos]

Replying to rblank:

Works great, applied in [8310].

Thanks!

… Shouldn't we actually be finding the common ancestor of SOURCE and TARGET instead?

I don't see how the common ancestor plays a role here.

In your example, we have 9,10 eligible from branch-1.1, regardless of what the common ancestor can be. It's the same situation as described in comment:28 for other-pre and other-post branches.

We can even have:

            9 -- 10 (branch-1.1)
           /
     2 -- 5 -- 6 (branch-1)
    /
-- 1 -- 3 -- 7 (trunk)
         \
          4 -- 8 (branch-2) (TARGET)
                \
                 11 -- 12 (branch-2.1) (SOURCE)

and we'll still have 11,12 eligible from branch-2.1.

What I see as still problematic is the performance impact for long-running branches (e.g. trunk) when they are considered as merge source (again comment:28). This might work fine for <10000 changesets, but possibly not so well above. We still have the solution to get rid of the CASTs (ticket:6654#comment:7) and that might be enough. Anyway, along with the TODO left in [8310], this is a performance improvement we can keep for later (0.12).

So … I think we're done with this :-) Great job everyone, starting with Javier Sanz who got the ball rolling!

[Remy Blank]

Replying to cboos:

I don't see how the common ancestor plays a role here.

In your example, we have 9,10 eligible from branch-1.1, regardless of what the common ancestor can be. It's the same situation as described in comment:28 for other-pre and other-post branches.

So what you're saying is that the eligible revisions are the revisions on the source since its creation, with a special case when the target is branched directly or indirectly from the source, in which case it should start at the branching point. Mmh, makes sense now, I must have got confused at some point.

Great, now I'm impatiently waiting for Manu's feedback with his 1000's of branches :-)

[Christian Boos]

While testing merge tracking in branch subfolders, I noticed that the algorithm introduced in r8310 for getting the "branch points" was not working in this situation. r8332 should fix this.

I also figured that in several places throughout the #7715 related changes, we don't take the scope into account. This will be fixed next.

[Emmanuel Blot]

I've started re-installing Trac from scratch on my desktop machine, to test this very feature - among others.

First news are … bad news: on my iMac 2.8GHz 4GB Core 2 Duo [Apache2, mod_wsgi, SVN 1.6.3, SQLite], it takes nearly 5 seconds to obtain a browser view with svn:mergeinfo properties (vs. ~ 2 seconds for a subdirectory w/o svn:merginfo properties)

It's far too long for a regular basis. I'm not sure if it's the machine or the latest property handler implementation.

• Server feat. w/ Core Duo, 4GB big mem, Debian, [Apache2, mod_wsgi, SVN 1.5.6, PostgreSQL] takes 2 seconds to render the same view, although it's running Trac r8263 with some property render patches
• iMac is running the pristine Trac r8335

I still need some time to finalize the installation (fighting with Mercurial, Queues and SVN import, …). I'll try to run the same config as the one on our server to compare with the exact same hardware. Still, 5 seconds is really too much

BTW there's a small UI glitch when toggling in and out the deleted branch: 'merged' to 'eligible' column spacing varies a bit (see attached images)

[Remy Blank]

Replying to eblot:

BTW there's a small UI glitch when toggling in and out the deleted branch: 'merged' to 'eligible' column spacing varies a bit (see attached images)

Yes, displaying the deleted branches makes the table wider, and therefore the column widths are recalculated. Maybe we could set the first two columns to minimum width, and only have the third expand?

[Remy Blank]

About performance: I think we can still improve it somewhat, possibly by caching some stuff in the Request object. OTOH, I would suggest that we move the merge property renderers into a separate class in the same module, and add a simple renderer (the one that was previously on trunk, which just adds zero-width spaces after commas) in SubversionPropertyRenderer, with a lower priority. This would allow disabling the fancy renderer if performance is an issue, and still get reasonable output.

Thoughts? Pre- or post-0.11.5?

[Emmanuel Blot]

Replying to rblank:

Thoughts? Pre- or post-0.11.5?

I think it is not mature enough for 0.11.5.

However, the previous renderer is simply ugly when a branch has received many branches. Either use <br> or some kind of option to hide the impossible-to-read property value… ;-)

[Christian Boos]

Replying to rblank:

I would suggest that we move the merge property renderers into a separate class in the same module, and add a simple renderer (the one that was previously on trunk, which just adds zero-width spaces after commas) in SubversionPropertyRenderer, with a lower priority. This would allow disabling the fancy renderer if performance is an issue, and still get reasonable output.

The above approach makes sense for 0.11.5, if we don't have a better way before the release.

Manu, just to be sure, were you using repository_type = svn or direct-svnfs? If the latter, please try with the former as well.

For improving performance, we could speed up the CachedRepository._get_node_revs queries, by avoiding the CAST using the padding with zeroes idea of #6654, but that's a 0.12 thing.

We could also do like I suggested in comment:29 and don't compute the eligible info when we know it will be costly (and we know that since SubversionNode.get_copy_ancestry is fast and gives the initial [first_rev-target_rev] range). In this case, the link would not directly show the eligible info but have a tooltip saying click to see eligible revisions… leading to /log/$path>?revs=$first_rev-$target_rev&excluding=$blocked and have the Log module compute the appropriate range (I have to check but we might even omit the call to _get_node_revs there, as we're going anyway through get_history). #8349 would really be needed, in this case.

[anonymous]

Replying to cboos:

Manu, just to be sure, were you using repository_type = svn or direct-svnfs? If the latter, please try with the former as well.

I've only use the default one, svn: I did not even know about direct-svnfs.

Here are some figures. I've used the timeit Python module to retrieve the browser view of our trunk, i.e. authenticate then download http://<server>/trac/sdk/browser/trunk

I've run it several times each (and discarded the first download timings after an Apache SIGHUP)

Round	ms, [8263]	ms, [8335]
1	143.790007	5433.205843
2	144.277096	6102.168083
3	145.091057	5436.594009
4	145.329952	5503.677130
5	145.175934	5403.434992
6	146.250010	5527.288914
7	144.883156	5553.313971
8	144.109964	5562.607050
9	144.565105	5567.209005

So it's about 40 slower than it used to be (with a Core 2 Duo iMac)

[Remy Blank]

Split renderers into distinct components.

[Remy Blank]

Replying to rblank:

I would suggest that we move the merge property renderers into a separate class in the same module, and add a simple renderer (the one that was previously on trunk, which just adds zero-width spaces after commas) in SubversionPropertyRenderer, with a lower priority.

That's what attachment:7715-component-split-r8342.patch​ does. Actually, the property and the diff renderers are in separate components, so that they can be disabled individually. This allows disabling the fancy renderer for cases where performance is too bad.

I'd say that this change, together with a fix taking into account the repository scope, should be enough for 0.11.5. Optimization can be done later.

BTW, the diff renderer could be optimized by only performing costly computations on the lines that have actually changed.

[Remy Blank]

BTW, performance on t.e.o is excellent, on both trunk and 0.11-stable! This feature will be really useful.

[Christian Boos]

Replying to Manu:

So it's about 40 slower than it used to be (with a Core 2 Duo iMac)

Could you please retry with attachment:7715-optimize-eligible-r8342.diff​?

This implements a slightly different approach for computing the eligible set:

• we first get the range of changesets that are susceptible to be merged (as per comment:28), using the informations provided either by get_copy_ancestry or the new get_branch_origin used for unrelated source paths; both should be fast
• we subtract the already merged and blocked revisions
• iff there's something left in the eligible range, we then call _get_node_revs, starting with the lowest revision from the above eligible set

So with this approach, the eventually slow _get_node_revs is not even called when there's no eligible changesets left and in other cases is called for a smaller range. For example if on a target branch we look for changes eligible from the trunk source and the merged range is already say 1-25043, we now start the _get_node_revs on 25043 instead of 1.

[Christian Boos]

A hopefully faster way to compute eligible changesets, patch on 0.11.5rc1

[Remy Blank]

Fix for scoped repositories.

[Remy Blank]

The patch above fixes rendering of merge properties when the repository is scoped. It also hides entries whose source path is outside of the scope.

As we already have a few post-0.11.5rc1 fixes, I assume there is going to be a 0.11.5rc2. Ok to commit?

[anonymous]

Replying to cboos:

Replying to Manu: Could you please retry with attachment:7715-optimize-eligible-r8342.diff​?

Sorry, back from short holidays. I was about to give a try to this new patch, but I'm lost:

• Is there a patch against the trunk (0.12dev)?
• What is the series of patch to apply here? [I finally managed to install and use Mercurial Queues, it will hopefully help with this long lasting series of patches]

[Remy Blank]

Replying to anonymous (presumably eblot):

• Is there a patch against the trunk (0.12dev)?

Not that I know of, but I think attachment:7715-optimize-eligible-r8342.diff​ should apply cleanly to trunk as well.

• What is the series of patch to apply here?

Everything up to the last three patches is already in 0.11-stable. A few merges are missing in trunk, though. I'll do that right away.

The patch above is an optimization and should be applied alone. The other two patches are a fix for scoped repositories and a cleanup, and hence should not be necessary for your tests.

[Remy Blank]

There you go, since [8349] the relevant changesets have been merged from 0.11-stable to trunk and the patch above applies cleanly with some fuzz.

[Remy Blank]

The fix for scoped repositories has been committed in [8351]. The renderers have been split into separate components in [8352].

Optimization will be done post-0.11.5, so I suggest we close this ticket, which has already way too many patches attached, and open a new one for the optimization work.

[Emmanuel Blot]

Replying to rblank:

Replying to anonymous (presumably eblot):

• Is there a patch against the trunk (0.12dev)?

Not that I know of, but I think attachment:7715-optimize-eligible-r8342.diff​ should apply cleanly to trunk as well.

I've not been able to apply it to the trunk. I give up for now, I'm really short of time.

[Emmanuel Blot]

Replying to eblot:

I've not been able to apply it to the trunk. I give up for now, I'm really short of time.

Or I'm simply stupid and the patch has already been applied to the trunk as well. Sorry.

[Remy Blank]

Replying to eblot:

Or I'm simply stupid and the patch has already been applied to the trunk as well. Sorry.

No, it hasn't. I thought I had it apply cleanly yesterday to the (almost) latest trunk@8349​, but I may have missed something. Sorry about the trouble. I'll refresh the patch for 0.11-stable and trunk tonight, and track the optimization in a new ticket.

[Emmanuel Blot]

Replying to rblank:

Replying to eblot: Sorry about the trouble.

No problem

I'll refresh the patch for 0.11-stable and trunk tonight, and track the optimization in a new ticket.

I already created it: #8459