Hi Junio, On Wed, 8 Sep 2021, Junio C Hamano wrote: > "Johannes Schindelin via GitGitGadget" <gitgitgadget@xxxxxxxxx> > writes: > > > From: Johannes Schindelin <johannes.schindelin@xxxxxx> > > > > 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@xxxxxx> > > --- > > 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. I like those ideas, and submitted a follow-up patch series. > Will queue, but will not merge down to 'next' until I hear an Ack on > the commit-graph stuff. Thank you. For procedural reasons, I would like to keep the current patch series as-is, because that will free some mental space for me maintaining Git for Windows (where I already merged them, after a contributor verified the fix). Thanks, Dscho
