Follow-up to js/pull-release-backs-before-fetching
cover Follow-up to js/pull-release-backs-before-fetching
This implements the API enhancements suggested in
https://lore.kernel.org/git/[email protected]: we introduce a way to let run_command() handle the releasing of the file handles to the object store. It should make the code easier to work with in the future because it will be more obvious how to support Windows better when copy/editing code.
Johannes Schindelin (4):
run-command: prettify the RUN_COMMAND_* flags
run-command: offer to close the object store before running run_auto_maintenance(): implicitly close the object store Close object store closer to spawning child processes
builtin/am.c | 1 - builtin/fetch.c | 2 -- builtin/gc.c | 18 ++++++------------ builtin/merge.c | 1 - builtin/pull.c | 3 +-- builtin/rebase.c | 1 - builtin/receive-pack.c | 3 +-- run-command.c | 6 ++++++ run-command.h | 23 ++++++++++++++++------- 9 files changed, 30 insertions(+), 28 deletions(-)
base-commit: 7e44ff7a3983ad0c7be5c9edcfea2e8355ce9a65
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1035%2Fdscho%2Fclose-object-store-in-run-command-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1035/dscho/close-object-store-in-run-command-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1035
Signature · 1 line
gitgitgadget
PATCH 1/4 run-command: prettify the `RUN_COMMAND_*` flags
From: Johannes Schindelin <[email protected]>
The values were listed unaligned, and with powers of two spelled out in decimal. The list is easier to parse for human readers if the numbers are aligned and spelled out as powers of two (using the bit-shift operator <<).
While at it, remove a code comment that was unclear at best, and confusing at worst.
--- run-command.h | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
run-command.h +7 -7
@@ -233,13 +233,13 @@ int run_hook_ve(const char *const *env, const char *name, va_list args);*/ int run_auto_maintenance (int quiet); -#define RUN_COMMAND_NO_STDIN 1 -#define RUN_GIT_CMD 2/*If this is to be git sub-command */ -#define RUN_COMMAND_STDOUT_TO_STDERR 4 -#define RUN_SILENT_EXEC_FAILURE 8 -#define RUN_USING_SHELL 16 -#define RUN_CLEAN_ON_EXIT 32 -#define RUN_WAIT_AFTER_CLEAN 64 +#define RUN_COMMAND_NO_STDIN (1<<0) +#define RUN_GIT_CMD (1<<1) +#define RUN_COMMAND_STDOUT_TO_STDERR (1<<2) +#define RUN_SILENT_EXEC_FAILURE (1<<3) +#define RUN_USING_SHELL (1<<4) +#define RUN_CLEAN_ON_EXIT (1<<5) +#define RUN_WAIT_AFTER_CLEAN (1<<6)/** * Convenience functions that encapsulate a sequence of
Signature · 1 line
gitgitgadget
PATCH 2/4 run-command: offer to close the object store before running
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
Especially on Windows, where files cannot be deleted if _any_ process holds an open file handle to them, it is important to close the object store (releasing all handles to all .pack files) before running a command that might spawn a garbage collection.
This scenario is so common that we frequently see the pattern of closing the object store before running auto maintenance or another Git command.
Let's make this much more convenient by teaching the run_command() machinery a new flag to release the object store before spawning the process.
--- run-command.c | 5 +++++ run-command.h | 9 +++++++++ 2 files changed, 14 insertions(+)
run-command.c +5 -0
@@ -8,6 +8,7 @@#include "string-list.h" #include "quote.h" #include "config.h" +#include "packfile.h" void child_process_init (struct child_process *child) {
@@ -740,6 +741,9 @@ fail_pipe:fflush (NULL ); +if (cmd->close_object_store) +close_object_store (the_repository->objects); +#ifndef GIT_WINDOWS_NATIVE {int notify_pipe[2 ];
@@ -1044,6 +1048,7 @@ int run_command_v_opt_cd_env_tr2(const char **argv, int opt, const char *dir, cmd.use_shell = opt &RUN_USING_SHELL ?1 :0 ; cmd.clean_on_exit = opt &RUN_CLEAN_ON_EXIT ?1 :0 ; cmd.wait_after_clean = opt &RUN_WAIT_AFTER_CLEAN ?1 :0 ; + cmd.close_object_store = opt &RUN_CLOSE_OBJECT_STORE ?1 :0 ; cmd.dir = dir; cmd.env = env; cmd.trace2_child_class = tr2_class;
run-command.h +9 -0
@@ -134,6 +134,14 @@ struct child_process {*/ unsigned use_shell:1; +/** +* Release any open file handles to the object store before running +* the command; This is necessary e.g. when the spawned process may +* want to repack because that would delete `.pack` files (and on +* Windows, you cannot delete files that are still in use). +*/ + unsigned close_object_store:1; + unsigned stdout_to_stderr:1; unsigned clean_on_exit:1; unsigned wait_after_clean:1;
@@ -240,6 +248,7 @@ int run_auto_maintenance(int quiet);#define RUN_USING_SHELL (1<<4)#define RUN_CLEAN_ON_EXIT (1<<5)#define RUN_WAIT_AFTER_CLEAN (1<<6) +#define RUN_CLOSE_OBJECT_STORE (1<<7)/** * Convenience functions that encapsulate a sequence of
Signature · 1 line
gitgitgadget
PATCH 3/4 run_auto_maintenance(): implicitly close the object store
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
Before spawning the auto maintenance, we need to make sure that we release all open file handles to all the .pack files (and MIDX files and commit-graph files and...) so that the maintenance process has the freedom to delete those files.
So far, we did this manually every time before calling run_auto_maintenance(). With the new close_object_store flag, we can do that implicitly in that function, which is more robust because future callers won't be able to forget to close the object store.
Note: this changes behavior slightly, as we previously _always_ closed the object store, but now we only close the object store when actually running the auto maintenance. In practice, this should not matter (if anything, it might speed up operations where auto maintenance is disabled).
--- builtin/am.c | 1 - builtin/fetch.c | 2 -- builtin/merge.c | 1 - builtin/rebase.c | 1 - run-command.c | 1 + 5 files changed, 1 insertion(+), 5 deletions(-)
builtin/am.c +0 -1
@@ -1848,7 +1848,6 @@ next:*/ if (!state->rebasing) {am_destroy (state); -close_object_store (the_repository->objects);run_auto_maintenance (state->quiet); } }
builtin/fetch.c +0 -2
@@ -2133,8 +2133,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix) NULL); } -close_object_store (the_repository->objects); -if (enable_auto_gc)run_auto_maintenance (verbosity <0 );
builtin/merge.c +0 -1
@@ -469,7 +469,6 @@ static void finish(struct commit *head_commit,* We ignore errors in 'gc --auto', since the * user should see them. */ -close_object_store (the_repository->objects);run_auto_maintenance (verbosity <0 ); } }
builtin/rebase.c +0 -1
@@ -740,7 +740,6 @@ static int finish_rebase(struct rebase_options *opts)delete_ref (NULL ,"REBASE_HEAD" ,NULL ,REF_NO_DEREF );unlink (git_path_auto_merge (the_repository));apply_autostash (state_dir_path ("autostash" , opts)); -close_object_store (the_repository->objects);/* * We ignore errors in 'git maintenance run --auto', since the * user should see them.
run-command.c +1 -0
@@ -1891,6 +1891,7 @@ int run_auto_maintenance(int quiet)return 0 ; maint.git_cmd =1 ; + maint.close_object_store =1 ;strvec_pushl (&maint.args,"maintenance" ,"run" ,"--auto" ,NULL );strvec_push (&maint.args, quiet ?"--quiet" :"--no-quiet" );
Signature · 1 line
gitgitgadget
PATCH 4/4 Close object store closer to spawning child processes
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
In many cases where we spawned child processes that _may_ trigger a repack, we explicitly closed the object store first (so that the repack process can delete the .pack files, which would otherwise not be possible on Windows since files cannot be deleted as long as they as still in use).
Wherever possible, we now use the new close_object_store bit of the run_command() API, to delay closing the object store even further. This makes the code easier to maintain because it is now more obvious that we only release those file handles because of those child processes.
--- builtin/gc.c | 18 ++++++------------ builtin/pull.c | 3 +-- builtin/receive-pack.c | 3 +-- 3 files changed, 8 insertions(+), 16 deletions(-)
builtin/gc.c +6 -12
@@ -663,8 +663,8 @@ int cmd_gc(int argc, const char **argv, const char *prefix)gc_before_repack ();if (!repository_format_precious_objects) { -close_object_store (the_repository->objects); -if (run_command_v_opt (repack.v,RUN_GIT_CMD )) +if (run_command_v_opt (repack.v, +RUN_GIT_CMD |RUN_CLOSE_OBJECT_STORE ))die (FAILED_RUN , repack.v[0 ]);if (prune_expire) {
@@ -848,7 +848,7 @@ static int run_write_commit_graph(struct maintenance_run_opts *opts) {struct child_process child =CHILD_PROCESS_INIT ; - child.git_cmd =1 ; + child.git_cmd = child.close_object_store =1 ;strvec_pushl (&child.args,"commit-graph" ,"write" ,"--split" ,"--reachable" ,NULL );
@@ -864,7 +864,6 @@ static int maintenance_task_commit_graph(struct maintenance_run_opts *opts)if (!the_repository->settings.core_commit_graph)return 0 ; -close_object_store (the_repository->objects);if (run_write_commit_graph (opts)) {error (_ ("failed to write commit-graph" ));return 1 ;
@@ -913,7 +912,7 @@ static int maintenance_task_gc(struct maintenance_run_opts *opts) {struct child_process child =CHILD_PROCESS_INIT ; - child.git_cmd =1 ; + child.git_cmd = child.close_object_store =1 ;strvec_push (&child.args,"gc" );if (opts->auto_flag)
@@ -923,7 +922,6 @@ static int maintenance_task_gc(struct maintenance_run_opts *opts)else strvec_push (&child.args,"--no-quiet" ); -close_object_store (the_repository->objects);return run_command (&child); }
@@ -1097,14 +1095,12 @@ static int multi_pack_index_expire(struct maintenance_run_opts *opts) {struct child_process child =CHILD_PROCESS_INIT ; - child.git_cmd =1 ; + child.git_cmd = child.close_object_store =1 ;strvec_pushl (&child.args,"multi-pack-index" ,"expire" ,NULL );if (opts->quiet)strvec_push (&child.args,"--no-progress" ); -close_object_store (the_repository->objects); -if (run_command (&child))return error (_ ("'git multi-pack-index expire' failed" ));
@@ -1155,7 +1151,7 @@ static int multi_pack_index_repack(struct maintenance_run_opts *opts) {struct child_process child =CHILD_PROCESS_INIT ; - child.git_cmd =1 ; + child.git_cmd = child.close_object_store =1 ;strvec_pushl (&child.args,"multi-pack-index" ,"repack" ,NULL );if (opts->quiet)
@@ -1164,8 +1160,6 @@ static int multi_pack_index_repack(struct maintenance_run_opts *opts)strvec_pushf (&child.args,"--batch-size=%" PRIuMAX, (uintmax_t )get_auto_pack_size ()); -close_object_store (the_repository->objects); -if (run_command (&child))return error (_ ("'git multi-pack-index repack' failed" ));
builtin/pull.c +1 -2
@@ -578,7 +578,7 @@ static int run_fetch(const char *repo, const char **refspecs)strvec_pushv (&args, refspecs); }else if (*refspecs)BUG ("refspecs without repo?" ); - ret =run_command_v_opt (args.v,RUN_GIT_CMD ); + ret =run_command_v_opt (args.v,RUN_GIT_CMD |RUN_CLOSE_OBJECT_STORE );strvec_clear (&args);return ret; }
@@ -999,7 +999,6 @@ int cmd_pull(int argc, const char **argv, const char *prefix)oidclr (&rebase_fork_point); } -close_object_store (the_repository->objects);if (run_fetch (repo, refspecs))return 1 ;
builtin/receive-pack.c +1 -2
@@ -2580,10 +2580,9 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix) proc.no_stdin =1 ; proc.stdout_to_stderr =1 ; proc.err = use_sideband ?-1 :0 ; - proc.git_cmd =1 ; + proc.git_cmd = proc.close_object_store =1 ; proc.argv = argv_gc_auto; -close_object_store (the_repository->objects);if (!start_command (&proc)) {if (use_sideband)copy_to_sideband (proc.err,-1 ,NULL );
Signature · 1 line
gitgitgadget