sln-add: Don't add solution folders outside the scope of solution directory #48611
Conversation
ghost
added
edvilme
changed the title
edvilme
changed the title
There was a problem hiding this comment.
Though I generally look favorably upon trying to clean up code, I don't think it's positive in servicing PRs, as it makes it harder to review and can lead to unexpected errors. I found GenerateIntermediateSolutionFoldersForProjectPath particularly confusing. I'm not saying this can't go in, but I don't think it should go into 9.0.3xx as-is.
| @@ -31,13 +32,21 @@ private static string GetSolutionFolderPathWithForwardSlashes(string path) | |||
| return "/" + string.Join("/", PathUtility.GetPathWithDirectorySeparator(path).Split(Path.DirectorySeparatorChar, StringSplitOptions.RemoveEmptyEntries)) + "/"; | |||
| } | |||
|
|
|||
| private static bool IsSolutionFolderPathInDirectoryScope(string relativePath) | |||
| { | |||
| return !string.IsNullOrEmpty(relativePath) | |||
There was a problem hiding this comment.
So for instance, if I had a path like src\Build\..\..\..\..\test\foo...
There was a problem hiding this comment.
I am not sure it is entirely possible to run into this issue since relativePath is obtained through Path.GetRelativePath, which should return the actual resolved path.
So instead of src\Build\..\..\..\..\test\foo, it should be ..\..\test\foo
edvilme
force-pushed
the
edvilme-sln-add-invalidchars
branch
from
835d30c to
0d20f00
Compare
|
I think you could potentially be more lenient with things with .. that end up still in the sln's cone, but 🤷 |
edvilme
added
Servicing-consider
and removed
untriaged
Fixes #48608
Description
When adding a project to a solution file, solution folders are automatically generated with the relative path (relative to the solution file path). When adding projects outside the scope of the solution working directory, solution folders with invalid names are created (e.g.,
..,:, etc).This was addressed in #46456 but didn't consider paths in different volumes.
Risk
Low - this doesn't alter existing behavior but rather considers a previously not considered edge case.
Regression
Yes - this worked before 9.0.2xx
Testing
Adding automated tests is difficult as it involves mounting several volumes, but manual tests were performed.
Expected behavior
Current behavior