Re: [PATCH v4 2/4] maintenance: introduce ENABLE/DISABLE for code clarity
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- May 24, 2021, 09:47 UTC
- Message-ID
- <87tumso3xn.fsf@evledraar.gmail.com>
- In-Reply-To
- <20210524071538.46862-3-lenaic@lhuard.fr>
On Mon, May 24 2021, 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 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.
Show 5 quoted lines
> -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...
Show 16 quoted lines
> +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... [...]
Show 12 quoted lines
> {
> - 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.
Show 21 quoted lines
>
> 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?
Show 17 quoted lines
> #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?