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

Windows pathsep #911

Open
wants to merge 6 commits into
base: master
from
Open

Windows pathsep #911

wants to merge 6 commits into from

Conversation

@Xjs
Copy link

Xjs commented Feb 13, 2020

Relating to #897, in my daily use I have found that completing Windows paths with slashes is usually better than with escaped backslashes, even if not entirely correct. This is due to the slash being a character that needs not be escaped. Completion with a character that is only valid in quotes does not feel good in everyday usage, at least to me -- even if it works.

This PR fixes several completion uses I had when using Elvish on Windows 10 (as well as a typo in pkg/edit/builtins.go, for which I couldn't be bothered to open an extra PR).

Let me know what you think!

Further enhancements could include the quoted string concatenation function mentioned in #897, and using it to complete backslashy paths as-is, but still use slashes as default for relative navigation, or making the behaviour configurable.

@xiaq

This comment has been minimized.

Copy link
Member

xiaq commented Feb 16, 2020

#897 was fixed by #903.

I know that Windows supports forward slashes as a path separator on the kernel API level, but not all commands work well with it. Keeping the use of backslashes is a better choice.

@xiaq

This comment has been minimized.

Copy link
Member

xiaq commented Feb 16, 2020

Thanks for noting the typo, I fixed it in d623daf.

I see that you made a change to how files are determined to be executable but I'm not sure why you made it - care to elaborate?

@Xjs

This comment has been minimized.

Copy link
Author

Xjs commented Feb 17, 2020

Thanks for fixing the typo.

As for the slashes/backslashes thing, I agree that using backslashes is technically the better choice, but I find mandatorily having to use quoted strings for file paths is a usability regression. I would be happy if using slashes or backslashes were a configurable option, even at compile time (which it de facto is, I can of course patch it myself and will probably continue doing so for the time being). If you cannot be swayed at all, could we maybe discuss #583 again? I don't have a really good solution though, just the complaint that I feel the path separator is so frequently used it should not be a metacharacter.

(Sorry for being so fussy about that. I really like Elvish, and I'm glad that I found the project, and I'm really thankful for all the time and work you put in this so far -- and I'm really enthusiastic about using it daily and contributing. But obviously I'm very demanding regarding the UX of my daily used tools. That's why I would be really happy if we could reach a consensus here :))

As for the executable change, I noticed that tab-completion for external commands didn't work on Windows, and found out that the check for executable files checks the FileMode in the infos returned from ReadDir. However, on Windows file systems (NTFS here), files generally don't have an executable bit and so the check for all executable bits will always fail. This was a very quick'n'dirty fix since I couldn't immediately find what heuristic could be used to determine if a file is executable on Windows.

I think theoretically, every file could be executable, but the syscall will complain if the file doesn't contain an executable header or a script... but it could be that certain file name extensions would be a good heuristic as well. I would need to research more to be 100% certain. The fix I made condenses all the executable bit checks in one place, though, which would it easier to iterate on these kinds of heuristics. (NB: Checking for mode & 0111 on UNIX, i. e. whether every executable bit is set, is too broad a heuristic as well. A file I own could have mode 700 and still be executable for me. So there's room for improvement here as well I'd say.)

@SitiSchu

This comment has been minimized.

Copy link
Contributor

SitiSchu commented Feb 23, 2020

Regarding the slash conversion, a system like bash on msys2 would be nice where the shell uses forward slashes but converts them to backslashes for commands, this would make the experience a lot more consistent across OSs

@SitiSchu

This comment has been minimized.

Copy link
Contributor

SitiSchu commented Feb 23, 2020

Detecting executables on windows should probably be done using the PATHEXT variable, it's used to determine which file extensions are executable: https://superuser.com/questions/1027078/what-is-the-default-value-of-the-pathext-environment-variable-for-windows

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

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.