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