Re: [PATCH] gc --auto: release pack files before auto packing

["SZEDER Gábor" <szeder.dev@gmail.com>]

On Fri, Jul 6, 2018 at 6:39 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Duy Nguyen <pclouds@gmail.com> 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@infogroep.be>
> >> ---
> >>
> >> 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@gmail.com/
>
> 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.