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@xxxxxxxxx> 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
