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

[Junio C Hamano <gitster@pobox.com>]

Victoria Dye <vdye@github.com> writes:

>> 	/* 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.

OK, that is one valid way to go about it.  After I sent my review
comments, I however briefly wondered if we might *not* know if we
are already running one, there is a reliable exclusion mechansim
to prevent more than one monitor running at the same time, and we
are running pipe_command(), fully expecting that it may fail when
there is already a working one and a call to pipe_command() that
is not "checked" is just being lazy because we can afford to be
lazy here.

If that is not what is going on, then the cleaned up version I am
responding to does look more straight-forward and easy to
understand.  On the other hand, if "we can start more than we need
because we can rely on the exclusion mechanism" is what is going on,
that is fine as well, but it does need to be documented, preferrably
as in-code comment.

Thanks.