-
Notifications
You must be signed in to change notification settings - Fork 2k
Feature: PWD-per-drive to facilitate working on multiple drives at Windows #14411
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
Conversation
…susPlusUS/nushell into feature-current-directory-per-drive
|
Just for understanding, which commands beyond |
|
Based on my tests, 'mktemp' and 'glob' do not require any changes as they restrict name patterns without concerns about relative paths on some drives. Generally, other filesystem commands need slight modifications, usually changing direct calls to expand_path_with(...) to stack.expand_path_with(...). |
|
The shared DriveToPwdMap has been removed. Now, each Stack has its own map, which will be cloned to the child and cloned back when the child returns. The caller will take the callee's map if the environment is redirected (using def --env). Currently, auto_cd, cd, and auto_completion support PWD-per-drive. Other file system commands will be updated after the current commit passes review. In cd.rs, the call to nu_path::expand_path_with(...) has been modified to stack.expand_path_with(...). |
|
I think this looks pretty good now. I've asked other core-team members to take a look and provide feedback since the changes are pretty invasive. And of course, the CI needs to be green. Thanks for working so hard on this! |
|
|
||
| // Helper stub/proxy for nu_path::expand_path_with::<P, Q>(path, relative_to, expand_tilde) | ||
| // Facilitates file system commands to easily gain the ability to expand PWD-per-drive | ||
| pub fn expand_path_with<P, Q>(&self, path: P, relative_to: Q, expand_tilde: bool) -> PathBuf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like it's a little weird that this API does not expand relative to what you passed in if it is a drive-relative path
Does Stack always have enough information to do PWD anyway? I forget if EngineState::cwd actually needs anything from there if stack is provided. I would rather have an API that doesn't make you provide the CWD if it's going to be derived from the stack in some cases anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Originally, when the file system command code calls nu_path::expand_path_with<P, Q>(path, relative_to, expand_tilde), it provides relative_to as a hint and uses expand_tilde as an indicator. The Stack acts as a stub or proxy here, primarily to check if, on Windows, the path can be expanded by PWD-per-drive. If the path is relative to a drive, PWD-per-drive ensures it is expanded to an absolute path and returns it. If the path is not relative to drive, it forwards the call to nu_path::expand_path_with<P, Q>(path, relative_to, expand_tilde) to complete the task as before.
My pleasure! A phantom modification of Cargo.lock by background tasks often blocks CI auto verification. I'll try to catch and pause these background tasks. |
|
Let's move forward with this and see how we like it. Thanks for all the effort and working with us on changes @PegasusPlusUS! |
|
OK, continue on other file system commands… |
|
I'm not sure. There's some debate on whether we'll keep this PR landed. Let's see how it works for a bit. |
|
@PegasusPlusUS If you want to add other command PRs, go ahead. There is still some debate over this feature though. It would be good to add some integration tests to ensure this feature doesn't get accidentally broken, perhaps using the subst command. |
|
Since the PR changes some of our core structures, it is important to stress-test it with various edge cases. I discovered one case when it does not work correctly with overlays: |
|
OK, I'm working on other commands, and I'll check for the edge cases |
This PR implements PWD-per-drive as described in discussion #14355
Description
On Windows, CMD or PowerShell assigns each drive its own current directory. For example, if you are in 'C:\Windows', switch to 'D:', and navigate to 'D:\Game', you can return to 'C:\Windows' by simply typing 'C:'.
This PR enables Nushell on Windows to have the same capability, allowing each drive to maintain its own PWD (Present Working Directory).
User-Facing Changes
Currently, 'cd' or 'ls' only accept absolute paths if the path starts with 'C:' or another drive letter. With PWD-per-drive, users can use 'cd' (or auto cd) and 'ls' in the same way as 'cd' and 'dir' in PowerShell, or similarly to 'cd' and 'dir' in CMD (noting that cd in CMD has slightly different behavior, 'cd' for another drive only changes current directory of that drive, but does not switch there).
Interaction example on switching between drives:
Interaction example on auto-completion at cmd line:
Interaction example on pass PWD-per-drive to child process: (Note CMD will use it, but PowerShell will ignore it though it still prepares such info for child process)
Brief Change Description
1.Added 'crates/nu-path/src/pwd_per_drive.rs' to implement a 26-slot array mapping drive letters to PWDs. Test cases are included in the same file, along with a doctest for the usage of PWD-per-drive.
2. Modified 'crates/nu-path/src/lib.rs' to declare module of pwd_per_drive and export struct for PWD-per-drive.
3. Modified 'crates/nu-protocol/src/engine/stack.rs' to sync PWD when set_cwd() is called. Add PWD-per-drive map as member. Clone between parent and child. Stub/proxy for nu_path::expand_path_with() to facilitate filesystem commands using PWD-per-drive.
4. Modified 'crates/nu-cli/src/repl.rs' auto_cd uses PWD-per-drive to expand path.
5. Modified 'crates/nu-cli/src/completions/completion_common.rs' to expand relative path when press [TAB] at command line.
6. Modified 'crates/nu-engine/src/env.rs' to collect PWD-per-drive info as env vars for child process as CMD or PowerShell do, this can let child process inherit PWD-per-drive info.
7. Modified 'crates/nu-engine/src/eval.rs', caller clone callee's PWD-per-drive info, supporting 'def --env'
8. Modified 'crates/nu-engine/src/eval_ir.rs', 'def --env' support. Remove duplicated fn redirect_env()
9. Modified 'src/run.rs', to init PWD-per-drive when startup.
filesystem commands that modified:
Other commands, commit pending....
Local test def --env OK:
Tests + Formatting
cargo fmt --all -- --checkpassed.cargo clippy --workspace -- -D warnings -D clippy::unwrap_usedpassed.cargo test --workspacepassed on Windows developer mode and Ubuntu.cargo run -- -c "use toolkit.nu; toolkit test stdlib"passed.