Re: [PATCH 13/23] fsmonitor-fs-listen-win32: implement FSMonitor backend on Windows

> On 4/1/2021 11:40 AM, Jeff Hostetler via GitGitGadget wrote:
>> From: Jeff Hostetler <jeffhost@microsoft.com>
>>
>> Teach the win32 backend to register a watch on the working tree
>> root directory (recursively).  Also watch the <gitdir> if it is
>> not inside the working tree.  And to collect path change notifications
>> into batches and publish.
> 
> Is it valuable to list the important API methods here for an interested
> reader to discover them? Perhaps using links to the docs [1] might be
> too ephemeral, in case those URLs stop being valid.
> 
> In any case, here are the URLs I found helpful:
> 
> [1] https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-readdirectorychangesw
> [2] https://docs.microsoft.com/en-us/windows/win32/api/ioapiset/nf-ioapiset-getoverlappedresult
> [3] https://docs.microsoft.com/en-us/windows/win32/fileio/cancelioex-func
> [4] https://docs.microsoft.com/en-us/windows/win32/api/synchapi/nf-synchapi-resetevent
> [5] https://docs.microsoft.com/en-us/windows/win32/api/winnt/ns-winnt-file_notify_information
> [6] https://docs.microsoft.com/en-us/windows/win32/api/synchapi/nf-synchapi-waitformultipleobjects

