refactor(directory): Introduce `logical-path` argument which allows a shell to explicitly specify both a logical and physical filesystem path #2104

deadalusai · 2021-01-09T03:59:29Z

Description

Introduce logical-path argument which allows a shell to explicitly specify both a logical and physical filesystem path.

Fix directory::module to consume both path and logical-path (if provided). The "logical" path is preferred when rendering the "display path", while the "physical" path is used to resolve the "read only" flag. Repo- and home-directory contraction is based on the logical path if it is set, or the physical path if it is not.

The custom "get_current_dir" logic has been removed entirely, and the directory module now relies on context.current_dir / context.logical_dir directly.

Changes have been made to init/starship.ps1 to work with this new flag:

Calculate and pass "physical" and "logical" paths explicitly (as other shells do not pass --logical-path they always rendering the based on the physical path)
Moved the "powershell provider prefix" cleanup code to the PowerShell prompt script itself - this code should now support any kind of powershell path prefix.

Motivation and Context

I discovered this issue while testing starship with the Nu shell. It's a WIP shell, so still has some bugs. I switched to passing --path to work around one such issue and found that starship was inconsistent with how it handled the argument.

This screenshot demonstrates the problem:

Specifically, invoking the command starship prompt --path C:\Windows:

When called with no PWD environment variable, prints C:/Windows {lock symbol} ✔️
When called with a PWD environment variable, prints {the PWD env variable} {lock symbol} ❌

The issue is caused by the directory module function get_current_dir which looks to the PWD environment variable to select the path it is going to display to the user, overriding the provided --path argument.

To resolve this, I've introduced a new prompt argument --logical-path - rather than setting the PWD environment variable, a shell with "logical" path display requirements can instead provide --logical-path to set the logical path.

The same command as above, but with the fix applied:

See discussion on #2095 for more context.
This (abandoned?) PR #1209 also looked to introduce --logical-path as an alternative to PWD.

How Has This Been Tested?

I have tested using MacOS
I have tested using Linux
I have tested using Windows

I tested manually using the following scripts:

Windows/PowerShell:

function test ($Path, $LogicalPath) {
    "Testing: starship prompt --path '$Path' --logical-path '$LogicalPath'"
    target\debug\starship.exe prompt --path $Path --logical-path $LogicalPath
    "`n"
}

New-PSDrive -Name Sys32 -PSProvider FileSystem -Root 'C:\Windows\System32' -ErrorAction SilentlyContinue | Out-Null
New-PSDrive -Name Dev -PSProvider FileSystem -Root 'C:\Users\Ben Fox\Dev' -ErrorAction SilentlyContinue | Out-Null

test 'C:\Users\Ben Fox\Dev\starship' 'C:\Users\Ben Fox\Dev\starship'
test 'C:\Users\Ben Fox\Dev\starship' 'Dev:\starship'
test 'C:\Users\Ben Fox\Dev\starship' 'whatever\basically'
test 'C:\Windows\System32\' 'C:\Windows\System32\'
test 'C:\Windows\System32\' 'Sys32:\'
test 'HKEY_LOCAL_MACHINE\SOFTWARE' 'HKLM:\SOFTWARE'

(results)

Linux/bash:

function test() {
    echo Testing: starship prompt --path "$1" --logical-path "$2"
    target/debug/starship prompt --path "$1" --logical-path "$2"
    echo
}

test '~/Dev/starship' '~/Dev/starship'
test '~/Dev/starship' 'logical/path'
test '/' '/'
test '/' 'logical/path'

(results)

Checklist:

I have updated the documentation accordingly.
I have updated the tests accordingly.

… shell to explicitly specify both a logical and physical filesystem path Fix `directory::module` to consume both path and logical-path (if provided). The "logical" path is preferred when rendering the "display path", while the "physical" path is used to resolve the "read only" flag. Repo- and home-directory contraction behavior is maintained, based on the logical path if it is set, or the physical path if it is not. The custom "get_current_dir" logic has been removed entirely, and the `directory` module now relies on `context.current_dir` / `context.logical_dir` entirely. Changes have been made to `init/starship.ps1` to work with this new flag: - Calculate and pass "physical" and "logical" paths explicitly (as other shells do not pass `--logical-path` that they fall back to rendering the physical path) - Moved the "powershell provider prefix" cleanup code to the PowerShell script - this code _should_ now support any kind of powershell path prefix.

deadalusai · 2021-01-09T04:15:04Z

Something I noted during testing is that navigating to a repository directory with a non-physical logical directory "breaks" the repo truncation behavior:

