Re: [RFC/PATCH] attr: map builtin userdiff drivers to well-known extensions

From: Jeff King <peff@peff.net>
Date: Aug 26, 2011, 02:59 UTC
Message-ID: <20110826025913.GC17625@sigill.intra.peff.net>
In-Reply-To: <7v8vqhhzgd.fsf@alter.siamese.dyndns.org>

On Thu, Aug 25, 2011 at 03:57:06PM -0700, Junio C Hamano wrote:

Show 22 quoted lines

> > If you have any matching attribute line in your own files, it should
> > override. So:
> >
> >   foo/* -diff
> >
> > will still mark foo/bar.c as binary, even with this change.
> >
> > Can anyone think of other possible side effects?
> >
> > Also, any other extensions that would go into such a list? I have no
> > idea what the common extension is for something like pascal or csharp.
> 
> As long as the builtin ones are the lowest priority fallback, we should be
> Ok.
> 
> Do we say anywhere that "Ah, this has 'diff' attribute defined, so it must
> be text"? If so, we should fix _that_. In other words, having this one
> extra entry
> 
> 	"* diff=default"
> 
> in the builtin_attr[] array should be a no-op, I think.

No, certainly not since 122aa6f (diff: introduce diff.<driver>.binary, 2008-10-05). That commit's message claims that we did before it, but looking at the patch, I am not so sure. But I'm not about to start testing a 3-year-old patch to see if it really was the source of the fix; the point is that it is correct now. :)

I think it could be a problem in the future if the builtin userdiff drivers started growing more invasive options, like automatically claiming to be non-binary (i.e., setting diff.cpp.binary = false by default). In other words, I think we have two options:

  1. Builtin drivers like "cpp" can stay minimal, only setting funcname
     and color-words headers that aren't going to produce terrible
     results if we are wrong about detecting by extension.

  2. We force the user to identify file types manually, so we can't be
     wrong. The "cpp" diff driver means "you are a text C file", and if
     a user mis-marks a binary file with that diff driver, they are the
     one who is wrong.

So if it's an either/or situation, we should decide not only that extension auto-detection is a good feature, but that it trumps adding more advanced features to the builtin drivers in the future.

Or we could decide that the extensions really are good enough, and if you really do have binary files named "foo.c", it's your problem to override the defaults with "*.c -diff".

-Peff