So then it would not be possible to produce a diff that mixed binary and text diffs? If `--binary` is for email patches, then it should still be easy to detect non-email-safe characters (probably anything that isn't a printable ASCII character). Also if git's binary detection is really just checking for a NUL byte, then it could be improved. Example: checking if bytes form any common encoding, like UTF-8, and if not then marking it as a binary. Or more advanced `file` like heuristics. I don't know if this is worth the effort, or if others think the current binary detection is fine (it likely works fine for actual executables, but not short binary data like my example). On Mon, Jul 13, 2020, 6:39 PM brian m. carlson <sandals@xxxxxxxxxxxxxxxxxxxx> wrote: > > On 2020-07-13 at 04:58:23, Jason Xu wrote: > > Hello everyone, > > > > Here are two small files with non-ASCII data, yet git diff considers > > them text instead of binary. > > > > echo -n -e '\x01\xff\xdf' > bin1 > > echo -n -e '\x01\xdf\xff' > bin2 > > git diff --binary bin1 bin2 > > > > Result: > > > > diff --git a/bin1 b/bin2 > > index 802dc8e..c39b638 100644 > > --- a/bin1 > > +++ b/bin2 > > @@ -1 +1 @@ > > -^A<FF><DF> > > \ No newline at end of file > > +^A<DF><FF> > > \ No newline at end of file > > > > I think `--binary` for `git diff` should force the creation of git > > binary patches if there are any non-printable-ASCII characters, since > > my understanding is that `--binary` is for safe encoding for email. > > Otherwise `-a` can be used. > > I was the one that asked you to post this here. While Junio's comment > explains _why_ this happens, I think that --binary should, well, produce > a binary patch, regardless of whether the file is text or binary. After > all, --text does the opposite. > > The description for the --binary option reads as follows: > > In addition to --full-index, output a binary diff that can be applied > with git-apply. Implies --patch. > > So we need to fix either the documentation or the code. > > I looked into what it takes to fix the code to do this; it's fairly > straightforward, but it does cause some testsuite failures which assume > the current behavior and will likely involve a small series. So if > other folks agree, I'm happy to pick this up in the next couple of weeks > and add support for it that would hopefully hit Git 2.29. > -- > brian m. carlson: Houston, Texas, US
