Re: [PATCH v4 2/4] maintenance: introduce ENABLE/DISABLE for code clarity

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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?