Summary

Issue 451: handle svn switch'd paths

Review Request #347 — Created April 5, 2008 and submitted

Information

Owner

dan

Repository

Review Board SVN (deprecated)

Branch

Bugs

451

Depends On

Reviewers

Groups

reviewboard

People

chipx86

Description

Fixes:

http://code.google.com/p/reviewboard/issues/detail?id=451

Server code should still be backwards compatible with old unmodified versions of post-commit, as it only assumes the diff paths are absolute if they begin with a leadling slash

I've only just started using this software and I don't really know my way around the code yet, so I don't know if I've done this the right way by you guys... please let me know :) Also I'm not sure what people/groups this should be going to, but I picked some at random :)

Testing Done

Made sure server still works with old version of post-review

Tested posting & reposting a review for unswitched and switched files, some addd, some modified, some deleted.

svn only

dan April 15, 2008, 12:27 a.m.

Thanks for the review! Looking forward to familiarising myself further and helping out more

DA

dan April 15, 2008, 11:07 p.m.
```
Hopefully should be ok now?
```

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

Instead of setting front and line based on fixed indexes, you should be able to do:

front, line = line.split(" ", 1)

You will then have to append the space later, but this makes it a lot cleaner, as this code condenses to:

if line.startswith("+++ ") or line.startswith("--- ") or line.startswith("Index: "):
    front, line = line.split(" ", 1)

DA

dan April 15, 2008, 12:08 a.m.
```
Great, done
```

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

When would this evaluate to false?

dan April 15, 2008, 12:09 a.m.

When the line doesn't start with any of the above prefixes

chipx86 April 15, 2008, 9:41 a.m.

Oh, right. I was clearly reviewing this while tired :)

/trunk/reviewboard/contrib/tools/post-review (Diff revision 2)
```
No need for the []
```
1. DA
  
  dan April 15, 2008, 12:09 a.m.
  Done

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

I'd rather you compute all this in SVNClient.get_repository_info.

dan April 15, 2008, 12:10 a.m.

Would be nice... what do you propose? If I'm understanding what you're asking correctly, I can only see two ways: send additional meta data up along with the diff, or depend on SVNClient. I'm guessing the latter isn't really an option. The data we get from svn info is not available on the server, because only a checkout knows what paths have been switched.

chipx86 April 15, 2008, 9:46 a.m.

Right.. I had read this wrong. I saw the "svn info" but wasn't thinking in terms of "svn info <filename>". What you have is probably fine.

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

I'd instead do:

key, value = info.split(": ", 1)

if key == "URL":
   url = value
elif key == "Repository Root":
   root = value

dan April 15, 2008, 12:20 a.m.

Done. Slightly differently because not all lines have a colon in them.

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

You're going to want to use the urlparse module to build the URL, I think, so we don't end up with "../" and stuff in the URL.

dan April 15, 2008, 12:10 a.m.

Can you think of when this might occur? I'm fairly certain the outputs we get from the svn command will give us a nice normalised path.

chipx86 April 15, 2008, 9:53 a.m.

Yeah. It's more of a precaution, but it's probably fine to address that down the road if we do hit it.

I thought there was a case where svn would generate "../" paths.. Maybe diffing between branches?

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

I'm not a huge fan of the "x and y or z" expression. Can you break this apart a little? I'm also not sure what this particular expression is doing.

dan April 15, 2008, 12:10 a.m.

It's just putting back the old bits of the line. I've factored out the logic into parse_filename_header now, so it returns the interesting bit (filename) and the rest, whatever that is.

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

Why not just build the string instead of an array and then converting that to a string?

dan April 15, 2008, 12:11 a.m.

I was making the assumption that python arrays use size doubling and so it's ok to append to them, whereas just building a string would cause the time taken to grow quadratically, which could be bad for particularly large diffs. I'm no python guru though... and I guess I'm also making big assumptions about the implementation of join...

cuviper April 15, 2008, 1:19 a.m.

PEP 8 recommends this append-join style to improve compatibility with other Python runtimes.  Apparently, not all have an efficient in-place string concatenation like CPython's.

chipx86 April 15, 2008, 9:54 a.m.

Okay, in that case I'm fine with the join.

DA

dan April 15, 2008, 11:07 p.m.
```
Ok, I've reverted it
```

/trunk/reviewboard/diffviewer/forms.py (Diff revision 2)

No need for the outer parens.

I wonder if we'll have to account for anything other than "/". Probably okay for now. Does this take care of bug #446?

Also, could you put the bug numbers this change affects in the bug list on the review request?

dan April 15, 2008, 12:12 a.m.

Done

With regards to accounting for anything else... hopefully not? There's issue #121 which might be related, but I haven't yet delved into the source enough to fully understand that problem.

446: I don't use either of those tools, so I can't be sure, but it appears that it should fix it from reading the description.

chipx86 April 15, 2008, 9:54 a.m.
```
Cool.
```

Ship it!

Looks good - there are a couple potential things Christian pointed out, but I think those can be fixed if they ever turn out to be a problem.

Committed as r1311.  Thanks!