Missing directory slashes when completion chosen

timabell commented 3 weeks ago

Checklist

[X] I have read through the manual page (man fzf)
[X] I have searched through the existing issues
[ ] For bug reports, I have checked if the bug is reproducible in the latest version of fzf

Output of `fzf --version`

0.53.0 (c4a9ccd)

OS

[ ] Linux
[ ] macOS
[X] Windows
[ ] Etc.

Shell

[ ] bash
[X] zsh
[ ] fish

Problem / Steps to reproduce

Thanks for the fab tool.

For a new windows install I'm working on I am getting the backslashes that separate directories all go missing when I hit enter to choose an entry which then means the path is corrupted.

I have done a load of searching and can't find any mention of this being an issue anywhere or what it might be. I don't know enough about how fzf works to guess further so any ideas welcome.

Begin a completion with file **<tab>:

Hit enter to choose an item in a subfolder and all the slashes are missing:

Curiously if I call fzf directly and then search it works fine:

This is my dotmatrix config for the windows machine (windows branch) at time of writing https://github.com/timabell/dotmatrix/blob/0c0de5b7107de4a43c22f1f6c727b2537ffc1056/.zshrc#L107

junegunn commented 3 weeks ago

Is this WSL? Looks like you're running an fzf binary for Windows that uses backslash as the file separator. How did you install fzf?

junegunn commented 2 weeks ago

You should be using Linux binary on WSL. It works for me fine.

junegunn commented 2 weeks ago

Please comment if you still have the problem with a linux binary.

timabell commented 2 weeks ago

Hi, thanks for responding, this is not WSL, this is zsh installed directly on windows, being used in windows terminal.

Yes this is still an issue.

I am unable to use WSL because it's a Virtual Box windows image, and for some reason I can't get the nested virtualisation to work.

I think I installed it with the shallow clone as per the instructions - https://github.com/timabell/dotmatrix/blob/2e554b3093b4d0f8c53f8f2730f0aef63c9e4774/software/fzf.sh#L5-L6 - let me know if you want me to check anything on the windows machine to add more details.

junegunn commented 2 weeks ago

Oh, so does zsh transform the path separators on Windows so you can access paths in Unix style? If that is the case, the Window binary of fzf needs a way to use forward-slashes instead.

junegunn commented 6 days ago

@charlievieth Hi, would it be possible to add path separator option to your fastwalk library for cases where you want forward slashes on Windows? I guess fzf can replace backslashes with slashes afterwards if that is not possible.

charlievieth commented 6 days ago

@charlievieth Hi, would it be possible to add path separator option to your fastwalk library for cases where you want forward slashes on Windows? I guess fzf can replace backslashes with slashes afterwards if that is not possible.

My first thought is that fastwalk is really only concerned about directory traversal and should provide its callback canonical paths so maybe an adapter like the one below would work (which if need be I could add to fastwalk as an adapter):

// ~120ns for a path like "C:\Users\username\go\src\github.com\charlievieth\fastwalk"
// no-op if the path separator is `/`
func ForwardSlashPaths(walkFn fs.WalkDirFunc) fs.WalkDirFunc {
    return func(path string, d fs.DirEntry, err error) error {
        return walkFn(filepath.ToSlash(path), d, err)
    }
}

That said, WSL is a funny thing (Linux, but Windows) and having to convert the path each time does incur some cost so if we think the cost of an adapter is too much then more than happy to add it as an option.

charlievieth commented 6 days ago

Following up on my last comment: an adapter on the fzf side is best way to immediately fix this issue; I think this should be supported in fastwalk the question is whether it's an adapter (like the one below) or as a config option. I'll think about this and post a new release in the next few days.

func toSlash(path string) string {
    if os.PathSeparator == '/' {
        return path
    }
    i := strings.IndexByte(path, '\\')
    if i < 0 {
        return path
    }
    b := []byte(path)
    for ; i < len(b); i++ {
        if b[i] == '\\' {
            b[i] = '/'
        }
    }
    return unsafe.String(&b[0], len(b))
}

// ~50ns if the PathSeparator != `\`
func ToSlashPaths(walkFn fs.WalkDirFunc) fs.WalkDirFunc {
    return func(path string, d fs.DirEntry, err error) error {
        return walkFn(toSlash(path), d, err)
    }
}

junegunn commented 6 days ago

Thanks for the quick response!

an adapter on the fzf side is best way to immediately fix this issue

So if I understood correctly, fzf can replace the separators itself if needed, and it doesn't require a code change in fastwalk. Something like this:

diff --git a/src/reader.go b/src/reader.go
index f39f47f..3a15ac4 100644
--- a/src/reader.go
+++ b/src/reader.go
@@ -8,6 +8,7 @@ import (
    "os"
    "os/exec"
    "path/filepath"
+   "strings"
    "sync"
    "sync/atomic"
    "time"
@@ -254,6 +255,7 @@ func (r *Reader) readFiles(root string, opts walkerOpts, ignores []string) bool
                    }
                }
            }
