Re: [PATCH v3 6/7] maintenance: use default schedule if not configured
- From
- Martin Ågren <martin.agren@gmail.com>
- Date
- Oct 5, 2020, 19:57 UTC
- Message-ID
- <20201005195701.14268-1-martin.agren@gmail.com>
- In-Reply-To
- <d833fffe89c94ecf3551c075960ba6d7607e9b17.1601902635.git.gitgitgadget@gmail.com>
Hi Stolee,
On Mon, 5 Oct 2020 at 15:07, Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> wrote:
> The 'git maintenance (register|start)' subcommands add the current > repository to the global Git config so maintenance will operate on that > repository. It does not specify what maintenance should occur or how > often.
I see that you posted a "how about this?" some days ago. I was offline for the weekend with some margin on both sides, so I didn't see it until now. Good that you just went ahead and posted the whole series anyway.
Show 8 quoted lines
> If a user sets any 'maintenance.<task>.schedule' config value, then > they have chosen a specific schedule for themselves and Git should > respect that when running 'git maintenance run --schedule=<frequency>'. > > To make this process extremely simple for users, assume a default > schedule when no 'maintenance.<task>.schedule' or '...enabled' config > settings are concretely set. This is only an in-process assumption, so > future versions of Git could adjust this expected schedule.
This obviously makes sense to me. ;-) One thing it does mean though is that something like this:
$ git maintenance register # Time goes by... # Someone said to try this: $ git config --add maintenance.commit-graph.schedule hourly $ git config --add maintenance.commit-graph.enable true # That could have been a no-op, since we were already on # such an hourly schedule, but it will effectively turn off # all other scheduled tasks. So some time later: # -- Why are my fetches so slow all of a sudden? :-(
That could be different if `git maintenance register` would turn on, say, `maintenance.baseSchedule = standard` where setting those `maintenance.commit-graph.*` would tweak that "standard" "base schedule" (in a no-op way as it happens).
> --- a/Documentation/git-maintenance.txt > +++ b/Documentation/git-maintenance.txt > @@ -37,6 +37,21 @@ register::
Adding some more context manually:
register:: Initialize Git config values so any scheduled maintenance will start running on this repository. This adds the repository to the `maintenance.repo` config variable in the current user's global config and enables some recommended configuration values for
> `maintenance.<task>.schedule`. The tasks that are enabled are safe > for running in the background without disrupting foreground > processes.
The part about "and enables some recommended configuration values" should probably be in this patch, not an earlier one, and maybe it shouldn't even be here. With the new approach of this version, this doesn't really enable some recommended configuration values. Or maybe it does, I can't make up my mind, nor can I come up with an alternative formulation.
Show 6 quoted lines
> ++ > +If your repository has no `maintenance.<task>.schedule` configuration > +values set, then Git will use a recommended default schedule that performs > +background maintenance that will not interrupt foreground commands. The > +default schedule is as follows: > ++
If you add a line of "--" here...
Show 5 quoted lines
> +* `gc`: disabled. > +* `commit-graph`: hourly. > +* `prefetch`: hourly. > +* `loose-objects`: daily. > +* `incremental-repack`: daily.
... and one here, you'll drop some indentation at this point so that the next paragraph doesn't align with the list above. (See patch below.)
> ++ > +`git maintenance register` will also disable foreground maintenance by > +setting `maintenance.auto = false` in the current repository. This config > +setting will remain after a `git maintenance unregister` command.
That last paragraph does belong here. The part about the different tasks ... maybe. With this new approach of not actually setting any `schedule`/`enabled` configuration variables, that list doesn't obviously have its natural home here. Maybe under `--schedule`, which is where the detection actually happens and the default defaults are imposed? Or maybe in a separate "CONFIGURATION" section. It could include config/maintenance.txt, then go on to define the whole fallback mechanism without having to worry about breaking the reader's flow. (The way it is now, this `register` command is fairly heavy compared to the surrounding parts.)
Show 32 quoted lines
> +static int has_schedule_config(void)
> +{
> + int i, found = 0;
> + struct strbuf config_name = STRBUF_INIT;
> + size_t prefix;
> +
> + strbuf_addstr(&config_name, "maintenance.");
> + prefix = config_name.len;
> +
> + for (i = 0; !found && i < TASK__COUNT; i++) {
> + char *value;
> +
> + strbuf_setlen(&config_name, prefix);
> + strbuf_addf(&config_name, "%s.schedule", tasks[i].name);
> +
> + if (!git_config_get_string(config_name.buf, &value)) {
> + found = 1;
> + FREE_AND_NULL(value);
> + }
> +
> + strbuf_setlen(&config_name, prefix);
> + strbuf_addf(&config_name, "%s.enabled", tasks[i].name);
> +
> + if (!git_config_get_string(config_name.buf, &value)) {
> + found = 1;
> + FREE_AND_NULL(value);
> + }
> + }
> +
> + strbuf_release(&config_name);
> + return found;
> +}I had the same reaction to `FREE_AND_NULL()` as on my previous reading. If you have $reasons for doing it this way, not a big deal. I offer a suggestion in patch form below anyway. Feel free to squash, adapt or ignore as you see fit.
Show 21 quoted lines
> +
> +static void set_recommended_schedule(void)
> +{
> + if (has_schedule_config())
> + return;
> +
> + tasks[TASK_GC].enabled = 0;
> +
> + tasks[TASK_PREFETCH].enabled = 1;
> + tasks[TASK_PREFETCH].schedule = SCHEDULE_HOURLY;
> +
> + tasks[TASK_COMMIT_GRAPH].enabled = 1;
> + tasks[TASK_COMMIT_GRAPH].schedule = SCHEDULE_HOURLY;
> +
> + tasks[TASK_LOOSE_OBJECTS].enabled = 1;
> + tasks[TASK_LOOSE_OBJECTS].schedule = SCHEDULE_DAILY;
> +
> + tasks[TASK_INCREMENTAL_REPACK].enabled = 1;
> + tasks[TASK_INCREMENTAL_REPACK].schedule = SCHEDULE_DAILY;
> +}
> +One thing I can't make up my mind about is how these `enabled` are used for two purposes: Deciding what to do on `git maintenance run` without any `--task`, and deciding what to do on `git maintenance run --scheduled`.
Show 9 quoted lines
> static int maintenance_run_tasks(struct maintenance_run_opts *opts)
> {
> int i, found_selected = 0;
> @@ -1280,6 +1333,8 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts)
>
> if (found_selected)
> QSORT(tasks, TASK__COUNT, compare_tasks_by_selection);
> + else if (opts->schedule != SCHEDULE_NONE)
> + set_recommended_schedule();... And especially how we only impose the magic `maintenance.<task>.enabled` values when we are running with `--schedule`. So the answer to "what is the default value of `maintenance.commit-graph.enabled`?" is "it depends on several factors".
Sort of related: The presence of a `maintenance.<task>.schedule` is not sufficient to schedule the task. This looks like something that one could easily trip on. Maybe you have already considered letting a zero value for `maintenance.<task>.schedule` mean "disabled" and ignoring the `enabled` config item for the scheduled runs, but rejected that for good reasons?
Show 23 quoted lines
> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh > index 7715e40391..7154987fd2 100755 > --- a/t/t7900-maintenance.sh > +++ b/t/t7900-maintenance.sh > @@ -305,11 +305,14 @@ test_expect_success 'register and unregister' ' > git config --global --add maintenance.repo /existing1 && > git config --global --add maintenance.repo /existing2 && > git config --global --get-all maintenance.repo >before && > + > git maintenance register && > - git config --global --get-all maintenance.repo >actual && > - cp before after && > - pwd >>after && > - test_cmp after actual && > + test_cmp_config false maintenance.auto && > + git config --global --get-all maintenance.repo >between && > + cp before expect && > + pwd >>expect && > + test_cmp expect between && > + > git maintenance unregister && > git config --global --get-all maintenance.repo >actual && > test_cmp before actual
This tests the one-time config tweaking. But we don't test any of the "detect no config and impose a default" logic. Neither that it kicks in at all, nor that it doesn't when it shouldn't.
As mentioned above, I end with some minor suggestions.
Martin
diff --git a/Documentation/git-maintenance.txt b/Documentation/git-maintenance.txt index 738a4c7ebd..2085b53dc5 100644 --- a/Documentation/git-maintenance.txt +++ b/Documentation/git-maintenance.txt @@ -43,11 +43,13 @@ values set, then Git will use a recommended default schedule that performs background maintenance that will not interrupt foreground commands. The default schedule is as follows: + +-- * `gc`: disabled. * `commit-graph`: hourly. * `prefetch`: hourly. * `loose-objects`: daily. * `incremental-repack`: daily. +-- + `git maintenance register` will also disable foreground maintenance by setting `maintenance.auto = false` in the current repository. This config diff --git a/builtin/gc.c b/builtin/gc.c index 965690704b..63f4c102b1 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -1253,35 +1253,31 @@ static int compare_tasks_by_selection(const void *a_, const void *b_) static int has_schedule_config(void) { - int i, found = 0; + int i; struct strbuf config_name = STRBUF_INIT; size_t prefix; + char *value = NULL; strbuf_addstr(&config_name, "maintenance."); prefix = config_name.len; - for (i = 0; !found && i < TASK__COUNT; i++) { - char *value; - + for (i = 0; i < TASK__COUNT; i++) { strbuf_setlen(&config_name, prefix); strbuf_addf(&config_name, "%s.schedule", tasks[i].name); - if (!git_config_get_string(config_name.buf, &value)) { - found = 1; - FREE_AND_NULL(value); - } + if (!git_config_get_string(config_name.buf, &value)) + break; strbuf_setlen(&config_name, prefix); strbuf_addf(&config_name, "%s.enabled", tasks[i].name); - if (!git_config_get_string(config_name.buf, &value)) { - found = 1; - FREE_AND_NULL(value); - } + if (!git_config_get_string(config_name.buf, &value)) + break; } strbuf_release(&config_name); - return found; + free(value); + return i < TASK__COUNT; } static void set_recommended_schedule(void)
-- 2.28.0.297.g1956fa8f8d