RE: [PATCH v4 4/4] fsmonitor: normalize FSEvents event paths to the real path

From: Eric DeCosta <edecosta@mathworks.com>
Date: Sep 2, 2022, 16:35 UTC
Message-ID: <BYAPR05MB5573B477ED53CBC4C996729DD97A9@BYAPR05MB5573.namprd05.prod.outlook.com>
In-Reply-To: <bdf26d53-7068-87a6-484b-aeef99be35bd@jeffhostetler.com>

Show 31 quoted lines

> -----Original Message-----
> From: Jeff Hostetler <git@jeffhostetler.com>
> Sent: Thursday, September 1, 2022 4:06 PM
> To: Eric DeCosta via GitGitGadget <gitgitgadget@gmail.com>;
> git@vger.kernel.org
> Cc: Eric DeCosta <edecosta@mathworks.com>
> Subject: Re: [PATCH v4 4/4] fsmonitor: normalize FSEvents event paths to the
> real path
> 
> 
> 
> On 8/31/22 12:09 PM, Eric DeCosta via GitGitGadget wrote:
> > From: Eric DeCosta <edecosta@mathworks.com>
> >
> > Consider the following network working directory that is mounted under
> > /System/Volumes/Data:
> >
> > /network/working/directory
> >
> > The git working directory path is:
> >
> > /System/Volumes/Data/network/working/directory
> >
> > The paths reported by FSEvents always start with /network. fsmonitor
> > expects paths to be under the working directory; therefore it fails to
> > match /network/... and ignores the change.
> 
> I'm not sure I understand what's going on here.
> Are you saying that FSEvents reports network mounted directories with a
> path relative to the mount-point rather than an absolute path?
>

Yes

Show 14 quoted lines

> In your example, is "/network/working/directory" a valid path on your
> system (that also happens to be the same directory as
> "/System/Volumes/...")
> 
> That is, do you have some aliasing going on where both paths are valid -- like
> a pair of hard-linked directories?
> Or, is there something special about a network mount point?
> 
> 
> Did you have to "cd /System/Volumes/..." to get Git to have the absolute
> path be this?  Or were you doing a "cd /network/..."?
> (Sorry to ask so many questions but I don't have a pair of systems to test any
> of this on right now.)
>

 "/network/working/directory" is mounted under 
"/System/Volumes/Data/network/working/directory"

There is also a symlink:

"/network" -> "/System/Volumes/Data/network"

No matter if I "cd /System/Volumes/Data/network/working/directory" or "cd /network/working/directory" the paths reported by FSEvents always start with "/network/working/directory"

If I do a similar symlink with local directories, I do not get this unexpected behavior.

I need to remove the symlink and try it that way, but I need to coordinate with the machine's owner.

Show 48 quoted lines

> >
> > Change things such that if fsmonitor.allowRemote is true that the
> > paths reported via FSEevents are normalized to the real path.
> >
> > Signed-off-by: Eric DeCosta <edecosta@mathworks.com>
> > ---
> >   compat/fsmonitor/fsm-listen-darwin.c | 11 ++++++++---
> >   1 file changed, 8 insertions(+), 3 deletions(-)
> >
> > diff --git a/compat/fsmonitor/fsm-listen-darwin.c
> > b/compat/fsmonitor/fsm-listen-darwin.c
> > index 8e208e8289e..2ed828649ff 100644
> > --- a/compat/fsmonitor/fsm-listen-darwin.c
> > +++ b/compat/fsmonitor/fsm-listen-darwin.c
> > @@ -26,6 +26,7 @@
> >   #include "fsmonitor.h"
> >   #include "fsm-listen.h"
> >   #include "fsmonitor--daemon.h"
> > +#include "fsmonitor-settings.h"
> >
> >   struct fsm_listen_data
> >   {
> > @@ -183,7 +184,6 @@ static void my_add_path(struct fsmonitor_batch
> *batch, const char *path)
> >   	free(composed);
> >   }
> >
> > -
> >   static void fsevent_callback(ConstFSEventStreamRef streamRef,
> >   			     void *ctx,
> >   			     size_t num_of_events,
> > @@ -209,7 +209,12 @@ static void
> fsevent_callback(ConstFSEventStreamRef streamRef,
> >   		/*
> >   		 * On Mac, we receive an array of absolute paths.
> >   		 */
> > -		path_k = paths[k];
> > +		if (fsm_settings__get_allow_remote(the_repository) > 0) {
> > +			strbuf_reset(&tmp);
> > +			strbuf_realpath_forgiving(&tmp, paths[k], 0);
> > +			path_k = tmp.buf;
> > +		} else
> > +			path_k = paths[k];
> 
> This feels wrong.
> 
> It is not that fsmonitor.allowRemote is true, but whether or not this
> particular file system *IS* remote, right?

True. I suppose each path could be checked, or some bit could be set if the working directory is remote, perhaps along the lines of fsmonitor_ipc__get_path. Then the determination of the IPC path and whether it is remote would be in one place. fsm-settings-* would then just get that bit and check it against allowRemote.

Thoughts?

Show 25 quoted lines

> >
> >   		/*
> >   		 * If you want to debug FSEvents, log them to
> GIT_TRACE_FSMONITOR.
> > @@ -237,6 +242,7 @@ static void
> fsevent_callback(ConstFSEventStreamRef
> > streamRef,
> >
> >   			fsmonitor_force_resync(state);
> >   			fsmonitor_batch__free_list(batch);
> > +			batch = NULL;
> >   			string_list_clear(&cookie_list, 0);
> >
> >   			/*
> > @@ -313,7 +319,6 @@ static void
> fsevent_callback(ConstFSEventStreamRef
> > streamRef,
> >
> >   		case IS_WORKDIR_PATH:
> >   			/* try to queue normal pathnames */
> > -
> >   			if (trace_pass_fl(&trace_fsmonitor))
> >   				log_flags_set(path_k, event_flags[k]);
> >
> >