+           path = strings.ReplaceAll(path, `\`, "/")
            if ((opts.file && !isDir) || (opts.dir && isDir)) && r.pusher([]byte(path)) {
                atomic.StoreInt32(&r.event, int32(EvtReadNew))
            }

  b := []byte(path)

I thinkunsafe.StringData can help here. (if it's okay to modify the bytes in-place)

https://github.com/junegunn/fzf/blob/a06745826a4cba4f578a69258f9def75c59530fc/src/functions.go#L29-L31

junegunn commented 5 days ago

bf515a3d3251c54d04cd8e2e9f422cdd3f789fb8 implements --walker-path-sep.

Please let me know if it doesn't work as expected.

charlievieth commented 1 day ago

@junegunn I just cut v1.0.5 which includes https://github.com/charlievieth/fastwalk/pull/23 and adds an option for enforcing forward-slashes and adds the DefaultToSlash function which detects if we're a Windows executable running under WSL (and enforces forward-slashes if so). This should solve the issue here.

I cut a draft PR https://github.com/junegunn/fzf/pull/3907 that should address this issue. It probably needs to be updated since it's currently incompatible with the -walker-path-sep option (it only allows the option to force forward slashes, but does auto-detect when they're needed on WSL see: DefaultToSlash).

junegunn commented 1 day ago

@charlievieth Thanks for looking into this. I'd also like to avoid adding an option and automatically print the right separator, but @timabell stated above that he was having the problem not on WSL but on zsh on Windows. I don't have experience with it so I'm not quite sure but it looks like zsh on Windows is doing some path conversion.

junegunn commented 1 day ago

@timabell Can you tell us more about your setup? How did you install zsh on Windows? Is it msys?

timabell commented 22 hours ago

Hi,

Sorry, I'm not 100% sure how I ended up with this zsh installed. It did quite possibly come with git extensions or msys git. It was a while ago that I set this VM up.

This is the windows terminal config for the zsh profile that I use to launch it:

            {
                "closeOnExit": "always",
                "commandline": "\"%PROGRAMFILES%\\Git\\usr\\bin\\zsh.exe\" --login -i",
                "guid": "{27ed4f8e-4ea3-4dae-b251-0a792cadd5cd}",
                "hidden": false,
                "name": "zsh",
                "startingDirectory": "C:\\dev\\"
            }

This is what the zsh says about itself

╭─User@WinDev2404Eval /c/dev/dotmatrix ‹windows›
╰─$ which zsh
/usr/bin/zsh
╭─User@WinDev2404Eval /c/dev/dotmatrix ‹windows›
╰─$ zsh --version
zsh 5.9 (x86_64-pc-msys)
╭─User@WinDev2404Eval /c/dev/dotmatrix ‹windows›
╰─$ file `which zsh`
/usr/bin/zsh: PE32+ executable (console) x86-64 (stripped to external PDB), for MS Windows, 11 sections

How do I test the new version? Currently on

fzf --version
0.53.0 (c4a9ccd)

Would above patch fix it or is that only for WSL?

junegunn commented 20 hours ago

So it's MSYS2.

I just tested these things both on bash and zsh on MSYS2, and realized that they work as expected on bash even though fzf is printing paths with backslashes, because the bash script properly escapes the selected entries.

vim <CTRL-T>
Select foo\bar\baz
vim foo\\bar\\baz. Yes.

However, the escaping part is missing on zsh version, and it becomes vim foobarbaz. So we can fix this by properly escaping backslashes on the zsh script.

Using read -r instead of read is a start.

diff --git a/shell/completion.zsh b/shell/completion.zsh
index 46103f9..dce74c6 100644
--- a/shell/completion.zsh
+++ b/shell/completion.zsh
@@ -171,7 +171,7 @@ __fzf_generic_path_completion() {
             rest=${FZF_COMPLETION_PATH_OPTS-}
           fi
           __fzf_comprun "$cmd" ${(Q)${(Z+n+)fzf_opts}} -q "$leftover" --walker "$walker" --walker-root="$dir" ${(Q)${(Z+n+)rest}} < /dev/tty
-        fi | while read item; do
+        fi | while read -r item; do
           item="${item%$suffix}$suffix"
           echo -n "${(q)item} "
         done
diff --git a/shell/key-bindings.zsh b/shell/key-bindings.zsh
index 1490595..8cb95cd 100644
--- a/shell/key-bindings.zsh
+++ b/shell/key-bindings.zsh
@@ -52,7 +52,7 @@ __fzf_select() {
   local item
   FZF_DEFAULT_COMMAND=${FZF_CTRL_T_COMMAND:-} \
   FZF_DEFAULT_OPTS=$(__fzf_defaults "--reverse --walker=file,dir,follow,hidden --scheme=path" "${FZF_CTRL_T_OPTS-} -m") \
-  FZF_DEFAULT_OPTS_FILE='' $(__fzfcmd) "$@" < /dev/tty | while read item; do
+  FZF_DEFAULT_OPTS_FILE='' $(__fzfcmd) "$@" < /dev/tty | while read -r item; do
     echo -n "${(q)item} "
   done
   local ret=$?

Then it gives vim foo\bar\baz. It's an improvement but it's not quite there 😕

You can test this on non-Windows environment by setting

FZF_CTRL_T_COMMAND="echo 'foo\bar\baz'; echo 'hello\world'"

bash gives foo\\bar\\baz hello\\world
zsh gives foobarbaz helloworld

Having said that, things will be simpler if fzf prints forward slashes. But not everyone uses the built-in walker, so we still need to fix the script nevertheless.

junegunn commented 20 hours ago

You can test this on non-Windows environment by setting
FZF_CTRL_T_COMMAND="echo 'foo\bar\baz'; echo 'hello\world'"
bash gives foo\\bar\\baz hello\\world

zsh gives foobarbaz helloworld

@LangLangBart Your expertise in zsh will be really helpful here. Would it be possible to make zsh behave the same as bash in this case?

junegunn / fzf