use platform-specific filepath matching and add or revise relevant tests#432
Merged
chrisd8088 merged 9 commits intomicrosoft:mainfrom Sep 25, 2020
Merged
Conversation
2bf8edd to
364161d
Compare
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>
All OS platforms are capable of supporting directories with case-sensitivity of an opposite mode to their conventional one (i.e., case-sensitive on Windows/Mac or case-insensitive on Linux). However, Scalar at present assumes the normal case-sensitivity of each platform. Therefore we expand the check currently implemented only in the MacFileSystem IsFileSystemSupported() method so it executes on all platforms and tests whether the directory into which we are cloning has the expected case-sensitivity. (The IsFileSystemSupported() method is only called by the CloneVerb command, so we only execute this check at the time of initial cloning.)
Prior to adding any tests which check path case-sensitivity, we refactor the existing CloneTests and make several minor refinements. The SubfolderCloneShouldFail() private method is no longer utilized by more than one test, as it was previously until commit e0c16fb which removed the CloneInsideUnmountedEnlistment() test. Therefore we remove this utility method and instead create a new RunCloneCommand() private method which is more generic and which we can use in several of the remaining tests to eliminate common ProcessHelper setup code. We standardize in RunCloneCommand() on using the repository URL from ScalarTestConfig.RepoToClone, which is normally the Default.RepoToClone value from the Scalar.FunctionalTests.Properties.Settings class, but which can be overridden using the --repo-to-clone command-line option to the functional test program. One test was already using this value, but two others used the default one only. In the CloneInsideExistingEnlistment() test we also replace a Windows-specific enlistment root path which used backslash path separators with one created using Path.Combine().
We add a CloneInNonEmptyDirectory() test to CloneTests and use it to verify both that Scalar will refuse to clone within a non-empty directory.
We expand the CloneWithLocalCachePathWithinSrc() test so that we perform a further test of the check of the path given to the clone command's --local-cache-path option; specifically, we ensure that the check of whether the path falls under the Scalar working directory (i.e., the "src" directory) is done with case-sensitivity appropriate to the platform.
When listing all of the *.keep files in order to delete all but the one corresponding to the most recent good prefetch packfile, we ignore any whose names do not start with our "prefetch" prefix. This aligns with the logic expressed earlier in the same UpdateKeepPacks() method where *.pack files are searched, but only if their name starts with "prefetch-" and the expected maximum timestamp.
With the changes in commit e8f0a0f the need for a GetOldestPackTimestamp() utility method in the FetchStepWithoutSharedCacheTests was dropped, so we can tidy up by removing this method.
We add three tests to the FetchStepWithoutSharedCacheTests suite, one of which is based on the existing FetchStepCleansUpBadPrefetchPack() test but which checks the behaviour when the bad prefetch pack files' names differ in case from the expected ones. On Linux, where we expect case-sensitive filesystems, we ignore such bad files, while on macOS and Windows we treat them identically as in the existing test and delete them. The other two new tests perform similar checks but for the *.keep sentinel files that are created to mark the newest good prefetch packfile. Since a Scalar fetch maintenance job is expected to clean these up, leaving only one such file, we test that is true in FetchStepCleansUpBadKeepFiles(), and then our other new test performs the same case-sensitive variation as described above for FetchStepCleansUpBadPrefetchPack().
364161d to
31f8941
Compare
derrickstolee
approved these changes
Sep 24, 2020
Contributor
derrickstolee
left a comment
There was a problem hiding this comment.
I recommend only adding a new pointer in the FAQ about the fact that we now require the filesystem to match our expectations for case-sensitivity. It seems prudent to be forthcoming with that.
I really like how you extended and improved the existing tests to demonstrate this case-checking behavior!
| bool caseSensitiveFileSystem = this.Constants.CaseSensitiveFileSystem; | ||
| if (caseSensitive != caseSensitiveFileSystem) | ||
| { | ||
| errorMessage = $"Scalar does not support case {(caseSensitiveFileSystem ? "in" : "")}sensitive filesystems on {this.Name}"; |
Contributor
There was a problem hiding this comment.
This could use an update in the docs/faq.md possibly. Perhaps create an issue for supporting non-standard case-sensitivities? We could theoretically support it by adding some Git config or something. But, we would need some evidence that users actually care about that.
Contributor
Author
There was a problem hiding this comment.
Sounds good. I added some explanation to the FAQ, and also opened #441 to track any future enhancements.
1b45d2b to
52a9817
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Use filesystem-specific case matching when comparing file paths throughout Scalar and the functional test suite.
The primary change in this PR is to introduce a pair of wrappers which we use in place of
StringComparison.OrdinalIgnoreCaseandStringComparer.OrdinalIgnoreCasethroughout the codebase whenever we need to compare or match file names or paths.These new comparison utilities are named
ScalarPlatform.Instance.Constants.Path{Comparison,Comparer}in the main Scalar project, and have equivalents namedFileSystemHelpers.Path{Comparison,Comparer}in the functional test program's project.From microsoft/VFSForGit#1232, microsoft/VFSForGit#1412, and microsoft/VFSForGit#1503, with additional new work and tests.
Given its scope this PR is probably best reviewed commit-by-commit; they should be logically separate and relatively manageable in size.
Note that this PR is not a perfect solution, just a first approximation, because all three platforms support filesystems and/or directories with the opposite sensitivity to their typical mode:
casefoldext4 mounts on LinuxTherefore in commit f712d43 we rework the existing Mac-specific case-sensitivity check for all platforms and refactor it into a common method in
ScalarPlatform.This check only runs during the
scalar clonestep, and then only when theCreateScalarDirectories()method is called, which requires, among other things, that the local Git installation support the GVFS protocol. (This seems reasonable given that the key places where filepath matching is performed in the main Scalar codebase appear to be in maintenance routines handling the prefetch packfiles returned by the GVFS protocol and helper.)We also add and expand a few of the tests in the
EnlistmentPerFixture.CloneTestsandEnlistmentPerFixture.FetchStepWithoutSharedCacheTestsfunctional test suites, as these correspond most closely with the key remaining platform-variable file path checks in the mainScalar/CommandLineandScalar.Commoncode (now that much of the filepath-matching logic associated with VFSForGit has been removed).Specifically, the
scalar clonecommand executes a check to determine whether the user is accidentally passing a path with--local-cache-paththat is within the Scalar enlistment (i.e., the Scalar working directory), and that check is may be case-sensitive or case-insensitive depending on the platform.And the
scalar run fetchmaintenance command, when the GVFS protocol is in use, executes several cleanup steps such as theUpdateKeepPacks()method, which now looks forprefetch-*.packand*.keepfiles using our newScalarPlatform.Instance.Constants.PathComparisonpath-matching wrapper instead ofStringComparison.OrdinalIgnoreCase.So we extend the functional tests to cover these actions and in particular test that on Linux we see case-sensitive behaviour, while macOS and Windows behave case-insensitively when matching filename patterns and paths.
In addition to the above, we take the opportunity to tidy up both of the relevant functional test classes, removing some now-unused methods, refactoring a number of the
CloneTeststo use a new commonRunCloneCommand()method which also standardizes the test repository URL so that the--repo-to-clonefunctional test command-line option is fully respected, fixing a hard-coded path which contained Windows-style backslashes as path separators, and adding aCloneInNonEmptyDirectory()test. (The latter unfortunately cannot test the filepath check inTryCreateEnlistment(), though, because that check is used only to determine whether the user-provided path differs from the normalized path [which for our purposes is a Windows-specific concept] when printing the error message, and is not used in the actual check for an empty directory.)And lastly we make a change to the
UpdateKeepPacks()method mentioned above as part the maintenance executed by ascalar run fetchcommand; we adjust the retrieval of the list of "keep" sentinel files so that it filters out any which do not start with theScalarConstants.PrefetchPackPrefixstring, i.e., the prefixprefetch-. This aligns the behaviour when handling "keep" files to match that of the packfiles themselves earlier in the same method, and it then also allows our new tests to parallel each other for both types of files. It also looks to be in keeping with the case-sensitive filename prefix matching performed in thecb_keep_files()andcb_find_last()functions ingvfs-helper.cin https://github.com/microsoft/git, which I believe is what primarily writes newprefetch-*.{pack,keep,idx}files into the Scalar cache directory in the first place.