Summary

Allow rbt patch to work in SVN subdirectories

Review Request #7110 — Created March 24, 2015 and submitted March 26, 2015, 12:27 p.m.

Information

Owner

brennie

Repository

RBTools

Branch

release-0.7.x

Bugs

Depends On

Commit

7fd84ee...

Reviewers

Groups

rbtools

People

gmyers

Description

Previously rbt patch would not work in a subdirectory or subdirectory
checkout. This was because the incorrect number of leading directories
were being stripped (due to our SVN diffs having absolute paths). We now
compute the correct number of leading directories to strip when applying
patches and they now apply correctly.

If a patch for SVN is generated in a different directory, files included
in the patch that are not under the current directory will not be
applied. If this occurs, the user will be informed.

This patch also fixes the issue of passing a value --px flag on the
commandline causing a formatting error.

Testing Done

Ran unit tests.

Manually tested the cases that gmyers outlined in
https://reviews.reviewboard.org/r/7095/. rbt patch works in all of
these cases.

Issues

Description	From	Last Updated
I don't understand what you are trying to protect against here. If I remove this, then all of the test …	gmyers	March 24, 2015, 9:22 p.m.
Very minor, but should this be self.INDEX_FILE_RE?	gmyers	March 25, 2015, 7:41 p.m.
Not sure if this is important, but I was looking at filter_diff() in utils/diffs.py which follows a similar recipe, and …	gmyers	March 25, 2015, 7:35 p.m.
local variable 'filtered_diff' is assigned to but never used	reviewbot	March 24, 2015, 9:36 p.m.

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



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

Change Summary:

Wrong commit.

Commit:

6b045e1bce55c84d0bfcd0241023f44376279361

a20cc632ff8984a607bac6ca710fa75b47560ec0

Diff:

Revision 2 (+21 -5)

Show changes

	rbtools/clients/errors.py
	rbtools/clients/svn.py

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



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

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

The issue has been resolved. Show all issues

I don't understand what you are trying to protect against here.  If I remove this, then all of the test cases from https://reviews.reviewboard.org/r/7095/ work correctly, whereas with this code in place cases 3 and 5 throw the exception.
As far as I understand it base_dir is metadata that describes where rbt post was executed from in terms of the directory hierarchy in the repository.  But, for any particular file in a diff, it doesn't matter where rbt post is executed from because the path in the diff is always an absolute path.  So in 7095, cases 1 and 2 have different base_dir metadata but the contents of the diff are identical:

Index: /trunk/a.txt
===================================================================
--- /trunk/a.txt        (revision 4)
+++ /trunk/a.txt        (working copy)
@@ -1 +1,2 @@
 This is a file.
+I'm from wcRoot


base_path is analogous to base_dir in that it describes where the current working directory (on the local filesystem) in the working copy is in terms of the repository hierarchy.  For the purposes of patching we need to know this current location in the hierarchy so we can strip the appropriate number of leading paths from the absolute paths in the diff.  I don't see why base_dir, which again only describes where the post originated from, should have any bearing on where and how the patch can be applied.
Is there a case that I'm overlooking?

brennie March 24, 2015, 6:44 p.m.

Say we have the following folder structure

/
  trunk/
    foo.txt
  branches/
    some_branch/
      bar.txt
  tags/

When we run rbt diff in / our patch will have things from trunk/ and branches/.

If we then apply the patch in, say, trunk/ we will end up with the following because of how svn patch works with --strip:

/
  trunk/
    foo.txt
    some_branch/
      bar.txt
  branches/
  tags/

Which is not what we want. That is why we guard against doing a patch in a subdirectory of where the patch was generated.

gmyers March 24, 2015, 8:08 p.m.

What you describe would indeed occur in the case where some_branch/bar.txt is a new file (and I think new directory as well) and thus maps to an add operation in the diff. If it were only a modified file then it would be rejected by svn patch as a missing target. I imagine you could also construct a case where a file is inadvertently deleted when you have matching file names in sibling subdirectories.

I understand the thought process here, but I still think it is too restrictive. I think there has to be some onus on the user to understand the contents/scope of the diff and then attempt to apply the patch in the correct place. Here is an example of why I do not like this restriction:

Suppose the basic trunk, branches, tags hierarchy. A colleague may, for whatever reason, have checked out the repo at root, but only made changes to trunk, and then run rbt post from the root of their working copy. base_dir in this case is /.

