Re: [PATCH v4 2/4] maintenance: introduce ENABLE/DISABLE for code clarity
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- May 24, 2021, 09:41 UTC
- Message-ID
- <8e013441-08dc-fbb5-f9b9-649b2ffd78db@gmail.com>
- In-Reply-To
- <20210524071538.46862-3-lenaic@lhuard.fr>
Hi Lénaïc
On 24/05/2021 08:15, Lénaïc Huard wrote:
Show 5 quoted lines
> 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
Show 118 quoted lines
> Signed-off-by: Lénaïc Huard <lenaic@lhuard.fr>
> ---
> 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();
>
>