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(path): expand ~\ prefix on MS-Windows #28515

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

Conversation

rkitover
Copy link

@rkitover rkitover commented Apr 26, 2024

In fix_fname() for MS-Windows, expand "~\" or "~/" prefixed paths to the USERPROFILE environment variable for the user's profile directory.

@@ -1872,7 +1873,13 @@ char *fix_fname(const char *fname)
#ifdef UNIX
return FullName_save(fname, true);
#else
if (!vim_isAbsName(fname)
if (*fname == '~' && (fname[1] == '/' || fname[1] == '\\')) {
const char *profile_dir = os_getenv("USERPROFILE");
Copy link
Member

Choose a reason for hiding this comment

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

os_homedir

static char *os_homedir(void)

Copy link
Author

Choose a reason for hiding this comment

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

That function is static, how do I get homedir out of env.c?

@@ -1863,7 +1863,8 @@ int vim_FullName(const char *fname, char *buf, size_t len, bool force)
/// the root may have relative paths (like dir/../subdir) or symlinks
/// embedded, or even extra separators (//). This function addresses
/// those possibilities, returning a resolved absolute path.
/// For MS-Windows, this also expands names like "longna~1".
/// For MS-Windows, this also expands names like "longna~1" and
/// replaces "~/" or "~\" prefixes with the user profile directory.
Copy link
Member

Choose a reason for hiding this comment

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

does this function not expand ~/ on unix? doesn't seem like the right place to do this then.

Copy link
Author

Choose a reason for hiding this comment

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

On unix I believe the shell does this itself.

@justinmk
Copy link
Member

What problem is this solving? Give a specific use-case, describing the steps exactly.

@rkitover
Copy link
Author

What problem is this solving? Give a specific use-case, describing the steps exactly.

So, PowerShell on Windows recently removed the feature where arguments such as ~/.ssh/config or ~/.vimrc are expanded to ${env:USERPROFILE}/.ssh/config etc..

What this means is, in PowerShell, when you run a command such as:

nvim ~/.ssh/config

, what happens is that nvim tries to open ${PWD}/~/.ssh/config because that looks like a relative path to a literal ~ directory to it.

@rkitover rkitover force-pushed the expand-tilde-windows branch 2 times, most recently from a9121fa to 41b7810 Compare April 26, 2024 13:05
@rkitover
Copy link
Author

If you don't want to do this that's up to you, I believe this is a harmless enough change because in these cases the user is very unlikely to want to open a file in literal ~ relative directory. It's a huge annoyance for me in PowerShell because I run these commands frequently and now I have to run a command such as:

nvim (gi ~/.ssh/config)

, instead to get the file I want.

@rkitover
Copy link
Author

I don't understand the lint failure, what am I doing wrong there?

@justinmk
Copy link
Member

justinmk commented Apr 26, 2024

nvim (gi ~/.ssh/config)

nvim "+n ~/.ssh/config" is another workaround. #23901 (comment)

We might want to address this, but need to be sure that fix_fname is the right place to do it. (I haven't analyzed the codepaths involved.)

PowerShell on Windows recently removed the feature where arguments such as ~/.ssh/config or ~/.vimrc are expanded to ${env:USERPROFILE}/.ssh/config etc..

I had heard differently in another issue. Maybe it's configurable? Can you provide a reference (which should have been mentioned from the start, in the PR description)

@rkitover
Copy link
Author

We might want to address this, but need to be sure that fix_fname is the right place to do it. (I haven't analyzed the codepaths involved.)

So I tried to do this in os_open() in os/fs.c first, but it is too late to do it there because fix_fname() has already converted the path to an absolute path to a relative ~ directory, this is why I put it there.

I had heard differently in another issue. Maybe it's configurable? Can you provide a reference (which should have been mentioned from the start, in the PR description)

There used to be an experimental feature for this in get-experimentalfeature that you could enable, I don't remember which exactly, but it has been removed.

In the unreleased development version of PowerShell, which may not be released for quite a while yet, there is an experimental feature for this, PSNativeWindowsTildeExpansion, see here.

@rkitover
Copy link
Author

I believe it would be better if we address this in neovim, because this PowerShell experimental feature will not be available for a while yet, and when it is it would require enabling it, and if this is done in neovim it would also work in WinPS (powershell.exe) and cmd.exe.

@rkitover
Copy link
Author

I should also mention that the PowerShell completion behavior for tilde paths has also recently changed. Before, when you would type:

nvim ~/.ssh/config<TAB>

, it would expand to the absolute path, but PowerShell no longer expands tilde paths to absolute paths, it expands them to canonical tilde paths, leading to the behavior in neovim that I described.

@justinmk
Copy link
Member

If there is not already an issue tracking this, please consider opening an issue with your notes.

In fix_fname() for MS-Windows, expand "~\" or "~/" prefixed paths to the
USERPROFILE environment variable for the user's profile directory
instead of a relative path to a literal "~" directory.

Fix neovim#23901

Signed-off-by: Rafael Kitover <[email protected]>
@rkitover
Copy link
Author

Done, the issue you linked was it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants