do we have to have 5 mods in pwd_per_drive.rs (excluding tests)? it's kind of hard to read.

Just split into several mods to improve understanding of the implementation before initiated PR, :(. OK, rollback to flatten the code without extra mods.

CI test for windows not very stable, currently these 3 failed:

failures:
commands::network::http::get::http_get_with_accept_errors_and_full_raw_response
commands::network::http::head::http_head_failed_due_to_server_error
commands::network::http::patch::http_patch_failed_due_to_server_error

Checked these tests, they're using stack variable, no global or shared, separately test all OK, do not understand what happened in CI test.

Sometimes other network commands fail, sometimes one or more tests for PWD-per-drive failed at SUBST not succeed etc. Usually at least one test will fail, maybe the CI environment needs upgrade to larger memory? :).

I've retried the windows test twice now and it still fails. I'm still not sure we'll move forward with this though.

I've retried the windows test twice now and it still fails. I'm still not sure we'll move forward with this though.

These network commands are not PWD-per-drive related, just recently some commands can't pass CI, seems somewhat randomly.

I've retried the windows test twice now and it still fails. I'm still not sure we'll move forward with this though.

The 1700+ lines of changes may seem very complex, but actually:

1. There is a bias in GitHub's statistics, as a single touch.rs file move from one directory to another contributes 654 lines, that's nothing about real change.
2. Among the changes are about ten filesystem commands, each with just test cases spanning dozens of lines, and half of the remaining 1000 lines are test cases for these commands.
3. Excluding parts unrelated to PWD but incidentally improved, such as auto_cd support for the Playground and refactoring of management of managed env variables, the actual implementation of the PWD functionality is less than 100 lines.

Since around 2024-12-15, I’ve primarily been writing tests, especially for the watch feature. Because Nushell doesn’t support background tasks, I tried numerous approaches, repeatedly encountering failures (and the output for nu!() macro is not friendly for debugging, have to manually proceeded line by line to isolate problems). Eventually, I solved the issue, wrapping up the PWD-related work.

So, in my view, this version of the PWD implementation is not complicated at all. It uses the existing layered env variable mechanism of the stack/engine_state to store and retrieve PWD information, naturally coexisting in harmony with overlays and export-env modules. Now, the refactoring and testing of filesystem commands have been fully completed. The reason CI cannot pass smoothly does not appear to be related to the test code itself; it might be due to insufficient resources in the CI environment as sometime PWD commands all succeed but some network commands randomly fail in CI at the same time. Hope after resolving CI unstable problem, PWD related work can be online soon, :).

It's concerning to me that the HTTP tests are specifically failing on Windows, since this PR specifically touches Windows. Those tests don't seem to be failing for any other recent commit CI runs, and they pass on this PR for Linux and macOS. We've disabled all of the tests which make actual outbound network connections, all of the HTTP remaining tests are mocked, so it's not an network issue. I know you didn't touch the HTTP code, but maybe some change in this PR changes the way the mock is set up, or some detail of how Nushell interacts with the mock, breaking those tests?

It sounds like based on @fdncred's comment that these tests are failing locally, not just in CI (unless he meant that he re-ran the CI tests). Have you tried running the tests locally? You can run them locally by running this in your nushell folder:

use toolkit.nu
toolkit check pr

It's concerning to me that the HTTP tests are specifically failing on Windows, since this PR specifically touches Windows. Those tests don't seem to be failing for any other recent commit CI runs, and they pass on this PR for Linux and macOS. We've disabled all of the tests which make actual outbound network connections, all of the HTTP remaining tests are mocked, so it's not an network issue. I know you didn't touch the HTTP code, but maybe some change in this PR changes the way the mock is set up, or some detail of how Nushell interacts with the mock, breaking those tests?

It sounds like based on @fdncred's comment that these tests are failing locally, not just in CI. Have you tried running the tests locally? You can run them locally by running this in your nushell folder:

use toolkit.nu
toolkit check pr

Already locally run 'cargo test --workspace', and run separately in IDE, all can pass.

It's concerning to me that the HTTP tests are specifically failing on Windows, since this PR specifically touches Windows. Those tests don't seem to be failing for any other recent commit CI runs, and they pass on this PR for Linux and macOS. We've disabled all of the tests which make actual outbound network connections, all of the HTTP remaining tests are mocked, so it's not an network issue. I know you didn't touch the HTTP code, but maybe some change in this PR changes the way the mock is set up, or some detail of how Nushell interacts with the mock, breaking those tests?

It sounds like based on @fdncred's comment that these tests are failing locally, not just in CI (unless he meant that he re-ran the CI tests). Have you tried running the tests locally? You can run them locally by running this in your nushell folder:

use toolkit.nu
toolkit check pr

Recently I added test case for 'watch', the test case also runs on Ubuntu and macOS, at first, tests passed at my local macOS and Ubuntu, however, they all fail in CI. Finally I found watch in nushell is sensitive on different events at macOS and Ubuntu, and CI machine may be resource constraint, it often misses some events. Then I modify the test case to repeat create/delete after a while. After that, CI test for 'watch' on all platform passed.

CI test for windows not very stable, currently these 3 failed:

failures: commands::network::http::get::http_get_with_accept_errors_and_full_raw_response commands::network::http::head::http_head_failed_due_to_server_error commands::network::http::patch::http_patch_failed_due_to_server_error

Checked these tests, they're using stack variable, no global or shared, separately test all OK, do not understand what happened in CI test.

Sometimes other network commands fail, sometimes one or more tests for PWD-per-drive failed at SUBST not succeed etc. Usually at least one test will fail, maybe the CI environment needs upgrade to larger memory? :).

This time these 3 test cases failed in CI:
commands::network::http::get::http_get_with_accept_errors_and_full_raw_response
commands::network::http::get::http_get_with_unknown_mime_type
commands::network::http::patch::http_patch_failed_due_to_server_error
the first is the same, and the other 2 are not the same with last sample. So, it seems there might be interfere between some test cases in resource constraint environment.

I see, I might understand why there're randomly testcases that will fail in CI, that might be the test of 'watch' that interfere some other test cases happened to run on other threads at the same time. Let me find and update a solution to test again.

Fixed, previously the powershell script kills watch process (since that process will never exit itself), and it used name to kill process, which will also kill the nu.exe in test cases that runs at other threads that uses nu!() to execute commands.

@PegasusPlusUS Is this PR ready for review then? If so I will try to take a look in the next couple of days

@PegasusPlusUS Is this PR ready for review then? If so I will try to take a look in the next couple of days

All done for review, half of the time spent on 'watch', :).

@PegasusPlusUS Is this PR ready for review then? If so I will try to take a look in the next couple of days

In GitHub review, there’s a touch.rs in nu-command/tests/command/filesystem, it marks the whole file 654 lines new, indeed it was moved from ../, only last test is new added, other part does not need spend time to review.

I'd like to apologize for the lack of feedback over the last week. I definitely appreciate both your contribution, as well as your thorough explanation of the changes, and I'm still working through them myself.

The team has been discussing this PR, and the general agreement is that it creates (at least some) additional maintenance burden for what might not be a common use-case. When future filesystem commands are contributed, it seems likely that it might miss this Windows nuance. It's especially likely that the contributor won't remember to add tests for this feature with the new command.

It seems that even now, some commands may have been overlooked? For example, I could be wrong, but it appears that path expand isn't currently on the list of commands that were updated here. I just tested 'C:' | path expand, and it's always returning C:\, even when there's different PWD on C:.

Side-note: I would recommend holding off on implementing this, though, until we reach a final decision.

Even the Windows users among us haven't commonly utilized per-drive-PWD in PowerShell or CMD in the past. At the same time, we do want Windows users to have a "first class" experience on Nushell, and we're not opposed to adding new OS-specific features when it makes sense.

At the request of the team, I've created a poll in the Discussions area here on GitHub in order to try to understand how commonly used this feature would be.

I'd like to apologize for the lack of feedback over the last week. I definitely appreciate both your contribution, as well as your thorough explanation of the changes, and I'm still working through them myself.

The team has been discussing this PR, and the general agreement is that it creates (at least some) additional maintenance burden for what might not be a common use-case. When future filesystem commands are contributed, it seems likely that it might miss this Windows nuance. It's especially likely that the contributor won't remember to add tests for this feature with the new command.

It seems that even now, some commands may have been overlooked? For example, I could be wrong, but it appears that path expand isn't currently on the list of commands that were updated here. I just tested 'C:' | path expand, and it's always returning C:\, even when there's different PWD on C:.

Side-note: I would recommend holding off on implementing this, though, until we reach a final decision.

Even the Windows users among us haven't commonly utilized per-drive-PWD in PowerShell or CMD in the past. At the same time, we do want Windows users to have a "first class" experience on Nushell, and we're not opposed to adding new OS-specific features when it makes sense.

At the request of the team, I've created a poll in the Discussions area here on GitHub in order to try to understand how commonly used this feature would be.

Yes, I just patched commands in filesystem\ subdir, there might be other commands that should be patched to fully support PWD, maybe I'd better to search for the usage of nu_path::expand_path_with() to find all commands that should support PWD. I guess if adding a signature for nu_path::expand_path_with() as deprecated, that might help newly created commands directly support PWD, and chances of missing test case to verify supporting of PWD can also be reduced.

OK, just wait guidance for additional action, :). If PWD-per-drive not used, I'll extract updates for test of 'watch' and some other improvements not directly related to PWD-per-drive to issue other small PRs.

I'd like to apologize for the lack of feedback over the last week. I definitely appreciate both your contribution, as well as your thorough explanation of the changes, and I'm still working through them myself.

The team has been discussing this PR, and the general agreement is that it creates (at least some) additional maintenance burden for what might not be a common use-case. When future filesystem commands are contributed, it seems likely that it might miss this Windows nuance. It's especially likely that the contributor won't remember to add tests for this feature with the new command.

It seems that even now, some commands may have been overlooked? For example, I could be wrong, but it appears that path expand isn't currently on the list of commands that were updated here. I just tested 'C:' | path expand, and it's always returning C:\, even when there's different PWD on C:.

Side-note: I would recommend holding off on implementing this, though, until we reach a final decision.

Even the Windows users among us haven't commonly utilized per-drive-PWD in PowerShell or CMD in the past. At the same time, we do want Windows users to have a "first class" experience on Nushell, and we're not opposed to adding new OS-specific features when it makes sense.

At the request of the team, I've created a poll in the Discussions area here on GitHub in order to try to understand how commonly used this feature would be.

Will respect decision, but really can't agree with the conclusion of 'creates additional maintenance burden', if some new commands forget to support PWD-per-drive, (while the possibility is relatively low after nu_path::expand_path_with() marked as deprecated), why not try to improve the new commands, but rather let all commands discard support of PWD-per-drive? If supporting PWD-per-drive is optional not important, then it's okay for the new command not to support it. It wouldn't really count as increasing the maintenance difficulty, would it?