Junio C Hamano wrote:
> "Matthew John Cheetham via GitGitGadget" <gitgitgadget@xxxxxxxxx>
> 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.
