Re: [PATCH v2 6/7] maintenance: recommended schedule in register/start

From: Martin Ågren <martin.agren@gmail.com>
Date: Sep 29, 2020, 19:48 UTC
Message-ID: <CAN0heSqkJoqXKP5ccaGMA1_ppd0bcQ7G0ozUH+H7tBMonhcrjQ@mail.gmail.com>
In-Reply-To: <f609c1bde27558db2a9601ac34f8a51ce86f0e2c.1599846561.git.gitgitgadget@gmail.com>

Hi Stolee,

On Fri, 11 Sep 2020 at 19:53, Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> wrote:

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.
>
> However, in an effort to recommend a good schedule for repositories of
> all sizes, set new config values for recommended tasks that are safe to
> run in the background while users run foreground Git commands. These
> commands are generally everything but the 'gc' task.

If there aren't any "schedule" configurations, we'll go ahead and sprinkle in quite a few of them. I suppose that another approach would be that later, much later, when we go look for these configuration items, we could go "there is not a single one set, let's act as if *these* were configured".

The advantage there would be that we can tweak those defaults over time. Whereas with the approach of this patch, v2.29.0 will give the user a snapshot of 2020's best practices. If they want to catch up, they will need to drop all their "schedule" config and re-"register", or use a future `git maintenance reregister`. ;-)

Anyway, this is a convenience thing. There's a chance that "convenience" interferes with "perfect" and "optimal". I guess that's to be expected.

> +If your repository has no 'maintenance.<task>.schedule' configuration

Thank you for going above and beyond with marking config items et cetera for rendering in `monospace`. I just noticed that this is slightly mis-marked-upped. If you end up rerolling this patch series for some reason, you might want to switch from 'single quotes' to `backticks` in this particular instance.

While I'm commenting anyway...

Show 24 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_release(&config_name);
> +       return found;
> +}

That `FREE_AND_NULL()` caught me off-guard. The pointer is on the stack. I suppose it doesn't *hurt*, but being careful to set it to NULL made me go "huh".

I suppose you could drop the `!found` check in favour of `break`-ing precisely when you get a hit.

And I do wonder how much the reuse of the "maintenance." part of the buffer helps performance.

In the end, you could use something like the following (not compiled):

  static int has_schedule_config(void)
  {
         int i, found = 0;
         struct strbuf config_name = STRBUF_INIT;

         for (i = 0; i < TASK__COUNT; i++) {
                 const char *value;

                 strbuf_reset(&config_name);
                 strbuf_addf(&config_name, "maintenance.%s.schedule",
tasks[i].name);

                 if (!git_config_get_value(config_name.buf, &value)) {
                         found = 1;
                         break;
                 }
         }

         strbuf_release(&config_name);
         return found;
  }

Anyway, that's just microniting, obviously, but maybe in the sum it has some value.

Martin