Re: [PATCHv2 1/2] attr: map builtin userdiff drivers to well-known extensions

[Jeff King <peff@peff.net>]

On Mon, Dec 19, 2011 at 12:07:33PM -0600, Jonathan Nieder wrote:

> > The C mappings are still here, but see the next patch.
> 
> This is adding a regression in order to remove it.  I guess it's
> harmless, but I don't see the point.

It's purely an attempt to help somebody reading "git log" later
understand what happened. Maybe a comment in the commit message is more
appropriate.

> > +test_expect_success 'binary files are not considered text by file extension' '
> > +	echo Q | q_to_nul >binary.c &&
> > +	git add binary.c &&
> > +	cat >expect <<-\EOF &&
> > +	diff --git a/binary.c b/binary.c
> > +	new file mode 100644
> > +	index 0000000..1f2a4f5
> > +	Binary files /dev/null and b/binary.c differ
> > +	EOF
> > +	git diff --cached binary.c >actual &&
> > +	test_cmp expect actual
> 
> Re the idea of this test: very good idea.
> 
> Re the mechanics: I would have been happier to see
> 
> 	echo Q | q_to_nul >binary.c &&
> 	git add binary.c &&
> 	git diff --cached binary.c >diff &&
> 	grep Binary files diff

Yeah, I think that's fine, and I'll squash it in to my local version.

It does miss one problem, though (that is also present in my original):
using "binary.c" is no longer a good name, since the next patch will
revert the "*.c" bits. :)

> > --- a/t/t4018-diff-funcname.sh
> > +++ b/t/t4018-diff-funcname.sh
> > @@ -124,7 +124,9 @@ do
> >  done
> >  
> >  test_expect_success 'default behaviour' '
> > -	rm -f .gitattributes &&
> > +	cat >.gitattributes <<-\EOF &&
> > +	*.java diff=default
> > +	EOF
> >  	test_expect_funcname "public class Beer\$"
> >  '
> 
> 	echo "*.java diff=default" >.gitattributes
> 
> would do the same with two lines fewer. :)

Yup. I was following the style of the test directly below, which sets
both java and perl drivers. But the "default" test that needed updating
only checks the java case.

Will squash.

> Thanks for working on this.  I owe you a beer.

You're welcome. :)

-Peff