Combine rootDir with relative file names

[jozkee]

Fixes #50648

Using Path.GetFullPath on relative file names causes that the in-memory Matcher.Match to treat files as relative to the CWD blocking scenarios that are using a rootDir in a different drive as the one discussed in #50648. We should combine the rootDir with the relative file names instead. This is also what's described in the documentation.

files IEnumerable<String>
Collection of file names. If relative paths rootDir will be prepended to the paths.

However, this is a (massive?) breaking change as there's already a lot of tests showing that filenames are relative to CWD and not to rootDir, and in the wild, there will be most likely use cases that depend in this bug (need to check).

[ghost]

Added needs-breaking-change-doc-created label because this PR has the breaking-change label.

When you commit this breaking change:

1. Create and link to this PR and the issue a matching issue in the dotnet/docs repo using the breaking change documentation template, then remove this needs-breaking-change-doc-created label.
2. Ask a committer to mail the .NET Breaking Change Notification DL.

Tagging @dotnet/compat for awareness of the breaking change.

[ghost]

Tagging subscribers to this area: @dotnet/area-extensions-filesystem
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Fixes #50648

Using Path.GetFullPath on relative file names causes that the in-memory Matcher.Match to treat files as relative to the CWD blocking scenarios that are using a rootDir in a different drive as the one discussed in #50648. We should combine the rootDir with the relative file names instead. This is also what's described in the documentation.

files IEnumerable<String>
Collection of file names. If relative paths rootDir will be prepended to the paths.

However, this is a (massive?) breaking change as there's already a lot of tests showing that filenames are relative to CWD and not to rootDir, and in the wild, there will be most likely use cases that depend in this bug (need to check).

Author:	Jozkee
Assignees:	Jozkee
Labels:	breaking-change, area-Extensions-FileSystem
Milestone:	9.0.0

[jeffhandley]

This looks like a good change to make, and it's valuable to make the change very early in .NET 9.

Given the potential breaking-change impact though, we might need to produce a qualified assessment of risk and probably even find ways to alert users this is coming. We can partner with the @dotnet/compat folks to study call sites, identify some places that might be depending on this behavior, and determine if this is something we need to phase in with a compat switch. We might even discover a scenario where this behavior could not be opted into. If we see a lot of risk, we might even want to explore an analyzer to produce a warning on call sites. @jozkee I'll follow up with you offline.

In the meantime, I'll mark this PR as NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) .

[jozkee]

I found one usage of the affected overload in our first party data. It is for smoke tests on dotnet/installer. This leads me to believe that most people use the overloads that don’t take a rootDir, those use the cwd as rootDir inplicitly.

I’ll try to visualize the breaking change:

string rootDir = "dir1";
string[] files = ["dir1/test.0", "dir1/subdir/test.1", "dir2/test.2"];

PatternMatchingResult result = new Matcher().AddInclude("**/*").Match(rootDir, files);
Console.WriteLine(string.Join(", ", result.Files.Select(x => x.Path)));

Because both args are treated as relative to the cwd.
This currently prints test.0, subdir/test.1.
With the breaking change this now would print dir1/test.0, dir1/subdir/test.1, dir2/test.2. It includes dir2 because now all is under rootDir.

In order to get a similar (but not same) output as prior you need to change the code to something like the following:

string rootDir = "dir1";
string[] files = ["dir1/test.0", "dir1/subdir/test.1", "dir2/test.2"];

-PatternMatchingResult result = new Matcher().AddInclude("**/*").Match(rootDir, files);
+PatternMatchingResult result = new Matcher().AddInclude("dir1/**/*").Match(rootDir, files);
Console.WriteLine(string.Join(", ", result.Files.Select(x => x.Path)));

// prints
// dir1/test.0
// dir1/subdir/test.1

@ericstj would be good to send this to the VMR in order to validate that the dotnet/installer test doesn't break with this change?

[jozkee]

Ping @adamsitnik @jeffhandley @ericstj

[jeffhandley]

Good find, @jozkee! Is there a way to update the calling code before the change goes through that would allow the calling code to function as desired both now and after the breaking change goes through? If so, we could go ahead and update dotnet/installer so that as the breaking change goes through the tests will keep passing. If not, then we might want to consider disabling that test until the change flows through and then re-enabling it.

[jozkee]

@jeffhandley I can verify it, just need to find out how to run the smoke tests in dotnet/installer.

[ericstj]

With the breaking change this now would print dir1/test.0, dir1/subdir/test.1, dir2/test.2. It includes dir2 because now all is under rootDir.

How is dir2 under rootDir? I thought rootDir was dir1.

@ericstj would be good to send this to the VMR

Not necessary - it wouldn't be running these tests (nor is it accepting commits yet). You'd just need to watch this change flowing up to installer then fix up the test when it hits it.

[jozkee]

How is dir2 under rootDir?

Because now rootDir is prefixed to each path in files, except when a path is absolute. In a simplified manner:

foreach (string file in files)
{
    if (Path.IsPathRooted(file))
        fileList.Add(file);
    else
        fileList.Add(Path.Combine(rootDir, file));
}

fileList now contains
"C:/somepath/dir1/dir1/test.0"
"C:/somepath/dir1/dir1/subdir/test.1"
"C:/somepath/dir1/dir2/test.2"

While previously it contained the following, because rootDir was not being prefixed.
"C:/somepath/dir1/test.0"
"C:/somepath/dir1/subdir/test.1"
"C:/somepath/dir2/test.2"

[ericstj]

I see - might be better to not reuse the name - since the use of dir1 implies the same dir1

[jozkee]

Breaking change doc. created: dotnet/docs#39189

[ViktorHofer]

I think this change caused another unintended breaking change. Filed: #100762