Skip splitting for paths Fixes #48794 #48874
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes issue #48794 by preventing unintended splitting of package paths.
- Introduces a check to determine if a package argument is a valid file or directory.
- Uses this check to conditionally skip path splitting, treating the entire argument as a package path if it exists.
| List<InstallRequest> installRequests = new(); | ||
|
|
||
| foreach (string installArg in args.TemplatePackages) | ||
| { |
There was a problem hiding this comment.
Consider adding a comment to explain that the check is used to skip splitting for arguments that represent existing file or directory paths, which will help future maintainers understand the purpose of this logic.
| { | |
| { | |
| // Check if the argument represents an existing file or directory path. | |
| // If it is a path, skip splitting the argument, as it is already in the desired format. |
| foreach (string installArg in args.TemplatePackages) | ||
| { | ||
| string[] split = installArg.Split(["::"], StringSplitOptions.RemoveEmptyEntries).SelectMany(arg => arg.Split('@', StringSplitOptions.RemoveEmptyEntries)).ToArray(); | ||
| bool isPath = File.Exists(installArg) || Directory.Exists(installArg); |
There was a problem hiding this comment.
Isn't there something to check if it is a path? Like, something related to that RelativePath utility thing. Because you shouldn't need to check if something is on-disk to know if the argument is a path or not.
There was a problem hiding this comment.
We could call http://msdn.microsoft.com/en-us/library/system.io.path.getfullpath.aspx Path.GetFullPath and catch the exception to see if it's not a path?
There was a problem hiding this comment.
I don't know that I agree with this. If you want to dotnet new install something in your current directory, it would be very hard to tell (without checking if it exists) that it's a path.
There was a problem hiding this comment.
And if you were to just check for an exception from Path.GetFullPath, I think ~any package would show up as a path
nagilson
left a comment
nagilson
left a comment
There was a problem hiding this comment.
This change looks important, I think this is a good way to do it. May you also please add a theory test to prevent regression of this behavior with paths on linux/osx/win?
Test added. The temp path should be OS-specific to some extent, so I think that would cover all those cases and would mean that you don't need OS-specific Facts. |
|
Note: Ready for review and merge. |
|
/backport to release/9.0.3xx |
|
Started backporting to release/9.0.3xx: https://github.com/dotnet/sdk/actions/runs/17869154103 |
|
@baronfel backporting to "release/9.0.3xx" failed, the patch most likely resulted in conflicts: $ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch
Applying: Skip splitting for paths
Using index info to reconstruct a base tree...
M src/Cli/Microsoft.TemplateEngine.Cli/TemplatePackageCoordinator.cs
Falling back to patching base and 3-way merge...
Auto-merging src/Cli/Microsoft.TemplateEngine.Cli/TemplatePackageCoordinator.cs
Applying: Add test
Using index info to reconstruct a base tree...
A test/dotnet-new.IntegrationTests/DotnetNewInstallTests.cs
Falling back to patching base and 3-way merge...
Auto-merging test/dotnet-new.Tests/DotnetNewInstallTests.cs
CONFLICT (content): Merge conflict in test/dotnet-new.Tests/DotnetNewInstallTests.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config set advice.mergeConflict false"
Patch failed at 0002 Add test
Error: The process '/usr/bin/git' failed with exit code 128Please backport manually! |
5 participants
Fixes #48794