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