On 2/24/22 10:26 AM, Derrick Stolee wrote:
On 2/15/2022 10:59 AM, Jeff Hostetler via GitGitGadget wrote:
From: Jeff Hostetler <jeffhost@xxxxxxxxxxxxx>
Teach Git to detect remote working directories on macOS and mark them as
incompatible with FSMonitor.
With this, `git fsmonitor--daemon run` will error out with a message
like it does for bare repos.
Client commands, like `git status`, will not attempt to start the daemon.
...
+ * A client machine (such as a laptop) may choose to suspend/resume
+ * and it is unclear (without lots of testing) whether the watcher can
+ * resync after a resume. We might be able to treat this as a normal
+ * "events were dropped by the kernel" event and do our normal "flush
+ * and resync" --or-- we might need to close the existing (zombie?)
+ * notification fd and create a new one.
+ *
+ * In theory, the above issues need to be addressed whether we are
+ * using the Hook or IPC API.
The only thing I can think about is a case where the filesystem
monitor is actually running on the remote machine and Git
communicates with it over the network. This is currently possible
with the hook, but I am not aware of a hook implementation that
does this.
We can find a way to update the hook interface to communicate to
Git that a remote disk is an appropriate case, but only if there
is a real need for that.
I'm not saying we can't support remote working directories.
We do get events from SMB for example, but there network
buffer limits and all the usual connectivity issues with a
local daemon reading an event stream from a remote file system.
So out of caution, I want to disable it for now. We can always
revisit it later.
WRT the builtin IPC-based daemon, I have the socket/named pipe
restricted to local access only. This prevents a client like
"git status" from talking to a remote daemon, but it also prevents
a remote bad guy from talking our daemon. But those are secondary
concerns right now -- I'm mainly concerned with correctly getting
all of the (possibly high volume/speed) events to the client.
There's also the possible incompatibilities of vendor X client-side
OS and vendor Y server-side OS and how well notifications are
supported between them....
So I'd just like to shut it off for now.
+ * For the builtin FSMonitor, we create the Unix domain socket for the
+ * IPC in the .git directory. If the working directory is remote,
+ * then the socket will be created on the remote file system. This
The socket is on the remote file system, but the daemon process is still
local, so I still see this a problem.
+ * can fail if the remote file system does not support UDS file types
+ * (e.g. smbfs to a Windows server) or if the remote kernel does not
+ * allow a non-local process to bind() the socket. (These problems
+ * could be fixed by moving the UDS out of the .git directory and to a
+ * well-known local directory on the client machine, but care should
+ * be taken to ensure that $HOME is actually local and not a managed
+ * file share.)
+ *
+ * So (for now at least), mark remote working directories as
+ * incompatible.
+ */
+static enum fsmonitor_reason is_remote(struct repository *r)
+{
+ struct statfs fs;
+
+ if (statfs(r->worktree, &fs) == -1) {
+ int saved_errno = errno;
+ trace_printf_key(&trace_fsmonitor, "statfs('%s') failed: %s",
+ r->worktree, strerror(saved_errno));
+ errno = saved_errno;
+ return FSMONITOR_REASON_ZERO;
So if we fail to inspect the filesystem, we report it as compatible?
I suppose other things are likely to fail if checks like this are
fialing, but I wonder if we should preempt that by marking this as
incompatible due to filesystem errors.
Good point.
+ }
+
+ trace_printf_key(&trace_fsmonitor,
+ "statfs('%s') [type 0x%08x][flags 0x%08x] '%s'",
+ r->worktree, fs.f_type, fs.f_flags, fs.f_fstypename);
+
+ if (!(fs.f_flags & MNT_LOCAL))
+ return FSMONITOR_REASON_REMOTE;
I do see that we need a successful response to give this specific
reason for incompatibility.
+ return FSMONITOR_REASON_ZERO;
+}
enum fsmonitor_reason fsm_os__incompatible(struct repository *r)
{
+ enum fsmonitor_reason reason;
+
+ reason = is_remote(r);
+ if (reason)
+ return reason;
This organization is looking like you want to short-circuit the
checks if you find an incompatibility, with the intent of having
multiple checks in the future.
But this can be done with simple || operators:
return is_remote() ||
reason_check_2() ||
reason_check_3();
True. That is a little shorter.
Thanks
Jeff
