{"thread":{"id":"51116","subject":"[PATCH 0/3] Close commit-graph before calling 'gc'","startedAt":"2019-05-17T18:41:50Z","lastAt":"2019-05-28T16:29:55Z","messageCount":11,"participants":["Derrick Stolee via GitGitGadget","Junio C Hamano","Johannes Schindelin","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"375810","messageId":"pull.208.git.gitgitgadget@gmail.com","threadId":"51116","inReplyTo":null,"subject":"[PATCH 0/3] Close commit-graph before calling 'gc'","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-05-17T18:41:46Z","receivedAt":"2019-05-17T18:41:50Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"In Windows, the way we rename a lock file to replace the underlying file\ndoes not work when a process holds a read handle. For this reason, we call \nclose_all_packs() everywhere before starting a git gc --auto subprocess. We\nalso call close_commit_graph() before renaming the commit-graph lock file.\nBut we don't close the commit-graph handles before running gc, which can\ncause an issue when gc.writeCommitGraph is enabled.\n\nThis series adds close_commit_graph() to close_all_packs() and then renames \nclose_all_packs() to close_object_store(). This name is more descriptive of\nits larger purpose.\n\nThis is based on ds/commit-graph-write-refactor to avoid conflicts.\n\nThanks, -Stolee\n\nDerrick Stolee (3):\n  commit-graph: use raw_object_store when closing\n  packfile: close commit-graph in close_all_packs\n  packfile: close_all_packs to close_object_store\n\n builtin/am.c           | 2 +-\n builtin/clone.c        | 2 +-\n builtin/fetch.c        | 2 +-\n builtin/gc.c           | 4 ++--\n builtin/merge.c        | 2 +-\n builtin/rebase.c       | 2 +-\n builtin/receive-pack.c | 2 +-\n builtin/repack.c       | 2 +-\n commit-graph.c         | 8 ++++----\n commit-graph.h         | 2 +-\n object.c               | 2 +-\n packfile.c             | 5 ++++-\n packfile.h             | 2 +-\n upload-pack.c          | 2 +-\n 14 files changed, 21 insertions(+), 18 deletions(-)\n\n\nbase-commit: 8520d7fc7c6edd4d71582c69a873436029b6cb1b\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-208%2Fderrickstolee%2Fclose-graph-everywhere-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-208/derrickstolee/close-graph-everywhere-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/208\n-- \ngitgitgadget\n"},{"id":"375811","messageId":"9ff662e5e94d4a157c27e066ee4d01a04f00c295.1558118506.git.gitgitgadget@gmail.com","threadId":"51116","inReplyTo":"pull.208.git.gitgitgadget@gmail.com","subject":"[PATCH 1/3] commit-graph: use raw_object_store when closing","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-05-17T18:41:47Z","receivedAt":"2019-05-17T18:41:51Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nThe close_commit_graph() method took a repository struct, but then\nonly uses the raw_object_store within. Change the function prototype\nto make the method more flexible.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n commit-graph.c | 8 ++++----\n commit-graph.h | 2 +-\n upload-pack.c  | 2 +-\n 3 files changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 7723156964..e777e269aa 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -359,10 +359,10 @@ int generation_numbers_enabled(struct repository *r)\n \treturn !!first_generation;\n }\n \n-void close_commit_graph(struct repository *r)\n+void close_commit_graph(struct raw_object_store *o)\n {\n-\tfree_commit_graph(r->objects->commit_graph);\n-\tr->objects->commit_graph = NULL;\n+\tfree_commit_graph(o->commit_graph);\n+\to->commit_graph = NULL;\n }\n \n static int bsearch_graph(struct commit_graph *g, struct object_id *oid, uint32_t *pos)\n@@ -1086,7 +1086,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n \tstop_progress(&ctx->progress);\n \tstrbuf_release(&progress_title);\n \n-\tclose_commit_graph(ctx->r);\n+\tclose_commit_graph(ctx->r->objects);\n \tfinalize_hashfile(f, NULL, CSUM_HASH_IN_STREAM | CSUM_FSYNC);\n \tcommit_lock_file(&lk);\n \ndiff --git a/commit-graph.h b/commit-graph.h\nindex 70f4caf0c7..afa3633592 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -76,7 +76,7 @@ int write_commit_graph(const char *obj_dir,\n \n int verify_commit_graph(struct repository *r, struct commit_graph *g);\n \n-void close_commit_graph(struct repository *);\n+void close_commit_graph(struct raw_object_store *);\n void free_commit_graph(struct commit_graph *);\n \n #endif\ndiff --git a/upload-pack.c b/upload-pack.c\nindex d098ef5982..b51bed21e4 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -717,7 +717,7 @@ static void deepen_by_rev_list(struct packet_writer *writer, int ac,\n {\n \tstruct commit_list *result;\n \n-\tclose_commit_graph(the_repository);\n+\tclose_commit_graph(the_repository->objects);\n \tresult = get_shallow_commits_by_rev_list(ac, av, SHALLOW, NOT_SHALLOW);\n \tsend_shallow(writer, result);\n \tfree_commit_list(result);\n-- \ngitgitgadget\n\n"},{"id":"375812","messageId":"e4811af2ecb72fbaf22e41c74d9fe0dc6fef246e.1558118506.git.gitgitgadget@gmail.com","threadId":"51116","inReplyTo":"pull.208.git.gitgitgadget@gmail.com","subject":"[PATCH 2/3] packfile: close commit-graph in close_all_packs","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-05-17T18:41:48Z","receivedAt":"2019-05-17T18:41:51Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nThe close_all_packs() method is used to close all read handles to\npack-files and the multi-pack-index before running 'git gc --auto'.\nThis is particularly important on the Windows platform, where read\nhandles block any writes to those files. Replacing one of these\nfiles with a rename() will fail in this situation.\n\nThe commit-graph also performs a rename, so is susceptable to this\nproblem. We are careful to close the commit-graph before writing,\nbut that doesn't work when a 'git fetch' (or similar) process runs\n'git gc --auto' which may write a commit-graph.\n\nHere, close the commit-graph as part of close_all_packs().\n\nReported-by: Johannes Schindelin <johannes.schindelin@gmx.de>\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n packfile.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/packfile.c b/packfile.c\nindex 16bcb75262..ce12bffe3e 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -16,6 +16,7 @@\n #include \"tree.h\"\n #include \"object-store.h\"\n #include \"midx.h\"\n+#include \"commit-graph.h\"\n \n char *odb_pack_name(struct strbuf *buf,\n \t\t    const unsigned char *sha1,\n@@ -350,6 +351,8 @@ void close_all_packs(struct raw_object_store *o)\n \t\tclose_midx(o->multi_pack_index);\n \t\to->multi_pack_index = NULL;\n \t}\n+\n+\tclose_commit_graph(o);\n }\n \n /*\n-- \ngitgitgadget\n\n"},{"id":"375813","messageId":"0e948f639fb5209f07f8e3eb356b5886c41ff2be.1558118506.git.gitgitgadget@gmail.com","threadId":"51116","inReplyTo":"pull.208.git.gitgitgadget@gmail.com","subject":"[PATCH 3/3] packfile: close_all_packs to close_object_store","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-05-17T18:41:49Z","receivedAt":"2019-05-17T18:41:52Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nThe close_all_packs() method is now responsible for more than just pack-files.\nIt also closes the commit-graph and the multi-pack-index. Rename the function\nto be more descriptive of its larger role. The name also fits because the\ninput parameter is a raw_object_store.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n builtin/am.c           | 2 +-\n builtin/clone.c        | 2 +-\n builtin/fetch.c        | 2 +-\n builtin/gc.c           | 4 ++--\n builtin/merge.c        | 2 +-\n builtin/rebase.c       | 2 +-\n builtin/receive-pack.c | 2 +-\n builtin/repack.c       | 2 +-\n object.c               | 2 +-\n packfile.c             | 2 +-\n packfile.h             | 2 +-\n 11 files changed, 12 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 58a2aef28b..9315d32d2a 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1800,7 +1800,7 @@ static void am_run(struct am_state *state, int resume)\n \t */\n \tif (!state->rebasing) {\n \t\tam_destroy(state);\n-\t\tclose_all_packs(the_repository->objects);\n+\t\tclose_object_store(the_repository->objects);\n \t\trun_command_v_opt(argv_gc_auto, RUN_GIT_CMD);\n \t}\n }\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 50bde99618..82ce682c80 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -1240,7 +1240,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \ttransport_disconnect(transport);\n \n \tif (option_dissociate) {\n-\t\tclose_all_packs(the_repository->objects);\n+\t\tclose_object_store(the_repository->objects);\n \t\tdissociate_from_references();\n \t}\n \ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex b620fd54b4..3aec95608f 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1670,7 +1670,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \n \tstring_list_clear(&list, 0);\n \n-\tclose_all_packs(the_repository->objects);\n+\tclose_object_store(the_repository->objects);\n \n \targv_array_pushl(&argv_gc_auto, \"gc\", \"--auto\", NULL);\n \tif (verbosity < 0)\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex df2573f124..20c8f1bfe8 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -632,7 +632,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \tgc_before_repack();\n \n \tif (!repository_format_precious_objects) {\n-\t\tclose_all_packs(the_repository->objects);\n+\t\tclose_object_store(the_repository->objects);\n \t\tif (run_command_v_opt(repack.argv, RUN_GIT_CMD))\n \t\t\tdie(FAILED_RUN, repack.argv[0]);\n \n@@ -660,7 +660,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \treport_garbage = report_pack_garbage;\n \treprepare_packed_git(the_repository);\n \tif (pack_garbage.nr > 0) {\n-\t\tclose_all_packs(the_repository->objects);\n+\t\tclose_object_store(the_repository->objects);\n \t\tclean_pack_garbage();\n \t}\n \ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex e47d77baee..72d7a7c909 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -449,7 +449,7 @@ static void finish(struct commit *head_commit,\n \t\t\t * We ignore errors in 'gc --auto', since the\n \t\t\t * user should see them.\n \t\t\t */\n-\t\t\tclose_all_packs(the_repository->objects);\n+\t\t\tclose_object_store(the_repository->objects);\n \t\t\trun_command_v_opt(argv_gc_auto, RUN_GIT_CMD);\n \t\t}\n \t}\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 7c7bc13e91..ed30fcd633 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -328,7 +328,7 @@ static int finish_rebase(struct rebase_options *opts)\n \n \tdelete_ref(NULL, \"REBASE_HEAD\", NULL, REF_NO_DEREF);\n \tapply_autostash(opts);\n-\tclose_all_packs(the_repository->objects);\n+\tclose_object_store(the_repository->objects);\n \t/*\n \t * We ignore errors in 'gc --auto', since the\n \t * user should see them.\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex d58b7750b6..92cd1f508c 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -2032,7 +2032,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n \t\t\tproc.git_cmd = 1;\n \t\t\tproc.argv = argv_gc_auto;\n \n-\t\t\tclose_all_packs(the_repository->objects);\n+\t\t\tclose_object_store(the_repository->objects);\n \t\t\tif (!start_command(&proc)) {\n \t\t\t\tif (use_sideband)\n \t\t\t\t\tcopy_to_sideband(proc.err, -1, NULL);\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 67f8978043..4de8b6600c 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -419,7 +419,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tif (!names.nr && !po_args.quiet)\n \t\tprintf_ln(_(\"Nothing new to pack.\"));\n \n-\tclose_all_packs(the_repository->objects);\n+\tclose_object_store(the_repository->objects);\n \n \t/*\n \t * Ok we have prepared all new packfiles.\ndiff --git a/object.c b/object.c\nindex e81d47a79c..cf1a2b7086 100644\n--- a/object.c\n+++ b/object.c\n@@ -517,7 +517,7 @@ void raw_object_store_clear(struct raw_object_store *o)\n \to->loaded_alternates = 0;\n \n \tINIT_LIST_HEAD(&o->packed_git_mru);\n-\tclose_all_packs(o);\n+\tclose_object_store(o);\n \to->packed_git = NULL;\n }\n \ndiff --git a/packfile.c b/packfile.c\nindex ce12bffe3e..017046fcf9 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -337,7 +337,7 @@ void close_pack(struct packed_git *p)\n \tclose_pack_index(p);\n }\n \n-void close_all_packs(struct raw_object_store *o)\n+void close_object_store(struct raw_object_store *o)\n {\n \tstruct packed_git *p;\n \ndiff --git a/packfile.h b/packfile.h\nindex d70c6d9afb..e95e389eb8 100644\n--- a/packfile.h\n+++ b/packfile.h\n@@ -81,7 +81,7 @@ extern uint32_t get_pack_fanout(struct packed_git *p, uint32_t value);\n extern unsigned char *use_pack(struct packed_git *, struct pack_window **, off_t, unsigned long *);\n extern void close_pack_windows(struct packed_git *);\n extern void close_pack(struct packed_git *);\n-extern void close_all_packs(struct raw_object_store *o);\n+extern void close_object_store(struct raw_object_store *o);\n extern void unuse_pack(struct pack_window **);\n extern void clear_delta_base_cache(void);\n extern struct packed_git *add_packed_git(const char *path, size_t path_len, int local);\n-- \ngitgitgadget\n"},{"id":"375870","messageId":"xmqqk1en19pp.fsf@gitster-ct.c.googlers.com","threadId":"51116","inReplyTo":"pull.208.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/3] Close commit-graph before calling 'gc'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-05-19T02:04:02Z","receivedAt":"2019-05-19T02:04:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> This series adds close_commit_graph() to close_all_packs() and then renames \n> close_all_packs() to close_object_store(). This name is more descriptive of\n> its larger purpose.\n\nOK.  Somebody needs to go though all the existing callers of\nclose_all_packs() to see if the above claim is true (unless you\nhaven't done so, that is), but I really like the general direction.\n\nThanks.\n\n\n"},{"id":"375900","messageId":"nycvar.QRO.7.76.6.1905201139120.46@tvgsbejvaqbjf.bet","threadId":"51116","inReplyTo":"pull.208.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/3] Close commit-graph before calling 'gc'","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-05-20T09:40:21Z","receivedAt":"2019-05-20T09:40:45Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Stolee,\n\nOn Fri, 17 May 2019, Derrick Stolee via GitGitGadget wrote:\n\n> In Windows, the way we rename a lock file to replace the underlying file\n> does not work when a process holds a read handle. For this reason, we call\n> close_all_packs() everywhere before starting a git gc --auto subprocess. We\n> also call close_commit_graph() before renaming the commit-graph lock file.\n> But we don't close the commit-graph handles before running gc, which can\n> cause an issue when gc.writeCommitGraph is enabled.\n>\n> This series adds close_commit_graph() to close_all_packs() and then renames\n> close_all_packs() to close_object_store(). This name is more descriptive of\n> its larger purpose.\n>\n> This is based on ds/commit-graph-write-refactor to avoid conflicts.\n\nThank you so much for jumping on my (off-list, in-team) bug report!\n\nFWIW I integrated this series into Git for Windows v2.22.0-rc1 already.\n\nThanks,\nDscho\n"},{"id":"375902","messageId":"nycvar.QRO.7.76.6.1905201141000.46@tvgsbejvaqbjf.bet","threadId":"51116","inReplyTo":"0e948f639fb5209f07f8e3eb356b5886c41ff2be.1558118506.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/3] packfile: close_all_packs to close_object_store","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-05-20T10:01:04Z","receivedAt":"2019-05-20T10:01:31Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Stolee,\n\n*really* minor nit: the commit subject probably wants to have a \"rename\"\nafter the colon ;-)\n\nThe patch looks sensible to me. Since Junio asked for a sanity check\nwhether all of the call sites of `close_all_packs()` actually want to\nclose the MIDX and the commit graph, too, I'll do the \"speak out loud\"\ntype of patch review here (spoiler: all of them check out):\n\nOn Fri, 17 May 2019, Derrick Stolee via GitGitGadget wrote:\n\n> diff --git a/builtin/am.c b/builtin/am.c\n> index 58a2aef28b..9315d32d2a 100644\n> --- a/builtin/am.c\n> +++ b/builtin/am.c\n> @@ -1800,7 +1800,7 @@ static void am_run(struct am_state *state, int resume)\n>  \t */\n>  \tif (!state->rebasing) {\n>  \t\tam_destroy(state);\n> -\t\tclose_all_packs(the_repository->objects);\n> +\t\tclose_object_store(the_repository->objects);\n>  \t\trun_command_v_opt(argv_gc_auto, RUN_GIT_CMD);\n\nHere, we run `git gc --auto`, so we obviously really want to close all\nread handles.\n\nCheck.\n\n>  \t}\n>  }\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index 50bde99618..82ce682c80 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -1240,7 +1240,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n>  \ttransport_disconnect(transport);\n>\n>  \tif (option_dissociate) {\n> -\t\tclose_all_packs(the_repository->objects);\n> +\t\tclose_object_store(the_repository->objects);\n>  \t\tdissociate_from_references();\n\nHere, we prepare for disassociating the reference repository specified via\n`git clone --reference <directory>`. Obviously, we need to let go of all\nthe handles we might have open there.\n\nCheck.\n\n>  \t}\n>\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index b620fd54b4..3aec95608f 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -1670,7 +1670,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n>\n>  \tstring_list_clear(&list, 0);\n>\n> -\tclose_all_packs(the_repository->objects);\n> +\tclose_object_store(the_repository->objects);\n>\n>  \targv_array_pushl(&argv_gc_auto, \"gc\", \"--auto\", NULL);\n\nAgain, a `git gc --auto` that needs closing of all read handles to the\nfiles that might be overwritten by the garbage collection.\n\nCheck.\n\n>  \tif (verbosity < 0)\n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index df2573f124..20c8f1bfe8 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -632,7 +632,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n>  \tgc_before_repack();\n>\n>  \tif (!repository_format_precious_objects) {\n> -\t\tclose_all_packs(the_repository->objects);\n> +\t\tclose_object_store(the_repository->objects);\n>  \t\tif (run_command_v_opt(repack.argv, RUN_GIT_CMD))\n\nHere, we want to repack. AFAICT it is the only sane thing we can do to\ninvalidate whatever we read from the object store into memory.\n\nCheck.\n\n>  \t\t\tdie(FAILED_RUN, repack.argv[0]);\n>\n> @@ -660,7 +660,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n>  \treport_garbage = report_pack_garbage;\n>  \treprepare_packed_git(the_repository);\n>  \tif (pack_garbage.nr > 0) {\n> -\t\tclose_all_packs(the_repository->objects);\n> +\t\tclose_object_store(the_repository->objects);\n>  \t\tclean_pack_garbage();\n\nThis wants to delete a number of files that are now obsolete, and it makes\nsense to make sure that there are no open read handles to those anymore.\nIt is a bit unclear from just reading the code what types of files are\naccumulated into the `pack_garbage` string list, but then, we're in the\nlast throngs of a garbage collection, and *just* about to write a new\ncommit graph (if `gc.writeCommitGraph=true`), so I think it is quite okay\nto close not only the packs here, but everything we opened from the object\nstore.\n\nSo I'd give this a check mark, too.\n\n>  \t}\n>\n> diff --git a/builtin/merge.c b/builtin/merge.c\n> index e47d77baee..72d7a7c909 100644\n> --- a/builtin/merge.c\n> +++ b/builtin/merge.c\n> @@ -449,7 +449,7 @@ static void finish(struct commit *head_commit,\n>  \t\t\t * We ignore errors in 'gc --auto', since the\n>  \t\t\t * user should see them.\n>  \t\t\t */\n> -\t\t\tclose_all_packs(the_repository->objects);\n> +\t\t\tclose_object_store(the_repository->objects);\n>  \t\t\trun_command_v_opt(argv_gc_auto, RUN_GIT_CMD);\n\nObviously yet another `git gc --auto`, so yes, we need to close the object\nstore handles we have.\n\nCheck.\n\n>  \t\t}\n>  \t}\n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index 7c7bc13e91..ed30fcd633 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -328,7 +328,7 @@ static int finish_rebase(struct rebase_options *opts)\n>\n>  \tdelete_ref(NULL, \"REBASE_HEAD\", NULL, REF_NO_DEREF);\n>  \tapply_autostash(opts);\n> -\tclose_all_packs(the_repository->objects);\n> +\tclose_object_store(the_repository->objects);\n>  \t/*\n>  \t * We ignore errors in 'gc --auto', since the\n>  \t * user should see them.\n\nYet another `git gc --auto`.\n\nCheck.\n\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index d58b7750b6..92cd1f508c 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -2032,7 +2032,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n>  \t\t\tproc.git_cmd = 1;\n>  \t\t\tproc.argv = argv_gc_auto;\n>\n> -\t\t\tclose_all_packs(the_repository->objects);\n> +\t\t\tclose_object_store(the_repository->objects);\n>  \t\t\tif (!start_command(&proc)) {\n\nThis `proc` refers to another `git gc --auto` (see a couple lines above,\nstill within the hunk).\n\nCheck.\n\n>  \t\t\t\tif (use_sideband)\n>  \t\t\t\t\tcopy_to_sideband(proc.err, -1, NULL);\n> diff --git a/builtin/repack.c b/builtin/repack.c\n> index 67f8978043..4de8b6600c 100644\n> --- a/builtin/repack.c\n> +++ b/builtin/repack.c\n> @@ -419,7 +419,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n>  \tif (!names.nr && !po_args.quiet)\n>  \t\tprintf_ln(_(\"Nothing new to pack.\"));\n>\n> -\tclose_all_packs(the_repository->objects);\n> +\tclose_object_store(the_repository->objects);\n>\n>  \t/*\n>  \t * Ok we have prepared all new packfiles.\n\nAh, the joys of un-dynamic patch review. What you, dear reader, cannot see\nin this hunk is that the code comment at the end continues thusly:\n\n         * First see if there are packs of the same name and if so\n         * if we can move them out of the way (this can happen if we\n         * repacked immediately after packing fully.\n         */\n\nMeaning: we're about to rename some pack files. So the pack file handles\nneed to be closed, all right, but what about the other object store\nhandles? There is no mention of the commit graph (more on that below), but\nthe loop following the code comment contains this:\n\n                        if (!midx_cleared) {\n                                clear_midx_file(the_repository);\n                                midx_cleared = 1;\n                        }\n\nSo yes, I would give this a check.\n\nIt does puzzle me, I have to admit, that there is no (opt-in) code block\nto re-write the commit graph. After all, the commit graph references the\npack files, right? So if they are repacked, it would at least be\ninvalidated at this point...\n\n> diff --git a/object.c b/object.c\n> index e81d47a79c..cf1a2b7086 100644\n> --- a/object.c\n> +++ b/object.c\n> @@ -517,7 +517,7 @@ void raw_object_store_clear(struct raw_object_store *o)\n>  \to->loaded_alternates = 0;\n>\n>  \tINIT_LIST_HEAD(&o->packed_git_mru);\n> -\tclose_all_packs(o);\n> +\tclose_object_store(o);\n\nWe're in the middle of a function called `raw_object_store_clear()`. So...\n\nCheck.\n\n>  \to->packed_git = NULL;\n>  }\n>\n> diff --git a/packfile.c b/packfile.c\n> index ce12bffe3e..017046fcf9 100644\n> --- a/packfile.c\n> +++ b/packfile.c\n> @@ -337,7 +337,7 @@ void close_pack(struct packed_git *p)\n>  \tclose_pack_index(p);\n>  }\n>\n> -void close_all_packs(struct raw_object_store *o)\n> +void close_object_store(struct raw_object_store *o)\n>  {\n>  \tstruct packed_git *p;\n>\n> diff --git a/packfile.h b/packfile.h\n> index d70c6d9afb..e95e389eb8 100644\n> --- a/packfile.h\n> +++ b/packfile.h\n> @@ -81,7 +81,7 @@ extern uint32_t get_pack_fanout(struct packed_git *p, uint32_t value);\n>  extern unsigned char *use_pack(struct packed_git *, struct pack_window **, off_t, unsigned long *);\n>  extern void close_pack_windows(struct packed_git *);\n>  extern void close_pack(struct packed_git *);\n> -extern void close_all_packs(struct raw_object_store *o);\n> +extern void close_object_store(struct raw_object_store *o);\n>  extern void unuse_pack(struct pack_window **);\n>  extern void clear_delta_base_cache(void);\n>  extern struct packed_git *add_packed_git(const char *path, size_t path_len, int local);\n> --\n> gitgitgadget\n\nAnd this concludes my review.\n\nThank you!\nDscho\n"},{"id":"375906","messageId":"nycvar.QRO.7.76.6.1905201204390.46@tvgsbejvaqbjf.bet","threadId":"51116","inReplyTo":"xmqqk1en19pp.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 0/3] Close commit-graph before calling 'gc'","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-05-20T10:13:23Z","receivedAt":"2019-05-20T10:13:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Sun, 19 May 2019, Junio C Hamano wrote:\n\n> \"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>\n> > This series adds close_commit_graph() to close_all_packs() and then renames\n> > close_all_packs() to close_object_store(). This name is more descriptive of\n> > its larger purpose.\n>\n> OK.  Somebody needs to go though all the existing callers of\n> close_all_packs() to see if the above claim is true (unless you\n> haven't done so, that is), but I really like the general direction.\n\nI tried to provide that in my review of 3/3.\n\nPlease note that these patches are most relevant for Windows, where\nrenames and deletes fail when any process has an open handle to the\nfile(s) in question.\n\nCiao,\nDscho\n\nP.S.: Now that I think of it, if we close the object store, that's\nprobably also a time when we want to release the loose object cache.\n"},{"id":"375908","messageId":"42c3857c-ef26-a05d-e2d8-30a450199fe8@gmail.com","threadId":"51116","inReplyTo":"xmqqk1en19pp.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 0/3] Close commit-graph before calling 'gc'","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-05-20T11:05:31Z","receivedAt":"2019-05-20T11:05:34Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/18/2019 10:04 PM, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> \n>> This series adds close_commit_graph() to close_all_packs() and then renames \n>> close_all_packs() to close_object_store(). This name is more descriptive of\n>> its larger purpose.\n> \n> OK.  Somebody needs to go though all the existing callers of\n> close_all_packs() to see if the above claim is true (unless you\n> haven't done so, that is), but I really like the general direction.\n\nHere is a full inventory. Most of the context can be found simply by\n'git grep -B 10 -A 10 close_object_store' but some cases need reading\na helper function to know for sure.\n\nAbout to call gc or repack:\n\n* am\n* clone\n* fetch\n* merge\n* rebase\n* receive-pack\n\nIs about to delete packs or pack-garbage:\n\n* gc\n* repack\n\nAnd in object.c, we are in the \"raw_object_store_clear()\" method.\n\nThanks,\n-Stolee\n"},{"id":"375911","messageId":"0c13a91f-e45e-d177-758a-30e9517a664f@gmail.com","threadId":"51116","inReplyTo":"nycvar.QRO.7.76.6.1905201141000.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 3/3] packfile: close_all_packs to close_object_store","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-05-20T11:55:10Z","receivedAt":"2019-05-20T11:55:13Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/20/2019 6:01 AM, Johannes Schindelin wrote:\n> Hi Stolee,\n> \n> *really* minor nit: the commit subject probably wants to have a \"rename\"\n> after the colon ;-)\n\nI did put that there, but then the subject line was too long. I'm not\nopposed to putting it back.\n \n> The patch looks sensible to me. Since Junio asked for a sanity check\n> whether all of the call sites of `close_all_packs()` actually want to\n> close the MIDX and the commit graph, too, I'll do the \"speak out loud\"\n> type of patch review here (spoiler: all of them check out):\n\nThanks for the detail here!\n\n>> diff --git a/builtin/repack.c b/builtin/repack.c\n>> index 67f8978043..4de8b6600c 100644\n>> --- a/builtin/repack.c\n>> +++ b/builtin/repack.c\n>> @@ -419,7 +419,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n>>  \tif (!names.nr && !po_args.quiet)\n>>  \t\tprintf_ln(_(\"Nothing new to pack.\"));\n>>\n>> -\tclose_all_packs(the_repository->objects);\n>> +\tclose_object_store(the_repository->objects);\n>>\n>>  \t/*\n>>  \t * Ok we have prepared all new packfiles.\n> \n> Ah, the joys of un-dynamic patch review. What you, dear reader, cannot see\n> in this hunk is that the code comment at the end continues thusly:\n> \n>          * First see if there are packs of the same name and if so\n>          * if we can move them out of the way (this can happen if we\n>          * repacked immediately after packing fully.\n>          */\n> \n> Meaning: we're about to rename some pack files. So the pack file handles\n> need to be closed, all right, but what about the other object store\n> handles? There is no mention of the commit graph (more on that below), but\n> the loop following the code comment contains this:\n> \n>                         if (!midx_cleared) {\n>                                 clear_midx_file(the_repository);\n>                                 midx_cleared = 1;\n>                         }\n> \n> So yes, I would give this a check.\n> \n> It does puzzle me, I have to admit, that there is no (opt-in) code block\n> to re-write the commit graph. After all, the commit graph references the\n> pack files, right? So if they are repacked, it would at least be\n> invalidated at this point...\n\nThe commit-graph does not directly reference the packs. The file will still be\nvalid, except if we GC'd some commits that it references. We have the ability\nto rewrite the graph in 'git gc'.\n\nThe MIDX does reference packs by name, so it needs to be cleared before we delete\npacks. This _could_ be done with more care: we only need to delete it if a pack\nit references is queued for deletion. However, you can do that using the\n'git multi-pack-index expire|repack' pattern currently cooking.\n\nThanks,\n-Stolee\n\n"},{"id":"376315","messageId":"xmqqy32q4k5g.fsf@gitster-ct.c.googlers.com","threadId":"51116","inReplyTo":"0c13a91f-e45e-d177-758a-30e9517a664f@gmail.com","subject":"Re: [PATCH 3/3] packfile: close_all_packs to close_object_store","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-05-28T16:29:47Z","receivedAt":"2019-05-28T16:29:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee <stolee@gmail.com> writes:\n\n> On 5/20/2019 6:01 AM, Johannes Schindelin wrote:\n>> Hi Stolee,\n>> \n>> *really* minor nit: the commit subject probably wants to have a \"rename\"\n>> after the colon ;-)\n>\n> I did put that there, but then the subject line was too long. I'm not\n> opposed to putting it back.\n\nLet me locally amend what I queued in the meantime, then.  Thanks, both.\n"}]}