Summary

Fixes bug 2134

Review Request #2441 — Created June 29, 2011 and submitted Sept. 30, 2012, 7:51 a.m.

Information

Owner

NateBragg

Repository

Review Board

Branch

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

Fixes bug 2134

diffs were being created for files where the area surrounding keywords
had changed, and the keywords were not being collapsed.  This was
happening for two reasons:
  * it couldn't understand paths with two leading slashes
  * `svn propget svn:keywords` can return strings with non-space delimiters

Also, I had submitted this previously as a pull request on github - sorry about that.
I couldn't find your policy for contributions, so I just figured that was it.  You might
want to make that a bit more visible.

Testing Done

Using reviewboard 1.5.5 on a Windows 7 server using Apache+mod_wsgi running on python 2.7 with subversion 1.6.15 (if that matters)

NateBragg July 4, 2011, 10:35 p.m.

Wow, it posted both of my comments mid-edit... sorry about double posting there, guess that's what I get for looking up more information... take the more recent ones as the intended ones.

reviewboard/scmtools/svn.py (Diff revision 1)

Test case?

NateBragg July 4, 2011, 10:28 p.m.

It would be simple - rev the scmtools/testdata/svn_repo with different characters in between the tokens in the svn:keywords property (mine, for example, has newlines), and then rerun `testKeywordDiff` on that new rev.  Sadly, I can't do this - for some reason svn will not let me checkout that repository. :^\

NateBragg July 4, 2011, 10:32 p.m.

It would be simple - rev reviewboard/scmtools/testdata/svn_repo with different characters in between the tokens in the svn:keywords property (mine, for example, has newlines), and then rerun `reviewboard.scmtools.tests.SubversionTests.testKeywordDiff` on that new revision.  Sadly, I can't seem to do this myself to submit it - for some reason svn will not let me checkout that repository - it claims "unknown FS type fsfs". :^\

reviewboard/scmtools/svn.py (Diff revision 1)

What kind of path is this?  I guess it's an abstract POSIX-style path?  Or is it defined in rb as a URI path component or something?  Either way, I guess // is a valid path.  I'm asking because if it's an invalid path rather than just a de-normalized path, the bug should presumably be fixed elsewhere.

If // is a valid path, then why not "///" (etc.)?  If the code wants a path normalized such that double slashes don't appear, probably want to do path = posixpath.normpath(path) right at the top of this function.

Test case would be nice here, too :-)

NateBragg July 4, 2011, 9:48 p.m.

Yeah, that's a good question.  Our svn server doesn't return paths like that - but it's what gets put into the database when the review request is created using post-review.exe.  I tried going down that path, but I didn't have the time to devote to learning it well enough to root it out from that side.

NateBragg July 4, 2011, 10:29 p.m.

Yeah, that's a good question.  Our svn server doesn't return paths like that - but it's what gets put into the database when the review request is created using post-review.exe.  I tried going down that path, but I didn't have the time to devote to learning it well enough to root it out from that side.  If you wanted to try to rout this in parallel, let me know since I can reproduce it easily on my side.

jjlee July 8, 2011, 3:48 a.m.

From the code, I assume the path is intended to be a POSIX path -- but it looks like it just takes whatever string you give it in your patch file.  I don't see any filtering of "..", so I guess it's a good thing the "local file" SCM backend isn't enabled by default! (that backend isn't intended to be used in production, of course)

So I think you're missing a posixpath.normpath.

```
??
```

```
very good
```

Ship it!

```
Ship It!
```

reviewboard/scmtools/svn.py (Diff revision 1)

I don't know if it's the intended behavior, but the result of `posixpath.normpath` on my system (with is python 2.7.1, windows XP):

    >>> posixpath.normpath("//something/something/else/")
    '//something/something/else'

So, that doesn't actually seem to correct that problem...

Ship it!

reviewboard/scmtools/svn.py (Diff revision 1)
```
good job
```

reviewboard/scmtools/svn.py (Diff revision 1)

what the hell is this?

NateBragg July 31, 2011, 9:16 p.m.

I'm not sure what you mean; I did not change this line.

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-1.6.x (3d5ad9c). Thanks!

Ship it!

```
Ship It!
```

Ship it!

Ship It!

mike_conley Jan. 20, 2016, 6:57 a.m.

Hey ma_lingshu,

This fix already shipped in one of the point-releases for Review Board 1.6. Note that this is a production instance of Review Board and should not be used for testing purposes. If you want to try a demo instance of Review Board, please use http://demo.reviewboard.org/