2359: Review creation fails with patches created with Subversion 1.7.x if they include a property change.

timva*****@gmai***** (Google Code) (Is this you? Claim this profile.)
chipx86
chipx86
Sept. 27, 2012
2396, 2434, 2547, 2561, 2660, 2729
What version are you running?

Reviewboard 1.6.1
Subversion 1.7.1

What's the URL of the page containing the problem?

reviews/r/new

What steps will reproduce the problem?
1. Install Subversion 1.7.x
2. In a working copy, make a change to an svn property (e.g. svn:externals).
3. Use svn diff to create a patch and attempt to create a review with it, or use post-review.

What is the expected output? What do you see instead?

Expected output is a successfully created review. What I see instead for a propchange on a directory, is this error:

URL 'http://PATH/TO/SVN/LOCATION/trunk' refers to a directory

or, for a propchange on a file:

The file 'http://PATH/TO/SVN/LOCATION/trunk/SomeFile.cs' (r198207) could not be found in the repository

What operating system are you using? What browser?

Windows 7, Chrome.

Please provide any additional information below.

The newly release Subversion 1.7 has a change in the way "svn diff" works for property changes. They are now shown in unidiff format. See here:

http://subversion.apache.org/docs/release-notes/1.7.html

Reviewboard does not handle this format correctly.
#1 timva*****@gmai***** (Google Code) (Is this you? Claim this profile.)
Anyone? This seems like a major show stopping bug to me. A lot of people use Subversion...
#2 elguero********@gmai***** (Google Code) (Is this you? Claim this profile.)
Here is a sample header extracted from a diff that has property changes and fails when being uploaded to ReviewBoard.  Hopefully this helps someone to track this down.  

I wonder if pysvn needs to be updated to use svn 1.7.x since ReviewBoard uses that to parse the diff file.  The devs for pysvn seem to be working on an update for svn 1.7.x.

Index: library/ACS/Filter/Date
===================================================================
--- library/ACS/Filter/Date	(revision 2210)
+++ library/ACS/Filter/Date	(revision 2214)

Property changes on: library/ACS/Filter/Date
___________________________________________________________________
Added: bugtraq:label
## -0,0 +1 ##
+Issue #
Added: bugtraq:message
## -0,0 +1 ##
+issue %BUGID%
Added: bugtraq:warnifnoissue
## -0,0 +1 ##
+true
Added: bugtraq:url
## -0,0 +1 ##
+http://xxxxxx.com/mantis/view.php?id=%BUGID%
Index: library/ACS/Pdf/Order.php
===================================================================
--- library/ACS/Pdf/Order.php	(revision 2210)
+++ library/ACS/Pdf/Order.php	(revision 2214)
#3 alexei.a*********@gmai***** (Google Code) (Is this you? Claim this profile.)
I have recently upgraded to Subversion 1.7.x and faced the same issue.  Currently it forces me to manually strip out items for directories from the diff file, this is ridiculous.
#4 craig*****@gmai***** (Google Code) (Is this you? Claim this profile.)
We are also suffering from the same issue, this is brutal has there been any mention on when it may be fixed?
#6 phok*****@gmai***** (Google Code) (Is this you? Claim this profile.)
We have been using ReviewBoard for a long time with SVN and really like it. Unfortunately, we recently upgraded our server to SVN 1.7.2 and started hitting this issue also (Using the latest ReviewBoard 1.6.3).

This is a big blocker. Almost seems like it should be a critical RB defect to support SVN 1.7.
#8 elguero********@gmai***** (Google Code) (Is this you? Claim this profile.)
FYI for those following this issue.  A new release of Pysvn was just released which supports svn 1.7.3.  Haven't been able to test it yet.
#9 ldela*****@gmai***** (Google Code) (Is this you? Claim this profile.)
new pysvn seems to go with python 3.2
review board tools are still on python 2.7.
Conclusion : review board post-review cannot yet be used with svn 1.7 clients ?
#10 elguero********@gmai***** (Google Code) (Is this you? Claim this profile.)
Hmm... according to the release notes, that shouldn't be the case.

http://pysvn.tigris.org/ds/viewMessage.do?dsForumId=1333&dsMessageId=2930786
"The kits have been built against SVN 1.7.3 and SVN 1.6.17 release
for python 2.6, 2.7 and 3.2."

I have not been able to test yet.
#12 denis.b********@gmai***** (Google Code) (Is this you? Claim this profile.)
Source of the problem is changed format of [svn diff] command, now it includes header for each changed directory while all RB-related code uses such headers as file header.

SVN 1.7.x format:
----8<--------8<--------8<--------8<--------8<--------8<--------8<----
Index: test-dir-props
===================================================================
--- test-dir-props      (revision 10223)
+++ test-dir-props      (working copy)

