On (07/07/18 11:40), SZEDER Gábor wrote: > > On Fri, Jul 6, 2018 at 6:39 PM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > > > Duy Nguyen <pclouds@xxxxxxxxx> writes: > > > > > On Sat, Jun 30, 2018 at 03:38:21PM +0200, Kim Gybels wrote: > > >> Teach gc --auto to release pack files before auto packing the repository > > >> to prevent failures when removing them. > > >> > > >> Also teach the test 'fetching with auto-gc does not lock up' to complain > > >> when it is no longer triggering an auto packing of the repository. > > >> > > >> Fixes https://github.com/git-for-windows/git/issues/500 > > >> > > >> Signed-off-by: Kim Gybels <kgybels@xxxxxxxxxxxx> > > >> --- > > >> > > >> Patch based on master, since problem doesn't reproduce on maint, > > >> however, the improvement to the test might be valuable on maint. > > >> > > >> builtin/gc.c | 1 + > > >> t/t5510-fetch.sh | 2 ++ > > >> 2 files changed, 3 insertions(+) > > >> > > >> diff --git a/builtin/gc.c b/builtin/gc.c > > >> index ccfb1ceaeb..63b62ab57c 100644 > > >> --- a/builtin/gc.c > > >> +++ b/builtin/gc.c > > >> @@ -612,6 +612,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix) > > >> return -1; > > >> > > >> if (!repository_format_precious_objects) { > > >> + close_all_packs(the_repository->objects); > > > > > > We have repo_clear() which does this and potentially closing file > > > descriptors on other things as well. I suggest we use it, and before > > > any external command is run. Something like > > > > > > diff --git a/builtin/gc.c b/builtin/gc.c > > > index ccfb1ceaeb..a852c71da6 100644 > > > --- a/builtin/gc.c > > > +++ b/builtin/gc.c > > > @@ -473,6 +473,13 @@ static int report_last_gc_error(void) > > > > > > static int gc_before_repack(void) > > > { > > > + /* > > > + * Shut down everything, we should have all the info we need > > > + * at this point. Leaving some file descriptors open may > > > + * prevent them from being removed on Windows. > > > + */ > > > + repo_clear(the_repository); > > > + > > > if (pack_refs && run_command_v_opt(pack_refs_cmd.argv, RUN_GIT_CMD)) > > > return error(FAILED_RUN, pack_refs_cmd.argv[0]); > > > > With > > > > https://public-inbox.org/git/20180509170409.13666-1-pclouds@xxxxxxxxx/ > > > > the call to repo_clear() on the_repository itself may be safe [*1*], > > but if command line parsing code etc. has pointers to in-core object > > etc. obtained before this function was called, the codeflow after > > this function returns will be quite disturbed. Has anybody in this > > review thread audited the codeflow _after_ this function is run to > > make sure that invalidating in-core repository here does not cause > > us problems? > > It does create us problems, unfortunately. > > Among other things, a call to repo_clear(the_repository) does this: > > raw_object_store_clear(repo->objects); > FREE_AND_NULL(repo->objects); > > Later on cmd_gc() calls reprepare_packed_git(the_repository), which > then attempts to: > > r->objects->approximate_object_count_valid = 0; > r->objects->packed_git_initialized = 0; > > but with r->objects being NULL at this point that won't work, and in > turn causes failures in several test scripts. > > > FWIW, the original patch appears to be working fine, at least the test > suite passes. Should I post a v3 that goes back to the original fix, but uses test_i18ngrep instead of grep? In addition to not breaking any tests, close_all_packs is already used in a similar way in am and fetch just before running "gc --auto". -Kim
