Re: [PATCH 09/23] fsmonitor--daemon: implement daemon command options

On 4/26/21 11:47 AM, Derrick Stolee wrote:
...

quoted

+
  static int is_ipc_daemon_listening(void)
  {
  	return fsmonitor_ipc__get_state() == IPC_STATE__LISTENING;
  }
  
+static int try_to_run_foreground_daemon(void)
+{
+	/*
+	 * Technically, we don't need to probe for an existing daemon
+	 * process, since we could just call `fsmonitor_run_daemon()`
+	 * and let it fail if the pipe/socket is busy.
+	 *
+	 * However, this method gives us a nicer error message for a
+	 * common error case.
+	 */
+	if (is_ipc_daemon_listening())
+		die("fsmonitor--daemon is already running.");

Here, it seems like we only care about IPC_STATE_LISTENING, while
earlier I mentioned that I ended up in IPC_STATE__NOT_LISTENING,
and my manually running of the daemon helped.

quoted

+	return !!fsmonitor_run_daemon();
+}

You are ignoring the IPC_STATE__NOT_LISTENING and creating a new
process, which is good. I'm just wondering why that state exists
and what is the proper way to handle it?

I'll revisit this and clarify.

quoted

+
+#ifndef GIT_WINDOWS_NATIVE

You are already creating a platform-specific mechanism for the
filesystem watcher. Shouldn't the implementation of this method
be part of that file in compat/fsmonitor/?

I guess the biggest reason is that macOS and Linux share this
implementation, so maybe this is the cleanest approach.

This has to do with how to spawn a background process and
disassociate from the console and all that.

On Windows, the "git fsmonitor--daemon --start" process[1] must
start a child process[2] with "git fsmonitor--daemon --run" and
then the [1] can exit (to let the caller/shell continue) while
[2] is free to continue.

On Unix, the "git fsmonitor-daemon --start" process[1] can
fork() a child process[2] and [2] can just call the _run_daemon()
code.  We don't need to exec the child, so this is a bit faster.

This code is platform-specific, so maybe it should go elsewhere,
but it has knowledge of fsmonitor--daemon-specific command line
args and private functions (`fsmonitor_run_daemon()`) and it knows
that it is not a library function.  So it made sense to keep it
close to the fsmonitor--daemon main entry point.


It didn't feel right to make these 2 versions of
`spawn_background_fsmonitor_daemon()` more generic (such as putting
them near `daemonize()`), because they know too much about
fsmonitor--daemon.

I did the same thing in `t/helper/test-simple-ipc.c` where I
created variants of this that started the test-tool in the background.
36a7eb6876 (t0052: add simple-ipc tests and t/helper/test-simple-ipc 
tool, 2021-03-22)


I thought about putting them inside `compat/fsmonitor/*.c`
but that has different problems.  Those files are concerned strictly
with the FS layer and how to get FS notification events from the
kernel and translating them into something cross-platform.  They
are, in a sense, a "driver" for that FS.  Process spawning is outside
of their scope.

And as you say, MacOS and Linux can both use the same process
spawning code, but will have vastly different FS layers.

So I left them here.


Thanks,
Jeff