I don't think this is a regression, but something we could improve.

Edit:
A small change produces this result

diff --git a/src/modules/directory.rs b/src/modules/directory.rs
index 85da1c3..80a9a82 100644
--- a/src/modules/directory.rs
+++ b/src/modules/directory.rs
@@ -56,7 +56,7 @@ pub fn module<'a>(context: &'a Context) -> Option<Module<'a>> {
     let dir_string = repo
         .and_then(|r| r.root.as_ref())
         .filter(|root| *root != &home_dir)
-        .and_then(|root| contract_repo_path(&logical_dir, root))
+        .and_then(|root| contract_repo_path(&physical_dir, root))
         .unwrap_or_else(|| contract_path(&logical_dir, &home_dir, HOME_SYMBOL));

     log::debug!("Dir string: {}", dir_string);

… causing command line parsing issues. This is a bit of a footgun! The work-around chosen is to append a trailing space when a path string ends with a backslash, and then trim any extra whitespace away in the Context constructor. Other alternatives considered and rejected: 1. Always trim trailing backslashes as the filesystem generally doesn't need them. 2. Escape trailing backslashes with another backslash. This proved complex as PS only quotes string args when the string includes some whitespace, and other backslashes within the string apparently don't need to be escaped.

src/modules/directory.rs

src/configs/directory.rs

src/init/starship.ps1

davidkna · 2021-01-09T11:20:07Z

Something I noted during testing is that navigating to a repository directory with a non-physical logical directory "breaks" the repo truncation behavior:

I don't think this is a regression, but something we could improve.

Edit:
A small change produces this result

diff --git a/src/modules/directory.rs b/src/modules/directory.rs
index 85da1c3..80a9a82 100644
--- a/src/modules/directory.rs
+++ b/src/modules/directory.rs
@@ -56,7 +56,7 @@ pub fn module<'a>(context: &'a Context) -> Option<Module<'a>> {
     let dir_string = repo
         .and_then(|r| r.root.as_ref())
         .filter(|root| *root != &home_dir)
-        .and_then(|root| contract_repo_path(&logical_dir, root))
+        .and_then(|root| contract_repo_path(&physical_dir, root))
         .unwrap_or_else(|| contract_path(&logical_dir, &home_dir, HOME_SYMBOL));

     log::debug!("Dir string: {}", dir_string);

But this could cause unexpected behavious outside powershell (symlinks etc). Maybe a third --display-path option is needed after all.

deadalusai · 2021-01-10T02:16:07Z

But this could cause unexpected behavious outside powershell (symlinks etc).

Other shells do not pass --logical-path at all, which means that contract_repo_path will always be called against the physical path anyway.

… executables with strings which may contain characters which need to be escaped carefully.

These were in place to clean up extra whitespace sometimes injected by starship.ps1::prompt, and are no longer required with the new Invoke-Native helper in place.

davidkna · 2021-01-10T16:15:56Z

In traditional shells:

The physical path is the path on disk with all symlinks resolved. (std::env::current_dir, std::fs::canonicalize($PWD))
The logical path is the path without any symlinks resolved. If you cd into a symlinked directory, the parent hierachy stays the same. ($PWD)

You can't just remove the logical handling for the traditional shells.

I think something like this might work for logical_dir:

--logical-path (if use_logical_path is false std::fs::canonicalize)
--path (if use_logical_path is false std::fs::canonicalize)
if use_logical_path $PWD else std::env::current_dir()
fall back to current_dir, (if use_logical_path is false std::fs::canonicalize)

deadalusai · 2021-01-10T23:10:28Z

In traditional shells:

The physical path is the path on disk with all symlinks resolved. (std::env::current_dir, std::fs::canonicalize($PWD))
The logical path is the path without any symlinks resolved. If you cd into a symlinked directory, the parent hierachy stays the same. ($PWD)

You can't just remove the logical handling for the traditional shells.

Ok, I think I understand what you're getting at here now.

It seems like we should aim to standardize what data current_dir and logical_dir hold, and populate them explicitly as part of the context population step. Would the following work?

current_dir - holds the phyisical current directory.
Initialised from --path, $PWD or std::env::current_dir and canonicalized (if it is possible to canonicalize).
logical_dir - holds the logical current directory.
Initialised from --logical-path, $PWD, or std::env::current_dir and not canonicalized.

use_logical_path would just select between those two values.

davidkna · 2021-01-11T14:12:36Z

