Re: [PATCH 3/4] diff: introduce diff.<driver>.binary

From: Jeff King <peff@peff.net>
Date: Oct 7, 2008, 15:35 UTC
Message-ID: <20081007153543.GA26531@coredump.intra.peff.net>
In-Reply-To: <48EB7D74.40302@viscovery.net>

On Tue, Oct 07, 2008 at 05:17:08PM +0200, Johannes Sixt wrote:

Show 7 quoted lines

> > However, there is at least one conflicting situation: there
> > is no way to say "use the regular rules to determine whether
> > this file is binary, but if we do diff it textually, use
> > this funcname pattern." That is, currently setting diff=foo
> > indicates that the file is definitely text.
> 
> I don't get your point. Can you provide an example?

Let's say I have a subdirectory of files, some of which are binary. But for those that _aren't_ binary, I want to use a particular funcname pattern. So I do this:

  echo '* diff=foo' >subdir/.gitattributes
  git config diff.foo.funcname some-regex

Under the old behavior, I have just claimed all of the subdir as text. But that's not necessarily what I wanted.

In practice, this doesn't happen much, because funcname tends to follow the file extension, as does binary-ness. So you get "*.java diff=java", and you really would be insane to have binary files named *.java. But I think it makes the concept clearer to explain, and the code simpler, if the various facets of a diff driver are orthogonal. In particular, I think this makes things cleaner for adding new driver properties in the future.

As to your complaint (which I think is valid)...

> The reason why I'd like to understand the issue is because I like the
> diff.foo.textconv that you introduce in patch 4/4, but I dislike that I
> have to set diff.foo.binary to false in order to use the textconv. So, why
> is this .binary needed?

I think this .binary is something we can and should get rid of; as it stands now, you always end up having to set it along with .textconv. I have considered two ways:

  - because textconv is for converting binary to text for diffing, the
    result should always be text. So whenever we do the conversion, we
    should note that it is no longer binary, and automatically treat the
    result as a text diff. So in this case, we are explicitly saying
    that binaryness is _not_ orthogonal to this property of the diff
    driver.

  - textconv should arguably just be "canonicalize" or similar. That is,
    there is no reason you couldn't take something like text XML and
    canonicalize it for a more readable diff. Or even canonicalize a
    binary file to get a smaller or more sensible binary diff, if you
    wanted to.

    In that case, I think the right thing is to set it back to "unknown,
    check the file contents" if .binary is not set. So you really are
    saying "this is just a conversion, treat the canonicalized contents
    exactly as you would have treated the actual contents, including
    binary detection magic".

And both of those obviously involve changes to patch 4/4.

-Peff