{"thread":{"id":"40463","subject":"[PATCH/RFC 2/2] sha1_file: set packfile to O_CLOEXEC at open","startedAt":"2015-10-01T03:29:21Z","lastAt":"2015-10-05T09:03:19Z","messageCount":12,"participants":["Max Kirillov","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"270969","messageId":"1443670163-31193-1-git-send-email-max@max630.net","threadId":"40463","inReplyTo":"xmqqlhbn7ky1.fsf@gitster.mtv.corp.google.com","subject":"[PATCH/RFC 0/2] close packs files when they are not needed","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2015-10-01T03:29:21Z","receivedAt":"2015-10-01T03:29:21Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"> The right approach may to have a helper in sha1_file.c that closes\n> and cleans up _all_ packs, and call it from here, instead of having\n> builtin/clone.c even know about implementation details such as\n> packed_git is a linked list, etc. \n\nLike this?\n\nNote I did not test it to actually work for the current\ncode. Trying now what I could do with lsof, just to be sure\nmyself, but probably its use not appropriate for the\nproject.\n\nMax Kirillov (2):\n  sha1_file: close all pack files after running\n  sha1_file: set packfile to O_CLOEXEC at open\n\n builtin/pack-objects.c |  2 +-\n cache.h                |  3 +-\n git.c                  |  2 ++\n pack-bitmap.c          |  2 +-\n sha1_file.c            | 80 +++++++++++++++++++++++++++++++++++---------------\n 5 files changed, 63 insertions(+), 26 deletions(-)\n\n-- \n2.3.4.2801.g3d0809b\n"},{"id":"270970","messageId":"1443670163-31193-2-git-send-email-max@max630.net","threadId":"40463","inReplyTo":"1443670163-31193-1-git-send-email-max@max630.net","subject":"[PATCH/RFC 1/2] sha1_file: close all pack files after running","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2015-10-01T03:29:22Z","receivedAt":"2015-10-01T03:29:22Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"When a builtin has done its job, but waits for pager or not waited\nby its caller and still hanging it keeps pack files opened.\nThis can cause a number of issues, for example on Windows git gc\ncannot remove the packs.\n\nFix this by explicitly closing all pack files and unmapping memory\nfrom the packs after running builtin. Do not die() if the memory region\nis still used though, just print a warning, because failure to close\na file should not prevent the currently running program from finishing\nits task.\n\nSigned-off-by: Max Kirillov <max@max630.net>\n---\n cache.h     |  1 +\n git.c       |  2 ++\n sha1_file.c | 32 +++++++++++++++++++++++++++++---\n 3 files changed, 32 insertions(+), 3 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 79066e5..153bc46 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1279,6 +1279,7 @@ extern void unuse_pack(struct pack_window **);\n extern void free_pack_by_name(const char *);\n extern void clear_delta_base_cache(void);\n extern struct packed_git *add_packed_git(const char *, int, int);\n+extern void close_all_packs(void);\n \n /*\n  * Return the SHA-1 of the nth object within the specified packfile.\ndiff --git a/git.c b/git.c\nindex 5feba41..ad34680 100644\n--- a/git.c\n+++ b/git.c\n@@ -1,4 +1,5 @@\n #include \"builtin.h\"\n+#include \"cache.h\"\n #include \"exec_cmd.h\"\n #include \"help.h\"\n #include \"run-command.h\"\n@@ -348,6 +349,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)\n \ttrace_argv_printf(argv, \"trace: built-in: git\");\n \n \tstatus = p->fn(argc, argv, prefix);\n+\tclose_all_packs();\n \tif (status)\n \t\treturn status;\n \ndiff --git a/sha1_file.c b/sha1_file.c\nindex 08302f5..62f1dad 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -773,20 +773,28 @@ void *xmmap(void *start, size_t length,\n \treturn ret;\n }\n \n-void close_pack_windows(struct packed_git *p)\n+static int close_pack_windows_nodie(struct packed_git *p)\n {\n \twhile (p->windows) {\n \t\tstruct pack_window *w = p->windows;\n \n \t\tif (w->inuse_cnt)\n-\t\t\tdie(\"pack '%s' still has open windows to it\",\n-\t\t\t    p->pack_name);\n+\t\t\treturn 1;\n+\n \t\tmunmap(w->base, w->len);\n \t\tpack_mapped -= w->len;\n \t\tpack_open_windows--;\n \t\tp->windows = w->next;\n \t\tfree(w);\n \t}\n+\n+\treturn 0;\n+}\n+\n+void close_pack_windows(struct packed_git *p)\n+{\n+\tif (close_pack_windows_nodie(p))\n+\t\tdie(\"pack '%s' still has open windows to it\", p->pack_name);\n }\n \n /*\n@@ -866,6 +874,24 @@ static int close_one_pack(void)\n \treturn 0;\n }\n \n+void close_all_packs(void)\n+{\n+\tstruct packed_git *p = NULL;\n+\n+\tfor (p = packed_git; p; p = p->next) {\n+\t\tif (close_pack_windows_nodie(p))\n+\t\t\twarning(\"pack '%s' still has open windows to it\", p->pack_name);\n+\n+\t\tif (p->pack_fd != -1) {\n+\t\t\tif (close(p->pack_fd) != 0)\n+\t\t\t\twarning(\"close(%s) failed: %d\", p->pack_name, errno);\n+\t\t\tp->pack_fd = -1;\n+\t\t}\n+\n+\t\tclose_pack_index(p);\n+\t}\n+}\n+\n void unuse_pack(struct pack_window **w_cursor)\n {\n \tstruct pack_window *w = *w_cursor;\n-- \n2.3.4.2801.g3d0809b\n"},{"id":"270968","messageId":"1443670163-31193-3-git-send-email-max@max630.net","threadId":"40463","inReplyTo":"1443670163-31193-1-git-send-email-max@max630.net","subject":"[PATCH/RFC 2/2] sha1_file: set packfile to O_CLOEXEC at open","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2015-10-01T03:29:23Z","receivedAt":"2015-10-01T03:29:23Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"Windows does not support setting O_CLOEXEC by fcntl,\nbut there is an open flag O_NOINHERIT which results in same\nbehaviour. Use it in git_open_noatime() and also bring\nsetting O_CLOEXEC there also to make it consistent. Rename\nthe function to git_open_noatime_cloexec(), to avoid confusion.\n\nSigned-off-by: Max Kirillov <max@max630.net>\n---\n builtin/pack-objects.c |  2 +-\n cache.h                |  2 +-\n pack-bitmap.c          |  2 +-\n sha1_file.c            | 48 ++++++++++++++++++++++++++++--------------------\n 4 files changed, 31 insertions(+), 23 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 1c63f8f..a8052f4 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -714,7 +714,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_noatime_cloexec(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 153bc46..77ef5ca 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -972,7 +972,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_noatime_cloexec(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 637770a..4a3b23c 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -274,7 +274,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_noatime_cloexec(idx_name);\n \tfree(idx_name);\n \n \tif (fd < 0)\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 62f1dad..2da5de2 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -385,7 +385,7 @@ void read_info_alternates(const char * relative_base, int depth)\n \tint fd;\n \n \tsprintf(path, \"%s/%s\", relative_base, alt_file_name);\n-\tfd = git_open_noatime(path);\n+\tfd = git_open_noatime_cloexec(path);\n \tif (fd < 0)\n \t\treturn;\n \tif (fstat(fd, &st) || (st.st_size == 0)) {\n@@ -575,7 +575,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_noatime_cloexec(path);\n \tstruct stat st;\n \n \tif (fd < 0)\n@@ -995,7 +995,6 @@ static int open_packed_git_1(struct packed_git *p)\n \tstruct pack_header hdr;\n \tunsigned char sha1[20];\n \tunsigned char *idx_sha1;\n-\tlong fd_flag;\n \n \tif (!p->index_data && open_pack_index(p))\n \t\treturn error(\"packfile %s index unavailable\", p->pack_name);\n@@ -1013,7 +1012,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_noatime_cloexec(p->pack_name);\n \tif (p->pack_fd < 0 || fstat(p->pack_fd, &st))\n \t\treturn -1;\n \tpack_open_fds++;\n@@ -1026,16 +1025,6 @@ static int open_packed_git_1(struct packed_git *p)\n \t} else if (p->pack_size != st.st_size)\n \t\treturn error(\"packfile %s size changed\", p->pack_name);\n \n-\t/* We leave these file descriptors open with sliding mmap;\n-\t * there is no point keeping them open across exec(), though.\n-\t */\n-\tfd_flag = fcntl(p->pack_fd, F_GETFD, 0);\n-\tif (fd_flag < 0)\n-\t\treturn error(\"cannot determine file descriptor flags\");\n-\tfd_flag |= FD_CLOEXEC;\n-\tif (fcntl(p->pack_fd, F_SETFD, fd_flag) == -1)\n-\t\treturn error(\"cannot set FD_CLOEXEC\");\n-\n \t/* Verify we recognize this pack file format. */\n \tif (read_in_full(p->pack_fd, &hdr, sizeof(hdr)) != sizeof(hdr))\n \t\treturn error(\"file %s is far too short to be a packfile\", p->pack_name);\n@@ -1515,17 +1504,21 @@ 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_noatime_cloexec(const char *name)\n {\n+#ifdef __WIN32__\n+\tstatic int sha1_file_open_flag = O_NOATIME | O_NOINHERIT;\n+#else\n \tstatic int sha1_file_open_flag = O_NOATIME;\n+\tlong fd_flag;\n+#endif\n+\tint fd;\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+\t\t\tbreak;\n \n \t\t/* Might the failure be due to O_NOATIME? */\n \t\tif (errno != ENOENT && sha1_file_open_flag) {\n@@ -1535,6 +1528,21 @@ int git_open_noatime(const char *name)\n \n \t\treturn -1;\n \t}\n+\n+#ifndef __WIN32__\n+\tfd_flag = fcntl(fd, F_GETFD, 0);\n+\tif (fd_flag < 0) {\n+\t\tclose(fd);\n+\t\treturn error(\"cannot determine file descriptor flags\");\n+\t}\n+\tfd_flag |= FD_CLOEXEC;\n+\tif (fcntl(fd, F_SETFD, fd_flag) == -1) {\n+\t\tclose(fd);\n+\t\treturn error(\"cannot set FD_CLOEXEC\");\n+\t}\n+#endif\n+\n+\treturn fd;\n }\n \n static int stat_sha1_file(const unsigned char *sha1, struct stat *st)\n@@ -1561,7 +1569,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_noatime_cloexec(sha1_file_name(sha1));\n \tif (fd >= 0)\n \t\treturn fd;\n \tmost_interesting_errno = errno;\n@@ -1569,7 +1577,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\tfill_sha1_path(alt->name, sha1);\n-\t\tfd = git_open_noatime(alt->base);\n+\t\tfd = git_open_noatime_cloexec(alt->base);\n \t\tif (fd >= 0)\n \t\t\treturn fd;\n \t\tif (most_interesting_errno == ENOENT)\n-- \n2.3.4.2801.g3d0809b\n"},{"id":"270997","messageId":"33b74e875c7298f67640f5850e88c152@dscho.org","threadId":"40463","inReplyTo":"1443670163-31193-2-git-send-email-max@max630.net","subject":"Re: [PATCH/RFC 1/2] sha1_file: close all pack files after running","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-10-02T10:05:55Z","receivedAt":"2015-10-02T10:05:55Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Max,\n\nOn 2015-10-01 05:29, Max Kirillov wrote:\n> When a builtin has done its job, but waits for pager or not waited\n> by its caller and still hanging it keeps pack files opened.\n> This can cause a number of issues, for example on Windows git gc\n> cannot remove the packs.\n\nI did not experience that issue. In any case, closing the packs after the builtin function has returned might not change anything anyway because we are about to `exit()` and that's that.\n\nSo I would like to skip this:\n\n> diff --git a/git.c b/git.c\n> index 5feba41..ad34680 100644\n> --- a/git.c\n> +++ b/git.c\n> @@ -348,6 +349,7 @@ static int run_builtin(struct cmd_struct *p, int\n> argc, const char **argv)\n>  \ttrace_argv_printf(argv, \"trace: built-in: git\");\n>  \n>  \tstatus = p->fn(argc, argv, prefix);\n> +\tclose_all_packs();\n>  \tif (status)\n>  \t\treturn status;\n>  \n\nAlso, I would move the new declaration directly before `close_pack_windows()`:\n\n> diff --git a/cache.h b/cache.h\n> index 79066e5..153bc46 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -1279,6 +1279,7 @@ extern void unuse_pack(struct pack_window **);\n>  extern void free_pack_by_name(const char *);\n>  extern void clear_delta_base_cache(void);\n>  extern struct packed_git *add_packed_git(const char *, int, int);\n> +extern void close_all_packs(void);\n>  \n>  /*\n>   * Return the SHA-1 of the nth object within the specified packfile.\n\nThe convention in Git seems to call things _gently rather than _nodie:\n\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 08302f5..62f1dad 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -773,20 +773,28 @@ void *xmmap(void *start, size_t length,\n>  \treturn ret;\n>  }\n>  \n> -void close_pack_windows(struct packed_git *p)\n> +static int close_pack_windows_nodie(struct packed_git *p)\n>  {\n>  \twhile (p->windows) {\n>  \t\tstruct pack_window *w = p->windows;\n>  \n>  \t\tif (w->inuse_cnt)\n> -\t\t\tdie(\"pack '%s' still has open windows to it\",\n> -\t\t\t    p->pack_name);\n> +\t\t\treturn 1;\n> +\n>  \t\tmunmap(w->base, w->len);\n>  \t\tpack_mapped -= w->len;\n>  \t\tpack_open_windows--;\n>  \t\tp->windows = w->next;\n>  \t\tfree(w);\n>  \t}\n> +\n> +\treturn 0;\n> +}\n\nAnd while we're at it, why not teach that function a new parameter `int close_pack_fd`?\n\nThere is another problem: when we cannot close the pack window, we cannot really continue, can we? Because if we do, we *still* have the lock, and we'll just fail later, most likely with an unhelpful error message because we do not know where the pack closing failed. I do not think that the warning really helps...\n\nCiao,\nDscho\n"},{"id":"270998","messageId":"16e4ef29524283f51b2fa00e536cf153@dscho.org","threadId":"40463","inReplyTo":"1443670163-31193-3-git-send-email-max@max630.net","subject":"Re: [PATCH/RFC 2/2] sha1_file: set packfile to O_CLOEXEC at open","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-10-02T10:08:24Z","receivedAt":"2015-10-02T10:08:24Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Max,\n\nOn 2015-10-01 05:29, Max Kirillov wrote:\n> Windows does not support setting O_CLOEXEC by fcntl,\n> but there is an open flag O_NOINHERIT which results in same\n> behaviour. Use it in git_open_noatime() and also bring\n> setting O_CLOEXEC there also to make it consistent. Rename\n> the function to git_open_noatime_cloexec(), to avoid confusion.\n\nWhich problem does this solve? I am actually suspecting that this will rather cause problems because now `exec()`ing children might cause bogus file descriptors to be still held on.\n\nSo I would recommend to drop this patch. It is not needed to fix the reported bug.\n\nCiao,\nDscho\n"},{"id":"270999","messageId":"763ac2b63d56d250e5e1a27f490f79db@dscho.org","threadId":"40463","inReplyTo":"33b74e875c7298f67640f5850e88c152@dscho.org","subject":"Re: [PATCH/RFC 1/2] sha1_file: close all pack files after running","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-10-02T10:13:40Z","receivedAt":"2015-10-02T10:13:40Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Max,\n\nOn 2015-10-02 12:05, Johannes Schindelin wrote:\n\n> On 2015-10-01 05:29, Max Kirillov wrote:\n>> When a builtin has done its job, but waits for pager or not waited\n>> by its caller and still hanging it keeps pack files opened.\n>> This can cause a number of issues, for example on Windows git gc\n>> cannot remove the packs.\n\nCould you do me another favor? It seems that you want to work on this, so I will step back (I have to take off for the weekend very soon anyway, so I am really glad that you take care of it). But I would really love to see the line\n\nThis fixes https://github.com/git-for-windows/git/issues/446\n\nin the commit message, as this will keep the connection between the fix and the original report.\n\nThanks,\nDscho\n"},{"id":"271029","messageId":"20151002190646.GC26154@wheezy.local","threadId":"40463","inReplyTo":"33b74e875c7298f67640f5850e88c152@dscho.org","subject":"Re: [PATCH/RFC 1/2] sha1_file: close all pack files after running","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2015-10-02T19:06:46Z","receivedAt":"2015-10-02T19:06:46Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"On Fri, Oct 02, 2015 at 12:05:55PM +0200, Johannes Schindelin wrote:\n> Hi Max,\n> \n> On 2015-10-01 05:29, Max Kirillov wrote:\n>> When a builtin has done its job, but waits for pager or not waited\n>> by its caller and still hanging it keeps pack files opened.\n>> This can cause a number of issues, for example on Windows git gc\n>> cannot remove the packs.\n> \n> I did not experience that issue. In any case, closing the\n> packs after the builtin function has returned might not\n> change anything anyway because we are about to `exit()`\n> and that's that.\n\nSteps to reproduce:\n1. install the latest git-for-windows, 2.6.0.windows.1\n2. run the script in the end of message\n3. remember the directory name, press the enter as it asks\n4. in the less scrool down\n\nThen inspect the processes git.exe and less.exe with Process\nExplorer of something you find convenient. Both processes\nshould have opened the pack file, and git.exe should have it\nmapped (I found it in the \"View\"->\"Lower Pane views\"->\"Dlls\").\n\nThen, if you run another bash window, cd to the repository\nand run \"git repack -a -d\", it should print:\n\n----\n$ git repack -a -d\nCounting objects: 3001, done.\nCompressing objects: 100% (1003/1003), done.\nWriting objects: 100% (3001/3001), done.\nTotal 3001 (delta 997), reused 3001 (delta 997)\nUnlink of file '.git/objects/pack/pack-e1b0e3ac01ff8d79a77648de3370f49b93c58a8b.pack' failed. Should I try again? (y/n)\n----\n\nI would like the git gc to succeed, because I run it as a\nscheduled task nightly and feel a bit annoyed by the opened\nwindows which wait for me to say yes (after exiting pager).\n\nSo, the fix is needed approximately in that place, after\nrunning any builtin command.\n\nFor your case, I think, the same close_all_packs() should be\ninvoked before the repacking.\n\n> The convention in Git seems to call things _gently rather\n> than _nodie:\n\nThanks, will change it if the idea is welcomed.\n\n>> -void close_pack_windows(struct packed_git *p)\n>> +static int close_pack_windows_nodie(struct packed_git *p)\n>>  {\n>>  \twhile (p->windows) {\n>>  \t\tstruct pack_window *w = p->windows;\n>>  \n>>  \t\tif (w->inuse_cnt)\n>> -\t\t\tdie(\"pack '%s' still has open windows to it\",\n>> -\t\t\t    p->pack_name);\n>> +\t\t\treturn 1;\n>> +\n>>  \t\tmunmap(w->base, w->len);\n>>  \t\tpack_mapped -= w->len;\n>>  \t\tpack_open_windows--;\n>>  \t\tp->windows = w->next;\n>>  \t\tfree(w);\n>>  \t}\n>> +\n>> +\treturn 0;\n>> +}\n> \n> And while we're at it, why not teach that function a new\n> parameter `int close_pack_fd`?\n\nI think \"close windows\" should close windows, if it also\ncloses pack fd probably should be another name. But current\ncode seems quite logical. Close the packs, and run closing\nwindows from it.\n\n> There is another problem: when we cannot close the pack\n> window, we cannot really continue, can we?\n\nYes we can, unlocking of the window is not needed for the\ncurrent process to do what it intended to do, it would just\ninterfere with concurrent git gc.\n\nFor the clone case probably die would be appropriate. If you\nfeel like it worth complicating the code we might search for\nsome solution.\n\n-- \nMax\n\nThe script:\n--------- ./t-windows-pack-close.sh ---------\n#!/bin/sh\n\nset -e\n\nTEST_DIR=`mktemp -d`\n\nt_git() {\n    git --work-tree=\"$TEST_DIR\" --git-dir=\"$TEST_DIR/.git\" \"$@\"\n}\n\nt_git init\n\nfor i in $(seq 1000)\ndo\n    echo \"commit$i\" >\"$TEST_DIR/commit.$i\"\n    t_git add \"commit.$i\"\n    t_git commit -m \"commit$i\" -q\ndone\n\nt_git repack\n\npack_path=$(find \"$TEST_DIR/.git/objects/pack\" -name \"pack-.pack\")\n\necho \"dir: $TEST_DIR\"\necho press enter to start git log\nread\n\nt_git -c core.packedGitWindowSize=100 log\n-----------------------------\n"},{"id":"271030","messageId":"20151002192148.GD26154@wheezy.local","threadId":"40463","inReplyTo":"763ac2b63d56d250e5e1a27f490f79db@dscho.org","subject":"Re: [PATCH/RFC 1/2] sha1_file: close all pack files after running","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2015-10-02T19:21:48Z","receivedAt":"2015-10-02T19:21:48Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"On Fri, Oct 02, 2015 at 12:13:40PM +0200, Johannes Schindelin wrote:\n> Hi Max,\n> \n> On 2015-10-02 12:05, Johannes Schindelin wrote:\n> \n> > On 2015-10-01 05:29, Max Kirillov wrote:\n>>> When a builtin has done its job, but waits for pager or not waited\n>>> by its caller and still hanging it keeps pack files opened.\n>>> This can cause a number of issues, for example on Windows git gc\n>>> cannot remove the packs.\n> \n> Could you do me another favor? It seems that you want to\n> work on this, so I will step back (I have to take off for\n> the weekend very soon anyway, so I am really glad that you\n> take care of it). But I would really love to see the line\n\nAs I explained in other message, your case is a bit\ndifferent.\n\nI could add another call of close_all_packs() as a separate\ncommit with the \"fixes\" link, but I'm not so sure about it\nif it turns out that additional efforts are required.\n\n-- \nMax\n"},{"id":"271031","messageId":"20151002200649.GE26154@wheezy.local","threadId":"40463","inReplyTo":"20151002190646.GC26154@wheezy.local","subject":"Re: [PATCH/RFC 1/2] sha1_file: close all pack files after running","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2015-10-02T20:06:49Z","receivedAt":"2015-10-02T20:06:49Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"On Fri, Oct 02, 2015 at 10:06:46PM +0300, Max Kirillov wrote:\n> for i in $(seq 1000)\n> t_git -c core.packedGitWindowSize=100 log\n\nIt was 32-bit build.\nI cannot promise those exactly numbers will work, because I don not clearly\nunderstand what do they mean. With 100 commits the pack size was 20K, but it\nwas mapped fully with the window=100.\n\nBut I believe with the same build same numbers should reproduce the issue.\n\nFor 32-bit builds I can see it on any significantly big repository, like the\ngit itself. Maybe for 64bit it is less likely.\n\nThe code which decides whether to close the pack is in use_pack() in sha1_file.c:\n-----------\n                        if (!win->offset && win->len == p->pack_size\n                                && !p->do_not_close) {\n                                close(p->pack_fd);\n                                pack_open_fds--;\n                                p->pack_fd = -1;\n                        }\n-----------\n"},{"id":"271087","messageId":"21bd1c9c394a421bc06c6fa1837f16b6@dscho.org","threadId":"40463","inReplyTo":"20151002192148.GD26154@wheezy.local","subject":"Re: [PATCH/RFC 1/2] sha1_file: close all pack files after running","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-10-04T14:53:30Z","receivedAt":"2015-10-04T14:53:30Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Max,\n\nOn 2015-10-02 21:21, Max Kirillov wrote:\n> On Fri, Oct 02, 2015 at 12:13:40PM +0200, Johannes Schindelin wrote:\n>> On 2015-10-02 12:05, Johannes Schindelin wrote:\n>>\n>> > On 2015-10-01 05:29, Max Kirillov wrote:\n>>>> When a builtin has done its job, but waits for pager or not waited\n>>>> by its caller and still hanging it keeps pack files opened.\n>>>> This can cause a number of issues, for example on Windows git gc\n>>>> cannot remove the packs.\n>>\n>> Could you do me another favor? It seems that you want to\n>> work on this, so I will step back (I have to take off for\n>> the weekend very soon anyway, so I am really glad that you\n>> take care of it). But I would really love to see the line\n> \n> As I explained in other message, your case is a bit\n> different.\n\nI guess then we would need two different patches for the two different fixes, at least.\n\nSo now I am unsure how to proceed: I do not want to step on your toes, but I also want to see my use case fixed and I want to move forward on this. At the moment, it looks as if we are at an impasse.\n\nCiao,\nJohannes\n"},{"id":"271120","messageId":"20151005045748.GF26154@wheezy.local","threadId":"40463","inReplyTo":"21bd1c9c394a421bc06c6fa1837f16b6@dscho.org","subject":"Re: [PATCH/RFC 1/2] sha1_file: close all pack files after running","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2015-10-05T04:57:48Z","receivedAt":"2015-10-05T04:57:48Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"On Sun, Oct 04, 2015 at 04:53:30PM +0200, Johannes Schindelin wrote:\n> I guess then we would need two different patches for the\n> two different fixes, at least.\n> \n> So now I am unsure how to proceed: I do not want to step\n> on your toes, but I also want to see my use case fixed and\n> I want to move forward on this. At the moment, it looks as\n> if we are at an impasse.\n\nJust continue with your patches then. I do not hurry.\n"},{"id":"271134","messageId":"2588666fce6a367fa46cf2501a78fe76@dscho.org","threadId":"40463","inReplyTo":"20151005045748.GF26154@wheezy.local","subject":"Re: [PATCH/RFC 1/2] sha1_file: close all pack files after running","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-10-05T09:03:19Z","receivedAt":"2015-10-05T09:03:19Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Max,\n\nOn 2015-10-05 06:57, Max Kirillov wrote:\n> On Sun, Oct 04, 2015 at 04:53:30PM +0200, Johannes Schindelin wrote:\n>> I guess then we would need two different patches for the\n>> two different fixes, at least.\n>>\n>> So now I am unsure how to proceed: I do not want to step\n>> on your toes, but I also want to see my use case fixed and\n>> I want to move forward on this. At the moment, it looks as\n>> if we are at an impasse.\n> \n> Just continue with your patches then. I do not hurry.\n\nOkay!\nJohannes\n"}]}