Add support for -X to git SCMClient

Review Request #6339 — Created Sept. 19, 2014 and submitted

Information

RBTools
master
191bc37...

Reviewers

Git (and the git-svn and git-p4 backends) now support the -X/--exclude
commandline option and the EXCLUDE_PATTERNS .reviewboardrc option.

Add unit tests for excluding files from Git diffs.

Rename exclude_files/EXCLUDE_FILES to exclude_patterns/EXCLUDE_PATTERNS
to reflect the actual behaviour of -X.

Unit tests pass.

Generated diffs with excluded files via rbt diff with git, git-svn,
and git-p4.

Description From Last Updated

Col: 59 E231 missing whitespace after ','

reviewbotreviewbot

Col: 39 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Col: 39 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Col: 39 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Col: 39 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Can you rename to filename? file is reserved in Python.

chipx86chipx86

I assume this is an error condition? If so, we should probably log something.

chipx86chipx86

No blank line here.

chipx86chipx86

Any chance of supporting both include and exclude at the same time? (Did the Mercurial change support this?)

chipx86chipx86

local variable 'include_Files' is assigned to but never used

reviewbotreviewbot

This should be updated to say "pattern" instead of "file".

daviddavid

This should probably have a final 'else' that just asserts.

daviddavid

Can we log this message at a higher level than 'debug'?

daviddavid

Please replace the `' with "" and put this whole string in single-quotes instead of double.

daviddavid

This line should be indented one more space (to match up with the '(').

daviddavid

This should be indented one more space.

daviddavid
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/clients/tests.py
        rbtools/clients/git.py
    
    Ignored Files:
        docs/rbtools/rbt/commands/post.rst
        docs/rbtools/rbt/commands/diff.rst
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/clients/tests.py
        rbtools/clients/git.py
    
    Ignored Files:
        docs/rbtools/rbt/commands/post.rst
        docs/rbtools/rbt/commands/diff.rst
    
    
  2. rbtools/clients/git.py (Diff revision 1)
     
     
    Show all issues
    Col: 59
     E231 missing whitespace after ','
    
  3. rbtools/clients/git.py (Diff revision 1)
     
     
    Show all issues
    Col: 39
     E127 continuation line over-indented for visual indent
    
  4. rbtools/clients/git.py (Diff revision 1)
     
     
    Show all issues
    Col: 39
     E127 continuation line over-indented for visual indent
    
  5. rbtools/clients/git.py (Diff revision 1)
     
     
    Show all issues
    Col: 39
     E127 continuation line over-indented for visual indent
    
  6. rbtools/clients/git.py (Diff revision 1)
     
     
    Show all issues
    Col: 39
     E127 continuation line over-indented for visual indent
    
  7. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/clients/tests.py
        rbtools/clients/git.py
    
    Ignored Files:
        docs/rbtools/rbt/commands/post.rst
        docs/rbtools/rbt/commands/diff.rst
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/clients/tests.py
        rbtools/clients/git.py
    
    Ignored Files:
        docs/rbtools/rbt/commands/post.rst
        docs/rbtools/rbt/commands/diff.rst
    
    
  2. 
      
chipx86
  1. 
      
  2. rbtools/clients/git.py (Diff revision 2)
     
     
    Show all issues

    Can you rename to filename? file is reserved in Python.

  3. rbtools/clients/git.py (Diff revision 2)
     
     
     
     
    Show all issues

    I assume this is an error condition? If so, we should probably log something.

  4. rbtools/clients/git.py (Diff revision 2)
     
     
     
     
    Show all issues

    No blank line here.

  5. rbtools/clients/git.py (Diff revision 2)
     
     
     
    Show all issues

    Any chance of supporting both include and exclude at the same time? (Did the Mercurial change support this?)

    1. By this do you mean removing files in exclude_files from include_files?

      The mercurial change just adds -X f1 -X f2 to the commandline because it natively supports it.

    2. If you have excluded files set up, then no included files passed on the command line will be used, since we never reach this block.

      What does Mercurial do if you specify both specific files and use -X?

    3. As it turns out, hg diff -r r1 -r r2 -X f -- f returns nothing. I /think/ this is a good behaviour and will replicate this in the git tool.

  6. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/clients/cvs.py
        rbtools/clients/tests.py
        rbtools/commands/post.py
        rbtools/commands/__init__.py
        rbtools/commands/diff.py
        rbtools/clients/clearcase.py
        rbtools/clients/plastic.py
        rbtools/clients/__init__.py
        rbtools/clients/mercurial.py
        rbtools/clients/git.py
        rbtools/clients/perforce.py
        rbtools/clients/bazaar.py
        rbtools/clients/svn.py
    
    Ignored Files:
        docs/rbtools/rbt/commands/post.rst
        docs/rbtools/rbt/commands/diff.rst
    
    
  2. rbtools/clients/git.py (Diff revision 3)
     
     
    Show all issues
     local variable 'include_Files' is assigned to but never used
    
  3. 
      
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        rbtools/clients/cvs.py
        rbtools/clients/tests.py
        rbtools/commands/post.py
        rbtools/commands/__init__.py
        rbtools/commands/diff.py
        rbtools/clients/clearcase.py
        rbtools/clients/plastic.py
        rbtools/clients/__init__.py
        rbtools/clients/mercurial.py
        rbtools/clients/git.py
        rbtools/clients/perforce.py
        rbtools/clients/bazaar.py
        rbtools/clients/svn.py
    
    Ignored Files:
        docs/rbtools/rbt/commands/post.rst
        docs/rbtools/rbt/commands/diff.rst
    
    
  2. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/clients/cvs.py
        rbtools/clients/tests.py
        rbtools/commands/post.py
        rbtools/commands/__init__.py
        rbtools/commands/diff.py
        rbtools/clients/clearcase.py
        rbtools/clients/plastic.py
        rbtools/clients/__init__.py
        rbtools/clients/mercurial.py
        rbtools/clients/git.py
        rbtools/clients/perforce.py
        rbtools/clients/bazaar.py
        rbtools/clients/svn.py
    
    Ignored Files:
        docs/rbtools/rbt/commands/post.rst
        docs/rbtools/rbt/commands/diff.rst
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/clients/cvs.py
        rbtools/clients/tests.py
        rbtools/commands/post.py
        rbtools/commands/__init__.py
        rbtools/commands/diff.py
        rbtools/clients/clearcase.py
        rbtools/clients/plastic.py
        rbtools/clients/__init__.py
        rbtools/clients/mercurial.py
        rbtools/clients/git.py
        rbtools/clients/perforce.py
        rbtools/clients/bazaar.py
        rbtools/clients/svn.py
    
    Ignored Files:
        docs/rbtools/rbt/commands/post.rst
        docs/rbtools/rbt/commands/diff.rst
    
    
  2. 
      
AS
  1. 
      
  2. rbtools/clients/tests.py (Diff revision 4)
     
     

    Can you tell me which assert statements actually test your changes? I'm having trouble understainding the test case

    1. The test compares the result of running the git diff after including two files and excluding one of them to what the result would be of just including one file.

  3. 
      
dkus
  1. 
      
  2. 
      
AS
  1. Ship It!

  2. 
      
david
  1. 
      
  2. docs/rbtools/rbt/commands/post.rst (Diff revision 4)
     
     
    Show all issues

    This should be updated to say "pattern" instead of "file".

  3. rbtools/clients/git.py (Diff revision 4)
     
     
     
     
     
     
     
    Show all issues

    This should probably have a final 'else' that just asserts.

  4. rbtools/clients/git.py (Diff revision 4)
     
     
     
     
     
    Show all issues

    Can we log this message at a higher level than 'debug'?

  5. rbtools/clients/git.py (Diff revision 4)
     
     
     
     
    Show all issues

    Please replace the `' with "" and put this whole string in single-quotes instead of double.

  6. rbtools/commands/diff.py (Diff revision 4)
     
     
    Show all issues

    This line should be indented one more space (to match up with the '(').

  7. rbtools/commands/post.py (Diff revision 4)
     
     
    Show all issues

    This should be indented one more space.

  8. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/clients/cvs.py
        rbtools/clients/tests.py
        rbtools/commands/post.py
        rbtools/commands/__init__.py
        rbtools/commands/diff.py
        rbtools/clients/clearcase.py
        rbtools/clients/plastic.py
        rbtools/clients/__init__.py
        rbtools/clients/mercurial.py
        rbtools/clients/git.py
        rbtools/clients/perforce.py
        rbtools/clients/bazaar.py
        rbtools/clients/svn.py
    
    Ignored Files:
        docs/rbtools/rbt/commands/post.rst
        docs/rbtools/rbt/commands/diff.rst
    
    
  2. 
      
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        rbtools/clients/cvs.py
        rbtools/clients/tests.py
        rbtools/commands/post.py
        rbtools/commands/__init__.py
        rbtools/commands/diff.py
        rbtools/clients/clearcase.py
        rbtools/clients/plastic.py
        rbtools/clients/__init__.py
        rbtools/clients/mercurial.py
        rbtools/clients/git.py
        rbtools/clients/perforce.py
        rbtools/clients/bazaar.py
        rbtools/clients/svn.py
    
    Ignored Files:
        docs/rbtools/rbt/commands/post.rst
        docs/rbtools/rbt/commands/diff.rst
    
    
  2. 
      
