What's the URL of the page containing the problem? http://reviews/r/18/diff/raw/ What steps will reproduce the problem? 1. svn diff > changes.diff 2. Create a review request 3. Download the new diff as rboard.diff What is the expected output? What do you see instead? changes.diff and rboard.diff should be same but they are not. rboard.diff doesn't apply cleanly on a fresh copy of svn base.
I'm thinking about submitting a patch to ReviewBoard that fixes this. It seems like reviewboard could simply save the uploaded diff somewhere, and return that to the user when the user asks to download it. Do you think it'd likely to be that simple? If so I will submit a patch that does this.
It does do this. It just breaks it up per-file (which is needed). The problem is that the parser doesn't know about some of the extra metadata. So really, the parser just needs to be fixed to not lose this.
I wonder if this wouldn't still be a good idea. I was trying to review http://reviews.reviewboard.org/r/1737/ the other day because I thought it might address this problem, at least for git. However, besides missing the mail headers from format-patch, it was also missing the new file mode lines, so even once I had an appropriate base to apply on, I had to manually edit the diff just to get it to apply. I suspect there's always going to be the risk of losing portions of a diff if you break up and process it and then try to recreate it. Saving a pristine 'patch' alongside the reviewboard internal diff segments might actually be a more robust approach. Whatever we do, we really need to do something. I was hoping Eduardo's patch would do this, but it definitely had some bugs in it and I'm a bit confused over the 'bundle' concept he introduced since a format-patch output is only a single patch...
Our initial designs never anticipated reviewboard being used as a "diff sharing location", merely as a code review tool. I agree it would be nice to fix but right now we're busy with other things. Unless someone steps up to do it, we don't have a timeline for fixing this.
All "git diff format support" tickets were closed as duplicate of this ticket. I don't know why but the need for git diff format support has more ramifications than just "sharing diffs": - It can describe move/rename operations which saves the reviewer's time to understand the "three pages of removed line block" and "three pages of added line block" were actually a single rename. - Mercurial Queues REQUIRE git-diff format to work correctly. It's recommended to be set it in the .hgrc. That prevents developer from being able to generate unified diff format and requires editing .hgrc file over and over for each specific goal. - When binary files are present, MQ switches to git format which cannot be turned off in newer Mercurial versions (namely 2.2.1).
Hi sedatk, The reason they were closed is that they're all symptoms of Review Board inadvertently stripping data from the diff when storing in the database. That's a key problem that must be fixed (and I believe we've done that for Git at least, but maybe not in a released version -- I forget when that landed). Can you describe for me how fixing that isn't sufficient for the cases you described?
Hi Chris, can you elaborate on how you changed diff handling for git? We have a user of reviewboard.kde.org who saw that all information above line 10 was stripped. I guess he was expecting to be able to see this information (e.g. commit message) even after he uploaded the patch to review board. Any news on this issue? David
It is not clear whether this issue has been closed or not. Comment #11 gives me the impression this has been fixed? Could you please let us know the version of RB that has the fix. We're desperately trying to get this to work at reviews.apache.org.
- NeedInfo + New
- New + PendingReview
- Priority-Medium + Priority-Critical + Milestone-Release1.6.x
This should be fixed properly now, and a release is going up tonight.
- PendingReview + Fixed
Pushed to release-1.6.x (8281c37c4fae1f5a283c735f1c7e7cf8546767ac)