Subversion mergeinfo rendering - need to support asterisk

[osimons]

I see this in my logs sometimes:

changeset.py:826 WARNING: Diff rendering failed for property svn:mergeinfo with renderer SubversionMergePropertyDiffRenderer: 
Traceback (most recent call last):
  File "/..../Trac-0.12.1dev_r10090-py2.6.egg/trac/versioncontrol/web_ui/changeset.py", line 821, in render_property_diff
    options)
  File "/..../Trac-0.12.1dev_r10090-py2.6.egg/trac/versioncontrol/svn_prop.py", line 277, in render_property_diff
    old_sources = parse_sources(old_props)
  File "/..../Trac-0.12.1dev_r10090-py2.6.egg/trac/versioncontrol/svn_prop.py", line 275, in parse_sources
    sources[spath] = set(Ranges(revs.strip()))
  File "/..../Trac-0.12.1dev_r10090-py2.6.egg/trac/util/__init__.py", line 707, in __init__
    self.appendrange(r)
  File "/..../Trac-0.12.1dev_r10090-py2.6.egg/trac/util/__init__.py", line 718, in appendrange
    a, b = int(x), int(x)
ValueError: invalid literal for int() with base 10: '6164-6259*'

From what I see and read, I think we need to handle the fact that sometimes an asterisk gets appended to mergeinfo. From ​background information: "This '*' is the marker for a non-inheritable mergeinfo range. The '*' means that only the path on which the mergeinfo is explicitly set has had this range merged into it"

What we need to do, I suppose, is to strip the '*' before we try to handle the range.

[Remy Blank]

I suggest we do this in 0.12.x.

[Christian Boos]

I'm curious, as I never saw '*' in svn:mergeinfos: is that "variant" still written by current svn merge (and how to trigger this?) or is it a remnant from 1.5 days experiments?

[osimons]

I have a feeling it isn't actually written to mergeinfo at that level, but more a marker for a partial merge at some subpath(s). I don't see the asterix written anywhere in properties. As for version, I've got 1.5.7 on the server.

[cmpilato@…]

The asterisk *is* stored in the actual svn:mergeinfo property value. As noted previously, it represents non-inheritable mergeinfo. Here's a recipe script to help you generate this kind of mergeinfo:

#!/bin/sh
rm -rf mergeinfo-demo
mkdir mergeinfo-demo
cd mergeinfo-demo
svnadmin create repos
svn co file://`pwd`/repos wc
cd wc
svn mkdir trunk trunk/dir
echo "foo" > trunk/foo
echo "bar" > trunk/dir/bar
svn add --force .
svn ci -m "Add initial data"
svn up
svn cp trunk branch
svn ci -m "Create a branch"
echo "another line" >> trunk/foo
echo "another line" >> trunk/dir/bar
svn ci -m "Add other lines."
svn merge --depth immediates ^/trunk branch
svn pget -vR svn:mergeinfo
cd ..
cd ..

[osimons]

Thanks! Checking in this version 4 to the repos then reveals the asterisk in svn:mergeinfo for /branch/dir when Trac tries to render it - with traceback like above.

The reality of this merge information is then that /branch/dir is merged with trunk, but not any of its children as /branch/dir/bar does not contain the latest "another line" content.

So, for Trac visualization purposes: Do we render the directory listing as merged or not? Or perhaps we need to add a third alternative so that we get:

Property svn:mergeinfo set to
 /trunk merged non-inherited eligible

The immediate fix to avoid the traceback would be to just ignore the asteriks in input so that it at least renders without exception. That patch looks something like this:

trac/versioncontrol/svn_prop.py

@@ -167 +167 @@
             spath = _path_within_scope(repos.scope, path)
             if spath is None:
                 continue
-            revs = revs.strip()
+            # FIXME: Ignoring information about non-inherited merges for now...
+            revs = revs.strip().replace('*', '')
             deleted = False
             try:
                 node = repos.get_node(spath, target_rev)

I'm tempted to commit this as an interim solution.

[Remy Blank]

Replying to osimons:

I'm tempted to commit this as an interim solution.

For the soon-to-be-released 0.12.1, I would tend to agree. I also have to admit that I still haven't completely understood what the asterisk means, despite having read the explanation five times…

[Christian Boos]

Replying to osimons:

… So, for Trac visualization purposes: Do we render the directory listing as merged or not?

As not merged, therefore eligible.

