Re: [PATCH v4 4/4] maintenance: add support for systemd timers on Linux

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Mon, May 24 2021, Lénaïc Huard wrote:

If we're going to use ifdefs then... 

> +static int is_systemd_timer_available(void)
> +{
> +	const char *cmd = "systemctl";
> +	int is_available;
> +	static int cached_result = -1;
> +#ifdef __linux__
> +	struct child_process child = CHILD_PROCESS_INIT;
> +#endif
> +
> +	if (cached_result != -1)
> +		return cached_result;
> +
> +	if (get_schedule_cmd(&cmd, &is_available))
> +		return is_available;
> +
> +#ifdef __linux__
> +	strvec_split(&child.args, cmd);
> +	strvec_pushl(&child.args, "--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)) {
> +		cached_result = 0;
> +		return cached_result;
> +	}
> +	if (finish_command(&child)) {
> +		cached_result = 0;
> +		return cached_result;
> +	}
> +	cached_result = 1;
> +	return cached_result;
> +#else
> +	return 0;
> +#endif
> +}

This sort of function would, I think, be clearer if we just had two
different functions in an ifdef/else, e.g. here cached_result" is
checked under !__linux__, but is never set in that case.

But aside from that, why we do we need all this is_available caching,
isn't this all called as a one-off? If it's not, it seems much better to
push that cache one level up the callstack. Have whatevere's keeping
track of the struct cache is_available itself, or cache it in another
(or the same, then tristate) field in the same struct.

> [...]
> +test_expect_success 'start and stop Linux/systemd maintenance' '
> +	write_script print-args <<-\EOF &&
> +	printf "%s\n" "$*" >>args
> +	EOF
> +
> +	XDG_CONFIG_HOME="$PWD" &&
> +	export XDG_CONFIG_HOME &&
> +	rm -f args &&

If you're going to care about cleanup here, and personally I wouldn't,
just call that "args" by the name "expect" instead (as is convention)
and clobber it every time...

Anyway, a better way to do the cleanup is:

    test_when_finished "rm args" &&
    echo this is the first time you write the file >args
    [the rest of the test code]

Then you don't need to re-rm it.