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

From: Jeff King <peff@peff.net>
Date: Oct 13, 2008, 01:23 UTC
Message-ID: <20081013012311.GE3768@coredump.intra.peff.net>
In-Reply-To: <7vabdaidwj.fsf@gitster.siamese.dyndns.org>

On Sat, Oct 11, 2008 at 10:24:44PM -0700, Junio C Hamano wrote:

Show 8 quoted lines

> >   echo '* diff=foo' >subdir/.gitattributes
> >   git config diff.foo.funcname some-regex
> > ...
> > In practice, this doesn't happen much, because funcname tends to follow
> > the file extension, as does binary-ness.
> 
> I find this a highly contrived example.  Is this ever be useful in
> practice?

Well, I was doing something like it. But after reading JSixt's messages, I think I agree that I was probably abusing the attributes system.

Show 6 quoted lines

> The logic behind the original behaviour was that the file ought to be
> "diff-able" if you are setting up funcname pattern because the funcname
> pattern only makes sense if you are doing the textual diff.  In other
> words, "should we do textual diff?" and "what funcname pattern should we
> use?" are _not_ orthogonal, as wanting to configure the latter does imply
> that you do want to see the change in the textual diff format.

Yeah, I don't think I can really disagree with that. I had some vague notion that it opens the path for adding orthogonal options later. But really, I'm not sure that any exist, since they are, by definition related to the diff. Unless we want to have diff driver options for how to do a binary diff.

Show 5 quoted lines

> For the same rationale, if you have .textconv, I think it is natural for
> us to say that you do want to see the change in the textual diff format.
> So I'd agree that you can get rid of this .binary business by saying that
> having .textconv marks it diffable (IOW, I think your first alternative
> makes more sense).

OK. My re-rolled series will keep the assumption that a diff=* attribute makes a file non-binary. However, I will still include the 'binary' struct member in the diff driver, as it greatly simplifies the code. It is trivial then to support "diff.*.binary" (which would default to 'false') if we feel like it.

-Peff