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

[Jeff King <peff@peff.net>]

On Fri, Dec 16, 2011 at 03:00:51PM +0100, Johannes Sixt wrote:

> Am 12/16/2011 12:00, schrieb Jeff King:
> >  static const char *builtin_attr[] = {
> ...
> > +	"*.c diff=cpp",
> > +	"*.cc diff=cpp",
> > +	"*.cxx diff=cpp",
> > +	"*.cpp diff=cpp",
> > +	"*.h diff=cpp",
> > +	"*.hpp diff=cpp",
> 
> Please don't do this. It would be a serious regression for C++ coders, and
> some C coders as well. The built-in hunk header patterns are severly
> broken and don't work well with C++ code. I know for sure that the
> following are not recognized:
> 
> - template declarations, e.g. template<class T> func(T x);
> - constructor definitionss, e.g. MyClass::MyClass()
> - functions that return references, e.g. const string& func()
> - function definitions along the GNU coding style, e.g.
> 
>      void
>      the_func ()

Hmm. I think it's a legitimate criticism to say "hunk-header detection
is a broken feature because our heuristics aren't good enough, and we
shouldn't start using it by default because people will complain because
it sucks too much".

At the same time, I think we have seen people complaining that the
regular dumb funcname detection is not good enough[1], and that using
language-specific funcnames, while not 100% perfect, produces better
results on the whole.

So I think rather than saying "this doesn't always work", it's important
to ask "on the whole, does this tend to produce better results than
without, and when we are wrong, how bad is it?"

I'm not clear from what you wrote on whether you were saying it is
simply sub-optimal, or whether on balance it is way worse than the
default funcname matching.

And if it is bad on balance, is the right solution to avoid exposing
people to it, or is it to make our patterns better? I.e., is it fixable,
or is it simply too hard a problem to get right in the general case, and
we shouldn't turn it on by default?

> I am currently using this pattern (but I'm sure it can be optimized) with
> an appropriate xcpp attribute:
> 
> [diff "xcpp"]
>         xfuncname = "!^[
> \\t]*[a-zA-Z_][a-zA-Z_0-9]*[^()]*:[[:space:]]*$\n^[a-zA-Z_][a-zA-Z_0-9]*.*"

So, I'm confused. If you are using this, surely you have "*.c diff=xcpp"
in your attributes file, and my patch has no effect for you, as it is
lower precedence than user-supplied gitattributes? Also, if you called
it diff.cpp.xfuncname, then wouldn't my patch still be useful, as your
complaint is not "my *.c files are not actually C language" but "the C
language driver sucks" (but you be remedying that by providing your own
config).

-Peff