>> +static struct one_watch *create_watch(struct fsmonitor_daemon_state *state,
>> +				      const char *path)
>> +{
>> +	struct one_watch *watch = NULL;
>> +	DWORD desired_access = FILE_LIST_DIRECTORY;
>> +	DWORD share_mode =
>> +		FILE_SHARE_WRITE | FILE_SHARE_READ | FILE_SHARE_DELETE;
> 
> Ah, this is probably why we can delete a repo that is under a watch.

>> +static int recv_rdcw_watch(struct one_watch *watch)
>> +{
>> +	watch->is_active = FALSE;
>> +
>> +	if (GetOverlappedResult(watch->hDir, &watch->overlapped, &watch->count,
>> +				TRUE))
>> +		return 0;
>> +
>> +	// TODO If an external <gitdir> is deleted, the above returns an error.
>> +	// TODO I'm not sure that there's anything that we can do here other
>> +	// TODO than failing -- the <worktree>/.git link file would be broken
>> +	// TODO anyway.  We might try to check for that and return a better
>> +	// TODO error message.
> 
> These are not fit C-style comments. This situation can be handled
> by a later patch series, if valuable enough.

> 
>> +
>> +	error("GetOverlappedResult failed on '%s' [GLE %ld]",
>> +	      watch->path.buf, GetLastError());
>> +	return -1;
>> +}
>> +
>> +static void cancel_rdcw_watch(struct one_watch *watch)
>> +{
>> +	DWORD count;
>> +
>> +	if (!watch || !watch->is_active)
>> +		return;
>> +
>> +	CancelIoEx(watch->hDir, &watch->overlapped);
>> +	GetOverlappedResult(watch->hDir, &watch->overlapped, &count, TRUE);
>> +	watch->is_active = FALSE;
>> +}
>> +
>> +/*
>> + * Process filesystem events that happen anywhere (recursively) under the
>> + * <worktree> root directory.  For a normal working directory, this includes
>> + * both version controlled files and the contents of the .git/ directory.
>> + *
>> + * If <worktree>/.git is a file, then we only see events for the file
>> + * itself.
>> + */
>> +static int process_worktree_events(struct fsmonitor_daemon_state *state)
>> +{
>> +	struct fsmonitor_daemon_backend_data *data = state->backend_data;
>> +	struct one_watch *watch = data->watch_worktree;
>> +	struct strbuf path = STRBUF_INIT;
>> +	struct string_list cookie_list = STRING_LIST_INIT_DUP;
>> +	struct fsmonitor_batch *batch = NULL;
>> +	const char *p = watch->buffer;
>> +
>> +	/*
>> +	 * If the kernel gets more events than will fit in the kernel
>> +	 * buffer associated with our RDCW handle, it drops them and
>> +	 * returns a count of zero.  (A successful call, but with
>> +	 * length zero.)
>> +	 */
> 
> I suppose that since we create a cookie file, we don't expect a zero
> result to ever be a meaningful value? Or, is there another way to
> differentiate between "nothing happened" and "too much happened"?

> 
>> +	if (!watch->count) {
>> +		trace2_data_string("fsmonitor", NULL, "fsm-listen/kernel",
>> +				   "overflow");
>> +		fsmonitor_force_resync(state);
>> +		return LISTENER_HAVE_DATA_WORKTREE;
>> +	}
>> +
>> +	/*
>> +	 * On Windows, `info` contains an "array" of paths that are
>> +	 * relative to the root of whichever directory handle received
>> +	 * the event.
>> +	 */
>> +	for (;;) {
>> +		FILE_NOTIFY_INFORMATION *info = (void *)p;
>> +		const char *slash;
>> +		enum fsmonitor_path_type t;
>> +
>> +		strbuf_reset(&path);
>> +		if (normalize_path_in_utf8(info, &path) == -1)
>> +			goto skip_this_path;
>> +
>> +		t = fsmonitor_classify_path_workdir_relative(path.buf);
>> +
>> +		switch (t) {
>> +		case IS_INSIDE_DOT_GIT_WITH_COOKIE_PREFIX:
>> +			/* special case cookie files within .git */
>> +
>> +			/* Use just the filename of the cookie file. */
>> +			slash = find_last_dir_sep(path.buf);
>> +			string_list_append(&cookie_list,
>> +					   slash ? slash + 1 : path.buf);
> 
> Ok, I see now how we special-case cookies in the list of events.
> 
>> +			break;
>> +
>> +		case IS_INSIDE_DOT_GIT:
>> +			/* ignore everything inside of "<worktree>/.git/" */
>> +			break;
>> +
>> +		case IS_DOT_GIT:
>> +			/* "<worktree>/.git" was deleted (or renamed away) */
>> +			if ((info->Action == FILE_ACTION_REMOVED) ||
>> +			    (info->Action == FILE_ACTION_RENAMED_OLD_NAME)) {
>> +				trace2_data_string("fsmonitor", NULL,
>> +						   "fsm-listen/dotgit",
>> +						   "removed");
>> +				goto force_shutdown;
>> +			}
>> +			break;
>> +
>> +		case IS_WORKDIR_PATH:
>> +			/* queue normal pathname */
>> +			if (!batch)
>> +				batch = fsmonitor_batch__new();
>> +			fsmonitor_batch__add_path(batch, path.buf);
>> +			break;
>> +
>> +		case IS_GITDIR:
>> +		case IS_INSIDE_GITDIR:
>> +		case IS_INSIDE_GITDIR_WITH_COOKIE_PREFIX:
>> +		default:
>> +			BUG("unexpected path classification '%d' for '%s'",
>> +			    t, path.buf);
> So these events should be caught by the _other_ watcher. I suppose
> BUG() is somewhat appropriate, but also seems heavy-handed. For
> example, the 'goto' in the next line will never be visited. A die()
> would even be appropriate, but somewhat less harsh than a BUG(),
> especially for a background process.
> 
>> +			goto skip_this_path;

>> +		}
>> +
>> +skip_this_path:
>> +		if (!info->NextEntryOffset)
>> +			break;
>> +		p += info->NextEntryOffset;
>> +	}
>> +
>> +	fsmonitor_publish(state, batch, &cookie_list);
>> +	batch = NULL;
>> +	string_list_clear(&cookie_list, 0);
>> +	strbuf_release(&path);
>> +	return LISTENER_HAVE_DATA_WORKTREE;
>> +
>> +force_shutdown:
>> +	fsmonitor_batch__free(batch);
>> +	string_list_clear(&cookie_list, 0);
>> +	strbuf_release(&path);
>> +	return LISTENER_SHUTDOWN;
>> +}
>> +
>> +/*
>> + * Process filesystem events that happend anywhere (recursively) under the
> 
> s/happend/happened
> 
>> + * external <gitdir> (such as non-primary worktrees or submodules).
>> + * We only care about cookie files that our client threads created here.
>> + *
>> + * Note that we DO NOT get filesystem events on the external <gitdir>
>> + * itself (it is not inside something that we are watching).  In particular,
>> + * we do not get an event if the external <gitdir> is deleted.
> 
> This is an interesting change of behavior. I forget if it is listed in
> the documentation file, but definitely could be. I imagine wanting a
> "Troubleshooting" section that describes special cases like this.
> 
> Also, because of this worktree-specific behavior, we might want to
> recommend using 'git config --worktree' when choosing to use FS Monitor,
> so that each worktree is opted-in as requested. Without --worktree, all
> worktrees with a common base would stard using FS Monitor simultaneously.

> 
>> + */
>> +static int process_gitdir_events(struct fsmonitor_daemon_state *state)
>> +{
>> +	struct fsmonitor_daemon_backend_data *data = state->backend_data;
>> +	struct one_watch *watch = data->watch_gitdir;
>> +	struct strbuf path = STRBUF_INIT;
>> +	struct string_list cookie_list = STRING_LIST_INIT_DUP;
>> +	const char *p = watch->buffer;
>> +
>> +	if (!watch->count) {
>> +		trace2_data_string("fsmonitor", NULL, "fsm-listen/kernel",
>> +				   "overflow");
>> +		fsmonitor_force_resync(state);
>> +		return LISTENER_HAVE_DATA_GITDIR;
>> +	}
>> +
>> +	for (;;) {
>> +		FILE_NOTIFY_INFORMATION *info = (void *)p;
>> +		const char *slash;
>> +		enum fsmonitor_path_type t;
>> +
>> +		strbuf_reset(&path);
>> +		if (normalize_path_in_utf8(info, &path) == -1)
>> +			goto skip_this_path;
>> +
>> +		t = fsmonitor_classify_path_gitdir_relative(path.buf);
>> +
>> +		trace_printf_key(&trace_fsmonitor, "BBB: %s", path.buf);
>> +
>> +		switch (t) {
>> +		case IS_INSIDE_GITDIR_WITH_COOKIE_PREFIX:
>> +			/* special case cookie files within gitdir */
>> +
>> +			/* Use just the filename of the cookie file. */
>> +			slash = find_last_dir_sep(path.buf);
>> +			string_list_append(&cookie_list,
>> +					   slash ? slash + 1 : path.buf);
>> +			break;
>> +
>> +		case IS_INSIDE_GITDIR:
>> +			goto skip_this_path;
>> +
>> +		default:
>> +			BUG("unexpected path classification '%d' for '%s'",
>> +			    t, path.buf);
> 
> If we decide against BUG() earlier, then also get this one.
> 
>> +			goto skip_this_path;
>> +		}
>> +
>> +skip_this_path:
>> +		if (!info->NextEntryOffset)
>> +			break;
>> +		p += info->NextEntryOffset;
>> +	}
>> +
>> +	fsmonitor_publish(state, NULL, &cookie_list);
>> +	string_list_clear(&cookie_list, 0);
>> +	strbuf_release(&path);
>> +	return LISTENER_HAVE_DATA_GITDIR;
>>   }
>>   
>>   void fsmonitor_fs_listen__loop(struct fsmonitor_daemon_state *state)
>>   {
>> +	struct fsmonitor_daemon_backend_data *data = state->backend_data;
>> +	DWORD dwWait;
>> +
>> +	state->error_code = 0;
>> +
>> +	if (start_rdcw_watch(data, data->watch_worktree) == -1)
>> +		goto force_error_stop;
>> +
>> +	if (data->watch_gitdir &&
>> +	    start_rdcw_watch(data, data->watch_gitdir) == -1)
>> +		goto force_error_stop;
>> +
>> +	for (;;) {
>> +		dwWait = WaitForMultipleObjects(data->nr_listener_handles,
>> +						data->hListener,
>> +						FALSE, INFINITE);
> 
> Since you use INFINITE here, that says that we will wait for at least one
> signal, solving the confusion about zero results: zero results unambiguously
> indicates a loss of events.

> 
>> +
>> +		if (dwWait == WAIT_OBJECT_0 + LISTENER_HAVE_DATA_WORKTREE) {
>> +			if (recv_rdcw_watch(data->watch_worktree) == -1)
>> +				goto force_error_stop;
>> +			if (process_worktree_events(state) == LISTENER_SHUTDOWN)
>> +				goto force_shutdown;
>> +			if (start_rdcw_watch(data, data->watch_worktree) == -1)
>> +				goto force_error_stop;
>> +			continue;
>> +		}
>> +
>> +		if (dwWait == WAIT_OBJECT_0 + LISTENER_HAVE_DATA_GITDIR) {
>> +			if (recv_rdcw_watch(data->watch_gitdir) == -1)
>> +				goto force_error_stop;
>> +			if (process_gitdir_events(state) == LISTENER_SHUTDOWN)
>> +				goto force_shutdown;
>> +			if (start_rdcw_watch(data, data->watch_gitdir) == -1)
>> +				goto force_error_stop;
>> +			continue;
>> +		}
>> +
>> +		if (dwWait == WAIT_OBJECT_0 + LISTENER_SHUTDOWN)
>> +			goto clean_shutdown;
>> +
>> +		error(_("could not read directory changes [GLE %ld]"),
>> +		      GetLastError());
>> +		goto force_error_stop;
>> +	}
>> +
>> +force_error_stop:
>> +	state->error_code = -1;
>> +
>> +force_shutdown:
>> +	/*
>> +	 * Tell the IPC thead pool to stop (which completes the await
>> +	 * in the main thread (which will also signal this thread (if
>> +	 * we are still alive))).
>> +	 */
>> +	ipc_server_stop_async(state->ipc_server_data);
>> +
>> +clean_shutdown:
>> +	cancel_rdcw_watch(data->watch_worktree);
>> +	cancel_rdcw_watch(data->watch_gitdir);
>>   }
>>   
>>   int fsmonitor_fs_listen__ctor(struct fsmonitor_daemon_state *state)
>>   {
>> +	struct fsmonitor_daemon_backend_data *data;
>> +
>> +	data = xcalloc(1, sizeof(*data));
> 
> CALLOC_ARRAY()
> 
>> +
>> +	data->hEventShutdown = CreateEvent(NULL, TRUE, FALSE, NULL);
>> +
>> +	data->watch_worktree = create_watch(state,
>> +					    state->path_worktree_watch.buf);
>> +	if (!data->watch_worktree)
>> +		goto failed;
>> +
>> +	if (state->nr_paths_watching > 1) {
>> +		data->watch_gitdir = create_watch(state,
>> +						  state->path_gitdir_watch.buf);
>> +		if (!data->watch_gitdir)
>> +			goto failed;
>> +	}
>> +
>> +	data->hListener[LISTENER_SHUTDOWN] = data->hEventShutdown;
>> +	data->nr_listener_handles++;
>> +
>> +	data->hListener[LISTENER_HAVE_DATA_WORKTREE] =
>> +		data->watch_worktree->hEvent;
>> +	data->nr_listener_handles++;
>> +
>> +	if (data->watch_gitdir) {
>> +		data->hListener[LISTENER_HAVE_DATA_GITDIR] =
>> +			data->watch_gitdir->hEvent;
>> +		data->nr_listener_handles++;
>> +	}
> 
> This is a clever organization of the event handles. I imagine it
> will requires some rework if we decide to include another optional
> handle whose inclusion is orthogonal to the gitdir one, but that
> is unlikely enough to keep these well-defined array indices.

> 
>> +	state->backend_data = data;
>> +	return 0;
>> +
>> +failed:
>> +	CloseHandle(data->hEventShutdown);
>> +	destroy_watch(data->watch_worktree);
>> +	destroy_watch(data->watch_gitdir);
>> +
>>   	return -1;
>>   }
>>   
>>   void fsmonitor_fs_listen__dtor(struct fsmonitor_daemon_state *state)
>>   {
>> +	struct fsmonitor_daemon_backend_data *data;
>> +
>> +	if (!state || !state->backend_data)
>> +		return;
>> +
>> +	data = state->backend_data;
>> +
>> +	CloseHandle(data->hEventShutdown);
>> +	destroy_watch(data->watch_worktree);
>> +	destroy_watch(data->watch_gitdir);
>> +
>> +	FREE_AND_NULL(state->backend_data);
>>   }
> 
> I tried to follow all the API calls and check the documentation for
> any misuse, but did not find any. I can only contribute nitpicks
> here, and rely on the tests to really see that this is working as
> expected.
> 
> I was hoping to find in here why we need to sleep in the test suite,
> but have not pinpointed that issue yet.