Re: [PATCH 6/6] maintenance: use random minute in systemd scheduler

[Taylor Blau <me@ttaylorr.com>]

On Mon, Aug 07, 2023 at 06:51:40PM +0000, Derrick Stolee via GitGitGadget wrote:
> In order to set these schedules to a given minute, we can no longer use
> the 'hourly', 'daily', or 'weekly' strings for '<schedule>' and instead
> need to abandon the template model.

Makes sense.

> Modify the template with a custom schedule in the 'OnCalendar' setting.
> This schedule has some interesting differences from cron-like patterns,
> but is relatively easy to figure out from context. The one that might be
> confusing is that '*-*-*' is a date-based pattern, but this must be
> omitted when using 'Mon' to signal that we care about the day of the
> week. Monday is used since that matches the day used for the 'weekly'
> schedule used previously.

I think the launchd version (which uses "0" for the day of the week)
runs on Sunday, if I remember correctly. I don't think that these two
necessarily need to run on the same day of the week when configured to
run weekly.

But I figured I'd raise the question in case you did mean for them to
both run on either Sunday or Monday.

> The rest of the change involves making sure we are writing these .timer
> and .service files before initializing the schedule with 'systemctl' and
> deleting the files when we are done. Some changes are also made to share
> the random minute along with a single computation of the execution path
> of the current Git executable.
>
> Signed-off-by: Derrick Stolee <derrickstolee@github.com>
> ---
>  builtin/gc.c           | 82 ++++++++++++++++++++++++++++++++----------
>  t/t7900-maintenance.sh |  4 ++-
>  2 files changed, 66 insertions(+), 20 deletions(-)
>
> diff --git a/builtin/gc.c b/builtin/gc.c
> index b3ef95b10aa..5f5bb95641f 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -2299,13 +2299,20 @@ static char *xdg_config_home_systemd(const char *filename)
>  	return xdg_config_home_for("systemd/user", filename);
>  }
>
> -static int systemd_timer_write_unit_templates(const char *exec_path)
> +static int systemd_timer_write_unit_template(enum schedule_priority schedule,
> +					     const char *exec_path,
> +					     int minute)
>  {
>  	char *filename;
>  	FILE *file;
>  	const char *unit;
> +	char *schedule_pattern = NULL;

You should be able to drop the NULL initialization, since you assign
this value unconditionally in the switch statement below (or BUG() on an
unknown schedule type).

> +	const char *frequency = get_frequency(schedule);
> +	char *local_timer_name = xstrfmt("git-maintenance@%s.timer", frequency);
> +	char *local_service_name = xstrfmt("git-maintenance@%s.service", frequency);
> +
> +	filename = xdg_config_home_systemd(local_timer_name);
>
> -	filename = xdg_config_home_systemd("git-maintenance@.timer");
>  	if (safe_create_leading_directories(filename)) {
>  		error(_("failed to create directories for '%s'"), filename);
>  		goto error;
> @@ -2314,6 +2321,23 @@ static int systemd_timer_write_unit_templates(const char *exec_path)
>  	if (!file)
>  		goto error;
>
> +	switch (schedule) {
> +	case SCHEDULE_HOURLY:
> +		schedule_pattern = xstrfmt("*-*-* *:%02d:00", minute);
> +		break;
> +
> +	case SCHEDULE_DAILY:
> +		schedule_pattern = xstrfmt("*-*-* 0:%02d:00", minute);
> +		break;
> +
> +	case SCHEDULE_WEEKLY:
> +		schedule_pattern = xstrfmt("Mon 0:%02d:00", minute);
> +		break;
> +
> +	default:
> +		BUG("Unhandled schedule_priority");
> +	}
> +
>  	unit = "# This file was created and is maintained by Git.\n"
>  	       "# Any edits made in this file might be replaced in the future\n"
>  	       "# by a Git command.\n"
> @@ -2322,12 +2346,12 @@ static int systemd_timer_write_unit_templates(const char *exec_path)
>  	       "Description=Optimize Git repositories data\n"
>  	       "\n"
>  	       "[Timer]\n"
> -	       "OnCalendar=%i\n"
> +	       "OnCalendar=%s\n"
>  	       "Persistent=true\n"
>  	       "\n"
>  	       "[Install]\n"
>  	       "WantedBy=timers.target\n";
> -	if (fputs(unit, file) == EOF) {
> +	if (fprintf(file, unit, schedule_pattern) < 0) {

OK, this is the templating part that you were mentioning earlier. I was
wondering what we were doing fputs()-ing a string with "%i" in it
without a formatting value to fill it in with. But that "%i" pertains to
systemd's instance value, IIUC.

The rest all looks good, thanks.

Thanks,
Taylor