[07/23] fsmonitor-settings: virtual repos are incompatible with FSMonitor

Commit Message

Jeff Hostetler Feb. 15, 2022, 3:59 p.m. UTC

From: Jeff Hostetler <[e-mail]>

Virtual repos, such as GVFS (aka VFS for Git), are incompatible
with FSMonitor.

Signed-off-by: Jeff Hostetler <[e-mail]>
---
 compat/fsmonitor/fsm-settings-win32.c | 26 ++++++++++++++++++++++++++
 fsmonitor-settings.c                  |  5 +++++
 fsmonitor-settings.h                  |  1 +
 t/t7519-status-fsmonitor.sh           |  9 +++++++++
 4 files changed, 41 insertions(+)

Comments

Derrick Stolee Feb. 24, 2022, 3:11 p.m. UTC | #1

On 2/15/2022 10:59 AM, Jeff Hostetler via GitGitGadget wrote:
> From: Jeff Hostetler <[e-mail]>
> 
> Virtual repos, such as GVFS (aka VFS for Git), are incompatible
> with FSMonitor.

I would swap all of your "GVFS (aka VFS for Git)" for just
"VFS for Git".

> +/*
> + * GVFS (aka VFS for Git) is incompatible with FSMonitor.
> + *
> + * Granted, core Git does not know anything about GVFS and we
> + * shouldn't make assumptions about a downstream feature, but users
> + * can install both versions.  And this can lead to incorrect results
> + * from core Git commands.  So, without bringing in any of the GVFS
> + * code, do a simple config test for a published config setting.  (We
> + * do not look at the various *_TEST_* environment variables.)
> + */
> +static enum fsmonitor_reason is_virtual(struct repository *r)
> +{
> +	const char *const_str;
> +
> +	if (!repo_config_get_value(r, "core.virtualfilesystem", &const_str))
> +		return FSMONITOR_REASON_VIRTUAL;
> +
> +	return FSMONITOR_REASON_ZERO;
> +}

This reason seems to be specific to a config setting that only
exists in the microsoft/git fork. Perhaps this patch should remain
there.

However, there is also the discussion of vfsd going on, so something
similar might be necessary for that system. Junio also mentioned
wanting to collaborate on a common indicator that virtualization was
being used, so maybe we _should_ make core.virtualFilesystem a config
setting in the core Git project.

The reason for the incompatibility here is that VFS for Git has its
own filesystem watcher and Git gets updates from it via custom code
that is a precursor to this FS Monitor feature. I don't know if vfsd
has plans for a similar setup. (It would probably be best to fit
into the FS Monitor client/server model and use a different daemon
for those virtualized repos, but I don't know enough details to be
sure.)

CC'ing Jonathan Nieder for thoughts on this.

Thanks,
-Stolee

Jeff Hostetler March 1, 2022, 9:01 p.m. UTC | #2

On 2/24/22 10:11 AM, Derrick Stolee wrote:
> On 2/15/2022 10:59 AM, Jeff Hostetler via GitGitGadget wrote:
>> From: Jeff Hostetler <[e-mail]>
>>
>> Virtual repos, such as GVFS (aka VFS for Git), are incompatible
>> with FSMonitor.
> 
> I would swap all of your "GVFS (aka VFS for Git)" for just
> "VFS for Git".
> 
>> +/*
>> + * GVFS (aka VFS for Git) is incompatible with FSMonitor.
>> + *
>> + * Granted, core Git does not know anything about GVFS and we
>> + * shouldn't make assumptions about a downstream feature, but users
>> + * can install both versions.  And this can lead to incorrect results
>> + * from core Git commands.  So, without bringing in any of the GVFS
>> + * code, do a simple config test for a published config setting.  (We
>> + * do not look at the various *_TEST_* environment variables.)
>> + */
>> +static enum fsmonitor_reason is_virtual(struct repository *r)
>> +{
>> +	const char *const_str;
>> +
>> +	if (!repo_config_get_value(r, "core.virtualfilesystem", &const_str))
>> +		return FSMONITOR_REASON_VIRTUAL;
>> +
>> +	return FSMONITOR_REASON_ZERO;
>> +}
> 
> This reason seems to be specific to a config setting that only
> exists in the microsoft/git fork. Perhaps this patch should remain
> there.
> 
> However, there is also the discussion of vfsd going on, so something
> similar might be necessary for that system. Junio also mentioned
> wanting to collaborate on a common indicator that virtualization was
> being used, so maybe we _should_ make core.virtualFilesystem a config
> setting in the core Git project.
> 
> The reason for the incompatibility here is that VFS for Git has its
> own filesystem watcher and Git gets updates from it via custom code
> that is a precursor to this FS Monitor feature. I don't know if vfsd
> has plans for a similar setup. (It would probably be best to fit
> into the FS Monitor client/server model and use a different daemon
> for those virtualized repos, but I don't know enough details to be
> sure.)
> 
> CC'ing Jonathan Nieder for thoughts on this.

