Re: [PATCH 1/3] scalar: enable built-in FSMonitor on `register`

[Victoria Dye <vdye@github.com>]

Junio C Hamano wrote:
> "Matthew John Cheetham via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
> 
>> +static int start_fsmonitor_daemon(void)
>> +{
>> +	int res = 0;
>> +	if (fsmonitor_ipc__is_supported() &&
>> +	    fsmonitor_ipc__get_state() != IPC_STATE__LISTENING) {
>> +		struct strbuf err = STRBUF_INIT;
>> +		struct child_process cp = CHILD_PROCESS_INIT;
>> +
>> +		/* Try to start the FSMonitor daemon */
>> +		cp.git_cmd = 1;
>> +		strvec_pushl(&cp.args, "fsmonitor--daemon", "start", NULL);
>> +		if (!pipe_command(&cp, NULL, 0, NULL, 0, &err, 0)) {
>> +			/* Successfully started FSMonitor */
>> +			strbuf_release(&err);
>> +			return 0;
>> +		}
>> +
>> +		/* If FSMonitor really hasn't started, emit error */
>> +		if (fsmonitor_ipc__get_state() != IPC_STATE__LISTENING)
>> +			res = error(_("could not start the FSMonitor daemon: %s"),
>> +				    err.buf);
>> +
>> +		strbuf_release(&err);
>> +	}
>> +
>> +	return res;
>> +}
> 
> This somewhat curious code structure made me look, and made me
> notice that the behaviour is even more curious.  Even though
> pipe_command() fails, fsmonitor_ipc__get_state() can somehow become
> LISTENING, in which case we are OK?  If that is the case, a more natural
> way to write it would be:
> 
> 	int res = 0; /* assume success */
> 
> 	if (fsmonitor_ipc__is_supported() &&
>             fsmonitor_ipc__get_state() != IPC_STATE__LISTENING) {
> 		...
>                 /* 
>                  * if we fail to start it ourselves, and there is no
>                  * daemon listening to us, it is an error.
>                  */
> 		if (pipe_command(...) &&
> 		    fsmonitor_ipc__get_state() != IPC_STATE__LISTENING)
> 			res = error(...);
> 		strbuf_release(&err);
> 	}
> 	return res;
> 
> and that would utilize "res" consistently throughout the function.
> 
> Note that (I omitted unnecessary blank lines and added necessary
> ones in the above outline of the rewrite.
> 
> Stopping, stepping back a bit and rethinking, the above is not still
> exactly right.  If pipe_command() could lie and say "we failed to
> start" when we immediately after the failure can find a running
> daemon, what guarantees us that pipe_command() does not lie in the
> other direction?  So, in that sense, perhaps doing
> 
> 	/* we do not care if pipe_command() succeeds or not */
> 	(void) pipe_command(...);
> 
>         /*
>          * we check ourselves if we do have a usable daemon 
>          * and that is the authoritative answer.  we were asked
>          * to ensure that usable daemon exists, and we answer
>          * if we do or don't.
>          */
> 	if (fsmonitor_ipc__get_state() != IPC_STATE__LISTENING)
> 		res = error(...);
> 
> may be more true to the spirit of the code.

This is an unintentional artifact of some minor refactoring of the original
versions in 'microsoft/git'. Previously [1], there was no
'fsmonitor_ipc__get_state()' check before calling 'git fsmonitor--daemon
start', so we'd expect failures whenever FSMonitor was already running. To
avoid making that 'pipe_command()' call when FSMonitor was already running,
I added an earlier call to 'fsmonitor_ipc__get_state()'. But, because I
didn't remove the later check, the code now implies that 'pipe_command()'
may give us "false negatives" (that is, fail but still manage to start the
FSMonitor).

I left the extraneous check in to be overly cautious, but realistically I
don't actually expect 'git fsmonitor--daemon start' to give us any false
positives or negatives. The code should reflect that:

	int res = 0;
	if (fsmonitor_ipc__is_supported() &&
	    fsmonitor_ipc__get_state() != IPC_STATE__LISTENING) {
		struct strbuf err = STRBUF_INIT;
		struct child_process cp = CHILD_PROCESS_INIT;

		/* Try to start the FSMonitor daemon */
		cp.git_cmd = 1;
		strvec_pushl(&cp.args, "fsmonitor--daemon", "start", NULL);
		if (pipe_command(&cp, NULL, 0, NULL, 0, &err, 0))
			res = error(_("could not start the FSMonitor daemon: %s"),
				    err.buf);

		strbuf_release(&err);
	}

	return res;

I'll re-roll with this shortly.

[1] https://github.com/microsoft/git/commit/4f2e092d3c98

> 
> It also is slightly curious if the caller wants to see "success"
> when fsmonitor is not supported.  I would have expected the caller
> to check and refrain from calling start/stop when it is not
> supported (and if there is an end-user interface to force the scalar
> command to "start", complain by saying "not supported here").  But
> as long as we are consistent, I guess it is OK.

I don't mind moving the 'fsmonitor_ipc__is_supported()' checks into
'register_dir()' and 'unregister_dir()'; I can see how it makes more sense
with the existing function name. 

As a side note, though, while looking at where to move the condition I
noticed that 'unregister_dir()' doesn't handle positive, nonzero return
values properly. I'll fix this & move the 'fsmonitor_ipc__is_supported()'
check in the next version. Thanks!

> 
> The side that stops shares exactly the same two pieces of
> "curiosity" and may need to be updated exactly the same way.  It
> assumes that pipe_command() is unreliable and instead of reporting a
> possible failure, we sweep that under the rug, with a questionable
> "we do not care about pipe failing, as long as the daemon is
> listening, we do not care" attitude.  And the caller does not care
> "start" not stopping where it is not supported.
> 
> Thanks.