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

[Derrick Stolee <stolee@gmail.com>]

On 4/1/21 11:40 AM, Jeff Hostetler via GitGitGadget wrote:
> From: Jeff Hostetler <jeffhost@microsoft.com>
...
> +	/* 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