{"thread":{"id":"37295","subject":"[PATCH v2 0/2] Stream fd to clean filter","startedAt":"2014-08-06T05:32:12Z","lastAt":"2014-08-19T08:26:36Z","messageCount":13,"participants":["Steffen Prohaska","John Keeping","Andreas Schwab","Jeff King","Duy Nguyen"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"247310","messageId":"1407303134-16635-1-git-send-email-prohaska@zib.de","threadId":"37295","inReplyTo":null,"subject":"[PATCH v2 0/2] Stream fd to clean filter","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2014-08-06T05:32:12Z","receivedAt":"2014-08-06T05:32:12Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"The main difference to the previous version is that I've split off the\nrefactoring into a separate commit. The rest is polishing the style.\n\nSteffen Prohaska (2):\n convert: Refactor would_convert_to_git() to single arg 'path'\n convert: Stream from fd to required clean filter instead of mmap\n\n convert.c | 60 ++++++++++++++++++++++++++++++++++++++++-----\n convert.h | 10 +++++---\n sha1_file.c | 29 ++++++++++++++++++++--\n t/t0021-conversion.sh | 68 +++++++++++++++++++++++++++++++++++++++++++++++----\n 4 files changed, 151 insertions(+), 16 deletions(-)\n\n-- \n2.1.0.rc1.6.gb569bd8\n"},{"id":"247312","messageId":"1407303134-16635-2-git-send-email-prohaska@zib.de","threadId":"37295","inReplyTo":"1407303134-16635-1-git-send-email-prohaska@zib.de","subject":"[PATCH v2 1/2] convert: Refactor would_convert_to_git() to single arg 'path'","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2014-08-06T05:32:13Z","receivedAt":"2014-08-06T05:32:13Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"It is only the path that matters in the decision whether to filter or\nnot. Clarify this by making path the single argument of\nwould_convert_to_git().\n\nSigned-off-by: Steffen Prohaska \n---\n convert.h | 5 ++---\n sha1_file.c | 2 +-\n 2 files changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/convert.h b/convert.h\nindex 0c2143c..c638b33 100644\n--- a/convert.h\n+++ b/convert.h\n@@ -40,10 +40,9 @@ 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+static inline int would_convert_to_git(const char *path)\n {\n-\treturn convert_to_git(path, src, len, NULL, checksafe);\n+\treturn convert_to_git(path, NULL, 0, NULL, 0);\n }\n \n /*****************************************************************\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 3f70b1d..00c07f2 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -3144,7 +3144,7 @@ int index_fd(unsigned char *sha1, int fd, struct stat *st,\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-\t\t (path && would_convert_to_git(path, NULL, 0, 0)))\n+\t\t (path && would_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-- \n2.1.0.rc1.6.gb569bd8\n"},{"id":"247311","messageId":"1407303134-16635-3-git-send-email-prohaska@zib.de","threadId":"37295","inReplyTo":"1407303134-16635-1-git-send-email-prohaska@zib.de","subject":"[PATCH v2 2/2] convert: Stream from fd to required clean filter instead of mmap","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2014-08-06T05:32:14Z","receivedAt":"2014-08-06T05:32:14Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"The data is streamed to the filter process anyway. Better avoid mapping\nthe file if possible. This is especially useful if a clean filter\nreduces the size, for example if it computes a sha1 for binary data,\nlike git media. The file size that the previous implementation could\nhandle was limited by the available address space; large files for\nexample could not be handled with (32-bit) msysgit. The new\nimplementation can filter files of any size as long as the filter output\nis small enough.\n\nThe new code path is only taken if the filter is required. The filter\nconsumes data directly from the fd. The original data is not available\nto git, so it must fail if the filter fails.\n\nThe test that exercises required filters is modified to verify that the\ndata actually has been modified on its way from the file system to the\nobject store.\n\nThe expectation on the process size is tested using /usr/bin/time. An\nalternative would have been tcsh, which could be used to print memory\ninformation as follows:\n\n tcsh -c 'set time=(0 \"%M\"); '\n\nAlthough the logic could perhaps be simplified with tcsh, I chose to use\n'time' to avoid a dependency on tcsh.\n\nSigned-off-by: Steffen Prohaska \n---\n convert.c | 60 ++++++++++++++++++++++++++++++++++++++++-----\n convert.h | 5 ++++\n sha1_file.c | 27 +++++++++++++++++++-\n t/t0021-conversion.sh | 68 +++++++++++++++++++++++++++++++++++++++++++++++----\n 4 files changed, 148 insertions(+), 12 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex cb5fbb4..463f6de 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -312,11 +312,12 @@ static int crlf_to_worktree(const char *path, const char *src, size_t len,\n struct filter_params {\n \tconst char *src;\n \tunsigned long size;\n+\tint fd;\n \tconst char *cmd;\n \tconst char *path;\n };\n \n-static int filter_buffer(int in, int out, void *data)\n+static int filter_buffer_or_fd(int in, int out, void *data)\n {\n \t/*\n \t * Spawn cmd and feed the buffer contents through its stdin.\n@@ -325,6 +326,7 @@ static int filter_buffer(int in, int out, void *data)\n \tstruct filter_params *params = (struct filter_params *)data;\n \tint write_err, status;\n \tconst char *argv[] = { NULL, NULL };\n+\tint fd;\n \n \t/* apply % substitution to cmd */\n \tstruct strbuf cmd = STRBUF_INIT;\n@@ -355,7 +357,17 @@ static int filter_buffer(int in, int out, void *data)\n \n \tsigchain_push(SIGPIPE, SIG_IGN);\n \n-\twrite_err = (write_in_full(child_process.in, params->src, params->size) < 0);\n+\tif (params->src) {\n+\t write_err = (write_in_full(child_process.in, params->src, params->size) < 0);\n+\t} else {\n+\t /* dup(), because copy_fd() closes the input fd. */\n+\t fd = dup(params->fd);\n+\t if (fd < 0)\n+\t\twrite_err = error(\"failed to dup file descriptor.\");\n+\t else\n+\t\twrite_err = copy_fd(fd, child_process.in);\n+\t}\n+\n \tif (close(child_process.in))\n \t\twrite_err = 1;\n \tif (write_err)\n@@ -371,7 +383,7 @@ static int filter_buffer(int in, int out, void *data)\n \treturn (write_err || status);\n }\n \n-static int apply_filter(const char *path, const char *src, size_t len,\n+static int apply_filter(const char *path, const char *src, size_t len, int fd,\n struct strbuf *dst, const char *cmd)\n {\n \t/*\n@@ -392,11 +404,12 @@ static int apply_filter(const char *path, const char *src, size_t len,\n \t\treturn 1;\n \n \tmemset(&async, 0, sizeof(async));\n-\tasync.proc = filter_buffer;\n+\tasync.proc = filter_buffer_or_fd;\n \tasync.data = ¶ms;\n \tasync.out = -1;\n \tparams.src = src;\n \tparams.size = len;\n+\tparams.fd = fd;\n \tparams.cmd = cmd;\n \tparams.path = path;\n \n@@ -747,6 +760,24 @@ static void convert_attrs(struct conv_attrs *ca, const char *path)\n \t}\n }\n \n+int would_convert_to_git_filter_fd(const char *path)\n+{\n+\tstruct conv_attrs ca;\n+\n+\tconvert_attrs(&ca, path);\n+\tif (!ca.drv)\n+\t return 0;\n+\n+\t/* Apply a filter to an fd only if the filter is required to succeed.\n+\t * We must die if the filter fails, because the original data before\n+\t * filtering is not available.\n+\t */\n+\tif (!ca.drv->required)\n+\t return 0;\n+\n+\treturn apply_filter(path, NULL, 0, -1, NULL, ca.drv->clean);\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 {\n@@ -761,7 +792,7 @@ int convert_to_git(const char *path, const char *src, size_t len,\n \t\trequired = ca.drv->required;\n \t}\n \n-\tret |= apply_filter(path, src, len, dst, filter);\n+\tret |= apply_filter(path, src, len, -1, dst, filter);\n \tif (!ret && required)\n \t\tdie(\"%s: clean filter '%s' failed\", path, ca.drv->name);\n \n@@ -778,6 +809,23 @@ int convert_to_git(const char *path, const char *src, size_t len,\n \treturn ret | ident_to_git(path, src, len, dst, ca.ident);\n }\n \n+void convert_to_git_filter_fd(const char *path, int fd, struct strbuf *dst,\n+\t\t\t enum safe_crlf checksafe)\n+{\n+\tstruct conv_attrs ca;\n+\tconvert_attrs(&ca, path);\n+\n+\tassert(ca.drv);\n+\tassert(ca.drv->clean);\n+\n+\tif (!apply_filter(path, NULL, 0, fd, dst, ca.drv->clean))\n+\t\tdie(\"%s: clean filter '%s' failed\", path, ca.drv->name);\n+\n+\tca.crlf_action = input_crlf_action(ca.crlf_action, ca.eol_attr);\n+\tcrlf_to_git(path, dst->buf, dst->len, dst, ca.crlf_action, checksafe);\n+\tident_to_git(path, dst->buf, dst->len, dst, ca.ident);\n+}\n+\n static int convert_to_working_tree_internal(const char *path, const char *src,\n \t\t\t\t\t size_t len, struct strbuf *dst,\n \t\t\t\t\t int normalizing)\n@@ -811,7 +859,7 @@ static int convert_to_working_tree_internal(const char *path, const char *src,\n \t\t}\n \t}\n \n-\tret_filter = apply_filter(path, src, len, dst, filter);\n+\tret_filter = apply_filter(path, src, len, -1, dst, filter);\n \tif (!ret_filter && required)\n \t\tdie(\"%s: smudge filter %s failed\", path, ca.drv->name);\n \ndiff --git a/convert.h b/convert.h\nindex c638b33..d9d853c 100644\n--- a/convert.h\n+++ b/convert.h\n@@ -44,6 +44,11 @@ static inline int would_convert_to_git(const char *path)\n {\n \treturn convert_to_git(path, NULL, 0, NULL, 0);\n }\n+/* Precondition: would_convert_to_git_filter_fd(path) == true */\n+extern void convert_to_git_filter_fd(const char *path, int fd,\n+\t\t\t\t struct strbuf *dst,\n+\t\t\t\t enum safe_crlf checksafe);\n+extern int would_convert_to_git_filter_fd(const char *path);\n \n /*****************************************************************\n *\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 00c07f2..eaf3220 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -3074,6 +3074,29 @@ static int index_mem(unsigned char *sha1, void *buf, size_t size,\n \treturn ret;\n }\n \n+static int index_stream_convert_blob(unsigned char *sha1, int fd,\n+\t\t\t\t const char *path, unsigned flags)\n+{\n+\tint ret;\n+\tconst int write_object = flags & HASH_WRITE_OBJECT;\n+\tstruct strbuf sbuf = STRBUF_INIT;\n+\n+\tassert(path);\n+\tassert(would_convert_to_git_filter_fd(path));\n+\n+\tconvert_to_git_filter_fd(path, fd, &sbuf,\n+\t\t\t\t write_object ? safe_crlf : SAFE_CRLF_FALSE);\n+\n+\tif (write_object)\n+\t\tret = write_sha1_file(sbuf.buf, sbuf.len, typename(OBJ_BLOB),\n+\t\t\t\t sha1);\n+\telse\n+\t\tret = hash_sha1_file(sbuf.buf, sbuf.len, typename(OBJ_BLOB),\n+\t\t\t\t sha1);\n+\tstrbuf_release(&sbuf);\n+\treturn ret;\n+}\n+\n static int index_pipe(unsigned char *sha1, int fd, enum object_type type,\n \t\t const char *path, unsigned flags)\n {\n@@ -3141,7 +3164,9 @@ int index_fd(unsigned char *sha1, int fd, struct stat *st,\n \tint ret;\n \tsize_t size = xsize_t(st->st_size);\n \n-\tif (!S_ISREG(st->st_mode))\n+\tif (type == OBJ_BLOB && path && would_convert_to_git_filter_fd(path))\n+\t\tret = index_stream_convert_blob(sha1, fd, path, flags);\n+\telse if (!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 \t\t (path && would_convert_to_git(path)))\ndiff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\nindex f890c54..2cb2414 100755\n--- a/t/t0021-conversion.sh\n+++ b/t/t0021-conversion.sh\n@@ -153,17 +153,23 @@ test_expect_success 'filter shell-escaped filenames' '\n \t:\n '\n \n-test_expect_success 'required filter success' '\n-\tgit config filter.required.smudge cat &&\n-\tgit config filter.required.clean cat &&\n+test_expect_success 'required filter should filter data' '\n+\tgit config filter.required.smudge ./rot13.sh &&\n+\tgit config filter.required.clean ./rot13.sh &&\n \tgit config filter.required.required true &&\n \n \techo \"*.r filter=required\" >.gitattributes &&\n \n-\techo test >test.r &&\n+\tcat test.o >test.r &&\n \tgit add test.r &&\n+\n \trm -f test.r &&\n-\tgit checkout -- test.r\n+\tgit checkout -- test.r &&\n+\tcmp test.o test.r &&\n+\n+\t./rot13.sh expected &&\n+\tgit cat-file blob :test.r >actual &&\n+\tcmp expected actual\n '\n \n test_expect_success 'required filter smudge failure' '\n@@ -190,6 +196,58 @@ test_expect_success 'required filter clean failure' '\n \ttest_must_fail git add test.fc\n '\n \n+# Handle differences in /usr/bin/time.\n+#\n+# - Linux: call with '-v'.\n+# output: :\n+#\n+# - Mac: call with '-l'.\n+# output: \n+# Strip three digits to get to KB (base 10 is good enough).\n+#\n+case $(uname -s) in\n+Linux)\n+\ttest_set_prereq HAVE_MAX_MEM_USAGE\n+\tmax_mem_usage_KB () {\n+\t /usr/bin/time -v \"$@\" 2>&1 |\n+\t grep 'Maximum resident set size' |\n+\t cut -d ':' -f 2\n+\t}\n+\t;;\n+Darwin)\n+\ttest_set_prereq HAVE_MAX_MEM_USAGE\n+\tmax_mem_usage_KB () {\n+\t\t/usr/bin/time -l \"$@\" 2>&1 |\n+\t\tgrep 'maximum resident set size' |\n+\t\tsed -e 's/ */ /' |\n+\t\tcut -d ' ' -f 2 |\n+\t\tsed -e 's/...$//'\n+\t}\n+\t;;\n+esac\n+\n+max_mem_usage_is_lt_KB () {\n+\tlimit=$1\n+\tshift\n+\tmem_usage=$(max_mem_usage_KB \"$@\")\n+\tif [ $mem_usage -lt $limit ]; then\n+\t\ttrue\n+\telse\n+\t\tprintf 'Command used too much memory (expected limit %dKB, actual usage %dKB).\\n' \\\n+\t\t\t$limit $mem_usage\n+\t\tfalse\n+\tfi\n+}\n+\n+test_expect_success HAVE_MAX_MEM_USAGE \\\n+'filtering large input to small output should use little memory' '\n+\tgit config filter.devnull.clean \"cat >/dev/null\" &&\n+\tgit config filter.devnull.required true &&\n+\tfor i in $(test_seq 1 30); do printf \"%1048576d\" 1; done >30MB &&\n+\techo \"30MB filter=devnull\" >.gitattributes &&\n+\tmax_mem_usage_is_lt_KB 15000 git add 30MB\n+'\n+\n test_expect_success EXPENSIVE 'filter large file' '\n \tgit config filter.largefile.smudge cat &&\n \tgit config filter.largefile.clean cat &&\n-- \n2.1.0.rc1.6.gb569bd8\n"},{"id":"247772","messageId":"20140816102703.GD7857@serenity.lan","threadId":"37295","inReplyTo":"1407303134-16635-3-git-send-email-prohaska@zib.de","subject":"Re: [PATCH v2 2/2] convert: Stream from fd to required clean filter instead of mmap","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2014-08-16T10:27:03Z","receivedAt":"2014-08-16T10:27:03Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Wed, Aug 06, 2014 at 07:32:14AM +0200, Steffen Prohaska wrote:\n[...]\n> The expectation on the process size is tested using /usr/bin/time. An\n> alternative would have been tcsh, which could be used to print memory\n> information as follows:\n> \n> tcsh -c 'set time=(0 \"%M\"); '\n> \n> Although the logic could perhaps be simplified with tcsh, I chose to use\n> 'time' to avoid a dependency on tcsh.\n[...]\n> @@ -190,6 +196,58 @@ test_expect_success 'required filter clean failure' '\n> \ttest_must_fail git add test.fc\n> '\n> \n> +# Handle differences in /usr/bin/time.\n> +#\n> +# - Linux: call with '-v'.\n> +# output: :\n> +#\n> +# - Mac: call with '-l'.\n> +# output: \n> +# Strip three digits to get to KB (base 10 is good enough).\n> +#\n> +case $(uname -s) in\n> +Linux)\n> +\ttest_set_prereq HAVE_MAX_MEM_USAGE\n> +\tmax_mem_usage_KB () {\n> +\t /usr/bin/time -v \"$@\" 2>&1 |\n> +\t grep 'Maximum resident set size' |\n> +\t cut -d ':' -f 2\n> +\t}\n> +\t;;\n> +Darwin)\n> +\ttest_set_prereq HAVE_MAX_MEM_USAGE\n> +\tmax_mem_usage_KB () {\n> +\t\t/usr/bin/time -l \"$@\" 2>&1 |\n> +\t\tgrep 'maximum resident set size' |\n> +\t\tsed -e 's/ */ /' |\n> +\t\tcut -d ' ' -f 2 |\n> +\t\tsed -e 's/...$//'\n> +\t}\n> +\t;;\n> +esac\n> +\n> +max_mem_usage_is_lt_KB () {\n> +\tlimit=$1\n> +\tshift\n> +\tmem_usage=$(max_mem_usage_KB \"$@\")\n> +\tif [ $mem_usage -lt $limit ]; then\n> +\t\ttrue\n> +\telse\n> +\t\tprintf 'Command used too much memory (expected limit %dKB, actual usage %dKB).\\n' \\\n> +\t\t\t$limit $mem_usage\n> +\t\tfalse\n> +\tfi\n> +}\n> +\n> +test_expect_success HAVE_MAX_MEM_USAGE \\\n> +'filtering large input to small output should use little memory' '\n> +\tgit config filter.devnull.clean \"cat >/dev/null\" &&\n> +\tgit config filter.devnull.required true &&\n> +\tfor i in $(test_seq 1 30); do printf \"%1048576d\" 1; done >30MB &&\n> +\techo \"30MB filter=devnull\" >.gitattributes &&\n> +\tmax_mem_usage_is_lt_KB 15000 git add 30MB\n> +'\n\nThis test fails for me:\n\n-- 8< --\nexpecting success: \n git config filter.devnull.clean \"cat >/dev/null\" &&\n git config filter.devnull.required true &&\n for i in $(test_seq 1 30); do printf \"%1048576d\" 1; done >30MB &&\n echo \"30MB filter=devnull\" >.gitattributes &&\n max_mem_usage_is_lt_KB 15000 git add 30MB\n\nCommand used too much memory (expected limit 15000KB, actual usage 15808KB).\nnot ok 8 - filtering large input to small output should use little memory\n-- >8 --\n\nThis is on Linux 3.16 x86_64 with GCC 4.8.3 and glibc 2.19. My GCC also\nhas \"-fstack-protector\" and \"-D_FORTIFY_SOURCE=2\" enabled by default;\nturning those off for Git decreased the memory usage by about 500KB but\nnot enough to make the test pass. Of course, all of the libraries Git\nis linking are also compiled with those flags...\n\nIs the 15MB limit supposed to be imposed somewhere or is it just a guide\nof how much memory we expect Git to use in this scenario?\n"},{"id":"247776","messageId":"FA3F1197-25C5-42E4-9418-1C821D430819@zib.de","threadId":"37295","inReplyTo":"20140816102703.GD7857@serenity.lan","subject":"Re: [PATCH v2 2/2] convert: Stream from fd to required clean filter instead of mmap","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2014-08-16T16:26:08Z","receivedAt":"2014-08-16T16:26:08Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"\nOn Aug 16, 2014, at 12:27 PM, John Keeping wrote:\n\n>> +test_expect_success HAVE_MAX_MEM_USAGE \\\n>> +'filtering large input to small output should use little memory' '\n>> +\tgit config filter.devnull.clean \"cat >/dev/null\" &&\n>> +\tgit config filter.devnull.required true &&\n>> +\tfor i in $(test_seq 1 30); do printf \"%1048576d\" 1; done >30MB &&\n>> +\techo \"30MB filter=devnull\" >.gitattributes &&\n>> +\tmax_mem_usage_is_lt_KB 15000 git add 30MB\n>> +'\n> \n> This test fails for me:\n> \n> -- 8< --\n> expecting success: \n> git config filter.devnull.clean \"cat >/dev/null\" &&\n> git config filter.devnull.required true &&\n> for i in $(test_seq 1 30); do printf \"%1048576d\" 1; done >30MB &&\n> echo \"30MB filter=devnull\" >.gitattributes &&\n> max_mem_usage_is_lt_KB 15000 git add 30MB\n> \n> Command used too much memory (expected limit 15000KB, actual usage 15808KB).\n> not ok 8 - filtering large input to small output should use little memory\n> -- >8 --\n> \n> This is on Linux 3.16 x86_64 with GCC 4.8.3 and glibc 2.19. My GCC also\n> has \"-fstack-protector\" and \"-D_FORTIFY_SOURCE=2\" enabled by default;\n> turning those off for Git decreased the memory usage by about 500KB but\n> not enough to make the test pass. Of course, all of the libraries Git\n> is linking are also compiled with those flags...\n> \n> Is the 15MB limit supposed to be imposed somewhere or is it just a guide\n> of how much memory we expect Git to use in this scenario?\n\nThe test should confirm that the the file that is added is not mmapped to memory. The process size should be relatively small independently of the size of the file that is added. I wanted to keep the file size small. The chosen sizes worked for me on Mac and Linux.\n\nA simple solution could be to increase the size of the test file and the limit. I'd suggest to squash the diff below. The test should still run reasonably fast with a 50 MB test file.\n\nJunio, shall I send a whole updated patch series?\n\n\tSteffen\n\n\ndiff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\nindex 2cb2414..43de87d 100755\n--- a/t/t0021-conversion.sh\n+++ b/t/t0021-conversion.sh\n@@ -243,9 +243,9 @@ test_expect_success HAVE_MAX_MEM_USAGE \\\n 'filtering large input to small output should use little memory' '\n git config filter.devnull.clean \"cat >/dev/null\" &&\n git config filter.devnull.required true &&\n- for i in $(test_seq 1 30); do printf \"%1048576d\" 1; done >30MB &&\n- echo \"30MB filter=devnull\" >.gitattributes &&\n- max_mem_usage_is_lt_KB 15000 git add 30MB\n+ for i in $(test_seq 1 50); do printf \"%1048576d\" 1; done >50MB &&\n+ echo \"50MB filter=devnull\" >.gitattributes &&\n+ max_mem_usage_is_lt_KB 35000 git add 50MB\n '\n"},{"id":"247789","messageId":"874mxc9xp3.fsf@igel.home","threadId":"37295","inReplyTo":"FA3F1197-25C5-42E4-9418-1C821D430819@zib.de","subject":"Re: [PATCH v2 2/2] convert: Stream from fd to required clean filter instead of mmap","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2014-08-16T17:00:24Z","receivedAt":"2014-08-16T17:00:24Z","isPatch":true,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"Steffen Prohaska writes:\n\n> The test should confirm that the the file that is added is not mmapped to\n> memory.\n\nRSS doesn't tell you that. You can mmap a big file without RSS getting\nbigger.\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5\n\"And now for something completely different.\"\n"},{"id":"247803","messageId":"20140817072746.GD23808@peff.net","threadId":"37295","inReplyTo":"FA3F1197-25C5-42E4-9418-1C821D430819@zib.de","subject":"Re: [PATCH v2 2/2] convert: Stream from fd to required clean filter instead of mmap","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-08-17T07:27:47Z","receivedAt":"2014-08-17T07:27:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Aug 16, 2014 at 06:26:08PM +0200, Steffen Prohaska wrote:\n\n> > Is the 15MB limit supposed to be imposed somewhere or is it just a guide\n> > of how much memory we expect Git to use in this scenario?\n> \n> The test should confirm that the the file that is added is not mmapped\n> to memory. The process size should be relatively small independently\n> of the size of the file that is added. I wanted to keep the file size\n> small. The chosen sizes worked for me on Mac and Linux.\n\nMeasuring memory usage seems inherently a bit flaky for the test suite.\nIt's also a little out of place, as the test suite is generally about\ncorrectness and outcomes, and this is a performance issue.\n\nWould it make more sense to construct a t/perf test that shows off the\nchange? I suppose the run-time change may not be that impressive, but it\nwould be cool if t/perf could measure max memory use, too. Then we can\njust compare results between versions, which is enough to detect\nregressions.\n\nThere's some prior art in the jk/pack-bitmap-reuse-deltas series (which\nis not merged), where I taught it to measure output sizes of commands.\nThat should provide the necessary refactoring base, I think.\n\n-Peff\n"},{"id":"247813","messageId":"7147D39A-66CE-4824-B051-C625789BFFA5@zib.de","threadId":"37295","inReplyTo":"874mxc9xp3.fsf@igel.home","subject":"Re: [PATCH v2 2/2] convert: Stream from fd to required clean filter instead of mmap","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2014-08-17T10:08:30Z","receivedAt":"2014-08-17T10:08:30Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"\nOn Aug 16, 2014, at 7:00 PM, Andreas Schwab wrote:\n\n> Steffen Prohaska writes:\n> \n>> The test should confirm that the the file that is added is not mmapped to\n>> memory.\n> \n> RSS doesn't tell you that. You can mmap a big file without RSS getting\n> bigger.\n\nAll data are accessed after mapping, so RSS should be fine. The test always did what I expected.\n\n\tSteffen\n"},{"id":"247814","messageId":"C1695CEB-B278-422A-90D6-C3EAD8B6638C@zib.de","threadId":"37295","inReplyTo":"20140817072746.GD23808@peff.net","subject":"Re: [PATCH v2 2/2] convert: Stream from fd to required clean filter instead of mmap","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2014-08-17T10:25:51Z","receivedAt":"2014-08-17T10:25:51Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"\nOn Aug 17, 2014, at 9:27 AM, Jeff King wrote:\n\n> On Sat, Aug 16, 2014 at 06:26:08PM +0200, Steffen Prohaska wrote:\n> \n>>> Is the 15MB limit supposed to be imposed somewhere or is it just a guide\n>>> of how much memory we expect Git to use in this scenario?\n>> \n>> The test should confirm that the the file that is added is not mmapped\n>> to memory. The process size should be relatively small independently\n>> of the size of the file that is added. I wanted to keep the file size\n>> small. The chosen sizes worked for me on Mac and Linux.\n> \n> Measuring memory usage seems inherently a bit flaky for the test suite.\n> It's also a little out of place, as the test suite is generally about\n> correctness and outcomes, and this is a performance issue.\n\nFor files >2GB on a 32-bit system (e.g. msysgit), filtering with the previous code always failed. Now it works. I created the patch to change git from 'fundamentally doesn't handle this' to 'works as expected'.\n\n\n> Would it make more sense to construct a t/perf test that shows off the\n> change? I suppose the run-time change may not be that impressive, but it\n> would be cool if t/perf could measure max memory use, too. Then we can\n> just compare results between versions, which is enough to detect\n> regressions.\n\nI wasn't aware of t/perf. Thanks for suggesting this.\n\nI agree that testing memory usage might be a bit flaky. t/perf might indeed be a better place.\n\nI'm not yet entirely convinced, though. I'm wondering whether the proposed test would be robust enough with a large enough threshold to keep it in the main test suite.\n\n\tSteffen\n"},{"id":"247815","messageId":"871tsfwf74.fsf@igel.home","threadId":"37295","inReplyTo":"7147D39A-66CE-4824-B051-C625789BFFA5@zib.de","subject":"Re: [PATCH v2 2/2] convert: Stream from fd to required clean filter instead of mmap","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2014-08-17T11:03:43Z","receivedAt":"2014-08-17T11:03:43Z","isPatch":true,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"Steffen Prohaska writes:\n\n> On Aug 16, 2014, at 7:00 PM, Andreas Schwab wrote:\n>\n>> Steffen Prohaska writes:\n>> \n>>> The test should confirm that the the file that is added is not mmapped to\n>>> memory.\n>> \n>> RSS doesn't tell you that. You can mmap a big file without RSS getting\n>> bigger.\n>\n> All data are accessed after mapping, so RSS should be fine.\n\nRSS can shink any time when memory is tight.\n\n> The test always did what I expected.\n\nNot a sufficient condition.\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5\n\"And now for something completely different.\"\n"},{"id":"247816","messageId":"CACsJy8BUBfQeny4iu6nZ+pCy84_=8EEfEHY3pWLpx_1PLCwC5w@mail.gmail.com","threadId":"37295","inReplyTo":"C1695CEB-B278-422A-90D6-C3EAD8B6638C@zib.de","subject":"Re: [PATCH v2 2/2] convert: Stream from fd to required clean filter instead of mmap","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-08-17T11:33:54Z","receivedAt":"2014-08-17T11:33:54Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Aug 17, 2014 at 5:25 PM, Steffen Prohaska wrote:\n>\n> On Aug 17, 2014, at 9:27 AM, Jeff King wrote:\n>\n>> On Sat, Aug 16, 2014 at 06:26:08PM +0200, Steffen Prohaska wrote:\n>>\n>>>> Is the 15MB limit supposed to be imposed somewhere or is it just a guide\n>>>> of how much memory we expect Git to use in this scenario?\n>>>\n>>> The test should confirm that the the file that is added is not mmapped\n>>> to memory. The process size should be relatively small independently\n>>> of the size of the file that is added. I wanted to keep the file size\n>>> small. The chosen sizes worked for me on Mac and Linux.\n>>\n>> Measuring memory usage seems inherently a bit flaky for the test suite.\n>> It's also a little out of place, as the test suite is generally about\n>> correctness and outcomes, and this is a performance issue.\n>\n> For files >2GB on a 32-bit system (e.g. msysgit), filtering with the previous code always failed. Now it works. I created the patch to change git from 'fundamentally doesn't handle this' to 'works as expected'.\n\nI deal with similar problem and added $GIT_ALLOC_LIMIT to test large\nblob code. Maybe we could add $GIT_MMAP_LIMIT? I don't suppose we call\nxmmap/xmalloc so often that the extra test would slow git down.\n-- \nDuy\n"},{"id":"247895","messageId":"20140819075345.GA7712@peff.net","threadId":"37295","inReplyTo":"CACsJy8BUBfQeny4iu6nZ+pCy84_=8EEfEHY3pWLpx_1PLCwC5w@mail.gmail.com","subject":"Re: [PATCH v2 2/2] convert: Stream from fd to required clean filter instead of mmap","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-08-19T07:53:45Z","receivedAt":"2014-08-19T07:53:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Aug 17, 2014 at 06:33:54PM +0700, Duy Nguyen wrote:\n\n> > For files >2GB on a 32-bit system (e.g. msysgit), filtering with the\n> > previous code always failed. Now it works. I created the patch to\n> > change git from 'fundamentally doesn't handle this' to 'works as\n> > expected'.\n> \n> I deal with similar problem and added $GIT_ALLOC_LIMIT to test large\n> blob code. Maybe we could add $GIT_MMAP_LIMIT? I don't suppose we call\n> xmmap/xmalloc so often that the extra test would slow git down.\n\nYeah, I think I'd prefer that approach. We should mmap _way_ less than\nwe malloc, and I do not think the malloc check has caused any problems.\n\n-Peff\n"},{"id":"247898","messageId":"7D3A2FB9-F1CA-470C-91CC-8FF28097F812@zib.de","threadId":"37295","inReplyTo":"20140819075345.GA7712@peff.net","subject":"Re: [PATCH v2 2/2] convert: Stream from fd to required clean filter instead of mmap","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2014-08-19T08:26:36Z","receivedAt":"2014-08-19T08:26:36Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"\nOn Aug 19, 2014, at 9:53 AM, Jeff King wrote:\n\n>>> For files >2GB on a 32-bit system (e.g. msysgit), filtering with the\n>>> previous code always failed. Now it works. I created the patch to\n>>> change git from 'fundamentally doesn't handle this' to 'works as\n>>> expected'.\n>> \n>> I deal with similar problem and added $GIT_ALLOC_LIMIT to test large\n>> blob code. Maybe we could add $GIT_MMAP_LIMIT? I don't suppose we call\n>> xmmap/xmalloc so often that the extra test would slow git down.\n> \n> Yeah, I think I'd prefer that approach. We should mmap _way_ less than\n> we malloc, and I do not think the malloc check has caused any problems.\n\nI am going to update the patch accordingly.\n\n\tSteffen\n"}]}