{"thread":{"id":"11628","subject":"[PATCH] treat any file with NUL as binary","startedAt":"2008-01-15T14:28:29Z","lastAt":"2008-01-16T01:59:12Z","messageCount":7,"participants":["Dmitry Potapov","Steffen Prohaska","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"65384","messageId":"1200407309-10992-1-git-send-email-dpotapov@gmail.com","threadId":"11628","inReplyTo":null,"subject":"[PATCH] treat any file with NUL as binary","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2008-01-15T14:28:29Z","receivedAt":"2008-01-15T14:28:29Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"There are two heuristics in Git to detect whether a file is binary\nor text. One in xdiff-interface.c relied on existing NUL byte at\nthe beginning. However, convert.c used a different heuristic, which\nrelied that the number of non-printable symbols is less than 1%.\n\nDue to difference in approaches whether a file is binary or not,\nit was possible that a file that diff treats as binary will not be\ntreated as text by CRLF conversation. This is very confusing for\na user who seeing that 'git diff' shows file as binary expects it\nto be added as binary.\n\nThis patch makes is_binary to consider any file that contains at\nleast one NUL character as binary.\n---\n\nJunio,\n\nI believe that the current behavior where 'git diff' shows me a file\nas binary and then adds it as text with crlf conversation is a bug.\n\nThough, it is not very likely to happen, it still possible cases where\na binary file contains large amount of text. For instance, a tar file\nof text files can be such a file. Probably, word processor that store\ntext in binary format may also generate a file with more 99% printable\ncharacters. So, such files will be considered as text by current convert\nheuristic. Still such files are considered by diff due present of a NUL\ncharacter. This is very confusing for a user to see 'git diff' saying\nthat a file is binary and then having it converted as text. Because I\ndon't think that any real text file (especially one that requires CRLF\nconversation) may contain NUL character, I believe this change should\nimprove binary heuristic and avoid user confusion.\n\nSo, please, consider it for inclusion as a bug fix.\n\n convert.c |    9 +++++++--\n 1 files changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex 5adef4f..a51da1f 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -17,8 +17,8 @@\n #define CRLF_INPUT\t2\n \n struct text_stat {\n-\t/* CR, LF and CRLF counts */\n-\tunsigned cr, lf, crlf;\n+\t/* NUL, CR, LF and CRLF counts */\n+\tunsigned nul, cr, lf, crlf;\n \n \t/* These are just approximations! */\n \tunsigned printable, nonprintable;\n@@ -51,6 +51,9 @@ static void gather_stats(const char *buf, unsigned long size, struct text_stat *\n \t\t\tcase '\\b': case '\\t': case '\\033': case '\\014':\n \t\t\t\tstats->printable++;\n \t\t\t\tbreak;\n+\t\t\tcase 0:\n+\t\t\t\tstats->nul++;\n+\t\t\t\t/* fall through */\n \t\t\tdefault:\n \t\t\t\tstats->nonprintable++;\n \t\t\t}\n@@ -66,6 +69,8 @@ static void gather_stats(const char *buf, unsigned long size, struct text_stat *\n static int is_binary(unsigned long size, struct text_stat *stats)\n {\n \n+\tif (stats->nul)\n+\t\treturn 1;\n \tif ((stats->printable >> 7) < stats->nonprintable)\n \t\treturn 1;\n \t/*\n-- \n1.5.4.rc3.3.g0321b\n"},{"id":"65420","messageId":"A220A113-8D82-4D54-8759-BD7199FC22A8@zib.de","threadId":"11628","inReplyTo":"1200407309-10992-1-git-send-email-dpotapov@gmail.com","subject":"Re: [PATCH] treat any file with NUL as binary","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2008-01-15T21:03:33Z","receivedAt":"2008-01-15T21:03:33Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"\nOn Jan 15, 2008, at 3:28 PM, Dmitry Potapov wrote:\n\n> There are two heuristics in Git to detect whether a file is binary\n> or text. One in xdiff-interface.c relied on existing NUL byte at\n> the beginning. However, convert.c used a different heuristic, which\n> relied that the number of non-printable symbols is less than 1%.\n>\n> Due to difference in approaches whether a file is binary or not,\n> it was possible that a file that diff treats as binary will not be\n> treated as text by CRLF conversation. This is very confusing for\n> a user who seeing that 'git diff' shows file as binary expects it\n> to be added as binary.\n>\n> This patch makes is_binary to consider any file that contains at\n> least one NUL character as binary.\n\nShouldn't the commit message explicitly mention that the solution\nis to make the check in convert.c stricter than the check in\nxdiff-interface.c?  I think a comment in xdiff-interface.c\nwould also be a good thing to remember future developers about\nthis.\n\n\n> ---\n>\n> Junio,\n>\n> I believe that the current behavior where 'git diff' shows me a file\n> as binary and then adds it as text with crlf conversation is a bug.\n>\n> Though, it is not very likely to happen, it still possible cases where\n> a binary file contains large amount of text. For instance, a tar file\n> of text files can be such a file. Probably, word processor that store\n> text in binary format may also generate a file with more 99% printable\n> characters. So, such files will be considered as text by current  \n> convert\n> heuristic. Still such files are considered by diff due present of a  \n> NUL\n> character. This is very confusing for a user to see 'git diff' saying\n> that a file is binary and then having it converted as text. Because I\n> don't think that any real text file (especially one that requires CRLF\n> conversation) may contain NUL character, I believe this change should\n> improve binary heuristic and avoid user confusion.\n\nI think this is a good idea.  When reading the code for the first\ntime it took me some time to accept that we really have different\nways for detecting a binary file and to understand how these\ndetections are related.\n\nI also agree with Dimitry that convert.c should be stricter than\nxdiff-interface.c, because everything else could be confusing. ...\n\n\n> So, please, consider it for inclusion as a bug fix.\n\n... H\u0005ence, this should be considered a bug fix.\n\nThe patch looks good to me.\n\n\tSteffen\n"},{"id":"65434","messageId":"7vsl0yd6r8.fsf@gitster.siamese.dyndns.org","threadId":"11628","inReplyTo":"1200407309-10992-1-git-send-email-dpotapov@gmail.com","subject":"Re: [PATCH] treat any file with NUL as binary","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-15T23:11:07Z","receivedAt":"2008-01-15T23:11:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dmitry Potapov <dpotapov@gmail.com> writes:\n\n> So, please, consider it for inclusion as a bug fix.\n\nSomebody has to go back to the \"git log\" output and the list\narchive to see if you two did not forget other ramifications,\nbecause I vaguely recall this 1% thing was done for a reason and\nLinus had a very good argument (at least back then the argument\nsounded very good to me) supporting the deliberate difference\nbetween the two \"binary\" heuristics.\n\nIf I did not have that vague recollection and all I had to judge\nthe proposed change were the issue in this thread, I'd probably\nagree this is a good change, though.\n"},{"id":"65450","messageId":"20080116011321.GD13984@dpotapov.dyndns.org","threadId":"11628","inReplyTo":"7vsl0yd6r8.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] treat any file with NUL as binary","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2008-01-16T01:13:21Z","receivedAt":"2008-01-16T01:13:21Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"On Tue, Jan 15, 2008 at 03:11:07PM -0800, Junio C Hamano wrote:\n> Dmitry Potapov <dpotapov@gmail.com> writes:\n> \n> > So, please, consider it for inclusion as a bug fix.\n> \n> Somebody has to go back to the \"git log\" output and the list\n> archive to see if you two did not forget other ramifications,\n> because I vaguely recall this 1% thing was done for a reason and\n> Linus had a very good argument (at least back then the argument\n> sounded very good to me) supporting the deliberate difference\n> between the two \"binary\" heuristics.\n\nFirst of all, my patch does not make them being the same, it just\nmakes one being stricter than the other, and I explained why it\nis the tight thing to do.\n\nSecond, it is difficult for me to find to what particular words\nof Linus *you* refer to. However, if it is something like this\npost:\n\nhttp://article.gmane.org/gmane.comp.version-control.git/39618\n\nThen it seems to me, Linus sounded more in favor of that change than\nagainst it. His main argument was against 'diff' heuristic, which he\nfelt was not strict enough for CRLF translation: \"It's *much* better\nto miss some CRLF translation than to do too much of it.\"\n\n\nDmitry\n"},{"id":"65451","messageId":"7vve5ubmdm.fsf@gitster.siamese.dyndns.org","threadId":"11628","inReplyTo":"20080116011321.GD13984@dpotapov.dyndns.org","subject":"Re: [PATCH] treat any file with NUL as binary","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-16T01:16:37Z","receivedAt":"2008-01-16T01:16:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dmitry Potapov <dpotapov@gmail.com> writes:\n\n> Then it seems to me, Linus sounded more in favor of that change than\n> against it. His main argument was against 'diff' heuristic, which he\n> felt was not strict enough for CRLF translation: \"It's *much* better\n> to miss some CRLF translation than to do too much of it.\"\n\nYeah, you found the right one, and I agreed with the argument\nback then, and I agree with it now.  I think NUL heuristics is a\ngood one and obviously it makes it tighter than what diff uses,\nwhich should be what we are aiming for in this thread.\n"},{"id":"65452","messageId":"7vr6gibm56.fsf@gitster.siamese.dyndns.org","threadId":"11628","inReplyTo":"1200407309-10992-1-git-send-email-dpotapov@gmail.com","subject":"Re: [PATCH] treat any file with NUL as binary","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-16T01:21:41Z","receivedAt":"2008-01-16T01:21:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dmitry Potapov <dpotapov@gmail.com> writes:\n\n> There are two heuristics in Git to detect whether a file is binary\n> or text. One in xdiff-interface.c relied on existing NUL byte at\n\n\"relies on\" (not past tense); we may want to say that it is\nstolen from GNU diff to be compatible.\n\n> the beginning. However, convert.c used a different heuristic, which\n> relied that the number of non-printable symbols is less than 1%.\n>\n> Due to difference in approaches whether a file is binary or not,\n> it was possible that a file that diff treats as binary will not be\n> treated as text by CRLF conversation. This is very confusing for\n\n\"conversion\".\n\n> a user who seeing that 'git diff' shows file as binary expects it\n\n\"sees\".\n\n> to be added as binary.\n>\n> This patch makes is_binary to consider any file that contains at\n> least one NUL character as binary.\n> ---\n>\n> So, please, consider it for inclusion as a bug fix.\n\nPlease typofix and apply \"s/.$/, to ensure that the heuristics\nused for CRLF conversion is tighter than what is used by diff./\"\nor something like that at the end.\n\nAlso please add sign-off.  The patch looks correct.\n"},{"id":"65459","messageId":"1200448752-16024-1-git-send-email-dpotapov@gmail.com","threadId":"11628","inReplyTo":"7vr6gibm56.fsf@gitster.siamese.dyndns.org","subject":"[PATCH] treat any file with NUL as binary","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2008-01-16T01:59:12Z","receivedAt":"2008-01-16T01:59:12Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"There are two heuristics in Git to detect whether a file is binary\nor text. One in xdiff-interface.c (which is taken from GNU diff)\nrelies on existence of the NUL byte at the beginning. However,\nconvert.c used a different heuristic, which relied on the percent\nof non-printable symbols (less than 1% for text files).\n\nDue to differences in detection whether a file is binary or not,\nit was possible that a file that diff treats as binary could be\ntreated as text by CRLF conversion. This is very confusing for a\nuser who sees that 'git diff' shows the file as binary expects it\nto be added as binary.\n\nThis patch makes is_binary to consider any file that contains at\nleast one NUL character as binary, to ensure that the heuristics\nused for CRLF conversion is tighter than what is used by diff.\n\nSigned-off-by: Dmitry Potapov <dpotapov@gmail.com>\n---\n convert.c |    9 +++++++--\n 1 files changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex 5adef4f..a51da1f 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -17,8 +17,8 @@\n #define CRLF_INPUT\t2\n \n struct text_stat {\n-\t/* CR, LF and CRLF counts */\n-\tunsigned cr, lf, crlf;\n+\t/* NUL, CR, LF and CRLF counts */\n+\tunsigned nul, cr, lf, crlf;\n \n \t/* These are just approximations! */\n \tunsigned printable, nonprintable;\n@@ -51,6 +51,9 @@ static void gather_stats(const char *buf, unsigned long size, struct text_stat *\n \t\t\tcase '\\b': case '\\t': case '\\033': case '\\014':\n \t\t\t\tstats->printable++;\n \t\t\t\tbreak;\n+\t\t\tcase 0:\n+\t\t\t\tstats->nul++;\n+\t\t\t\t/* fall through */\n \t\t\tdefault:\n \t\t\t\tstats->nonprintable++;\n \t\t\t}\n@@ -66,6 +69,8 @@ static void gather_stats(const char *buf, unsigned long size, struct text_stat *\n static int is_binary(unsigned long size, struct text_stat *stats)\n {\n \n+\tif (stats->nul)\n+\t\treturn 1;\n \tif ((stats->printable >> 7) < stats->nonprintable)\n \t\treturn 1;\n \t/*\n-- \n1.5.3.5\n"}]}