| Message ID | 5fdd8188b1d9b6efc2803b557b3ba344e184d22e.1598380805.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Maintenance III: background maintenance | expand |
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Derrick Stolee <dstolee@microsoft.com> > > Some commands run 'git maintenance run --auto --[no-]quiet' after doing > their normal work, as a way to keep repositories clean as they are used. > Currently, users who do not want this maintenance to occur would set the > 'gc.auto' config option to 0 to avoid the 'gc' task from running. > However, this does not stop the extra process invocation. OK, that is because the configuration is checked on the other side, and the new check is implemented on this side before we decide to spawn the maintenance task. It sounds like a change worth having without even waiting for the "git maintenance" to materialize ;-). > @@ -1868,8 +1869,13 @@ int run_processes_parallel_tr2(int n, get_next_task_fn get_next_task, > > int run_auto_maintenance(int quiet) > { > + int enabled; > struct child_process maint = CHILD_PROCESS_INIT; > > + if (!git_config_get_bool("maintenance.auto", &enabled) && > + !enabled) > + return 0; So in a repository without this configuration, get_bool would fail and we do not short-circuit. Only if the value get_bool sees is false, we return without running the command. Makes sense.
On 8/25/2020 5:44 PM, Junio C Hamano wrote: > "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> From: Derrick Stolee <dstolee@microsoft.com> >> >> Some commands run 'git maintenance run --auto --[no-]quiet' after doing >> their normal work, as a way to keep repositories clean as they are used. >> Currently, users who do not want this maintenance to occur would set the >> 'gc.auto' config option to 0 to avoid the 'gc' task from running. >> However, this does not stop the extra process invocation. > > OK, that is because the configuration is checked on the other side, > and the new check is implemented on this side before we decide to > spawn the maintenance task. > > It sounds like a change worth having without even waiting for the > "git maintenance" to materialize ;-). True. This one could be pulled out of Part III and placed any time after Part I (outside of test script context conflicts). The only reason to wait until after Part I is that the name "maintenance.auto" implies a connection to the maintenance builtin instead of gc. Before the maintenance builtin, it would be natural to see "gc.auto=0" and do this same behavior, but that will not be general enough in the future. If you prefer, I can pull this out into a series on its own to be tracked separately. Thanks, -Stolee
Derrick Stolee <stolee@gmail.com> writes: > Before the maintenance builtin, it would be natural to see "gc.auto=0" > and do this same behavior, but that will not be general enough in the > future. I do think it is an excellent change to move the check done in the need_to_gc() to check the value of gc.auto to run_auto_gc() for exactly the reason the log message of this patch gives. And after the function is renamed to run_auto_maintenance(), at some point the variable that gets checked would also be updated, and we'd eventually reach the same state, I would think. But it is so small a change that it probably is not worth the book-keeping burden of remembering that the maintenance topic needs to build on the patch to update auto-gc. > If you prefer, I can pull this out into a series on its own to be > tracked separately. So let's leave it as-is. Thanks.
diff --git a/Documentation/config/maintenance.txt b/Documentation/config/maintenance.txt index a0706d8f09..06db758172 100644 --- a/Documentation/config/maintenance.txt +++ b/Documentation/config/maintenance.txt @@ -1,3 +1,8 @@ +maintenance.auto:: + This boolean config option controls whether some commands run + `git maintenance run --auto` after doing their normal work. Defaults + to true. + maintenance.<task>.enabled:: This boolean config option controls whether the maintenance task with name `<task>` is run when no `--task` option is specified to diff --git a/run-command.c b/run-command.c index 2ee59acdc8..ea4d0fb4b1 100644 --- a/run-command.c +++ b/run-command.c @@ -7,6 +7,7 @@ #include "strbuf.h" #include "string-list.h" #include "quote.h" +#include "config.h" void child_process_init(struct child_process *child) { @@ -1868,8 +1869,13 @@ int run_processes_parallel_tr2(int n, get_next_task_fn get_next_task, int run_auto_maintenance(int quiet) { + int enabled; struct child_process maint = CHILD_PROCESS_INIT; + if (!git_config_get_bool("maintenance.auto", &enabled) && + !enabled) + return 0; + maint.git_cmd = 1; strvec_pushl(&maint.args, "maintenance", "run", "--auto", NULL); strvec_push(&maint.args, quiet ? "--quiet" : "--no-quiet"); diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh index 6f878b0141..e0ba19e1ff 100755 --- a/t/t7900-maintenance.sh +++ b/t/t7900-maintenance.sh @@ -26,6 +26,19 @@ test_expect_success 'run [--auto|--quiet]' ' test_subcommand git gc --no-quiet <run-no-quiet.txt ' +test_expect_success 'maintenance.auto config option' ' + GIT_TRACE2_EVENT="$(pwd)/default" git commit --quiet --allow-empty -m 1 && + test_subcommand git maintenance run --auto --quiet <default && + GIT_TRACE2_EVENT="$(pwd)/true" \ + git -c maintenance.auto=true \ + commit --quiet --allow-empty -m 2 && + test_subcommand git maintenance run --auto --quiet <true && + GIT_TRACE2_EVENT="$(pwd)/false" \ + git -c maintenance.auto=false \ + commit --quiet --allow-empty -m 3 && + test_subcommand ! git maintenance run --auto --quiet <false +' + test_expect_success 'maintenance.<task>.enabled' ' git config maintenance.gc.enabled false && git config maintenance.commit-graph.enabled true &&