{"thread":{"id":"44366","subject":"[PATCH v2 0/2] Use CLOEXEC to avoid fd leaks","startedAt":"2016-10-24T18:03:16Z","lastAt":"2016-10-31T18:05:31Z","messageCount":54,"participants":["larsxschneider@gmail.com","Junio C Hamano","Eric Wong","Johannes Sixt","Lars Schneider","Johannes Schindelin","Jeff King","Linus Torvalds"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"304782","messageId":"20161024180300.52359-1-larsxschneider@gmail.com","threadId":"44366","inReplyTo":null,"subject":"[PATCH v2 0/2] Use CLOEXEC to avoid fd leaks","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-10-24T18:02:58Z","receivedAt":"2016-10-24T18:03:16Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider \n\nUse the CLOEXEC flag similar to 05d1ed61 to fix leaked file descriptors.\n\nThis mini patch series is necessary to make the \"ls/filter-process\" topic\nwork properly for Windows. Right now the topic generates test failures\non Windows as reported by Dscho:\nhttp://public-inbox.org/git/alpine.DEB.2.20.1610211457030.3264@virtualbox/\n\nPatch 1/2 was contemplated by Junio here:\nhttp://public-inbox.org/git/xmqq37lnbbpk.fsf@gitster.mtv.corp.google.com/\n\nThanks to\n Eric, Jakub, Dscho, and Junio for the review of v1,\nLars\n\n\n\n## Changes since v1\n * add fallbacks in case O_CLOEXEC is not available\n * rename 'git_open_noatime_cloexec' to 'git_open' (Eric, Dscho)\n * rebased the topic on `next` to fix a merge conflict\n\n\nLars Schneider (2):\n sha1_file: open window into packfiles with CLOEXEC\n read-cache: make sure file handles are not inherited by child\n processes\n\n builtin/pack-objects.c | 2 +-\n cache.h | 2 +-\n pack-bitmap.c | 2 +-\n read-cache.c | 6 +++++-\n sha1_file.c | 26 ++++++++++++++++----------\n 5 files changed, 24 insertions(+), 14 deletions(-)\n\n\n\n## Interdiff (v1..v2)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 0fdd331..0fd52bd 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -720,7 +720,7 @@ static off_t write_reused_pack(struct sha1file *f)\n \tif (!is_pack_valid(reuse_packfile))\n \t\tdie(\"packfile is invalid: %s\", reuse_packfile->pack_name);\n\n-\tfd = git_open_noatime_cloexec(reuse_packfile->pack_name);\n+\tfd = git_open(reuse_packfile->pack_name);\n \tif (fd < 0)\n \t\tdie_errno(\"unable to open packfile for reuse: %s\",\n \t\t\t reuse_packfile->pack_name);\ndiff --git a/cache.h b/cache.h\nindex 0dea548..419b7a0 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1125,7 +1125,7 @@ extern int write_sha1_file(const void *buf, unsigned long len, const char *type,\n extern int hash_sha1_file_literally(const void *buf, unsigned long len, const char *type, unsigned char *sha1, unsigned flags);\n extern int pretend_sha1_file(void *, unsigned long, enum object_type, unsigned char *);\n extern int force_object_loose(const unsigned char *sha1, time_t mtime);\n-extern int git_open_noatime_cloexec(const char *name);\n+extern int git_open(const char *name);\n extern void *map_sha1_file(const unsigned char *sha1, unsigned long *size);\n extern int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long mapsize, void *buffer, unsigned long bufsiz);\n extern int parse_sha1_header(const char *hdr, unsigned long *sizep);\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 1b39e5d..39bcc16 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -266,7 +266,7 @@ static int open_pack_bitmap_1(struct packed_git *packfile)\n \t\treturn -1;\n\n \tidx_name = pack_bitmap_filename(packfile);\n-\tfd = git_open_noatime_cloexec(idx_name);\n+\tfd = git_open(idx_name);\n \tfree(idx_name);\n\n \tif (fd < 0)\ndiff --git a/read-cache.c b/read-cache.c\nindex 200d4fa..b594865 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -158,6 +158,10 @@ static int ce_compare_data(const struct cache_entry *ce, struct stat *st)\n \tint match = -1;\n \tint fd = open(ce->name, O_RDONLY | O_CLOEXEC);\n\n+\tif (O_CLOEXEC && fd < 0 && errno == EINVAL)\n+\t\t/* Try again w/o O_CLOEXEC: the kernel might not support it */\n+\t\tfd = open(ce->name, O_RDONLY);\n+\n \tif (fd >= 0) {\n \t\tunsigned char sha1[20];\n \t\tif (!index_fd(sha1, fd, st, OBJ_BLOB, ce->name, 0))\ndiff --git a/sha1_file.c b/sha1_file.c\nindex dbe027b..93b836b 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -370,7 +370,7 @@ void read_info_alternates(const char * relative_base, int depth)\n \tint fd;\n\n \tpath = xstrfmt(\"%s/info/alternates\", relative_base);\n-\tfd = git_open_noatime_cloexec(path);\n+\tfd = git_open(path);\n \tfree(path);\n \tif (fd < 0)\n \t\treturn;\n@@ -663,7 +663,7 @@ static int check_packed_git_idx(const char *path, struct packed_git *p)\n \tstruct pack_idx_header *hdr;\n \tsize_t idx_size;\n \tuint32_t version, nr, i, *index;\n-\tint fd = git_open_noatime_cloexec(path);\n+\tint fd = git_open(path);\n \tstruct stat st;\n\n \tif (fd < 0)\n@@ -1069,7 +1069,7 @@ static int open_packed_git_1(struct packed_git *p)\n \twhile (pack_max_fds <= pack_open_fds && close_one_pack())\n \t\t; /* nothing */\n\n-\tp->pack_fd = git_open_noatime_cloexec(p->pack_name);\n+\tp->pack_fd = git_open(p->pack_name);\n \tif (p->pack_fd < 0 || fstat(p->pack_fd, &st))\n \t\treturn -1;\n \tpack_open_fds++;\n@@ -1586,7 +1586,7 @@ int check_sha1_signature(const unsigned char *sha1, void *map,\n \treturn hashcmp(sha1, real_sha1) ? -1 : 0;\n }\n\n-int git_open_noatime_cloexec(const char *name)\n+int git_open(const char *name)\n {\n \tstatic int sha1_file_open_flag = O_NOATIME | O_CLOEXEC;\n\n@@ -1598,12 +1598,18 @@ int git_open_noatime_cloexec(const char *name)\n \t\tif (fd >= 0)\n \t\t\treturn fd;\n\n-\t\t/* Might the failure be due to O_NOATIME? */\n-\t\tif (errno != ENOENT && sha1_file_open_flag) {\n-\t\t\tsha1_file_open_flag = 0;\n+\t\t/* Try again w/o O_CLOEXEC: the kernel might not support it */\n+\t\tif (O_CLOEXEC && errno == EINVAL &&\n+\t\t\t(sha1_file_open_flag & O_CLOEXEC)) {\n+\t\t\tsha1_file_open_flag &= ~O_CLOEXEC;\n \t\t\tcontinue;\n \t\t}\n\n+\t\t/* Might the failure be due to O_NOATIME? */\n+\t\tif (errno != ENOENT && (sha1_file_open_flag & O_NOATIME)) {\n+\t\t\tsha1_file_open_flag &= ~O_NOATIME;\n+\t\t\tcontinue;\n+\t\t}\n \t\treturn -1;\n \t}\n }\n@@ -1632,7 +1638,7 @@ static int open_sha1_file(const unsigned char *sha1)\n \tstruct alternate_object_database *alt;\n \tint most_interesting_errno;\n\n-\tfd = git_open_noatime_cloexec(sha1_file_name(sha1));\n+\tfd = git_open(sha1_file_name(sha1));\n \tif (fd >= 0)\n \t\treturn fd;\n \tmost_interesting_errno = errno;\n@@ -1640,7 +1646,7 @@ static int open_sha1_file(const unsigned char *sha1)\n \tprepare_alt_odb();\n \tfor (alt = alt_odb_list; alt; alt = alt->next) {\n \t\tconst char *path = alt_sha1_path(alt, sha1);\n-\t\tfd = git_open_noatime_cloexec(path);\n+\t\tfd = git_open(path);\n \t\tif (fd >= 0)\n \t\t\treturn fd;\n \t\tif (most_interesting_errno == ENOENT)\n\n--\n2.10.0\n\n"},{"id":"304783","messageId":"20161024180300.52359-2-larsxschneider@gmail.com","threadId":"44366","inReplyTo":"20161024180300.52359-1-larsxschneider@gmail.com","subject":"[PATCH v2 1/2] sha1_file: open window into packfiles with CLOEXEC","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-10-24T18:02:59Z","receivedAt":"2016-10-24T18:03:20Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider \n\nAll processes that the Git main process spawns inherit the open file\ndescriptors of the main process. These leaked file descriptors can\ncause problems.\n\nUse the CLOEXEC flag similar to 05d1ed61 to fix the leaked file\ndescriptors. Since `git_open_noatime` does not describe the function\nproperly anymore rename it to `git_open`.\n\nSigned-off-by: Lars Schneider \n---\n builtin/pack-objects.c | 2 +-\n cache.h | 2 +-\n pack-bitmap.c | 2 +-\n sha1_file.c | 26 ++++++++++++++++----------\n 4 files changed, 19 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 1e7c2a9..0fd52bd 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -720,7 +720,7 @@ static off_t write_reused_pack(struct sha1file *f)\n \tif (!is_pack_valid(reuse_packfile))\n \t\tdie(\"packfile is invalid: %s\", reuse_packfile->pack_name);\n \n-\tfd = git_open_noatime(reuse_packfile->pack_name);\n+\tfd = git_open(reuse_packfile->pack_name);\n \tif (fd < 0)\n \t\tdie_errno(\"unable to open packfile for reuse: %s\",\n \t\t\t reuse_packfile->pack_name);\ndiff --git a/cache.h b/cache.h\nindex b7f34b4..419b7a0 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1125,7 +1125,7 @@ extern int write_sha1_file(const void *buf, unsigned long len, const char *type,\n extern int hash_sha1_file_literally(const void *buf, unsigned long len, const char *type, unsigned char *sha1, unsigned flags);\n extern int pretend_sha1_file(void *, unsigned long, enum object_type, unsigned char *);\n extern int force_object_loose(const unsigned char *sha1, time_t mtime);\n-extern int git_open_noatime(const char *name);\n+extern int git_open(const char *name);\n extern void *map_sha1_file(const unsigned char *sha1, unsigned long *size);\n extern int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long mapsize, void *buffer, unsigned long bufsiz);\n extern int parse_sha1_header(const char *hdr, unsigned long *sizep);\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex b949e51..39bcc16 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -266,7 +266,7 @@ static int open_pack_bitmap_1(struct packed_git *packfile)\n \t\treturn -1;\n \n \tidx_name = pack_bitmap_filename(packfile);\n-\tfd = git_open_noatime(idx_name);\n+\tfd = git_open(idx_name);\n \tfree(idx_name);\n \n \tif (fd < 0)\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 1e41954..93b836b 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -370,7 +370,7 @@ void read_info_alternates(const char * relative_base, int depth)\n \tint fd;\n \n \tpath = xstrfmt(\"%s/info/alternates\", relative_base);\n-\tfd = git_open_noatime(path);\n+\tfd = git_open(path);\n \tfree(path);\n \tif (fd < 0)\n \t\treturn;\n@@ -663,7 +663,7 @@ static int check_packed_git_idx(const char *path, struct packed_git *p)\n \tstruct pack_idx_header *hdr;\n \tsize_t idx_size;\n \tuint32_t version, nr, i, *index;\n-\tint fd = git_open_noatime(path);\n+\tint fd = git_open(path);\n \tstruct stat st;\n \n \tif (fd < 0)\n@@ -1069,7 +1069,7 @@ static int open_packed_git_1(struct packed_git *p)\n \twhile (pack_max_fds <= pack_open_fds && close_one_pack())\n \t\t; /* nothing */\n \n-\tp->pack_fd = git_open_noatime(p->pack_name);\n+\tp->pack_fd = git_open(p->pack_name);\n \tif (p->pack_fd < 0 || fstat(p->pack_fd, &st))\n \t\treturn -1;\n \tpack_open_fds++;\n@@ -1586,9 +1586,9 @@ int check_sha1_signature(const unsigned char *sha1, void *map,\n \treturn hashcmp(sha1, real_sha1) ? -1 : 0;\n }\n \n-int git_open_noatime(const char *name)\n+int git_open(const char *name)\n {\n-\tstatic int sha1_file_open_flag = O_NOATIME;\n+\tstatic int sha1_file_open_flag = O_NOATIME | O_CLOEXEC;\n \n \tfor (;;) {\n \t\tint fd;\n@@ -1598,12 +1598,18 @@ int git_open_noatime(const char *name)\n \t\tif (fd >= 0)\n \t\t\treturn fd;\n \n-\t\t/* Might the failure be due to O_NOATIME? */\n-\t\tif (errno != ENOENT && sha1_file_open_flag) {\n-\t\t\tsha1_file_open_flag = 0;\n+\t\t/* Try again w/o O_CLOEXEC: the kernel might not support it */\n+\t\tif (O_CLOEXEC && errno == EINVAL &&\n+\t\t\t(sha1_file_open_flag & O_CLOEXEC)) {\n+\t\t\tsha1_file_open_flag &= ~O_CLOEXEC;\n \t\t\tcontinue;\n \t\t}\n \n+\t\t/* Might the failure be due to O_NOATIME? */\n+\t\tif (errno != ENOENT && (sha1_file_open_flag & O_NOATIME)) {\n+\t\t\tsha1_file_open_flag &= ~O_NOATIME;\n+\t\t\tcontinue;\n+\t\t}\n \t\treturn -1;\n \t}\n }\n@@ -1632,7 +1638,7 @@ static int open_sha1_file(const unsigned char *sha1)\n \tstruct alternate_object_database *alt;\n \tint most_interesting_errno;\n \n-\tfd = git_open_noatime(sha1_file_name(sha1));\n+\tfd = git_open(sha1_file_name(sha1));\n \tif (fd >= 0)\n \t\treturn fd;\n \tmost_interesting_errno = errno;\n@@ -1640,7 +1646,7 @@ static int open_sha1_file(const unsigned char *sha1)\n \tprepare_alt_odb();\n \tfor (alt = alt_odb_list; alt; alt = alt->next) {\n \t\tconst char *path = alt_sha1_path(alt, sha1);\n-\t\tfd = git_open_noatime(path);\n+\t\tfd = git_open(path);\n \t\tif (fd >= 0)\n \t\t\treturn fd;\n \t\tif (most_interesting_errno == ENOENT)\n-- \n2.10.0\n\n"},{"id":"304784","messageId":"20161024180300.52359-3-larsxschneider@gmail.com","threadId":"44366","inReplyTo":"20161024180300.52359-1-larsxschneider@gmail.com","subject":"[PATCH v2 2/2] read-cache: make sure file handles are not inherited by child processes","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-10-24T18:03:00Z","receivedAt":"2016-10-24T18:03:21Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider \n\nThis fixes \"convert: add filter..process option\" (edcc8581) on\nWindows.\n\nConsider the case of a file that requires filtering and is present in\nbranch A but not in branch B. If A is the current HEAD and we checkout B\nthen the following happens:\n\n1. ce_compare_data() opens the file\n2. index_fd() detects that the file requires to run a clean filter and\n calls index_stream_convert_blob()\n4. index_stream_convert_blob() calls convert_to_git_filter_fd()\n5. convert_to_git_filter_fd() calls apply_filter() which creates a\n new long running filter process (in case it is the first file\n of this kind to be filtered)\n6. The new filter process inherits all file handles. This is the\n default on Linux/OSX and is explicitly defined in the\n `CreateProcessW` call in `mingw.c` on Windows.\n7. ce_compare_data() closes the file\n8. Git unlinks the file as it is not present in B\n\nThe unlink operation does not work on Windows because the filter process\nhas still an open handle to the file. On Linux/OSX the unlink operation\nsucceeds but the file descriptors still leak into the child process.\n\nFix this problem by opening files in read-cache with the CLOEXEC flag to\nensure that the file descriptor does not remain open in a newly spawned\nprocess similar to 05d1ed61.\n\nSigned-off-by: Lars Schneider \n---\n read-cache.c | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 38d67fa..b594865 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -156,7 +156,11 @@ void fill_stat_cache_info(struct cache_entry *ce, struct stat *st)\n static int ce_compare_data(const struct cache_entry *ce, struct stat *st)\n {\n \tint match = -1;\n-\tint fd = open(ce->name, O_RDONLY);\n+\tint fd = open(ce->name, O_RDONLY | O_CLOEXEC);\n+\n+\tif (O_CLOEXEC && fd < 0 && errno == EINVAL)\n+\t\t/* Try again w/o O_CLOEXEC: the kernel might not support it */\n+\t\tfd = open(ce->name, O_RDONLY);\n \n \tif (fd >= 0) {\n \t\tunsigned char sha1[20];\n-- \n2.10.0\n\n"},{"id":"304787","messageId":"xmqq4m417gjf.fsf@gitster.mtv.corp.google.com","threadId":"44366","inReplyTo":"20161024180300.52359-1-larsxschneider@gmail.com","subject":"Re: [PATCH v2 0/2] Use CLOEXEC to avoid fd leaks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-24T18:23:00Z","receivedAt":"2016-10-24T18:23:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"larsxschneider@gmail.com writes:\n\n> ## Changes since v1\n> * add fallbacks in case O_CLOEXEC is not available\n\nThat is a good idea.\n\n> * rename 'git_open_noatime_cloexec' to 'git_open' (Eric, Dscho)\n\nOK. This is the old git_open_noatime() that is meant to be used\nONLY for the files Git uses for its internal implementation, and\nnever for end-user files. I think it is a good idea to open them\nwith O_CLOEXEC.\n\nAnd the separate patch to use O_CLOEXEC in ce_compare_data() that\nopens a working tree file for reading does not use git_open(), which\nis also correct. I like it.\n\n> * rebased the topic on `next` to fix a merge conflict\n\nI think this applies cleanly to 'master', so that is where I'd fork\nmy copy at.\n\nThanks.\n\n"},{"id":"304790","messageId":"20161024183900.GA12769@starla","threadId":"44366","inReplyTo":"20161024180300.52359-3-larsxschneider@gmail.com","subject":"Re: [PATCH v2 2/2] read-cache: make sure file handles are not inherited by child processes","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2016-10-24T18:39:00Z","receivedAt":"2016-10-24T18:39:05Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"larsxschneider@gmail.com wrote:\n> +++ b/read-cache.c\n> @@ -156,7 +156,11 @@ void fill_stat_cache_info(struct cache_entry *ce, struct stat *st)\n> static int ce_compare_data(const struct cache_entry *ce, struct stat *st)\n> {\n> \tint match = -1;\n> -\tint fd = open(ce->name, O_RDONLY);\n> +\tint fd = open(ce->name, O_RDONLY | O_CLOEXEC);\n> +\n> +\tif (O_CLOEXEC && fd < 0 && errno == EINVAL)\n> +\t\t/* Try again w/o O_CLOEXEC: the kernel might not support it */\n> +\t\tfd = open(ce->name, O_RDONLY);\n\nIn the case of O_CLOEXEC != 0 and repeated EINVALs,\nit'd be good to use something like sha1_file_open_flag as in 1/2\nso we don't repeatedly hit EINVAL. Thanks.\n"},{"id":"304795","messageId":"10135d1c-d9a7-d4ec-438a-bb0a8f6762fe@kdbg.org","threadId":"44366","inReplyTo":"20161024180300.52359-3-larsxschneider@gmail.com","subject":"Re: [PATCH v2 2/2] read-cache: make sure file handles are not inherited by child processes","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-10-24T19:22:34Z","receivedAt":"2016-10-24T19:24:26Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 24.10.2016 um 20:03 schrieb larsxschneider@gmail.com:\n> From: Lars Schneider \n>\n> This fixes \"convert: add filter..process option\" (edcc8581) on\n> Windows.\n\nToday's next falls flat on its face on Windows in t0021.15 \"required \nprocess filter should filter data\"; might it be the failure meant here? \n(I haven't dug deeper, yet.)\n\n++ test_config_global filter.protocol.process \n'/d/Src/mingw-git/t/t0021/rot13-filter.pl clean smudge'\n++ test_when_finished 'test_unconfig --global \n'\\''filter.protocol.process'\\'''\n++ test 0 = 0\n++ test_cleanup='{ test_unconfig --global '\\''filter.protocol.process'\\''\n } && (exit \"$eval_ret\"); eval_ret=$?; :'\n++ git config --global filter.protocol.process \n'/d/Src/mingw-git/t/t0021/rot13-filter.pl clean smudge'\n++ test_config_global filter.protocol.required true\n++ test_when_finished 'test_unconfig --global \n'\\''filter.protocol.required'\\'''\n++ test 0 = 0\n++ test_cleanup='{ test_unconfig --global '\\''filter.protocol.required'\\''\n } && (exit \"$eval_ret\"); eval_ret=$?; { test_unconfig \n--global '\\''filter.protocol.process'\\''\n } && (exit \"$eval_ret\"); eval_ret=$?; :'\n++ git config --global filter.protocol.required true\n++ rm -rf repo\n++ mkdir repo\n++ cd repo\n++ git init\nInitialized empty Git repository in d:/Src/mingw-git/t/trash \ndirectory.t0021-conversion/repo/.git/\n++ echo git-stderr.log\n++ echo '*.r filter=protocol'\n++ git add .\n++ git commit . -m 'test commit 1'\n[master (root-commit) aa5dd37] test commit 1\n Author: A U Thor \n 2 files changed, 2 insertions(+)\n create mode 100644 .gitattributes\n create mode 100644 .gitignore\n++ git branch empty-branch\n++ cp 'd:/Src/mingw-git/t/trash directory.t0021-conversion/test.o' test.r\n++ cp 'd:/Src/mingw-git/t/trash directory.t0021-conversion/test2.o' test2.r\n++ mkdir testsubdir\n++ cp 'd:/Src/mingw-git/t/trash directory.t0021-conversion/test3 \n'\\''sq'\\'',$x.o' 'testsubdir/test3 '\\''sq'\\'',$x.r'\n+++ file_size test.r\n+++ cat test.r\n+++ wc -c\n+++ sed 's/^[ ]*//'\n++ S=57\n+++ file_size test2.r\n+++ cat test2.r\n+++ wc -c\n+++ sed 's/^[ ]*//'\n++ S2=14\n+++ file_size 'testsubdir/test3 '\\''sq'\\'',$x.r'\n+++ cat 'testsubdir/test3 '\\''sq'\\'',$x.r'\n+++ wc -c\n+++ sed 's/^[ ]*//'\n++ S3=49\n++ filter_git add .\n++ rm -f rot13-filter.log\n++ git add .\nerror: last command exited with $?=128\nnot ok 15 - required process filter should filter data\n\n"},{"id":"304799","messageId":"xmqqwpgx4j89.fsf@gitster.mtv.corp.google.com","threadId":"44366","inReplyTo":"20161024183900.GA12769@starla","subject":"Re: [PATCH v2 2/2] read-cache: make sure file handles are not inherited by child processes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-24T19:53:10Z","receivedAt":"2016-10-24T19:53:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong writes:\n\n> larsxschneider@gmail.com wrote:\n>> +++ b/read-cache.c\n>> @@ -156,7 +156,11 @@ void fill_stat_cache_info(struct cache_entry *ce, struct stat *st)\n>> static int ce_compare_data(const struct cache_entry *ce, struct stat *st)\n>> {\n>> \tint match = -1;\n>> -\tint fd = open(ce->name, O_RDONLY);\n>> +\tint fd = open(ce->name, O_RDONLY | O_CLOEXEC);\n>> +\n>> +\tif (O_CLOEXEC && fd < 0 && errno == EINVAL)\n>> +\t\t/* Try again w/o O_CLOEXEC: the kernel might not support it */\n>> +\t\tfd = open(ce->name, O_RDONLY);\n>\n> In the case of O_CLOEXEC != 0 and repeated EINVALs,\n> it'd be good to use something like sha1_file_open_flag as in 1/2\n> so we don't repeatedly hit EINVAL. Thanks.\n\nSounds sane. \n\nIt's just only once, so perhaps we do not mind a recursion like\nthis?\n\n read-cache.c | 9 ++++++---\n 1 file changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex b594865d89..a6978b9321 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -156,11 +156,14 @@ void fill_stat_cache_info(struct cache_entry *ce, struct stat *st)\n static int ce_compare_data(const struct cache_entry *ce, struct stat *st)\n {\n \tint match = -1;\n-\tint fd = open(ce->name, O_RDONLY | O_CLOEXEC);\n+\tstatic int cloexec = O_CLOEXEC;\n+\tint fd = open(ce->name, O_RDONLY | cloexec);\n \n-\tif (O_CLOEXEC && fd < 0 && errno == EINVAL)\n+\tif ((cloexec & O_CLOEXEC) && fd < 0 && errno == EINVAL) {\n \t\t/* Try again w/o O_CLOEXEC: the kernel might not support it */\n-\t\tfd = open(ce->name, O_RDONLY);\n+\t\tcloexec &= ~O_CLOEXEC;\n+\t\treturn ce_compare_data(ce, st);\n+\t}\n \n \tif (fd >= 0) {\n \t\tunsigned char sha1[20];\n"},{"id":"304800","messageId":"4C96209A-756F-45F1-B075-037FE32B3291@gmail.com","threadId":"44366","inReplyTo":"10135d1c-d9a7-d4ec-438a-bb0a8f6762fe@kdbg.org","subject":"Re: [PATCH v2 2/2] read-cache: make sure file handles are not inherited by child processes","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-10-24T19:53:24Z","receivedAt":"2016-10-24T19:53:33Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 24 Oct 2016, at 21:22, Johannes Sixt wrote:\n> \n> Am 24.10.2016 um 20:03 schrieb larsxschneider@gmail.com:\n>> From: Lars Schneider \n>> \n>> This fixes \"convert: add filter..process option\" (edcc8581) on\n>> Windows.\n> \n> Today's next falls flat on its face on Windows in t0021.15 \"required process filter should filter data\"; might it be the failure meant here? (I haven't dug deeper, yet.)\n\nYes, this is the failure meant here :-)\n\nCheers,\nLars\n\n"},{"id":"304829","messageId":"alpine.DEB.2.20.1610251220280.3264@virtualbox","threadId":"44366","inReplyTo":"20161024180300.52359-2-larsxschneider@gmail.com","subject":"Re: [PATCH v2 1/2] sha1_file: open window into packfiles with CLOEXEC","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-10-25T10:27:52Z","receivedAt":"2016-10-25T10:28:14Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Lars,\n\nOn Mon, 24 Oct 2016, larsxschneider@gmail.com wrote:\n\n> From: Lars Schneider \n> \n> All processes that the Git main process spawns inherit the open file\n> descriptors of the main process. These leaked file descriptors can\n> cause problems.\n> \n> Use the CLOEXEC flag similar to 05d1ed61 to fix the leaked file\n> descriptors. Since `git_open_noatime` does not describe the function\n> properly anymore rename it to `git_open`.\n\nThe patch series may be a little bit more pleasant to read if you renamed\ngit_open_noatime() to git_open() first, in a separate commit.\n\n> @@ -1598,12 +1598,18 @@ int git_open_noatime(const char *name)\n> \t\tif (fd >= 0)\n> \t\t\treturn fd;\n> \n> -\t\t/* Might the failure be due to O_NOATIME? */\n> -\t\tif (errno != ENOENT && sha1_file_open_flag) {\n> -\t\t\tsha1_file_open_flag = 0;\n> +\t\t/* Try again w/o O_CLOEXEC: the kernel might not support it */\n> +\t\tif (O_CLOEXEC && errno == EINVAL &&\n> +\t\t\t(sha1_file_open_flag & O_CLOEXEC)) {\n> +\t\t\tsha1_file_open_flag &= ~O_CLOEXEC;\n\nHow about\n\n\t\tif ((O_CLOEXEC & sha1_file_open_flag) && errno == EINVAL) {\n\t\t\tsha1_file_open_flag &= ~O_CLOEXEC;\n\ninstead? It is shorter and should be just as easily optimized out by a\nC compiler if O_CLOEXEC was defined as 0.\n\n> \t\t\tcontinue;\n> \t\t}\n> \n> +\t\t/* Might the failure be due to O_NOATIME? */\n> +\t\tif (errno != ENOENT && (sha1_file_open_flag & O_NOATIME)) {\n> +\t\t\tsha1_file_open_flag &= ~O_NOATIME;\n> +\t\t\tcontinue;\n> +\t\t}\n\nI *think* the --patience diff option would have made that patch a little\nmore obvious.\n\nOtherwise, the patch looks fine to me,\nDscho\n"},{"id":"304830","messageId":"alpine.DEB.2.20.1610251230150.3264@virtualbox","threadId":"44366","inReplyTo":"xmqqwpgx4j89.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 2/2] read-cache: make sure file handles are not inherited by child processes","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-10-25T10:33:54Z","receivedAt":"2016-10-25T10:34:21Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 24 Oct 2016, Junio C Hamano wrote:\n\n> Eric Wong writes:\n> \n> > larsxschneider@gmail.com wrote:\n> >> +++ b/read-cache.c\n> >> @@ -156,7 +156,11 @@ void fill_stat_cache_info(struct cache_entry *ce, struct stat *st)\n> >> static int ce_compare_data(const struct cache_entry *ce, struct stat *st)\n> >> {\n> >> \tint match = -1;\n> >> -\tint fd = open(ce->name, O_RDONLY);\n> >> +\tint fd = open(ce->name, O_RDONLY | O_CLOEXEC);\n> >> +\n> >> +\tif (O_CLOEXEC && fd < 0 && errno == EINVAL)\n> >> +\t\t/* Try again w/o O_CLOEXEC: the kernel might not support it */\n> >> +\t\tfd = open(ce->name, O_RDONLY);\n> >\n> > In the case of O_CLOEXEC != 0 and repeated EINVALs,\n> > it'd be good to use something like sha1_file_open_flag as in 1/2\n> > so we don't repeatedly hit EINVAL. Thanks.\n> \n> Sounds sane. \n> \n> It's just only once, so perhaps we do not mind a recursion like\n> this?\n> \n> read-cache.c | 9 ++++++---\n> 1 file changed, 6 insertions(+), 3 deletions(-)\n> \n> diff --git a/read-cache.c b/read-cache.c\n> index b594865d89..a6978b9321 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -156,11 +156,14 @@ void fill_stat_cache_info(struct cache_entry *ce, struct stat *st)\n> static int ce_compare_data(const struct cache_entry *ce, struct stat *st)\n> {\n> \tint match = -1;\n> -\tint fd = open(ce->name, O_RDONLY | O_CLOEXEC);\n> +\tstatic int cloexec = O_CLOEXEC;\n> +\tint fd = open(ce->name, O_RDONLY | cloexec);\n> \n> -\tif (O_CLOEXEC && fd < 0 && errno == EINVAL)\n> +\tif ((cloexec & O_CLOEXEC) && fd < 0 && errno == EINVAL) {\n> \t\t/* Try again w/o O_CLOEXEC: the kernel might not support it */\n> -\t\tfd = open(ce->name, O_RDONLY);\n> +\t\tcloexec &= ~O_CLOEXEC;\n> +\t\treturn ce_compare_data(ce, st);\n> +\t}\n> \n\nThat still looks overly complicated, repeatedly ORing cloexec and\nrecursing without need. How about this instead?\n\n\tstatic int oflags = O_RDONLY | O_CLOEXEC;\n\tint fd = open(ce->name, oflags);\n\n\tif ((O_CLOEXEC & oflags) && fd < 0 && errno == EINVAL) {\n \t\t/* Try again w/o O_CLOEXEC: the kernel might not support it */\n\t\toflags &= ~O_CLOEXEC;\n\t\tfd = open(ce->name, oflags);\n\t}\n\nCiao,\nDscho\n"},{"id":"304835","messageId":"alpine.DEB.2.20.1610251327050.3264@virtualbox","threadId":"44366","inReplyTo":"20161024180300.52359-1-larsxschneider@gmail.com","subject":"Re: [PATCH v2 0/2] Use CLOEXEC to avoid fd leaks","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-10-25T11:27:37Z","receivedAt":"2016-10-25T11:27:53Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Lars,\n\nOn Mon, 24 Oct 2016, larsxschneider@gmail.com wrote:\n\n> This mini patch series is necessary to make the \"ls/filter-process\" topic\n> work properly for Windows. Right now the topic generates test failures\n> on Windows as reported by Dscho:\n> http://public-inbox.org/git/alpine.DEB.2.20.1610211457030.3264@virtualbox/\n\nI acknowledge that this fixes the test failures in Git for Windows' SDK.\n\nThanks,\nDscho\n"},{"id":"304850","messageId":"xmqqwpgws6ur.fsf@gitster.mtv.corp.google.com","threadId":"44366","inReplyTo":"alpine.DEB.2.20.1610251220280.3264@virtualbox","subject":"Re: [PATCH v2 1/2] sha1_file: open window into packfiles with CLOEXEC","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-25T16:58:52Z","receivedAt":"2016-10-25T16:59:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> The patch series may be a little bit more pleasant to read if you renamed\n> git_open_noatime() to git_open() first, in a separate commit.\n\nPossibly.\n\nIf this were \"we added a new parameter at the same time and each\ncalling site of the renamed function with an extra parameter chooses\nto pass different value to it\", then keeping the rename and the\ninterface enhancement as two separate steps would help a lot.\n\nBut this one only renames and updates the internal implementation,\nwithout changing what the calling sites need to do. I am OK with\nhaving them together in a single patch like the one posted.\n\n>> @@ -1598,12 +1598,18 @@ int git_open_noatime(const char *name)\n>> \t\tif (fd >= 0)\n>> \t\t\treturn fd;\n>> \n>> -\t\t/* Might the failure be due to O_NOATIME? */\n>> -\t\tif (errno != ENOENT && sha1_file_open_flag) {\n>> -\t\t\tsha1_file_open_flag = 0;\n>> +\t\t/* Try again w/o O_CLOEXEC: the kernel might not support it */\n>> +\t\tif (O_CLOEXEC && errno == EINVAL &&\n>> +\t\t\t(sha1_file_open_flag & O_CLOEXEC)) {\n>> +\t\t\tsha1_file_open_flag &= ~O_CLOEXEC;\n>\n> How about\n>\n> \t\tif ((O_CLOEXEC & sha1_file_open_flag) && errno == EINVAL) {\n> \t\t\tsha1_file_open_flag &= ~O_CLOEXEC;\n>\n> instead? It is shorter and should be just as easily optimized out by a\n> C compiler if O_CLOEXEC was defined as 0.\n\nYup, I think that makes things easier to read for humans, too.\n"},{"id":"304851","messageId":"xmqqshrks6ot.fsf@gitster.mtv.corp.google.com","threadId":"44366","inReplyTo":"alpine.DEB.2.20.1610251230150.3264@virtualbox","subject":"Re: [PATCH v2 2/2] read-cache: make sure file handles are not inherited by child processes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-25T17:02:26Z","receivedAt":"2016-10-25T17:02:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> That still looks overly complicated, repeatedly ORing cloexec and\n> recursing without need. How about this instead?\n>\n> \tstatic int oflags = O_RDONLY | O_CLOEXEC;\n> \tint fd = open(ce->name, oflags);\n>\n> \tif ((O_CLOEXEC & oflags) && fd < 0 && errno == EINVAL) {\n> \t\t/* Try again w/o O_CLOEXEC: the kernel might not support it */\n> \t\toflags &= ~O_CLOEXEC;\n> \t\tfd = open(ce->name, oflags);\n> \t}\n\nI deliberately separated the part that can and designed to be\ntoggled (O_CLOEXEC) and the part that is meant to be constant\n(O_RDONLY), and I do not think the first part of suggestion is\nparticularly a good idea.\n\nI didn't write the same open twice, but I agree that an extra \"we\nfallback to opening with a different flags\" inside the if () { }\nblock that is there exactly for implementing that fallback is an\nexcellent idea. I like that part of the suggestion.\n\nThanks.\n"},{"id":"304856","messageId":"20161025181621.4201-3-gitster@pobox.com","threadId":"44366","inReplyTo":"20161025181621.4201-1-gitster@pobox.com","subject":"[PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-25T18:16:20Z","receivedAt":"2016-10-25T18:17:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: Lars Schneider \n\nAll processes that the Git main process spawns inherit the open file\ndescriptors of the main process. These leaked file descriptors can\ncause problems.\n\nUse the O_CLOEXEC flag similar to 05d1ed61 to fix the leaked file\ndescriptors.\n\nSigned-off-by: Lars Schneider \nSigned-off-by: Junio C Hamano \n---\n\n * And the remainder of original 1/2, again taking suggestion by DScho.\n\n sha1_file.c | 13 +++++++++----\n 1 file changed, 9 insertions(+), 4 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 5d2bcd3ed1..09045df1dc 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1561,7 +1561,7 @@ int check_sha1_signature(const unsigned char *sha1, void *map,\n \n int git_open(const char *name)\n {\n-\tstatic int sha1_file_open_flag = O_NOATIME;\n+\tstatic int sha1_file_open_flag = O_NOATIME | O_CLOEXEC;\n \n \tfor (;;) {\n \t\tint fd;\n@@ -1571,12 +1571,17 @@ int git_open(const char *name)\n \t\tif (fd >= 0)\n \t\t\treturn fd;\n \n-\t\t/* Might the failure be due to O_NOATIME? */\n-\t\tif (errno != ENOENT && sha1_file_open_flag) {\n-\t\t\tsha1_file_open_flag = 0;\n+\t\t/* Try again w/o O_CLOEXEC: the kernel might not support it */\n+\t\tif ((sha1_file_open_flag & O_CLOEXEC) && errno == EINVAL) {\n+\t\t\tsha1_file_open_flag &= ~O_CLOEXEC;\n \t\t\tcontinue;\n \t\t}\n \n+\t\t/* Might the failure be due to O_NOATIME? */\n+\t\tif (errno != ENOENT && (sha1_file_open_flag & O_NOATIME)) {\n+\t\t\tsha1_file_open_flag &= ~O_NOATIME;\n+\t\t\tcontinue;\n+\t\t}\n \t\treturn -1;\n \t}\n }\n-- \n2.10.1-777-gd068e6bde7\n\n"},{"id":"304857","messageId":"20161025181621.4201-2-gitster@pobox.com","threadId":"44366","inReplyTo":"20161025181621.4201-1-gitster@pobox.com","subject":"[PATCH v3 1/3] sha1_file: rename git_open_noatime() to git_open()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-25T18:16:19Z","receivedAt":"2016-10-25T18:17:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: Lars Schneider \n\nThis function is meant to be used when reading from files in the\nobject store, and the original objective was to avoid smudging atime\nof loose object files too often, hence its name. Because we'll be\nextending its role in the next commit to also arrange the file\ndescriptors they return auto-closed in the child processes, rename\nit to lose \"noatime\" part that is too specific.\n\nSigned-off-by: Lars Schneider \nSigned-off-by: Junio C Hamano \n---\n\n * This is a pure-rename step suggested by Dscho.\n\n builtin/pack-objects.c | 2 +-\n cache.h | 2 +-\n pack-bitmap.c | 2 +-\n sha1_file.c | 12 ++++++------\n 4 files changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 1e7c2a98a5..0fd52bd6b4 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -720,7 +720,7 @@ static off_t write_reused_pack(struct sha1file *f)\n \tif (!is_pack_valid(reuse_packfile))\n \t\tdie(\"packfile is invalid: %s\", reuse_packfile->pack_name);\n \n-\tfd = git_open_noatime(reuse_packfile->pack_name);\n+\tfd = git_open(reuse_packfile->pack_name);\n \tif (fd < 0)\n \t\tdie_errno(\"unable to open packfile for reuse: %s\",\n \t\t\t reuse_packfile->pack_name);\ndiff --git a/cache.h b/cache.h\nindex 0dc39a998c..a902ca1f8e 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1122,7 +1122,7 @@ extern int write_sha1_file(const void *buf, unsigned long len, const char *type,\n extern int hash_sha1_file_literally(const void *buf, unsigned long len, const char *type, unsigned char *sha1, unsigned flags);\n extern int pretend_sha1_file(void *, unsigned long, enum object_type, unsigned char *);\n extern int force_object_loose(const unsigned char *sha1, time_t mtime);\n-extern int git_open_noatime(const char *name);\n+extern int git_open(const char *name);\n extern void *map_sha1_file(const unsigned char *sha1, unsigned long *size);\n extern int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long mapsize, void *buffer, unsigned long bufsiz);\n extern int parse_sha1_header(const char *hdr, unsigned long *sizep);\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex b949e51716..39bcc16846 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -266,7 +266,7 @@ static int open_pack_bitmap_1(struct packed_git *packfile)\n \t\treturn -1;\n \n \tidx_name = pack_bitmap_filename(packfile);\n-\tfd = git_open_noatime(idx_name);\n+\tfd = git_open(idx_name);\n \tfree(idx_name);\n \n \tif (fd < 0)\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 266152de36..5d2bcd3ed1 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -370,7 +370,7 @@ void read_info_alternates(const char * relative_base, int depth)\n \tint fd;\n \n \tpath = xstrfmt(\"%s/info/alternates\", relative_base);\n-\tfd = git_open_noatime(path);\n+\tfd = git_open(path);\n \tfree(path);\n \tif (fd < 0)\n \t\treturn;\n@@ -663,7 +663,7 @@ static int check_packed_git_idx(const char *path, struct packed_git *p)\n \tstruct pack_idx_header *hdr;\n \tsize_t idx_size;\n \tuint32_t version, nr, i, *index;\n-\tint fd = git_open_noatime(path);\n+\tint fd = git_open(path);\n \tstruct stat st;\n \n \tif (fd < 0)\n@@ -1069,7 +1069,7 @@ static int open_packed_git_1(struct packed_git *p)\n \twhile (pack_max_fds <= pack_open_fds && close_one_pack())\n \t\t; /* nothing */\n \n-\tp->pack_fd = git_open_noatime(p->pack_name);\n+\tp->pack_fd = git_open(p->pack_name);\n \tif (p->pack_fd < 0 || fstat(p->pack_fd, &st))\n \t\treturn -1;\n \tpack_open_fds++;\n@@ -1559,7 +1559,7 @@ int check_sha1_signature(const unsigned char *sha1, void *map,\n \treturn hashcmp(sha1, real_sha1) ? -1 : 0;\n }\n \n-int git_open_noatime(const char *name)\n+int git_open(const char *name)\n {\n \tstatic int sha1_file_open_flag = O_NOATIME;\n \n@@ -1605,7 +1605,7 @@ static int open_sha1_file(const unsigned char *sha1)\n \tstruct alternate_object_database *alt;\n \tint most_interesting_errno;\n \n-\tfd = git_open_noatime(sha1_file_name(sha1));\n+\tfd = git_open(sha1_file_name(sha1));\n \tif (fd >= 0)\n \t\treturn fd;\n \tmost_interesting_errno = errno;\n@@ -1613,7 +1613,7 @@ static int open_sha1_file(const unsigned char *sha1)\n \tprepare_alt_odb();\n \tfor (alt = alt_odb_list; alt; alt = alt->next) {\n \t\tconst char *path = alt_sha1_path(alt, sha1);\n-\t\tfd = git_open_noatime(path);\n+\t\tfd = git_open(path);\n \t\tif (fd >= 0)\n \t\t\treturn fd;\n \t\tif (most_interesting_errno == ENOENT)\n-- \n2.10.1-777-gd068e6bde7\n\n"},{"id":"304858","messageId":"20161025181621.4201-1-gitster@pobox.com","threadId":"44366","inReplyTo":"alpine.DEB.2.20.1610251327050.3264@virtualbox","subject":"[PATCH v3 0/3] quick reroll of Lars's git_open() w/ O_CLOEXEC","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-25T18:16:18Z","receivedAt":"2016-10-25T18:17:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Here is to make sure everybody is on the same page regarding the\nseries. The patches are adjusted for comments from Eric and Dscho.\n\nLars Schneider (3):\n sha1_file: rename git_open_noatime() to git_open()\n sha1_file: open window into packfiles with O_CLOEXEC\n read-cache: make sure file handles are not inherited by child\n processes\n\n builtin/pack-objects.c | 2 +-\n cache.h | 2 +-\n pack-bitmap.c | 2 +-\n read-cache.c | 9 ++++++++-\n sha1_file.c | 25 +++++++++++++++----------\n 5 files changed, 26 insertions(+), 14 deletions(-)\n\n-- \n2.10.1-777-gd068e6bde7\n\n"},{"id":"304859","messageId":"20161025181621.4201-4-gitster@pobox.com","threadId":"44366","inReplyTo":"20161025181621.4201-1-gitster@pobox.com","subject":"[PATCH v3 3/3] read-cache: make sure file handles are not inherited by child processes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-25T18:16:21Z","receivedAt":"2016-10-25T18:17:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: Lars Schneider \n\nThis fixes \"convert: add filter..process option\" (edcc8581) on\nWindows.\n\nConsider the case of a file that requires filtering and is present in\nbranch A but not in branch B. If A is the current HEAD and we checkout B\nthen the following happens:\n\n1. ce_compare_data() opens the file\n2. index_fd() detects that the file requires to run a clean filter and\n calls index_stream_convert_blob()\n4. index_stream_convert_blob() calls convert_to_git_filter_fd()\n5. convert_to_git_filter_fd() calls apply_filter() which creates a\n new long running filter process (in case it is the first file\n of this kind to be filtered)\n6. The new filter process inherits all file handles. This is the\n default on Linux/OSX and is explicitly defined in the\n `CreateProcessW` call in `mingw.c` on Windows.\n7. ce_compare_data() closes the file\n8. Git unlinks the file as it is not present in B\n\nThe unlink operation does not work on Windows because the filter process\nhas still an open handle to the file. On Linux/OSX the unlink operation\nsucceeds but the file descriptors still leak into the child process.\n\nFix this problem by opening files in read-cache with the O_CLOEXEC flag\nto ensure that the file descriptor does not remain open in a newly\nspawned process similar to 05d1ed6148 (\"mingw: ensure temporary file\nhandles are not inherited by child processes\", 2016-08-22).\n\nSigned-off-by: Lars Schneider \nSigned-off-by: Junio C Hamano \n---\n\n * With Eric's suggestion to avoid repeated EINVAL and fallback,\n implemented with Dscho's \"write open twice explicitly, we are\n retrying after all\" coding style.\n\n read-cache.c | 9 ++++++++-\n 1 file changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 38d67faf70..db5d910642 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -156,7 +156,14 @@ void fill_stat_cache_info(struct cache_entry *ce, struct stat *st)\n static int ce_compare_data(const struct cache_entry *ce, struct stat *st)\n {\n \tint match = -1;\n-\tint fd = open(ce->name, O_RDONLY);\n+\tstatic int cloexec = O_CLOEXEC;\n+\tint fd = open(ce->name, O_RDONLY | cloexec);\n+\n+\tif ((cloexec & O_CLOEXEC) && fd < 0 && errno == EINVAL) {\n+\t\t/* Try again w/o O_CLOEXEC: the kernel might not support it */\n+\t\tcloexec &= ~O_CLOEXEC;\n+\t\tfd = open(ce->name, O_RDONLY | cloexec);\n+\t}\n \n \tif (fd >= 0) {\n \t\tunsigned char sha1[20];\n-- \n2.10.1-777-gd068e6bde7\n\n"},{"id":"304869","messageId":"20161025213318.GB8683@starla","threadId":"44366","inReplyTo":"20161025181621.4201-4-gitster@pobox.com","subject":"Re: [PATCH v3 3/3] read-cache: make sure file handles are not inherited by child processes","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2016-10-25T21:33:18Z","receivedAt":"2016-10-25T21:33:23Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Junio C Hamano wrote:\n> @@ -156,7 +156,14 @@ void fill_stat_cache_info(struct cache_entry *ce, struct stat *st)\n> static int ce_compare_data(const struct cache_entry *ce, struct stat *st)\n> {\n> \tint match = -1;\n> -\tint fd = open(ce->name, O_RDONLY);\n> +\tstatic int cloexec = O_CLOEXEC;\n> +\tint fd = open(ce->name, O_RDONLY | cloexec);\n> +\n> +\tif ((cloexec & O_CLOEXEC) && fd < 0 && errno == EINVAL) {\n> +\t\t/* Try again w/o O_CLOEXEC: the kernel might not support it */\n> +\t\tcloexec &= ~O_CLOEXEC;\n> +\t\tfd = open(ce->name, O_RDONLY | cloexec);\n> +\t}\n\nSeems fine, I prefer not using recursion so it's\neasier-to-review.\n\nBut I have a _slight_ preference towards Dscho's version in\n in case we\ndecide to start using another O_* flag in here.\n(but I'm not usually a C programmer)\n"},{"id":"304871","messageId":"e8deda5f-11a6-1463-4fc5-25454084ccb1@kdbg.org","threadId":"44366","inReplyTo":"4C96209A-756F-45F1-B075-037FE32B3291@gmail.com","subject":"Re: [PATCH v2 2/2] read-cache: make sure file handles are not inherited by child processes","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-10-25T21:39:11Z","receivedAt":"2016-10-25T21:41:53Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 24.10.2016 um 21:53 schrieb Lars Schneider:\n>\n>> On 24 Oct 2016, at 21:22, Johannes Sixt wrote:\n>>\n>> Am 24.10.2016 um 20:03 schrieb larsxschneider@gmail.com:\n>>> From: Lars Schneider \n>>>\n>>> This fixes \"convert: add filter..process option\" (edcc8581) on\n>>> Windows.\n>>\n>> Today's next falls flat on its face on Windows in t0021.15\n>> \"required process filter should filter data\"; might it be the\n>> failure meant here?(I haven't dug deeper, yet.)\n>\n> Yes, this is the failure meant here :-)\n\nI trust your word and Dscho's that it fixes a failure on Windows. In my \ncase, however, it was an outdated perl (5.8.8) which left a message \nalong the lines of \"lookup of member flush in IO::Handle failed\" in one \nof the *.log files. I came up with the following workaround.\n\nFix-method: shot-in-the-dark\nPerl-foo: not-present\nNot-signed-off-by: Me\n\ndiff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl\nindex ae4c50f..bb88c5f 100755\n--- a/t/t0021/rot13-filter.pl\n+++ b/t/t0021/rot13-filter.pl\n@@ -22,6 +22,7 @@\n\n use strict;\n use warnings;\n+use FileHandle;\n\n my $MAX_PACKET_CONTENT_SIZE = 65516;\n my @capabilities = @ARGV;\n-- \n2.10.0.343.g37bc62b\n\n"},{"id":"304872","messageId":"A6C42931-835D-42FD-B4D9-7F84921909E2@gmail.com","threadId":"44366","inReplyTo":"20161025181621.4201-1-gitster@pobox.com","subject":"Re: [PATCH v3 0/3] quick reroll of Lars's git_open() w/ O_CLOEXEC","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-10-25T21:48:42Z","receivedAt":"2016-10-25T21:48:49Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 25 Oct 2016, at 20:16, Junio C Hamano wrote:\n> \n> Here is to make sure everybody is on the same page regarding the\n> series. The patches are adjusted for comments from Eric and Dscho.\n\nThank you, Junio! Your v3 looks good to me and I compiled and tested\nit on Windows.\n\nThere is one catch though:\nI ran the tests multiple times on Windows and I noticed that the\nfollowing test seems flaky:\nt0021 `required process filter should be used only for \"clean\" operation only'\n\nThis flaky-ness was not introduced by your v3. I was able to reproduce\nthe same behavior for v2. I wonder why I did not noticed that before. \nThe only difference to before is that my Windows machines does some \nheavy CPU/network task in the background now (I can't/don't want to\nstop that ATM).\n\nHere is the relevant part of the log:\n\n++ rm -f rot13-filter.log\n++ git checkout --quiet --no-progress .\n+ test_eval_ret_=255\n+ want_trace\n+ test t = t\n+ test t = t\n+ set +x\nerror: last command exited with $?=255\n\nLooks like Git exists with 255. Any idea why that might happen?\n\n@Dscho/Johannes: Can you try this on your Windows machines?\n\nThanks,\nLars\n\n\n"},{"id":"304873","messageId":"xmqqlgxcm43o.fsf@gitster.mtv.corp.google.com","threadId":"44366","inReplyTo":"20161025213318.GB8683@starla","subject":"Re: [PATCH v3 3/3] read-cache: make sure file handles are not inherited by child processes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-25T22:54:51Z","receivedAt":"2016-10-25T22:54:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong writes:\n\n> But I have a _slight_ preference towards Dscho's version in\n> in case we\n> decide to start using another O_* flag in here.\n\nInteresting. The reason why I have a slight preferene to separate\nthe fixed part and the toggle-able part is exactly because I want\nthe code to be prepared in case we decide to start using other O_*\nflags in the future.\n\nI guess different people have different tastes, as usual ;-)\n"},{"id":"304874","messageId":"xmqqh980m41e.fsf@gitster.mtv.corp.google.com","threadId":"44366","inReplyTo":"A6C42931-835D-42FD-B4D9-7F84921909E2@gmail.com","subject":"Re: [PATCH v3 0/3] quick reroll of Lars's git_open() w/ O_CLOEXEC","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-25T22:56:13Z","receivedAt":"2016-10-25T22:56:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider writes:\n\n> heavy CPU/network task in the background now (I can't/don't want to\n> stop that ATM).\n>\n> Here is the relevant part of the log:\n>\n> ++ rm -f rot13-filter.log\n> ++ git checkout --quiet --no-progress .\n> + test_eval_ret_=255\n> + want_trace\n> + test t = t\n> + test t = t\n> + set +x\n> error: last command exited with $?=255\n>\n> Looks like Git exists with 255. Any idea why that might happen?\n\nOther than \"the --quiet above may be hiding it?\", I do not\nimmediately have a useful guess, sorry.\n\n> @Dscho/Johannes: Can you try this on your Windows machines?\n>\n> Thanks,\n> Lars\n"},{"id":"304879","messageId":"20161026042555.neaxvnmggtcku5cc@sigill.intra.peff.net","threadId":"44366","inReplyTo":"20161025181621.4201-3-gitster@pobox.com","subject":"Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-26T04:25:55Z","receivedAt":"2016-10-26T04:26:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 25, 2016 at 11:16:20AM -0700, Junio C Hamano wrote:\n\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 5d2bcd3ed1..09045df1dc 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -1571,12 +1571,17 @@ int git_open(const char *name)\n> \t\tif (fd >= 0)\n> \t\t\treturn fd;\n> \n> -\t\t/* Might the failure be due to O_NOATIME? */\n> -\t\tif (errno != ENOENT && sha1_file_open_flag) {\n> -\t\t\tsha1_file_open_flag = 0;\n> +\t\t/* Try again w/o O_CLOEXEC: the kernel might not support it */\n> +\t\tif ((sha1_file_open_flag & O_CLOEXEC) && errno == EINVAL) {\n> +\t\t\tsha1_file_open_flag &= ~O_CLOEXEC;\n> \t\t\tcontinue;\n> \t\t}\n\nSo if we start with O_CLOEXEC|O_NOATIME, we drop CLOEXEC here and try\nagain with just O_NOATIME. And then if _that_ fails...\n\n> +\t\t/* Might the failure be due to O_NOATIME? */\n> +\t\tif (errno != ENOENT && (sha1_file_open_flag & O_NOATIME)) {\n> +\t\t\tsha1_file_open_flag &= ~O_NOATIME;\n> +\t\t\tcontinue;\n> +\t\t}\n\nWe drop O_NOATIME, and end up with an empty flag field.\n\nBut we will never have tried just O_CLOEXEC, which might have worked.\n\nBecause of the order here, this would not be a regression (i.e., any\nsystem that used to work will still eventually find a working comb), but\nit does mean that systems without O_NOATIME do not get the benefit of\nthe new O_CLOEXEC protection.\n\nUnfortunately, I think covering all of the cases would be 2^nr_flags.\nThat's only 4 right now, but it does not bode well as a pattern.\n\nI'm not sure it's worth worrying about or not; I don't know which\nsystems are actually lacking either of the flags, or if they tend to\nhave both.\n\n-Peff\n"},{"id":"304898","messageId":"xmqqa8drcc5i.fsf@gitster.mtv.corp.google.com","threadId":"44366","inReplyTo":"20161026042555.neaxvnmggtcku5cc@sigill.intra.peff.net","subject":"Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-26T16:23:21Z","receivedAt":"2016-10-26T16:23:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> +\t\t/* Try again w/o O_CLOEXEC: the kernel might not support it */\n>> +\t\tif ((sha1_file_open_flag & O_CLOEXEC) && errno == EINVAL) {\n>> +\t\t\tsha1_file_open_flag &= ~O_CLOEXEC;\n>> \t\t\tcontinue;\n>> \t\t}\n>\n> So if we start with O_CLOEXEC|O_NOATIME, we drop CLOEXEC here and try\n> again with just O_NOATIME. And then if _that_ fails...\n>\n>> +\t\t/* Might the failure be due to O_NOATIME? */\n>> +\t\tif (errno != ENOENT && (sha1_file_open_flag & O_NOATIME)) {\n>> +\t\t\tsha1_file_open_flag &= ~O_NOATIME;\n>> +\t\t\tcontinue;\n>> +\t\t}\n>\n> We drop O_NOATIME, and end up with an empty flag field.\n>\n> But we will never have tried just O_CLOEXEC, which might have worked.\n\nYes, doing so would smudge atime, so one question is which one\nbetween noatime or cloexec is more important to be done at open(2)\ntime.\n\nIt may be possible to open(2) only with cloexec and then fcntl(2)\nFD_SET noatime immediately after, but going that way would explode\nthe combination even more, as it may not be possible to set these\ntwo flags the other way around.\n\n> I'm not sure it's worth worrying about or not; I don't know which\n> systems are actually lacking either of the flags, or if they tend to\n> have both.\n\n"},{"id":"304899","messageId":"20161026164746.2fu57f4pji5qdtnh@sigill.intra.peff.net","threadId":"44366","inReplyTo":"xmqqa8drcc5i.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-26T16:47:46Z","receivedAt":"2016-10-26T16:47:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 26, 2016 at 09:23:21AM -0700, Junio C Hamano wrote:\n\n> >> +\t\t/* Might the failure be due to O_NOATIME? */\n> >> +\t\tif (errno != ENOENT && (sha1_file_open_flag & O_NOATIME)) {\n> >> +\t\t\tsha1_file_open_flag &= ~O_NOATIME;\n> >> +\t\t\tcontinue;\n> >> +\t\t}\n> >\n> > We drop O_NOATIME, and end up with an empty flag field.\n> >\n> > But we will never have tried just O_CLOEXEC, which might have worked.\n> \n> Yes, doing so would smudge atime, so one question is which one\n> between noatime or cloexec is more important to be done at open(2)\n> time.\n\nYes, but the missing case is one where we know that O_NOATIME does not\nwork (but O_CLOEXEC does), so we know we have to smudge the atime.\n\nOf the two flags, I would say CLOEXEC is the more important one to\nrespect because it may actually impact correctness (e.g., leaking\ndescriptors to sub-processes). Whereas O_NOATIME is purely a performance\noptimization.\n\nI actually wonder if it is worth carrying around the O_NOATIME hack at\nall. Linus added it on 2005-04-23 via 144bde78e9; the aim was to reduce\nthe cost of opening loose object files. Some things have changed since\nthen:\n\n 1. In June 2005, git learned about packfiles, which means we would do\n a lot fewer atime updates (rather than one per object access, we'd\n generally get one per packfile).\n\n 2. In late 2006, Linux learned about \"relatime\", which is generally\n the default on modern installs. So performance around atime updates\n is a non-issue there these days.\n\n All the world isn't Linux, of course, but I can't help that feel\n that atime performance hackery is something that belongs at the\n system level, not in individual applications.\n\nSo I don't have hard numbers, but I'd be surprised if O_NOATIME is\nreally buying us anything these days.\n\n-Peff\n"},{"id":"304903","messageId":"xmqqpomnatg6.fsf@gitster.mtv.corp.google.com","threadId":"44366","inReplyTo":"20161026164746.2fu57f4pji5qdtnh@sigill.intra.peff.net","subject":"Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-26T17:52:41Z","receivedAt":"2016-10-26T17:52:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Of the two flags, I would say CLOEXEC is the more important one to\n> respect because it may actually impact correctness (e.g., leaking\n> descriptors to sub-processes). Whereas O_NOATIME is purely a performance\n> optimization.\n\nI tend to agree.\n\n> I actually wonder if it is worth carrying around the O_NOATIME hack at\n> all.\n\nYes, I share the thought. We no longer have too many loose objects\nto matter.\n\nI do not mind flipping the order, but I'd prefer to cook the result\neven longer. I am tempted to suggest we take two step route:\n\n - ship 2.11 with the \"atime has been there and we won't regress it\"\n shape, while cooking the \"cloexec is semantically more\n important\" version in 'next' during the feature freeze\n\n - immediately after 2.11 merge it to 'master' for 2.12 to make sure\n there is no fallout.\n\n"},{"id":"304909","messageId":"20161026201721.2pw4slsuyhxhcwxj@sigill.intra.peff.net","threadId":"44366","inReplyTo":"xmqqpomnatg6.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-26T20:17:21Z","receivedAt":"2016-10-26T20:17:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 26, 2016 at 10:52:41AM -0700, Junio C Hamano wrote:\n\n> > I actually wonder if it is worth carrying around the O_NOATIME hack at\n> > all.\n> \n> Yes, I share the thought. We no longer have too many loose objects\n> to matter.\n> \n> I do not mind flipping the order, but I'd prefer to cook the result\n> even longer. I am tempted to suggest we take two step route:\n> \n> - ship 2.11 with the \"atime has been there and we won't regress it\"\n> shape, while cooking the \"cloexec is semantically more\n> important\" version in 'next' during the feature freeze\n> \n> - immediately after 2.11 merge it to 'master' for 2.12 to make sure\n> there is no fallout.\n\nThat sounds reasonable, though I'd consider jumping straight to \"NOATIME\nis not worth it; drop it\" as the patch for post-2.11.\n\n-Peff\n"},{"id":"304918","messageId":"xmqqd1imbymi.fsf@gitster.mtv.corp.google.com","threadId":"44366","inReplyTo":"20161026201721.2pw4slsuyhxhcwxj@sigill.intra.peff.net","subject":"Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-26T21:15:33Z","receivedAt":"2016-10-26T21:15:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Oct 26, 2016 at 10:52:41AM -0700, Junio C Hamano wrote:\n>\n>> > I actually wonder if it is worth carrying around the O_NOATIME hack at\n>> > all.\n>> \n>> Yes, I share the thought. We no longer have too many loose objects\n>> to matter.\n>> \n>> I do not mind flipping the order, but I'd prefer to cook the result\n>> even longer. I am tempted to suggest we take two step route:\n>> \n>> - ship 2.11 with the \"atime has been there and we won't regress it\"\n>> shape, while cooking the \"cloexec is semantically more\n>> important\" version in 'next' during the feature freeze\n>> \n>> - immediately after 2.11 merge it to 'master' for 2.12 to make sure\n>> there is no fallout.\n>\n> That sounds reasonable, though I'd consider jumping straight to \"NOATIME\n> is not worth it; drop it\" as the patch for post-2.11.\n\nThat endgame is fine by me too. Thanks for a sanity-check.\n"},{"id":"304959","messageId":"20161027102419.dbzigj7wtr355ofh@sigill.intra.peff.net","threadId":"44366","inReplyTo":"xmqqd1imbymi.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-27T10:24:19Z","receivedAt":"2016-10-27T14:27:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 26, 2016 at 02:15:33PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > On Wed, Oct 26, 2016 at 10:52:41AM -0700, Junio C Hamano wrote:\n> >\n> >> > I actually wonder if it is worth carrying around the O_NOATIME hack at\n> >> > all.\n> >> \n> >> Yes, I share the thought. We no longer have too many loose objects\n> >> to matter.\n> >> \n> >> I do not mind flipping the order, but I'd prefer to cook the result\n> >> even longer. I am tempted to suggest we take two step route:\n> >> \n> >> - ship 2.11 with the \"atime has been there and we won't regress it\"\n> >> shape, while cooking the \"cloexec is semantically more\n> >> important\" version in 'next' during the feature freeze\n> >> \n> >> - immediately after 2.11 merge it to 'master' for 2.12 to make sure\n> >> there is no fallout.\n> >\n> > That sounds reasonable, though I'd consider jumping straight to \"NOATIME\n> > is not worth it; drop it\" as the patch for post-2.11.\n> \n> That endgame is fine by me too. Thanks for a sanity-check.\n\nSo here's that endgame patch. My main concern with it was that there\nmight be non-Linux systems that could be affected. But when I dug into\nit, I found that this code was never activated anywhere besides Linux in\nthe first place. So I really doubt this will have any negative impact at\nall. I certainly don't mind cooking it until post-2.11, though.\n\n+cc Linus as the original author of 144bde78e9 in case there is\nsomething subtle I'm missing, but this really just seems like it's\nan outdated optimization.\n\n-- >8 --\nSubject: [PATCH] sha1_file: stop opening files with O_NOATIME\n\nWhen we open object files, we try to do so with O_NOATIME.\nThis dates back to 144bde78e9 (Use O_NOATIME when opening\nthe sha1 files., 2005-04-23), which is an optimization to\navoid creating a bunch of dirty inodes when we're accessing\nmany objects. But a few things have changed since then:\n\n 1. In June 2005, git learned about packfiles, which means\n we would do a lot fewer atime updates (rather than one\n per object access, we'd generally get one per packfile).\n\n 2. In late 2006, Linux learned about \"relatime\", which is\n generally the default on modern installs. So\n performance around atimes updates is a non-issue there\n these days.\n\n All the world isn't Linux, but as it turns out, Linux\n is the only platform to implement O_NOATIME in the\n first place.\n\nSo it's very unlikely that this code is helping anybody\nthese days.\n\nIt's not a particularly large amount of code, but the\nfallback-retry creates complexity. E.g., we do a similar\nfallback for CLOEXEC; which one should take precedence, or\nshould we try all possible combinations? Dropping O_NOATIME\nmakes those questions go away.\n\nSigned-off-by: Jeff King \n---\n sha1_file.c | 15 +--------------\n 1 file changed, 1 insertion(+), 14 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 09045df1dc..6f02a57d8b 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -27,14 +27,6 @@\n #include \"list.h\"\n #include \"mergesort.h\"\n \n-#ifndef O_NOATIME\n-#if defined(__linux__) && (defined(__i386__) || defined(__PPC__))\n-#define O_NOATIME 01000000\n-#else\n-#define O_NOATIME 0\n-#endif\n-#endif\n-\n #define SZ_FMT PRIuMAX\n static inline uintmax_t sz_fmt(size_t s) { return s; }\n \n@@ -1561,7 +1553,7 @@ int check_sha1_signature(const unsigned char *sha1, void *map,\n \n int git_open(const char *name)\n {\n-\tstatic int sha1_file_open_flag = O_NOATIME | O_CLOEXEC;\n+\tstatic int sha1_file_open_flag = O_CLOEXEC;\n \n \tfor (;;) {\n \t\tint fd;\n@@ -1577,11 +1569,6 @@ int git_open(const char *name)\n \t\t\tcontinue;\n \t\t}\n \n-\t\t/* Might the failure be due to O_NOATIME? */\n-\t\tif (errno != ENOENT && (sha1_file_open_flag & O_NOATIME)) {\n-\t\t\tsha1_file_open_flag &= ~O_NOATIME;\n-\t\t\tcontinue;\n-\t\t}\n \t\treturn -1;\n \t}\n }\n-- \n2.10.1.916.g0d2035c\n\n"},{"id":"304999","messageId":"xmqq60od5up3.fsf@gitster.mtv.corp.google.com","threadId":"44366","inReplyTo":"20161027102419.dbzigj7wtr355ofh@sigill.intra.peff.net","subject":"Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-27T21:49:12Z","receivedAt":"2016-10-27T21:49:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> +cc Linus as the original author of 144bde78e9 in case there is\n> something subtle I'm missing, but this really just seems like it's\n> an outdated optimization.\n>\n> -- >8 --\n> Subject: [PATCH] sha1_file: stop opening files with O_NOATIME\n>\n> When we open object files, we try to do so with O_NOATIME.\n> This dates back to 144bde78e9 (Use O_NOATIME when opening\n> the sha1 files., 2005-04-23), which is an optimization to\n> avoid creating a bunch of dirty inodes when we're accessing\n> many objects. But a few things have changed since then:\n>\n> 1. In June 2005, git learned about packfiles, which means\n> we would do a lot fewer atime updates (rather than one\n> per object access, we'd generally get one per packfile).\n>\n> 2. In late 2006, Linux learned about \"relatime\", which is\n> generally the default on modern installs. So\n> performance around atimes updates is a non-issue there\n> these days.\n>\n> All the world isn't Linux, but as it turns out, Linux\n> is the only platform to implement O_NOATIME in the\n> first place.\n>\n> So it's very unlikely that this code is helping anybody\n> these days.\n>\n> It's not a particularly large amount of code, but the\n> fallback-retry creates complexity. E.g., we do a similar\n> fallback for CLOEXEC; which one should take precedence, or\n> should we try all possible combinations? Dropping O_NOATIME\n> makes those questions go away.\n>\n> Signed-off-by: Jeff King \n> ---\n\nWe may want to lose the surrounding for (;;) loop as there is only\none flag to retry without, which was the original code structure\nback when 144bde78e9 (\"Use O_NOATIME when opening the sha1 files.\",\n2005-04-23) was written and refactored by 44d1c19ee8 (\"Make loose\nobject file reading more careful\", 2008-06-14).\n\nIOW, this on top. The update to ce_compare_data() Lars has in\na0a6cb9662 (\"read-cache: make sure file handles are not inherited by\nchild processes\", 2016-10-24) could then made into a call to\ngit_open().\n\n sha1_file.c | 23 ++++++++---------------\n 1 file changed, 8 insertions(+), 15 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 6f02a57d8b..e18ea053e6 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1553,24 +1553,17 @@ int check_sha1_signature(const unsigned char *sha1, void *map,\n \n int git_open(const char *name)\n {\n-\tstatic int sha1_file_open_flag = O_CLOEXEC;\n-\n-\tfor (;;) {\n-\t\tint fd;\n-\n-\t\terrno = 0;\n-\t\tfd = open(name, O_RDONLY | sha1_file_open_flag);\n-\t\tif (fd >= 0)\n-\t\t\treturn fd;\n+\tstatic int cloexec = O_CLOEXEC;\n+\tint fd;\n \n+\terrno = 0;\n+\tfd = open(name, O_RDONLY | cloexec);\n+\tif ((cloexec & O_CLOEXEC) && fd < 0 && errno == EINVAL) {\n \t\t/* Try again w/o O_CLOEXEC: the kernel might not support it */\n-\t\tif ((sha1_file_open_flag & O_CLOEXEC) && errno == EINVAL) {\n-\t\t\tsha1_file_open_flag &= ~O_CLOEXEC;\n-\t\t\tcontinue;\n-\t\t}\n-\n-\t\treturn -1;\n+\t\tcloexec &= ~O_CLOEXEC;\n+\t\tfd = open(name, O_RDONLY | cloexec);\n \t}\n+\treturn fd;\n }\n \n static int stat_sha1_file(const unsigned char *sha1, struct stat *st)\n"},{"id":"305013","messageId":"CA+55aFwfhFqV74s_O=GucycY9U19ysiACDqX=mK4Gf=eQ0coxQ@mail.gmail.com","threadId":"44366","inReplyTo":"20161027102419.dbzigj7wtr355ofh@sigill.intra.peff.net","subject":"Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2016-10-27T22:38:59Z","receivedAt":"2016-10-27T22:39:04Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Thu, Oct 27, 2016 at 3:24 AM, Jeff King wrote:\n>\n> +cc Linus as the original author of 144bde78e9 in case there is\n> something subtle I'm missing, but this really just seems like it's\n> an outdated optimization.\n\nI'd *really* like to keep O_NOATIME if at all possible. It made a huge\ndifference on older kernels, and I'm not convinced that relatime\nreally fixes it as well as O_NOATIME.\n\nThere are people who don't like relatime. And even if you do have\nrelatime enabled, it will update atime once every day, so then this\nmakes your filesystem have a storm of nasty inode writebacks if you\nhaven't touched that git repo in a while.\n\nIf this is purely about mixing things up with O_CLOEXEC, then that is\n*trivially* fixable by just using\n\n fcntl(fd, F_SETFD, FD_CLOEXEC);\n\nafter the open.\n\nWhich is what you have to do anyway if you want to be portable, so\nits' not like you could avoid that complexity in the first place.\n\nNote that you can *not* do the same thing with O_NOATIME, since the\nwhole point of O_NOATIME is that it changes the behavior of the open\nitself (unlike O_CLOEXEC which changes _later_ behavior, and can\nalways be replaced by FD_CLOEXEC fnclt modulo races that are\nimmaterial for git)\n\n Linus\n"},{"id":"305024","messageId":"xmqqoa254czs.fsf@gitster.mtv.corp.google.com","threadId":"44366","inReplyTo":"CA+55aFwfhFqV74s_O=GucycY9U19ysiACDqX=mK4Gf=eQ0coxQ@mail.gmail.com","subject":"Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-27T22:56:55Z","receivedAt":"2016-10-27T22:57:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> Note that you can *not* do the same thing with O_NOATIME, since the\n> whole point of O_NOATIME is that it changes the behavior of the open\n> itself (unlike O_CLOEXEC which changes _later_ behavior, and can\n> always be replaced by FD_CLOEXEC fnclt modulo races that are\n> immaterial for git)\n\nA tangent. \n\nI also thought O_NOATIME shouldn't be an effective fcntl(2) thing,\nfor the reasons you stated above, when I was studying the area\nbecause of the discussion on these patches. I was surprised to see\nthat http://man7.org/linux/man-pages/man2/fcntl.2.html contradicts\nby saying F_SETFL can take O_NOATIME. \n\nPerhaps it deserves a bugreport?\n\n\n"},{"id":"305026","messageId":"CA+55aFxTHF4BRfcrCiV1D26-be+_rPhwAV+Vq8Roz-NMpPBadg@mail.gmail.com","threadId":"44366","inReplyTo":"xmqqoa254czs.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2016-10-27T23:09:39Z","receivedAt":"2016-10-27T23:09:45Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Thu, Oct 27, 2016 at 3:56 PM, Junio C Hamano wrote:\n>\n> I also thought O_NOATIME shouldn't be an effective fcntl(2) thing,\n> for the reasons you stated above, when I was studying the area\n> because of the discussion on these patches. I was surprised to see\n> that http://man7.org/linux/man-pages/man2/fcntl.2.html contradicts\n> by saying F_SETFL can take O_NOATIME.\n>\n> Perhaps it deserves a bugreport?\n\nIt's not a bug per se.\n\nYou can indeed change O_NOATIME with F_SETFL. And it actually does\nmatter, since atime is normally changed by mmap(), read() and write()\ncalls.\n\nSo F_SETFL O_NOATIME actually does make sense, but it doesn't cover\nthe atime update done by open() itself.\n\nThat said, now that I think about it, I should double-check: maybe\nopen() doesn't actually set atime at all, and we *could* do NOATIME\nwith SETFL after all.\n\nThe exact [acm]time semantics are damn subtle.\n\n Linus\n"},{"id":"305028","messageId":"CA+55aFxdy4maom8byH0FoBBMWx+sQB8J7uWvHOxswjiaAhSjVg@mail.gmail.com","threadId":"44366","inReplyTo":"CA+55aFxTHF4BRfcrCiV1D26-be+_rPhwAV+Vq8Roz-NMpPBadg@mail.gmail.com","subject":"Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2016-10-27T23:19:37Z","receivedAt":"2016-10-27T23:19:43Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Thu, Oct 27, 2016 at 4:09 PM, Linus Torvalds\n wrote:\n>\n> That said, now that I think about it, I should double-check: maybe\n> open() doesn't actually set atime at all, and we *could* do NOATIME\n> with SETFL after all.\n\nChecked. Yup. O_NOATIME could easily be done with SETFL:, because as\nwith O_CLOEXEC, it only affects operations _after_ the open. The open\nitself doesn't set the access time.\n\nSo I was full of it.\n\nBut the basic issue still remains - I'd really prefer to have NOATIME\nstay around for all those poor misguided souls that for some reason\ndon't like \"relatime\" or run old kernels. But whether it is with\nO_NOATIME at open time or with F_SETFL, I don't care.\n\n Linus\n"},{"id":"305031","messageId":"xmqqfunh4b63.fsf@gitster.mtv.corp.google.com","threadId":"44366","inReplyTo":"CA+55aFxdy4maom8byH0FoBBMWx+sQB8J7uWvHOxswjiaAhSjVg@mail.gmail.com","subject":"Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-27T23:36:20Z","receivedAt":"2016-10-27T23:36:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> But the basic issue still remains - I'd really prefer to have NOATIME\n> stay around for all those poor misguided souls that for some reason\n> don't like \"relatime\" or run old kernels. But whether it is with\n> O_NOATIME at open time or with F_SETFL, I don't care.\n\nUnderstood. \n\nWould the best endgame shape for this function be to open with\nO_NOATIME (and retry without), and then add CLOEXEC with fcntl(2)\nbut ignoring an error from it, I guess? That would be the closest\nto what we historically had, I would think.\n\n\n\n"},{"id":"305032","messageId":"CA+55aFw83E+zOd+z5h-CA-3NhrLjVr-anL6pubrSWttYx3zu8g@mail.gmail.com","threadId":"44366","inReplyTo":"xmqqfunh4b63.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2016-10-27T23:44:14Z","receivedAt":"2016-10-27T23:44:21Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Thu, Oct 27, 2016 at 4:36 PM, Junio C Hamano wrote:\n>\n> Would the best endgame shape for this function be to open with\n> O_NOATIME (and retry without), and then add CLOEXEC with fcntl(2)\n> but ignoring an error from it, I guess? That would be the closest\n> to what we historically had, I would think.\n\nI think that's the best model.\n\nNote that the O_NOATIME code is very much designed to try O_NOATIME\nonly _once_. Because even when the kernel supports O_NOATIME, if you\nhave a shared object tree where you may not be the owner of all the\nfiles, a O_NOATIME open can fail with NOPERM (\"You are not allowed to\nhide your accesses to this file\").\n\nThis is why it uses that\n\n static unsigned int sha1_file_open_flag = O_NOATIME;\n\nand if the O_NOATIME open ever fails (and the no-O_NOATIME open\nsucceeds), it clears that flag. Exactly so that it will *not* end up\nin some kind of \"let's open and fail and re-open\" loop. It's designed\nto fail once.\n\nOr at least that's how it used to be originally. This code has\nobviously changed since that early design. Now it seems to clear it\nfor any non-ENOENT error. Which looks fine too.\n\n Linus\n"},{"id":"305034","messageId":"xmqqa8dp46wx.fsf@gitster.mtv.corp.google.com","threadId":"44366","inReplyTo":"CA+55aFw83E+zOd+z5h-CA-3NhrLjVr-anL6pubrSWttYx3zu8g@mail.gmail.com","subject":"Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-28T01:08:14Z","receivedAt":"2016-10-28T01:08:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> On Thu, Oct 27, 2016 at 4:36 PM, Junio C Hamano wrote:\n>>\n>> Would the best endgame shape for this function be to open with\n>> O_NOATIME (and retry without), and then add CLOEXEC with fcntl(2)\n>> but ignoring an error from it, I guess? That would be the closest\n>> to what we historically had, I would think.\n>\n> I think that's the best model.\n\nOK, so perhaps like this.\n\n-- >8 --\nSubject: git_open(): untangle possible NOATIME and CLOEXEC interactions\n\nThe way we structured the fallback-retry for opening with O_NOATIME\nand O_CLOEXEC meant that if we failed due to lack of support to open\nthe file with O_NOATIME option (i.e. EINVAL), we would still try to\ndrop O_CLOEXEC first and retry, and then drop O_NOATIME. A platform\non which O_NOATIME is defined in the header without support from the\nkernel wouldn't have a chance to open with O_CLOEXEC option due to\nthis code structure.\n\nArguably, O_CLOEXEC is more important than O_NOATIME, as the latter\nis mostly about performance, while the former can affect correctness.\nLet's revert the recent changes to the way git_open() attempts to\nopen a file with O_NOATIME and retries without to the original\nsequence, and then use a separate fcntl(fd, F_SETFD, FD_CLOEXEC) on\nthe resulting file descriptor. The helper to do the latter can be\nusable in the codepath in ce_compare_data() that was recently added\nto open a file descriptor with O_CLOEXEC, so let's refactor that\ncodepath with the helper while we are at it.\n\nSigned-off-by: Junio C Hamano \n---\n git-compat-util.h | 5 +++--\n read-cache.c | 12 ++++--------\n sha1_file.c | 49 ++++++++++++++++++++++++++++++-------------------\n 3 files changed, 37 insertions(+), 29 deletions(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 43718dabae..a751630db5 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -679,9 +679,10 @@ char *gitstrdup(const char *s);\n #define getpagesize() sysconf(_SC_PAGESIZE)\n #endif\n \n-#ifndef O_CLOEXEC\n-#define O_CLOEXEC 0\n+#ifndef FD_CLOEXEC\n+#define FD_CLOEXEC 0\n #endif\n+extern int git_set_cloexec(int);\n \n #ifdef FREAD_READS_DIRECTORIES\n #ifdef fopen\ndiff --git a/read-cache.c b/read-cache.c\nindex db5d910642..fb91514885 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -156,17 +156,13 @@ void fill_stat_cache_info(struct cache_entry *ce, struct stat *st)\n static int ce_compare_data(const struct cache_entry *ce, struct stat *st)\n {\n \tint match = -1;\n-\tstatic int cloexec = O_CLOEXEC;\n-\tint fd = open(ce->name, O_RDONLY | cloexec);\n-\n-\tif ((cloexec & O_CLOEXEC) && fd < 0 && errno == EINVAL) {\n-\t\t/* Try again w/o O_CLOEXEC: the kernel might not support it */\n-\t\tcloexec &= ~O_CLOEXEC;\n-\t\tfd = open(ce->name, O_RDONLY | cloexec);\n-\t}\n+\tint fd = open(ce->name, O_RDONLY);\n \n \tif (fd >= 0) {\n \t\tunsigned char sha1[20];\n+\n+\t\t/* do not let child processes to hold onto the open fd */\n+\t\tgit_set_cloexec(fd);\n \t\tif (!index_fd(sha1, fd, st, OBJ_BLOB, ce->name, 0))\n \t\t\tmatch = hashcmp(sha1, ce->oid.hash);\n \t\t/* index_fd() closed the file descriptor already */\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 09045df1dc..41383a6c20 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1559,31 +1559,42 @@ int check_sha1_signature(const unsigned char *sha1, void *map,\n \treturn hashcmp(sha1, real_sha1) ? -1 : 0;\n }\n \n-int git_open(const char *name)\n+int git_set_cloexec(int fd)\n {\n-\tstatic int sha1_file_open_flag = O_NOATIME | O_CLOEXEC;\n+\tstatic int cloexec = FD_CLOEXEC;\n \n-\tfor (;;) {\n-\t\tint fd;\n+\tif (cloexec) {\n+\t\tif (fcntl(fd, F_SETFD, FD_CLOEXEC) < 0)\n+\t\t\tcloexec = 0;\n+\t\t/*\n+\t\t * We might want to diagnose and complain upon seeing\n+\t\t * an error from this call, but let's keep the same\n+\t\t * behaviour as before for now.\n+\t\t */\n+\t}\n+\treturn 0;\n+}\n \n-\t\terrno = 0;\n-\t\tfd = open(name, O_RDONLY | sha1_file_open_flag);\n-\t\tif (fd >= 0)\n-\t\t\treturn fd;\n+int git_open(const char *name)\n+{\n+\tstatic int noatime = O_NOATIME;\n+\tint fd;\n \n-\t\t/* Try again w/o O_CLOEXEC: the kernel might not support it */\n-\t\tif ((sha1_file_open_flag & O_CLOEXEC) && errno == EINVAL) {\n-\t\t\tsha1_file_open_flag &= ~O_CLOEXEC;\n-\t\t\tcontinue;\n-\t\t}\n+\terrno = 0;\n+\tfd = open(name, O_RDONLY | noatime);\n \n-\t\t/* Might the failure be due to O_NOATIME? */\n-\t\tif (errno != ENOENT && (sha1_file_open_flag & O_NOATIME)) {\n-\t\t\tsha1_file_open_flag &= ~O_NOATIME;\n-\t\t\tcontinue;\n-\t\t}\n-\t\treturn -1;\n+\t/* Might the failure be due to O_NOATIME? */\n+\tif ((noatime & O_NOATIME) && errno != ENOENT) {\n+\t\tnoatime = 0;\n+\t\tfd = open(name, O_RDONLY);\n \t}\n+\n+\tif (fd < 0)\n+\t\treturn fd;\n+\n+\t/* do not let child processes to hold onto the open fd */\n+\tgit_set_cloexec(fd);\n+\treturn fd;\n }\n \n static int stat_sha1_file(const unsigned char *sha1, struct stat *st)\n"},{"id":"305035","messageId":"xmqq60od42s0.fsf@gitster.mtv.corp.google.com","threadId":"44366","inReplyTo":"xmqqa8dp46wx.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-28T02:37:35Z","receivedAt":"2016-10-28T02:37:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Linus Torvalds writes:\n>\n>> On Thu, Oct 27, 2016 at 4:36 PM, Junio C Hamano wrote:\n>>>\n>>> Would the best endgame shape for this function be to open with\n>>> O_NOATIME (and retry without), and then add CLOEXEC with fcntl(2)\n>>> but ignoring an error from it, I guess? That would be the closest\n>>> to what we historically had, I would think.\n>>\n>> I think that's the best model.\n>\n> OK, so perhaps like this.\n\nHmph. This may not fly well in practice, though. \n\nTo Unix folks, CLOEXEC is not a huge correctness issue. A child\nprocess may hold onto an open file descriptor a bit longer than the\nlifetime of the parent but as long as the child eventually exits,\nnothing is affected. Over there, things are different. The parent\ncannot even rename(2) or unlink(2) a file it created and closed\nwhile the child is still holding the file descriptor open and the\nlack of CLOEXEC will make the parent fail. I do not know how well\nfcntl(2) emulation works on Windows, but I would not be surprised\nif J6t or Dscho comes back and says that FD_CLOEXEC given to F_SETFD\nwould not work while O_CLOEXEC given to open(2) does.\n"},{"id":"305040","messageId":"20161028055159.GA25950@starla","threadId":"44366","inReplyTo":"xmqq60od42s0.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2016-10-28T05:51:59Z","receivedAt":"2016-10-28T05:52:05Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n> > Linus Torvalds writes:\n> >\n> >> On Thu, Oct 27, 2016 at 4:36 PM, Junio C Hamano wrote:\n> >>>\n> >>> Would the best endgame shape for this function be to open with\n> >>> O_NOATIME (and retry without), and then add CLOEXEC with fcntl(2)\n> >>> but ignoring an error from it, I guess? That would be the closest\n> >>> to what we historically had, I would think.\n> >>\n> >> I think that's the best model.\n\nActually, I would flip the order of flags. O_CLOEXEC is more\nimportant from a correctness standpoint.\n\n> > OK, so perhaps like this.\n> \n> Hmph. This may not fly well in practice, though. \n> \n> To Unix folks, CLOEXEC is not a huge correctness issue. A child\n> process may hold onto an open file descriptor a bit longer than the\n> lifetime of the parent but as long as the child eventually exits,\n\nI'm not too familiar with C internals of git; but I know we use\nthreads in some places, and fork+execve in others.\n\nIf our usage of threads and execve intersects, and we run\nuntrusted code in an execve-ed child, then only having cloexec\non open() will save us time when auditing for leaking FDs.\n\nfcntl(fd, F_SETFD, O_CLOEXEC) is racy in if there are other\nthreads doing execve; so I wouldn't rely on it as a first\nchoice.\n\nSo I suppose something like this:\n\n\tstatic int noatime = 1;\n\tint fd = open(... | O_CLOEXEC);\n\t...error checking and retrying...\n\n\tif (fd >= 0 && noatime && fcntl(fd, F_SETFL, O_NOATIME) != 0)\n\t\tnoatime = 0;\n\n\treturn fd;\n"},{"id":"305045","messageId":"20161028075104.la24zydnr3ogb6qv@sigill.intra.peff.net","threadId":"44366","inReplyTo":"CA+55aFwfhFqV74s_O=GucycY9U19ysiACDqX=mK4Gf=eQ0coxQ@mail.gmail.com","subject":"Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-28T07:51:05Z","receivedAt":"2016-10-28T07:51:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 27, 2016 at 03:38:59PM -0700, Linus Torvalds wrote:\n\n> On Thu, Oct 27, 2016 at 3:24 AM, Jeff King wrote:\n> >\n> > +cc Linus as the original author of 144bde78e9 in case there is\n> > something subtle I'm missing, but this really just seems like it's\n> > an outdated optimization.\n> \n> I'd *really* like to keep O_NOATIME if at all possible. It made a huge\n> difference on older kernels, and I'm not convinced that relatime\n> really fixes it as well as O_NOATIME.\n> \n> There are people who don't like relatime. And even if you do have\n> relatime enabled, it will update atime once every day, so then this\n> makes your filesystem have a storm of nasty inode writebacks if you\n> haven't touched that git repo in a while.\n\nThe existence of \"relatime\" is only half the story of its outdatedness.\n\nThe other half is packfiles, so that we are paying atime only once per\npackfile, not once per object (technically once per mmap(), so on a\n32-bit system with large packfiles, it would be multiple, depending on\nyour window size).\n\nSo I'm not convinced that \"storm\" is really the right word in a modern\ncontext. The atime updates due to object accesses are probably smaller\nthan those from all the other read() calls being done on non-object\nfiles (like config, refs, etc).\n\nThat being said, if you really care, it's not that much code to keep.\n\n-Peff\n"},{"id":"305054","messageId":"alpine.DEB.2.20.1610281306320.3264@virtualbox","threadId":"44366","inReplyTo":"xmqq60od42s0.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-10-28T11:11:00Z","receivedAt":"2016-10-28T11:11:33Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 27 Oct 2016, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n> \n> > Linus Torvalds writes:\n> >\n> >> On Thu, Oct 27, 2016 at 4:36 PM, Junio C Hamano wrote:\n> >>>\n> >>> Would the best endgame shape for this function be to open with\n> >>> O_NOATIME (and retry without), and then add CLOEXEC with fcntl(2)\n> >>> but ignoring an error from it, I guess? That would be the closest\n> >>> to what we historically had, I would think.\n> >>\n> >> I think that's the best model.\n> >\n> > OK, so perhaps like this.\n> \n> Hmph. This may not fly well in practice, though. \n> \n> To Unix folks, CLOEXEC is not a huge correctness issue. A child\n> process may hold onto an open file descriptor a bit longer than the\n> lifetime of the parent but as long as the child eventually exits,\n> nothing is affected. Over there, things are different. The parent\n> cannot even rename(2) or unlink(2) a file it created and closed\n> while the child is still holding the file descriptor open and the\n> lack of CLOEXEC will make the parent fail. I do not know how well\n> fcntl(2) emulation works on Windows, but I would not be surprised\n> if J6t or Dscho comes back and says that FD_CLOEXEC given to F_SETFD\n> would not work while O_CLOEXEC given to open(2) does.\n\nYou guys. I mean: You guys! You sure make my life hard. A brief look at\nmingw.h could have answered your implicit question:\n\n\tstatic inline int fcntl(int fd, int cmd, ...)\n\t{\n\t\tif (cmd == F_GETFD || cmd == F_SETFD)\n\t\t\treturn 0;\n\t\terrno = EINVAL;\n\t\treturn -1;\n\t}\n\nSo while you discuss in your Linux Ivory Tower how to optimize Git for\nLinux, and Linux only, I'll have to drop everything else and spend the\nrest of my Friday trying to find a way to adjust a file handle\n*immediately after opening it with undesired flags* (when it could have\nbeen opened with the desired flags, as suggested, to begin with).\n\nCiao,\nJohannes\n"},{"id":"305059","messageId":"xmqqh97w38gj.fsf@gitster.mtv.corp.google.com","threadId":"44366","inReplyTo":"xmqq60od42s0.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-28T13:32:28Z","receivedAt":"2016-10-28T13:32:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Hmph. This may not fly well in practice, though. \n\nWe can flip the order around, and then it becomes simpler to later\ndrop atime support. The first step goes like this.\n\n-- >8 --\nFrom: Junio C Hamano \nDate: Fri, 28 Oct 2016 06:23:07 -0700\nSubject: [PATCH] git_open(): untangle possible NOATIME and CLOEXEC interactions\n\nThe way we structured the fallback/retry mechanism for opening with\nO_NOATIME and O_CLOEXEC meant that if we failed due to lack of\nsupport to open the file with O_NOATIME option (i.e. EINVAL), we\nwould still try to drop O_CLOEXEC first and retry, and then drop\nO_NOATIME. A platform on which O_NOATIME is defined in the header\nwithout support from the kernel wouldn't have a chance to open with\nO_CLOEXEC option due to this code structure.\n\nArguably, O_CLOEXEC is more important than O_NOATIME, as the latter\nis mostly about performance, while the former can affect correctness.\n\nInstead use O_CLOEXEC to open the file, and then use fcntl(2) to set\nO_NOATIME on the resulting file descriptor. open(2) itself does not\ncause atime to be updated according to Linus [*1*].\n\nThe helper to do the former can be usable in the codepath in\nce_compare_data() that was recently added to open a file descriptor\nwith O_CLOEXEC; use it while we are at it.\n\n*1* \n\nSigned-off-by: Junio C Hamano \n---\n cache.h | 1 +\n read-cache.c | 9 +--------\n sha1_file.c | 39 +++++++++++++++++++--------------------\n 3 files changed, 21 insertions(+), 28 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex a902ca1f8e..6f1c21a352 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1122,6 +1122,7 @@ extern int write_sha1_file(const void *buf, unsigned long len, const char *type,\n extern int hash_sha1_file_literally(const void *buf, unsigned long len, const char *type, unsigned char *sha1, unsigned flags);\n extern int pretend_sha1_file(void *, unsigned long, enum object_type, unsigned char *);\n extern int force_object_loose(const unsigned char *sha1, time_t mtime);\n+extern int git_open_cloexec(const char *name, int flags);\n extern int git_open(const char *name);\n extern void *map_sha1_file(const unsigned char *sha1, unsigned long *size);\n extern int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long mapsize, void *buffer, unsigned long bufsiz);\ndiff --git a/read-cache.c b/read-cache.c\nindex db5d910642..c27d3e240b 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -156,14 +156,7 @@ void fill_stat_cache_info(struct cache_entry *ce, struct stat *st)\n static int ce_compare_data(const struct cache_entry *ce, struct stat *st)\n {\n \tint match = -1;\n-\tstatic int cloexec = O_CLOEXEC;\n-\tint fd = open(ce->name, O_RDONLY | cloexec);\n-\n-\tif ((cloexec & O_CLOEXEC) && fd < 0 && errno == EINVAL) {\n-\t\t/* Try again w/o O_CLOEXEC: the kernel might not support it */\n-\t\tcloexec &= ~O_CLOEXEC;\n-\t\tfd = open(ce->name, O_RDONLY | cloexec);\n-\t}\n+\tint fd = git_open_cloexec(ce->name, O_RDONLY);\n \n \tif (fd >= 0) {\n \t\tunsigned char sha1[20];\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 09045df1dc..dfbf398183 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1559,31 +1559,30 @@ int check_sha1_signature(const unsigned char *sha1, void *map,\n \treturn hashcmp(sha1, real_sha1) ? -1 : 0;\n }\n \n-int git_open(const char *name)\n+int git_open_cloexec(const char *name, int flags)\n {\n-\tstatic int sha1_file_open_flag = O_NOATIME | O_CLOEXEC;\n-\n-\tfor (;;) {\n-\t\tint fd;\n-\n-\t\terrno = 0;\n-\t\tfd = open(name, O_RDONLY | sha1_file_open_flag);\n-\t\tif (fd >= 0)\n-\t\t\treturn fd;\n+\tstatic int cloexec = O_CLOEXEC;\n+\tint fd = open(name, flags | cloexec);\n \n+\tif ((cloexec & O_CLOEXEC) && fd < 0 && errno == EINVAL) {\n \t\t/* Try again w/o O_CLOEXEC: the kernel might not support it */\n-\t\tif ((sha1_file_open_flag & O_CLOEXEC) && errno == EINVAL) {\n-\t\t\tsha1_file_open_flag &= ~O_CLOEXEC;\n-\t\t\tcontinue;\n-\t\t}\n+\t\tcloexec &= ~O_CLOEXEC;\n+\t\tfd = open(name, flags | cloexec);\n+\t}\n+\treturn fd;\n+}\n \n-\t\t/* Might the failure be due to O_NOATIME? */\n-\t\tif (errno != ENOENT && (sha1_file_open_flag & O_NOATIME)) {\n-\t\t\tsha1_file_open_flag &= ~O_NOATIME;\n-\t\t\tcontinue;\n-\t\t}\n-\t\treturn -1;\n+int git_open(const char *name)\n+{\n+\tstatic int noatime = O_NOATIME;\n+\tint fd = git_open_cloexec(name, O_RDONLY);\n+\n+\tif (0 <= fd && (noatime & O_NOATIME)) {\n+\t\tint flags = fcntl(fd, F_GETFL);\n+\t\tif (fcntl(fd, F_SETFL, flags | noatime))\n+\t\t\tnoatime = 0;\n \t}\n+\treturn fd;\n }\n \n static int stat_sha1_file(const unsigned char *sha1, struct stat *st)\n-- \n2.10.1-791-g404733b9cf\n\n"},{"id":"305060","messageId":"xmqqd1ik38f4.fsf@gitster.mtv.corp.google.com","threadId":"44366","inReplyTo":"xmqqh97w38gj.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-28T13:33:19Z","receivedAt":"2016-10-28T13:33:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Junio C Hamano writes:\n>\n>> Hmph. This may not fly well in practice, though. \n>\n> We can flip the order around, and then it becomes simpler to later\n> drop atime support. The first step goes like this.\n\nAnd the second step would be like this.\n\n-- >8 --\nSubject: [PATCH] sha1_file: stop opening files with O_NOATIME\n\nWhen we open object files, we try to do so with O_NOATIME.\nThis dates back to 144bde78e9 (Use O_NOATIME when opening\nthe sha1 files., 2005-04-23), which is an optimization to\navoid creating a bunch of dirty inodes when we're accessing\nmany objects. But a few things have changed since then:\n\n 1. In June 2005, git learned about packfiles, which means\n we would do a lot fewer atime updates (rather than one\n per object access, we'd generally get one per packfile).\n\n 2. In late 2006, Linux learned about \"relatime\", which is\n generally the default on modern installs. So\n performance around atimes updates is a non-issue there\n these days.\n\n All the world isn't Linux, but as it turns out, Linux\n is the only platform to implement O_NOATIME in the\n first place.\n\nSo it's very unlikely that this code is helping anybody\nthese days.\n\nHelped-by: Jeff King \n[jc: took idea and log message from peff]\nSigned-off-by: Junio C Hamano \n---\n cache.h | 2 +-\n sha1_file.c | 21 ---------------------\n 2 files changed, 1 insertion(+), 22 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 6f1c21a352..f440d3fd1e 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1123,7 +1123,7 @@ extern int hash_sha1_file_literally(const void *buf, unsigned long len, const ch\n extern int pretend_sha1_file(void *, unsigned long, enum object_type, unsigned char *);\n extern int force_object_loose(const unsigned char *sha1, time_t mtime);\n extern int git_open_cloexec(const char *name, int flags);\n-extern int git_open(const char *name);\n+#define git_open(name) git_open_cloexec(name, O_RDONLY)\n extern void *map_sha1_file(const unsigned char *sha1, unsigned long *size);\n extern int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long mapsize, void *buffer, unsigned long bufsiz);\n extern int parse_sha1_header(const char *hdr, unsigned long *sizep);\ndiff --git a/sha1_file.c b/sha1_file.c\nindex dfbf398183..25d9359402 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -27,14 +27,6 @@\n #include \"list.h\"\n #include \"mergesort.h\"\n \n-#ifndef O_NOATIME\n-#if defined(__linux__) && (defined(__i386__) || defined(__PPC__))\n-#define O_NOATIME 01000000\n-#else\n-#define O_NOATIME 0\n-#endif\n-#endif\n-\n #define SZ_FMT PRIuMAX\n static inline uintmax_t sz_fmt(size_t s) { return s; }\n \n@@ -1572,19 +1564,6 @@ int git_open_cloexec(const char *name, int flags)\n \treturn fd;\n }\n \n-int git_open(const char *name)\n-{\n-\tstatic int noatime = O_NOATIME;\n-\tint fd = git_open_cloexec(name, O_RDONLY);\n-\n-\tif (0 <= fd && (noatime & O_NOATIME)) {\n-\t\tint flags = fcntl(fd, F_GETFL);\n-\t\tif (fcntl(fd, F_SETFL, flags | noatime))\n-\t\t\tnoatime = 0;\n-\t}\n-\treturn fd;\n-}\n-\n static int stat_sha1_file(const unsigned char *sha1, struct stat *st)\n {\n \tstruct alternate_object_database *alt;\n-- \n2.10.1-791-g404733b9cf\n\n"},{"id":"305066","messageId":"CA+55aFw93vkraxBvFCXFSYJqn836tXW+OCOFuToN+HaxTcJ7cg@mail.gmail.com","threadId":"44366","inReplyTo":"alpine.DEB.2.20.1610281306320.3264@virtualbox","subject":"Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2016-10-28T16:13:41Z","receivedAt":"2016-10-28T16:13:51Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Fri, Oct 28, 2016 at 4:11 AM, Johannes Schindelin\n wrote:\n>\n> You guys. I mean: You guys! You sure make my life hard. A brief look at\n> mingw.h could have answered your implicit question:\n\nSo here's what you guys should do:\n\n - leave O_NOATIME damn well alone. It works. It has worked for 10+\nyears. Stop arguing against it, people who do.\n\n - get rid of all O_CLOEXEC games. They don't work. If you want to\nclose file descriptors at execve(), you - gasp - close the file\ndescriptor before doing an execve.\n\nSo O_CLOEXEC or FD_CLOEXEC is broken.\n\nDO NOT BREAK O_NOATIME JUST TO ADD COMPLETELY NEW BREAKAGE.\n\n Linus\n"},{"id":"305070","messageId":"xmqqshrg1ksv.fsf@gitster.mtv.corp.google.com","threadId":"44366","inReplyTo":"CA+55aFw93vkraxBvFCXFSYJqn836tXW+OCOFuToN+HaxTcJ7cg@mail.gmail.com","subject":"Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-28T16:48:48Z","receivedAt":"2016-10-28T16:48:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> On Fri, Oct 28, 2016 at 4:11 AM, Johannes Schindelin\n> wrote:\n>>\n>> You guys. I mean: You guys! You sure make my life hard. A brief look at\n>> mingw.h could have answered your implicit question:\n>\n> So here's what you guys should do:\n>\n> - leave O_NOATIME damn well alone. It works. It has worked for 10+\n> years. Stop arguing against it, people who do.\n>\n> - get rid of all O_CLOEXEC games. They don't work. If you want to\n> close file descriptors at execve(), you - gasp - close the file\n> descriptor before doing an execve.\n>\n> So O_CLOEXEC or FD_CLOEXEC is broken.\n>\n> DO NOT BREAK O_NOATIME JUST TO ADD COMPLETELY NEW BREAKAGE.\n\nExcuse me, but care to elaborate a bit more?\n\nDid I botch the way I ask fcntl(2) to set O_NOATIME in *1*? Its\nfollow-up in *2* is optional and as peff said in *3*, I do not think\nwe mind dropping that step and keeping O_NOATIME. It is not that\nmuch code to keep.\n\nSetting FD_CLOEXEC with fcntl(2) may be racy, but in what way\nit is broken to open(2) with O_CLOEXEC?\n\nIf we want to close file descriptors we opened at execve() time\nourselves, we'd need to somehow keep track of which file descriptors\nare meant to be closed upon execve() time, and one way to mark that\nmay be to use O_CLOEXEC, but once we mark a file descriptor with the\nbit, execve() would take care of \"closing\" it for us---wouldn't that\nbe the whole point of that bit?\n\n\n*1* http://public-inbox.org/git/xmqqh97w38gj.fsf@gitster.mtv.corp.google.com\n*2* http://public-inbox.org/git/xmqqd1ik38f4.fsf@gitster.mtv.corp.google.com\n*3* http://public-inbox.org/git/20161028075104.la24zydnr3ogb6qv@sigill.intra.peff.net\n"},{"id":"305075","messageId":"CA+55aFwUEzfvWVSZfhBi85QaKWSo-gVMOk1BJFrR0ZsdCRHRsg@mail.gmail.com","threadId":"44366","inReplyTo":"xmqqshrg1ksv.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2016-10-28T17:38:03Z","receivedAt":"2016-10-28T17:38:09Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Fri, Oct 28, 2016 at 9:48 AM, Junio C Hamano wrote:\n>\n> Excuse me, but care to elaborate a bit more?\n\nIt just seems entirely pointless to change the games with O_xyz.\n\n> Setting FD_CLOEXEC with fcntl(2) may be racy, but in what way\n> it is broken to open(2) with O_CLOEXEC?\n\nApparently windows doesn't even support it, at least not mingw. So\nyou'll have to have some supprt for just explicitly closing things at\nexecve anyway. And if you do that for windows, then what's the point\nof using O_CLOEXEC on Linux, and having two totally different paths\nthat will just mean maintenance headache.\n\nIn contrast, O_NOATIME isn't a maintenance problem, since it's purely\nan optimization and has no semantic difference, so it not existing on\nsome platform is immaterial.\n\nEnd result: leave O_NOATIME as it is (working), and just forget about O_CLOEXEC.\n\nI have no actual idea about mingw support for this, but Johannes'\nemail certainly implies it's painful on Windows. Some googling also\nshows people doing things like\n\n #ifdef O_CLOEXEC\n flags |= O_CLOEXEC;\n #endif\n\nor\n\n #ifndef O_CLOEXEC\n #define O_CLOEXEC 0\n #endif\n\nso O_CLOEXEC definitely isn't considered portable. Do you really want\nto rely on it?\n\n Linus\n"},{"id":"305076","messageId":"xmqq60oc1i3a.fsf@gitster.mtv.corp.google.com","threadId":"44366","inReplyTo":"CA+55aFwUEzfvWVSZfhBi85QaKWSo-gVMOk1BJFrR0ZsdCRHRsg@mail.gmail.com","subject":"Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-28T17:47:21Z","receivedAt":"2016-10-28T17:47:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> On Fri, Oct 28, 2016 at 9:48 AM, Junio C Hamano wrote:\n>>\n>> Setting FD_CLOEXEC with fcntl(2) may be racy, but in what way\n>> it is broken to open(2) with O_CLOEXEC?\n>\n> Apparently windows doesn't even support it, at least not mingw. So\n> you'll have to have some supprt for just explicitly closing things at\n> execve anyway. And if you do that for windows, then what's the point\n> of using O_CLOEXEC on Linux, and having two totally different paths\n> that will just mean maintenance headache.\n\nAh, that's where your reaction comes from. If I understood Dscho\ncorrectly, what he said was that I cannot set FD_CLOEXEC via fcntl()\nemulation they have. It wasn't an objection to O_CLOEXEC. In fact,\nsince 05d1ed6148 (\"mingw: ensure temporary file handles are not\ninherited by child processes\", 2016-08-22), we have been relying on\nopen(2) with O_CLOEXEC even on Windows (by emulating it with\nO_NOINHERIT, which Windows has) on some codepaths.\n\n\n"},{"id":"305149","messageId":"xmqqr370vtba.fsf@gitster.mtv.corp.google.com","threadId":"44366","inReplyTo":"CA+55aFwUEzfvWVSZfhBi85QaKWSo-gVMOk1BJFrR0ZsdCRHRsg@mail.gmail.com","subject":"Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-29T01:26:49Z","receivedAt":"2016-10-29T01:26:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> Apparently windows doesn't even support it, at least not mingw....\n\nAssuming that the above was a misunderstanding, assuming that we can\ndo O_CLOEXEC (but not FD_CLOEXEC) on Windows just fine, where stray\nfile descriptors held open in the children matter more, and ...\n\n> In contrast, O_NOATIME isn't a maintenance problem, since it's purely\n> an optimization and has no semantic difference, so it not existing on\n> some platform is immaterial.\n\n... assuming that I didn't screw up my use of fcntl() to set\nO_NOATIME on a fd opened with O_CLOEXEC, are you OK with the\napproach in patch *1*? We can drop *2* to keep O_NOATIME, which has\nbeen working for those with old kernels with many loose objects, and\nit is not too much code to keep.\n\n*1* http://public-inbox.org/git/xmqqh97w38gj.fsf@gitster.mtv.corp.google.com\n*2* http://public-inbox.org/git/xmqqd1ik38f4.fsf@gitster.mtv.corp.google.com\n"},{"id":"305155","messageId":"alpine.DEB.2.20.1610291022120.3264@virtualbox","threadId":"44366","inReplyTo":"xmqqr370vtba.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-10-29T08:25:03Z","receivedAt":"2016-10-29T08:25:40Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 28 Oct 2016, Junio C Hamano wrote:\n\n> Linus Torvalds writes:\n> \n> > Apparently windows doesn't even support it, at least not mingw....\n> \n> Assuming that the above was a misunderstanding, assuming that we can\n> do O_CLOEXEC (but not FD_CLOEXEC) on Windows just fine, where stray\n> file descriptors held open in the children matter more, and ...\n\nCorrect. We cannot change an open file handle's state (\"FD_CLOEXEC\") on\nWindows, but we can ask open() to open said file handle with the correct\nflag (\"O_CLOEXEC\", which is mapped to O_NOINHERIT on Windows.\n\nAnd for the record: I agree with Junio that we cannot simply drop this\nO_CLOEXEC business.\n\nBecause it was introduced to fix bugs.\n\nThank you for your time,\nJohannes\n"},{"id":"305168","messageId":"CA+55aFxsjiuR8cp9SiPS88OnzmCiNN3B-gybz1CS71avsU8OOw@mail.gmail.com","threadId":"44366","inReplyTo":"alpine.DEB.2.20.1610291022120.3264@virtualbox","subject":"Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2016-10-29T17:06:59Z","receivedAt":"2016-10-29T17:07:07Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Sat, Oct 29, 2016 at 1:25 AM, Johannes Schindelin\n wrote:\n>\n> Correct. We cannot change an open file handle's state (\"FD_CLOEXEC\") on\n> Windows, but we can ask open() to open said file handle with the correct\n> flag (\"O_CLOEXEC\", which is mapped to O_NOINHERIT on Windows.\n\nOk. So then I have no issues with it, and let's use O_CLOEXEC if it\nexists and fcntl(FD_CLOEXEC) if O_CLOEXEC doesn't exist.\n\n Linus\n"},{"id":"305196","messageId":"20161031135601.7immbp44wn7uksvs@sigill.intra.peff.net","threadId":"44366","inReplyTo":"CA+55aFw93vkraxBvFCXFSYJqn836tXW+OCOFuToN+HaxTcJ7cg@mail.gmail.com","subject":"Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-31T13:56:01Z","receivedAt":"2016-10-31T13:56:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 28, 2016 at 09:13:41AM -0700, Linus Torvalds wrote:\n\n> On Fri, Oct 28, 2016 at 4:11 AM, Johannes Schindelin\n> wrote:\n> >\n> > You guys. I mean: You guys! You sure make my life hard. A brief look at\n> > mingw.h could have answered your implicit question:\n> \n> So here's what you guys should do:\n> \n> - leave O_NOATIME damn well alone. It works. It has worked for 10+\n> years. Stop arguing against it, people who do.\n\nFor some definition of worked, perhaps.\n\nIf you set a probe on touch_atime() in the kernel (which is called for\nevery attempt to smudge the atime, regardless of mount options, but is\nskipped when the descriptor was opened with O_NOATIME), you can see the\nimpact. Here's a command I picked because it reads a lot of objects (run\non my git.git clone):\n\n $ perf stat -e probe:touch-atime git log -Sfoo >/dev/null\n\nAnd the probe:touch_atime counts before (stock git) and after (a patch\nto drop O_NOATIME):\n\n before: 22,235\n after: 22,362\n\nSo that's only half a percent difference. And it's on a reasonably messy\nclone that is partway to triggering an auto-repack:\n\n $ git count-objects -v\n count: 6167\n size: 61128\n in-pack: 275773\n packs: 18\n size-pack: 86857\n prune-packable: 25\n garbage: 0\n size-garbage: 0\n\nRunning \"git gc\" drops the probe count to 21,733.\n\nIt makes a bigger difference for some commands (it's more like 10% for\ngit-status). And smaller for others (\"git log -p\" triggers it over\n100,000 times).\n\nOne thing missing in that count is how many of those calls would have\nresulted in an actual disk write. Looking at strace, most of the\nfilesystem activity is opening .gitattributes files, and we end up\nopening the same ones repeatedly (e.g., t/.gitattributes in git.git).\nMultiple hits for a given inode in the same second get coalesced into at\nmost a single disk write.\n\nSo I guess it's possible that it produces a noticeable effect in some\ncases, but I'm still somewhat doubtful. And actually repacking your\nrepository had a greater effect in every case I measured (in addition to\nproviding other speedups).\n\nLike I said, I'm OK keeping O_NOATIME. It's just not that much code. But\nif you really care about the issue of dirtying inodes via atime, you\nshould look into vastly increasing our use of O_NOATIME. Or possibly\nlooking at caching more in the attribute code.\n\n-Peff\n"},{"id":"305200","messageId":"xmqq1sywv2qi.fsf@gitster.mtv.corp.google.com","threadId":"44366","inReplyTo":"CA+55aFxsjiuR8cp9SiPS88OnzmCiNN3B-gybz1CS71avsU8OOw@mail.gmail.com","subject":"Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-31T17:37:41Z","receivedAt":"2016-10-31T17:37:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> On Sat, Oct 29, 2016 at 1:25 AM, Johannes Schindelin\n> wrote:\n>>\n>> Correct. We cannot change an open file handle's state (\"FD_CLOEXEC\") on\n>> Windows, but we can ask open() to open said file handle with the correct\n>> flag (\"O_CLOEXEC\", which is mapped to O_NOINHERIT on Windows.\n>\n> Ok. So then I have no issues with it, and let's use O_CLOEXEC if it\n> exists and fcntl(FD_CLOEXEC) if O_CLOEXEC doesn't exist.\n\nI am not sure if the fallback to FD_CLOEXEC is worth doing, because\nit does not exist on Windows, where leaking an open file descriptor\nto a child process matters more than other platforms [*1*]. \n\nOf course, the world is not all Windows, and there may be other\nplatforms this fallback may help. But there is a chance that some\nother thread may fork between the time we open(2) and the time we\ncall fcntl(2), leaving FD_CLOEXEC ineffective and unreliable, so\nthat's another thing that makes me doubt if FD_CLOEXEC fallback is\nworth doing.\n\nHaving said that, here is a rewrite of [*2*] to use the fallback\nwould look like. After this topic settles, we may want to also\nupdate the tempfile.c::create_tempfile() implementation to use the\nhelper function we use here.\n\n\n[Footnotes]\n\n*1* The call in ce_compare_data() is to see if the contents in the\n working tree file match what is in the index. Perhaps the\n program is \"git checkout another-branch\" that knows this path is\n absent in the branch being switched to and trying to make sure\n we lose no local modification. When the path needs to be passed\n through the clean/smudge filter before hashing to see if the\n working tree contents hashes to the object in the index, we need\n to fork and then after the filter finishes its processing, we\n close the file descriptor and then unlink(2) the path if it is\n clean. We are about to add a variant of clean/smudge mechanism\n where a lazily spawned process can clean contents of multiple\n paths and that is where cloexec starts to matter. This function\n starts to inspect one path, opens a fd to it, finds that it\n needs filtering, spawns a long-lingering process, while leaking\n the original fd to it. After feeding the contents via pipe and\n then receiving the filtered contents (the protocol is not full\n duplex and there won't be a stuck pipe), the parent would decide\n that the path is clean and attempts to unlink(2). Windows does\n not let you unlink a path with open filedescriptor held, causing\n \"checkout\" to fail. As it is only one fd that leaks to the\n child, no matter how many subsequent paths are filtered by the\n same long-lingering child process, it will be a far less\n important issue on other platforms that lets you unlink(2) such\n a file.\n\n*2* http://public-inbox.org/git/xmqqh97w38gj.fsf@gitster.mtv.corp.google.com\n\n---\n cache.h | 1 +\n git-compat-util.h | 4 ++++\n read-cache.c | 9 +--------\n sha1_file.c | 47 ++++++++++++++++++++++++++++-------------------\n 4 files changed, 34 insertions(+), 27 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex a902ca1f8e..6f1c21a352 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1122,6 +1122,7 @@ extern int write_sha1_file(const void *buf, unsigned long len, const char *type,\n extern int hash_sha1_file_literally(const void *buf, unsigned long len, const char *type, unsigned char *sha1, unsigned flags);\n extern int pretend_sha1_file(void *, unsigned long, enum object_type, unsigned char *);\n extern int force_object_loose(const unsigned char *sha1, time_t mtime);\n+extern int git_open_cloexec(const char *name, int flags);\n extern int git_open(const char *name);\n extern void *map_sha1_file(const unsigned char *sha1, unsigned long *size);\n extern int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long mapsize, void *buffer, unsigned long bufsiz);\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 43718dabae..64407c701c 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -683,6 +683,10 @@ char *gitstrdup(const char *s);\n #define O_CLOEXEC 0\n #endif\n \n+#ifndef FD_CLOEXEC\n+#define FD_CLOEXEC 0\n+#endif\n+\n #ifdef FREAD_READS_DIRECTORIES\n #ifdef fopen\n #undef fopen\ndiff --git a/read-cache.c b/read-cache.c\nindex db5d910642..c27d3e240b 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -156,14 +156,7 @@ void fill_stat_cache_info(struct cache_entry *ce, struct stat *st)\n static int ce_compare_data(const struct cache_entry *ce, struct stat *st)\n {\n \tint match = -1;\n-\tstatic int cloexec = O_CLOEXEC;\n-\tint fd = open(ce->name, O_RDONLY | cloexec);\n-\n-\tif ((cloexec & O_CLOEXEC) && fd < 0 && errno == EINVAL) {\n-\t\t/* Try again w/o O_CLOEXEC: the kernel might not support it */\n-\t\tcloexec &= ~O_CLOEXEC;\n-\t\tfd = open(ce->name, O_RDONLY | cloexec);\n-\t}\n+\tint fd = git_open_cloexec(ce->name, O_RDONLY);\n \n \tif (fd >= 0) {\n \t\tunsigned char sha1[20];\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 09045df1dc..fc8613a847 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1559,31 +1559,40 @@ int check_sha1_signature(const unsigned char *sha1, void *map,\n \treturn hashcmp(sha1, real_sha1) ? -1 : 0;\n }\n \n-int git_open(const char *name)\n+int git_open_cloexec(const char *name, int flags)\n {\n-\tstatic int sha1_file_open_flag = O_NOATIME | O_CLOEXEC;\n+\tint fd;\n+\tstatic int o_cloexec = O_CLOEXEC;\n+\tstatic int fd_cloexec = FD_CLOEXEC;\n \n-\tfor (;;) {\n-\t\tint fd;\n+\tfd = open(name, flags | o_cloexec);\n+\tif ((o_cloexec & O_CLOEXEC) && fd < 0 && errno == EINVAL) {\n+\t\t/* Try again w/o O_CLOEXEC: the kernel might not support it */\n+\t\to_cloexec &= ~O_CLOEXEC;\n+\t\tfd = open(name, flags | o_cloexec);\n+\t}\n \n-\t\terrno = 0;\n-\t\tfd = open(name, O_RDONLY | sha1_file_open_flag);\n-\t\tif (fd >= 0)\n-\t\t\treturn fd;\n+\tif (!o_cloexec && 0 <= fd && fd_cloexec) {\n+\t\t/* Opened w/o O_CLOEXEC? try with fcntl(2) to add it */\n+\t\tint flags = fcntl(fd, F_GETFL);\n+\t\tif (fcntl(fd, F_SETFL, flags | fd_cloexec))\n+\t\t\tfd_cloexec = 0;\n+\t}\n \n-\t\t/* Try again w/o O_CLOEXEC: the kernel might not support it */\n-\t\tif ((sha1_file_open_flag & O_CLOEXEC) && errno == EINVAL) {\n-\t\t\tsha1_file_open_flag &= ~O_CLOEXEC;\n-\t\t\tcontinue;\n-\t\t}\n+\treturn fd;\n+}\n \n-\t\t/* Might the failure be due to O_NOATIME? */\n-\t\tif (errno != ENOENT && (sha1_file_open_flag & O_NOATIME)) {\n-\t\t\tsha1_file_open_flag &= ~O_NOATIME;\n-\t\t\tcontinue;\n-\t\t}\n-\t\treturn -1;\n+int git_open(const char *name)\n+{\n+\tstatic int noatime = O_NOATIME;\n+\tint fd = git_open_cloexec(name, O_RDONLY);\n+\n+\tif (0 <= fd && (noatime & O_NOATIME)) {\n+\t\tint flags = fcntl(fd, F_GETFL);\n+\t\tif (fcntl(fd, F_SETFL, flags | noatime))\n+\t\t\tnoatime = 0;\n \t}\n+\treturn fd;\n }\n \n static int stat_sha1_file(const unsigned char *sha1, struct stat *st)\n\n\n"},{"id":"305201","messageId":"xmqqwpgotncb.fsf@gitster.mtv.corp.google.com","threadId":"44366","inReplyTo":"20161031135601.7immbp44wn7uksvs@sigill.intra.peff.net","subject":"Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-31T17:55:32Z","receivedAt":"2016-10-31T17:56:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> If you set a probe on touch_atime() in the kernel (which is called for\n> every attempt to smudge the atime, regardless of mount options, but is\n> skipped when the descriptor was opened with O_NOATIME), you can see the\n> impact. Here's a command I picked because it reads a lot of objects (run\n> on my git.git clone):\n>\n> $ perf stat -e probe:touch-atime git log -Sfoo >/dev/null\n>\n> And the probe:touch_atime counts before (stock git) and after (a patch\n> to drop O_NOATIME):\n>\n> before: 22,235\n> after: 22,362\n>\n> So that's only half a percent difference. And it's on a reasonably messy\n> clone that is partway to triggering an auto-repack:\n> ...\n> So I guess it's possible that it produces a noticeable effect in some\n> cases, but I'm still somewhat doubtful. And actually repacking your\n> repository had a greater effect in every case I measured (in addition to\n> providing other speedups).\n\nLet's keep doubting. I prefer one-step-at-a-time approach to\nthings anyway, and what I plan in the near term are:\n\n * use the \"open() with O_NOATIME|O_CLOEXEC, gradually losing the\n bits during fallback\" approach in the ls/git-open-cloexec topic,\n in order to help ls/filter-process topic be part of the upcoming\n release;\n\n * simplify the logic to the \"open(2) with O_CLOEXEC, set O_NOATIME\n with fcntl(2)\" in jc/git-open-cloexec~1 after 2.11 ships;\n\n * cook \"drop the latter half of setting O_NOATIME\" which is at the\n tip of jc/git-open-cloexec in 'next', and while Linus is looking\n the other way ^W^W^W^W^W^W^W after people had chance to complain\n with numbers, merge it to a future release iff it still looked OK\n to drop O_NOATIME thing.\n"},{"id":"305202","messageId":"20161031180524.ss37h4njdl3on53m@sigill.intra.peff.net","threadId":"44366","inReplyTo":"xmqqwpgotncb.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-31T18:05:24Z","receivedAt":"2016-10-31T18:05:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 31, 2016 at 10:55:32AM -0700, Junio C Hamano wrote:\n\n> > So I guess it's possible that it produces a noticeable effect in some\n> > cases, but I'm still somewhat doubtful. And actually repacking your\n> > repository had a greater effect in every case I measured (in addition to\n> > providing other speedups).\n> \n> Let's keep doubting. I prefer one-step-at-a-time approach to\n> things anyway, and what I plan in the near term are:\n> \n> * use the \"open() with O_NOATIME|O_CLOEXEC, gradually losing the\n> bits during fallback\" approach in the ls/git-open-cloexec topic,\n> in order to help ls/filter-process topic be part of the upcoming\n> release;\n> \n> * simplify the logic to the \"open(2) with O_CLOEXEC, set O_NOATIME\n> with fcntl(2)\" in jc/git-open-cloexec~1 after 2.11 ships;\n> \n> * cook \"drop the latter half of setting O_NOATIME\" which is at the\n> tip of jc/git-open-cloexec in 'next', and while Linus is looking\n> the other way ^W^W^W^W^W^W^W after people had chance to complain\n> with numbers, merge it to a future release iff it still looked OK\n> to drop O_NOATIME thing.\n\nGreat, that sounds like a good way to proceed (and if the final step\nnever happens, no big deal).\n\n-Peff\n"}]}