Hi Eric
On 10/05/2021 19:25, Eric Sunshine wrote:
On Mon, May 10, 2021 at 2:04 PM Phillip Wood <phillip.wood123@xxxxxxxxx> wrote:
On 09/05/2021 22:32, Lénaïc Huard wrote:
+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
