Mismatched binary detection behaviour on diffs

[FintanH]

Hey 👋

First off, apologies that this is via the git2 crate, I'm better at Rust than I am C 😬 I also reported the issue on the git2 repository rust-lang/git2-rs#991.

Reproduction steps

If you clone this repository https://github.com/FintanH/git2-binary-check and do cargo run

Expected behavior

Path: "bin/cat"
Is binary: true
Path: "bin/ls"
Is binary: true
Path: "bin/test"
Is binary: true

I'll note here that git diff 373b3404371e359404392aff715d504293112c8b abdedcec89e3c48ac520e4a96c7ea39364316b9e will output:

diff --git a/bin/cat b/bin/cat
new file mode 100755
index 0000000..0bbb14b
Binary files /dev/null and b/bin/cat differ
diff --git a/bin/ls b/bin/ls
new file mode 100755
index 0000000..87c2d51
Binary files /dev/null and b/bin/ls differ
diff --git a/bin/test b/bin/test
new file mode 100755
index 0000000..0cced79
Binary files /dev/null and b/bin/test differ

Actual behavior

Path: "bin/cat"
Is binary: false
Path: "bin/ls"
Is binary: false
Path: "bin/test"
Is binary: false

Version of libgit2 (release number or SHA1)

I believe it's v0.16.1 based off of this output:

cargo tree
git2-binary-check v0.1.0 
└── git2 v0.18.1
    ├── bitflags v2.4.0
    ├── libc v0.2.148
    ├── libgit2-sys v0.16.1+1.7.1

Operating system(s) tested

NixOS 23.05 (Stoat)

[ethomson]

I haven't dug in here yet, but I suspect that this is an optimization around not loading the content when there's no content callbacks configured.

See the documentation for git_diff_delta:

Under some circumstances, in the name of efficiency, not all fields will
be filled in, but we generally try to fill in as much as possible.  One
example is that the "flags" field may not have either the `BINARY` or the
`NOT_BINARY` flag set to avoid examining file contents if you do not pass
in hunk and/or line callbacks to the diff foreach iteration function.  It
will just use the git attributes for those files.

This is not the first time that I've been bitten by this. Probably this behavior should be optional.

But in the meantime, if you wire up a callback do you get the expected behavior?

[FintanH]

Ya, you're right. When I added the call to foreach:

diff.foreach(&mut |_, _| true, None, None, None).unwrap();

just before iterating over the deltas, I do get back that the binaries are true.

[ethomson]

OK, thanks for validating that. I'm going to keep this open because I want to change this behavior somehow.

[FintanH]

OK, thanks for validating that. I'm going to keep this open because I want to change this behavior somehow.

Appreciate that!