brennie got review request #7000!
Make file exclusion support syntax consistent for all SCMClients
Review Request #7000 — Created March 2, 2015 and submitted
File exclusion support (e.g., with
rbt diff -X patternor
rbt post -X pattern) now behaves identically (with some caveats).
Previously, the case of having a pattern matching against the root of
the repository was not well defined and was misinterpreted by all
SCMClients. Now all patterns beginning with a path separator (e.g.
-X /patternon Linux and Mac OS, and-X \patternon Windows) are
treated as being relative to the root of the repository checkoutl.
However, because CVS generates diffs relative to the current working
directory (without an easy way to determine the checkout root), all
patterns (even those beginning with a path separator) are treated as
relative to the current working directory.The documentation for the
-Xflag has been updated to reflect these
changes.Unit tests have been added for Bazaar, Git, Mercurial, and Subversion
for generating diffs correctly with this new syntax. A unit test has
also been added for Perforce that checks that paths are normalized
correctly.
Ran unit tests.
Manually tested that files were excluded correctly using CVS.
Manually verified that all cases in issue 3776 are resolved:
- Running
rbt diff -X PATTERNin a subdirectory checkout
works as expected.- We no longer rely on the
Working Copy Root Pathfield of
svn info.- Running
rbt diff --repository-url URL -X PATTERN R1:R2
works as expected when ran outside of an SVN checkout.
| Description | From | Last Updated |
|---|---|---|
|
Leftover debugging print? |
 david
|
|
|
local variable 'normalized_patterns' is assigned to but never used |
 reviewbot
