Skip to content
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

feat: implement physical and logical cwd #219

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

39555
Copy link
Contributor

@39555 39555 commented Oct 20, 2024

After diving into the system's paths rabbit hole for a while, I've implemented cross-platform cwd handling for the shell, along with support for cd -L -P and pwd -L -P.

Closes: #202

With this PR, the shell now maintains the logical current working directory alongside with the physical one. To correctly create an absolute path and expand all .. and . from the logical path, the functions have been added to the brush_core::sys::fs crate:

  • expand_tilde_with_home - expands the tilde using the given home directory
  • make_absolute - makes the path absolute based on the given base path.
  • normalize_lexically - performs canonicalization without touching the filesystem.

Posix notes:

There is a special handling for paths with double slashes, such as //users/home

If a pathname begins with two successive characters, the first component following the leading characters may be interpreted in an implementation-defined manner, although more than two leading characters shall be treated as a single character.

See: https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap04.html#tag_04_13

Windows notes

There is a lot of special handling for Windows weird path formats:

  • UNC \\server\share\my_dir.
  • Verbatim paths \\?\C$\super_path, this paths must not be normalized.
  • Device namespace \\.\Pipe and UNC with the device namespace \\.\UNC\.
  • volume relative C:iamrelative.

See: https://learn.microsoft.com/en-us/dotnet/standard/io/file-path-formats
See: The book of windows file path: https://chrisdenton.github.io/omnipath/

I've written a lot of tests, trying to cover all the special cases.

Copy link

github-actions bot commented Oct 20, 2024

Performance Benchmark Report

Benchmark name Baseline (μs) Test/PR (μs) Delta (μs) Delta %
expand_one_string 3.42 μs 3.57 μs 0.15 μs 🟠 +4.27%
instantiate_shell 59.72 μs 70.61 μs 10.89 μs 🟠 +18.24%
instantiate_shell_with_init_scripts 31137.02 μs 30989.98 μs -147.04 μs ⚪ Unchanged
parse_bash_completion 2829.91 μs 2702.85 μs -127.05 μs ⚪ Unchanged
parse_sample_script 4.31 μs 4.16 μs -0.15 μs 🟢 -3.48%
run_echo_builtin_command 90.77 μs 102.49 μs 11.72 μs 🟠 +12.91%
run_one_builtin_command 109.48 μs 122.58 μs 13.10 μs 🟠 +11.97%
run_one_external_command 1991.67 μs 1956.48 μs -35.19 μs 🟢 -1.77%
run_one_external_command_directly 1017.00 μs 1017.80 μs 0.80 μs ⚪ Unchanged

Code Coverage Report: Only Changed Files listed

Package Base Coverage New Coverage Difference
brush-core/src/builtins/cd.rs 🟢 81.82% 🟢 82.61% 🟢 0.79%
brush-core/src/builtins/dirs.rs 🟢 85.71% 🟢 88.1% 🟢 2.39%
brush-core/src/builtins/pwd.rs 🟢 80.95% 🟢 83.33% 🟢 2.38%
brush-core/src/interp.rs 🟢 90.02% 🟢 90.01% 🔴 -0.01%
brush-core/src/patterns.rs 🟢 97.77% 🟢 97.78% 🟢 0.01%
brush-core/src/shell.rs 🟢 77.97% 🟢 80.03% 🟢 2.06%
brush-core/src/sys/fs.rs 🔴 0% 🟢 97.04% 🟢 97.04%
brush-shell/src/main.rs 🟢 90.2% 🟢 90.85% 🟢 0.65%
Overall Coverage 🟢 73.41% 🟢 74.13% 🟢 0.72%

Minimum allowed coverage is 70%, this run produced 74.13%

@reubeno
Copy link
Owner

reubeno commented Oct 21, 2024

Thanks for diving deep into this; I'll need a bit of time to review the changes here. Is there a reasonable way to break this up into multiple, smaller changes?

@39555
Copy link
Contributor Author

39555 commented Oct 21, 2024

I don't think so, maybe cd and pwd, but they just call the new shell api Shell.set_current_working_dir_from_logical.

All changes are based on the new Cwd type instead of PathBuf for Shell.working_dir. Cwd contains both a physical PathBuf and a logical PathBuf. When we set a new cwd with Shell.set_current_working_dir_from_logical, the path should be normalized but without using syscalls (for preserving symlinks):

  1. expand tilde
  2. make it absolute based on our current physical or logical cwd
  3. expand dots

For startup cwd Inside Shell::new, we try to get the logical path from the PWD environment variable if it is present.

In other changed files, such as dirs or interp, I fixed the usage of working_dir to use the physical path for now. This should be addressed in other PRs if completion needs the logical path instead.

@reubeno reubeno added the needs_review PR or issue author is waiting for a review label Oct 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs_review PR or issue author is waiting for a review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

correct implementation of the physical -P and logical -L directories in cd, pwd and dirs
2 participants