{"thread":{"id":"29718","subject":"[BUG?] bulk checkin does not respect filters","startedAt":"2012-02-24T03:02:45Z","lastAt":"2012-02-24T22:42:02Z","messageCount":19,"participants":["Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"185312","messageId":"20120224030244.GA15742@sigill.intra.peff.net","threadId":"29718","inReplyTo":null,"subject":"[BUG?] bulk checkin does not respect filters","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-24T03:02:45Z","receivedAt":"2012-02-24T03:02:45Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If I do this:\n\n  git init repo &&\n  cd repo &&\n  echo foo >small &&\n  cat small small small small >large &&\n  echo '* filter=foo2bar' >.gitattributes &&\n  git config filter.foo2bar.clean 'sed s/foo/bar/' &&\n  git config core.bigfilethreshold 10 &&\n  git add . &&\n  echo \"===> small\" && git cat-file blob :small\n  echo \"===> large\" && git cat-file blob :large\n\nthe output I get is:\n\n  ===> small\n  bar\n  ===> large\n  foo\n  foo\n  foo\n  foo\n\nI.e., the clean filter is not applied to the bulk checkin file. Nor can\nit be easily, because we need to know the size of the file to write the\nblob header, and we don't know that until we see all of the filter's\noutput.\n\nIn practice, I don't know if this is a huge deal, as people aren't going\nto be asking to de-CRLF files that actually cross the 512M\nbigfilethreshold (OTOH, I seem to recall there are some filters floating\naround for normalizing gzip'd files, which could plausibly be gigantic).\n\nBut it seems like the right choice when we see this conflict is not\n\"don't do filters for streaming checkin\", but rather \"don't do streaming\ncheckin when filters are in use\" (because streaming is an optimization,\nand filters are about correctness).\n\nIt would be even nicer if filters could play well with bulk checkin, but\nI think that would involve streaming to a tempfile, checking the size of\nthe file, and then streaming that into an object. Which is better than\nputting the whole thing in memory if it would involve swapping, but\nprobably worse than doing so if you can possibly fit the whole thing in\n(because you're doing a ton of extra I/O for the tempfile).\n\nThoughts? Was this intentional, or just overlooked?\n\n-Peff\n"},{"id":"185313","messageId":"7vvcmw2a3m.fsf@alter.siamese.dyndns.org","threadId":"29718","inReplyTo":"20120224030244.GA15742@sigill.intra.peff.net","subject":"Re: [BUG?] bulk checkin does not respect filters","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-24T03:17:49Z","receivedAt":"2012-02-24T03:17:49Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Thoughts? Was this intentional, or just overlooked?\n\nThis is intentional in the sense it is not worth worrying about (I\npersonally consider large and binary gunk equivalent and something nobody\nshould care about anything more than 1. what the exact contents it had at\neach point in history and 2. if it did or did not change between two\nversions, but definitely not 3. how different these two versions were),\nand does not deserve the complexity to support filtering and textconv'ing.\n\nIf somebody strongly feels about lifting the limitation with a clean patch\nthat does not harm the common case codepaths, patches are welcome, but I\nam not planning to do it myself ;-)\n\nIt is worth documenting this limitation, though.\n"},{"id":"185314","messageId":"7vr4xk28z0.fsf@alter.siamese.dyndns.org","threadId":"29718","inReplyTo":"7vvcmw2a3m.fsf@alter.siamese.dyndns.org","subject":"Re: [BUG?] bulk checkin does not respect filters","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-24T03:42:11Z","receivedAt":"2012-02-24T03:42:11Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Jeff King <peff@peff.net> writes:\n>\n>> Thoughts? Was this intentional, or just overlooked?\n\nIt is a bit curious that anything filtered even goes to the streaming\ncodepath, given this piece of code in write_entry() in entry.c:\n\n\tif (ce_mode_s_ifmt == S_IFREG) {\n\t\tstruct stream_filter *filter = get_stream_filter(path, ce->sha1);\n\t\tif (filter &&\n\t\t    !streaming_write_entry(ce, path, filter,\n\t\t\t\t\t   state, to_tempfile,\n\t\t\t\t\t   &fstat_done, &st))\n\t\t\tgoto finish;\n\t}\n\nand get_stream_filter() in convert.c has an explicit exception for this\ncase at the very beginning:\n\nstruct stream_filter *get_stream_filter(const char *path, const unsigned char *sha1)\n{\n\tstruct conv_attrs ca;\n\tenum crlf_action crlf_action;\n\tstruct stream_filter *filter = NULL;\n\n\tconvert_attrs(&ca, path);\n\n\tif (ca.drv && (ca.drv->smudge || ca.drv->clean))\n\t\treturn filter;\n\nto make sure that it says \"No streaming filtering is possible, do not even\nattempt to call streaming_write_entry()\".\n"},{"id":"185316","messageId":"20120224075425.GA18688@sigill.intra.peff.net","threadId":"29718","inReplyTo":"7vr4xk28z0.fsf@alter.siamese.dyndns.org","subject":"Re: [BUG?] bulk checkin does not respect filters","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-24T07:54:25Z","receivedAt":"2012-02-24T07:54:25Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 23, 2012 at 07:42:11PM -0800, Junio C Hamano wrote:\n\n> It is a bit curious that anything filtered even goes to the streaming\n> codepath, given this piece of code in write_entry() in entry.c:\n> \n> \tif (ce_mode_s_ifmt == S_IFREG) {\n> \t\tstruct stream_filter *filter = get_stream_filter(path, ce->sha1);\n> \t\tif (filter &&\n> \t\t    !streaming_write_entry(ce, path, filter,\n> \t\t\t\t\t   state, to_tempfile,\n> \t\t\t\t\t   &fstat_done, &st))\n> \t\t\tgoto finish;\n> \t}\n> \n> and get_stream_filter() in convert.c has an explicit exception for this\n> case at the very beginning:\n\nI think it is because we don't follow that code path at all. The stack trace\nfor \"git add\" looks on a large file looks like:\n\n#0  stream_to_pack (...) at bulk-checkin.c:101\n#1  deflate_to_pack (...) at bulk-checkin.c:219\n#2  index_bulk_checkin (...) at bulk-checkin.c:258\n#3  index_stream (...) at sha1_file.c:2712\n#4  index_fd (...) at sha1_file.c:2726\n#5  index_path (...) at sha1_file.c:2742\n#6  add_to_index (...) at read-cache.c:644\n#7  add_file_to_index (...) at read-cache.c:673\n#8  add_files (...) at builtin/add.c:363\n#9  cmd_add (...) at builtin/add.c:474\n#10 run_builtin (...) at git.c:324\n#11 handle_internal_command (...) at git.c:484\n#12 run_argv (...) at git.c:530\n#13 main (...) at git.c:605 \n\nIsn't write_entry the _other_ side of things. I.e., checking out, not\nchecking in? That side works fine (making the large file handling\nfor git-add even more odd. We will fail to apply the \"clean\" filter when\nadding it, but we will apply the smudge filter when we write it out).\n\n-Peff\n"},{"id":"185317","messageId":"20120224082803.GB18688@sigill.intra.peff.net","threadId":"29718","inReplyTo":"7vvcmw2a3m.fsf@alter.siamese.dyndns.org","subject":"Re: [BUG?] bulk checkin does not respect filters","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-24T08:28:03Z","receivedAt":"2012-02-24T08:28:03Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 23, 2012 at 07:17:49PM -0800, Junio C Hamano wrote:\n\n> > Thoughts? Was this intentional, or just overlooked?\n> \n> This is intentional in the sense it is not worth worrying about (I\n> personally consider large and binary gunk equivalent and something nobody\n> should care about anything more than 1. what the exact contents it had at\n> each point in history and 2. if it did or did not change between two\n> versions, but definitely not 3. how different these two versions were),\n> and does not deserve the complexity to support filtering and textconv'ing.\n\nWe're purely in the hypothetical devil's advocate realm here, as I do\nnot use the filtering feature myself. But what if I had a filter that\ncanonicalized large binary files in some way (e.g., by re-zipping large\nzipfiles using deterministic parameters). Even though they are large\nbinary gunk and you would never want to diff two versions, the important\nthing is getting an identical sha1 when the underlying data has not\nchanged. I.e., your point 2 above, but applied to the \"clean\" repository\nversions of files.\n\n> If somebody strongly feels about lifting the limitation with a clean patch\n> that does not harm the common case codepaths, patches are welcome, but I\n> am not planning to do it myself ;-)\n\nPerhaps something like this:\n\n---\n convert.c   |   18 ++++++++++++++++++\n convert.h   |    1 +\n sha1_file.c |    3 ++-\n 3 files changed, 21 insertions(+), 1 deletion(-)\n\ndiff --git a/convert.c b/convert.c\nindex 12868ed..254301a 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -742,6 +742,24 @@ static void convert_attrs(struct conv_attrs *ca, const char *path)\n \t}\n }\n \n+int want_convert_to_git(const char *path)\n+{\n+\tstruct conv_attrs ca;\n+\tconvert_attrs(&ca, path);\n+\n+\tif (ca.drv && ca.drv->clean)\n+\t\treturn 1;\n+\tif (ca.ident)\n+\t\treturn 1;\n+\n+\tca.crlf_action = input_crlf_action(ca.crlf_action, ca.eol_attr);\n+\tif (ca.crlf_action == CRLF_BINARY)\n+\t\treturn 0;\n+\tif (ca.crlf_action == CRLF_GUESS && auto_crlf == AUTO_CRLF_FALSE)\n+\t\treturn 0;\n+\treturn 1;\n+}\n+\n int convert_to_git(const char *path, const char *src, size_t len,\n                    struct strbuf *dst, enum safe_crlf checksafe)\n {\ndiff --git a/convert.h b/convert.h\nindex d799a16..4d5936d 100644\n--- a/convert.h\n+++ b/convert.h\n@@ -36,6 +36,7 @@ extern enum eol core_eol;\n /* returns 1 if *dst was used */\n extern int convert_to_git(const char *path, const char *src, size_t len,\n \t\t\t  struct strbuf *dst, enum safe_crlf checksafe);\n+extern int want_convert_to_git(const char *path);\n extern int convert_to_working_tree(const char *path, const char *src,\n \t\t\t\t   size_t len, struct strbuf *dst);\n extern int renormalize_buffer(const char *path, const char *src, size_t len,\ndiff --git a/sha1_file.c b/sha1_file.c\nindex f9f8d5e..6c0e05c 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2720,7 +2720,8 @@ int index_fd(unsigned char *sha1, int fd, struct stat *st,\n \n \tif (!S_ISREG(st->st_mode))\n \t\tret = index_pipe(sha1, fd, type, path, flags);\n-\telse if (size <= big_file_threshold || type != OBJ_BLOB)\n+\telse if (size <= big_file_threshold || type != OBJ_BLOB ||\n+\t\t want_convert_to_git(path))\n \t\tret = index_core(sha1, fd, size, type, path, flags);\n \telse\n \t\tret = index_stream(sha1, fd, size, type, path, flags);\n\nThere should be no performance impact, as the new code only kicks in for\nfiles that exceed big_file_threshold (and even then, it is just an extra\nduplicate attr lookup, which the check_attr code caches anyway).\n\nI don't like repeating all of the convert_to_git policy logic. Perhaps\nif you pass a NULL buffer to convert_to_git, it should run through its\nusual logic, stopping just short of actually writing anything, and\nreturn a flag indicating whether it _would_ convert (this can't be 100%\naccurate, as sometimes conversion depends on looking at the actual\ncontents of the buffer, but it could at least tell us \"yes, I might\nconvert\" versus \"no, I will definitely not convert\").\n\n-Peff\n"},{"id":"185319","messageId":"20120224093924.GA11680@sigill.intra.peff.net","threadId":"29718","inReplyTo":"20120224082803.GB18688@sigill.intra.peff.net","subject":"Re: [BUG?] bulk checkin does not respect filters","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-24T09:39:24Z","receivedAt":"2012-02-24T09:39:24Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 24, 2012 at 03:28:03AM -0500, Jeff King wrote:\n\n> I don't like repeating all of the convert_to_git policy logic. Perhaps\n> if you pass a NULL buffer to convert_to_git, it should run through its\n> usual logic, stopping just short of actually writing anything, and\n> return a flag indicating whether it _would_ convert...\n\nSo here's a series that does that.\n\n  [1/2]: teach convert_to_git a \"dry run\" mode\n  [2/2]: do not stream large files to pack when filters are in use\n\n-Peff\n"},{"id":"185320","messageId":"20120224094151.GA11846@sigill.intra.peff.net","threadId":"29718","inReplyTo":"20120224093924.GA11680@sigill.intra.peff.net","subject":"[PATCH 1/2] teach convert_to_git a \"dry run\" mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-24T09:41:55Z","receivedAt":"2012-02-24T09:41:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Some callers may want to know whether convert_to_git will do\nany conversion without actually feeding file data to it\n(e.g., if you are using the decision to decide how to\nacquire the data that might be converted). Rather than\nreplicate convert_to_git's logic in a separate function,\nthis patch lets callers pass a NULL buffer. The function\nruns through its usual logic, except that it does not do any\nactual conversion; the return value, instead of specifying\nwhether conversion happened, indicates whether conversion\nmight occur.\n\nNote the use of the word \"might\" above. We cannot know for\nsure whether conversion will occur without seeing the buffer\nitself (because CRLF conversion may decide the file is\nbinary and do nothing). However, \"might\" is close enough for\ncallers which are trying to find out whether filters are in\nused; they can be conservative and assume that the filters\nwill be used in such a case.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n convert.c |   18 ++++++++++++++----\n 1 file changed, 14 insertions(+), 4 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex 12868ed..33aa72f 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -195,9 +195,13 @@ static int crlf_to_git(const char *path, const char *src, size_t len,\n \tchar *dst;\n \n \tif (crlf_action == CRLF_BINARY ||\n-\t    (crlf_action == CRLF_GUESS && auto_crlf == AUTO_CRLF_FALSE) || !len)\n+\t    (crlf_action == CRLF_GUESS && auto_crlf == AUTO_CRLF_FALSE) ||\n+\t    (src && !len))\n \t\treturn 0;\n \n+\tif (!src)\n+\t\treturn 1;\n+\n \tgather_stats(src, len, &stats);\n \n \tif (crlf_action == CRLF_AUTO || crlf_action == CRLF_GUESS) {\n@@ -391,6 +395,9 @@ static int apply_filter(const char *path, const char *src, size_t len,\n \tif (!cmd)\n \t\treturn 0;\n \n+\tif (!src)\n+\t\treturn 1;\n+\n \tmemset(&async, 0, sizeof(async));\n \tasync.proc = filter_buffer;\n \tasync.data = &params;\n@@ -522,9 +529,12 @@ static int ident_to_git(const char *path, const char *src, size_t len,\n {\n \tchar *dst, *dollar;\n \n-\tif (!ident || !count_ident(src, len))\n+\tif (!ident || (src && !count_ident(src, len)))\n \t\treturn 0;\n \n+\tif (!src)\n+\t\treturn 1;\n+\n \t/* only grow if not in place */\n \tif (strbuf_avail(buf) + buf->len < len)\n \t\tstrbuf_grow(buf, len - buf->len);\n@@ -754,13 +764,13 @@ int convert_to_git(const char *path, const char *src, size_t len,\n \t\tfilter = ca.drv->clean;\n \n \tret |= apply_filter(path, src, len, dst, filter);\n-\tif (ret) {\n+\tif (ret && src) {\n \t\tsrc = dst->buf;\n \t\tlen = dst->len;\n \t}\n \tca.crlf_action = input_crlf_action(ca.crlf_action, ca.eol_attr);\n \tret |= crlf_to_git(path, src, len, dst, ca.crlf_action, checksafe);\n-\tif (ret) {\n+\tif (ret && src) {\n \t\tsrc = dst->buf;\n \t\tlen = dst->len;\n \t}\n-- \n1.7.9.9.g04d94\n"},{"id":"185321","messageId":"20120224094805.GB11846@sigill.intra.peff.net","threadId":"29718","inReplyTo":"20120224093924.GA11680@sigill.intra.peff.net","subject":"[PATCH 2/2] do not stream large files to pack when filters are in use","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-24T09:48:05Z","receivedAt":"2012-02-24T09:48:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Because git's object format requires us to specify the\nnumber of bytes in the object in its header, we must know\nthe size before streaming a blob into the object database.\nThis is not a problem when adding a regular file, as we can\nget the size from stat(). However, when filters are in use\n(such as autocrlf, or the ident, filter, or eol\ngitattributes), we have no idea what the ultimate size will\nbe.\n\nThe current code just punts on the whole issue and ignores\nfilter configuration entirely for files larger than\ncore.bigfilethreshold. This can generate confusing results\nif you use filters for large binary files, as the filter\nwill suddenly stop working as the file goes over a certain\nsize.  Rather than try to handle unknown input sizes with\nstreaming, this patch just turns off the streaming\noptimization when filters are in use.\n\nThis has a slight performance regression in a very specific\ncase: if you have autocrlf on, but no gitattributes, a large\nbinary file will avoid the streaming code path because we\ndon't know beforehand whether it will need conversion or\nnot. But if you are handling large binary files, you should\nbe marking them as such via attributes (or at least not\nusing autocrlf, and instead marking your text files as\nsuch). And the flip side is that if you have a large\n_non_-binary file, there is a correctness improvement;\nbefore we did not apply the conversion at all.\n\nThe first half of the new t1051 script covers these failures\non input. The second half tests the matching output code\npaths. These already work correctly, and do not need any\nadjustment.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI'm on the fence about the performance regression above. On the one\nhand, if you're telling git to autocrlf your gigantic binary files, you\nshould fix your attributes, and expect not to use the streaming\noptimization until you do. On the other hand, while I can see a remote\npossibility of using filter.*.clean on a large binary file, the\nprobability that autocrlf would apply seems quite absurdly unlikely.\n\nSo rather than be pessimistic that filters will be used in the \"auto\"\ncase, perhaps we should be optimistic that they would not be. Or another\nway of thinking about it is: from the CRLF code's perspective, anything\nlarger than core.bigfilethreshold should be considered binary without\nactually looking at it, and we can say for sure (not the \"might\" from\nthe last patch) that such large things would not be converted in the\nCRLF_GUESS case.\n\n sha1_file.c                 |    3 +-\n t/t1051-large-conversion.sh |   86 +++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 88 insertions(+), 1 deletion(-)\n create mode 100755 t/t1051-large-conversion.sh\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex f9f8d5e..61f597b 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2720,7 +2720,8 @@ int index_fd(unsigned char *sha1, int fd, struct stat *st,\n \n \tif (!S_ISREG(st->st_mode))\n \t\tret = index_pipe(sha1, fd, type, path, flags);\n-\telse if (size <= big_file_threshold || type != OBJ_BLOB)\n+\telse if (size <= big_file_threshold || type != OBJ_BLOB ||\n+\t\t convert_to_git(path, NULL, 0, NULL, 0))\n \t\tret = index_core(sha1, fd, size, type, path, flags);\n \telse\n \t\tret = index_stream(sha1, fd, size, type, path, flags);\ndiff --git a/t/t1051-large-conversion.sh b/t/t1051-large-conversion.sh\nnew file mode 100755\nindex 0000000..8b7640b\n--- /dev/null\n+++ b/t/t1051-large-conversion.sh\n@@ -0,0 +1,86 @@\n+#!/bin/sh\n+\n+test_description='test conversion filters on large files'\n+. ./test-lib.sh\n+\n+set_attr() {\n+\ttest_when_finished 'rm -f .gitattributes' &&\n+\techo \"* $*\" >.gitattributes\n+}\n+\n+check_input() {\n+\tgit read-tree --empty &&\n+\tgit add small large &&\n+\tgit cat-file blob :small >small.index &&\n+\tgit cat-file blob :large | head -n 1 >large.index &&\n+\ttest_cmp small.index large.index\n+}\n+\n+check_output() {\n+\trm -f small large &&\n+\tgit checkout small large &&\n+\thead -n 1 large >large.head &&\n+\ttest_cmp small large.head\n+}\n+\n+test_expect_success 'setup input tests' '\n+\tprintf \"\\$Id: foo\\$\\\\r\\\\n\" >small &&\n+\tcat small small >large &&\n+\tgit config core.bigfilethreshold 20 &&\n+\tgit config filter.test.clean \"sed s/.*/CLEAN/\"\n+'\n+\n+test_expect_success 'autocrlf=true converts on input' '\n+\ttest_config core.autocrlf true &&\n+\tcheck_input\n+'\n+\n+test_expect_success 'eol=crlf converts on input' '\n+\tset_attr eol=crlf &&\n+\tcheck_input\n+'\n+\n+test_expect_success 'ident converts on input' '\n+\tset_attr ident &&\n+\tcheck_input\n+'\n+\n+test_expect_success 'user-defined filters convert on input' '\n+\tset_attr filter=test &&\n+\tcheck_input\n+'\n+\n+test_expect_success 'setup output tests' '\n+\techo \"\\$Id\\$\" >small &&\n+\tcat small small >large &&\n+\tgit add small large &&\n+\tgit config core.bigfilethreshold 7 &&\n+\tgit config filter.test.smudge \"sed s/.*/SMUDGE/\"\n+'\n+\n+test_expect_success 'autocrlf=true converts on output' '\n+\ttest_config core.autocrlf true &&\n+\tcheck_output\n+'\n+\n+test_expect_success 'eol=crlf converts on output' '\n+\tset_attr eol=crlf &&\n+\tcheck_output\n+'\n+\n+test_expect_success 'user-defined filters convert on output' '\n+\tset_attr filter=test &&\n+\tcheck_output\n+'\n+\n+test_expect_success 'ident converts on output' '\n+\tset_attr ident &&\n+\trm -f small large &&\n+\tgit checkout small large &&\n+\tsed -n \"s/Id: .*/Id: SHA/p\" <small >small.clean &&\n+\thead -n 1 large >large.head &&\n+\tsed -n \"s/Id: .*/Id: SHA/p\" <large.head >large.clean &&\n+\ttest_cmp small.clean large.clean\n+'\n+\n+test_done\n-- \n1.7.9.9.g04d94\n"},{"id":"185342","messageId":"7vd394130q.fsf@alter.siamese.dyndns.org","threadId":"29718","inReplyTo":"20120224075425.GA18688@sigill.intra.peff.net","subject":"Re: [BUG?] bulk checkin does not respect filters","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-24T18:48:21Z","receivedAt":"2012-02-24T18:48:21Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Isn't write_entry the _other_ side of things. I.e., checking out, not\n> checking in?\n\nAh, of course.\n"},{"id":"185354","messageId":"7vehtkyp6i.fsf@alter.siamese.dyndns.org","threadId":"29718","inReplyTo":"20120224094805.GB11846@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] do not stream large files to pack when filters are in use","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-24T20:03:17Z","receivedAt":"2012-02-24T20:03:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I'm on the fence about the performance regression above. On the one\n> hand, if you're telling git to autocrlf your gigantic binary files, you\n> should fix your attributes, and expect not to use the streaming\n> optimization until you do.\n\nThat is a sensible angle to view the issue from, I would think.\n\n> diff --git a/sha1_file.c b/sha1_file.c\n> index f9f8d5e..61f597b 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -2720,7 +2720,8 @@ int index_fd(unsigned char *sha1, int fd, struct stat *st,\n>  \n>  \tif (!S_ISREG(st->st_mode))\n>  \t\tret = index_pipe(sha1, fd, type, path, flags);\n> -\telse if (size <= big_file_threshold || type != OBJ_BLOB)\n> +\telse if (size <= big_file_threshold || type != OBJ_BLOB ||\n> +\t\t convert_to_git(path, NULL, 0, NULL, 0))\n\nNice.  It would be even nicer to give a readability macro whose name makes\nit clear that this is a query (unfortunately we cannot add '?' at the end\nof the function name) and not a conversion.  Any name suggestions?\n\nBy the way, I tried this with the tip of 'pu' as of last night, and the\nresult segfaults on me in t1050 (hash-object -w) ;-).\n"},{"id":"185355","messageId":"7v7gzcyp6b.fsf@alter.siamese.dyndns.org","threadId":"29718","inReplyTo":"20120224093924.GA11680@sigill.intra.peff.net","subject":"Re: [BUG?] bulk checkin does not respect filters","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-24T20:03:24Z","receivedAt":"2012-02-24T20:03:24Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Fri, Feb 24, 2012 at 03:28:03AM -0500, Jeff King wrote:\n>\n>> I don't like repeating all of the convert_to_git policy logic. Perhaps\n>> if you pass a NULL buffer to convert_to_git, it should run through its\n>> usual logic, stopping just short of actually writing anything, and\n>> return a flag indicating whether it _would_ convert...\n\nMakes very good sense.  Thanks.\n"},{"id":"185371","messageId":"20120224204810.GC21447@sigill.intra.peff.net","threadId":"29718","inReplyTo":"7vehtkyp6i.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] do not stream large files to pack when filters are in use","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-24T20:48:10Z","receivedAt":"2012-02-24T20:48:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 24, 2012 at 12:03:17PM -0800, Junio C Hamano wrote:\n\n> > -\telse if (size <= big_file_threshold || type != OBJ_BLOB)\n> > +\telse if (size <= big_file_threshold || type != OBJ_BLOB ||\n> > +\t\t convert_to_git(path, NULL, 0, NULL, 0))\n> \n> Nice.  It would be even nicer to give a readability macro whose name makes\n> it clear that this is a query (unfortunately we cannot add '?' at the end\n> of the function name) and not a conversion.  Any name suggestions?\n\nYeah, I cringed a little at all of the NULLs. How about:\n\n  int would_convert_to_git(const char *path);\n\n?\n\n> By the way, I tried this with the tip of 'pu' as of last night, and the\n> result segfaults on me in t1050 (hash-object -w) ;-).\n\nEek. I ran through \"make test\" on my finished series without a problem,\nbut it may be a bad interaction with something in \"pu\", or maybe a\nheisenbug. I'll investigate.\n\n-Peff\n"},{"id":"185376","messageId":"20120224210134.GA22177@sigill.intra.peff.net","threadId":"29718","inReplyTo":"20120224204810.GC21447@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] do not stream large files to pack when filters are in use","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-24T21:01:34Z","receivedAt":"2012-02-24T21:01:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 24, 2012 at 03:48:10PM -0500, Jeff King wrote:\n\n> On Fri, Feb 24, 2012 at 12:03:17PM -0800, Junio C Hamano wrote:\n> \n> > > -\telse if (size <= big_file_threshold || type != OBJ_BLOB)\n> > > +\telse if (size <= big_file_threshold || type != OBJ_BLOB ||\n> > > +\t\t convert_to_git(path, NULL, 0, NULL, 0))\n> > \n> > Nice.  It would be even nicer to give a readability macro whose name makes\n> > it clear that this is a query (unfortunately we cannot add '?' at the end\n> > of the function name) and not a conversion.  Any name suggestions?\n> \n> Yeah, I cringed a little at all of the NULLs. How about:\n> \n>   int would_convert_to_git(const char *path);\n> \n> ?\n\nI started to do this but had a thought: would other callers want more\nflexibility? That is, should convert_to_git's dry run trigger actually\nbe: the _destination_ buffer is NULL?\n\nI.e., you could call it like this:\n\n  convert_to_git(path, buf, len, NULL, 0);\n\nand find out the real answer, including inspecting buf, about whether\nwe would convert. Or you could also use a NULL buffer to get the\npessimistic \"we might convert\" case.\n\nI don't think it really matters that much, as I am introducing only one\ncaller. I'm not sure if any other code paths would care about this\nspeculative \"maybe we would convert\" question, so perhaps it is simply\nover-engineering.\n\n-Peff\n"},{"id":"185380","messageId":"20120224211913.GA30942@sigill.intra.peff.net","threadId":"29718","inReplyTo":"20120224204810.GC21447@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] do not stream large files to pack when filters are in use","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-24T21:19:13Z","receivedAt":"2012-02-24T21:19:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 24, 2012 at 03:48:10PM -0500, Jeff King wrote:\n\n> > By the way, I tried this with the tip of 'pu' as of last night, and the\n> > result segfaults on me in t1050 (hash-object -w) ;-).\n> \n> Eek. I ran through \"make test\" on my finished series without a problem,\n> but it may be a bad interaction with something in \"pu\", or maybe a\n> heisenbug. I'll investigate.\n\nOr maybe I'm just an idiot, and apparently didn't actually run the test\nsuite correctly. Because it is very reproducible, and is because\nhash-object --stdin passes a NULL for the path.\n\nI'll post the fixed series in a minute (with this fix, and the improved\nconvert_to_git wrapper).\n\n-Peff\n"},{"id":"185381","messageId":"7vfwe0x726.fsf@alter.siamese.dyndns.org","threadId":"29718","inReplyTo":"20120224210134.GA22177@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] do not stream large files to pack when filters are in use","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-24T21:20:01Z","receivedAt":"2012-02-24T21:20:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Fri, Feb 24, 2012 at 03:48:10PM -0500, Jeff King wrote:\n>\n>> On Fri, Feb 24, 2012 at 12:03:17PM -0800, Junio C Hamano wrote:\n>> \n>> > > -\telse if (size <= big_file_threshold || type != OBJ_BLOB)\n>> > > +\telse if (size <= big_file_threshold || type != OBJ_BLOB ||\n>> > > +\t\t convert_to_git(path, NULL, 0, NULL, 0))\n>> > \n>> > Nice.  It would be even nicer to give a readability macro whose name makes\n>> > it clear that this is a query (unfortunately we cannot add '?' at the end\n>> > of the function name) and not a conversion.  Any name suggestions?\n>> ...\n> I.e., you could call it like this:\n>\n>   convert_to_git(path, buf, len, NULL, 0);\n>\n> and find out the real answer, including inspecting buf, about whether\n> we would convert. Or you could also use a NULL buffer to get the\n> pessimistic \"we might convert\" case.\n>\n> I don't think it really matters that much, as I am introducing only one\n> caller. I'm not sure if any other code paths would care about this\n> speculative \"maybe we would convert\" question, so perhaps it is simply\n> over-engineering.\n\nI agree checking if dst is NULL makes a lot more sense than checking if\nthe src is NULL.\n\nThe reason I said \"readability\" macro was *NOT* because of these NULLs\nlooked unsightly.  I wanted the function name to tell \"We are *NOT*\nactually converting anything here at this point\" to the readers.\n\nI am perfectly fine to keep the source side of the parameters in the\nreadability macro, even if this partcular caller may have to spell NULL\nand 0 there.\n\nThanks.\n"},{"id":"185389","messageId":"20120224220237.GA1809@sigill.intra.peff.net","threadId":"29718","inReplyTo":"20120224211913.GA30942@sigill.intra.peff.net","subject":"[PATCHv2 1/3] teach convert_to_git a \"dry run\" mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-24T22:02:37Z","receivedAt":"2012-02-24T22:02:37Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Some callers may want to know whether convert_to_git will\nactually do anything before performing the conversion\nitself (e.g., to decide whether to stream or handle blobs\nin-core). This patch lets callers specify the dry run mode\nby passing a NULL destination buffer. The return value,\ninstead of indicating whether conversion happened, will\nindicate whether conversion would occur.\n\nFor readability, we also include a wrapper function which\nmakes it more obvious we are not actually performing the\nconversion.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis splits the 1/2 from the first series into two patches. This part\nhandles the dry-run aspect. Unlike the previous version, it uses a NULL\ndestination, rather than a NULL source to indicate the dry-run mode.\nWhich just makes more sense, and means that the \"don't bother doing\nsource analysis\" bit is split off.\n\nNo callers introduced in this series actually want to do a dry-run with\na non-NULL source buffer, so that feature is of dubious value. But it's\nnot any extra code to do it this way, and the resulting commits are much\neasier to read, I think.\n\n convert.c |   17 +++++++++++++++--\n convert.h |    5 +++++\n 2 files changed, 20 insertions(+), 2 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex 12868ed..65fa9d5 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -231,6 +231,13 @@ static int crlf_to_git(const char *path, const char *src, size_t len,\n \tif (!stats.cr)\n \t\treturn 0;\n \n+\t/*\n+\t * At this point all of our source analysis is done, and we are sure we\n+\t * would convert. If we are in dry-run mode, we can give an answer.\n+\t */\n+\tif (!buf)\n+\t\treturn 1;\n+\n \t/* only grow if not in place */\n \tif (strbuf_avail(buf) + buf->len < len)\n \t\tstrbuf_grow(buf, len - buf->len);\n@@ -391,6 +398,9 @@ static int apply_filter(const char *path, const char *src, size_t len,\n \tif (!cmd)\n \t\treturn 0;\n \n+\tif (!dst)\n+\t\treturn 1;\n+\n \tmemset(&async, 0, sizeof(async));\n \tasync.proc = filter_buffer;\n \tasync.data = &params;\n@@ -525,6 +535,9 @@ static int ident_to_git(const char *path, const char *src, size_t len,\n \tif (!ident || !count_ident(src, len))\n \t\treturn 0;\n \n+\tif (!buf)\n+\t\treturn 1;\n+\n \t/* only grow if not in place */\n \tif (strbuf_avail(buf) + buf->len < len)\n \t\tstrbuf_grow(buf, len - buf->len);\n@@ -754,13 +767,13 @@ int convert_to_git(const char *path, const char *src, size_t len,\n \t\tfilter = ca.drv->clean;\n \n \tret |= apply_filter(path, src, len, dst, filter);\n-\tif (ret) {\n+\tif (ret && dst) {\n \t\tsrc = dst->buf;\n \t\tlen = dst->len;\n \t}\n \tca.crlf_action = input_crlf_action(ca.crlf_action, ca.eol_attr);\n \tret |= crlf_to_git(path, src, len, dst, ca.crlf_action, checksafe);\n-\tif (ret) {\n+\tif (ret && dst) {\n \t\tsrc = dst->buf;\n \t\tlen = dst->len;\n \t}\ndiff --git a/convert.h b/convert.h\nindex d799a16..ec5fd69 100644\n--- a/convert.h\n+++ b/convert.h\n@@ -40,6 +40,11 @@ extern int convert_to_working_tree(const char *path, const char *src,\n \t\t\t\t   size_t len, struct strbuf *dst);\n extern int renormalize_buffer(const char *path, const char *src, size_t len,\n \t\t\t      struct strbuf *dst);\n+static inline int would_convert_to_git(const char *path, const char *src,\n+\t\t\t\t       size_t len, enum safe_crlf checksafe)\n+{\n+\treturn convert_to_git(path, src, len, NULL, checksafe);\n+}\n \n /*****************************************************************\n  *\n-- \n1.7.9.11.gca600\n"},{"id":"185390","messageId":"20120224220503.GB1809@sigill.intra.peff.net","threadId":"29718","inReplyTo":"20120224211913.GA30942@sigill.intra.peff.net","subject":"[PATCHv2 2/3] teach dry-run convert_to_git not to require a src buffer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-24T22:05:03Z","receivedAt":"2012-02-24T22:05:03Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we call convert_to_git in dry-run mode, it may still\nwant to look at the source buffer, because some CRLF\nconversion modes depend on analyzing the source to determine\nwhether it is in fact convertible CRLF text.\n\nHowever, the main motivation for convert_to_git's dry-run\nmode is that we would decide which method to use to acquire\nthe blob's data (streaming versus in-core). Requiring this\nsource analysis creates a chicken-and-egg problem. We are\nbetter off simply guessing that anything we can't analyze\nwill end up needing conversion.\n\nThis patch lets a caller specify a NULL src buffer when\nusing dry-run mode (and only dry-run mode). A non-zero\nreturn value goes from \"we would convert\" to \"we might\nconvert\"; a zero return value remains \"we would definitely\nnot convert\".\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nAnd this is the second half of v1's patch 1/2.\n\nBy splitting this off, I think it makes it much more obvious if we want\nto re-visit the pessimism later (e.g., to optimistically assume that\nmissing src buffers should be assumed to be binary if they are large).\n\n convert.c |   12 ++++++++++--\n 1 file changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex 65fa9d5..aa7f72d 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -195,9 +195,17 @@ static int crlf_to_git(const char *path, const char *src, size_t len,\n \tchar *dst;\n \n \tif (crlf_action == CRLF_BINARY ||\n-\t    (crlf_action == CRLF_GUESS && auto_crlf == AUTO_CRLF_FALSE) || !len)\n+\t    (crlf_action == CRLF_GUESS && auto_crlf == AUTO_CRLF_FALSE) ||\n+\t    (src && !len))\n \t\treturn 0;\n \n+\t/*\n+\t * If we are doing a dry-run and have no source buffer, there is\n+\t * nothing to analyze; we must assume we would convert.\n+\t */\n+\tif (!buf && !src)\n+\t\treturn 1;\n+\n \tgather_stats(src, len, &stats);\n \n \tif (crlf_action == CRLF_AUTO || crlf_action == CRLF_GUESS) {\n@@ -532,7 +540,7 @@ static int ident_to_git(const char *path, const char *src, size_t len,\n {\n \tchar *dst, *dollar;\n \n-\tif (!ident || !count_ident(src, len))\n+\tif (!ident || (src && !count_ident(src, len)))\n \t\treturn 0;\n \n \tif (!buf)\n-- \n1.7.9.11.gca600\n"},{"id":"185392","messageId":"20120224221017.GC1809@sigill.intra.peff.net","threadId":"29718","inReplyTo":"20120224211913.GA30942@sigill.intra.peff.net","subject":"[PATCHv2 3/3] do not stream large files to pack when filters are in use","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-24T22:10:17Z","receivedAt":"2012-02-24T22:10:17Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Because git's object format requires us to specify the\nnumber of bytes in the object in its header, we must know\nthe size before streaming a blob into the object database.\nThis is not a problem when adding a regular file, as we can\nget the size from stat(). However, when filters are in use\n(such as autocrlf, or the ident, filter, or eol\ngitattributes), we have no idea what the ultimate size will\nbe.\n\nThe current code just punts on the whole issue and ignores\nfilter configuration entirely for files larger than\ncore.bigfilethreshold. This can generate confusing results\nif you use filters for large binary files, as the filter\nwill suddenly stop working as the file goes over a certain\nsize.  Rather than try to handle unknown input sizes with\nstreaming, this patch just turns off the streaming\noptimization when filters are in use.\n\nThis has a slight performance regression in a very specific\ncase: if you have autocrlf on, but no gitattributes, a large\nbinary file will avoid the streaming code path because we\ndon't know beforehand whether it will need conversion or\nnot. But if you are handling large binary files, you should\nbe marking them as such via attributes (or at least not\nusing autocrlf, and instead marking your text files as\nsuch). And the flip side is that if you have a large\n_non_-binary file, there is a correctness improvement;\nbefore we did not apply the conversion at all.\n\nThe first half of the new t1051 script covers these failures\non input. The second half tests the matching output code\npaths. These already work correctly, and do not need any\nadjustment.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis patches patch 2/2 from v1 of the series. The changes are:\n\n  - use the new would_convert_to_git macro\n\n  - only check filters if we have a path, which handles \"hash-object\n    --stdin\". It actually seems to me that one could have autocrlf on,\n    and would expect hash-object to do automatic conversion even without\n    a path with which to look up attributes. But this is the same test\n    that index_mem uses, so we are matching the behavior there.\n\n  - update the comment above index_stream. I have no idea how I\n    previously missed this giant comment explaining the exact situation\n    I was testing.\n\n sha1_file.c                 |   14 ++++---\n t/t1051-large-conversion.sh |   86 +++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 95 insertions(+), 5 deletions(-)\n create mode 100755 t/t1051-large-conversion.sh\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex f9f8d5e..4f06a0e 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2700,10 +2700,13 @@ static int index_core(unsigned char *sha1, int fd, size_t size,\n  * This also bypasses the usual \"convert-to-git\" dance, and that is on\n  * purpose. We could write a streaming version of the converting\n  * functions and insert that before feeding the data to fast-import\n- * (or equivalent in-core API described above), but the primary\n- * motivation for trying to stream from the working tree file and to\n- * avoid mmaping it in core is to deal with large binary blobs, and\n- * by definition they do _not_ want to get any conversion.\n+ * (or equivalent in-core API described above). However, that is\n+ * somewhat complicated, as we do not know the size of the filter\n+ * result, which we need to know beforehand when writing a git object.\n+ * Since the primary motivation for trying to stream from the working\n+ * tree file and to avoid mmaping it in core is to deal with large\n+ * binary blobs, they generally do not want to get any conversion, and\n+ * callers should avoid this code path when filters are requested.\n  */\n static int index_stream(unsigned char *sha1, int fd, size_t size,\n \t\t\tenum object_type type, const char *path,\n@@ -2720,7 +2723,8 @@ int index_fd(unsigned char *sha1, int fd, struct stat *st,\n \n \tif (!S_ISREG(st->st_mode))\n \t\tret = index_pipe(sha1, fd, type, path, flags);\n-\telse if (size <= big_file_threshold || type != OBJ_BLOB)\n+\telse if (size <= big_file_threshold || type != OBJ_BLOB ||\n+\t\t (path && would_convert_to_git(path, NULL, 0, 0)))\n \t\tret = index_core(sha1, fd, size, type, path, flags);\n \telse\n \t\tret = index_stream(sha1, fd, size, type, path, flags);\ndiff --git a/t/t1051-large-conversion.sh b/t/t1051-large-conversion.sh\nnew file mode 100755\nindex 0000000..8b7640b\n--- /dev/null\n+++ b/t/t1051-large-conversion.sh\n@@ -0,0 +1,86 @@\n+#!/bin/sh\n+\n+test_description='test conversion filters on large files'\n+. ./test-lib.sh\n+\n+set_attr() {\n+\ttest_when_finished 'rm -f .gitattributes' &&\n+\techo \"* $*\" >.gitattributes\n+}\n+\n+check_input() {\n+\tgit read-tree --empty &&\n+\tgit add small large &&\n+\tgit cat-file blob :small >small.index &&\n+\tgit cat-file blob :large | head -n 1 >large.index &&\n+\ttest_cmp small.index large.index\n+}\n+\n+check_output() {\n+\trm -f small large &&\n+\tgit checkout small large &&\n+\thead -n 1 large >large.head &&\n+\ttest_cmp small large.head\n+}\n+\n+test_expect_success 'setup input tests' '\n+\tprintf \"\\$Id: foo\\$\\\\r\\\\n\" >small &&\n+\tcat small small >large &&\n+\tgit config core.bigfilethreshold 20 &&\n+\tgit config filter.test.clean \"sed s/.*/CLEAN/\"\n+'\n+\n+test_expect_success 'autocrlf=true converts on input' '\n+\ttest_config core.autocrlf true &&\n+\tcheck_input\n+'\n+\n+test_expect_success 'eol=crlf converts on input' '\n+\tset_attr eol=crlf &&\n+\tcheck_input\n+'\n+\n+test_expect_success 'ident converts on input' '\n+\tset_attr ident &&\n+\tcheck_input\n+'\n+\n+test_expect_success 'user-defined filters convert on input' '\n+\tset_attr filter=test &&\n+\tcheck_input\n+'\n+\n+test_expect_success 'setup output tests' '\n+\techo \"\\$Id\\$\" >small &&\n+\tcat small small >large &&\n+\tgit add small large &&\n+\tgit config core.bigfilethreshold 7 &&\n+\tgit config filter.test.smudge \"sed s/.*/SMUDGE/\"\n+'\n+\n+test_expect_success 'autocrlf=true converts on output' '\n+\ttest_config core.autocrlf true &&\n+\tcheck_output\n+'\n+\n+test_expect_success 'eol=crlf converts on output' '\n+\tset_attr eol=crlf &&\n+\tcheck_output\n+'\n+\n+test_expect_success 'user-defined filters convert on output' '\n+\tset_attr filter=test &&\n+\tcheck_output\n+'\n+\n+test_expect_success 'ident converts on output' '\n+\tset_attr ident &&\n+\trm -f small large &&\n+\tgit checkout small large &&\n+\tsed -n \"s/Id: .*/Id: SHA/p\" <small >small.clean &&\n+\thead -n 1 large >large.head &&\n+\tsed -n \"s/Id: .*/Id: SHA/p\" <large.head >large.clean &&\n+\ttest_cmp small.clean large.clean\n+'\n+\n+test_done\n-- \n1.7.9.11.gca600\n"},{"id":"185395","messageId":"7v399zhn0l.fsf@alter.siamese.dyndns.org","threadId":"29718","inReplyTo":"20120224211913.GA30942@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] do not stream large files to pack when filters are in use","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-24T22:42:02Z","receivedAt":"2012-02-24T22:42:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I'll post the fixed series in a minute (with this fix, and the improved\n> convert_to_git wrapper).\n\nThanks.\n\nThe exclusion of path==NULL case was something I didn't think much about,\nbut I think your solution is the right one.\n"}]}