{"thread":{"id":"48808","subject":"[PATCH] gc --auto: release pack files before auto packing","startedAt":"2018-06-30T13:46:35Z","lastAt":"2018-07-11T15:34:17Z","messageCount":11,"participants":["Kim Gybels","Duy Nguyen","Junio C Hamano","SZEDER Gábor"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"351452","messageId":"20180630133822.4580-1-kgybels@infogroep.be","threadId":"48808","inReplyTo":null,"subject":"[PATCH] gc --auto: release pack files before auto packing","fromName":"Kim Gybels","fromEmail":"kgybels@infogroep.be","sentAt":"2018-06-30T13:38:21Z","receivedAt":"2018-06-30T13:46:35Z","isPatch":true,"sender":{"key":"kgybels@infogroep.be","avatar":"https://avatars.githubusercontent.com/u/2051188?v=4"},"body":"Teach gc --auto to release pack files before auto packing the repository\nto prevent failures when removing them.\n\nAlso teach the test 'fetching with auto-gc does not lock up' to complain\nwhen it is no longer triggering an auto packing of the repository.\n\nFixes https://github.com/git-for-windows/git/issues/500\n\nSigned-off-by: Kim Gybels \n---\n\nPatch based on master, since problem doesn't reproduce on maint,\nhowever, the improvement to the test might be valuable on maint.\n\n builtin/gc.c | 1 +\n t/t5510-fetch.sh | 2 ++\n 2 files changed, 3 insertions(+)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex ccfb1ceaeb..63b62ab57c 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -612,6 +612,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \t\treturn -1;\n \n \tif (!repository_format_precious_objects) {\n+\t\tclose_all_packs(the_repository->objects);\n \t\tif (run_command_v_opt(repack.argv, RUN_GIT_CMD))\n \t\t\treturn error(FAILED_RUN, repack.argv[0]);\n \ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex e402aee6a2..b91637e48f 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -828,9 +828,11 @@ test_expect_success 'fetching with auto-gc does not lock up' '\n \ttest_commit test2 &&\n \t(\n \t\tcd auto-gc &&\n+\t\tgit config fetch.unpackLimit 1 &&\n \t\tgit config gc.autoPackLimit 1 &&\n \t\tgit config gc.autoDetach false &&\n \t\tGIT_ASK_YESNO=\"$D/askyesno\" git fetch >fetch.out 2>&1 &&\n+\t\tgrep \"Auto packing the repository\" fetch.out &&\n \t\t! grep \"Should I try again\" fetch.out\n \t)\n '\n-- \n2.18.0.windows.1\n\n"},{"id":"351453","messageId":"20180630145849.GA9416@duynguyen.home","threadId":"48808","inReplyTo":"20180630133822.4580-1-kgybels@infogroep.be","subject":"Re: [PATCH] gc --auto: release pack files before auto packing","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-06-30T14:58:50Z","receivedAt":"2018-06-30T14:58:58Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Jun 30, 2018 at 03:38:21PM +0200, Kim Gybels wrote:\n> Teach gc --auto to release pack files before auto packing the repository\n> to prevent failures when removing them.\n> \n> Also teach the test 'fetching with auto-gc does not lock up' to complain\n> when it is no longer triggering an auto packing of the repository.\n> \n> Fixes https://github.com/git-for-windows/git/issues/500\n> \n> Signed-off-by: Kim Gybels \n> ---\n> \n> Patch based on master, since problem doesn't reproduce on maint,\n> however, the improvement to the test might be valuable on maint.\n> \n> builtin/gc.c | 1 +\n> t/t5510-fetch.sh | 2 ++\n> 2 files changed, 3 insertions(+)\n> \n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index ccfb1ceaeb..63b62ab57c 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -612,6 +612,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n> \t\treturn -1;\n> \n> \tif (!repository_format_precious_objects) {\n> +\t\tclose_all_packs(the_repository->objects);\n\nWe have repo_clear() which does this and potentially closing file\ndescriptors on other things as well. I suggest we use it, and before\nany external command is run. Something like\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex ccfb1ceaeb..a852c71da6 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -473,6 +473,13 @@ static int report_last_gc_error(void)\n \n static int gc_before_repack(void)\n {\n+\t/*\n+\t * Shut down everything, we should have all the info we need\n+\t * at this point. Leaving some file descriptors open may\n+\t * prevent them from being removed on Windows.\n+\t */\n+\trepo_clear(the_repository);\n+\n \tif (pack_refs && run_command_v_opt(pack_refs_cmd.argv, RUN_GIT_CMD))\n \t\treturn error(FAILED_RUN, pack_refs_cmd.argv[0]);\n \n\n\n> \t\tif (run_command_v_opt(repack.argv, RUN_GIT_CMD))\n> \t\t\treturn error(FAILED_RUN, repack.argv[0]);\n> \n> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\n> index e402aee6a2..b91637e48f 100755\n> --- a/t/t5510-fetch.sh\n> +++ b/t/t5510-fetch.sh\n> @@ -828,9 +828,11 @@ test_expect_success 'fetching with auto-gc does not lock up' '\n> \ttest_commit test2 &&\n> \t(\n> \t\tcd auto-gc &&\n> +\t\tgit config fetch.unpackLimit 1 &&\n> \t\tgit config gc.autoPackLimit 1 &&\n> \t\tgit config gc.autoDetach false &&\n> \t\tGIT_ASK_YESNO=\"$D/askyesno\" git fetch >fetch.out 2>&1 &&\n> +\t\tgrep \"Auto packing the repository\" fetch.out &&\n\nNot sure if this should be test_i18ngrep\n\n> \t\t! grep \"Should I try again\" fetch.out\n> \t)\n> '\n> -- \n> 2.18.0.windows.1\n> \n"},{"id":"351701","messageId":"20180704201600.9908-1-kgybels@infogroep.be","threadId":"48808","inReplyTo":"20180630133822.4580-1-kgybels@infogroep.be","subject":"[PATCH v2] gc --auto: clear repository before auto packing","fromName":"Kim Gybels","fromEmail":"kgybels@infogroep.be","sentAt":"2018-07-04T20:16:00Z","receivedAt":"2018-07-04T20:16:29Z","isPatch":true,"sender":{"key":"kgybels@infogroep.be","avatar":"https://avatars.githubusercontent.com/u/2051188?v=4"},"body":"Teach gc --auto to clear the repository before auto packing it to\nprevent failures when removing files on Windows.\n\nAlso teach the test 'fetching with auto-gc does not lock up' to complain\nwhen it is no longer triggering an auto packing of the repository.\n\nFixes https://github.com/git-for-windows/git/issues/500\n\nSigned-off-by: Kim Gybels \n---\n\nUpdated after Duy Nguyen's comments:\n- use repo_clear instead of close_all_packs, and add the call in\n gc_before_repack intead of just before executing git repack\n- use test_i18ngrep instead of grep in updated test\n\n builtin/gc.c | 7 +++++++\n t/t5510-fetch.sh | 4 +++-\n 2 files changed, 10 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex ccfb1ceaeb..22a6fc4863 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -473,6 +473,13 @@ static int report_last_gc_error(void)\n \n static int gc_before_repack(void)\n {\n+\t/*\n+\t * Shut down everything, we should have all the info we need\n+\t * at this point. Leaving some file descriptors open may\n+\t * prevent them from being removed on Windows.\n+\t */\n+\trepo_clear(the_repository);\n+\n \tif (pack_refs && run_command_v_opt(pack_refs_cmd.argv, RUN_GIT_CMD))\n \t\treturn error(FAILED_RUN, pack_refs_cmd.argv[0]);\n \ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex e402aee6a2..ef599c11cd 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -828,10 +828,12 @@ test_expect_success 'fetching with auto-gc does not lock up' '\n \ttest_commit test2 &&\n \t(\n \t\tcd auto-gc &&\n+\t\tgit config fetch.unpackLimit 1 &&\n \t\tgit config gc.autoPackLimit 1 &&\n \t\tgit config gc.autoDetach false &&\n \t\tGIT_ASK_YESNO=\"$D/askyesno\" git fetch >fetch.out 2>&1 &&\n-\t\t! grep \"Should I try again\" fetch.out\n+\t\ttest_i18ngrep \"Auto packing the repository\" fetch.out &&\n+\t\ttest_i18ngrep ! \"Should I try again\" fetch.out\n \t)\n '\n \n-- \n2.18.0.windows.1\n\n"},{"id":"351781","messageId":"xmqqpo00mi7q.fsf@gitster-ct.c.googlers.com","threadId":"48808","inReplyTo":"20180630145849.GA9416@duynguyen.home","subject":"Re: [PATCH] gc --auto: release pack files before auto packing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-06T16:39:37Z","receivedAt":"2018-07-06T16:39:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> On Sat, Jun 30, 2018 at 03:38:21PM +0200, Kim Gybels wrote:\n>> Teach gc --auto to release pack files before auto packing the repository\n>> to prevent failures when removing them.\n>> \n>> Also teach the test 'fetching with auto-gc does not lock up' to complain\n>> when it is no longer triggering an auto packing of the repository.\n>> \n>> Fixes https://github.com/git-for-windows/git/issues/500\n>> \n>> Signed-off-by: Kim Gybels \n>> ---\n>> \n>> Patch based on master, since problem doesn't reproduce on maint,\n>> however, the improvement to the test might be valuable on maint.\n>> \n>> builtin/gc.c | 1 +\n>> t/t5510-fetch.sh | 2 ++\n>> 2 files changed, 3 insertions(+)\n>> \n>> diff --git a/builtin/gc.c b/builtin/gc.c\n>> index ccfb1ceaeb..63b62ab57c 100644\n>> --- a/builtin/gc.c\n>> +++ b/builtin/gc.c\n>> @@ -612,6 +612,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n>> \t\treturn -1;\n>> \n>> \tif (!repository_format_precious_objects) {\n>> +\t\tclose_all_packs(the_repository->objects);\n>\n> We have repo_clear() which does this and potentially closing file\n> descriptors on other things as well. I suggest we use it, and before\n> any external command is run. Something like\n>\n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index ccfb1ceaeb..a852c71da6 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -473,6 +473,13 @@ static int report_last_gc_error(void)\n> \n> static int gc_before_repack(void)\n> {\n> +\t/*\n> +\t * Shut down everything, we should have all the info we need\n> +\t * at this point. Leaving some file descriptors open may\n> +\t * prevent them from being removed on Windows.\n> +\t */\n> +\trepo_clear(the_repository);\n> +\n> \tif (pack_refs && run_command_v_opt(pack_refs_cmd.argv, RUN_GIT_CMD))\n> \t\treturn error(FAILED_RUN, pack_refs_cmd.argv[0]);\n\nWith \n\n https://public-inbox.org/git/20180509170409.13666-1-pclouds@gmail.com/\n\nthe call to repo_clear() on the_repository itself may be safe [*1*],\nbut if command line parsing code etc. has pointers to in-core object\netc. obtained before this function was called, the codeflow after\nthis function returns will be quite disturbed. Has anybody in this\nreview thread audited the codeflow _after_ this function is run to\nmake sure that invalidating in-core repository here does not cause\nus problems? Just checking, because I won't queue this change until\nI hear that somebody familiar with the codepath actually did so (I\nmay get around to do so myself later).\n\nThanks.\n\n\n[Footnote]\n\n*1* ... and of course changing the_index to &the_repo->index would\n make it even safer ;-)\n\n"},{"id":"351839","messageId":"CAM0VKj=u0OVad3QDRFOc+NWZ9TfwqAwmZ47s=5e5jGZaPQRH6g@mail.gmail.com","threadId":"48808","inReplyTo":"xmqqpo00mi7q.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] gc --auto: release pack files before auto packing","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-07-07T09:40:08Z","receivedAt":"2018-07-07T09:42:06Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Jul 6, 2018 at 6:39 PM Junio C Hamano wrote:\n>\n> Duy Nguyen writes:\n>\n> > On Sat, Jun 30, 2018 at 03:38:21PM +0200, Kim Gybels wrote:\n> >> Teach gc --auto to release pack files before auto packing the repository\n> >> to prevent failures when removing them.\n> >>\n> >> Also teach the test 'fetching with auto-gc does not lock up' to complain\n> >> when it is no longer triggering an auto packing of the repository.\n> >>\n> >> Fixes https://github.com/git-for-windows/git/issues/500\n> >>\n> >> Signed-off-by: Kim Gybels \n> >> ---\n> >>\n> >> Patch based on master, since problem doesn't reproduce on maint,\n> >> however, the improvement to the test might be valuable on maint.\n> >>\n> >> builtin/gc.c | 1 +\n> >> t/t5510-fetch.sh | 2 ++\n> >> 2 files changed, 3 insertions(+)\n> >>\n> >> diff --git a/builtin/gc.c b/builtin/gc.c\n> >> index ccfb1ceaeb..63b62ab57c 100644\n> >> --- a/builtin/gc.c\n> >> +++ b/builtin/gc.c\n> >> @@ -612,6 +612,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n> >> return -1;\n> >>\n> >> if (!repository_format_precious_objects) {\n> >> + close_all_packs(the_repository->objects);\n> >\n> > We have repo_clear() which does this and potentially closing file\n> > descriptors on other things as well. I suggest we use it, and before\n> > any external command is run. Something like\n> >\n> > diff --git a/builtin/gc.c b/builtin/gc.c\n> > index ccfb1ceaeb..a852c71da6 100644\n> > --- a/builtin/gc.c\n> > +++ b/builtin/gc.c\n> > @@ -473,6 +473,13 @@ static int report_last_gc_error(void)\n> >\n> > static int gc_before_repack(void)\n> > {\n> > + /*\n> > + * Shut down everything, we should have all the info we need\n> > + * at this point. Leaving some file descriptors open may\n> > + * prevent them from being removed on Windows.\n> > + */\n> > + repo_clear(the_repository);\n> > +\n> > if (pack_refs && run_command_v_opt(pack_refs_cmd.argv, RUN_GIT_CMD))\n> > return error(FAILED_RUN, pack_refs_cmd.argv[0]);\n>\n> With\n>\n> https://public-inbox.org/git/20180509170409.13666-1-pclouds@gmail.com/\n>\n> the call to repo_clear() on the_repository itself may be safe [*1*],\n> but if command line parsing code etc. has pointers to in-core object\n> etc. obtained before this function was called, the codeflow after\n> this function returns will be quite disturbed. Has anybody in this\n> review thread audited the codeflow _after_ this function is run to\n> make sure that invalidating in-core repository here does not cause\n> us problems?\n\nIt does create us problems, unfortunately.\n\nAmong other things, a call to repo_clear(the_repository) does this:\n\n raw_object_store_clear(repo->objects);\n FREE_AND_NULL(repo->objects);\n\nLater on cmd_gc() calls reprepare_packed_git(the_repository), which\nthen attempts to:\n\n r->objects->approximate_object_count_valid = 0;\n r->objects->packed_git_initialized = 0;\n\nbut with r->objects being NULL at this point that won't work, and in\nturn causes failures in several test scripts.\n\n\nFWIW, the original patch appears to be working fine, at least the test\nsuite passes.\n"},{"id":"351840","messageId":"CAM0VKjkCf+V1ZPGfghOcE73TAshd58V1QJz4af57jAjtktTu-A@mail.gmail.com","threadId":"48808","inReplyTo":"20180704201600.9908-1-kgybels@infogroep.be","subject":"Re: [PATCH v2] gc --auto: clear repository before auto packing","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-07-07T09:57:51Z","receivedAt":"2018-07-07T09:58:10Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Wed, Jul 4, 2018 at 10:16 PM Kim Gybels wrote:\n\n> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\n> index e402aee6a2..ef599c11cd 100755\n> --- a/t/t5510-fetch.sh\n> +++ b/t/t5510-fetch.sh\n> @@ -828,10 +828,12 @@ test_expect_success 'fetching with auto-gc does not lock up' '\n> test_commit test2 &&\n> (\n> cd auto-gc &&\n> + git config fetch.unpackLimit 1 &&\n> git config gc.autoPackLimit 1 &&\n> git config gc.autoDetach false &&\n> GIT_ASK_YESNO=\"$D/askyesno\" git fetch >fetch.out 2>&1 &&\n> - ! grep \"Should I try again\" fetch.out\n> + test_i18ngrep \"Auto packing the repository\" fetch.out &&\n> + test_i18ngrep ! \"Should I try again\" fetch.out\n\nThe messages containing \"Auto packing the repository\" are indeed\ntranslated, thus they have to be checked with 'test_i18ngrep', good.\n\nHowever, none of the \"Should I try again\" messages are translated, so\nchecking them with bare 'grep' is fine and it shouldn't be changed.\n\n\n> )\n> '\n>\n> --\n> 2.18.0.windows.1\n>\n"},{"id":"351861","messageId":"20180707231651.GB6152@infogroep.be","threadId":"48808","inReplyTo":"CAM0VKj=u0OVad3QDRFOc+NWZ9TfwqAwmZ47s=5e5jGZaPQRH6g@mail.gmail.com","subject":"Re: [PATCH] gc --auto: release pack files before auto packing","fromName":"Kim Gybels","fromEmail":"kgybels@infogroep.be","sentAt":"2018-07-07T23:16:51Z","receivedAt":"2018-07-07T23:16:59Z","isPatch":true,"sender":{"key":"kgybels@infogroep.be","avatar":"https://avatars.githubusercontent.com/u/2051188?v=4"},"body":"On (07/07/18 11:40), SZEDER Gábor wrote:\n> \n> On Fri, Jul 6, 2018 at 6:39 PM Junio C Hamano wrote:\n> >\n> > Duy Nguyen writes:\n> >\n> > > On Sat, Jun 30, 2018 at 03:38:21PM +0200, Kim Gybels wrote:\n> > >> Teach gc --auto to release pack files before auto packing the repository\n> > >> to prevent failures when removing them.\n> > >>\n> > >> Also teach the test 'fetching with auto-gc does not lock up' to complain\n> > >> when it is no longer triggering an auto packing of the repository.\n> > >>\n> > >> Fixes https://github.com/git-for-windows/git/issues/500\n> > >>\n> > >> Signed-off-by: Kim Gybels \n> > >> ---\n> > >>\n> > >> Patch based on master, since problem doesn't reproduce on maint,\n> > >> however, the improvement to the test might be valuable on maint.\n> > >>\n> > >> builtin/gc.c | 1 +\n> > >> t/t5510-fetch.sh | 2 ++\n> > >> 2 files changed, 3 insertions(+)\n> > >>\n> > >> diff --git a/builtin/gc.c b/builtin/gc.c\n> > >> index ccfb1ceaeb..63b62ab57c 100644\n> > >> --- a/builtin/gc.c\n> > >> +++ b/builtin/gc.c\n> > >> @@ -612,6 +612,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n> > >> return -1;\n> > >>\n> > >> if (!repository_format_precious_objects) {\n> > >> + close_all_packs(the_repository->objects);\n> > >\n> > > We have repo_clear() which does this and potentially closing file\n> > > descriptors on other things as well. I suggest we use it, and before\n> > > any external command is run. Something like\n> > >\n> > > diff --git a/builtin/gc.c b/builtin/gc.c\n> > > index ccfb1ceaeb..a852c71da6 100644\n> > > --- a/builtin/gc.c\n> > > +++ b/builtin/gc.c\n> > > @@ -473,6 +473,13 @@ static int report_last_gc_error(void)\n> > >\n> > > static int gc_before_repack(void)\n> > > {\n> > > + /*\n> > > + * Shut down everything, we should have all the info we need\n> > > + * at this point. Leaving some file descriptors open may\n> > > + * prevent them from being removed on Windows.\n> > > + */\n> > > + repo_clear(the_repository);\n> > > +\n> > > if (pack_refs && run_command_v_opt(pack_refs_cmd.argv, RUN_GIT_CMD))\n> > > return error(FAILED_RUN, pack_refs_cmd.argv[0]);\n> >\n> > With\n> >\n> > https://public-inbox.org/git/20180509170409.13666-1-pclouds@gmail.com/\n> >\n> > the call to repo_clear() on the_repository itself may be safe [*1*],\n> > but if command line parsing code etc. has pointers to in-core object\n> > etc. obtained before this function was called, the codeflow after\n> > this function returns will be quite disturbed. Has anybody in this\n> > review thread audited the codeflow _after_ this function is run to\n> > make sure that invalidating in-core repository here does not cause\n> > us problems?\n> \n> It does create us problems, unfortunately.\n> \n> Among other things, a call to repo_clear(the_repository) does this:\n> \n> raw_object_store_clear(repo->objects);\n> FREE_AND_NULL(repo->objects);\n> \n> Later on cmd_gc() calls reprepare_packed_git(the_repository), which\n> then attempts to:\n> \n> r->objects->approximate_object_count_valid = 0;\n> r->objects->packed_git_initialized = 0;\n> \n> but with r->objects being NULL at this point that won't work, and in\n> turn causes failures in several test scripts.\n> \n> \n> FWIW, the original patch appears to be working fine, at least the test\n> suite passes.\n\nShould I post a v3 that goes back to the original fix, but uses\ntest_i18ngrep instead of grep?\n\nIn addition to not breaking any tests, close_all_packs is already used\nin a similar way in am and fetch just before running \"gc --auto\".\n\n-Kim\n"},{"id":"351945","messageId":"CACsJy8C=Xs1QY_cMu+H4DR9XovBd5bO-ZC=ie-1x9yZepgUMdA@mail.gmail.com","threadId":"48808","inReplyTo":"20180707231651.GB6152@infogroep.be","subject":"Re: [PATCH] gc --auto: release pack files before auto packing","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-07-09T14:33:22Z","receivedAt":"2018-07-09T14:33:53Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Jul 8, 2018 at 1:16 AM Kim Gybels wrote:\n> Should I post a v3 that goes back to the original fix, but uses\n> test_i18ngrep instead of grep?\n\nYes please. In my comment I did write we didn't need the repo anymore\n(or something along that line) which turns out to be wrong.\n\n> In addition to not breaking any tests, close_all_packs is already used\n> in a similar way in am and fetch just before running \"gc --auto\".\n>\n> -Kim\n\n\n\n-- \nDuy\n"},{"id":"352015","messageId":"20180709203727.8236-1-kgybels@infogroep.be","threadId":"48808","inReplyTo":"20180704201600.9908-1-kgybels@infogroep.be","subject":"[PATCH v3] gc --auto: release pack files before auto packing","fromName":"Kim Gybels","fromEmail":"kgybels@infogroep.be","sentAt":"2018-07-09T20:37:27Z","receivedAt":"2018-07-09T20:38:07Z","isPatch":true,"sender":{"key":"kgybels@infogroep.be","avatar":"https://avatars.githubusercontent.com/u/2051188?v=4"},"body":"Teach gc --auto to release pack files before auto packing the repository\nto prevent failures when removing them.\n\nAlso teach the test 'fetching with auto-gc does not lock up' to complain\nwhen it is no longer triggering an auto packing of the repository.\n\nFixes https://github.com/git-for-windows/git/issues/500\n\nSigned-off-by: Kim Gybels \n---\n\nChanges since v2:\n- revert fix back to v1: use close_all_packs instead of repo_clear\n- use test_i18ngrep only for translated string\n\n builtin/gc.c | 1 +\n t/t5510-fetch.sh | 2 ++\n 2 files changed, 3 insertions(+)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex ccfb1ceaeb..63b62ab57c 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -612,6 +612,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \t\treturn -1;\n \n \tif (!repository_format_precious_objects) {\n+\t\tclose_all_packs(the_repository->objects);\n \t\tif (run_command_v_opt(repack.argv, RUN_GIT_CMD))\n \t\t\treturn error(FAILED_RUN, repack.argv[0]);\n \ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex e402aee6a2..6f1450eb69 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -828,9 +828,11 @@ test_expect_success 'fetching with auto-gc does not lock up' '\n \ttest_commit test2 &&\n \t(\n \t\tcd auto-gc &&\n+\t\tgit config fetch.unpackLimit 1 &&\n \t\tgit config gc.autoPackLimit 1 &&\n \t\tgit config gc.autoDetach false &&\n \t\tGIT_ASK_YESNO=\"$D/askyesno\" git fetch >fetch.out 2>&1 &&\n+\t\ttest_i18ngrep \"Auto packing the repository\" fetch.out &&\n \t\t! grep \"Should I try again\" fetch.out\n \t)\n '\n-- \n2.18.0.windows.1\n\n"},{"id":"352027","messageId":"xmqqwou4azev.fsf@gitster-ct.c.googlers.com","threadId":"48808","inReplyTo":"CACsJy8C=Xs1QY_cMu+H4DR9XovBd5bO-ZC=ie-1x9yZepgUMdA@mail.gmail.com","subject":"Re: [PATCH] gc --auto: release pack files before auto packing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-09T21:10:16Z","receivedAt":"2018-07-09T21:10:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> On Sun, Jul 8, 2018 at 1:16 AM Kim Gybels wrote:\n>> Should I post a v3 that goes back to the original fix, but uses\n>> test_i18ngrep instead of grep?\n>\n> Yes please. In my comment I did write we didn't need the repo anymore\n> (or something along that line) which turns out to be wrong.\n>\n>> In addition to not breaking any tests, close_all_packs is already used\n>> in a similar way in am and fetch just before running \"gc --auto\".\n>>\n>> -Kim\n\nSound good. \n\nI recall that \"clear repo should treat the_repository special\" was\ndiscussed when we saw the patch that became 74373b5f (\"repository:\nfix free problem with repo_clear(the_repository)\", 2018-05-10),\ninstead of treating only the index portion specially. Perhaps it\nwas a more correct approach after all?\n\n\n"},{"id":"352256","messageId":"CACsJy8Agm7qQqed4yoWWRXcZmRFuny5dPSue8NhkNo1tk58zbQ@mail.gmail.com","threadId":"48808","inReplyTo":"xmqqwou4azev.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] gc --auto: release pack files before auto packing","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-07-11T15:33:47Z","receivedAt":"2018-07-11T15:34:17Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Jul 9, 2018 at 11:10 PM Junio C Hamano wrote:\n>\n> Duy Nguyen writes:\n>\n> > On Sun, Jul 8, 2018 at 1:16 AM Kim Gybels wrote:\n> >> Should I post a v3 that goes back to the original fix, but uses\n> >> test_i18ngrep instead of grep?\n> >\n> > Yes please. In my comment I did write we didn't need the repo anymore\n> > (or something along that line) which turns out to be wrong.\n> >\n> >> In addition to not breaking any tests, close_all_packs is already used\n> >> in a similar way in am and fetch just before running \"gc --auto\".\n> >>\n> >> -Kim\n>\n> Sound good.\n>\n> I recall that \"clear repo should treat the_repository special\" was\n> discussed when we saw the patch that became 74373b5f (\"repository:\n> fix free problem with repo_clear(the_repository)\", 2018-05-10),\n> instead of treating only the index portion specially. Perhaps it\n> was a more correct approach after all?\n\nI think it's good that we have a way to \"shut down the repo\" when we\nrun an external command. But what we lack is \"reinitialize the repo\"\nafter the external command is done. We could treat the_repository\nspecial in this case so shutting down does not require\nreinitialization. Then repo_clear() should work well here. We could\nalso add a flag in repo_clear() to say \"release all the resources you\nare holding, but keep the repo settings/location..., we're not done\nwith this repo yet\" then we don't need to re-initialize the repo\nafterwards and still don't make the_repository so special.\n-- \nDuy\n"}]}