Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Building VFS for Git in 2024 #1804

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

derrickstolee
Copy link
Contributor

@derrickstolee derrickstolee commented Feb 15, 2024

In order to evaluate the merit of other changes, such as #1803, I needed these changes locally in order to get a clean functional test run. Each change is isolated to its own commit, so review commit-by-commit.

Generally, there are a few categories of problems:

  1. README updates focusing on the actual requirements and dropping VS 2017 from mention.
  2. A fix for a version parsing problem. Also update the version to the latest sprint (https://whatsprintis.it/).
  3. Update a stale Git config key to avoid warnings.
  4. Modify a couple failing functional test due to Git's updated handling of empty/truncated objects. They fail with v2.43.0.vfs.0.0 which is not currently used in the PR/CI builds.

When trying to run functional tests for a locally-build version, I got
the following error:

Cannot clone @ C:\Repos\GVFSFunctionalTests\enlistment\29fd71cb642b4523bc0a: System.FormatException: Input string was not in a correct format.
   at System.Version.VersionResult.SetFailure(ParseFailureKind failure, String argument)
   at System.Version.TryParseComponent(String component, String componentName, VersionResult& result, Int32& parsedComponent)
   at System.Version.TryParseVersion(String version, VersionResult& result)
   at System.Version.Parse(String input)
   at System.Version..ctor(String version)
   at GVFS.CommandLine.GVFSVerb.TryValidateGVFSVersion(GVFSEnlistment enlistment, ITracer tracer, ServerGVFSConfig config, String& errorMessage, Boolean& errorIsFatal)
   at GVFS.CommandLine.GVFSVerb.ValidateClientVersions(ITracer tracer, GVFSEnlistment enlistment, ServerGVFSConfig gvfsConfig, Boolean showWarnings)
   at GVFS.CommandLine.CloneVerb.Execute()

The version string I was seeing was of the form "GVFS 0.2.XXX.Y+<id>"
so my guess is that the "+<id>" portion is causing the System.Version
logic to complain.
At some point, the config option was renamed from
core.useBuiltinFSMonitor to simply core.fsmonitor=(true|false). The
core.fsmonitor can also be used to specify a hook name, which led to the
initial distinction.

Without this update, the functional tests fail due to stderr differences
including this warning:

  warning: virtual repository '...' is incompatible with fsmonitor

This is due to core.fsmonitor=true being implied by default.
Fixing these tests will require changes to microsoft/git to reenable
behavior that is more careful about broken objects.
@@ -55,7 +55,7 @@ public void Initialize()
GitProcess.Invoke(this.RootPath, "config merge.renames false");
GitProcess.Invoke(this.RootPath, "config advice.statusUoption false");
GitProcess.Invoke(this.RootPath, "config core.abbrev 40");
GitProcess.Invoke(this.RootPath, "config core.useBuiltinFSMonitor false");
GitProcess.Invoke(this.RootPath, "config core.fsmonitor false");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jeffhostetler: Perhaps the old line should stay here, and we just add the core.fsmonitor line, too. Keeping the old config won't hurt, and might help for users still on an older version of Git.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, leave both. It won't hurt anything. I haven't checked recently to see how many users are still using very old versions of Git, but let's assume we have some...

@@ -311,7 +311,7 @@ public static bool TrySetRequiredGitConfigSettings(Enlistment enlistment)
{ "commitGraph.generationVersion", "1" },

// Disable the builtin FS Monitor in case it was enabled globally.
{ "core.useBuiltinFSMonitor", "false" },
{ "core.fsmonitor", "false" },

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants