Summary

FIx for issue 121. svn cp/mv not handled well by review board.

Review Request #367 — Created April 26, 2008 and submitted

Information

Owner

jeeva

Repository

Review Board SVN (deprecated)

Branch

Bugs

121

Depends On

Reviewers

Groups

reviewboard

People

Description

This is primarily a patch for http://code.google.com/p/reviewboard/issues/detail?id=121 - along with some relevant refactorings.

Changes include
 * Adding function handle_renames which acts like a filter to fix the svn mv/cp issue.
 * Changed convert_absolute_paths to return a list of results like handle_renames for consistency.
 * Made convert_absolute_paths ignore files which where already had an absolute path
 * added a function svn_info which converts the results of 'svn info filename' to a dict and refactored convert_absolute_paths to use it.

Testing Done

I hacked this together because I had a changeset which did do a lot of svn mv/cp. So thats how I tested these changes.

I also checked that it still worked on vanilla changes without svn mv/cp.

This is mostly just nitpicking.  The implementation itself looks great.

jeeva April 27, 2008, 11:22 p.m.

Nitpicking is good. Its all too easy to miss the little stuff :)

/trunk/reviewboard/contrib/tools/post-review (Diff revision 1)

It would be nice to give a little bit more detail here.  The problem is really that the diff contains parent revisions for the "new" files.

Comments should also be properly sentence cased (start with a capital letter) and end with a period.

jeeva April 27, 2008, 11:21 p.m.

Done - although it's a bit wordy (its late here in Australia...)

/trunk/reviewboard/contrib/tools/post-review (Diff revision 1)

Should be a space between the # and the comment.  Also should be capitalized/punctuated.

JE

jeeva April 27, 2008, 11:06 p.m.
```
done.
```

/trunk/reviewboard/contrib/tools/post-review (Diff revision 1)
```
Trailing whitespace.
```
1. JE
  
  jeeva April 27, 2008, 11:07 p.m.
  deleted
/trunk/reviewboard/contrib/tools/post-review (Diff revision 1)
```
Space between # and We.  This should also be wrapped to 80 characters.
```
1. JE
  
  jeeva April 27, 2008, 11:08 p.m.
  fixed

This should also fix bug 288, right?

jeeva April 27, 2008, 11:22 p.m.

Only if you use post-review. I don't think we have enough information to fix this on the server so this is probably the closest we will get.

david April 28, 2008, 5:14 a.m.

Well, that's the same with the problem here -- if someone tries to use "svn diff" and the web UI, it'll break.

It might be nice to try to detect this situation and point people towards post-review.

This patch looks good now -- I've committed it as r1321.  Thanks!