{"thread":{"id":"15276","subject":"[PATCH] diff: treat -crlf files as binary","startedAt":"2008-08-29T21:28:20Z","lastAt":"2008-08-31T18:34:47Z","messageCount":8,"participants":["Junio C Hamano","Avery Pennarun","Matthieu Moy","Alex Riesen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"89161","messageId":"7vfxon4ikr.fsf@gitster.siamese.dyndns.org","threadId":"15276","inReplyTo":null,"subject":"[PATCH] diff: treat -crlf files as binary","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-29T21:28:20Z","receivedAt":"2008-08-29T21:28:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The manual advertises that setting \"crlf\" attribute to false marks the\nfile as binary. We should pay attention to this condition in addition\nto the \"do not diff\" attribute (i.e. setting \"diff\" to false) when\ndeciding not to show the textual diff.\n\nSigned-off-by: Junio C Hamano \n---\n\n * Strictly speaking any change is backward incompatible, and this is\n certainly one, but I do not think of a good use case to depend on the\n previous behaviour, which was reported as a bug by my coworker.\n\n diff.c | 28 +++++++++++++++++++---------\n 1 files changed, 19 insertions(+), 9 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 18fa7a7..258b438 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1314,36 +1314,46 @@ static void emit_binary_diff(FILE *file, mmfile_t *one, mmfile_t *two)\n static void setup_diff_attr_check(struct git_attr_check *check)\n {\n \tstatic struct git_attr *attr_diff;\n+\tstatic struct git_attr *attr_crlf;\n \n-\tif (!attr_diff) {\n+\tif (!attr_diff)\n \t\tattr_diff = git_attr(\"diff\", 4);\n-\t}\n+\tif (!attr_crlf)\n+\t\tattr_crlf = git_attr(\"crlf\", 4);\n \tcheck[0].attr = attr_diff;\n+\tcheck[1].attr = attr_crlf;\n }\n \n static void diff_filespec_check_attr(struct diff_filespec *one)\n {\n-\tstruct git_attr_check attr_diff_check;\n+\tstruct git_attr_check attr_diff_check[2];\n \tint check_from_data = 0;\n \n \tif (one->checked_attr)\n \t\treturn;\n \n-\tsetup_diff_attr_check(&attr_diff_check);\n+\tsetup_diff_attr_check(attr_diff_check);\n \tone->is_binary = 0;\n \tone->funcname_pattern_ident = NULL;\n \n-\tif (!git_checkattr(one->path, 1, &attr_diff_check)) {\n+\tif (!git_checkattr(one->path, 2, attr_diff_check)) {\n \t\tconst char *value;\n \n-\t\t/* binaryness */\n-\t\tvalue = attr_diff_check.value;\n+\t\t/* binaryness; check both \"diff\" and \"crlf\" */\n+\t\tvalue = attr_diff_check[0].value;\n \t\tif (ATTR_TRUE(value))\n \t\t\t;\n \t\telse if (ATTR_FALSE(value))\n \t\t\tone->is_binary = 1;\n-\t\telse\n-\t\t\tcheck_from_data = 1;\n+\t\telse {\n+\t\t\tconst char *crlf = attr_diff_check[1].value;\n+\t\t\tif (ATTR_TRUE(crlf))\n+\t\t\t\t;\n+\t\t\telse if (ATTR_FALSE(crlf))\n+\t\t\t\tone->is_binary = 1;\n+\t\t\telse\n+\t\t\t\tcheck_from_data = 1;\n+\t\t}\n \n \t\t/* funcname pattern ident */\n \t\tif (ATTR_TRUE(value) || ATTR_FALSE(value) || ATTR_UNSET(value))\n-- \n1.6.0.1.90.g27a6e\n"},{"id":"89166","messageId":"32541b130808291456k3de953a2yd1e93bc27ad14293@mail.gmail.com","threadId":"15276","inReplyTo":"7vfxon4ikr.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] diff: treat -crlf files as binary","fromName":"Avery Pennarun","fromEmail":"apenwarr@gmail.com","sentAt":"2008-08-29T21:56:38Z","receivedAt":"2008-08-29T21:56:38Z","isPatch":true,"sender":{"key":"apenwarr@gmail.com","avatar":"https://avatars.githubusercontent.com/u/20592?v=4"},"body":"On Fri, Aug 29, 2008 at 5:28 PM, Junio C Hamano wrote:\n> The manual advertises that setting \"crlf\" attribute to false marks the\n> file as binary. We should pay attention to this condition in addition\n> to the \"do not diff\" attribute (i.e. setting \"diff\" to false) when\n> deciding not to show the textual diff.\n>\n> Signed-off-by: Junio C Hamano \n> ---\n>\n> * Strictly speaking any change is backward incompatible, and this is\n> certainly one, but I do not think of a good use case to depend on the\n> previous behaviour, which was reported as a bug by my coworker.\n\nI'm not sure this is a good idea. In our repository, for example, we\nhave a few files that are strictly speaking \"text\" but absolutely,\npositively must be CRLF on all platforms because the idiotic\nproprietary parsers and generators that manipulate them need it to be\nthat way.\n\nI think the bug is that \"crlf=false\" should not be considered the same\nas \"binary=true\", which seems to be a bug in the documentation, not\nthe program.\n\nHave fun,\n\nAvery\n"},{"id":"89268","messageId":"7vljyefaps.fsf@gitster.siamese.dyndns.org","threadId":"15276","inReplyTo":"32541b130808291456k3de953a2yd1e93bc27ad14293@mail.gmail.com","subject":"Re: [PATCH] diff: treat -crlf files as binary","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-30T21:34:55Z","receivedAt":"2008-08-30T21:34:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Avery Pennarun\" writes:\n\n> I think the bug is that \"crlf=false\" should not be considered the same\n> as \"binary=true\", which seems to be a bug in the documentation, not\n> the program.\n\nYeah, that's right. How about doing something like this?\n\n Documentation/gitattributes.txt | 39 ++++++++++++++++++++++++++++++++++++---\n 1 files changed, 36 insertions(+), 3 deletions(-)\n\ndiff --git i/Documentation/gitattributes.txt w/Documentation/gitattributes.txt\nindex db16b0c..ec8a860 100644\n--- i/Documentation/gitattributes.txt\n+++ w/Documentation/gitattributes.txt\n@@ -105,9 +105,8 @@ Set::\n \n Unset::\n \n-\tUnsetting the `crlf` attribute on a path is meant to\n-\tmark the path as a \"binary\" file. The path never goes\n-\tthrough line endings conversion upon checkin/checkout.\n+\tUnsetting the `crlf` attribute on a path is tells git\n+\tnot to attempt any end-of-line conversion upon checkin/checkout.\n \n Unspecified::\n \n@@ -482,6 +481,40 @@ in the file. E.g. the string `$Format:%H$` will be replaced by the\n commit hash.\n \n \n+USING ATTRIBUTE MACROS\n+----------------------\n+\n+You do not want any end-of-line conversions applied to, nor textual diffs\n+produced for any binary file you track. You would need to specify e.g.\n+\n+------------\n+*.jpg -crlf -diff\n+------------\n+\n+but that is cumbersome. Using attribute macros, you can specify groups of\n+attributes set or unset at the same time. The system knows a built-in\n+attribute macro, `binary`:\n+\n+------------\n+*.jpg binary\n+------------\n+\n+which is equivalent to the above. Note that the attribute macros can only\n+be \"Set\" (see the above example).\n+\n+\n+DEFINING ATTRIBUTE MACROS\n+-------------------------\n+\n+Custom attribute macros can be defined only in the `.gitattributes` file\n+at the toplevel (i.e. not in any subdirectory). The built-in attribute\n+macro \"binary\" is equivalent to:\n+\n+------------\n+[attr]binary -diff -crlf\n+------------\n+\n+\n EXAMPLE\n -------\n \n"},{"id":"89280","messageId":"32541b130808301934t6034d52bva0f8c3608a88e585@mail.gmail.com","threadId":"15276","inReplyTo":"7vljyefaps.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] diff: treat -crlf files as binary","fromName":"Avery Pennarun","fromEmail":"apenwarr@gmail.com","sentAt":"2008-08-31T02:34:01Z","receivedAt":"2008-08-31T02:34:01Z","isPatch":true,"sender":{"key":"apenwarr@gmail.com","avatar":"https://avatars.githubusercontent.com/u/20592?v=4"},"body":"On Sat, Aug 30, 2008 at 5:34 PM, Junio C Hamano wrote:\n> Unset::\n>\n> - Unsetting the `crlf` attribute on a path is meant to\n> - mark the path as a \"binary\" file. The path never goes\n> - through line endings conversion upon checkin/checkout.\n> + Unsetting the `crlf` attribute on a path is tells git\n> + not to attempt any end-of-line conversion upon checkin/checkout.\n[...]\n\nWell, _I_ understand it :)\n\nAvery\n"},{"id":"89295","messageId":"vpqk5dxmvx0.fsf@bauges.imag.fr","threadId":"15276","inReplyTo":"7vljyefaps.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] diff: treat -crlf files as binary","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2008-08-31T08:27:23Z","receivedAt":"2008-08-31T08:27:23Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"Junio C Hamano writes:\n\n> +\tUnsetting the `crlf` attribute on a path is tells git\n> +\tnot to attempt any end-of-line conversion upon checkin/checkout.\n\nIf you apply it, and to be picky: typo (\"is tells\") and strange\nformatting (double space before upon).\n\nOther than that, it seems to be a good change, yes.\n\n-- \nMatthieu\n"},{"id":"89297","messageId":"81b0412b0808310216h6c349e1en740a1f4c7fef75e6@mail.gmail.com","threadId":"15276","inReplyTo":"7vljyefaps.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] diff: treat -crlf files as binary","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-08-31T09:16:29Z","receivedAt":"2008-08-31T09:16:29Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"2008/8/30 Junio C Hamano :\n> diff --git i/Documentation/gitattributes.txt w/Documentation/gitattributes.txt\n> index db16b0c..ec8a860 100644\n> --- i/Documentation/gitattributes.txt\n> +++ w/Documentation/gitattributes.txt\n> @@ -105,9 +105,8 @@ Set::\n>\n> Unset::\n>\n> - Unsetting the `crlf` attribute on a path is meant to\n> - mark the path as a \"binary\" file. The path never goes\n> - through line endings conversion upon checkin/checkout.\n> + Unsetting the `crlf` attribute on a path is tells git\n> + not to attempt any end-of-line conversion upon checkin/checkout.\n\n\"+ Unsetting the `crlf` attribute on a path tells git\"\n\n(remove \"is\" before \"tells\")\n"},{"id":"89322","messageId":"7vej45b18y.fsf@gitster.siamese.dyndns.org","threadId":"15276","inReplyTo":"81b0412b0808310216h6c349e1en740a1f4c7fef75e6@mail.gmail.com","subject":"Re: [PATCH] diff: treat -crlf files as binary","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-31T16:25:17Z","receivedAt":"2008-08-31T16:25:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Alex Riesen\" writes:\n\n> 2008/8/30 Junio C Hamano :\n>> diff --git i/Documentation/gitattributes.txt w/Documentation/gitattributes.txt\n>> ...\n>> - through line endings conversion upon checkin/checkout.\n>> + Unsetting the `crlf` attribute on a path is tells git\n>> ...\n>\n> \"+ Unsetting the `crlf` attribute on a path tells git\"\n>\n> (remove \"is\" before \"tells\")\n\nYeah, thanks. I reworded the sentence number of times and somehow crufts\nwere left behind. Fixed, together with the double space before \"upon\",\nbefore committing.\n\nI am actually more surprised by the lack of any surprised comment about\nthe unadvertised presense of \"binary\" (pseudo)attribute, though. The\nsupport has been there since f48fd68 (attribute macro support,\n2007-04-14), two days after introduction of gitattributes mechanism.\n\nMaybe somebody should re-read the log message of that commit, and refine\nthe attribute macros sections the patch under discussion has added.\n\nSomebody who can write and spell English better than me, that is ;-)\n"},{"id":"89331","messageId":"32541b130808311134ia3bdab6w37c26649717085aa@mail.gmail.com","threadId":"15276","inReplyTo":"7vej45b18y.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] diff: treat -crlf files as binary","fromName":"Avery Pennarun","fromEmail":"apenwarr@gmail.com","sentAt":"2008-08-31T18:34:47Z","receivedAt":"2008-08-31T18:34:47Z","isPatch":true,"sender":{"key":"apenwarr@gmail.com","avatar":"https://avatars.githubusercontent.com/u/20592?v=4"},"body":"On Sun, Aug 31, 2008 at 12:25 PM, Junio C Hamano wrote:\n> I am actually more surprised by the lack of any surprised comment about\n> the unadvertised presense of \"binary\" (pseudo)attribute, though. The\n> support has been there since f48fd68 (attribute macro support,\n> 2007-04-14), two days after introduction of gitattributes mechanism.\n\nEr, well, I was actually talking about a *hypothetical* \"binary\"\nattribute when I commented on this in the first place. But when there\nthen turned out to actually be one that worked exactly as I wanted, I\nwas hardly going to be the first to complain :)\n\nHave fun,\n\nAvery\n"}]}