| Message ID | 20210524071538.46862-3-lenaic@lhuard.fr (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | add support for systemd timers on Linux | expand |
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 > 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(); > >
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?
On 2021-05-24 10:41:18+0100, Phillip Wood <phillip.wood123@gmail.com> wrote: > 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. Yes, in this form, I also think the change looks bizarre. And, it's entirely my fault. I also agree with Ævar that 0 and 1 is meant well for off/on. However, I still think launchctl_boot_plist(0, filename, cmd) would require some degree on code navigation to figure out what would that LoC does. I'm thinking about rename the function. But, it would trigger a forever bikeshedding, which shouldn't be a blocker for this series. > I'd be happy to see this being dropped I'm afraid So, let's drop this patch and start a new conversation when the dust settled.
Le lundi 24 mai 2021, 14:36:14 CEST Đoàn Trần Công Danh a écrit : > On 2021-05-24 10:41:18+0100, Phillip Wood <phillip.wood123@gmail.com> wrote: > > 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. > > Yes, in this form, I also think the change looks bizarre. > And, it's entirely my fault. > > I also agree with Ævar that 0 and 1 is meant well for off/on. > > However, I still think > > launchctl_boot_plist(0, filename, cmd) > > would require some degree on code navigation to figure out what would > that LoC does. > > I'm thinking about rename the function. But, it would trigger a forever > bikeshedding, which shouldn't be a blocker for this series. > > > I'd be happy to see this being dropped I'm afraid > > So, let's drop this patch and start a new conversation when the dust > settled. Hi, I think the reason why the code looks worse is because I used an enum and I didn’t want to make any assumption about how the enum members would be evaluated in a boolean context. Do you think it would make sense to drop the enum type, to revert all logic changes (Use `if (enabled)` back instead of `switch`, etc.), and to define the following constants : static const int DISABLE = 0; static const int ENABLE = 1; so that we can keep function invocation in the form of `launchctl_boot_plist(DISABLE, filename, cmd)` ?
Lénaïc Huard <lenaic@lhuard.fr> writes: > I think the reason why the code looks worse is because I used an enum and I > didn’t want to make any assumption about how the enum members would be > evaluated in a boolean context. > > Do you think it would make sense to drop the enum type, to revert all logic > changes (Use `if (enabled)` back instead of `switch`, etc.), and to define the > following constants : > > static const int DISABLE = 0; > static const int ENABLE = 1; > > so that we can keep function invocation in the form of > `launchctl_boot_plist(DISABLE, filename, cmd)` ? I think the code is much better off without DISABLE/ENABLE at all. As has already been pointed out, you cannot read and write _without_ being aware of the fact that DISABLE is 0 if you want to write readable code, i.e. instead of "if (able == ENABLE) do this;", you would want to say "if (able) do this;".
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();
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. Signed-off-by: Lénaïc Huard <lenaic@lhuard.fr> --- builtin/gc.c | 49 +++++++++++++++++++++++++++++++------------------ 1 file changed, 31 insertions(+), 18 deletions(-)