Perform case-sensitive path comparisons on Linux by kivikakk · Pull Request #1232 · microsoft/VFSForGit

kivikakk · 2019-06-03T07:16:32Z

On Windows and Mac, path comparisons are case-insensitive, as the filesystems are case-insensitive -- .git and .GIT are the same path name, and you can't have two different files or directories with names that only differ in case.

On Linux, filesystems (usually) are case-sensitive -- it is possible to have .git, .GIT and .Git entries in the one directory, all referring to different things. Accordingly, we need to do all path comparisons in a case-sensitive fashion.

This change should be a no-op for Windows and Mac, inasmuch as no different values will result after the change. This change does mean GVFSPlatform.Instance is being relied on in more places, and it's difficult to audit if a given piece of code could be called without GVFSPlatform.Instance already being initialised. (This bit me when working on #1084, where I tried to use GVFSPlatform.Instance in GVFS.FunctionalTests.LockHolder, which silently died with NRE because it wasn't initialised.)

Nonetheless, unit and (Windows) functional tests pass, so it might be fine.

Fixes github#29.
/cc @chrisd8088

kivikakk · 2019-06-04T07:01:54Z

/azp run GitHub VFSForGit Mac Functional Tests

azure-pipelines · 2019-06-04T07:02:00Z

No pipelines are associated with this pull request.

kivikakk · 2019-06-04T07:03:07Z

I thought I'd give it a try! Looks like maybe I don't have permissions?

derrickstolee · 2019-06-04T14:26:19Z

I thought I'd give it a try! Looks like maybe I don't have permissions?

It looks like the mac build worked for you. Sorry that you don't have access to the build results themselves. It's unfortunate that we always get those "No pipelines are associated..." messages.

derrickstolee

This seems straight-forward. I just have one question about how we can skip tests more eloquently using annotations, but I don't know if it is possible. Thanks!

derrickstolee · 2019-06-04T14:29:22Z

