Re: [PATCHv2 1/2] attr: map builtin userdiff drivers to well-known extensions
- From
Jeff King <peff@peff.net>
- Date
- Dec 19, 2011, 18:55 UTC
- Message-ID
- <20111219185511.GA3852@sigill.intra.peff.net>
- In-Reply-To
- <20111219180733.GA12200@elie.hsd1.il.comcast.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.
Show 20 quoted lines
> > +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. :)
Show 16 quoted lines
> > --- 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