From: Jeff Hostetler <git@jeffhostetler.com>
Date: Fri, 30 Apr 2021 14:31:34 GMT
Subject: Re: [PATCH 05/23] fsmonitor--daemon: add a built-in fsmonitor daemon
Message-ID: <9e4f67f0-9a94-b93c-2f21-76555199a38e@jeffhostetler.com>
In-Reply-To: <1c45542a-5d80-6e51-26fe-94066987a566@gmail.com>



On 4/26/21 11:45 AM, Derrick Stolee wrote:
> On 4/26/21 11:08 AM, Derrick Stolee wrote:
>> 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.
> 
> I see that you use OPT_CMDMODE in your implementation, which
> makes this concern invalid.
> 
>> Better to use a positional argument and drop the "--" prefix,
>> in my opinion.
> 
> This is my personal taste, but the technical reason to do this
> doesn't exist.

Either method is fine/equivalent and I'm open to doing it either
way.  (In fact, I did the t/helper/test-simple-ipc the other way
and didn't even think about it.)

Does the mailing list have a preference for one form over the other?
That is:

     git fsmonitor--daemon --start [<options>]
vs
     git fsmonitor--daemon start [<options>]

Jeff