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.
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)
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.
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.
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.
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.
@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
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
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.
A fix will be out tonight.
- Priority-Medium + Priority-Critical + Milestone-Release1.6.x + Component-DiffParser
[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
This is with SVN 1.7? I'll play around with this more before we release it.
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.
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/
There's a new diff up if you guys can give it a shot. I'd like to release tonight if possible.
- PendingReview + Fixed
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.
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.