current_dir - holds the phyisical current directory.
Initialised from --path, $PWD or std::env::current_dir and canonicalized (if it is possible to canonicalize).

That would work, but current_dir doesn't really need to be canonicalized unless use_logical_dirs is false, because it only matters in the directory module. (std::env::current_dir is always canonicalized as far as I know.) Not sure how expensive canonicalize is.

logical_dir - holds the logical current directory.
Initialised from --logical-path, $PWD, or std::env::current_dir and not canonicalized.

I think part of the aim for this pr for logical_dir to prefer --path unless --logical-path is also given? Do you intend to keep that behaviour?

deadalusai · 2021-01-11T22:17:41Z

That would work, but current_dir doesn't really need to be canonicalized unless use_logical_dirs is false, because it only matters in the directory module. (std::env::current_dir is always canonicalized as far as I know.) Not sure how expensive canonicalize is.

Looking at the existing code, I can only see one call to canonicalize as part of real_path which is used by contract_repo_path. I haven't touched any of that code - I'm not sure we need extra canonicalize calls here?

I think part of the aim for this pr for logical_dir to prefer --path unless --logical-path is also given? Do you intend to keep that behaviour?

My initial goal at least was just to make it possible to override the logical path from the command line and the addition of --logical-path solves that issue. But I think you're right in that it might fit the principal of least surprise for --path to imply --logical-path by default as that's definitely what confused me initially.

…ng it to `current_dir` but overridable by the `--logical-dir` flag. - Restore `use_logical_path` config flag. - Always attempt to contract repo paths from the `current_dir`.

davidkna · 2021-01-11T22:57:41Z

Looking at the existing code, I can only see one call to canonicalize as part of real_path which is used by contract_repo_path. I haven't touched any of that code - I'm not sure we need extra canonicalize calls here?

With the old code std::env::current_dir would have taken care of that, unless a fallback was used, which should almost never happen. All the other path sources need calls to canonicalize unless use_logical_path is false.

My initial goal at least was just to make it possible to override the logical path from the command line and the addition of --logical-path solves that issue. But I think you're right in that it might fit the principal of least surprise for --path to imply --logical-path by default as that's definitely what confused me initially.

I think having --path imply --logical-path would be best.

deadalusai · 2021-01-11T23:18:37Z

@davidkna On further investigation it may not be safe to always canonicalize paths on Windows:

On Windows, this converts the path to use extended length path syntax

std::env::current_dir does not return extended length path syntax under Windows.

This keeps the two calls to contract_path in sync.

davidkna · 2021-01-12T12:03:16Z

@davidkna On further investigation it may not be safe to always canonicalize paths on Windows:

On Windows, this converts the path to use extended length path syntax

std::env::current_dir does not return extended length path syntax under Windows.

This might have the upside of getting starship to support long paths on windows! (I think it might be best to run that on current_dir after all) I think removing the \\?\ prefix on windows for display should work.

… when use_logical_path = false - This requires some clean-up to remove the extended-path prefix on Windows - The configured logical_dir is ignored entirely in this mode - we calculate a new logical_dir by cleaning up the physical_dir path for display. - Test coverage

deadalusai · 2021-01-12T22:56:35Z

This might have the upside of getting starship to support long paths on windows! (I think it might be best to run that on current_dir after all) I think removing the \\?\ prefix on windows for display should work.

Ok, I've implemented this now and I think it works pretty well, given how I understand what you're aiming for. Tested on Linux and Windows and added test coverage.

The use_logical_path flag now switches between these two routines for resolving the physical/logical paths:

use_logical_path = true
- Physical path: current_dir
- Logical path: logical_dir or current_dir if it's not set
use_logical_path = false
- Physical path current_dir canonicalized (which resolves symlinks)
- Logical path: current_dir canonicalized (and cleaned up when on Windows)

Example of the path canonicalization with use_logical_path = false on Windows:

And the same on Linux:

src/modules/directory.rs

…ay string later in the render process

deadalusai · 2021-01-15T00:09:03Z

@davidkna I think I've found a robust solution for dealing with with comparisons between verbatim and non-verbatim paths (verbatim is the Rust std lib's name for the Windows extended-path prefixes).

I've added two routines for doing equals and starts_with comparisons with Paths which knows how to compare verbatim and non-verbatim path prefixes. The Rust Path library is pretty robust here and does most of the heavy lifting.

If there are any other modules doing "starts with" checks on context.current_dir we would need to fix them.

deadalusai · 2021-01-15T00:48:05Z

I've raised a PR to fix path-slash: rhysd/path-slash#5

