| Message ID | 929b6bfa08132523ee97f5adc376c3600f779a99.1713334241.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | b396ee6bed7882af3333eb52333f8a34b648437d |
| Headers | show |
| Series | builtin/receive-pack: convert to use git-maintenance(1) | expand |
Patrick Steinhardt <ps@pks.im> writes: > The `run_auto_maintenance()` function is responsible for spawning a new > `git maintenance run --auto` process. To do so, it sets up the `sturct > child_process` and then runs it by executing `run_command()` directly. > This is rather inflexible in case callers want to modify the child > process somewhat, e.g. to redirect stderr or stdout. > > Introduce a new `prepare_auto_maintenance()` function to plug this gap. I guess the mention of "inflexible" and "redirection" above refers to some incompatibile behaviour we would introduce if we just replaced the manual spawning of "gc --auto" with a call to run_auto_maintenance(), but I would have expected that will be solved by making the interface to run_auto_maintenance() richer, not forcing the callers that would want to deviate from the norm to write the second half of the run_auto_maintenance() themselves. > +int run_auto_maintenance(int quiet) > +{ > + struct child_process maint = CHILD_PROCESS_INIT; > + if (!prepare_auto_maintenance(quiet, &maint)) > + return 0; > return run_command(&maint); > } But given that the "second half" is to just call run_command() on the prepared child control structure, it is probably not a huge deal. It just felt somewhat an uneven API surface that 'quiet' can be controlled with just a single bit and doing anything more than that would require the caller to go into the structure to tweak. Will queue. Thanks.
On Wed, Apr 17, 2024 at 08:53:25AM -0700, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: > > > The `run_auto_maintenance()` function is responsible for spawning a new > > `git maintenance run --auto` process. To do so, it sets up the `sturct > > child_process` and then runs it by executing `run_command()` directly. > > This is rather inflexible in case callers want to modify the child > > process somewhat, e.g. to redirect stderr or stdout. > > > > Introduce a new `prepare_auto_maintenance()` function to plug this gap. > > I guess the mention of "inflexible" and "redirection" above refers > to some incompatibile behaviour we would introduce if we just > replaced the manual spawning of "gc --auto" with a call to > run_auto_maintenance(), but I would have expected that will be > solved by making the interface to run_auto_maintenance() richer, not > forcing the callers that would want to deviate from the norm to > write the second half of the run_auto_maintenance() themselves. > > > +int run_auto_maintenance(int quiet) > > +{ > > + struct child_process maint = CHILD_PROCESS_INIT; > > + if (!prepare_auto_maintenance(quiet, &maint)) > > + return 0; > > return run_command(&maint); > > } > > But given that the "second half" is to just call run_command() on > the prepared child control structure, it is probably not a huge > deal. It just felt somewhat an uneven API surface that 'quiet' can > be controlled with just a single bit and doing anything more than > that would require the caller to go into the structure to tweak. > > Will queue. Thanks. git-receive-pack(1) needs to do some magic with file descriptors and needs to copy output of the command to the sideband. I first thought about extending `run_auto_maintenance()` to support this, but found it to be messy as this really is quite a specific usecase. So I figured that prepping the `struct child_process` like this is the nicer way to approach it. Thanks. Patrick
diff --git a/run-command.c b/run-command.c index 0e7435718a..1b821042b4 100644 --- a/run-command.c +++ b/run-command.c @@ -1793,20 +1793,27 @@ void run_processes_parallel(const struct run_process_parallel_opts *opts) trace2_region_leave(tr2_category, tr2_label, NULL); } -int run_auto_maintenance(int quiet) +int prepare_auto_maintenance(int quiet, struct child_process *maint) { int enabled; - struct child_process maint = CHILD_PROCESS_INIT; if (!git_config_get_bool("maintenance.auto", &enabled) && !enabled) 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"); + 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"); + + return 1; +} +int run_auto_maintenance(int quiet) +{ + struct child_process maint = CHILD_PROCESS_INIT; + if (!prepare_auto_maintenance(quiet, &maint)) + return 0; return run_command(&maint); } diff --git a/run-command.h b/run-command.h index 1f22cc3827..55f6631a2a 100644 --- a/run-command.h +++ b/run-command.h @@ -217,6 +217,13 @@ int finish_command_in_signal(struct child_process *); */ int run_command(struct child_process *); +/* + * Prepare a `struct child_process` to run auto-maintenance. Returns 1 if the + * process has been prepared and is ready to run, or 0 in case auto-maintenance + * should be skipped. + */ +int prepare_auto_maintenance(int quiet, struct child_process *maint); + /* * Trigger an auto-gc */
The `run_auto_maintenance()` function is responsible for spawning a new `git maintenance run --auto` process. To do so, it sets up the `sturct child_process` and then runs it by executing `run_command()` directly. This is rather inflexible in case callers want to modify the child process somewhat, e.g. to redirect stderr or stdout. Introduce a new `prepare_auto_maintenance()` function to plug this gap. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- run-command.c | 19 +++++++++++++------ run-command.h | 7 +++++++ 2 files changed, 20 insertions(+), 6 deletions(-)