From: Phillip Wood <phillip.wood123@gmail.com>
Date: Mon, 14 Aug 2023 11:26:46 GMT
Subject: Re: [PATCH v2 6/8] maintenance: use random minute in systemd scheduler
Message-ID: <5d431d2c-2d2c-4fb5-82ca-b9684523e512@gmail.com>
In-Reply-To: <e43778d3e408f5a77b01bce13df6f8b037473cc3.1691699987.git.gitgitgadget@gmail.com>

Hi Stolee

This all looks good to me now, I've left a comment on the test but I 
think it is probably fine to go in as it is.

> -static int systemd_timer_write_unit_templates(const char *exec_path)
> +/*
> + * Write the schedule information into a git-maintenance@<schedule>.timer
> + * file using a custom minute. This timer file cannot use the templating
> + * system, so we generate a specific file for each.
> + */

Thanks for adding comments to the functions that write the timer and 
service files, they really helpful especially for readers who are not 
that familiar with systemd.

> +static int systemd_timer_delete_unit_files(void)
> +{
> +	systemd_timer_delete_stale_timer_templates();
> +
> +	/* Purposefully not short-circuited to make sure all are called. */
> +	return systemd_timer_delete_timer_file(SCHEDULE_HOURLY) |
> +	       systemd_timer_delete_timer_file(SCHEDULE_DAILY) |
> +	       systemd_timer_delete_timer_file(SCHEDULE_WEEKLY) |
> +	       systemd_timer_delete_service_template();

Using "|" rather than "||" is a nice touch.

> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> index 487e326b3fa..9ffe76729e6 100755
> --- a/t/t7900-maintenance.sh
> +++ b/t/t7900-maintenance.sh
> @@ -744,7 +744,15 @@ test_expect_success 'start and stop Linux/systemd maintenance' '
>   	# start registers the repo
>   	git config --get --global --fixed-value maintenance.repo "$(pwd)" &&
>   
> -	test_systemd_analyze_verify "systemd/user/git-maintenance@.service" &&
> +	for schedule in hourly daily weekly
> +	do
> +		test_path_is_file "systemd/user/git-maintenance@$schedule.timer" || return 1
> +	done &&
> +	test_path_is_file "systemd/user/git-maintenance@.service" &&
> +
> +	test_systemd_analyze_verify "systemd/user/git-maintenance@hourly.service" &&
> +	test_systemd_analyze_verify "systemd/user/git-maintenance@daily.service" &&
> +	test_systemd_analyze_verify "systemd/user/git-maintenance@weekly.service" &&

As we only write the template service file I'm not sure what we gain by 
these three checks but they don't seem to be doing any harm.

Best Wishes

Phillip