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

[Derrick Stolee <stolee@gmail.com>]

On 9/29/2020 3:48 PM, Martin Ågren wrote:
> Hi Stolee,
> 
> On Fri, 11 Sep 2020 at 19:53, Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>> 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".

I do like this alternative.

> 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`. ;-)

This is a significant advantage! Great idea.

It might be a bit difficult to slide this in, but I bet it would work
out OK if we have a "initialize_schedule()" option that is only run
when the "--schedule=<...>" option is given? The trickiest part is
actually setting the ".enabled" configs to "true" as well. The condition
for using the "default" schedule might get a bit complicated. I do think
it is worth some effort to do, as adjusting defaults in code is certainly
easier than modifying config values.

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

Thanks! Yeah that was a mis-type.

> While I'm commenting anyway...
> 
>> +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.

All valid points.

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

Sounds good to me. I'll work on a new version that makes your
recommendations.

Thanks,
-Stolee