Edgewall Software
Modify

Opened 14 years ago

Closed 14 years ago

Last modified 11 years ago

#9622 closed defect (fixed)

Subversion mergeinfo rendering - need to support asterisk

Reported by: osimons Owned by: Christian Boos
Priority: normal Milestone: 0.12.1
Component: version control Version: 0.12dev
Severity: normal Keywords: svn mergeinfo
Cc: Branch:
Release Notes:

svn: support displaying non-inheritable svn:mergeinfo properties

API Changes:
Internal Changes:

Description (last modified by Remy Blank)

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.

Attachments (2)

9622-non-inheritable-r10116.patch (2.7 KB ) - added by Christian Boos 14 years ago.
show non-inheritable mergeinfo
9622-non-inheritable-diff-renderer.patch (7.1 KB ) - added by Christian Boos 14 years ago.
also display details about modifications of non-inheritable mergeinfo in changesets (applies on 9622-non-inheritable-r10116.patch)

Download all attachments as: .zip

Change History (23)

comment:1 by Remy Blank, 14 years ago

Milestone: next-minor-0.12.x

I suggest we do this in 0.12.x.

comment:2 by Christian Boos, 14 years ago

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?

comment:3 by osimons, 14 years ago

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.

comment:4 by cmpilato@…, 14 years ago

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

comment:5 by osimons, 14 years ago

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

    a b  
    167167            spath = _path_within_scope(repos.scope, path)
    168168            if spath is None:
    169169                continue
    170             revs = revs.strip()
     170            # FIXME: Ignoring information about non-inherited merges for now...
     171            revs = revs.strip().replace('*', '')
    171172            deleted = False
    172173            try:
    173174                node = repos.get_node(spath, target_rev)

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

in reply to:  5 ; comment:6 by Remy Blank, 14 years ago

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…

in reply to:  5 ; comment:7 by Christian Boos, 14 years ago

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

    a b  
    167167            spath = _path_within_scope(repos.scope, path)
    168168            if spath is None:
    169169                continue
    170             revs = revs.strip()
     170            # FIXME: Ignoring information about non-inherited merges for now...
     171            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] != '*']
Last edited 14 years ago by Christian Boos (previous) (diff)

in reply to:  6 comment:8 by Christian Boos, 14 years ago

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" :-)

comment:9 by Christian Boos, 14 years ago

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

in reply to:  7 comment:10 by osimons, 14 years ago

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.

comment:11 by osimons, 14 years ago

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.

in reply to:  11 comment:12 by Christian Boos, 14 years ago

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.

by Christian Boos, 14 years ago

show non-inheritable mergeinfo

comment:13 by Remy Blank, 14 years ago

I assume you will also fix the SubversionMergePropertyDiffRenderer?

comment:14 by Christian Boos, 14 years ago

Forgot about this ;-)

by Christian Boos, 14 years ago

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

comment:15 by osimons, 14 years ago

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?

comment:16 by Christian Boos, 14 years ago

Keywords: svn mergeinfo added
Owner: set to Christian Boos
Release Notes: modified (diff)

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.

comment:17 by Christian Boos, 14 years ago

Milestone: next-minor-0.12.x0.12.1

comment:18 by osimons, 14 years ago

Description: modified (diff)
Summary: Subversion mergeinfo rendering - need to support asterixSubversion mergeinfo rendering - need to support asteriks

typo: 'asterix' → 'asteriks'

comment:19 by Remy Blank, 14 years ago

Description: modified (diff)
Summary: Subversion mergeinfo rendering - need to support asteriksSubversion mergeinfo rendering - need to support asterisk

Isn't that "asterisk"?

comment:20 by osimons, 14 years ago

Gaaaaahhhh….

comment:21 by Christian Boos, 14 years ago

Resolution: fixed
Status: newclosed

Committed in [10135:10136].

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Christian Boos.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Christian Boos 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.