|
|
|
Before we backed out the -I change, we had a bunch of reports of people (probably with older versions of … |
david
|
|
|
'spy_on' imported but unused |
reviewbot
|
|
|
Per the third item mentioned in #3776, using the current working directory (e.g. '.') here as the target for 'svn … |
 gmyers
|
|
|
Barret, I really appreciate you taking the time to address the ticket I opened. I hate to be nit-picky, but … |
gmyers
|
|
|
Comment needs to be updated to reflect the modified code. |
gmyers
|
|
|
This test appears to be failing. |
gmyers
|
|
|
Extra leading space on this line. |
gmyers
|
|
|
Can we move the comments into the conditional's body? It's easier to scan. |
 chipx86
|
|
|
This is a bit weird to read. How about calling the function and storing as a variable, and then doing … |
chipx86
|
- Change Summary:
-
PEP8
- Commit:
-
36188a57ba501e2935dea8c1fb7162709d5cd5e2c99b23e1adb9d4a6f4711281097f4d4f9a4081c6
-
Tool: Pyflakes Processed Files: rbtools/clients/cvs.py rbtools/utils/diffs.py rbtools/commands/__init__.py rbtools/clients/tests.py rbtools/clients/bazaar.py rbtools/clients/git.py rbtools/clients/perforce.py rbtools/clients/svn.py Tool: PEP8 Style Checker Processed Files: rbtools/clients/cvs.py rbtools/utils/diffs.py rbtools/commands/__init__.py rbtools/clients/tests.py rbtools/clients/bazaar.py rbtools/clients/git.py rbtools/clients/perforce.py rbtools/clients/svn.py
-
I wanted to call your attention to Issue #3776 (https://code.google.com/p/reviewboard/issues/detail?id=3776) where I detailed some issues I observed with -X under svn. This includes the KeyError that David mentioned earlier in his review. Perhaps these issues are out of scope for this task, but I tested with the updated code and they are all still present.
- Change Summary:
-
Fix SVN behaviour.
- Commit:
-
c99b23e1adb9d4a6f4711281097f4d4f9a4081c6587a1f2d95f72b9f6ce06020aaebefd83505ffa9
-
Tool: Pyflakes Processed Files: rbtools/clients/cvs.py rbtools/utils/diffs.py rbtools/commands/__init__.py rbtools/clients/tests.py rbtools/clients/bazaar.py rbtools/clients/git.py rbtools/clients/perforce.py rbtools/clients/svn.py Tool: PEP8 Style Checker Processed Files: rbtools/clients/cvs.py rbtools/utils/diffs.py rbtools/commands/__init__.py rbtools/clients/tests.py rbtools/clients/bazaar.py rbtools/clients/git.py rbtools/clients/perforce.py rbtools/clients/svn.py
-
Tool: Pyflakes Processed Files: rbtools/clients/cvs.py rbtools/utils/diffs.py rbtools/commands/__init__.py rbtools/clients/tests.py rbtools/clients/bazaar.py rbtools/clients/git.py rbtools/clients/perforce.py rbtools/clients/svn.py Tool: PEP8 Style Checker Processed Files: rbtools/clients/cvs.py rbtools/utils/diffs.py rbtools/commands/__init__.py rbtools/clients/tests.py rbtools/clients/bazaar.py rbtools/clients/git.py rbtools/clients/perforce.py rbtools/clients/svn.py
- Testing Done:
-
Ran unit tests.
Manually tested that files were excluded correctly using CVS.
+ + Manually verified that all cases in issue 3776 are resolved:
+ + - TODO: need to verify case 1.
+ - We no longer rely on the
Working Copy Root Pathfield of
svn info.
+ - Running
rbt diff --repository-url URL -X PATTERN R1:R2
works as expected when ran outside of an SVN checkout.
- Bugs:
-
- Commit:
04324cc0fa977fe83e0d521824909f4425385208f1c260e795abe752f575f0e9e8f8f4ed76115d13
-
Tool: PEP8 Style Checker Processed Files: rbtools/clients/cvs.py rbtools/utils/diffs.py rbtools/commands/__init__.py rbtools/clients/tests.py rbtools/clients/bazaar.py rbtools/clients/git.py rbtools/clients/perforce.py rbtools/clients/svn.py Tool: Pyflakes Processed Files: rbtools/clients/cvs.py rbtools/utils/diffs.py rbtools/commands/__init__.py rbtools/clients/tests.py rbtools/clients/bazaar.py rbtools/clients/git.py rbtools/clients/perforce.py rbtools/clients/svn.py
- Change Summary:
-
Update testing done.
- Testing Done:
-
Ran unit tests.
Manually tested that files were excluded correctly using CVS.
Manually verified that all cases in issue 3776 are resolved:
~ - TODO: need to verify case 1.
~ - Running
rbt diff -X PATTERNin a subdirectory checkout
works as expected.
- We no longer rely on the
Working Copy Root Pathfield of
svn info.
- Running
rbt diff --repository-url URL -X PATTERN R1:R2
works as expected when ran outside of an SVN checkout.
-
-
Barret, I really appreciate you taking the time to address the ticket I opened. I hate to be nit-picky, but I still have a concern here particularly with the use of
base_pathfrom theSVNRepositoryInfoobject. The base_path here is determined once and is based on the directory in the working copy which rbt is run out of. The problem is that this path is not necessarily what you want when you start to use -I/--include to include specific files to be diffed.Here is a bash script, in the spirit of the one I provided in Issue 3776 to illustrate the problem:
####################################################### #!/bin/bash set -x svnadmin create /tmp/testrepo mkdir foo cd foo mkdir trunk branches tags echo "test" > trunk/a.txt echo "data" > trunk/b.dat svn import . file:///tmp/testrepo -m "initial commit" cd .. svn co file:///tmp/testrepo foo_root_wc cd foo_root_wc echo "stuff" >> trunk/a.txt echo "more data" >> trunk/b.dat cd branches rbt diff --server 127.0.0.1:8080 -I ../trunk -X *.dat cd ../trunk rbt diff --server 127.0.0.1:8080 -I . -X *.dat ######################################################This will create a repo with trunk, branches, and tags directories in the root. There are txt and dat files in trunk. The script next checks out the root of the repository, manipulates the files, changes to the branches subdirectory then runs
rbt diffwith an explicit include to ../trunk and an exclude of *.dat. File b.dat is not excluded from the diff as expected. Finally, the same command is run from trunk where it does behave as expected.The problem is that when running from the branches subdirectory the base_path is /branches, which normalizes the exclusion pattern to /branches/*.dat. This does not match with /trunk/b.dat.
I'll admit that this specific example is a fairly contrived. But, at my workplace we rely heavily on SVN externals and to get RBTools to work like we need/want it to in the presence of externals, we basically have a big wrapper script around rbt that heavily leverages includes via -I. And, if there is way to break something that is exacerbated by includes, then we will probably run into it :)
- Change Summary:
-
Allow global patterns in SVN
- Commit:
-
f1c260e795abe752f575f0e9e8f8f4ed76115d13297742cb7fd625203b3edad016885dbe0eeedd28
-
Tool: Pyflakes Processed Files: rbtools/clients/cvs.py rbtools/utils/diffs.py rbtools/commands/__init__.py rbtools/clients/tests.py rbtools/clients/bazaar.py rbtools/clients/git.py rbtools/clients/perforce.py rbtools/clients/svn.py Tool: PEP8 Style Checker Processed Files: rbtools/clients/cvs.py rbtools/utils/diffs.py rbtools/commands/__init__.py rbtools/clients/tests.py rbtools/clients/bazaar.py rbtools/clients/git.py rbtools/clients/perforce.py rbtools/clients/svn.py
-
Tool: Pyflakes Processed Files: rbtools/clients/cvs.py rbtools/utils/diffs.py rbtools/commands/__init__.py rbtools/clients/tests.py rbtools/clients/bazaar.py rbtools/clients/git.py rbtools/clients/perforce.py rbtools/clients/svn.py Tool: PEP8 Style Checker Processed Files: rbtools/clients/cvs.py rbtools/utils/diffs.py rbtools/commands/__init__.py rbtools/clients/tests.py rbtools/clients/bazaar.py rbtools/clients/git.py rbtools/clients/perforce.py rbtools/clients/svn.py
-
Tool: Pyflakes Processed Files: rbtools/clients/cvs.py rbtools/utils/diffs.py rbtools/commands/__init__.py rbtools/clients/tests.py rbtools/clients/bazaar.py rbtools/clients/git.py rbtools/clients/perforce.py rbtools/clients/svn.py Tool: PEP8 Style Checker Processed Files: rbtools/clients/cvs.py rbtools/utils/diffs.py rbtools/commands/__init__.py rbtools/clients/tests.py rbtools/clients/bazaar.py rbtools/clients/git.py rbtools/clients/perforce.py rbtools/clients/svn.py
Tool: Pyflakes Processed Files: rbtools/clients/cvs.py rbtools/utils/diffs.py rbtools/commands/__init__.py rbtools/clients/tests.py rbtools/clients/bazaar.py rbtools/clients/git.py rbtools/clients/perforce.py rbtools/clients/svn.py Tool: PEP8 Style Checker Processed Files: rbtools/clients/cvs.py rbtools/utils/diffs.py rbtools/commands/__init__.py rbtools/clients/tests.py rbtools/clients/bazaar.py rbtools/clients/git.py rbtools/clients/perforce.py rbtools/clients/svn.py