Add git binary support.

Review Request #7571 — Created Aug. 5, 2015 and updated

Information

RBTools
master
1b02044...

Reviewers

git diff only contains the difference between binary files when exactly with "--binary" params. When use rbtools to submit path, currently it could not contains binary files. So my patch is to fix this problem.

nosetests -v

reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/clients/git.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/clients/git.py
    
    
  2. 
      
chipx86
  1. Can you go into more detail on this change? We require a Description field that clearly states the need/purpose for the change, and a Testing Done that goes into step-by-step detail of how this was tested (Git version(s), prep work, result).

    1. Yes, I test done.

  2. 
      
HA
Review request changed
Description:
~  

See summary.

  ~

git diff only contains the difference between binary files when exactly with "--binary" params. When use rbtools to submit path, currently it could not contains binary files. So my patch is to fix this problem.

Testing Done:
  +

nosetests -v

david
  1. I still don't really understand how this is supposed to improve things. Review Board doesn't know what to do with git binary patches, so at best it will be ignored and at worst it will show an error instead of what it currently does, which is to show "XXX is a binary file"

    1. We use this to update binary file in review board interally. And it runs well and could apply binary changes. Because binary chages become base64 in patches.

    2. Perhaps show me some screenshots of what you see in Review Board's diff viewer after posting changes with this? Because I'm 100% sure that Review Board doesn't know what to do with diffs that include git binary blobs.

    3. We have longer-term plans for handling binary files, but it won't be done with Git's binary diffs.

    4. @david Please see this review board https://reviews.apache.org/r/40053/diff/1#index_header 3rdparty/libprocess/3rdparty/boost-1.53.0.tar.gz is a binary file. And after I apply this patch to rbtools, I could add it to review board correctly.

    5. Can you attach a screenshot of what happens in the diff viewer when you "add it to review board correctly"?

    6. screenshot

    7. I think what haosdent was trying to fix (and the same issue that we run into regularly in Mesos) is that our bots and tooling are unable to pull down the binary portions of patches from reviewboard.

      An example is in this patch:
      https://reviews.apache.org/r/67987/

      Our bot tries to apply it at the end of the patch chain here:
      https://reviews.apache.org/r/67992/

      And it's unable to:

      error: missing binary patch data for '3rdparty/rapidjson-1.1.0.tar.gz'
      error: binary patch does not apply to '3rdparty/rapidjson-1.1.0.tar.gz'
      error: 3rdparty/rapidjson-1.1.0.tar.gz: patch does not apply
      

      Apparently, this patch fixes the issue. Does that help clarify the intent?

    8. In fact, it is not (only) the pulling down of binary portions - the problem already arises when trying to publish a binary patch.
      
      I recently hit this problem again when trying to upgrade some binary dependencies within the Apache Mesos repo. The failure to publish a binary patch is not apparent to the user - hence a bit of a trap. After applying haosdent's patch, things went smooth like butter. The results shown in reviewboard may not be ideal, but that is not a problem. I for one would highly appreciate a short-term fix - which is what haosdent has generously proposed.
  2. 
      
tillt
  1. Ship It!
  2.