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

Missing directory slashes when completion chosen #3859

Closed
4 of 10 tasks
timabell opened this issue Jun 13, 2024 · 16 comments · Fixed by #3907 or #3909
Closed
4 of 10 tasks

Missing directory slashes when completion chosen #3859

timabell opened this issue Jun 13, 2024 · 16 comments · Fixed by #3907 or #3909
Labels

Comments

@timabell
Copy link

Checklist

  • I have read through the manual page (man fzf)
  • 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
  • Windows
  • Etc.

Shell

  • bash
  • 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>:
image

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

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

image

image

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

@timabell timabell changed the title Missing diretcory slashes when completion chosen Missing directory slashes when completion chosen Jun 13, 2024
@junegunn
Copy link
Owner

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
Copy link
Owner

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

image

@junegunn
Copy link
Owner

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

@timabell
Copy link
Author

timabell commented Jun 19, 2024

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 junegunn reopened this Jun 19, 2024
@junegunn
Copy link
Owner

junegunn commented Jun 20, 2024

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.

Related:

@junegunn
Copy link
Owner

@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
Copy link
Contributor

@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
Copy link
Contributor

charlievieth commented Jun 29, 2024

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
Copy link
Owner

junegunn commented Jun 29, 2024

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)

fzf/src/functions.go

Lines 29 to 31 in a067458

func stringBytes(data string) []byte {
return unsafe.Slice(unsafe.StringData(data), len(data))
}

@junegunn
Copy link
Owner

bf515a3 implements --walker-path-sep.

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

charlievieth added a commit to charlievieth/fzf that referenced this issue Jul 3, 2024
This commit changes FZF to enforce that all paths are joined with
forward-slashes when running on WSL (Windows Subsystem for Linux)
even when the FZF binary was compiled for Windows.

Update: github.com/charlievieth/fastwalk to v1.0.5
Fixes:  junegunn#3859
charlievieth added a commit to charlievieth/fzf that referenced this issue Jul 3, 2024
This commit changes FZF to enforce that all paths are joined with
forward-slashes when running on WSL (Windows Subsystem for Linux)
even when the FZF binary was compiled for Windows.

Update: github.com/charlievieth/fastwalk to v1.0.5
Fixes:  junegunn#3859
charlievieth added a commit to charlievieth/fzf that referenced this issue Jul 3, 2024
This commit changes FZF to enforce that all paths are joined with
forward-slashes when running on WSL (Windows Subsystem for Linux)
even when the FZF binary was compiled for Windows.

Update: github.com/charlievieth/fastwalk to v1.0.5
Fixes:  junegunn#3859
@charlievieth
Copy link
Contributor

@junegunn I just cut v1.0.5 which includes charlievieth/fastwalk#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 #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
Copy link
Owner

junegunn commented Jul 3, 2024

@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
Copy link
Owner

junegunn commented Jul 3, 2024

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

@timabell
Copy link
Author

timabell commented Jul 4, 2024

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
Copy link
Owner

junegunn commented Jul 4, 2024

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
Copy link
Owner

junegunn commented Jul 4, 2024

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 added a commit that referenced this issue Jul 4, 2024
Fix #3859

To test:

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

  _fzf_compgen_path() {
    eval $FZF_CTRL_T_COMMAND
  }

  source shell/key-bindings.zsh
  source shell/completion.zsh
junegunn added a commit that referenced this issue Jul 4, 2024
Fix #3859

To test:

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

  _fzf_compgen_path() {
    eval $FZF_CTRL_T_COMMAND
  }

  source shell/key-bindings.zsh
  source shell/completion.zsh
junegunn added a commit that referenced this issue Jul 5, 2024
junegunn added a commit that referenced this issue Jul 8, 2024
…3907)

This commit changes FZF to enforce that all paths are joined with
forward-slashes when running on WSL or MSYS
even when the FZF binary was compiled for Windows.

Update: github.com/charlievieth/fastwalk
Fixes:  #3859

---------

Co-authored-by: Junegunn Choi <junegunn.c@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants