{"thread":{"id":"56471","subject":"[PATCH 0/4] Follow-up to js/pull-release-backs-before-fetching","startedAt":"2021-09-09T09:47:13Z","lastAt":"2021-09-09T09:47:18Z","messageCount":5,"participants":["Johannes Schindelin via GitGitGadget"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"435225","messageId":"pull.1035.git.1631180828.gitgitgadget@gmail.com","threadId":"56471","inReplyTo":null,"subject":"[PATCH 0/4] Follow-up to js/pull-release-backs-before-fetching","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-09T09:47:04Z","receivedAt":"2021-09-09T09:47:13Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"This implements the API enhancements suggested in\nhttps://lore.kernel.org/git/xmqqh7eu3mjb.fsf@gitster.g: we introduce a way\nto let run_command() handle the releasing of the file handles to the object\nstore. It should make the code easier to work with in the future because it\nwill be more obvious how to support Windows better when copy/editing code.\n\nJohannes Schindelin (4):\n run-command: prettify the `RUN_COMMAND_*` flags\n run-command: offer to close the object store before running\n run_auto_maintenance(): implicitly close the object store\n Close object store closer to spawning child processes\n\n builtin/am.c | 1 -\n builtin/fetch.c | 2 --\n builtin/gc.c | 18 ++++++------------\n builtin/merge.c | 1 -\n builtin/pull.c | 3 +--\n builtin/rebase.c | 1 -\n builtin/receive-pack.c | 3 +--\n run-command.c | 6 ++++++\n run-command.h | 23 ++++++++++++++++-------\n 9 files changed, 30 insertions(+), 28 deletions(-)\n\n\nbase-commit: 7e44ff7a3983ad0c7be5c9edcfea2e8355ce9a65\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1035%2Fdscho%2Fclose-object-store-in-run-command-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1035/dscho/close-object-store-in-run-command-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1035\n-- \ngitgitgadget\n"},{"id":"435226","messageId":"75f39fc80f46455a93f312cce2e989711c748f8a.1631180828.git.gitgitgadget@gmail.com","threadId":"56471","inReplyTo":"pull.1035.git.1631180828.gitgitgadget@gmail.com","subject":"[PATCH 1/4] run-command: prettify the `RUN_COMMAND_*` flags","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-09T09:47:05Z","receivedAt":"2021-09-09T09:47:14Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe values were listed unaligned, and with powers of two spelled out in\ndecimal. The list is easier to parse for human readers if the numbers\nare aligned and spelled out as powers of two (using the bit-shift\noperator `<<`).\n\nWhile at it, remove a code comment that was unclear at best, and\nconfusing at worst.\n\nSigned-off-by: Johannes Schindelin \n---\n run-command.h | 14 +++++++-------\n 1 file changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/run-command.h b/run-command.h\nindex af1296769f9..3893193f32f 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -233,13 +233,13 @@ int run_hook_ve(const char *const *env, const char *name, va_list args);\n */\n int run_auto_maintenance(int quiet);\n \n-#define RUN_COMMAND_NO_STDIN 1\n-#define RUN_GIT_CMD\t 2\t/*If this is to be git sub-command */\n-#define RUN_COMMAND_STDOUT_TO_STDERR 4\n-#define RUN_SILENT_EXEC_FAILURE 8\n-#define RUN_USING_SHELL 16\n-#define RUN_CLEAN_ON_EXIT 32\n-#define RUN_WAIT_AFTER_CLEAN 64\n+#define RUN_COMMAND_NO_STDIN\t\t(1<<0)\n+#define RUN_GIT_CMD\t\t\t(1<<1)\n+#define RUN_COMMAND_STDOUT_TO_STDERR\t(1<<2)\n+#define RUN_SILENT_EXEC_FAILURE\t\t(1<<3)\n+#define RUN_USING_SHELL\t\t\t(1<<4)\n+#define RUN_CLEAN_ON_EXIT\t\t(1<<5)\n+#define RUN_WAIT_AFTER_CLEAN\t\t(1<<6)\n \n /**\n * Convenience functions that encapsulate a sequence of\n-- \ngitgitgadget\n\n"},{"id":"435227","messageId":"df76ee7b77f0a81db32e28eb4c8a253a19e68028.1631180828.git.gitgitgadget@gmail.com","threadId":"56471","inReplyTo":"pull.1035.git.1631180828.gitgitgadget@gmail.com","subject":"[PATCH 2/4] run-command: offer to close the object store before running","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-09T09:47:06Z","receivedAt":"2021-09-09T09:47:15Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nEspecially on Windows, where files cannot be deleted if _any_ process\nholds an open file handle to them, it is important to close the object\nstore (releasing all handles to all `.pack` files) before running a\ncommand that might spawn a garbage collection.\n\nThis scenario is so common that we frequently see the pattern of closing\nthe object store before running auto maintenance or another Git command.\n\nLet's make this much more convenient by teaching the `run_command()`\nmachinery a new flag to release the object store before spawning the\nprocess.\n\nSigned-off-by: Johannes Schindelin \n---\n run-command.c | 5 +++++\n run-command.h | 9 +++++++++\n 2 files changed, 14 insertions(+)\n\ndiff --git a/run-command.c b/run-command.c\nindex f72e72cce73..e2dc6243774 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -8,6 +8,7 @@\n #include \"string-list.h\"\n #include \"quote.h\"\n #include \"config.h\"\n+#include \"packfile.h\"\n \n void child_process_init(struct child_process *child)\n {\n@@ -740,6 +741,9 @@ fail_pipe:\n \n \tfflush(NULL);\n \n+\tif (cmd->close_object_store)\n+\t\tclose_object_store(the_repository->objects);\n+\n #ifndef GIT_WINDOWS_NATIVE\n {\n \tint notify_pipe[2];\n@@ -1044,6 +1048,7 @@ int run_command_v_opt_cd_env_tr2(const char **argv, int opt, const char *dir,\n \tcmd.use_shell = opt & RUN_USING_SHELL ? 1 : 0;\n \tcmd.clean_on_exit = opt & RUN_CLEAN_ON_EXIT ? 1 : 0;\n \tcmd.wait_after_clean = opt & RUN_WAIT_AFTER_CLEAN ? 1 : 0;\n+\tcmd.close_object_store = opt & RUN_CLOSE_OBJECT_STORE ? 1 : 0;\n \tcmd.dir = dir;\n \tcmd.env = env;\n \tcmd.trace2_child_class = tr2_class;\ndiff --git a/run-command.h b/run-command.h\nindex 3893193f32f..ad207daced7 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -134,6 +134,14 @@ struct child_process {\n \t */\n \tunsigned use_shell:1;\n \n+\t/**\n+\t * Release any open file handles to the object store before running\n+\t * the command; This is necessary e.g. when the spawned process may\n+\t * want to repack because that would delete `.pack` files (and on\n+\t * Windows, you cannot delete files that are still in use).\n+\t */\n+\tunsigned close_object_store:1;\n+\n \tunsigned stdout_to_stderr:1;\n \tunsigned clean_on_exit:1;\n \tunsigned wait_after_clean:1;\n@@ -240,6 +248,7 @@ int run_auto_maintenance(int quiet);\n #define RUN_USING_SHELL\t\t\t(1<<4)\n #define RUN_CLEAN_ON_EXIT\t\t(1<<5)\n #define RUN_WAIT_AFTER_CLEAN\t\t(1<<6)\n+#define RUN_CLOSE_OBJECT_STORE\t\t(1<<7)\n \n /**\n * Convenience functions that encapsulate a sequence of\n-- \ngitgitgadget\n\n"},{"id":"435228","messageId":"990d4ca934862845429506583b73bc2fd4ad2405.1631180828.git.gitgitgadget@gmail.com","threadId":"56471","inReplyTo":"pull.1035.git.1631180828.gitgitgadget@gmail.com","subject":"[PATCH 3/4] run_auto_maintenance(): implicitly close the object store","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-09T09:47:07Z","receivedAt":"2021-09-09T09:47:17Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nBefore spawning the auto maintenance, we need to make sure that we\nrelease all open file handles to all the `.pack` files (and MIDX files\nand commit-graph files and...) so that the maintenance process has the\nfreedom to delete those files.\n\nSo far, we did this manually every time before calling\n`run_auto_maintenance()`. With the new `close_object_store` flag, we can\ndo that implicitly in that function, which is more robust because future\ncallers won't be able to forget to close the object store.\n\nNote: this changes behavior slightly, as we previously _always_ closed\nthe object store, but now we only close the object store when actually\nrunning the auto maintenance. In practice, this should not matter (if\nanything, it might speed up operations where auto maintenance is\ndisabled).\n\nSuggested-by: Junio C Hamano \nSigned-off-by: Johannes Schindelin \n---\n builtin/am.c | 1 -\n builtin/fetch.c | 2 --\n builtin/merge.c | 1 -\n builtin/rebase.c | 1 -\n run-command.c | 1 +\n 5 files changed, 1 insertion(+), 5 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 0c2ad96b70e..f239e4ddde0 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1848,7 +1848,6 @@ next:\n \t */\n \tif (!state->rebasing) {\n \t\tam_destroy(state);\n-\t\tclose_object_store(the_repository->objects);\n \t\trun_auto_maintenance(state->quiet);\n \t}\n }\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 25740c13df1..c9ac8664e18 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -2133,8 +2133,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\t\t\t\t NULL);\n \t}\n \n-\tclose_object_store(the_repository->objects);\n-\n \tif (enable_auto_gc)\n \t\trun_auto_maintenance(verbosity < 0);\n \ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 22f23990b37..e4994e369af 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -469,7 +469,6 @@ 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_object_store(the_repository->objects);\n \t\t\trun_auto_maintenance(verbosity < 0);\n \t\t}\n \t}\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 33e09619005..ba09ebb9e66 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -740,7 +740,6 @@ static int finish_rebase(struct rebase_options *opts)\n \tdelete_ref(NULL, \"REBASE_HEAD\", NULL, REF_NO_DEREF);\n \tunlink(git_path_auto_merge(the_repository));\n \tapply_autostash(state_dir_path(\"autostash\", opts));\n-\tclose_object_store(the_repository->objects);\n \t/*\n \t * We ignore errors in 'git maintenance run --auto', since the\n \t * user should see them.\ndiff --git a/run-command.c b/run-command.c\nindex e2dc6243774..229bdff9971 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1891,6 +1891,7 @@ int run_auto_maintenance(int quiet)\n \t\treturn 0;\n \n \tmaint.git_cmd = 1;\n+\tmaint.close_object_store = 1;\n \tstrvec_pushl(&maint.args, \"maintenance\", \"run\", \"--auto\", NULL);\n \tstrvec_push(&maint.args, quiet ? \"--quiet\" : \"--no-quiet\");\n \n-- \ngitgitgadget\n\n"},{"id":"435229","messageId":"dd708c84687b40a5482f4c6ac8a39e23fc59ac3b.1631180828.git.gitgitgadget@gmail.com","threadId":"56471","inReplyTo":"pull.1035.git.1631180828.gitgitgadget@gmail.com","subject":"[PATCH 4/4] Close object store closer to spawning child processes","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-09T09:47:08Z","receivedAt":"2021-09-09T09:47:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn many cases where we spawned child processes that _may_ trigger a\nrepack, we explicitly closed the object store first (so that the\n`repack` process can delete the `.pack` files, which would otherwise not\nbe possible on Windows since files cannot be deleted as long as they as\nstill in use).\n\nWherever possible, we now use the new `close_object_store` bit of the\n`run_command()` API, to delay closing the object store even further.\nThis makes the code easier to maintain because it is now more obvious\nthat we only release those file handles because of those child\nprocesses.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/gc.c | 18 ++++++------------\n builtin/pull.c | 3 +--\n builtin/receive-pack.c | 3 +--\n 3 files changed, 8 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex f05d2f0a1ac..ddee9f8324f 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -663,8 +663,8 @@ 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_object_store(the_repository->objects);\n-\t\tif (run_command_v_opt(repack.v, RUN_GIT_CMD))\n+\t\tif (run_command_v_opt(repack.v,\n+\t\t\t\t RUN_GIT_CMD | RUN_CLOSE_OBJECT_STORE))\n \t\t\tdie(FAILED_RUN, repack.v[0]);\n \n \t\tif (prune_expire) {\n@@ -848,7 +848,7 @@ static int run_write_commit_graph(struct maintenance_run_opts *opts)\n {\n \tstruct child_process child = CHILD_PROCESS_INIT;\n \n-\tchild.git_cmd = 1;\n+\tchild.git_cmd = child.close_object_store = 1;\n \tstrvec_pushl(&child.args, \"commit-graph\", \"write\",\n \t\t \"--split\", \"--reachable\", NULL);\n \n@@ -864,7 +864,6 @@ static int maintenance_task_commit_graph(struct maintenance_run_opts *opts)\n \tif (!the_repository->settings.core_commit_graph)\n \t\treturn 0;\n \n-\tclose_object_store(the_repository->objects);\n \tif (run_write_commit_graph(opts)) {\n \t\terror(_(\"failed to write commit-graph\"));\n \t\treturn 1;\n@@ -913,7 +912,7 @@ static int maintenance_task_gc(struct maintenance_run_opts *opts)\n {\n \tstruct child_process child = CHILD_PROCESS_INIT;\n \n-\tchild.git_cmd = 1;\n+\tchild.git_cmd = child.close_object_store = 1;\n \tstrvec_push(&child.args, \"gc\");\n \n \tif (opts->auto_flag)\n@@ -923,7 +922,6 @@ static int maintenance_task_gc(struct maintenance_run_opts *opts)\n \telse\n \t\tstrvec_push(&child.args, \"--no-quiet\");\n \n-\tclose_object_store(the_repository->objects);\n \treturn run_command(&child);\n }\n \n@@ -1097,14 +1095,12 @@ static int multi_pack_index_expire(struct maintenance_run_opts *opts)\n {\n \tstruct child_process child = CHILD_PROCESS_INIT;\n \n-\tchild.git_cmd = 1;\n+\tchild.git_cmd = child.close_object_store = 1;\n \tstrvec_pushl(&child.args, \"multi-pack-index\", \"expire\", NULL);\n \n \tif (opts->quiet)\n \t\tstrvec_push(&child.args, \"--no-progress\");\n \n-\tclose_object_store(the_repository->objects);\n-\n \tif (run_command(&child))\n \t\treturn error(_(\"'git multi-pack-index expire' failed\"));\n \n@@ -1155,7 +1151,7 @@ static int multi_pack_index_repack(struct maintenance_run_opts *opts)\n {\n \tstruct child_process child = CHILD_PROCESS_INIT;\n \n-\tchild.git_cmd = 1;\n+\tchild.git_cmd = child.close_object_store = 1;\n \tstrvec_pushl(&child.args, \"multi-pack-index\", \"repack\", NULL);\n \n \tif (opts->quiet)\n@@ -1164,8 +1160,6 @@ static int multi_pack_index_repack(struct maintenance_run_opts *opts)\n \tstrvec_pushf(&child.args, \"--batch-size=%\"PRIuMAX,\n \t\t\t\t (uintmax_t)get_auto_pack_size());\n \n-\tclose_object_store(the_repository->objects);\n-\n \tif (run_command(&child))\n \t\treturn error(_(\"'git multi-pack-index repack' failed\"));\n \ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex d9f0156d969..751372041c2 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -578,7 +578,7 @@ static int run_fetch(const char *repo, const char **refspecs)\n \t\tstrvec_pushv(&args, refspecs);\n \t} else if (*refspecs)\n \t\tBUG(\"refspecs without repo?\");\n-\tret = run_command_v_opt(args.v, RUN_GIT_CMD);\n+\tret = run_command_v_opt(args.v, RUN_GIT_CMD | RUN_CLOSE_OBJECT_STORE);\n \tstrvec_clear(&args);\n \treturn ret;\n }\n@@ -999,7 +999,6 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \t\t\toidclr(&rebase_fork_point);\n \t}\n \n-\tclose_object_store(the_repository->objects);\n \tif (run_fetch(repo, refspecs))\n \t\treturn 1;\n \ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 2d1f97e1ca7..9d5e0e3d11c 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -2580,10 +2580,9 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n \t\t\tproc.no_stdin = 1;\n \t\t\tproc.stdout_to_stderr = 1;\n \t\t\tproc.err = use_sideband ? -1 : 0;\n-\t\t\tproc.git_cmd = 1;\n+\t\t\tproc.git_cmd = proc.close_object_store = 1;\n \t\t\tproc.argv = argv_gc_auto;\n \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);\n-- \ngitgitgadget\n"}]}