Re: [PATCH 2/2] pull: release packs before fetching
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 8, 2021, 19:16 UTC
- Message-ID
- <xmqqh7eu3mjb.fsf@gitster.g>
- In-Reply-To
- <0e956ae7bba906277c77103ae329bce6d6da4cfe.1631089771.git.gitgitgadget@gmail.com>
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 21 quoted lines
> 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.
Will queue, but will not merge down to 'next' until I hear an Ack on the commit-graph stuff.
Thanks.
Show 19 quoted lines
> diff --git a/builtin/pull.c b/builtin/pull.c > index 3e13f810843..d9f0156d969 100644 > --- a/builtin/pull.c > +++ b/builtin/pull.c > @@ -26,6 +26,7 @@ > #include "wt-status.h" > #include "commit-reach.h" > #include "sequencer.h" > +#include "packfile.h" > > /** > * Parses the value of --rebase. If value is a false value, returns > @@ -998,6 +999,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix) > oidclr(&rebase_fork_point); > } > > + close_object_store(the_repository->objects); > if (run_fetch(repo, refspecs)) > return 1;