I was wondering whether this should be upstream or just in our
downstream forks, but I thought it better to include it so that
we don't try to monitor a virtualized repo and not mislead the
user.  It may be that we can correctly watch the repo and
generate correct results, but without knowing any details of
what the virtualization is doing behind the scenes, we would
be making some unsafe assumptions.  And since Windows users often
have multiple versions of Git installed (their CL tools and
whatever their IDE installed), promoting this check to upstream
felt important.

WRT the ongoing "vfsd" effort, I'm not sure what that looks like
yet and/or whether repos managed by "vfsd" have similar concerns.

I'll rename the variables in my patch here to be "vfs4git" rather
the generic "virtual".  This will avoid confusion later if another
case needs to be added for "vfsd".  My code is Win32-specific and
it is unclear it theirs will be Linux-only or cross-platform, so
hopefully with the rename we can coexist sanely.

Jeff

Patch

diff --git a/compat/fsmonitor/fsm-settings-win32.c b/compat/fsmonitor/fsm-settings-win32.c
index 176a6f5726c..7caa79570af 100644
--- a/compat/fsmonitor/fsm-settings-win32.c
+++ b/compat/fsmonitor/fsm-settings-win32.c
@@ -3,7 +3,33 @@ 
 #include "repository.h"
 #include "fsmonitor-settings.h"
 
+/*
+ * GVFS (aka VFS for Git) is incompatible with FSMonitor.
+ *
+ * Granted, core Git does not know anything about GVFS and we
+ * shouldn't make assumptions about a downstream feature, but users
+ * can install both versions.  And this can lead to incorrect results
+ * from core Git commands.  So, without bringing in any of the GVFS
+ * code, do a simple config test for a published config setting.  (We
+ * do not look at the various *_TEST_* environment variables.)
+ */
+static enum fsmonitor_reason is_virtual(struct repository *r)
+{
+	const char *const_str;
+
+	if (!repo_config_get_value(r, "core.virtualfilesystem", &const_str))
+		return FSMONITOR_REASON_VIRTUAL;
+
+	return FSMONITOR_REASON_ZERO;
+}
+
 enum fsmonitor_reason fsm_os__incompatible(struct repository *r)
 {
+	enum fsmonitor_reason reason;
+
+	reason = is_virtual(r);
+	if (reason)
+		return reason;
+
 	return FSMONITOR_REASON_ZERO;
 }
diff --git a/fsmonitor-settings.c b/fsmonitor-settings.c
index e445572354e..bb2ddd2457f 100644
--- a/fsmonitor-settings.c
+++ b/fsmonitor-settings.c
@@ -156,6 +156,11 @@  static void create_reason_message(struct repository *r,
 			      _("bare repos are incompatible with fsmonitor"));
 		return;
 
+	case FSMONITOR_REASON_VIRTUAL:
+		strbuf_addstr(buf_reason,
+			      _("virtual repos are incompatible with fsmonitor"));
+		return;
+
 	default:
 		BUG("Unhandled case in create_reason_message '%d'", s->reason);
 	}
diff --git a/fsmonitor-settings.h b/fsmonitor-settings.h
index b52bf8edaf1..c169683bf2d 100644
--- a/fsmonitor-settings.h
+++ b/fsmonitor-settings.h
@@ -16,6 +16,7 @@  enum fsmonitor_mode {
 enum fsmonitor_reason {
 	FSMONITOR_REASON_ZERO = 0,
 	FSMONITOR_REASON_BARE = 1,
+	FSMONITOR_REASON_VIRTUAL = 2,
 };
 
 void fsm_settings__set_ipc(struct repository *r);
diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
index 3c4e6f5f89c..d7540931a16 100755
--- a/t/t7519-status-fsmonitor.sh
+++ b/t/t7519-status-fsmonitor.sh
@@ -81,6 +81,15 @@  test_expect_success FSMONITOR_DAEMON 'run fsmonitor-daemon in bare repo' '
 	grep "bare repos are incompatible with fsmonitor" actual
 '
 
+test_expect_success MINGW,FSMONITOR_DAEMON 'run fsmonitor-daemon in virtual repo' '
+	test_when_finished "rm -rf ./fake-virtual-clone actual" &&
+	git init fake-virtual-clone &&
+	test_must_fail git -C ./fake-virtual-clone \
+			   -c core.virtualfilesystem=true \
+			   fsmonitor--daemon run 2>actual &&
+	grep "virtual repos are incompatible with fsmonitor" actual
+'
+
 test_expect_success 'setup' '
 	mkdir -p .git/hooks &&
 	: >tracked &&