Edgewall Software
Modify

Opened 8 years ago

Closed 8 years ago

Last modified 5 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:
Release Notes:

svn: support displaying non-inheritable svn:mergeinfo properties

API 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 8 years ago.
show non-inheritable mergeinfo
9622-non-inheritable-diff-renderer.patch (7.1 KB ) - added by Christian Boos 8 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 Changed 8 years ago by Remy Blank

Milestone: next-minor-0.12.x

I suggest we do this in 0.12.x.

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

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

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

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

    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.

comment:6 in reply to:  5 ; Changed 8 years ago by 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…

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

    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 8 years ago by Christian Boos (previous) (diff)

comment:8 in reply to:  6 Changed 8 years ago by 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" :-)

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

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

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

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

Changed 8 years ago by Christian Boos

show non-inheritable mergeinfo

comment:13 Changed 8 years ago by Remy Blank

I assume you will also fix the SubversionMergePropertyDiffRenderer?

comment:14 Changed 8 years ago by Christian Boos

Forgot about this ;-)

Changed 8 years ago by Christian Boos

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

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

comment:16 Changed 8 years ago by Christian Boos

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 Changed 8 years ago by Christian Boos

Milestone: next-minor-0.12.x0.12.1

comment:18 Changed 8 years ago by osimons

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

typo: 'asterix' → 'asteriks'

comment:19 Changed 8 years ago by Remy Blank

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

Isn't that "asterisk"?

comment:20 Changed 8 years ago by osimons

Gaaaaahhhh….

comment:21 Changed 8 years ago by Christian Boos

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