I, on the other hand, may have a checkout of this repo at trunk. I would like to apply this patch, and I know that it is a reasonable thing to do because it only applies to trunk. Executing rbt patch from the "root" of my working copy, where base_path is /trunk, results in:

"This patch was generated in the / directory. You are currently in the /trunk directory. You cannot apply it from this location. Change to the / directory to apply this patch."

There is no way possible for me to "change to the / directory" in my current working copy. My only option, if I wish to apply this diff automatically with rbt patch, is to checkout an entirely new working copy relative to the repo root and run rbt patch from there. I can work around this with rbt patch --print to dump the diff to file and then manually apply it with svn patch --strip=N, but I think it is a stretch for the average user to know they could do this when they have just been hit with the previous exception.

brennie March 24, 2015, 9:34 p.m.

This latest patch will exclude files not under the directory where the patch is being applied. In other words, if a patch generated in / is applied at /trunk, then all changes not under /trunk will not be applied. The user is warned in this case.

Description:

		Previously `rbt patch` would not work in a subdirectory or subdirectory
		checkout. This was because the incorrect number of leading directories
		were being stripped (due to our SVN diffs having absolute paths). We now
		compute the correct number of leading directories to strip when applying
		patches and they now apply correctly.

~		Patches for SVN must now be applied in the directory they were generated
~		or a higher level directory. You can not apply a patch from a higher
~		level directory in a lower level directory or `rbt` will exit with an
	~	If a patch for SVN is generated in a different directory, files included
	~	in the patch that are not under the current directory will not be
	~	applied. If this occurs, the user will be informed.
-		error message indicating where the patch should be applied.

		This patch also fixes the issue of passing a value `--px` flag on the
		commandline causing a formatting error.

Testing Done:

		Ran unit tests.

		Manually tested the cases that gmyers outlined in
~		https://reviews.reviewboard.org/r/7095/. `rbt patch` works in cases
~		except where the diff was created in a higher level directory; those
	~	https://reviews.reviewboard.org/r/7095/. `rbt patch` works in all of
	~	these cases.
-		cases correctly raise an exception.

Commit:

a20cc632ff8984a607bac6ca710fa75b47560ec0

c5fb7d3a90726053f51e3d04484c42b153b47432

Diff:

Revision 3 (+62 -5)

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/svn.py (Diff revision 3)
The issue has been resolved. Show all issues
```
 local variable 'filtered_diff' is assigned to but never used
```

Change Summary:

PEP8 / documentation fixes.

Commit:

c5fb7d3a90726053f51e3d04484c42b153b47432

bb5692c82d1bbf9776c39acfbfe5d4a4cf3be2e7

Diff:

Revision 4 (+66 -5)

Show changes

rbtools/clients/svn.py

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



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

Commit:

bb5692c82d1bbf9776c39acfbfe5d4a4cf3be2e7

fa6e6595b2dcbcada70a0ad7f5e4446c585fcf10

Diff:

Revision 5 (+65 -5)

Show changes

rbtools/clients/svn.py

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



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

Change Summary:

Remove debug print statement.

Commit:

fa6e6595b2dcbcada70a0ad7f5e4446c585fcf10

867c5a4bc5dfce3d098c1583eaeccc33b6523e1a

Diff:

Revision 6 (+64 -5)

Show changes

rbtools/clients/svn.py

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



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

Ship it!

```
Ship It!
```

Ship it!

Looks great, Barret!  I really like the new filtering approach.

rbtools/clients/svn.py (Diff revisions 2 - 6)
The issue has been resolved. Show all issues
```
Very minor, but should this be self.INDEX_FILE_RE?
```

rbtools/clients/svn.py (Diff revisions 2 - 6)

The issue has been resolved. Show all issues

Not sure if this is important, but I was looking at filter_diff() in utils/diffs.py which follows a similar recipe, and there m.group(1).decode('utf-8') is used.

brennie March 25, 2015, 6:49 p.m.
```
Oops, nice catch!
```

rbtools/clients/svn.py (Diff revisions 2 - 6)

Is there any need to and/or value in returning early from apply_patch() or returning a different PatchResult.applied value in the special case where all files are excluded?

brennie March 25, 2015, 6:49 p.m.

That is actually a good case that I had not considered. I will add it in the next revision.

Commit:

867c5a4bc5dfce3d098c1583eaeccc33b6523e1a

7fd84eed56a5d4f2f16d01c7fce2bcf47b5044f0

Diff:

Revision 7 (+74 -5)

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 (dce02e4)