Summary

Fix rbt patch for SVN backend

Review Request #7095 — Created March 20, 2015 and submitted March 20, 2015, 3:31 p.m.

Information

Owner

brennie

Repository

RBTools

Branch

release-0.7.x

Bugs

Depends On

~~7091~~

Blocks

7084

Commit

c5087c6...

Reviewers

Groups

rbtools

People

Description

Previously, rbt patch would not work with SVN repositories. This was
because the the value for patch's -p flag was being calculated
incorrectly for SVN repositories. This has been corrected and now we use
svn patch to apply the patch instead of the patch executable. This
allows patches containing SVN keywords to be applied.

Testing Done

Ran unit tests.

Successfully used rbt patch to apply a patch to an SVN repository.
Successfully used rbt patch to apply a patch to an SVN repository that
used SVN keywords (e.g. $Rev$ ).

Issues

Description	From	Last Updated
These parens aren't useful.	david	March 20, 2015, 2:57 p.m.
The logic in _get_p_number() does not seem correct in all cases, particularly when the working copy where the patch is …	gmyers	March 24, 2015, 2:30 p.m.
If I provide a --px NUM option on the command line, then I get an exception here: ... File "/xxxx/rbtools/rbtools/clients/svn.py", …	gmyers	March 24, 2015, 1:50 p.m.

Tool: PEP8 Style Checker
Processed Files:
    rbtools/clients/__init__.py
    rbtools/clients/svn.py



Tool: Pyflakes
Processed Files:
    rbtools/clients/__init__.py
    rbtools/clients/svn.py

Ship it!

```
Ship It!
```

Diff:

Revision 2 (+46 -3)

Show changes

rbtools/clients/svn.py

Tool: PEP8 Style Checker
Processed Files:
    rbtools/clients/svn.py



Tool: Pyflakes
Processed Files:
    rbtools/clients/svn.py

rbtools/clients/__init__.py (Diff revision 1)
The issue has been resolved. Show all issues
```
These parens aren't useful.
```
1. brennie March 20, 2015, 3:18 p.m.
  They are removed in diff revision 2.

Diff:

Revision 3 (+46 -3)

Show changes

rbtools/clients/svn.py

Tool: Pyflakes
Processed Files:
    rbtools/clients/svn.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/clients/svn.py

Status:: Completed
Change Summary:: Pushed to release-0.7.x (d0f33dc)

I've been leveraging svn patch for a while as an indirect mechanism for applying patches from RB, so I like this change. But, I did want to note that svn patch didn't come into existence until Subversion 1.7 (https://subversion.apache.org/docs/release-notes/1.7.html#patch) so I suppose it's possible this could lead to a regression for users of older SVN releases, although it sounds like rbt patch for SVN was broken to begin with.

brennie March 20, 2015, 10:48 p.m.

Hey Griffin, thanks for the heads up. I'm pretty sure that rbt patch hasn't ever worked with SVN. I've got another patch up at https://reviews.reviewboard.org/r/7097 that provides an error message when rbt patch is used with svn prior to 1.7.0 instead of yielding unhelpful errors.

brennie March 21, 2015, 12:54 p.m.

Thanks for bringing up these issues. I will address them in another patch.

brennie March 24, 2015, 3:55 p.m.

These issues are addressed in https://reviews.reviewboard.org/r/7110/.

rbtools/clients/svn.py (Diff revision 3)

The issue has been resolved. Show all issues

The logic in _get_p_number() does not seem correct in all cases, particularly when the working copy where the patch is being applied does not correspond to the repository root. Suppose I have a repo that looks something like:

/
|-- branches
|-- tags
|-- trunk
|-- a.txt

I might have two different checkouts here, one from the repository root and one from trunk:

wcRoot: svn checkout file://URL/repo /tmp/wcRoot
wcTrunk: svn checkout file://URL/repo/trunk /tmp/wcTrunk

Suppose then I make a trivial modification to a.txt in wcRoot. I can execute rbt post from either /tmp/wcRoot (post-method-A) or /tmp/wcRoot/trunk (post-method-B). These appear to result in different base_path metadata, although it is not clear to me if this is really desired. The underlying content of the diffs is identical.

Anyway with these two working copies and two post methods, I now have several possible combinations for applying a patch with rbt patch:

Case 1
Post from: post-method-A
Patch to: wcRoot (execute rbt patch in /tmp/wcRoot)
base_dir: /
base_path: /
p_num: 1
Result: Success

Case 2
Post from: post-method-B
Patch to: wcRoot (execute rbt patch in /tmp/wcRoot)
base_dir: /trunk
base_path: /
p_num: 1
Result: Success

Case 3
Post from: post-method-A
Patch to: wcRoot (execute rbt patch in /tmp/wcRoot/trunk)
base_dir: /
base_path: /trunk
p_num: None
Result: Failure to patch (Skipped missing target: '/trunk/a.txt')

Case 4
Post from: post-method-B
Patch to: wcRoot (execute rbt patch in /tmp/wcRoot/trunk)
base_dir: /trunk
base_path: /trunk
p_num: 1
Result: Failure to patch (Skipped missing target: '/trunk/a.txt')

Case 5
Post from: post-method-A
Patch to: wcTrunk (execute rbt patch in /tmp/wcTrunk)
base_dir: /
base_path: /trunk
p_num: None
Result: Failure to patch (Skipped missing target: '/trunk/a.txt')

Case 6
Post from: post-method-B
Patch to: wcTrunk (execute rbt patch in /tmp/wcTrunk)
base_dir: /trunk
base_path: /trunk
p_num: 1
Result: Failure to patch (Skipped missing target: '/trunk/a.txt')

Cases 3 and 5 and cases 4 and 6 are effectively identical. Cases 3-6 would all work as expected if p_num was 2 rather than None or 1.

brennie March 24, 2015, 3:55 p.m.

So we've decided that a patch must be applied in the directory it was generated in or a higher level directory. Otherwise this can cause some issues.

Consider the case where a patch is created in the root checkout and modifies things in both trunk and branches. If we apply it in /, there is no issue. However, if we apply it in trunk/, svn will create the things that should have been in branches/ in trunk/.

rbtools/clients/svn.py (Diff revision 3)

The issue has been resolved. Show all issues

If I provide a --px NUM option on the command line, then I get an exception here:

...
  File "/xxxx/rbtools/rbtools/clients/svn.py", line 682, in apply_patch
    cmd.append('--strip=%d' % p_num)
TypeError: %d format: a number is required, not str