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

On 4/1/21 11:40 AM, Jeff Hostetler via GitGitGadget wrote:

From: Jeff Hostetler <redacted>

...

+	/* Prepare to (recursively) watch the <worktree-root> directory. */
+	strbuf_init(&state.path_worktree_watch, 0);
+	strbuf_addstr(&state.path_worktree_watch, absolute_path(get_git_work_tree()));
+	state.nr_paths_watching = 1;

Yes, let's watch the working directory.

+	/*
+	 * If ".git" is not a directory, then <gitdir> is not inside the
+	 * cone of <worktree-root>, so set up a second watch for it.
+	 */
+	strbuf_init(&state.path_gitdir_watch, 0);
+	strbuf_addbuf(&state.path_gitdir_watch, &state.path_worktree_watch);
+	strbuf_addstr(&state.path_gitdir_watch, "/.git");
+	if (!is_directory(state.path_gitdir_watch.buf)) {
+		strbuf_reset(&state.path_gitdir_watch);
+		strbuf_addstr(&state.path_gitdir_watch, absolute_path(get_git_dir()));
+		state.nr_paths_watching = 2;
+	}

But why watch the .git directory, especially for a worktree (or
submodule I guess)? What benefit do we get from events within the
.git directory? I'm expecting any event within the .git directory
should be silently ignored.

+
 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.

+	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?

+
+#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 is adapted from `wait_or_whine()`.  Watch the child process and
+ * let it get started and begin listening for requests on the socket
+ * before reporting our success.
+ */
+static int wait_for_background_startup(pid_t pid_child)
+{
+	int status;
+	pid_t pid_seen;
+	enum ipc_active_state s;
+	time_t time_limit, now;
+
+	time(&time_limit);
+	time_limit += fsmonitor__start_timeout_sec;
+
+	for (;;) {
+		pid_seen = waitpid(pid_child, &status, WNOHANG);
+
+		if (pid_seen == -1)
+			return error_errno(_("waitpid failed"));
+
+		else if (pid_seen == 0) {

There is some non-standard whitespace throughout this
if/else if/else:
...> +			continue;

+		}
+
+		else if (pid_seen == pid_child) {

...

+			return error(_("fsmonitor--daemon failed to start"));
+		}
+
+		else
+			return error(_("waitpid is confused"));

The rest of the glue in this patch looks good.

Thanks,
-Stolee