{"thread":{"id":"41182","subject":"[PATCH 0/4] Always release pack files before calling gc --auto","startedAt":"2016-01-13T17:20:02Z","lastAt":"2016-01-13T18:52:05Z","messageCount":7,"participants":["Johannes Schindelin","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"275936","messageId":"cover.1452705584.git.johannes.schindelin@gmx.de","threadId":"41182","inReplyTo":null,"subject":"[PATCH 0/4] Always release pack files before calling gc --auto","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-13T17:20:02Z","receivedAt":"2016-01-13T17:20:02Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"These patches are companions to 786b150 (clone --dissociate: avoid\nlocking pack files, 2015-10-06), triggered by a bug report to the Git\nfor Windows project.\n\n\nJohannes Schindelin (4):\n fetch: release pack files before garbage-collecting\n am: release pack files before garbage-collecting\n merge: release pack files before garbage-collecting\n receive-pack: release pack files before garbage-collecting\n\n builtin/am.c | 1 +\n builtin/fetch.c | 2 ++\n builtin/merge.c | 1 +\n builtin/receive-pack.c | 1 +\n t/t5510-fetch.sh | 13 +++++++++++++\n 5 files changed, 18 insertions(+)\n\n-- \n2.6.3.windows.1.300.g1c25e49\n"},{"id":"275938","messageId":"4fd3f0c6d02ec33a832b6c1688e9f348825d074e.1452705584.git.johannes.schindelin@gmx.de","threadId":"41182","inReplyTo":"cover.1452705584.git.johannes.schindelin@gmx.de","subject":"[PATCH 1/4] fetch: release pack files before garbage-collecting","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-13T17:20:11Z","receivedAt":"2016-01-13T17:20:11Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Before auto-gc'ing, we need to make sure that the pack files are\nreleased in case they need to be repacked and garbage-collected.\n\nThis fixes https://github.com/git-for-windows/git/issues/500\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/fetch.c | 2 ++\n t/t5510-fetch.sh | 13 +++++++++++++\n 2 files changed, 15 insertions(+)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 33f04c1..8e74213 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1225,6 +1225,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \tlist.strdup_strings = 1;\n \tstring_list_clear(&list, 0);\n \n+\tclose_all_packs();\n+\n \targv_array_pushl(&argv_gc_auto, \"gc\", \"--auto\", NULL);\n \tif (verbosity < 0)\n \t\targv_array_push(&argv_gc_auto, \"--quiet\");\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex 0ba9db0..e3ee4bd 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -708,4 +708,17 @@ test_expect_success 'fetching a one-level ref works' '\n \t)\n '\n \n+test_expect_success 'fetching with auto-gc does not lock up' '\n+\twrite_script askyesno <<-\\EOF &&\n+\techo \"$*\" &&\n+\tfalse\n+\tEOF\n+\tgit clone \"file://$D\" auto-gc &&\n+\ttest_commit test2 &&\n+\tcd auto-gc &&\n+\tgit config gc.autoPackLimit 1 &&\n+\tGIT_ASK_YESNO=\"$D/askyesno\" git fetch >fetch.out 2>&1 &&\n+\t! grep \"Should I try again\" fetch.out\n+'\n+\n test_done\n-- \n2.6.3.windows.1.300.g1c25e49\n"},{"id":"275937","messageId":"999dc2556d5a4683722b4c63870f43c48d2a3211.1452705584.git.johannes.schindelin@gmx.de","threadId":"41182","inReplyTo":"cover.1452705584.git.johannes.schindelin@gmx.de","subject":"[PATCH 2/4] am: release pack files before garbage-collecting","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-13T17:20:16Z","receivedAt":"2016-01-13T17:20:16Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Before auto-gc'ing, we need to make sure that the pack files are\nreleased in case they need to be repacked and garbage-collected.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/am.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 9fb42fd..de235cf 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1939,6 +1939,7 @@ next:\n \t */\n \tif (!state->rebasing) {\n \t\tam_destroy(state);\n+\t\tclose_all_packs();\n \t\trun_command_v_opt(argv_gc_auto, RUN_GIT_CMD);\n \t}\n }\n-- \n2.6.3.windows.1.300.g1c25e49\n"},{"id":"275939","messageId":"83f7c1a7ad4f2d63733b61115b4a1dd88468e96f.1452705584.git.johannes.schindelin@gmx.de","threadId":"41182","inReplyTo":"cover.1452705584.git.johannes.schindelin@gmx.de","subject":"[PATCH 3/4] merge: release pack files before garbage-collecting","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-13T17:20:21Z","receivedAt":"2016-01-13T17:20:21Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Before auto-gc'ing, we need to make sure that the pack files are\nreleased in case they need to be repacked and garbage-collected.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/merge.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 15bf95b..b98a348 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -404,6 +404,7 @@ static void finish(struct commit *head_commit,\n \t\t\t * We ignore errors in 'gc --auto', since the\n \t\t\t * user should see them.\n \t\t\t */\n+\t\t\tclose_all_packs();\n \t\t\trun_command_v_opt(argv_gc_auto, RUN_GIT_CMD);\n \t\t}\n \t}\n-- \n2.6.3.windows.1.300.g1c25e49\n"},{"id":"275940","messageId":"63654ac891616aae8a8e2f9560098f1823e87abe.1452705584.git.johannes.schindelin@gmx.de","threadId":"41182","inReplyTo":"cover.1452705584.git.johannes.schindelin@gmx.de","subject":"[PATCH 4/4] receive-pack: release pack files before garbage-collecting","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-13T17:20:26Z","receivedAt":"2016-01-13T17:20:26Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Before auto-gc'ing, we need to make sure that the pack files are\nreleased in case they need to be repacked and garbage-collected.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/receive-pack.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 2b3b746..f2d6761 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1796,6 +1796,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n \t\t\t\t\"gc\", \"--auto\", \"--quiet\", NULL,\n \t\t\t};\n \t\t\tint opt = RUN_GIT_CMD | RUN_COMMAND_STDOUT_TO_STDERR;\n+\t\t\tclose_all_packs();\n \t\t\trun_command_v_opt(argv_gc_auto, opt);\n \t\t}\n \t\tif (auto_update_server_info)\n-- \n2.6.3.windows.1.300.g1c25e49\n"},{"id":"275946","messageId":"20160113175237.GA13139@sigill.intra.peff.net","threadId":"41182","inReplyTo":"cover.1452705584.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 0/4] Always release pack files before calling gc --auto","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-13T17:52:38Z","receivedAt":"2016-01-13T17:52:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 13, 2016 at 06:20:02PM +0100, Johannes Schindelin wrote:\n\n> These patches are companions to 786b150 (clone --dissociate: avoid\n> locking pack files, 2015-10-06), triggered by a bug report to the Git\n> for Windows project.\n> \n> Johannes Schindelin (4):\n> fetch: release pack files before garbage-collecting\n> am: release pack files before garbage-collecting\n> merge: release pack files before garbage-collecting\n> receive-pack: release pack files before garbage-collecting\n\nI think this is fine, but I noticed an almost-problem that I think is\nworth mentioning.\n\nClosing packs can open up race conditions in some cases. I.e., we think\nwe have an object in a particular pack, do some work on that basis, and\nthen later find the pack to be inaccessible (due to somebody else\nrunning gc). If we keep the fd open, there's no race, but if we close\nit, we can't reopen it.\n\nI think all of the cases here are fine, for two reasons:\n\n 1. The new closing is right before the program would exit anyway, so\n there's no interesting work left to do (and obviously that's where\n we generally want to call \"gc --auto\", too)\n\n 2. This type of race might only be an issue for pack-objects (or at least that's\n the only place I've seen it). Most normal read_sha1_file() callers\n will happily re-scan the pack directory for the new packs upon\n finding that the old pack went away. But pack-objects peeks in the\n packfiles a little more intimately, and will record the actual pack\n and offset (for delta reuse).\n\n It _might_ also be possible to trigger this race using bitmaps,\n which are also pretty intimate with the packfile code. I'm not\n sure.\n\nSo I think this series is OK.\n\n-Peff\n"},{"id":"275956","messageId":"alpine.DEB.2.20.1601131951040.2964@virtualbox","threadId":"41182","inReplyTo":"20160113175237.GA13139@sigill.intra.peff.net","subject":"Re: [PATCH 0/4] Always release pack files before calling gc --auto","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-13T18:52:05Z","receivedAt":"2016-01-13T18:52:05Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Wed, 13 Jan 2016, Jeff King wrote:\n\n> On Wed, Jan 13, 2016 at 06:20:02PM +0100, Johannes Schindelin wrote:\n> \n> > These patches are companions to 786b150 (clone --dissociate: avoid\n> > locking pack files, 2015-10-06), triggered by a bug report to the Git\n> > for Windows project.\n> > \n> > Johannes Schindelin (4):\n> > fetch: release pack files before garbage-collecting\n> > am: release pack files before garbage-collecting\n> > merge: release pack files before garbage-collecting\n> > receive-pack: release pack files before garbage-collecting\n> \n> [...]\n>\n> I think all of the cases here are fine, for two reasons:\n> \n> 1. The new closing is right before the program would exit anyway, so\n> there's no interesting work left to do (and obviously that's where\n> we generally want to call \"gc --auto\", too)\n\nThis is actually what I checked, but of course I managed to forget to\nmention it in the cover letter :-(\n\nThanks for verifying, and for spelling it out.\nDscho\n"}]}