gc/repack: release packs when needed
cover gc/repack: release packs when needed
This fixes more "can't delete files while they are still open" issues on Windows.
Johannes Schindelin (1):
gc/repack: release packs when needed
builtin/gc.c | 4 +++- builtin/repack.c | 2 ++ 2 files changed, 5 insertions(+), 1 deletion(-)
base-commit: 5d826e972970a784bd7a7bdf587512510097b8c7
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-95%2Fdscho%2Frelease-gc-repack-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-95/dscho/release-gc-repack-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/95
Signature · 1 line
gitgitgadget
PATCH 1/1 gc/repack: release packs when needed
From: Johannes Schindelin <[email protected]>
On Windows, files cannot be removed nor renamed if there are still handles held by a process. To remedy that, we introduced the close_all_packs() function.
Earlier, we made sure that the packs are released just before git gc is spawned, in case that gc wants to remove no-longer needed packs.
But this developer forgot that gc itself also needs to let go of packs, e.g. when consolidating all packs via the --aggressive option.
Likewise, git repack -d wants to delete obsolete packs and therefore needs to close all pack handles, too.
--- builtin/gc.c | 4 +++- builtin/repack.c | 2 ++ 2 files changed, 5 insertions(+), 1 deletion(-)
builtin/gc.c +3 -1
@@ -659,8 +659,10 @@ int cmd_gc(int argc, const char **argv, const char *prefix) report_garbage = report_pack_garbage;reprepare_packed_git (the_repository); -if (pack_garbage.nr >0 ) +if (pack_garbage.nr >0 ) { +close_all_packs (the_repository->objects);clean_pack_garbage (); + }if (gc_write_commit_graph) write_commit_graph_reachable(get_object_directory(), 0,
builtin/repack.c +2 -0
@@ -419,6 +419,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)if (!names.nr && !po_args.quiet)printf ("Nothing new to pack.\n" ); +close_all_packs (the_repository->objects); +/* * Ok we have prepared all new packfiles. * First see if there are packs of the same name and if so
Signature · 1 line
gitgitgadget
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 26 lines · writes:
From: Johannes Schindelin <[email protected]>
On Windows, files cannot be removed nor renamed if there are still handles held by a process. To remedy that, we introduced the
close_all_packs()function.Earlier, we made sure that the packs are released just before
git gcis spawned, in case that gc wants to remove no-longer needed packs.But this developer forgot that gc itself also needs to let go of packs, e.g. when consolidating all packs via the --aggressive option.
Likewise,
git repack -dwants to delete obsolete packs and therefore needs to close all pack handles, too.Signed-off-by Johannes Schindelin <[email protected]>--- builtin/gc.c | 4 +++- builtin/repack.c | 2 ++ 2 files changed, 5 insertions(+), 1 deletion(-)builtin/gc.c +0
-0
builtin/gc.c int cmd_gc(int argc, const char **argv, const char *prefix) · line 665
Quoted 8 lines
builtin/gc.c +3
-1@@ -659,8 +659,10 @@ int cmd_gc(int argc, const char **argv, const char *prefix) report_garbage = report_pack_garbage;reprepare_packed_git (the_repository); -if (pack_garbage.nr >0 ) +if (pack_garbage.nr >0 ) { +close_all_packs (the_repository->objects);clean_pack_garbage (); + }
Closing before removing does make sense, but wouldn't we want to move reprepare_packed_git() after clean_pack_garbage() while at it? After all, the logical sequence is that we used the current set of packs to figure out whihch ones are garbage, then now we are about to discard. We close the packs in the current set (i.e. the fix made in this patch), discard the garbage packs. It would make sense to start using the new set (i.e. "reprepare") after all that is done, no? Especially, given that the next step (write-commit-graph) still wants to read quite a lot of data from now the latest set of packfiles...
builtin/repack.c int cmd_repack(int argc, const char **argv, const char *prefix) · line 423
Quoted 12 lines
builtin/gc.c +0
-0if (gc_write_commit_graph) write_commit_graph_reachable(get_object_directory(), 0,builtin/repack.c +2
-0@@ -419,6 +419,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)if (!names.nr && !po_args.quiet)printf ("Nothing new to pack.\n" ); +close_all_packs (the_repository->objects); +
On the other hand, This one is added to the ideal and perfect location, I think.
Thanks.
Quoted 3 lines
builtin/repack.c +0
-0/* * Ok we have prepared all new packfiles. * First see if there are packs of the same name and if so
Quoted 16 lines · Junio C Hamano
builtin/gc.c int cmd_gc(int argc, const char **argv, const char *prefix) · line 665
Quoted 13 lines
builtin/gc.c +3
-1@@ -659,8 +659,10 @@ int cmd_gc(int argc, const char **argv, const char *prefix) report_garbage = report_pack_garbage;reprepare_packed_git (the_repository); -if (pack_garbage.nr >0 ) +if (pack_garbage.nr >0 ) { +close_all_packs (the_repository->objects);clean_pack_garbage (); + }Closing before removing does make sense, but wouldn't we want to move
reprepare_packed_git()afterclean_pack_garbage()while at it?
Quoted 8 lines
After all, the logical sequence is that we used the current set of packs to figure out whihch ones are garbage, then now we are about to discard. We close the packs in the current set (i.e. the fix made in this patch), discard the garbage packs. It would make sense to start using the new set (i.e. "reprepare") after all that is done, no? Especially, given that the next step (write-commit-graph) still wants to read quite a lot of data from now the latest set of packfiles...
I agree that your suggested ordering makes more sense, but I don't think it matters in practice with the current code. reprepare_packed_git() never throws away old pack entries (and if they're mmap'd, we might even continue to use them). So the end result is the same either way.
-Peff
Quoted 27 lines · Jeff King
Quoted 16 lines · Junio C Hamano
builtin/gc.c int cmd_gc(int argc, const char **argv, const char *prefix) · line 665
Quoted 13 lines
builtin/gc.c +3
-1@@ -659,8 +659,10 @@ int cmd_gc(int argc, const char **argv, const char *prefix) report_garbage = report_pack_garbage;reprepare_packed_git (the_repository); -if (pack_garbage.nr >0 ) +if (pack_garbage.nr >0 ) { +close_all_packs (the_repository->objects);clean_pack_garbage (); + }Closing before removing does make sense, but wouldn't we want to move
reprepare_packed_git()afterclean_pack_garbage()while at it?Quoted 8 lines
After all, the logical sequence is that we used the current set of packs to figure out whihch ones are garbage, then now we are about to discard. We close the packs in the current set (i.e. the fix made in this patch), discard the garbage packs. It would make sense to start using the new set (i.e. "reprepare") after all that is done, no? Especially, given that the next step (write-commit-graph) still wants to read quite a lot of data from now the latest set of packfiles...
Quoted 4 lines
I agree that your suggested ordering makes more sense, but I don't think it matters in practice with the current code.
reprepare_packed_git()never throws away old pack entries (and if they're mmap'd, we might even continue to use them). So the end result is the same either way.
Yeah, it would not make difference to the machine. I was trying to be more helpful to human readers.
In any case, this patch from Dec 15 last year is where my backlog sweeping is at right now X-<.