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.

Sign up for GitHub

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

Jump to bottom

feat: implement physical and logical cwd #219

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

feat: implement physical and logical cwd #219

wants to merge 1 commit into 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.

@39555 39555 force-pushed the cd-physical-logical branch from 9262eb8 to 88cbdb7 Compare October 20, 2024 14:54
@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 20, 2024 •
edited
Loading

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
@reubeno reubeno requested a review from Copilot December 14, 2024 06:35
Copilot
Copilot AI reviewed Dec 14, 2024
View reviewed changes

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 6 out of 17 changed files in this pull request and generated no comments.

Files not reviewed (11)
  • brush-shell/tests/completion_tests.rs: Evaluated as low risk
  • brush-shell/tests/cases/builtins/pwd.yaml: Evaluated as low risk
  • brush-core/src/patterns.rs: Evaluated as low risk
  • brush-core/src/prompt.rs: Evaluated as low risk
  • brush-interactive/src/completion.rs: Evaluated as low risk
  • brush-core/src/interp.rs: Evaluated as low risk
  • brush-core/src/builtins/cd.rs: Evaluated as low risk
  • brush-core/src/commands.rs: Evaluated as low risk
  • brush-core/src/builtins/pushd.rs: Evaluated as low risk
  • brush-core/src/completion.rs: Evaluated as low risk
  • brush-core/src/builtins/popd.rs: Evaluated as low risk
Comments suppressed due to low confidence (3)

brush-core/src/builtins/pwd.rs:9

  • [nitpick] The variable name 'physical' is ambiguous. It should be renamed to 'print_physical_directory'.
#[arg(short = 'P', overrides_with = "allow_symlinks")]

brush-core/src/builtins/pwd.rs:13

  • [nitpick] The variable name 'allow_symlinks' is ambiguous. It should be renamed to 'print_logical_directory'.
#[arg(short = 'L', overrides_with = "physical")]

brush-core/src/builtins/pwd.rs:36

  • The error message for unimplemented flags should be clear and helpful. Consider updating it to 'Error: The -P and -L flags are not yet implemented.'
writeln!(context.stdout(), "{}", cwd.display())?;
@reubeno
Copy link
Owner

reubeno commented Jan 23, 2025

My apologies for having put this on hold for so long, @39555 --thanks again for putting so much hard work into this and being patient while we addressed other areas of the project. We've since cleared out a number of the most commonly hit gaps in brush and I'd like to be able to get through the backlog of other good changes -- this one included.

I recall you mentioning in another issue that you may not have bandwidth for contributions right now. Is that the case? If so, would you be open to me helping rebase this against latest changes and shepherd it through? I'm also okay waiting if you'd prefer to come back to this yourself at some point in the future.

@39555
Copy link
Contributor Author

39555 commented Jan 23, 2025

Hi @reubeno ! Yes it would be great if you could help me finalize this feature. Big thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

Copilot code review Copilot Copilot left review comments

Assignees
No one assigned
Labels
needs_review PR or issue author is waiting for a review
Projects
None yet
Milestone
No milestone
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
@39555 @reubeno