The immediate fix to avoid the traceback would be to just ignore the asteriks in input so that it at least renders without exception. That patch looks something like this:

trac/versioncontrol/svn_prop.py

@@ -167 +167 @@
             spath = _path_within_scope(repos.scope, path)
             if spath is None:
                 continue
-            revs = revs.strip()
+            # FIXME: Ignoring information about non-inherited merges for now...
+            revs = revs.strip().replace('*', '')

That would wrongly keep them part of the merged set. We should simply exclude them:

revs = [range for range in revs.strip().split(',') if range[-1] != '*']

[Christian Boos]

Replying to rblank:

Replying to osimons:

I'm tempted to commit this as an interim solution.

For the soon-to-be-released 0.12.1, I would tend to agree. I also have to admit that I still haven't completely understood what the asterisk means, despite having read the explanation five times…

This boils down to "we're not supporting inheritable mergeinfo" :-)

[Christian Boos]

And to save you from reading my comment 5 times, let's expand a bit ;-) We're only displaying mergeinfo on the files or folders that actually have the svn:mergeinfo property set. Subversion goes beyond that, svn mergeinfo is able to tell you what are the merged vs. eligible revs on any path, based on the values of the svn:mergeinfo on ancestor paths, more precisely by combining inheritable and non-inheritable ranges.

Let's expand a bit cmpilato's example from comment:4:

$ cd wc/branch
$ svn merge -c 3 ^/trunk/dir dir
--- Merging r3 into 'dir\bar':
U    dir\bar

$ svn diff dir/

Property changes on: dir
___________________________________________________________________
Added: svn:mergeinfo
   Merged /trunk/dir:r2*,3

Index: dir/bar
...

$ svn plist -v .
Properties on '.':
  svn:mergeinfo
    /trunk:2-3
$ svn mergeinfo --show-revs=eligible dir/bar
r1
r2

i.e. for computing the mergeinfo for branch/dir/bar, it took the merged range 2-3 from branch (eligible is [1]), filtered out the non-inheritable "range" 2 from branch/dir (eligble is [1-2]).

In contrast, in Trac we simply don't show anything for branch/dir/bar. For branch/dir, we would try to show something here as the property is defined, and we should show merged [3] and eligible [1-2].

[osimons]

Replying to cboos:

As not merged, therefore eligible.

That is not strictly true. If you adapt the example from cmpilato so that in addition to doing the final two edits, you also do this:

svn propset test trac trunk/dir

It will merge the property from trunk, so the directory itself is of course merged at the last revision - even though its children may not be.

[osimons]

I've concluded that we should consider it as merged, not eligible. cmpilato constructed a nice, simple example that illustrates the non-inherit indication by not merging below a certain depth. What I see is that it may just as well occur in a directory that is partially merged - like 19 of 20 files or folders in that directory. Then the parent will get the non-inherit marking, and 19 children will be tagged as merged leaving the sole non-merged cause of this construction without any mergeinfo.

From the Python Zen: "In the face of ambiguity, refuse the temptation to guess." What we know is that the revision(s) has been merged for the level we are currently rendering. = Merged.

[Christian Boos]

Replying to osimons:

From the Python Zen: "In the face of ambiguity, refuse the temptation to guess." What we know is that the revision(s) has been merged for the level we are currently rendering. = Merged.

But for the same reason, wouldn't your suggestion from comment:5 (show "merged non-inherited eligible") be better? If you use non-inheritable merge, isn't it precisely for indicating that you've not merged yet everything below?

I'm currently testing a patch that does this.

[Christian Boos]

show non-inheritable mergeinfo

[Remy Blank]

I assume you will also fix the SubversionMergePropertyDiffRenderer?

[Christian Boos]

Forgot about this ;-)

[Christian Boos]

also display details about modifications of non-inheritable mergeinfo in changesets (applies on 9622-non-inheritable-r10116.patch​)

[osimons]

Nice! Works well.

I thought such changes would also deserve some new tests, but I'm unable to find any tests at all for svn prop/propdiff rendering? Or really any repository property rendering as far as my search-foo takes me. Something for todo list?

[Christian Boos]

Thanks for testing!

Regarding the tests, Itamar added lots of tests for the svn:externals, in #7687, we should be able to augment that with tests on svn:mergeinfo afterwards.

[osimons]

typo: 'asterix' → 'asteriks'

[Remy Blank]

Isn't that "asterisk"?

[osimons]

Gaaaaahhhh….

[Christian Boos]

Committed in [10135:10136].