Property changes on: test-dir-props
___________________________________________________________________
Added: PROP-NAME
## -0,0 +1 ##
+PROP-VALUE
----8<--------8<--------8<--------8<--------8<--------8<--------8<----

Old SVN 1.6.x format:
----8<--------8<--------8<--------8<--------8<--------8<--------8<----
Property changes on: test-dir-props
___________________________________________________________________
Added: PROP-NAME
## -0,0 +1 ##
+PROP-VALUE
----8<--------8<--------8<--------8<--------8<--------8<--------8<----

I've attached simple script which you can use instead of [svn diff] command in working directory. Script calls [svn diff] by itself, cuts SVN 1.7.x extra directory headers and print result to stdout. You can grab/pipe it to file and then upload to ReviewBoard.

In case you use Subversion's pre-commit hook its easy to pass transaction's dirs-changed list to function strip_svn17x_extra_dirs_info() and then upload resulting diff to ReviewBoard.
ATTENTION: you can call this function in hook after expanding all diff paths to absolute ones.
  • +
    #!/usr/local/bin/python
    # -*- coding: utf-8 -*-
    """
    Inspired by migration to SVN Client 1.7.x
    Details:
    http://code.google.com/p/reviewboard/issues/detail?id=2359
    """
    import os
    import subprocess
    import sys
    import tempfile
    def out(cmds, split_lines=True):
        """
        get command, return list of lines (with EOL) or string
        """
        fd, pathname = tempfile.mkstemp()    
        os.close(fd)
        fh = open(pathname, 'r+')
        p = subprocess.Popen(cmds, stdout=fh,
                             env={'LANG':'en_US.UTF-8'})
        ret = p.wait()
        if ret != 0:
            raise Exception("Process ret=%d" % (ret))
        fh.seek(0)
        if split_lines:
            data = fh.readlines()
        else:
            data = fh.read()
        fh.close()
        os.unlink(pathname)
        return data
    DIFF_PREFIXES = ['=' * 67, '--- ', '+++ ']
    INDEX_PREFIX = 'Index: '
    def strip_svn17x_extra_dirs_info(diff_content, dirs_changed=None):
        """
        tries to remove extra SVN 1.7.x header for changed directories
        Its easy to pass a l
#13 dhum*****@gmai***** (Google Code) (Is this you? Claim this profile.)
Can you post a version that supports revision ranges?
#14 denis.b********@gmai***** (Google Code) (Is this you? Claim this profile.)
New version pass CLI arguments to [svn diff].

python cast-svn17x-diff-to-svn16x_ver2.py -r 10:15

of course, it wont work with options like --xml :)
  • +
    #!/usr/local/bin/python
    # -*- coding: utf-8 -*-
    """
    Inspired by migration to SVN Client 1.7.x
    Details:
    http://code.google.com/p/reviewboard/issues/detail?id=2359
    """
    import os
    import subprocess
    import sys
    import tempfile
    def out(cmds, split_lines=True):
        """
        get command, return list of lines (with EOL) or string
        """
        fd, pathname = tempfile.mkstemp()    
        os.close(fd)
        fh = open(pathname, 'r+')
        p = subprocess.Popen(cmds, stdout=fh,
                             env={'LANG':'en_US.UTF-8'})
        ret = p.wait()
        if ret != 0:
            raise Exception("Process ret=%d" % (ret))
        fh.seek(0)
        if split_lines:
            data = fh.readlines()
        else:
            data = fh.read()
        fh.close()
        os.unlink(pathname)
        return data
    DIFF_PREFIXES = ['=' * 67, '--- ', '+++ ']
    INDEX_PREFIX = 'Index: '
    def strip_svn17x_extra_dirs_info(diff_content, dirs_changed=None):
        """
        tries to remove extra SVN 1.7.x header for changed directories
        Its easy to pass a l
#15 dhum*****@gmai***** (Google Code) (Is this you? Claim this profile.)
It worked after I removed the setting of the env parameter in the call to Popen.  With it in there my default environment variables were not set and an older version of SVN ran.  Does it matter if LANG is set?
#17 denis.b********@gmai***** (Google Code) (Is this you? Claim this profile.)
> Does it matter if LANG is set?

It depends on files and properties -- if they have non-ascii characters then it can be exported in diff even in base64-encoded form.
So I suggest use utf-8 under unix/linux :).
#20 ben.m*****@gmai***** (Google Code) (Is this you? Claim this profile.)
My development team uses 'feature branches' so almost every time we generate a diff for review it contains svn:mergeinfo property changes.  We have to manually strip these out of the diff and it makes the post-review tool unusable.  This is probably the biggest usability issue that we have. I recommend making this issue a high priority.
#22 xSy***@gmai***** (Google Code) (Is this you? Claim this profile.)
@Denis
How to use your patch file with post-review.exe?

