Close commit-graph before calling 'gc'
cover Close commit-graph before calling 'gc'
In Windows, the way we rename a lock file to replace the underlying file does not work when a process holds a read handle. For this reason, we call close_all_packs() everywhere before starting a git gc --auto subprocess. We also call close_commit_graph() before renaming the commit-graph lock file. But we don't close the commit-graph handles before running gc, which can cause an issue when gc.writeCommitGraph is enabled.
This series adds close_commit_graph() to close_all_packs() and then renames close_all_packs() to close_object_store(). This name is more descriptive of its larger purpose.
This is based on ds/commit-graph-write-refactor to avoid conflicts.
Thanks, -Stolee
Derrick Stolee (3):
commit-graph: use raw_object_store when closing
packfile: close commit-graph in close_all_packs
packfile: close_all_packs to close_object_store
builtin/am.c | 2 +- builtin/clone.c | 2 +- builtin/fetch.c | 2 +- builtin/gc.c | 4 ++-- builtin/merge.c | 2 +- builtin/rebase.c | 2 +- builtin/receive-pack.c | 2 +- builtin/repack.c | 2 +- commit-graph.c | 8 ++++---- commit-graph.h | 2 +- object.c | 2 +- packfile.c | 5 ++++- packfile.h | 2 +- upload-pack.c | 2 +- 14 files changed, 21 insertions(+), 18 deletions(-)
base-commit: 8520d7fc7c6edd4d71582c69a873436029b6cb1b
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-208%2Fderrickstolee%2Fclose-graph-everywhere-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-208/derrickstolee/close-graph-everywhere-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/208
Signature · 1 line
gitgitgadget
PATCH 1/3 commit-graph: use raw_object_store when closing
From: Derrick Stolee <[email protected]>
The close_commit_graph() method took a repository struct, but then only uses the raw_object_store within. Change the function prototype to make the method more flexible.
--- commit-graph.c | 8 ++++---- commit-graph.h | 2 +- upload-pack.c | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-)
commit-graph.c +4 -4
@@ -359,10 +359,10 @@ int generation_numbers_enabled(struct repository *r)return !!first_generation; } -void close_commit_graph (struct repository *r) +void close_commit_graph (struct raw_object_store *o) { -free_commit_graph (r->objects->commit_graph); - r->objects->commit_graph =NULL ; +free_commit_graph (o->commit_graph); + o->commit_graph =NULL ; }static int bsearch_graph (struct commit_graph *g,struct object_id *oid,uint32_t *pos)
@@ -1086,7 +1086,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)stop_progress (&ctx->progress);strbuf_release (&progress_title); -close_commit_graph (ctx->r); +close_commit_graph (ctx->r->objects);finalize_hashfile (f,NULL ,CSUM_HASH_IN_STREAM |CSUM_FSYNC );commit_lock_file (&lk);
commit-graph.h +1 -1
@@ -76,7 +76,7 @@ int write_commit_graph(const char *obj_dir,int verify_commit_graph (struct repository *r,struct commit_graph *g); -void close_commit_graph (struct repository *); +void close_commit_graph (struct raw_object_store *);void free_commit_graph (struct commit_graph *);#endif
upload-pack.c +1 -1
@@ -717,7 +717,7 @@ static void deepen_by_rev_list(struct packet_writer *writer, int ac, {struct commit_list *result; -close_commit_graph (the_repository); +close_commit_graph (the_repository->objects); result =get_shallow_commits_by_rev_list (ac, av,SHALLOW ,NOT_SHALLOW );send_shallow (writer, result);free_commit_list (result);
Signature · 1 line
gitgitgadget
PATCH 2/3 packfile: close commit-graph in close_all_packs
In reply to Derrick Stolee via GitGitGadgetFrom: Derrick Stolee <[email protected]>
The close_all_packs() method is used to close all read handles to pack-files and the multi-pack-index before running 'git gc --auto'. This is particularly important on the Windows platform, where read handles block any writes to those files. Replacing one of these files with a rename() will fail in this situation.
The commit-graph also performs a rename, so is susceptable to this problem. We are careful to close the commit-graph before writing, but that doesn't work when a 'git fetch' (or similar) process runs 'git gc --auto' which may write a commit-graph.
Here, close the commit-graph as part of close_all_packs().
--- packfile.c | 3 +++ 1 file changed, 3 insertions(+)
packfile.c +3 -0
@@ -16,6 +16,7 @@#include "tree.h" #include "object-store.h" #include "midx.h" +#include "commit-graph.h" char *odb_pack_name(struct strbuf *buf, const unsigned char *sha1,
@@ -350,6 +351,8 @@ void close_all_packs(struct raw_object_store *o)close_midx (o->multi_pack_index); o->multi_pack_index =NULL ; } + +close_commit_graph (o); }/*
Signature · 1 line
gitgitgadget
PATCH 3/3 packfile: close_all_packs to close_object_store
In reply to Derrick Stolee via GitGitGadgetFrom: Derrick Stolee <[email protected]>
The close_all_packs() method is now responsible for more than just pack-files. It also closes the commit-graph and the multi-pack-index. Rename the function to be more descriptive of its larger role. The name also fits because the input parameter is a raw_object_store.
--- builtin/am.c | 2 +- builtin/clone.c | 2 +- builtin/fetch.c | 2 +- builtin/gc.c | 4 ++-- builtin/merge.c | 2 +- builtin/rebase.c | 2 +- builtin/receive-pack.c | 2 +- builtin/repack.c | 2 +- object.c | 2 +- packfile.c | 2 +- packfile.h | 2 +- 11 files changed, 12 insertions(+), 12 deletions(-)
builtin/am.c +1 -1
@@ -1800,7 +1800,7 @@ static void am_run(struct am_state *state, int resume)*/ if (!state->rebasing) {am_destroy (state); -close_all_packs (the_repository->objects); +close_object_store (the_repository->objects);run_command_v_opt (argv_gc_auto,RUN_GIT_CMD ); } }
builtin/clone.c +1 -1
@@ -1240,7 +1240,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)transport_disconnect (transport);if (option_dissociate) { -close_all_packs (the_repository->objects); +close_object_store (the_repository->objects);dissociate_from_references (); }
builtin/fetch.c +1 -1
@@ -1670,7 +1670,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)string_list_clear (&list,0 ); -close_all_packs (the_repository->objects); +close_object_store (the_repository->objects);argv_array_pushl (&argv_gc_auto,"gc" ,"--auto" ,NULL );if (verbosity <0 )
builtin/gc.c +2 -2
@@ -632,7 +632,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)gc_before_repack ();if (!repository_format_precious_objects) { -close_all_packs (the_repository->objects); +close_object_store (the_repository->objects);if (run_command_v_opt (repack.argv,RUN_GIT_CMD ))die (FAILED_RUN , repack.argv[0 ]);
@@ -660,7 +660,7 @@ 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 ) { -close_all_packs (the_repository->objects); +close_object_store (the_repository->objects);clean_pack_garbage (); }
builtin/merge.c +1 -1
@@ -449,7 +449,7 @@ static void finish(struct commit *head_commit,* We ignore errors in 'gc --auto', since the * user should see them. */ -close_all_packs (the_repository->objects); +close_object_store (the_repository->objects);run_command_v_opt (argv_gc_auto,RUN_GIT_CMD ); } }
builtin/rebase.c +1 -1
@@ -328,7 +328,7 @@ static int finish_rebase(struct rebase_options *opts)delete_ref (NULL ,"REBASE_HEAD" ,NULL ,REF_NO_DEREF );apply_autostash (opts); -close_all_packs (the_repository->objects); +close_object_store (the_repository->objects);/* * We ignore errors in 'gc --auto', since the * user should see them.
builtin/receive-pack.c +1 -1
@@ -2032,7 +2032,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix) proc.git_cmd =1 ; proc.argv = argv_gc_auto; -close_all_packs (the_repository->objects); +close_object_store (the_repository->objects);if (!start_command (&proc)) {if (use_sideband)copy_to_sideband (proc.err,-1 ,NULL );
builtin/repack.c +1 -1
@@ -419,7 +419,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)if (!names.nr && !po_args.quiet)printf_ln (_ ("Nothing new to pack." )); -close_all_packs (the_repository->objects); +close_object_store (the_repository->objects);/* * Ok we have prepared all new packfiles.
object.c +1 -1
@@ -517,7 +517,7 @@ void raw_object_store_clear(struct raw_object_store *o) o->loaded_alternates =0 ;INIT_LIST_HEAD (&o->packed_git_mru); -close_all_packs (o); +close_object_store (o); o->packed_git =NULL ; }
packfile.c +1 -1
@@ -337,7 +337,7 @@ void close_pack(struct packed_git *p)close_pack_index (p); } -void close_all_packs (struct raw_object_store *o) +void close_object_store (struct raw_object_store *o) {struct packed_git *p;
packfile.h +1 -1
@@ -81,7 +81,7 @@ extern uint32_t get_pack_fanout(struct packed_git *p, uint32_t value);extern unsigned char *use_pack (struct packed_git *,struct pack_window **,off_t ,unsigned long *);extern void close_pack_windows (struct packed_git *);extern void close_pack (struct packed_git *); -extern void close_all_packs (struct raw_object_store *o); +extern void close_object_store (struct raw_object_store *o);extern void unuse_pack (struct pack_window **);extern void clear_delta_base_cache (void );extern struct packed_git *add_packed_git (const char *path,size_t path_len,int local);
Signature · 1 line
gitgitgadget
Re: [PATCH 3/3] packfile: close_all_packs to close_object_store
Hi Stolee,
*really* minor nit: the commit subject probably wants to have a "rename" after the colon ;-)
The patch looks sensible to me. Since Junio asked for a sanity check whether all of the call sites of close_all_packs() actually want to close the MIDX and the commit graph, too, I'll do the "speak out loud" type of patch review here (spoiler: all of them check out):
builtin/am.c static void am_run(struct am_state *state, int resume) · line 1804
Quoted 11 lines · Derrick Stolee via GitGitGadget
builtin/am.c +1
-1@@ -1800,7 +1800,7 @@ static void am_run(struct am_state *state, int resume)*/ if (!state->rebasing) {am_destroy (state); -close_all_packs (the_repository->objects); +close_object_store (the_repository->objects);run_command_v_opt (argv_gc_auto,RUN_GIT_CMD );
Here, we run git gc --auto, so we obviously really want to close all read handles.
Check.
builtin/clone.c int cmd_clone(int argc, const char **argv, const char *prefix) · line 1244
Quoted 13 lines
builtin/am.c +0
-0} }builtin/clone.c +1
-1@@ -1240,7 +1240,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)transport_disconnect (transport);if (option_dissociate) { -close_all_packs (the_repository->objects); +close_object_store (the_repository->objects);dissociate_from_references ();
Here, we prepare for disassociating the reference repository specified via git clone --reference <directory>. Obviously, we need to let go of all the handles we might have open there.
Check.
Quoted 7 lines
builtin/clone.c +0
-0}builtin/fetch.c +0
-0
builtin/fetch.c int cmd_fetch(int argc, const char **argv, const char *prefix) · line 1675
Quoted 7 lines
builtin/fetch.c +1
-1@@ -1670,7 +1670,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)string_list_clear (&list,0 ); -close_all_packs (the_repository->objects); +close_object_store (the_repository->objects);argv_array_pushl (&argv_gc_auto,"gc" ,"--auto" ,NULL );
Again, a git gc --auto that needs closing of all read handles to the files that might be overwritten by the garbage collection.
Check.
builtin/gc.c int cmd_gc(int argc, const char **argv, const char *prefix) · line 636
Quoted 12 lines
builtin/fetch.c +0
-0if (verbosity <0 )builtin/gc.c +1
-1@@ -632,7 +632,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)gc_before_repack ();if (!repository_format_precious_objects) { -close_all_packs (the_repository->objects); +close_object_store (the_repository->objects);if (run_command_v_opt (repack.argv,RUN_GIT_CMD ))
Here, we want to repack. AFAICT it is the only sane thing we can do to invalidate whatever we read from the object store into memory.
Check.
builtin/gc.c int cmd_gc(int argc, const char **argv, const char *prefix) · line 664
Quoted 9 lines
builtin/gc.c +1
-1die (FAILED_RUN , repack.argv[0 ]);@@ -660,7 +660,7 @@ 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 ) { -close_all_packs (the_repository->objects); +close_object_store (the_repository->objects);clean_pack_garbage ();
This wants to delete a number of files that are now obsolete, and it makes sense to make sure that there are no open read handles to those anymore. It is a bit unclear from just reading the code what types of files are accumulated into the pack_garbage string list, but then, we're in the last throngs of a garbage collection, and *just* about to write a new commit graph (if gc.writeCommitGraph=true), so I think it is quite okay to close not only the packs here, but everything we opened from the object store.
So I'd give this a check mark, too.
builtin/merge.c static void finish(struct commit *head_commit, · line 453
Quoted 13 lines
builtin/gc.c +0
-0}builtin/merge.c +1
-1@@ -449,7 +449,7 @@ static void finish(struct commit *head_commit,* We ignore errors in 'gc --auto', since the * user should see them. */ -close_all_packs (the_repository->objects); +close_object_store (the_repository->objects);run_command_v_opt (argv_gc_auto,RUN_GIT_CMD );
Obviously yet another git gc --auto, so yes, we need to close the object store handles we have.
Check.
Quoted 7 lines
builtin/merge.c +0
-0} }builtin/rebase.c +0
-0
builtin/rebase.c static int finish_rebase(struct rebase_options *opts) · line 334
Quoted 8 lines
builtin/rebase.c +1
-1@@ -328,7 +328,7 @@ static int finish_rebase(struct rebase_options *opts)delete_ref (NULL ,"REBASE_HEAD" ,NULL ,REF_NO_DEREF );apply_autostash (opts); -close_all_packs (the_repository->objects); +close_object_store (the_repository->objects);/* * We ignore errors in 'gc --auto', since the * user should see them.
Yet another git gc --auto.
Check.
builtin/receive-pack.c int cmd_receive_pack(int argc, const char **argv, const char *prefix) · line 2036
Quoted 11 lines
builtin/receive-pack.c +1
-1@@ -2032,7 +2032,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix) proc.git_cmd =1 ; proc.argv = argv_gc_auto; -close_all_packs (the_repository->objects); +close_object_store (the_repository->objects);if (!start_command (&proc)) {
This proc refers to another git gc --auto (see a couple lines above, still within the hunk).
Check.
Quoted 7 lines
builtin/receive-pack.c +0
-0if (use_sideband)copy_to_sideband (proc.err,-1 ,NULL );builtin/repack.c +0
-0
builtin/repack.c int cmd_repack(int argc, const char **argv, const char *prefix) · line 425
Quoted 8 lines
builtin/repack.c +1
-1@@ -419,7 +419,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)if (!names.nr && !po_args.quiet)printf_ln (_ ("Nothing new to pack." )); -close_all_packs (the_repository->objects); +close_object_store (the_repository->objects);/* * Ok we have prepared all new packfiles.
Ah, the joys of un-dynamic patch review. What you, dear reader, cannot see in this hunk is that the code comment at the end continues thusly:
* First see if there are packs of the same name and if so
* if we can move them out of the way (this can happen if we
* repacked immediately after packing fully.
*/
Meaning: we're about to rename some pack files. So the pack file handles need to be closed, all right, but what about the other object store handles? There is no mention of the commit graph (more on that below), but the loop following the code comment contains this:
if (!midx_cleared) {
clear_midx_file(the_repository);
midx_cleared = 1;
}So yes, I would give this a check.
It does puzzle me, I have to admit, that there is no (opt-in) code block to re-write the commit graph. After all, the commit graph references the pack files, right? So if they are repacked, it would at least be invalidated at this point...
object.c void raw_object_store_clear(struct raw_object_store *o) · line 520
Quoted 10 lines
object.c +1
-1@@ -517,7 +517,7 @@ void raw_object_store_clear(struct raw_object_store *o) o->loaded_alternates =0 ;INIT_LIST_HEAD (&o->packed_git_mru); -close_all_packs (o); +close_object_store (o);
We're in the middle of a function called raw_object_store_clear(). So...
Check.
Quoted 30 lines
object.c +0
-0o->packed_git =NULL ; }packfile.c +1
-1@@ -337,7 +337,7 @@ void close_pack(struct packed_git *p)close_pack_index (p); } -void close_all_packs (struct raw_object_store *o) +void close_object_store (struct raw_object_store *o) {struct packed_git *p;packfile.h +1
-2@@ -81,7 +81,7 @@ extern uint32_t get_pack_fanout(struct packed_git *p, uint32_t value);extern unsigned char *use_pack (struct packed_git *,struct pack_window **,off_t ,unsigned long *);extern void close_pack_windows (struct packed_git *);extern void close_pack (struct packed_git *); -extern void close_all_packs (struct raw_object_store *o); +extern void close_object_store (struct raw_object_store *o);extern void unuse_pack (struct pack_window **);extern void clear_delta_base_cache (void );extern struct packed_git *add_packed_git (const char *path,size_t path_len,int local); --
Quoted 1 line
gitgitgadget
And this concludes my review.
Thank you!
Dscho
Re: [PATCH 3/3] packfile: close_all_packs to close_object_store
Quoted 4 lines · Johannes Schindelin
Hi Stolee,
*really* minor nit: the commit subject probably wants to have a "rename" after the colon ;-)
I did put that there, but then the subject line was too long. I'm not opposed to putting it back.
Quoted 4 lines
The patch looks sensible to me. Since Junio asked for a sanity check whether all of the call sites of
close_all_packs()actually want to close the MIDX and the commit graph, too, I'll do the "speak out loud" type of patch review here (spoiler: all of them check out):
Thanks for the detail here!
Quoted 32 lines
builtin/repack.c int cmd_repack(int argc, const char **argv, const char *prefix) · line 425
Quoted 13 lines
builtin/repack.c +1
-1@@ -419,7 +419,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)if (!names.nr && !po_args.quiet)printf_ln (_ ("Nothing new to pack." )); -close_all_packs (the_repository->objects); +close_object_store (the_repository->objects);/* * Ok we have prepared all new packfiles. Ah, the joys of un-dynamic patch review. What you, dear reader, cannot see in this hunk is that the code comment at the end continues thusly:
* First see if there are packs of the same name and if so
* if we can move them out of the way (this can happen if we
* repacked immediately after packing fully.
*/Meaning: we're about to rename some pack files. So the pack file handles need to be closed, all right, but what about the other object store handles? There is no mention of the commit graph (more on that below), but the loop following the code comment contains this:
if (!midx_cleared) { clear_midx_file(the_repository); midx_cleared = 1; }
Quoted 6 lines
So yes, I would give this a check.
It does puzzle me, I have to admit, that there is no (opt-in) code block to re-write the commit graph. After all, the commit graph references the pack files, right? So if they are repacked, it would at least be invalidated at this point...
The commit-graph does not directly reference the packs. The file will still be valid, except if we GC'd some commits that it references. We have the ability to rewrite the graph in 'git gc'.
The MIDX does reference packs by name, so it needs to be cleared before we delete packs. This _could_ be done with more care: we only need to delete it if a pack it references is queued for deletion. However, you can do that using the 'git multi-pack-index expire|repack' pattern currently cooking.
Thanks,
-Stolee
Re: [PATCH 3/3] packfile: close_all_packs to close_object_store
Quoted 8 lines · Derrick Stolee
Quoted 4 lines · Johannes Schindelin
Hi Stolee,
*really* minor nit: the commit subject probably wants to have a "rename" after the colon ;-)
I did put that there, but then the subject line was too long. I'm not opposed to putting it back.
Let me locally amend what I queued in the meantime, then. Thanks, both.
Quoted 3 lines · Derrick Stolee via GitGitGadget
This series adds
close_commit_graph()toclose_all_packs()and then renamesclose_all_packs()toclose_object_store(). This name is more descriptive of its larger purpose.
OK. Somebody needs to go though all the existing callers of close_all_packs() to see if the above claim is true (unless you haven't done so, that is), but I really like the general direction.
Thanks.
Hi Junio,
Quoted 9 lines · Junio C Hamano
Quoted 3 lines · Derrick Stolee via GitGitGadget
This series adds
close_commit_graph()toclose_all_packs()and then renamesclose_all_packs()toclose_object_store(). This name is more descriptive of its larger purpose.OK. Somebody needs to go though all the existing callers of
close_all_packs()to see if the above claim is true (unless you haven't done so, that is), but I really like the general direction.
I tried to provide that in my review of 3/3.
Please note that these patches are most relevant for Windows, where renames and deletes fail when any process has an open handle to the file(s) in question.
Ciao,
Dscho
P.S.: Now that I think of it, if we close the object store, that's probably also a time when we want to release the loose object cache.
Quoted 9 lines · Junio C Hamano
Quoted 3 lines · Derrick Stolee via GitGitGadget
This series adds
close_commit_graph()toclose_all_packs()and then renamesclose_all_packs()toclose_object_store(). This name is more descriptive of its larger purpose.OK. Somebody needs to go though all the existing callers of
close_all_packs()to see if the above claim is true (unless you haven't done so, that is), but I really like the general direction.
Here is a full inventory. Most of the context can be found simply by 'git grep -B 10 -A 10 close_object_store' but some cases need reading a helper function to know for sure.
About to call gc or repack:
* am
* clone
* fetch
* merge
* rebase
* receive-pack
Is about to delete packs or pack-garbage:
* gc
* repack
And in object.c, we are in the "raw_object_store_clear()" method.
Thanks,
-Stolee
Hi Stolee,
Quoted 7 lines · Derrick Stolee via GitGitGadget
In Windows, the way we rename a lock file to replace the underlying file does not work when a process holds a read handle. For this reason, we call
close_all_packs()everywhere before starting a git gc --auto subprocess. We also callclose_commit_graph()before renaming the commit-graph lock file. But we don't close the commit-graph handles before running gc, which can cause an issue when gc.writeCommitGraph is enabled.
Quoted 5 lines
This series adds
close_commit_graph()toclose_all_packs()and then renamesclose_all_packs()toclose_object_store(). This name is more descriptive of its larger purpose.This is based on ds/commit-graph-write-refactor to avoid conflicts.
Thank you so much for jumping on my (off-list, in-team) bug report!
FWIW I integrated this series into Git for Windows v2.22.0-rc1 already.
Thanks,
Dscho