Skip to content

Conversation

@PegasusPlusUS
Copy link
Contributor

@PegasusPlusUS PegasusPlusUS commented Nov 22, 2024

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:

~>D:
D:\>cd Test
D:\Test\>C:
~>D:
D:\Test\>C:
~>cd D:..
D:\>C:x/../y/../z/..
~>cd D:Test\Test
D:\Test\Test>C:
~>D:...
D:\>

Interaction example on auto-completion at cmd line:

~>cd D:\test[Enter]
D:\test>~[Enter]
~>D:[TAB]
~>D:\test[Enter]
D:\test>c:.c[TAB]
c:\users\nushell\.cargo\ c:\users\nushell\.config\

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)

~>cd D:\Test
D:\Test>cd E:\Test
E:\Test\>~
~>CMD
Microsoft Windows [Version 10.0.22631.4460]
(c) Microsoft Corporation. All rights reserved.

C:\Users\Nushell>d:
D:\Test>e:
E:\Test>

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:

  1. Modified 'crates/nu-command/src/filesystem/cd.rs', 1 line change to use stackscoped PWD-per-drive.
    Other commands, commit pending....

Local test def --env OK:

E:\study\nushell> def --env env_cd_demo [] {                 
:::     cd ~
:::     cd D:\Project
:::     cd E:Crates
::: }
E:\study\nushell>                                                   
E:\study\nushell> def cd_no_demo [] {                   
:::     cd ~
:::     cd D:\Project
:::     cd E:Crates
::: }
E:\study\nushell> cd_no_demo                                 
E:\study\nushell> C:
C:\>D:
D:\>E:                                     
E:\study\nushell>env_cd_demo
E:\study\nushell\crates> C:
~>D:
D:\Project>E:                                     
E:\study\nushell\crates>     

Tests + Formatting

  • cargo fmt --all -- --check passed.
  • cargo clippy --workspace -- -D warnings -D clippy::unwrap_used passed.
  • cargo test --workspace passed on Windows developer mode and Ubuntu.
  • cargo run -- -c "use toolkit.nu; toolkit test stdlib" passed.
  • nushell:
> use toolkit.nu  # or use an `env_change` hook to activate it automatically
> toolkit check pr
> ```
passed

@sholderbach
Copy link
Member

Just for understanding, which commands beyond cd do you think need changes?

@PegasusPlusUS
Copy link
Contributor Author

PegasusPlusUS commented Nov 26, 2024

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(...).

@PegasusPlusUS
Copy link
Contributor Author

PegasusPlusUS commented Nov 27, 2024

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(...).

@fdncred
Copy link
Contributor

fdncred commented Nov 27, 2024

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
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@PegasusPlusUS
Copy link
Contributor Author

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!

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.

@fdncred fdncred merged commit c8b5909 into nushell:main Dec 2, 2024
13 checks passed
@github-actions github-actions bot added this to the v0.101.0 milestone Dec 2, 2024
@fdncred
Copy link
Contributor

fdncred commented Dec 2, 2024

Let's move forward with this and see how we like it. Thanks for all the effort and working with us on changes @PegasusPlusUS!

@PegasusPlusUS
Copy link
Contributor Author

OK, continue on other file system commands…

@fdncred
Copy link
Contributor

fdncred commented Dec 2, 2024

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.

@fdncred
Copy link
Contributor

fdncred commented Dec 3, 2024

@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.

@kubouch
Copy link
Contributor

kubouch commented Dec 3, 2024

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:

C:\Users\kubouch\> module foo { export-env { cd C:\Users } }
C:\Users\kubouch\> overlay use foo  # changes dir to C:\Users
C:\Users\> overlay hide foo  # hides the overlay, changes back to the original directory
C:\Users\kubouch> cd D:
D:\> cd C:
C:\Users> # should be C:\Users\kubouch, but it loads C:\Users from the hidden overlay 
          # because that's the last one stored in the env map

@PegasusPlusUS
Copy link
Contributor Author

PegasusPlusUS commented Dec 4, 2024

OK, I'm working on other commands, and I'll check for the edge cases

@fdncred fdncred added A:file-system Related to commands and core nushell behavior around the file system platform:windows Issues which are specific to Windows notes:mention Include the release notes summary in the "Hall of Fame" section labels Dec 6, 2024
fdncred added a commit that referenced this pull request Dec 16, 2024
@hustcer hustcer removed this from the v0.101.0 milestone Dec 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A:file-system Related to commands and core nushell behavior around the file system notes:mention Include the release notes summary in the "Hall of Fame" section platform:windows Issues which are specific to Windows

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants