Refs #3920

One thing I'd like to discuss is whether we should pass NULL to the binary_cb or, as I do in the patch, a git_diff_binary struct initialized to all-zero. I only took the second approach as passing NULL breaks some tests, so it was more of a lazy shortcut. I'm still open to changing this, though.

We should probably be passing in the minimal information there. The comment for the type doesn't say what fields you can expect to have filled, so it looks like we should define what gets filled and then fill that. Presumably we'd want to pass in the path, object ids and mode so you can display the message that it changed if that's all you're after.

I think I would prefer to pass a NULL here if we didn't load it. The path and object IDs and such are in the delta, and is suitable for a caller that just wanted to emulate Binary files a/foo.txt and b/foo.txt differ.

The git_diff_binary contains the old side and the new side of the binary data which is only useful for displaying the actual binary contents and emulating git diff --binary. If we don't load that data then I think NULL would be very appropriate.

This is a good fix, I'm going to merge it and add NULL handling on top.

Thanks @pks-t !

After poking around with this a bit more, I realized that NULL probably isn't appropriate but that instead a git_diff_delta should indicate whether there's any data in its files... (It can't look at the file sides since it may have been generated by parsing a patch that said Binary files differ or by running git_diff_blob_to_blob on two zero byte files, which are harder to discern.)

While looking at this I realized that we didn't have any way to parse a patch that had Binary files differ situation, so I'm fixing that up.

The aforementioned Binary files differ situation is a little more obnoxious than I had initially thought; there are some tests that assume behavior that I'm not sure makes sense. I'm going to go ahead and merge this to unblock #3920 since this is correct.