Add Perforce binary file patching and fix up existing patch issues.

 






 

 


  Review Request #14722
  —
  Created Dec. 3, 2025 and updated 

 




















 

  

   Information

  





 
 

  david
 









 
 

  RBTools
 









 
 

  master
 









 
 

  
 









 
 

  
 






 




 

  

   Reviewers

  





 
 

  rbtools
 









 
 

  
 






 








 





  


 
 

  
This change adds support to the PerforceParser for applying binary

files as part of changes. The most basic part of this is just the

implementation of the add/move/delete methods, but while working on this

I discovered that patching for Perforce was broken in a variety of ways:


    

  • 

    We've traditionally relied on the patch command to handle opening
    
  the relevant files, and we still do, but it now opens and edits them
    
  in a way which requires running p4 reconcile. This command has been
    
  added to the end of the patch sequence.
    

    • 

  • 

    apply_patch_for_empty_files would use a regex to pull out filenames
    
  from the diff that then got passed to p4 add or p4 delete, but
    
  that regex only stripped off a leading "//" and did not map filenames
    
  to client names. It seems like in some cases Perforce was clever
    
  enough to know that "depot/something/..." really meant
    
  "//depot/something/...", but that depended highly on what that depot
    
  was called and what the client mapping was set up to look like. I've
    
  changed it to map the depot filenames to local first.
    

    • 

  • 

    apply_patch_for_empty_files was only being called in the case that
    
  the patch entirely failed to apply, meaning we had a change that
    
  comprised only empty files. Patches with a mix of empty and
    
  non-empty file changes would apply the non-empty ones and skip the
    
  rest. The fix for this issue is actually inside /r/14711, which
    
  reorganizes the top-level patching process to always call this method
    
  when needed.
    

    • 


 








  


 
 

  
    

  • Ran unit tests.
    • 

  • Applied a variety of patches that included mixes of empty, non-empty,
    
  and binary files.
    • 


 







 









 





  

 


 
 

  



 

  
   


    Summary
    ID

   

  
  

   


    

     

      Add Perforce binary file patching and fix up existing patch issues.
      
This change adds support to the `PerforceParser` for applying binary
files as part of changes. The most basic part of this is just the
implementation of the add/move/delete methods, but while working on this
I discovered that patching for Perforce was broken in a variety of ways:

* We've traditionally relied on the `patch` command to handle opening
  the relevant files, and we still do, but it now opens and edits them
  in a way which requires running `p4 reconcile`. This command has been
  added to the end of the patch sequence.

* `apply_patch_for_empty_files` would use a regex to pull out filenames
  from the diff that then got passed to `p4 add` or `p4 delete`, but
  that regex only stripped off a leading "//" and did not map filenames
  to client names. It seems like in some cases Perforce was clever
  enough to know that "depot/something/..." really meant
  "//depot/something/...", but that depended highly on what that depot
  was called and what the client mapping was set up to look like. I've
  changed it to map the depot filenames to local first.

* `apply_patch_for_empty_files` was only being called in the case that
  the patch entirely failed to apply, meaning we had a change that
  comprised *only* empty files. Patches with a mix of empty and
  non-empty file changes would apply the non-empty ones and skip the
  rest. The fix for this issue is actually inside /r/14711, which
  reorganizes the top-level patching process to always call this method
  when needed.

Testing Done:
- Ran unit tests.
- Applied a variety of patches that included mixes of empty, non-empty,
  and binary files.

     


    		
    totsrmtxxypqpkrklwumpyuzowqklzov

   



  
 




 













 






 
 
 

  

   

    

     
    

    
    
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
    

   

   

    
     

      Description
      From
      Last Updated
     

    
    



     

      
       
       
local variable 'f' is assigned to but never used Column: 63 Error code: F841

      		
      
       reviewbotreviewbot
      
      
       
      
     


    
   

  

 







 






  

 



















 




 

  


   

   

    
 
Checks run (1 failed, 1 succeeded)


    
   


  

  

  





 

 flake8
 failed.
 





 

 JSHint
 passed.
 













 
flake8

 


 










  

 





 














 



 

  

   




david




   

   

  

 



 















 



 

  

   




david




   

   

  

 



 















 



 

  

   




david




   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  















  

   

    Commits:
   

   

    





 

  
   


    

    Summary
    ID

   

  
  

   


    

    

     

      Add Perforce binary file patching and fix up existing patch issues.
      
This change adds support to the `PerforceParser` for applying binary
files as part of changes. The most basic part of this is just the
implementation of the add/move/delete methods, but while working on this
I discovered that patching for Perforce was broken in a variety of ways:

* We've traditionally relied on the `patch` command to handle opening
  the relevant files, and we still do, but it now opens and edits them
  in a way which requires running `p4 reconcile`. This command has been
  added to the end of the patch sequence.

* `apply_patch_for_empty_files` would use a regex to pull out filenames
  from the diff that then got passed to `p4 add` or `p4 delete`, but
  that regex only stripped off a leading "//" and did not map filenames
  to client names. It seems like in some cases Perforce was clever
  enough to know that "depot/something/..." really meant
  "//depot/something/...", but that depended highly on what that depot
  was called and what the client mapping was set up to look like. I've
  changed it to map the depot filenames to local first.

* `apply_patch_for_empty_files` was only being called in the case that
  the patch entirely failed to apply, meaning we had a change that
  comprised *only* empty files. Patches with a mix of empty and
  non-empty file changes would apply the non-empty ones and skip the
  rest. The fix for this issue is actually inside /r/14711, which
  reorganizes the top-level patching process to always call this method
  when needed.

Testing Done:
- Ran unit tests.
- Applied a variety of patches that included mixes of empty, non-empty,
  and binary files.

     


    		
    totsrmtxxypqpkrklwumpyuzowqklzov

   




   


    

    

     

      Add Perforce binary file patching and fix up existing patch issues.
      
This change adds support to the `PerforceParser` for applying binary
files as part of changes. The most basic part of this is just the
implementation of the add/move/delete methods, but while working on this
I discovered that patching for Perforce was broken in a variety of ways:

* We've traditionally relied on the `patch` command to handle opening
  the relevant files, and we still do, but it now opens and edits them
  in a way which requires running `p4 reconcile`. This command has been
  added to the end of the patch sequence.

* `apply_patch_for_empty_files` would use a regex to pull out filenames
  from the diff that then got passed to `p4 add` or `p4 delete`, but
  that regex only stripped off a leading "//" and did not map filenames
  to client names. It seems like in some cases Perforce was clever
  enough to know that "depot/something/..." really meant
  "//depot/something/...", but that depended highly on what that depot
  was called and what the client mapping was set up to look like. I've
  changed it to map the depot filenames to local first.

* `apply_patch_for_empty_files` was only being called in the case that
  the patch entirely failed to apply, meaning we had a change that
  comprised *only* empty files. Patches with a mix of empty and
  non-empty file changes would apply the non-empty ones and skip the
  rest. The fix for this issue is actually inside /r/14711, which
  reorganizes the top-level patching process to always call this method
  when needed.

Testing Done:
- Ran unit tests.
- Applied a variety of patches that included mixes of empty, non-empty,
  and binary files.

     


    		
    totsrmtxxypqpkrklwumpyuzowqklzov

   



  
 







   

  





  

   

    Diff:
   

   

    
 Revision 4 (+2282 -62)
Show changes
 
    rbtools/clients/perforce.py 
    rbtools/clients/tests/test_p4.py 
 

   

  











 

  
Checks run (2 succeeded)



  

 flake8
 passed.
 





  

 JSHint
 passed.