AD
  1. 
      
  2. rbtools/clients/git.py (Diff revision 4)
     
     
     
     
     
     
     
     

    You can use fnmatch.filter function to get a subset of the filenames that match each pattern. It will simplify the code a bit but not necessary.

    1. Implemented in latest diff.

  3. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/clients/cvs.py
        rbtools/clients/tests.py
        rbtools/commands/post.py
        rbtools/commands/__init__.py
        rbtools/commands/diff.py
        rbtools/clients/clearcase.py
        rbtools/clients/plastic.py
        rbtools/clients/__init__.py
        rbtools/clients/mercurial.py
        rbtools/clients/git.py
        rbtools/clients/perforce.py
        rbtools/clients/bazaar.py
        rbtools/clients/svn.py
    
    Ignored Files:
        docs/rbtools/rbt/commands/post.rst
        docs/rbtools/rbt/commands/diff.rst
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/clients/cvs.py
        rbtools/clients/tests.py
        rbtools/commands/post.py
        rbtools/commands/__init__.py
        rbtools/commands/diff.py
        rbtools/clients/clearcase.py
        rbtools/clients/plastic.py
        rbtools/clients/__init__.py
        rbtools/clients/mercurial.py
        rbtools/clients/git.py
        rbtools/clients/perforce.py
        rbtools/clients/bazaar.py
        rbtools/clients/svn.py
    
    Ignored Files:
        docs/rbtools/rbt/commands/post.rst
        docs/rbtools/rbt/commands/diff.rst
    
    
  2. 
      
david
  1. Ship It!

  2. 
      
brennie
Review request changed
Status:
Completed
Change Summary:
Pushed to release-0.6.x (f7a6d7d)