Summary

Add git binary support.

Review Request #7571 — Created Aug. 5, 2015 and updated Sept. 12, 2018, 6:17 p.m.

Information

Owner

haosdent

Repository

RBTools

Branch

master

Bugs

Depends On

Commit

1b02044...

Reviewers

Groups

rbtools

People

brennie

Description

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

Tool: Pyflakes
Processed Files:
    rbtools/clients/git.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/clients/git.py

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).

HA

haosdent Aug. 7, 2015, 1:58 a.m.
```
Yes, I test done.
```

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

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"

haosdent Oct. 11, 2015, 9:38 a.m.

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.

david Oct. 11, 2015, 6:26 p.m.

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.

chipx86 Oct. 11, 2015, 6:27 p.m.

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

haosdent Nov. 7, 2015, 11:06 p.m.

@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.

david Nov. 18, 2015, 2:25 p.m.

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

HA

haosdent March 23, 2016, 11:18 a.m.

bmahler July 20, 2018, 2:37 p.m.

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?

tillt Sept. 12, 2018, 3:05 p.m.

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.

Ship it!

```
Ship It!
```