10167 unable to check out illumos-gate on Windows Reviewed by: Reviewed by: Approved by:

Review Request #2185 — Created July 16, 2019 and submitted

Information
gdamore
illumos-gate
master
75da172...
Reviewers
general 
10167 unable to check out illumos-gate on Windows Reviewed by: Reviewed by: Approved by:
Test build in progress.  git pbhck verified by hand -- this includes special test cases to keep us from needing to fix this in the future.
  • 0
  • 0
  • 4
  • 1
  • 5
Description From Last Updated
yuri.pankov
yuri.pankov 
  1. Ship It!
  2. 
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 





 

  

   




citrus




   

   







  

 



 

  


   

   

    
 
citrus


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      usr/src/tools/scripts/git-pbchk.py
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Have you tested this with toold built both against python2 and python3?
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      No, I will endeavor to do so.
      
 
      

      2. 



    2. 
 
 
      
 
      
  
 
      
 
      
  
      Well, nuts.  It doesn't work under python2.7, because of split and differences between str and unicode.
      

      Is operation under python 2.7 required?
      
 
      

      3. 



    3. 
 
 
      
 
      
  
 
      
 
      
  
      It is now; there were bugs.
      
 
      

      4. 


 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      usr/src/tools/scripts/git-pbchk.py
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Rather than duplicating the entire gen_files() here, I would add an extra parameter to gen_files().
    

    Something like
    

    

    def gen_files(root, parent, paths, exclude, filter=None):
...
    if filter is None:
        filter = lambda x: os.path.isfile(x)
    ...
    def gen(select=None):
        ...
        if (filter(path) and not empty and
                select(path) and not exclude(abspath)):
                yield path

    

    

    Then gen_links can be implemented as:
    

    

    def gen_links(root, parent, paths, exclude)
    return gen_files(root, parent, paths, exclude, lambda x: os.path.islink(x))

    

    

    or the call to gen_links() could just be replaced with this.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Oh, that's a good idea.  In all fairness, its been a long time since I've messed with Python, and I'm sure some of what I've done isn't terrifically pythonic.
      
 
      

      2. 


 
    


    

 
    

    4. 






  4. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      usr/src/tools/scripts/git-pbchk.py
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    This is not necessary, only symlinks will be returned from the flist() generator.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 






  5. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      usr/src/tools/scripts/git-pbchk.py
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Have you tried using a pre-compiled regexp for this (and the previous test)? It should be significantly faster for large changesets.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Nope.  It may be, but I wouldn't expect it to make much difference.  Even if it's a few hundred msec (which would be large) I doubt anyone is likely to ever notice.  This isn't terrifically hot code.  But I will investigate if it improves readability.
      
 
      

      2. 



    2. 
 
 
      
 
      
  
 
      
 
      
  
      Because all of these are special characters, I am concerned that that a regular expression is likely to be less readable.
      

      It might be faster to reverse the loop though, and iterate over each character of the path, rather than characters in the specials array.  I could conceivably make the specials a string as well, which might be faster than using an array; it would be terser -- not sure about readability.
      

      I've settled on the following, which I strongly suspect will compare favorably with any regular expression.  (The only way to do this faster than I can think of would be to have the characters used as an array index -- it's possible that a regular expression compiler will generate such a lookup array, but I sort of doubt it.) At any rate, I think readability trumps a tiny performance improvement.
      

      

      def haswinspecial(name):
   specials = '<>:"\\|?*'
   for c in name:
       if c in specials:
           return True
   return False

      
 
      

      3. 



    3. 
 
 
      
 
      
  
 
      
 
      
  
      As you say, it is probably not worth optimising this much and the new version is more readable, thanks.
      

      I was, however, interested so I tried:
      

      

      import re

specials = re.compile(r'[<>:"\\|?*]')

def haswinspecial(name):
        return re.search(specials, name) is not None

      

      

      and this is twice as fast on an 80-character test pathname (I'm not saying to change it).
      
 
      

      4. 


 
    


    

 
    

    6. 




  6. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    7. 




  

 
















 



 

  

   




gdamore




   

   

  

 



 















 





 

  

   




citrus




   

   









  

 



 

  


   

   

    
 
citrus


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      usr/src/tools/scripts/git-pbchk.py
      


       (Diff revision 2)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    This should be moved up out of this loop. It does not need to be done each iteration.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 
















 



 

  

   




gdamore




   

   

  

 



 















 





 

  

   




citrus




   

   







  

 



 

  


   

   

    
 
citrus


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Ship It!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 





 

  

   




andy_js




   

   







  

 



 

  


   

   

    
 
andy_js


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Ship It!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 



 

  

   




gdamore




   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  






 

  Status:
  

  Closed (submitted)

  
 







 


 










  

 









     

    

   

  

  

   
   Loading...