I'm using RB as review tool for every commit, so I have SVN post-commit-hook that generates review using:
post-review --repository-url=$repoURL --revision-range=$prev:$rev --summary=\"$summary\" --server=$reviewBoardURL --username=$reviewBoardUser --password=$reviewBoardPwd --submit-as=$auth -p --description-file=$descfile

for each commit.

Thanks
#23 denis.b********@gmai***** (Google Code) (Is this you? Claim this profile.)
Hi,

my patch is for altering diff file in manual way and then uploading to ReviewBoard.

I never used post-review.exe, but if there is some option for specifying diff utility available, it is possible to create simple batch script which will:
1. get diff to temporary file via 'svn diff' command
2. alter it by my patch
3. return to post-review.exe

Regards,
Denis
#24 huangsha********@gmai***** (Google Code) (Is this you? Claim this profile.)
SVN property should be treated as code modification. When submmiter change the value of svn:externals, it really make a big change to the code base and must be reviewd.

I hope we can have a patch on ReviewBoard server or on RBTools, althouth it may introduce some big change for the implementation of ReviewBoard.
#26 ma**@zieg.***** (Google Code) (Is this you? Claim this profile.)
Just wanted to add that this issue caused me embarrassment on the very day I was publicly demonstrating the value of ReviewBoard to our company software manager.  Not good.
chipx86
#27 chipx86
A fix will be out tonight.
  • +PendingReview
  • -Priority-Medium
    +Priority-Critical
    +Milestone-Release1.6.x
    +Component-DiffParser
  • +chipx86
#30 mbra*****@gmai***** (Google Code) (Is this you? Claim this profile.)
[Sorry for the double-post: the first one had the wrong email address on it.]

I tried pulling down the patch, and it looks like there's still a bit more work needed before the bug can be completely closed.  Of course, I could have pulled it in wrong (I'm using 1.5, until we get some things in place to do an upgrade.)

One of the diffs I tried feeding it updated the svn:mergeinfo property, since I had merged in the trunk changes onto this branch:
Index: .
===================================================================
--- .   (.../base/mainline/foo)       (revision 249342)
+++ .   (.../uid/mbraun/293895) (revision 249342)

Property changes on: .
___________________________________________________________________
Modified: svn:mergeinfo
   Merged /components/foo/base/mainline/foo:r248401-248793

When I tried uploading it, RB refused complaining:
   URL 'https://XXX/components/foo/base/mainline/foo' refers to a directory 
   Can't get text contents of a directory
chipx86
#31 chipx86
This is with SVN 1.7?

I'll play around with this more before we release it.
#32 timva*****@gmai***** (Google Code) (Is this you? Claim this profile.)
I just want to express how excited I am that this is finally being addressed. After almost a year of manually editing patch files this will be a very welcome fix. :)

If there's an way I can help test a fix, please let me know.
chipx86
#33 chipx86
Yeah, I dropped the ball, but you'll hopefully never have to deal with this again :)

I'll have a new patch up before long if people want to pull it down and give it a try. Stay tuned to http://reviews.reviewboard.org/r/3347/
chipx86
#34 chipx86
There's a new diff up if you guys can give it a shot. I'd like to release tonight if possible.
#35 timva*****@gmai***** (Google Code) (Is this you? Claim this profile.)
Just FYI, it looks like this was released as part of 1.6.12. :)
david
#36 david
Yep.
  • -PendingReview
    +Fixed
#37 lroze******@gmai***** (Google Code) (Is this you? Claim this profile.)
Hello guys, was this bug really fixed? We've installed ReviewBoard 1.7.11 (and we're using SVN client 1.8, while SVN server 1.6), and the problem still exists.
#38 denis.b********@gmai***** (Google Code) (Is this you? Claim this profile.)
We patch diff using third-party module for diff parsing:
http://bazaar.launchpad.net/~bzr-pqm/bzr/bzr.dev/view/head:/bzrlib/patches.py

We have moved to RB 1.6.11 but still upload patched diff with property changes stripped off.
#39 lroze******@gmai***** (Google Code) (Is this you? Claim this profile.)
I see -> It means it's not fixed (as for me). Probably I'll start some discussion in mailing list - since I'm not sure what's the policy about commenting close issues here.
#40 elguero********@gmai***** (Google Code) (Is this you? Claim this profile.)
We are using latest RB 1.7 with SVN 1.7 without any issues.  This issue was fixed.

I seem to recall reading in the release notes of SVN 1.8, there may have been changes made again to the output of svn diff.

If I were you, I would open a new issue against SVN 1.8 with the appropriate information, perhaps a sample header, to help with any necessary code changes in order to work with patches produced by SVN 1.8.
#41 lroze******@gmai***** (Google Code) (Is this you? Claim this profile.)
Thank you! I've already started a discussion in the google group https://groups.google.com/forum/#!topic/reviewboard-dev/--bazsEla5I
probably as a result of this discussion there might be an issue created