{"thread":{"id":"37417","subject":"[PATCH v6 1/6] convert: drop arguments other than 'path' from would_convert_to_git()","startedAt":"2014-08-26T15:23:19Z","lastAt":"2014-08-28T17:15:21Z","messageCount":19,"participants":["Steffen Prohaska","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":6,"patchTotal":6},"messages":[{"id":"248314","messageId":"1409066605-4851-1-git-send-email-prohaska@zib.de","threadId":"37417","inReplyTo":null,"subject":"[PATCH v6 0/6] Stream fd to clean filter; GIT_MMAP_LIMIT, GIT_ALLOC_LIMIT with git_env_ulong()","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2014-08-26T15:23:19Z","receivedAt":"2014-08-26T15:23:19Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"Changes since v5:\n\n - Introduce and use git_env_ulong().\n - Change copy_fd() to not close input fd, which simplified changes to convert.\n - More detailed explanation why filter must be marked \"required\" in commit\n message of 6/6.\n - Style fixes.\n\nSteffen Prohaska (6):\n convert: drop arguments other than 'path' from would_convert_to_git()\n Add git_env_ulong() to parse environment variable\n Change GIT_ALLOC_LIMIT check to use git_env_ulong()\n Introduce GIT_MMAP_LIMIT to allow testing expected mmap size\n Change copy_fd() to not close input fd\n convert: stream from fd to required clean filter to reduce used\n address space\n\n cache.h | 1 +\n config.c | 11 +++++++++++\n convert.c | 55 +++++++++++++++++++++++++++++++++++++++++++++------\n convert.h | 10 +++++++---\n copy.c | 5 +----\n lockfile.c | 3 +++\n sha1_file.c | 47 ++++++++++++++++++++++++++++++++++++++++---\n t/t0021-conversion.sh | 24 +++++++++++++++++-----\n t/t1050-large.sh | 2 +-\n wrapper.c | 15 +++++++-------\n 10 files changed, 144 insertions(+), 29 deletions(-)\n\n-- \n2.1.0.8.gf3a29c8\n"},{"id":"248310","messageId":"1409066605-4851-2-git-send-email-prohaska@zib.de","threadId":"37417","inReplyTo":"1409066605-4851-1-git-send-email-prohaska@zib.de","subject":"[PATCH v6 1/6] convert: drop arguments other than 'path' from would_convert_to_git()","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2014-08-26T15:23:20Z","receivedAt":"2014-08-26T15:23:20Z","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.8.gf3a29c8\n"},{"id":"248311","messageId":"1409066605-4851-3-git-send-email-prohaska@zib.de","threadId":"37417","inReplyTo":"1409066605-4851-1-git-send-email-prohaska@zib.de","subject":"[PATCH v6 2/6] Add git_env_ulong() to parse environment variable","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2014-08-26T15:23:21Z","receivedAt":"2014-08-26T15:23:21Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"The new function will be used to parse GIT_MMAP_LIMIT and\nGIT_ALLOC_LIMIT.\n\nSigned-off-by: Steffen Prohaska \n---\n cache.h | 1 +\n config.c | 11 +++++++++++\n 2 files changed, 12 insertions(+)\n\ndiff --git a/cache.h b/cache.h\nindex fcb511d..b820b6a 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1318,6 +1318,7 @@ extern int git_config_rename_section_in_file(const char *, const char *, const c\n extern const char *git_etc_gitconfig(void);\n extern int check_repository_format_version(const char *var, const char *value, void *cb);\n extern int git_env_bool(const char *, int);\n+extern unsigned long git_env_ulong(const char *, unsigned long);\n extern int git_config_system(void);\n extern int config_error_nonbool(const char *);\n #if defined(__GNUC__)\ndiff --git a/config.c b/config.c\nindex 058505c..178721f 100644\n--- a/config.c\n+++ b/config.c\n@@ -1122,6 +1122,17 @@ int git_env_bool(const char *k, int def)\n \treturn v ? git_config_bool(k, v) : def;\n }\n \n+/*\n+ * Use default if environment variable is unset or empty string.\n+ */\n+unsigned long git_env_ulong(const char *k, unsigned long val)\n+{\n+\tconst char *v = getenv(k);\n+\tif (v && *v && !git_parse_ulong(v, &val))\n+\t\tdie(\"failed to parse %s\", k);\n+\treturn val;\n+}\n+\n int git_config_system(void)\n {\n \treturn !git_env_bool(\"GIT_CONFIG_NOSYSTEM\", 0);\n-- \n2.1.0.8.gf3a29c8\n"},{"id":"248316","messageId":"1409066605-4851-4-git-send-email-prohaska@zib.de","threadId":"37417","inReplyTo":"1409066605-4851-1-git-send-email-prohaska@zib.de","subject":"[PATCH v6 3/6] Change GIT_ALLOC_LIMIT check to use git_env_ulong()","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2014-08-26T15:23:22Z","receivedAt":"2014-08-26T15:23:22Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"GIT_ALLOC_LIMIT limits xmalloc()'s size, which is of type size_t.\nBetter use git_env_ulong() to parse the environment variable, so that\nthe postfixes 'k', 'm', and 'g' can be used; and use size_t to store the\nlimit for consistency. The change to size_t has no direct practical\nimpact, because we use GIT_ALLOC_LIMIT to test small sizes.\n\nThe cast of size in the call to die() is changed to uintmax_t to match\nthe format string PRIuMAX.\n\nSigned-off-by: Steffen Prohaska \n---\n t/t1050-large.sh | 2 +-\n wrapper.c | 15 ++++++++-------\n 2 files changed, 9 insertions(+), 8 deletions(-)\n\ndiff --git a/t/t1050-large.sh b/t/t1050-large.sh\nindex aea4936..e7657ab 100755\n--- a/t/t1050-large.sh\n+++ b/t/t1050-large.sh\n@@ -13,7 +13,7 @@ test_expect_success setup '\n \techo X | dd of=large2 bs=1k seek=2000 &&\n \techo X | dd of=large3 bs=1k seek=2000 &&\n \techo Y | dd of=huge bs=1k seek=2500 &&\n-\tGIT_ALLOC_LIMIT=1500 &&\n+\tGIT_ALLOC_LIMIT=1500k &&\n \texport GIT_ALLOC_LIMIT\n '\n \ndiff --git a/wrapper.c b/wrapper.c\nindex bc1bfb8..c5204f7 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -11,14 +11,15 @@ static void (*try_to_free_routine)(size_t size) = do_nothing;\n \n static void memory_limit_check(size_t size)\n {\n-\tstatic int limit = -1;\n-\tif (limit == -1) {\n-\t\tconst char *env = getenv(\"GIT_ALLOC_LIMIT\");\n-\t\tlimit = env ? atoi(env) * 1024 : 0;\n+\tstatic size_t limit = 0;\n+\tif (!limit) {\n+\t\tlimit = git_env_ulong(\"GIT_ALLOC_LIMIT\", 0);\n+\t\tif (!limit)\n+\t\t\tlimit = SIZE_MAX;\n \t}\n-\tif (limit && size > limit)\n-\t\tdie(\"attempting to allocate %\"PRIuMAX\" over limit %d\",\n-\t\t (intmax_t)size, limit);\n+\tif (size > limit)\n+\t\tdie(\"attempting to allocate %\"PRIuMAX\" over limit %\"PRIuMAX,\n+\t\t (uintmax_t)size, (uintmax_t)limit);\n }\n \n try_to_free_t set_try_to_free_routine(try_to_free_t routine)\n-- \n2.1.0.8.gf3a29c8\n"},{"id":"248312","messageId":"1409066605-4851-5-git-send-email-prohaska@zib.de","threadId":"37417","inReplyTo":"1409066605-4851-1-git-send-email-prohaska@zib.de","subject":"[PATCH v6 4/6] Introduce GIT_MMAP_LIMIT to allow testing expected mmap size","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2014-08-26T15:23:23Z","receivedAt":"2014-08-26T15:23:23Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"Similar to testing expectations about malloc with GIT_ALLOC_LIMIT (see\ncommit d41489 and previous commit), it can be useful to test\nexpectations about mmap.\n\nThis introduces a new environment variable GIT_MMAP_LIMIT to limit the\nlargest allowed mmap length. xmmap() is modified to check the limit.\nTogether with GIT_ALLOC_LIMIT tests can now easily confirm expectations\nabout memory consumption.\n\nGIT_MMAP_LIMIT will be used in the next commit to test that data will be\nstreamed to an external filter without mmaping the entire file.\n\n[commit d41489]: d41489a6424308dc9a0409bc2f6845aa08bd4f7d Add more large\n blob test cases\n\nSigned-off-by: Steffen Prohaska \n---\n sha1_file.c | 18 +++++++++++++++++-\n 1 file changed, 17 insertions(+), 1 deletion(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 00c07f2..d9b5157 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -663,10 +663,26 @@ void release_pack_memory(size_t need)\n \t\t; /* nothing */\n }\n \n+static void mmap_limit_check(size_t length)\n+{\n+\tstatic size_t limit = 0;\n+\tif (!limit) {\n+\t\tlimit = git_env_ulong(\"GIT_MMAP_LIMIT\", 0);\n+\t\tif (!limit)\n+\t\t\tlimit = SIZE_MAX;\n+\t}\n+\tif (length > limit)\n+\t\tdie(\"attempting to mmap %\"PRIuMAX\" over limit %\"PRIuMAX,\n+\t\t (uintmax_t)length, (uintmax_t)limit);\n+}\n+\n void *xmmap(void *start, size_t length,\n \tint prot, int flags, int fd, off_t offset)\n {\n-\tvoid *ret = mmap(start, length, prot, flags, fd, offset);\n+\tvoid *ret;\n+\n+\tmmap_limit_check(length);\n+\tret = mmap(start, length, prot, flags, fd, offset);\n \tif (ret == MAP_FAILED) {\n \t\tif (!length)\n \t\t\treturn NULL;\n-- \n2.1.0.8.gf3a29c8\n"},{"id":"248313","messageId":"1409066605-4851-6-git-send-email-prohaska@zib.de","threadId":"37417","inReplyTo":"1409066605-4851-1-git-send-email-prohaska@zib.de","subject":"[PATCH v6 5/6] Change copy_fd() to not close input fd","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2014-08-26T15:23:24Z","receivedAt":"2014-08-26T15:23:24Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"The caller opened the fd, so it should be responsible for closing it.\n\nSigned-off-by: Steffen Prohaska \n---\n copy.c | 5 +----\n lockfile.c | 3 +++\n 2 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/copy.c b/copy.c\nindex a7f58fd..d0a1d82 100644\n--- a/copy.c\n+++ b/copy.c\n@@ -10,7 +10,6 @@ int copy_fd(int ifd, int ofd)\n \t\t\tbreak;\n \t\tif (len < 0) {\n \t\t\tint read_error = errno;\n-\t\t\tclose(ifd);\n \t\t\treturn error(\"copy-fd: read returned %s\",\n \t\t\t\t strerror(read_error));\n \t\t}\n@@ -21,17 +20,14 @@ int copy_fd(int ifd, int ofd)\n \t\t\t\tlen -= written;\n \t\t\t}\n \t\t\telse if (!written) {\n-\t\t\t\tclose(ifd);\n \t\t\t\treturn error(\"copy-fd: write returned 0\");\n \t\t\t} else {\n \t\t\t\tint write_error = errno;\n-\t\t\t\tclose(ifd);\n \t\t\t\treturn error(\"copy-fd: write returned %s\",\n \t\t\t\t\t strerror(write_error));\n \t\t\t}\n \t\t}\n \t}\n-\tclose(ifd);\n \treturn 0;\n }\n \n@@ -60,6 +56,7 @@ int copy_file(const char *dst, const char *src, int mode)\n \t\treturn fdo;\n \t}\n \tstatus = copy_fd(fdi, fdo);\n+\tclose(fdi);\n \tif (close(fdo) != 0)\n \t\treturn error(\"%s: close error: %s\", dst, strerror(errno));\n \ndiff --git a/lockfile.c b/lockfile.c\nindex 2564a7f..2448d30 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -224,8 +224,11 @@ int hold_lock_file_for_append(struct lock_file *lk, const char *path, int flags)\n \t} else if (copy_fd(orig_fd, fd)) {\n \t\tif (flags & LOCK_DIE_ON_ERROR)\n \t\t\texit(128);\n+\t\tclose(orig_fd);\n \t\tclose(fd);\n \t\treturn -1;\n+\t} else {\n+\t\tclose(orig_fd);\n \t}\n \treturn fd;\n }\n-- \n2.1.0.8.gf3a29c8\n"},{"id":"248315","messageId":"1409066605-4851-7-git-send-email-prohaska@zib.de","threadId":"37417","inReplyTo":"1409066605-4851-1-git-send-email-prohaska@zib.de","subject":"[PATCH v6 6/6] convert: stream from fd to required clean filter to reduce used address space","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2014-08-26T15:23:25Z","receivedAt":"2014-08-26T15:23:25Z","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. If it fails, the original data is\nnot immediately available. The condition can easily be handled as\na fatal error, which is expected for a required filter anyway.\n\nIf the filter was not required, the condition would need to be handled\nin a different way, like seeking to 0 and reading the data. But this\nwould require more restructuring of the code and is probably not worth\nit. The obvious approach of falling back to reading all data would not\nhelp achieving the main purpose of this patch, which is to handle large\nfiles with limited address space. If reading all data is an option, we\ncan simply take the old code path right away and mmap the entire file.\n\nThe environment variable GIT_MMAP_LIMIT, which has been introduced in\na previous commit is used to test that the expected code path is taken.\nA related test that exercises required filters is modified to verify\nthat the data actually has been modified on its way from the file system\nto the object store.\n\nSigned-off-by: Steffen Prohaska \n---\n convert.c | 55 +++++++++++++++++++++++++++++++++++++++++++++------\n convert.h | 5 +++++\n sha1_file.c | 27 ++++++++++++++++++++++++-\n t/t0021-conversion.sh | 24 +++++++++++++++++-----\n 4 files changed, 99 insertions(+), 12 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex cb5fbb4..677d339 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@@ -355,7 +356,12 @@ 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\twrite_err = (write_in_full(child_process.in, params->src, params->size) < 0);\n+\t} else {\n+\t\twrite_err = copy_fd(params->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 +377,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 +398,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 +754,25 @@ 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\treturn 0;\n+\n+\t/*\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\treturn 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 +787,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 +804,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 +854,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 d9b5157..423ec64 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -3090,6 +3090,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@@ -3157,7 +3180,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..82ebbff 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@@ -189,6 +195,14 @@ test_expect_success 'required filter clean failure' '\n \techo test >test.fc &&\n \ttest_must_fail git add test.fc\n '\n+test_expect_success \\\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+\tGIT_MMAP_LIMIT=1m GIT_ALLOC_LIMIT=1m git add 30MB\n+'\n \n test_expect_success EXPENSIVE 'filter large file' '\n \tgit config filter.largefile.smudge cat &&\n-- \n2.1.0.8.gf3a29c8\n"},{"id":"248322","messageId":"xmqqd2bnjhx1.fsf@gitster.dls.corp.google.com","threadId":"37417","inReplyTo":"1409066605-4851-6-git-send-email-prohaska@zib.de","subject":"Re: [PATCH v6 5/6] Change copy_fd() to not close input fd","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-26T17:10:34Z","receivedAt":"2014-08-26T17:10:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Steffen Prohaska writes:\n\n> The caller opened the fd, so it should be responsible for closing it.\n\nHmmmm, I am not sure what the benefit of such a dogmatism. This\nfunction consumes all that is useful in the fd, and there is nothing\ninteresting that can be do further on it, no?\n\nAh, wait. The caller could choose to rewind and reuse the contents,\nand it is very selfish of this function to unilaterally declare that\nonce you give an fd to it you cannot do anything with it laster.\n\nSo I think this is a good change, but the justification is not\nquite. It is not \"the caller should be responsible\"; it is more\nabout \"the callee did not open it, does not own it, and should allow\nthe caller, if it chooses, reuse it by seeking after the callee is\ndone.\"\n\n>> Subject: Re: [PATCH v6 5/6] Change copy_fd() to not close input fd\n\nLet's follow the \": \" convention here, too, e.g.\n\n copy_fd(): allow callers to work on input fd after it returns\n\nor something.\n\nThanks.\n\n> copy.c | 5 +----\n> lockfile.c | 3 +++\n> 2 files changed, 4 insertions(+), 4 deletions(-)\n>\n> diff --git a/copy.c b/copy.c\n> index a7f58fd..d0a1d82 100644\n> --- a/copy.c\n> +++ b/copy.c\n> @@ -10,7 +10,6 @@ int copy_fd(int ifd, int ofd)\n> \t\t\tbreak;\n> \t\tif (len < 0) {\n> \t\t\tint read_error = errno;\n> -\t\t\tclose(ifd);\n> \t\t\treturn error(\"copy-fd: read returned %s\",\n> \t\t\t\t strerror(read_error));\n> \t\t}\n> @@ -21,17 +20,14 @@ int copy_fd(int ifd, int ofd)\n> \t\t\t\tlen -= written;\n> \t\t\t}\n> \t\t\telse if (!written) {\n> -\t\t\t\tclose(ifd);\n> \t\t\t\treturn error(\"copy-fd: write returned 0\");\n> \t\t\t} else {\n> \t\t\t\tint write_error = errno;\n> -\t\t\t\tclose(ifd);\n> \t\t\t\treturn error(\"copy-fd: write returned %s\",\n> \t\t\t\t\t strerror(write_error));\n> \t\t\t}\n> \t\t}\n> \t}\n> -\tclose(ifd);\n> \treturn 0;\n> }\n> \n> @@ -60,6 +56,7 @@ int copy_file(const char *dst, const char *src, int mode)\n> \t\treturn fdo;\n> \t}\n> \tstatus = copy_fd(fdi, fdo);\n> +\tclose(fdi);\n> \tif (close(fdo) != 0)\n> \t\treturn error(\"%s: close error: %s\", dst, strerror(errno));\n> \n> diff --git a/lockfile.c b/lockfile.c\n> index 2564a7f..2448d30 100644\n> --- a/lockfile.c\n> +++ b/lockfile.c\n> @@ -224,8 +224,11 @@ int hold_lock_file_for_append(struct lock_file *lk, const char *path, int flags)\n> \t} else if (copy_fd(orig_fd, fd)) {\n> \t\tif (flags & LOCK_DIE_ON_ERROR)\n> \t\t\texit(128);\n> +\t\tclose(orig_fd);\n> \t\tclose(fd);\n> \t\treturn -1;\n> +\t} else {\n> +\t\tclose(orig_fd);\n> \t}\n> \treturn fd;\n> }\n"},{"id":"248335","messageId":"20140826182125.GC17546@peff.net","threadId":"37417","inReplyTo":"1409066605-4851-3-git-send-email-prohaska@zib.de","subject":"Re: [PATCH v6 2/6] Add git_env_ulong() to parse environment variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-08-26T18:21:26Z","receivedAt":"2014-08-26T18:21:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 26, 2014 at 05:23:21PM +0200, Steffen Prohaska wrote:\n\n> +/*\n> + * Use default if environment variable is unset or empty string.\n> + */\n> +unsigned long git_env_ulong(const char *k, unsigned long val)\n> +{\n> +\tconst char *v = getenv(k);\n> +\tif (v && *v && !git_parse_ulong(v, &val))\n> +\t\tdie(\"failed to parse %s\", k);\n> +\treturn val;\n> +}\n\nI think the \"empty string\" behavior here is sensible. I notice that\ngit_env_bool is not so careful. I think we should probably do this\n(independent of your series):\n\n-- >8 --\nSubject: git_env_bool: treat empty string as \"not set\"\n\nIf an environment variable we treat as a boolean is not set,\nwe use some default value provided by the caller. If it is\nset but is the empty string, however, we treat it as\n\"false\". This can be rather confusing, as it is easy to set\nthe variable to the empty string in the shell (e.g., by\ncalling GIT_SMART_HTTP= instead of \"unset\").\n\nInstead, let's treat unset and empty variables the same.\nThis should not have any negative backwards-compatibility\nconsequences, because:\n\n 1. The existing behavior was confusing and undocumented in\n the first place.\n\n 2. For most variables, the default _is_ false, and so this\n change is a noop. The only affected variables are\n GIT_IMPLICIT_WORK_TREE (which is undocumented and\n internally always set to \"0\" or \"1\") and GIT_SMART_HTTP\n (which is also undocumented, and we use only for\n testing).\n\nSince there won't be any fallout with the current variables,\nthis is a good time to make the switch (before any other\nvariables are added).\n\nSigned-off-by: Jeff King \n---\n config.c | 2 +-\n t/t5551-http-fetch-smart.sh | 7 +++++++\n 2 files changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/config.c b/config.c\nindex 058505c..7bf0704 100644\n--- a/config.c\n+++ b/config.c\n@@ -1119,7 +1119,7 @@ const char *git_etc_gitconfig(void)\n int git_env_bool(const char *k, int def)\n {\n \tconst char *v = getenv(k);\n-\treturn v ? git_config_bool(k, v) : def;\n+\treturn v && *v ? git_config_bool(k, v) : def;\n }\n \n int git_config_system(void)\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex 6cbc12d..831f9e4 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -168,6 +168,13 @@ test_expect_success 'GIT_SMART_HTTP can disable smart http' '\n \t test_must_fail git fetch)\n '\n \n+test_expect_success 'empty GIT_SMART_HTTP leaves smart http enabled' '\n+\t(GIT_SMART_HTTP= &&\n+\t export GIT_SMART_HTTP &&\n+\t cd clone &&\n+\t git fetch)\n+'\n+\n test_expect_success 'invalid Content-Type rejected' '\n \ttest_must_fail git clone $HTTPD_URL/broken_smart/repo.git 2>actual\n \tgrep \"not valid:\" actual\n-- \n2.1.0.346.ga0367b9\n"},{"id":"248336","messageId":"20140826182905.GD17546@peff.net","threadId":"37417","inReplyTo":"1409066605-4851-6-git-send-email-prohaska@zib.de","subject":"Re: [PATCH v6 5/6] Change copy_fd() to not close input fd","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-08-26T18:29:06Z","receivedAt":"2014-08-26T18:29:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 26, 2014 at 05:23:24PM +0200, Steffen Prohaska wrote:\n\n> The caller opened the fd, so it should be responsible for closing it.\n> \n> Signed-off-by: Steffen Prohaska \n> ---\n> copy.c | 5 +----\n> lockfile.c | 3 +++\n> 2 files changed, 4 insertions(+), 4 deletions(-)\n> \n> diff --git a/copy.c b/copy.c\n> index a7f58fd..d0a1d82 100644\n> --- a/copy.c\n> +++ b/copy.c\n> @@ -10,7 +10,6 @@ int copy_fd(int ifd, int ofd)\n> \t\t\tbreak;\n> \t\tif (len < 0) {\n> \t\t\tint read_error = errno;\n> -\t\t\tclose(ifd);\n> \t\t\treturn error(\"copy-fd: read returned %s\",\n> \t\t\t\t strerror(read_error));\n> \t\t}\n\nThis saved errno is not necessary anymore (the problem was that close()\nclobbered the error in the original code). It can go away, and we can\neven drop the curly braces.\n\n> @@ -21,17 +20,14 @@ int copy_fd(int ifd, int ofd)\n> \t\t\t\tlen -= written;\n> \t\t\t}\n> \t\t\telse if (!written) {\n> -\t\t\t\tclose(ifd);\n> \t\t\t\treturn error(\"copy-fd: write returned 0\");\n> \t\t\t} else {\n> \t\t\t\tint write_error = errno;\n> -\t\t\t\tclose(ifd);\n> \t\t\t\treturn error(\"copy-fd: write returned %s\",\n> \t\t\t\t\t strerror(write_error));\n> \t\t\t}\n> \t\t}\n\nDitto here. Actually, isn't this whole write just a reimplementation of\nwrite_in_full? The latter treats a return of 0 as ENOSPC rather than\nusing a custom message, but I think that is sane.\n\nAll together:\n\n---\n copy.c | 28 +++++-----------------------\n 1 file changed, 5 insertions(+), 23 deletions(-)\n\ndiff --git a/copy.c b/copy.c\nindex a7f58fd..53a9ece 100644\n--- a/copy.c\n+++ b/copy.c\n@@ -4,34 +4,16 @@ int copy_fd(int ifd, int ofd)\n {\n \twhile (1) {\n \t\tchar buffer[8192];\n-\t\tchar *buf = buffer;\n \t\tssize_t len = xread(ifd, buffer, sizeof(buffer));\n \t\tif (!len)\n \t\t\tbreak;\n-\t\tif (len < 0) {\n-\t\t\tint read_error = errno;\n-\t\t\tclose(ifd);\n+\t\tif (len < 0)\n \t\t\treturn error(\"copy-fd: read returned %s\",\n-\t\t\t\t strerror(read_error));\n-\t\t}\n-\t\twhile (len) {\n-\t\t\tint written = xwrite(ofd, buf, len);\n-\t\t\tif (written > 0) {\n-\t\t\t\tbuf += written;\n-\t\t\t\tlen -= written;\n-\t\t\t}\n-\t\t\telse if (!written) {\n-\t\t\t\tclose(ifd);\n-\t\t\t\treturn error(\"copy-fd: write returned 0\");\n-\t\t\t} else {\n-\t\t\t\tint write_error = errno;\n-\t\t\t\tclose(ifd);\n-\t\t\t\treturn error(\"copy-fd: write returned %s\",\n-\t\t\t\t\t strerror(write_error));\n-\t\t\t}\n-\t\t}\n+\t\t\t\t strerror(errno));\n+\t\tif (write_in_full(ofd, buffer, len) < 0)\n+\t\t\treturn error(\"copy-fd: write returned %s\",\n+\t\t\t\t strerror(errno));\n \t}\n-\tclose(ifd);\n \treturn 0;\n }\n \n"},{"id":"248342","messageId":"xmqq38cjhuje.fsf@gitster.dls.corp.google.com","threadId":"37417","inReplyTo":"20140826182125.GC17546@peff.net","subject":"Re: [PATCH v6 2/6] Add git_env_ulong() to parse environment variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-26T20:20:53Z","receivedAt":"2014-08-26T20:20:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Aug 26, 2014 at 05:23:21PM +0200, Steffen Prohaska wrote:\n>\n>> +/*\n>> + * Use default if environment variable is unset or empty string.\n>> + */\n>> +unsigned long git_env_ulong(const char *k, unsigned long val)\n>> +{\n>> +\tconst char *v = getenv(k);\n>> +\tif (v && *v && !git_parse_ulong(v, &val))\n>> +\t\tdie(\"failed to parse %s\", k);\n>> +\treturn val;\n>> +}\n>\n> I think the \"empty string\" behavior here is sensible. I notice that\n> git_env_bool is not so careful. I think we should probably do this\n> (independent of your series):\n>\n> -- >8 --\n> Subject: git_env_bool: treat empty string as \"not set\"\n>\n> If an environment variable we treat as a boolean is not set,\n> we use some default value provided by the caller. If it is\n> set but is the empty string, however, we treat it as\n> \"false\". This can be rather confusing, as it is easy to set\n> the variable to the empty string in the shell (e.g., by\n> calling GIT_SMART_HTTP= instead of \"unset\").\n\nI think different people have different confusion criteria.\nTo me, these two are very different operations:\n\n $ VAR=\n $ unset VAR\n\nI think it boils down to that I see that the distance between \"unset\nvs set to empty\" is far larger than the distance between \"empty vs\nfalse\". You probably see these two distances the other way,\ni.e. \"set to empty is almost like unset\" and \"empty is not a valid\nway to say false\".\n\nDue to this difference, the new test confused me and had me read it\nthree times.\n\n> +test_expect_success 'empty GIT_SMART_HTTP leaves smart http enabled' '\n> +\t(GIT_SMART_HTTP= &&\n> +\t export GIT_SMART_HTTP &&\n> +\t cd clone &&\n> +\t git fetch)\n> +'\n\nThe test before this one explicitly sets GIT_SMART_HTTP to \"0\" and\nexpects the fetch to fail. It is sensible to you because \"0\" is a\nlot more explicit \"false\" than an empty string to you, and you\nequate an empty and unset, hence the new one should succeed.\n\nBut it looks nonsensical to me that the new one expects to succeed,\nbecause \"0\" and an empty string are both valid way to say \"false\"\nto me, and it should behave the same way as the \"0\" one.\n\nI view the *v check before git_parse_ulong() being unnecessarily\ndefensive to avoid triggering \"die()\". An empty string is obviously\nnot a number (somebody could argue that it is the same as zero,\nthough), but nevertheless the user _is_ telling us to use that value\nby setting and exporting the variable. If we cannot parse it,\nbarfing is what the user would appreciate.\n\nSo, I am not sure the patch in the message I am responding to, and I\nam not sure about that *v check in Steffen's patch, either.\n"},{"id":"248344","messageId":"20140826203158.GA30651@peff.net","threadId":"37417","inReplyTo":"xmqq38cjhuje.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v6 2/6] Add git_env_ulong() to parse environment variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-08-26T20:31:59Z","receivedAt":"2014-08-26T20:31:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 26, 2014 at 01:20:53PM -0700, Junio C Hamano wrote:\n\n> I think different people have different confusion criteria.\n> To me, these two are very different operations:\n> \n> $ VAR=\n> $ unset VAR\n> \n> I think it boils down to that I see that the distance between \"unset\n> vs set to empty\" is far larger than the distance between \"empty vs\n> false\". You probably see these two distances the other way,\n> i.e. \"set to empty is almost like unset\" and \"empty is not a valid\n> way to say false\".\n> \n> Due to this difference, the new test confused me and had me read it\n> three times.\n\nI agree that it is rather a subjective decision.\n\n> So, I am not sure the patch in the message I am responding to, and I\n> am not sure about that *v check in Steffen's patch, either.\n\nIf it is truly \"some people prefer it one way and some the other\", I am\nnot sure if we should leave it as-is (that is preferring one way). The\nmiddle ground would be to die(). That does not seem super-friendly, but\nthen we would also die with GIT_SMART_HTTP=foobar, so perhaps it is not\nunreasonable to just consider it a syntax error.\n\nI dunno. I can live with leaving it as-is. Certainly the existing\nbehavior is not what I expected, but it is not like it came up in the\nreal world (and I would not expect it to do so often). And it is\nconsistent with the config, which treats:\n\n [foo]\n bar =\n\nas boolean false. That _also_ seems weird to me, but that is not\nsomething I think we can easily change or outlaw at this point anyway.\n\n-Peff\n"},{"id":"248350","messageId":"xmqq38cihq7w.fsf@gitster.dls.corp.google.com","threadId":"37417","inReplyTo":"20140826203158.GA30651@peff.net","subject":"Re: [PATCH v6 2/6] Add git_env_ulong() to parse environment variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-26T21:54:11Z","receivedAt":"2014-08-26T21:54:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> If it is truly \"some people prefer it one way and some the other\", I am\n> not sure if we should leave it as-is (that is preferring one way). \n\nA worse position is to have git_env_bool() that says \"empty is\nfalse\" and add a new git_env_ulong() that says \"empty is unset\".\n\nWe should pick one or the other and use it for both.\n\nAs you pointed out in the later part of the message, an empty string\nis a valid way to spell \"false\" in the config subsystem since the\nbeginning at 17712991 (Add \".git/config\" file parser, 2005-10-10);\nconsistency with it probably is sensible.\n\nBut perhaps my brain is rotten with too much Perl and Python X-<.\nI do not know where Linus picked up that, though ;-)\n\n> The middle ground would be to die(). That does not seem super-friendly, but\n> then we would also die with GIT_SMART_HTTP=foobar, so perhaps it is not\n> unreasonable to just consider it a syntax error.\n\nHmm, I am not sure if dying is better. Unless we decide to make\nempty string no longer false everywhere and warn now and then later\ndie as part of a 3.0 transition plan or something, that is.\n"},{"id":"248380","messageId":"20140827044621.GA32141@peff.net","threadId":"37417","inReplyTo":"xmqq38cihq7w.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v6 2/6] Add git_env_ulong() to parse environment variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-08-27T04:46:21Z","receivedAt":"2014-08-27T04:46:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 26, 2014 at 02:54:11PM -0700, Junio C Hamano wrote:\n\n> A worse position is to have git_env_bool() that says \"empty is\n> false\" and add a new git_env_ulong() that says \"empty is unset\".\n> \n> We should pick one or the other and use it for both.\n\nYeah, I agree they should probably behave the same.\n\n> > The middle ground would be to die(). That does not seem super-friendly, but\n> > then we would also die with GIT_SMART_HTTP=foobar, so perhaps it is not\n> > unreasonable to just consider it a syntax error.\n> \n> Hmm, I am not sure if dying is better. Unless we decide to make\n> empty string no longer false everywhere and warn now and then later\n> die as part of a 3.0 transition plan or something, that is.\n\nI think it is better in the sense that while it may be unexpected, it\ndoes not unexpectedly do something that the user cannot easily undo.\n\nI really do not think this topic is worth the effort of a long-term\ndeprecation scheme (which I agree _is_ required for a change to the\nconfig behavior). Let's just leave it as-is. We've seen zero real-world\ncomplaints, only my own surprise after reading the code (and Steffen's\npatch should be tweaked to match).\n\n-Peff\n"},{"id":"248403","messageId":"xmqqtx4yf0r2.fsf@gitster.dls.corp.google.com","threadId":"37417","inReplyTo":"20140827044621.GA32141@peff.net","subject":"Re: [PATCH v6 2/6] Add git_env_ulong() to parse environment variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-27T14:47:13Z","receivedAt":"2014-08-27T14:47:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Aug 26, 2014 at 02:54:11PM -0700, Junio C Hamano wrote:\n>\n>> A worse position is to have git_env_bool() that says \"empty is\n>> false\" and add a new git_env_ulong() that says \"empty is unset\".\n>> \n>> We should pick one or the other and use it for both.\n>\n> Yeah, I agree they should probably behave the same.\n>\n>> > The middle ground would be to die(). That does not seem super-friendly, but\n>> > then we would also die with GIT_SMART_HTTP=foobar, so perhaps it is not\n>> > unreasonable to just consider it a syntax error.\n>> \n>> Hmm, I am not sure if dying is better. Unless we decide to make\n>> empty string no longer false everywhere and warn now and then later\n>> die as part of a 3.0 transition plan or something, that is.\n>\n> I think it is better in the sense that while it may be unexpected, it\n> does not unexpectedly do something that the user cannot easily undo.\n>\n> I really do not think this topic is worth the effort of a long-term\n> deprecation scheme (which I agree _is_ required for a change to the\n> config behavior). Let's just leave it as-is. We've seen zero real-world\n> complaints, only my own surprise after reading the code (and Steffen's\n> patch should be tweaked to match).\n\nOK, then let's do that at least for now and move on.\n"},{"id":"248494","messageId":"7AD881F3-DDD0-4094-90F3-C3E2F81DF664@zib.de","threadId":"37417","inReplyTo":"xmqqtx4yf0r2.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v6 2/6] Add git_env_ulong() to parse environment variable","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2014-08-28T15:21:31Z","receivedAt":"2014-08-28T15:21:31Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"\nOn Aug 27, 2014, at 4:47 PM, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n>> On Tue, Aug 26, 2014 at 02:54:11PM -0700, Junio C Hamano wrote:\n>> \n>>> A worse position is to have git_env_bool() that says \"empty is\n>>> false\" and add a new git_env_ulong() that says \"empty is unset\".\n>>> \n>>> We should pick one or the other and use it for both.\n>> \n>> Yeah, I agree they should probably behave the same.\n>> \n>>>> The middle ground would be to die(). That does not seem super-friendly, but\n>>>> then we would also die with GIT_SMART_HTTP=foobar, so perhaps it is not\n>>>> unreasonable to just consider it a syntax error.\n>>> \n>>> Hmm, I am not sure if dying is better. Unless we decide to make\n>>> empty string no longer false everywhere and warn now and then later\n>>> die as part of a 3.0 transition plan or something, that is.\n>> \n>> I think it is better in the sense that while it may be unexpected, it\n>> does not unexpectedly do something that the user cannot easily undo.\n>> \n>> I really do not think this topic is worth the effort of a long-term\n>> deprecation scheme (which I agree _is_ required for a change to the\n>> config behavior). Let's just leave it as-is. We've seen zero real-world\n>> complaints, only my own surprise after reading the code (and Steffen's\n>> patch should be tweaked to match).\n> \n> OK, then let's do that at least for now and move on.\n\nOk. I saw that you tweaked my patch on pu. Maybe remove the outdated\ncomment above the function completely:\n\ndiff --git a/config.c b/config.c\nindex 87db755..010bcd0 100644\n--- a/config.c\n+++ b/config.c\n@@ -1122,9 +1122,6 @@ int git_env_bool(const char *k, int def)\n return v ? git_config_bool(k, v) : def;\n }\n\n-/*\n- * Use default if environment variable is unset or empty string.\n- */\n unsigned long git_env_ulong(const char *k, unsigned long val)\n {\n const char *v = getenv(k);\n\n\tSteffen"},{"id":"248496","messageId":"3947B7A7-98D0-4313-B7D7-D5EB35427E56@zib.de","threadId":"37417","inReplyTo":"20140826182905.GD17546@peff.net","subject":"Re: [PATCH v6 5/6] Change copy_fd() to not close input fd","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2014-08-28T15:37:41Z","receivedAt":"2014-08-28T15:37:41Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"\nOn Aug 26, 2014, at 8:29 PM, Jeff King wrote:\n\n> On Tue, Aug 26, 2014 at 05:23:24PM +0200, Steffen Prohaska wrote:\n> \n>> The caller opened the fd, so it should be responsible for closing it.\n>> \n>> Signed-off-by: Steffen Prohaska \n>> ---\n>> copy.c | 5 +----\n>> lockfile.c | 3 +++\n>> 2 files changed, 4 insertions(+), 4 deletions(-)\n>> \n>> diff --git a/copy.c b/copy.c\n>> index a7f58fd..d0a1d82 100644\n>> --- a/copy.c\n>> +++ b/copy.c\n>> @@ -10,7 +10,6 @@ int copy_fd(int ifd, int ofd)\n>> \t\t\tbreak;\n>> \t\tif (len < 0) {\n>> \t\t\tint read_error = errno;\n>> -\t\t\tclose(ifd);\n>> \t\t\treturn error(\"copy-fd: read returned %s\",\n>> \t\t\t\t strerror(read_error));\n>> \t\t}\n> \n> This saved errno is not necessary anymore (the problem was that close()\n> clobbered the error in the original code). It can go away, and we can\n> even drop the curly braces.\n> \n>> @@ -21,17 +20,14 @@ int copy_fd(int ifd, int ofd)\n>> \t\t\t\tlen -= written;\n>> \t\t\t}\n>> \t\t\telse if (!written) {\n>> -\t\t\t\tclose(ifd);\n>> \t\t\t\treturn error(\"copy-fd: write returned 0\");\n>> \t\t\t} else {\n>> \t\t\t\tint write_error = errno;\n>> -\t\t\t\tclose(ifd);\n>> \t\t\t\treturn error(\"copy-fd: write returned %s\",\n>> \t\t\t\t\t strerror(write_error));\n>> \t\t\t}\n>> \t\t}\n> \n> Ditto here. Actually, isn't this whole write just a reimplementation of\n> write_in_full? The latter treats a return of 0 as ENOSPC rather than\n> using a custom message, but I think that is sane.\n> \n> All together:\n\nMakes all sense, and seems sane to me, too.\n\nJunio, I saw that you have the changes on pu with 'SQUASH???...'. Will you\nsquash it, or shall I send another complete update of the patch series?\n\n\tSteffen\n\n\n\n> ---\n> copy.c | 28 +++++-----------------------\n> 1 file changed, 5 insertions(+), 23 deletions(-)\n> \n> diff --git a/copy.c b/copy.c\n> index a7f58fd..53a9ece 100644\n> --- a/copy.c\n> +++ b/copy.c\n> @@ -4,34 +4,16 @@ int copy_fd(int ifd, int ofd)\n> {\n> \twhile (1) {\n> \t\tchar buffer[8192];\n> -\t\tchar *buf = buffer;\n> \t\tssize_t len = xread(ifd, buffer, sizeof(buffer));\n> \t\tif (!len)\n> \t\t\tbreak;\n> -\t\tif (len < 0) {\n> -\t\t\tint read_error = errno;\n> -\t\t\tclose(ifd);\n> +\t\tif (len < 0)\n> \t\t\treturn error(\"copy-fd: read returned %s\",\n> -\t\t\t\t strerror(read_error));\n> -\t\t}\n> -\t\twhile (len) {\n> -\t\t\tint written = xwrite(ofd, buf, len);\n> -\t\t\tif (written > 0) {\n> -\t\t\t\tbuf += written;\n> -\t\t\t\tlen -= written;\n> -\t\t\t}\n> -\t\t\telse if (!written) {\n> -\t\t\t\tclose(ifd);\n> -\t\t\t\treturn error(\"copy-fd: write returned 0\");\n> -\t\t\t} else {\n> -\t\t\t\tint write_error = errno;\n> -\t\t\t\tclose(ifd);\n> -\t\t\t\treturn error(\"copy-fd: write returned %s\",\n> -\t\t\t\t\t strerror(write_error));\n> -\t\t\t}\n> -\t\t}\n> +\t\t\t\t strerror(errno));\n> +\t\tif (write_in_full(ofd, buffer, len) < 0)\n> +\t\t\treturn error(\"copy-fd: write returned %s\",\n> +\t\t\t\t strerror(errno));\n> \t}\n> -\tclose(ifd);\n> \treturn 0;\n> }\n> \n"},{"id":"248502","messageId":"xmqqk35sbkra.fsf@gitster.dls.corp.google.com","threadId":"37417","inReplyTo":"7AD881F3-DDD0-4094-90F3-C3E2F81DF664@zib.de","subject":"Re: [PATCH v6 2/6] Add git_env_ulong() to parse environment variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-28T17:13:13Z","receivedAt":"2014-08-28T17:13:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Steffen Prohaska writes:\n\n>> OK, then let's do that at least for now and move on.\n>\n> Ok. I saw that you tweaked my patch on pu. Maybe remove the outdated\n> comment above the function completely:\n>\n> diff --git a/config.c b/config.c\n> index 87db755..010bcd0 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -1122,9 +1122,6 @@ int git_env_bool(const char *k, int def)\n> return v ? git_config_bool(k, v) : def;\n> }\n>\n> -/*\n> - * Use default if environment variable is unset or empty string.\n> - */\n\nThanks, will do.\n\n> unsigned long git_env_ulong(const char *k, unsigned long val)\n> {\n> const char *v = getenv(k);\n>\n> \tSteffen--\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"},{"id":"248503","messageId":"xmqqegw0bknq.fsf@gitster.dls.corp.google.com","threadId":"37417","inReplyTo":"3947B7A7-98D0-4313-B7D7-D5EB35427E56@zib.de","subject":"Re: [PATCH v6 5/6] Change copy_fd() to not close input fd","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-28T17:15:21Z","receivedAt":"2014-08-28T17:15:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Steffen Prohaska writes:\n\n> On Aug 26, 2014, at 8:29 PM, Jeff King wrote:\n> ...\n> Makes all sense, and seems sane to me, too.\n>\n> Junio, I saw that you have the changes on pu with 'SQUASH???...'. Will you\n> squash it, or shall I send another complete update of the patch series?\n\nIf I let you reroll, I will risk that you will forget to propagate\nthe log message fixes I did here while queuing the v6 iteration in\nit, so let me try doing the squashing and push out the result first.\n\nThanks.\n"}]}