585: post-review dies when binaries are part of a p4 changeset

may****@gmai***** (Google Code) (Is this you? Claim this profile.)
March 27, 2009
991, 1059 
What steps will reproduce the problem?
1. Add a binary to a p4 changeset
2. post-review that changeset


What is the expected output? What do you see instead?
I expected post-review to complete successfully with a link to a
reviewboard diff, but instead get this:

Traceback (most recent call last):
  File "/build/trees/bin/reviewboard/post-review", line 452, in <module>
    main()
  File "/build/trees/bin/reviewboard/post-review", line 448, in main
    tempt_fate(changenum, repository_path, client_root)
  File "/build/trees/bin/reviewboard/post-review", line 389, in tempt_fate
    upload_diff(review_request, changenum)
  File "/build/trees/bin/reviewboard/post-review", line 366, in upload_diff
    diff_content = generate_diff(changenum)
  File "/build/trees/bin/reviewboard/post-review", line 346, in generate_diff
    m = re.search(r'(\d\d\d\d-\d\d-\d\d \d\d:\d\d:\d\d)', dl[1])
IndexError: list index out of range


What operating system are you using? What browser?
Linux/Gentoo 2007.0.


Please provide any additional information below.
This error happened consistently when I tried to add the p4 binary itself
to a changeset that already included two other text files.  When I created
a new test changeset with just the p4 binary by itself, post-review
completed successfully but the diff showed a lot of gibberish instead of
perhaps just listing the file name and the fact that it was a binary.  When
I added the GNU 'ls' binary to this changeset (so the changeset included
the p4 binary and the ls binary) then the same error (listed above) occured
again and post-review did not complete successfully.
chipx86
#1 chipx86 
This is an internal problem in VMware's script, since it's quite old, and won't be
fixed until we finally migrate everything to the new architecture and new script.
  • +VMware
    +Component-Scripts
#2 pmcla*****@gmai***** (Google Code) (Is this you? Claim this profile.) 
I've had this same issue and done some debugging. The problem is on line 975 and is 
caused by that line not evaluating to True. I'm still not sure why this is, but for 
me on Windows I had thought it was due to line ending issues, but since I now see 
this other bug report on Linux, I'm not so sure. In any case, the attached patch 
fixed it for me. I basically replaced the direct string equality check with a regex 
match. It's possible that just using 'dl[0].startswith()' instead of '==' or my regex 
would work as well, but I haven't tried that. Maybe this will help mayesgr get revews 
submitted.
  • +
    post-review.patch 
    Index: post-review
    ===================================================================
    --- post-review (revision 1637)
    +++ post-review (working copy)
    @@ -972,7 +972,7 @@
                 # and the code below expects the outptu to start with
                 #     "Binary files "
                 if len(dl) == 1 and \
    -               dl[0] == ('Files %s and %s differ'% (old_file, new_file)):
    +               re.match(r'Files .+ and .+ differ$', dl[0]):
                     dl = ['Binary files %s and %s differ'% (old_file, new_file)]
                 if dl == [] or dl[0].startswith("Binary files "):
    
 
    

    


    
   
    • 


  


 








 

  

   chipx86

  

 
 

  

    
     

   
   #3
   chipx86


  


  
Fixed in r1862.



  
    


   
  • 
    
    
    +Fixed
    
   
    • 


  


 








 

  

   chipx86

  

 
 

  

    
     

   
   #4
   chipx86


  



  

 








 

  

   chipx86

  

 
 

  

    
     

   
   #5
   chipx86