fix(directory): avoid confusing modules with PowerShell paths

[jeanga]

Description

Powershell supports PSDrives (https://docs.microsoft.com/en-us/powershell/module/microsoft.powershell.management/new-psdrive?view=powershell-7) that allows to create "logical" drives mapped to actual Windows drives.

Motivation and Context

When using PSDrives, you can use paths that are related to a logical drive.
For example, test: can be "mapped" to c:\test folder.
You can then use "cd test:" to change the current location.

Many other drives exist be default (env, function, variable etc...).

These "Powershell" paths confuse starship modules that fail to return the correct result.

Changing the --path argument to a Windows path (instead of the Powershell one) fixes the issue.

In our example, with the fix, when "cd test:" is used, starship is called with --path C:\test (instead of test:).

Closes:

Types of changes

This change introduces a call to Convert-Path in startship.ps1 to convert the variable $PWD (a "PowerShell" path to a Windows path. This change avoids modules (like Rust, Git, ...) being confused by a PowerShell path.

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

Screenshots (if appropriate):

How Has This Been Tested?

Created a PSDrive:
New-PSDrive -Scope 1 -Name test -PSProvider FileSystem -root c:\test -Description "PSDrive test with starship"
Checked basic modules (Rust&Git) are now working properly.

• I have tested using MacOS
• I have tested using Linux
• I have tested using Windows
• I have tested using Windows Subsystem for Linux

Checklist:

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

[jimleroyer]

Being a PowerShell user myself, I gave this a spin locally. It does improve the behavior with PSDrive. Some examples (would have been nice to have these SS submitted with the PR -- before / after images of behavior):

The root is properly displayed within a PSDrive:

This is what's the best about this fix to me though. We can see that the master build breaks on the information display when we are in git repository with modules that should be detected (Rust+git in this case):

Looks good to me! 👏

[andytom]

I don't use Windows so can't test this but in general this LGTM, there are a few little fixes that I think you need to make, running cargo fmt should fix the broken formatting (https://github.com/starship/starship/pull/1114/checks?check_run_id=595902950) and it looks like there is another test that needs to be updated (

starship/tests/testsuite/directory.rs

Lines 127 to 138 in d7acffa

	#[test]
	#[cfg(target_os = "windows")]
	fn directory_in_root() -> io::Result<()> {
	let output = common::render_module("directory")
	.arg("--path=C:\\")
	.output()?;
	let actual = String::from_utf8(output.stdout).unwrap();
	let expected = format!("in {} ", Color::Cyan.bold().paint("/c"));
	assert_eq!(expected, actual);
	Ok(())
	}

).

[andytom]

Also I'm not sure but it looks like this will also fix #1106, is this correct?

[jeanga]

Sorry for the formatting errors & test failures ("cargo test" did not fail on my machine for a reason I still ignore).
My only excuse is to be new to Rust&Starship.
Thank you for your understanding!

[andytom]

Sorry for the formatting errors & test failures ("cargo test" did not fail on my machine for a reason I still ignore).
My only excuse is to be new to Rust&Starship.
Thank you for your understanding!

Not a problem, we have a few tests that are ignored locally because they depend on the machine running the tests being setup in a particular way (e.g specific versions of an application being installed or some particular files existing).

@jimleroyer and @ylor can you have a quick look at this to confirm it works as expected and I'll get this merged in for the next release.

[jimleroyer]

I don't think it works as expected yet. The root folder works fine by displaying C:/ but getting into a folder shows double forward slashes, which some of the old code prevented to do (and now does again).

I would suggest a) to remove the double forward slashes as it seems like a regression and b) to add a test that triggers this behavior by getting into a folder after a root, so we are sure it does not repeat again.

I can help with that if @jeanga wants me to submit a PR fixing that on his repo.

[davidkna]

I don't think it works as expected yet. The root folder works fine by displaying C:/ but getting into a folder shows double forward slashes, which some of the old code prevented to do (and now does again).

I would suggest a) to remove the double forward slashes as it seems like a regression and b) to add a test that triggers this behavior by getting into a folder after a root, so we are sure it does not repeat again.

I can help with that if @jeanga wants me to submit a PR fixing that on his repo.

The C:\ to /c is broken anyway because it only works with C:\. e.g. D://foo is broken. This brokenness is at least consistent.

Getting rid of the // should be possible by replacing path_slash
e.g. with

use std::path::Path;

fn to_slash(path: &Path) -> Option<String> {
    assert!(path.is_absolute());

    let size = path.as_os_str().len();
    let mut out = String::with_capacity(size);

    let mut iterator = path.iter();

    // Don't append / before root directory
    out.push_str(&iterator.next()?.to_string_lossy());
    
    // skip prevents double //
    for component in iterator.skip(1) {
        out.push('/');
        out.push_str(&component.to_string_lossy());
    }

    Some(out)
}

Obviously windows-only.

[jeanga]

Hi davidkna,

I noticed that behavior but I do not think it is a regression of this PR.
It was already behaving this way before.

Are you suggesting I add you to the members of my forked repo?
If so, I can certainly do that :-)

Jean

[davidkna]

Hi davidkna,

I noticed that behavior but I do not think it is a regression of this PR.
It was already behaving this way before.

Are you suggesting I add you to the members of my forked repo?
If so, I can certainly do that :-)

Jean

The removal of replace_c_dir() makes this something of a regression, but I'm not sure that if this change would be better of in a seperate PR.

[jeanga]

Yes, but the behavior showed with anything not c:\ like d:\ ... z:\ :-)

Jean

[ylor]

I don't think it works as expected yet. The root folder works fine by displaying C:/ but getting into a folder shows double forward slashes, which some of the old code prevented to do (and now does again).

I would suggest a) to remove the double forward slashes as it seems like a regression and b) to add a test that triggers this behavior by getting into a folder after a root, so we are sure it does not repeat again.

I can help with that if @jeanga wants me to submit a PR fixing that on his repo.

And the slashes are still going the wrong way. Not sure if this PR is for solving that though. Please see the behavior I expect when working in Powershell (that is to say not WSL).

[jeanga]

This PR tries to be as close as the previous behavior as possible while fixing annoying bugs for Windows Powershell users.
As previously stated, the double slash (D://Windows) and replacing the backslash with a slash was the previous way it behaved.
If there is a consensus on preserving \ and removing these double //, I'll be happy to submit a fix in another PR.

[ylor]

@jeanga Thanks for your work.

[andytom]

@jeanga thank you for your contribution and thank you to everyone else for your help testing this. I've left #1106 open and maybe that would be a good place to start discussing how to display paths in Powershell on Windows.