Re: [PATCH v3 6/7] maintenance: use default schedule if not configured

["Martin Ågren" <martin.agren@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.

> 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.

> ++
> +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...

> +* `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.)

> +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.

> +
> +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`.

>  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?

> 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