From: Derrick Stolee <stolee@gmail.com>
Date: Mon, 26 Apr 2021 15:08:36 GMT
Subject: Re: [PATCH 05/23] fsmonitor--daemon: add a built-in fsmonitor daemon
Message-ID: <522e10e6-f18d-9c28-7102-f5a855d4be50@gmail.com>
In-Reply-To: <95d511d83b1211f24aeb17edbd4918750f406ece.1617291666.git.gitgitgadget@gmail.com>

On 4/1/21 11:40 AM, Jeff Hostetler via GitGitGadget wrote:> +#ifdef HAVE_FSMONITOR_DAEMON_BACKEND

I think these compile-time macros should be replaced with a
method call, as I've said before. It should be simple to say

	if (!fsmonitor_ipc__is_supported())
		die(_("fsmonitor--daemon is not supported on this platform"));

and call it a day. This can be done before parsing arguments.

> +int cmd_fsmonitor__daemon(int argc, const char **argv, const char *prefix)
> +{
> +	enum daemon_mode {
> +		UNDEFINED_MODE,
> +	} mode = UNDEFINED_MODE;
> +
> +	struct option options[] = {
> +		OPT_END()
> +	};

I can see where you are going here, to use the parse-opts API
to get your "--<verb>" arguments to populate an 'enum'. However,
it seems like you will run into the problem where a user enters
multiple such arguments and you lose the information as the
parser overwrites 'mode' here.

Better to use a positional argument and drop the "--" prefix,
in my opinion.

Thanks,
-Stolee