GVFS/GVFS.UnitTests/Prefetch/DiffHelperTests.cs

        [TestCase]
        public void ParsesCaseChangesAsAdds()
        {
+            if (GVFSPlatform.Instance.Constants.CaseSensitiveFileSystem)


I wonder if there is a way to do this with test categories using the [] annotations.

Time to find out!

be79df2 uses this approach -- thanks!

wilbaker · 2019-06-04T16:33:20Z

GVFS/GVFS.Common/PlaceholderListDatabase.cs

@@ -260,7 +260,7 @@ public void WriteAllEntriesAndFlush(IEnumerable<IPlaceholderData> updatedPlaceho

        private IEnumerable<string> GenerateDataLines(IEnumerable<IPlaceholderData> updatedPlaceholders)


FYI - On master the placeholders are now being stored in a SQLite database (see #1140). When updating the Linux feature branch to pick up that change you'll need to make the table creation call aware of whether the platform is case sensitive (currently the table is created with a case-insensitive path/key).

wilbaker · 2019-06-04T16:41:20Z

FYI - If you need to support repos that have files whose names only differ by case, you'll also need to update GitIndexProjection.SortedFolderEntries.

It uses LazyUTF8String.CaseInsensitiveCompare and it will need to use a new CaseSensitiveCompare if you need to support repos that allow names that only differ by case.

Please note that this is an extremely hot path, and has been tuned to eliminate as many instructions as possible (it's hit for every entry when parsing the index) and so we'll have to be careful with how this code is updated.

Related, there is a project GVFS.PerfProfiling that can be used to measure the perf of VFS4G's index loading/parsing code which I've found to be helpful when making changes in this area. Unfortunately it looks like it hasn't been converted to .NET core yet, and so right now it can only be run on Windows.

derrickstolee · 2019-06-04T16:51:34Z

FYI - If you need to support repos that have files whose names only differ by case, you'll also need to update GitIndexProjection.SortedFolderEntries.

It uses LazyUTF8String.CaseInsensitiveCompare and it will need to use a new CaseSensitiveCompare if you need to support repos that allow names that only differ by case.

Sounds like we will need a new set of functional tests that only run on Linux that check case-sensitive comparisons by explicitly adding files or folders that differ only by case, otherwise, we will never know if we've checked all the things we need to make this switch.

kivikakk · 2019-06-12T02:00:57Z

For now we've decided to not support in-tree files whose name only differ by case -- this PR will be scoped only to the operations VFS for Git performs on the filesystem for its own functioning (like locating the .git directory, or matching .keep and .idx files, etc.) -- the repository content will remain case-insensitive.

This only applies to on-disk operations like locating the ".git" directory or ".idx" files, etc. -- the repository contents remain case-insensitive.

kivikakk · 2019-06-12T06:38:12Z

I think this should be good to go -- @derrickstolee @wilbaker fyi in case you'd like to re-review. I've lessened the scope of the change so nothing that impacts tracked content should be changed now.

chrisd8088 · 2019-06-13T21:25:03Z

Purely as a note for future work (not for this PR, unless someone feels it's important), I have a suspicion there are a few places in the functional test suite where we may want to have a similar OS-specific path comparator instead of StringComparison.OrdinalIgnoreCase().

kivikakk · 2019-06-13T23:19:16Z

The paths referred to in those instances are in-tree paths (tracked content), and so are not in the scope of the change proposed in this PR. We'll only change those if we proceed to make VFSForGit support case-sensitivity in the repository contents. Right now we're just behaving appropriately with respect to the backing store.

…Linux

TODO -- since we have no failing tests without this work, add some. Use filesystem-specific case matching when comparing path throughout Scalar and the functional test suite. From microsoft/VFSForGit#1232, microsoft/VFSForGit#1412, and microsoft/VFSForGit#1503. From microsoft/VFSForGit@e6eb054, microsoft/VFSForGit@66e3e21, microsoft/VFSForGit@6067f04, and microsoft/VFSForGit@7100179. Co-authored-by: Ashe Connor <ashe@kivikakk.ee>

Use filesystem-specific case matching when comparing path throughout Scalar and the functional test suite. From microsoft/VFSForGit#1232, microsoft/VFSForGit#1412, and microsoft/VFSForGit#1503. From microsoft/VFSForGit@e6eb054, microsoft/VFSForGit@66e3e21, microsoft/VFSForGit@6067f04, and microsoft/VFSForGit@7100179. Co-authored-by: Ashe Connor <ashe@kivikakk.ee>

kivikakk self-assigned this Jun 3, 2019

kivikakk added the platform: linux label Jun 3, 2019

kivikakk changed the title ~~introduce path case-sensitivity platform property~~ Perform case-sensitive path comparisons on Linux Jun 4, 2019

kivikakk marked this pull request as ready for review June 4, 2019 06:54

kivikakk requested review from derrickstolee and wilbaker June 4, 2019 06:54

derrickstolee approved these changes Jun 4, 2019

View reviewed changes

wilbaker reviewed Jun 4, 2019

View reviewed changes

This was referenced Jun 6, 2019

Tracking: Set up unit and functional test CI for Linux github/VFSForGit#18

Closed

Tracking: Run VFSForGit functional tests on Linux github/VFSForGit#16

Closed

kivikakk force-pushed the platform-case-sensitivity branch from b570b92 to 105c1f5 Compare June 12, 2019 02:00

kivikakk force-pushed the platform-case-sensitivity branch from 105c1f5 to 33369e0 Compare June 12, 2019 03:38

use platform-specific property for case matching

c2a89da

This only applies to on-disk operations like locating the ".git" directory or ".idx" files, etc. -- the repository contents remain case-insensitive.

kivikakk force-pushed the platform-case-sensitivity branch from 33369e0 to c2a89da Compare June 12, 2019 04:03

kivikakk merged commit 69917ec into microsoft:features/linuxprototype Jun 19, 2019

kivikakk deleted the platform-case-sensitivity branch June 19, 2019 03:56

kivikakk mentioned this pull request Jun 20, 2019

Support case-sensitive file matching on Linux github/VFSForGit#29

Closed

chrisd8088 pushed a commit that referenced this pull request Jul 20, 2019

Merge pull request #1232: Perform case-sensitive path comparisons on …

b5f523d

…Linux

chrisd8088 pushed a commit that referenced this pull request Jul 26, 2019

Merge pull request #1232: Perform case-sensitive path comparisons on …

a3e7532

…Linux

chrisd8088 mentioned this pull request Sep 16, 2020

use platform-specific filepath matching and add or revise relevant tests microsoft/scalar#432

Merged

		@@ -260,7 +260,7 @@ public void WriteAllEntriesAndFlush(IEnumerable<IPlaceholderData> updatedPlaceho

		private IEnumerable<string> GenerateDataLines(IEnumerable<IPlaceholderData> updatedPlaceholders)

Conversation

kivikakk commented Jun 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kivikakk commented Jun 4, 2019

Uh oh!

azure-pipelines bot commented Jun 4, 2019

Uh oh!

kivikakk commented Jun 4, 2019

Uh oh!

derrickstolee commented Jun 4, 2019

Uh oh!

derrickstolee left a comment

Choose a reason for hiding this comment

Uh oh!

derrickstolee Jun 4, 2019

Choose a reason for hiding this comment

Uh oh!

kivikakk Jun 6, 2019

Choose a reason for hiding this comment

Uh oh!

kivikakk Jun 6, 2019

Choose a reason for hiding this comment

Uh oh!

wilbaker Jun 4, 2019

Choose a reason for hiding this comment

Uh oh!

wilbaker commented Jun 4, 2019

Uh oh!

derrickstolee commented Jun 4, 2019

Uh oh!

kivikakk commented Jun 12, 2019

Uh oh!

kivikakk commented Jun 12, 2019

Uh oh!

chrisd8088 commented Jun 13, 2019

Uh oh!

kivikakk commented Jun 13, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kivikakk commented Jun 3, 2019 •

edited

Loading