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