Re: [PATCH v4 2/4] maintenance: introduce ENABLE/DISABLE for code clarity
From: Phillip Wood <hidden>
Date: 2021-05-24 09:41:28
Hi Lénaïc On 24/05/2021 08:15, 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 sorry to say that I'm not sure this does make the code clearer overall - I wish I'd spoken up when Danh suggested it. While launchctl_boot_plist(DISABLE, filename, cmd) is arguably clearer than launchctl_boot_plist(0, filename, cmd) we end up with bizarre tests like if (enabled == ENABLED) rather than if (enabled) and in the next patch we have (enable == ENABLE && (opts->scheduler == i)) ? ENABLE : DISABLE; rather than enable && opts->scheduler == i Also looking at the next patch it seems as this one is missing some conversions in maintenance_start() as it is still calling update_background_schedule() with an integer rather than the new enum. I'd be happy to see this being dropped I'm afraid Best Wishes Phillip
quoted hunk ↗ jump to hunk
Signed-off-by: Lénaïc Huard <redacted> --- builtin/gc.c | 49 +++++++++++++++++++++++++++++++------------------ 1 file changed, 31 insertions(+), 18 deletions(-)diff --git a/builtin/gc.c b/builtin/gc.c index ef7226d7bc..0caf8d45c4 100644 --- a/builtin/gc.c +++ b/builtin/gc.c@@ -1570,19 +1570,21 @@ static char *launchctl_get_uid(void) return xstrfmt("gui/%d", getuid()); } -static int launchctl_boot_plist(int enable, const char *filename, const char *cmd) +enum enable_or_disable { + DISABLE, + ENABLE +}; + +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); child.no_stderr = 1; child.no_stdout = 1;@@ -1601,7 +1603,7 @@ static int launchctl_remove_plist(enum schedule_priority schedule, const char *c const char *frequency = get_frequency(schedule); char *name = launchctl_service_name(frequency); char *filename = launchctl_service_filename(name); - int result = launchctl_boot_plist(0, filename, cmd); + int result = launchctl_boot_plist(DISABLE, filename, cmd); unlink(filename); free(filename); free(name);@@ -1684,8 +1686,8 @@ static int launchctl_schedule_plist(const char *exec_path, enum schedule_priorit fclose(plist); /* bootout might fail if not already running, so ignore */ - launchctl_boot_plist(0, filename, cmd); - if (launchctl_boot_plist(1, filename, cmd)) + launchctl_boot_plist(DISABLE, filename, cmd); + if (launchctl_boot_plist(ENABLE, filename, cmd)) die(_("failed to bootstrap service %s"), filename); free(filename);@@ -1702,12 +1704,17 @@ static int launchctl_add_plists(const char *cmd) launchctl_schedule_plist(exec_path, SCHEDULE_WEEKLY, cmd); } -static int launchctl_update_schedule(int run_maintenance, int fd, const char *cmd) +static int launchctl_update_schedule(enum enable_or_disable run_maintenance, + int fd, const char *cmd) { - 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"); + } } 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"); + } } #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();