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

Compare file paths in platform-appropriate way #2143

Open
ghost opened this issue Oct 30, 2020 · 6 comments
Open

Compare file paths in platform-appropriate way #2143

ghost opened this issue Oct 30, 2020 · 6 comments
Labels

Comments

@ghost
Copy link

ghost commented Oct 30, 2020

Windows file paths comparisons are case-insensitive; Unix file path comparisons are case-sensitive. Throughout the code we explicitly compare file paths with StringComparison.OrdinalIgnoreCase.

Possible approach: Provide a static class PlatformInformation with a property FilePathComparison

using System.Runtime.InteropServices;

public static class PlatformInformation
{
    public static StringComparison FilePathComparison =>
        RuntimeInformation.IsOSPlatform(OSPlatform.Windows)
        ? StringComparison.OrdinalIgnoreCase
        : StringComparison.Ordinal;
}

In all file path comparisons, use PlatformInformation.FilePathComparison instead of StringComparison.OrdinalIgnoreCase.

Other SARIF consumers (such as the VS Viewer extension, which is where I ran into this for the n+1st time) should use whatever facility the SDK provides.

@michaelcfanning

@ghost ghost added the bug label Oct 30, 2020
@michaelcfanning
Copy link
Member

michaelcfanning commented Oct 30, 2020

I don't believe a platform setting will help here. You can mount an NTFS volume as case-sensitive. In Windows 10 you can configure arbitrary directories as case-sensitive.

fsutil.exe file SetCaseSensitiveInfo C:\folder\path enable

@GabeDeBacker @roywill any advice on this problem?
SARIF allows for an artifact representing a directory to be persisted to the artifacts table. One possibility would be to require producers to persist case-sensitivity information as required to handle file look-ups for other artifacts. A sophisticated consumer might verify the directory setting at runtime (which could have changed).

Even without any directory information on this setting, perhaps a consumer could handle all operations as case-sensitive unless/until it's time to resolve a file path on disk (at which point, the case-sensitivity of the directory could be retrieved to help with name comparisons).

@jeffersonking
Copy link
Collaborator

Cursory research lead me to this: dotnet/runtime#15162 which leads to an open issue dotnet/runtime#14321. In short, even dotnet does not have a great answer.

@michaelcfanning
Copy link
Member

All right. Followed up offline with @roywill and here's the strat:

  1. On Win32, on attempting a file comparison against disk location, attempt to open the file using the provided name, which will resolve according to the volume/directory settings.
  2. If file open succeeds, use the file handle to call GetFinalPathNameByHandleW, which retrieves the actual file name as expressed in the file system.
  3. Perform a case-insensitive comparison between this final path name and the original in order to determine equivalence.
  4. Swap the GetFinalPathNameByHandleW-returned casing, if it differs, with the original request (to bring the name back into sync with the file system). [This last step seems like a best practice rather than a strict requirement].

In cases where the Win32 aren't available, we are running on a .NET core *NIX system and should consistently apply case-sensitive matches. As @lgolding suggests, a nice helper in the SDK would be good. From here, we need a fairly detailed scrub of path comparisons across the SDK itself to find places where utilizing the new API is necessary.

@ghost
Copy link
Author

ghost commented Oct 30, 2020

I'd like to understand if this will address the scenario (which I didn't originally explain) that prompted me to file this issue:

I have a SARIF file with a result, and the result offers a fix as follows:

{
  "results": [
    {
      "ruleId": "TEST1001",
      "message": {
        "text": "Something happened."
      },
      "locations": [
        {
          "artifactLocation": {
            "uri": "file.c",
            "uriBaseId": "SRCROOT"
          },
          "region": {
          }
        }
      ],
      "fixes": [
        {
          "description": {
            "text": "Fix the problem."
          },
          "artifactLocation": {
            "uri": "File.c",
            "uriBaseId": "SRCROOT"
          },
          "replacements": [
          ]
        }
      ]
    }
  ]
}

Note that the error was detected in file.c, but the fix offers to make a change in File.c. Are they the same file?

It's important to know that, not only because we have to make sure we're applying the fix in the right place, but also because -- at least for now -- the VS Viewer extension will only offer to apply a fix -- that is, it will only display the fix in a light bulb -- if all the replacements are in the same file you're currently editing (the one in which you clicked the light bulb). (There's nothing fundamental about that restriction. We just haven't thought about the user experience when multiple files are affected.)

I guess the viewer could call into the SDK helper twice, and it would try to open both files, and determine if they are the same. There would need to be a way to ask the helper that question.

@ghost
Copy link
Author

ghost commented Oct 30, 2020

Also, you might well ask why on earth a tool would have two different casings for the same file. Maybe the person who wrote the fixes code isn't the same as the person who wrote the results code.

Also, there are possibly ways out, for example, if both artifactLocations in my example specified the same index property, we'd know without looking that they were the same.

@michaelcfanning
Copy link
Member

We'd follow the algorithm in this case, look for File.c on disk, verify that it can be opened, retrieve the canonical file name from disk, and consult with VS to see if it's open in the IDE. I assume that VS honors the FS casing. This is a confusing topic, so if you'd prefer to discuss offline, let's do that.

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

No branches or pull requests

2 participants