…h` with variations of verbatim and non-verbatim paths

This fixes the issue with the trailing character of some Windows paths being truncated, e.g. `\\server\share` and `C:`

deadalusai · 2021-01-15T20:27:31Z

Updated path-slash to latest. This fixes the issue with the trailing character of some Windows paths being truncated, e.g. \\server\share and C:.

src/init/starship.ps1

src/modules/utils/path.rs

src/init/starship.ps1

- Use `ProcessStartInfo` to launch native executable, replacing manual UTF8 output encoding handling - If we detect we're on PWSH6+ use the new `System.Diagnostics.ProcessStartInfo.ArgumentList` parameter, otherwise manually escape the argument string - Move `Get-Cwd` and `Invoke-Native` into the prompt function scope so that they don't leak into the user's shell scope

…handling Conflicts: - src/test/mod.rs - took local changes, ported upstream changes

davidkna

LGTM. But because this PR changes some of the fundamentals I'd prefer a second review.

…-path-handling Conflicts: - src/modules/directory.rs - Took local, ported remote changes (use context.get_home).

…l-path-handling Conflicts: - src/modules/directory.rs - ported new home_symbol handling

deadalusai · 2021-02-06T23:36:18Z

@davidkna Is there still interest in merging this?

davidkna · 2021-02-07T11:03:56Z

I agree that this should be merged soon. Unless someone finds any issues or objects to merging I think I'll merge this tomorrow.

deadalusai mentioned this pull request Jan 9, 2021

fix(directory): Remove re-implementation of "current_dir" resolution logic #2095

Closed

5 tasks

deadalusai force-pushed the master--refactor-directory-logical-path-handling branch from b5ea856 to 7ccb0c5 Compare January 9, 2021 04:04

andytom requested a review from a team January 9, 2021 09:30

davidkna requested changes Jan 9, 2021

View reviewed changes

src/modules/directory.rs Outdated Show resolved Hide resolved

src/configs/directory.rs Outdated Show resolved Hide resolved

src/init/starship.ps1 Outdated Show resolved Hide resolved

deadalusai added 2 commits January 10, 2021 18:22

fix(powershell): Use Invoke-Native pattern for safely invoking native…

bd8aac9

… executables with strings which may contain characters which need to be escaped carefully.

fix(context): Remove superfluous argument trims

4857b29

These were in place to clean up extra whitespace sometimes injected by starship.ps1::prompt, and are no longer required with the new Invoke-Native helper in place.

deadalusai force-pushed the master--refactor-directory-logical-path-handling branch from 4986a1b to 7e9a8b4 Compare January 11, 2021 22:50

refactor(directory): Clean up the semantics of logical_dir defaulti…

892ce25

…ng it to `current_dir` but overridable by the `--logical-dir` flag. - Restore `use_logical_path` config flag. - Always attempt to contract repo paths from the `current_dir`.

deadalusai force-pushed the master--refactor-directory-logical-path-handling branch from 7e9a8b4 to 892ce25 Compare January 11, 2021 22:51

fix(directory) :Use logical_dir for contracting the home directory

6e0a083

This keeps the two calls to contract_path in sync.

deadalusai force-pushed the master--refactor-directory-logical-path-handling branch from 54874f5 to 6e0a083 Compare January 11, 2021 23:23

deadalusai added 2 commits January 13, 2021 10:18

fix(directory): Remove test script

513aea8

fix(directory): Use AsRef style for passing Path arguments

56106c4

davidkna requested changes Jan 13, 2021

View reviewed changes

src/modules/directory.rs Outdated Show resolved Hide resolved

src/modules/directory.rs Outdated Show resolved Hide resolved

src/modules/directory.rs Outdated Show resolved Hide resolved

src/modules/directory.rs Outdated Show resolved Hide resolved

mickimnet mentioned this pull request Jan 13, 2021

Overview of open issues (up to #2199) #2134

Closed

fix(directory): Strip the windows extended-path prefix from the displ…

d3a7b71

…ay string later in the render process

fix(path): Add test coverage for PathExt on *nix

13627a3

fix(directory): Test coverage for contract_repo_path, `contract_pat…

0274ad7

…h` with variations of verbatim and non-verbatim paths

deadalusai force-pushed the master--refactor-directory-logical-path-handling branch from 81d5a40 to 0274ad7 Compare January 15, 2021 01:35

fix(directory): Update path-slash to latest

ab8b107

This fixes the issue with the trailing character of some Windows paths being truncated, e.g. `\\server\share` and `C:`