None

brennie got review request #7000!

Make file exclusion support syntax consistent for all SCMClients

Review Request #7000 — Created March 2, 2015 and submitted

brennie
RBTools
release-0.7.x
3776
50ef470...
rbtools

File exclusion support (e.g., with rbt diff -X pattern or
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 /pattern on Linux and Mac OS, and -X \pattern on 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 -X flag 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:

  1. Running rbt diff -X PATTERN in a subdirectory checkout
    works as expected.
  2. We no longer rely on the Working Copy Root Path field of
    svn info.
  3. 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?

daviddavid

local variable 'normalized_patterns' is assigned to but never used

reviewbotreviewbot

Before we backed out the -I change, we had a bunch of reports of people (probably with older versions of ...

daviddavid

'spy_on' imported but unused

reviewbotreviewbot

Per the third item mentioned in #3776, using the current working directory (e.g. '.') here as the target for 'svn ...

gmyersgmyers

Barret, I really appreciate you taking the time to address the ticket I opened. I hate to be nit-picky, but ...

gmyersgmyers

Comment needs to be updated to reflect the modified code.

gmyersgmyers

This test appears to be failing.

gmyersgmyers

Extra leading space on this line.

gmyersgmyers

Can we move the comments into the conditional's body? It's easier to scan.

chipx86chipx86

This is a bit weird to read. How about calling the function and storing as a variable, and then doing ...

chipx86chipx86
reviewbot
  1. 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
    
    
  2. rbtools/clients/perforce.py (Diff revision 1)
     
     
     local variable 'normalized_patterns' is assigned to but never used
    
  3. rbtools/clients/tests.py (Diff revision 1)
     
     
     'spy_on' imported but unused
    
  4. 
      
brennie
david
  1. 
      
  2. rbtools/clients/bazaar.py (Diff revision 1)
     
     

    Leftover debugging print?

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

    Before we backed out the -I change, we had a bunch of reports of people (probably with older versions of SVN?) where this was causing a KeyError.

  4. 
      
reviewbot
  1. 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
    
    
  2. 
      
gmyers
  1. 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.
    1. Thanks for the heads up, Griffin. I'm working on getting this resolved. We shouldn't have to rely on the Working Copy Root Path and the next version won't assume a root checkout.

  2. 
      
brennie
reviewbot
  1. 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
    
    
  2. 
      
brennie
reviewbot
  1. 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
    
    
  2. 
      
gmyers
  1. 
      
  2. rbtools/clients/svn.py (Diff revisions 2 - 4)
     
     
    Per the third item mentioned in #3776, using the current working directory (e.g. '.') here as the target for 'svn info' does not support the `--repository-url` option.
  3. 
      
brennie
reviewbot
  1. 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
    
    
  2. 
      
brennie
gmyers
  1. 
      
  2. rbtools/clients/svn.py (Diff revision 5)
     
     
     
     

    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_path from the SVNRepositoryInfo object. 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 diff with 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 :)

    1. Ah. So using -X *.txt in a directory will only exclude files in the current directory and subdirectories. That is the intended behaviour and how it works with all other SCMs. To make a pattern "global", you would preface it with the path separator, e.g., -X /*.txt. I never considered the case of using -I to include a seperate directory, so it is currently broken with SVN. I will fix this case in the next revision.

    2. Please change your pattern to '/*.txt' and re-run your script on revision 6. You should find it to work there.

  3. 
      
brennie
reviewbot
  1. 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
    
    
  2. 
      
gmyers
  1. 
      
  2. rbtools/clients/svn.py (Diff revision 6)
     
     
     
     
     
     
     
     
     
    Comment needs to be updated to reflect the modified code.
  3. rbtools/clients/tests.py (Diff revision 6)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    This test appears to be failing.
  4. rbtools/commands/__init__.py (Diff revision 6)
     
     
    Extra leading space on this line.
  5. 
      
brennie
reviewbot
  1. 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
    
    
  2. 
      
chipx86
  1. 
      
  2. rbtools/clients/perforce.py (Diff revision 7)
     
     
     
     

    Can we move the comments into the conditional's body? It's easier to scan.

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

    This is a bit weird to read. How about calling the function and storing as a variable, and then doing the conditional from that?

  4. 
      
brennie
reviewbot
  1. 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
    
    
  2. 
      
chipx86
  1. Ship It!
  2. 
      
brennie
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-0.7.x (202154a)
Loading...