Re: [PATCH 1/3] scalar: enable built-in FSMonitor on `register`
- From
Victoria Dye <vdye@github.com>
- Date
- Aug 16, 2022, 21:57 UTC
- Message-ID
- <f766b31f-2f0c-316a-a445-407b8c5baddc@github.com>
- In-Reply-To
- <xmqq4jybud6h.fsf@gitster.g>
Junio C Hamano wrote:
Show 77 quoted lines
> "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
Show 7 quoted lines
> > 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!
Show 10 quoted lines
> > 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.