Re: [PATCH v4 2/4] maintenance: introduce ENABLE/DISABLE for code clarity
flat view
From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2021-05-24 09:51:24
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.
quoted hunk ↗ jump to hunk
-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...
quoted hunk ↗ jump to hunk
+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... [...]
quoted hunk ↗ jump to hunk
{ - 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.
quoted hunk ↗ jump to hunk
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?
quoted hunk ↗ jump to hunk
#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?