Fix output of rbt diff/post for git when -X specified.

Review Request #6350 — Created Sept. 21, 2014 and submitted

brennie
RBTools
release-0.6.x
3492099...
rbtools, students

Currently, git's filter support does the opposite behaviour
(only includes files that match all exclude_patterns).

A new module, rbtools.utils.diffs, specifies helper functions
filename_match_any_patterns, which determines if a filename matches
at least one of the given patterns, and
remove_filenames_matching_patterns, which returns an iterable of the
filenames that do not match any given pattern.

The parameters to git diff and git diff-tree have been
reordered so that the revision specification is always the last
argument before the filenames.

All unit tests pass now.
Generated diffs with rbt diff in a git repository with more than
one file changed. When -X filename was specified, only that
file's diff was excluded from the output.

  • 0
  • 0
  • 11
  • 2
  • 13
Description From Last Updated
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/utils/diffs.py
        rbtools/clients/git.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/utils/diffs.py
        rbtools/clients/git.py
    
    
  2. rbtools/clients/git.py (Diff revision 1)
     
     
     'functools' imported but unused
    
  3. rbtools/clients/git.py (Diff revision 1)
     
     
     'itertools' imported but unused
    
  4. rbtools/clients/git.py (Diff revision 1)
     
     
     'fnmatch' imported but unused
    
  5. rbtools/utils/diffs.py (Diff revision 1)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  6. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/utils/diffs.py
        rbtools/clients/git.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/utils/diffs.py
        rbtools/clients/git.py
    
    
  2. 
      
  1. 
      
  2. rbtools/clients/git.py (Diff revision 2)
     
     

    Imports should be in alphabetical order

  3. 
      
  1. 
      
  2. rbtools/clients/git.py (Diff revision 2)
     
     

    Not sure if this matters, but concatenating two lists is more computationally expensive than list creation. Feel free to drop this issue :)

  3. 
      
chipx86
  1. 
      
  2. rbtools/clients/git.py (Diff revision 2)
     
     
     

    Blank line between these.

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

    Would you mind documenting the output we expect to get briefly, so that it's obvious why we're doing filename.split()[-1]?

  4. rbtools/utils/diffs.py (Diff revision 2)
     
     

    I feel that the function names are a bit generic, given that we're specifically working with filenames. How about filename_matches_any_pattern?

  5. rbtools/utils/diffs.py (Diff revision 2)
     
     

    Same idea here.

  6. rbtools/utils/diffs.py (Diff revision 2)
     
     
     
     

    Small nit, but when dealing with multi-line generators or list comprehensions, we usually do:

    (
        filename
        for ...
        ...
    )
    
  7. 
      
brennie
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        rbtools/utils/diffs.py
        rbtools/clients/git.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        rbtools/utils/diffs.py
        rbtools/clients/git.py
    
    
  2. 
      
  1. 
      
  2. rbtools/utils/diffs.py (Diff revision 3)
     
     

    The closing parenthesis should probably not be in a seperate line

  3. 
      
  1. Ship It!

  2. 
      
mike_conley
  1. This looks good to me, but you might want an additional ship-it from one of the other mentors, since I'm not 100% up on the RBTools stuff.

  2. rbtools/utils/diffs.py (Diff revision 3)
     
     

    Nit - for every new .py file, we like to include:

    from future import unicode_literals

    at the top of the file.

    1. This only applies to the RB tree. RBTools will break on Python 2.4 with this.

    2. (And 2.5)

    3. Whoops - good to know. Thanks!

  3. 
      
brennie
brennie
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-0.6.x (cbbb365)
Loading...