RGlobs.to_filespec should generate filespecs that match git spec

Review Request #4078 — Created July 13, 2016 and submitted

yujiec
pants
3413, 3664
pants-reviews
kwlzn, nhoward_tw, stuhood

In v1 engine's rglobs implementation, '**' matches 1 or more dirs.

In v2 engine, we want to make rglobs follow git spec, which means '**' in BUILD file should match 0 or more dirs.

v2 engine used to treat '**' as matching 1 or more dirs. Thus for a pattern like 'a/**/*.py', RGlobs.to_filespec will generate 2 filespecs: 'a/**/*.py' and 'a/*.py', to make '**' from BUILD files match 0 or more dirs. However, after https://rbcommons.com/s/twitter/r/4034/, v2 engine started to match '**' with 0 or more dirs directly. The additional logic of handling '**' in RGlobs.to_filespec can be removed then.

In this patch,
1. RGlobs.to_filespec is simplified.
2. Add a logic in PathGlob.create to replace consecutive '**' with a single '**'.
3. Fix test failures caused by 1.

ci green:
https://travis-ci.org/pantsbuild/pants/builds/144851041

  • 0
  • 0
  • 1
  • 0
  • 1
Description From Last Updated
  1. Thanks Yujie: looks good.

  2. src/python/pants/engine/fs.py (Diff revision 1)
     
     

    This method probably deserves a better description... in particular, it looks like it assumes that the value at idx 0 == **.

    Alternatively, maybe move validating the assumption into the method? Could also move in the ValueError for malformed values.

    1. I was thinking should I really treat '**abcde' as an error case? Maybe instead of having "elif cls._DOUBLE in parts[0]" on line 153 I can just change it to "elif cls._DOUBLE == parts[0]" and treat case like '**abcde' same as '*abcde'.

    2. Yea, that's probably fine too. From what I can tell, git doesn't error for this case.

  3. tests/python/pants_test/engine/test_fs.py (Diff revision 1)
     
     
     

    Replacing set with sorted in both of these cases would let you drop the len check, and would likely be easier to debug.

  4. Are you able to drop the filters on any of these now? Might not be... just checking.

    1. no, they still fail.

  5. 
      
  1. Ship It!
  2. 
      
Review request changed

Status: Closed (submitted)

Change Summary:

Merged as cb799a7e4c5a9b467ce2acf600124878ae188200

Loading...