Re: [PATCH 2/2] pull: release packs before fetching

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > On Windows, files cannot be removed nor renamed if there are still
> > handles held by a process. To remedy that, we try to release all open
> > handles to any `.pack` file before e.g. repacking (which would want to
> > remove the original `.pack` file(s) after it is done).
> >
> > Since the `read_cache_unmerged()` and/or the `get_oid()` call in `git
> > pull` can cause `.pack` files to be opened, we need to release the open
> > handles before calling `git fetch`: the latter process might want to
> > spawn an auto-gc, which in turn might want to repack the objects.
> >
> > This commit is similar in spirit to 5bdece0d705 (gc/repack: release
> > packs when needed, 2018-12-15).
> >
> > This fixes https://github.com/git-for-windows/git/issues/3336.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  builtin/pull.c | 2 ++
> >  1 file changed, 2 insertions(+)
>
> After run_fetch() returns, we then go on to access objects from our
> object store (that's natural---after all, we fetched because we
> wanted to access the objects we have plus objects they have to offer
> to us) and the object store is transparently reopened for us.  Which
> may make a bit confusing API to newbies, but is an easy one to use,
> once we get used to it.
>
> A few general comments.
>
>  * Right now, run_fetch() does not do anything that needs to access
>    objects, but there is no reason to expect that will continue to
>    be the case, and once we added an call to an innocuous helper
>    function that happens to access objects, the close_object_store()
>    call made by the caller before run_fetch() was called becomes
>    moot.  The more we can delay the call to close_object_store(),
>    the better.  And the absolute last point we can defer the call to
>    close_object_store() is where immediately before run_fetch() calls
>    run_command_v_opt() to spawn "git fetch".
>
>  * Which makes me wonder if we may be better off having a bit in the
>    flags word the run_command() API takes to make a call to
>    close_object_store() for us.  run_fetch() that uses the
>    run_command API can use that bit without having to worry about
>    making a call to close_object_store() itself and when.
>
>  * Hits from "git grep -A2 close_object_store()" shows a notable
>    pattern.  Before run_auto_maintenance(), we often see a call to
>    it.  It almost feels (but I didn't dig it deeper) that a call to
>    run_auto_maintenance() that does not call close_object_store()
>    before doing so is a bug (there is one in builtin/commit.c).
>
>  * Which in turn makes me wonder if these many calls to close before
>    run_auto_maintenance() should be moved to run_auto_maintenance()
>    itself (which in turn can use the new flags bit in the
>    run_command() API).
>
> Sprinkling yet another call to close_object_store() as we discover
> need for doing so like this patch does is certainly OK, but as we
> add new hooks and higher-level commands, it will get messier and
> messier.  It probably may make sense to go in and clean it up,
> hopefully guided by the above observations, either before this
> "fix", or soon after it graduates before we forget.