Issue 451: handle svn switch'd paths

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


Review Board SVN (deprecated)



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 :)
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
    1. Thanks for the review! Looking forward to familiarising myself further and helping out more
    2. Hopefully should be ok now?
  2. /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)
  3. When would this evaluate to false?
    1. When the line doesn't start with any of the above prefixes
    2. Oh, right. I was clearly reviewing this while tired :)
  4. No need for the []
  5. /trunk/reviewboard/contrib/tools/post-review (Diff revision 2)
    I'd rather you compute all this in SVNClient.get_repository_info.
    1. 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.
    2. 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.
  6. /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
    1. Done. Slightly differently because not all lines have a colon in them.
  7. 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.
    1. 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.
    2. 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?
  8. 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.
    1. 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.
  9. /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?
    1. 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...
    2. 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.
    3. Okay, in that case I'm fine with the join.
    4. Ok, I've reverted it
  10. /trunk/reviewboard/diffviewer/ (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?
    1. 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.
    2. Cool.
  1. 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!