Re: [PATCH v8 3/3] maintenance: add support for systemd timers on Linux

From: Derrick Stolee <stolee@gmail.com>
Date: Aug 24, 2021, 17:45 UTC
Message-ID: <66b2961f-19c8-d308-c9d2-eb8d05599a84@gmail.com>
In-Reply-To: <20210823204011.87023-4-lenaic@lhuard.fr>

On 8/23/2021 4:40 PM, Lénaïc Huard wrote:

Show 27 quoted lines

> +#ifdef __linux__
> +
> +static int real_is_systemd_timer_available(void)
> +{
> +	struct child_process child = CHILD_PROCESS_INIT;
> +
> +	strvec_pushl(&child.args, "systemctl", "--user", "list-timers", NULL);
> +	child.no_stdin = 1;
> +	child.no_stdout = 1;
> +	child.no_stderr = 1;
> +	child.silent_exec_failure = 1;
> +
> +	if (start_command(&child))
> +		return 0;
> +	if (finish_command(&child))
> +		return 0;
> +	return 1;
> +}
> +
> +#else
> +
> +static int real_is_systemd_timer_available(void)
> +{
> +	return 0;
> +}
> +
> +#endif

This #ifdef option is one that could be changed. There is a lot of code inside the #ifdef that would be nice to compile on all platforms.

Technically, we could drop all conditionals here and rely on the start_command() and finish_command() to tell us that systemd is or is not installed. This would allow a potential future where maybe macOS supports systemd (or users install a version themselves).

Another option would be to compile in a conditional early return inside real_is_systemd_timer_available() such as

static int real_is_systemd_timer_available(void)
{
	struct child_process child = CHILD_PROCESS_INIT;

#ifndef __linux__
	if (1)
		return 0;
#endif

	strvec_pushl(&child.args, "systemctl", "--user", "list-timers", NULL);
	child.no_stdin = 1;
	child.no_stdout = 1;
	child.no_stderr = 1;
	child.silent_exec_failure = 1;

	if (start_command(&child))
		return 0;
	if (finish_command(&child))
		return 0;
	return 1;
}

...but this also looks a bit awkward in order to avoid compilers complaining about unreachable code (and some might still rightly warn about unreachable code).

Show 8 quoted lines

> +static int systemd_timer_setup_units(void)
> +{
> +	const char *exec_path = git_exec_path();
> +
> +	int ret = systemd_timer_write_unit_templates(exec_path) ||
> +	          systemd_timer_enable_unit(1, SCHEDULE_HOURLY) ||
> +	          systemd_timer_enable_unit(1, SCHEDULE_DAILY) ||
> +	          systemd_timer_enable_unit(1, SCHEDULE_WEEKLY);

These lines are incorrectly tabbed with spaces. Here is a corrected version:

+ int ret = systemd_timer_write_unit_templates(exec_path) || + systemd_timer_enable_unit(1, SCHEDULE_HOURLY) || + systemd_timer_enable_unit(1, SCHEDULE_DAILY) || + systemd_timer_enable_unit(1, SCHEDULE_WEEKLY);

'git rebase --whitespace=fix <base>' will also fix these issues. You can discover if they exist using 'git log --check'.

Thanks, -Stolee