Re: [PATCH v2 1/1] maintenance: use systemd timers on Linux

Hi Eric

On 10/05/2021 19:25, Eric Sunshine wrote:

On Mon, May 10, 2021 at 2:04 PM Phillip Wood [off-list ref] wrote:

quoted

On 09/05/2021 22:32, Lénaïc Huard wrote:

quoted

+static int systemd_timer_enable_unit(int enable,
+                                  enum schedule_priority schedule,
+                                  const char *cmd)

The cmd argument is pointless, it will always be "systemctl" and you
have even hard coded that value into the error message below.

The reason that `cmd` is passed around everywhere is that the actual
command can be overridden by GIT_TEST_MAINT_SCHEDULER which allows the
test script to mock up a scheduler command rather than running the
real scheduler command. I haven't read the new version of the patch
closely yet, but after a quick scan, I'm pretty confident that this is
still the case (despite the aggressive changes the patch makes to the
areas around GIT_TEST_MAINT_SCHEDULER).

Thanks for pointing that out I'd somehow glossed over the GIT_TEST_MAINT_SCHEDULER code, I agree it looks like the patch takes care to keep it working.

It is outside the scope of this patch but a possibly nicer pattern would be to have a function get_command_name(const char *default) that checks GIT_TEST_MAINT_SCHEDULER and returns the command name from that or the default if it is not set. We would then call that function to get the command name when we want to run a command. That way all the extra complexity is localized around the command call (and consists of a single function call), the usual command name is visible in the function calling the command and we'd avoid littering all the function signatures with a argument that is only relevant for testing.

As for hardcoding the command name in the error message, that seems
perfectly fine since, under normal circumstances, it _will_ be that
command (it's only different when testing).

I agree, thanks

Phillip