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? Just checking, because I won't queue this change until I hear that somebody familiar with the codepath actually did so (I may get around to do so myself later). Thanks. [Footnote] *1* ... and of course changing the_index to &the_repo->index would make it even safer ;-)
