On Mon, May 24 2021, Lénaïc Huard wrote:
> The first parameter of `XXX_update_schedule` and alike functions is a
> boolean specifying if the tasks should be scheduled or unscheduled.
>
> Using an `enum` with `ENABLE` and `DISABLE` values can make the code
> clearer.
I'm a fan of enums in general for N values, but I think for this sort of
boolean case it's stepping into the territory of just making things less
readable. There's nothing unreadable about 0/1 as an on/off.
> -static int launchctl_boot_plist(int enable, const char *filename, const char *cmd)
> +enum enable_or_disable {
> + DISABLE,
> + ENABLE
> +};
So here we have ENABLE/DISABLE...
> +static int launchctl_boot_plist(enum enable_or_disable enable,
> + const char *filename, const char *cmd)
> {
> int result;
> struct child_process child = CHILD_PROCESS_INIT;
> char *uid = launchctl_get_uid();
>
> strvec_split(&child.args, cmd);
> - if (enable)
> - strvec_push(&child.args, "bootstrap");
> - else
> - strvec_push(&child.args, "bootout");
> - strvec_push(&child.args, uid);
> - strvec_push(&child.args, filename);
> + strvec_pushl(&child.args, enable == ENABLE ? "bootstrap" : "bootout",
> + uid, filename, NULL);
..And here we just check ENABLE, and assume !ENABLE == DISABLE...
[...]
> {
> - if (run_maintenance)
> + switch (run_maintenance) {
> + case ENABLE:
> return launchctl_add_plists(cmd);
> - else
> + case DISABLE:
> return launchctl_remove_plists(cmd);
> + default:
> + BUG("invalid enable_or_disable value");
> + }
> }
And here we use a switch, but also a "default". It's actually better if
you're going to use an enum like this to leave out the "default", the
compiler will catch non-enumerated values for us.
>
> static char *schtasks_task_name(const char *frequency)
> @@ -1864,18 +1871,24 @@ static int schtasks_schedule_tasks(const char *cmd)
> schtasks_schedule_task(exec_path, SCHEDULE_WEEKLY, cmd);
> }
>
> -static int schtasks_update_schedule(int run_maintenance, int fd, const char *cmd)
> +static int schtasks_update_schedule(enum enable_or_disable run_maintenance,
> + int fd, const char *cmd)
> {
> - if (run_maintenance)
> + switch (run_maintenance) {
> + case ENABLE:
> return schtasks_schedule_tasks(cmd);
> - else
> + case DISABLE:
> return schtasks_remove_tasks(cmd);
> + default:
> + BUG("invalid enable_or_disable value");
> + }
> }
As an aside (I haven't read much/all the context) I wonder why we have
these wrapper functions, can't the caller just pass an "enable" flag?
> #define BEGIN_LINE "# BEGIN GIT MAINTENANCE SCHEDULE"
> #define END_LINE "# END GIT MAINTENANCE SCHEDULE"
>
> -static int crontab_update_schedule(int run_maintenance, int fd, const char *cmd)
> +static int crontab_update_schedule(enum enable_or_disable run_maintenance,
> + int fd, const char *cmd)
> {
> int result = 0;
> int in_old_region = 0;
> @@ -1925,7 +1938,7 @@ static int crontab_update_schedule(int run_maintenance, int fd, const char *cmd)
> fprintf(cron_in, "%s\n", line.buf);
> }
>
> - if (run_maintenance) {
> + if (run_maintenance == ENABLE) {
> struct strbuf line_format = STRBUF_INIT;
> const char *exec_path = git_exec_path();
Same !ENABLE == DISABLE assumption?
