Repository navigation
patch_generate: only calculate binary diffs if requested - #3922
ethomson merged 1 commit into
Conversation
Sorry, something went wrong.
When generating diffs for binary files, we load and decompress the blobs in order to generate the actual diff, which can be very costly. While we cannot avoid this for the case when we are called with the `GIT_DIFF_SHOW_BINARY` flag, we do not have to load the blobs in the case where this flag is not set, as the caller is expected to have no interest in the actual content of binary files. Fix the issue by only generating a binary diff when the caller is actually interested in the diff. As libgit2 uses heuristics to determine that a blob contains binary data by inspecting its size without loading from the ODB, this saves us quite some time when diffing in a repository with binary files.
|
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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
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 ! |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
When generating diffs for binary files, we load and decompress
the blobs in order to generate the actual diff, which can be very
costly. While we cannot avoid this for the case when we are
called with the GIT_DIFF_SHOW_BINARY flag, we do not have to
load the blobs in the case where this flag is not set, as the
caller is expected to have no interest in the actual content of
binary files.
Fix the issue by only generating a binary diff when the caller is
actually interested in the diff. As libgit2 uses heuristics to
determine that a blob contains binary data by inspecting its size
without loading from the ODB, this saves us quite some time when
diffing in a repository with binary files.