Default command outputs relative filepath on Windows (take #2)

[janlazo]

Retry #971 but don't use dir. Instead, use for /r. It outputs the same files as dir /a:-d/s/b.
Problem is ending the for loop when fzf exits. I don't know what's causing cmd.exe to not terminate.

I added v:on flag for now so fzf doesn't run a subshell.

[junegunn]

Thanks, so I guess this doesn't have the performance issue you mentioned in #971?

Problem is ending the for loop when fzf exits.

Just checking if I understood you correctly, you mean v:on fixes the problem?

[janlazo]

Yes, because this version of for loop doesn't rely on another command to generate the list of filepaths. Performance difference is negligible.

v:on is for supporting !my_var! syntax because cmd.exe doesn't enable it by default. It's not for terminating the default command, like ag. For some reason, fzf doesn't kill subprocesses on Windows when it exits. This includes subshells (ie. cmd.exe, powershell). When I close the shell on my terminal (ConEmu), it gets stuck waiting for the default command to terminate. This doesn't happen with the current default command but dir and for are shell builtins on cmd.exe.

v:on is not necessary if https://ss64.com/nt/delayedexpansion.html can be run before the default command but I don't know how to do it in one line.

[janlazo]

Is poll issue on Windows (golang/go#22024) related to this?

[janlazo]

golang/dep#862 may help. I considered taskkill but I need the pid and I don't know what fzf should do if that gets stuck.

[janlazo]

Have to wait on upstream golang/go#17608. Until it's resolved, Windows users must rely on cmd.exe builtins to avoid orphan processes.
I updated https://github.com/junegunn/fzf/wiki/Windows#relative-filepaths so it matches with the default command here.

[janlazo]

@junegunn I'd like to test my changes but I have difficulty compiling my local changes. Can you create an alpha version for me?

[junegunn]

Hey @janlazo, I built and uploaded two alpha binaries for Windows with your patch.

Find 0.17.4-alpha in https://github.com/junegunn/fzf-bin/releases/tag/alpha

[janlazo]

It works. I don't need sift and rg anymore for relative paths.

[junegunn]

Great. Does it still work with those external tools? Do you think it's safe to merge this? Thanks.

[janlazo]

Ah, I forgot to handle the root directory (ie. cd C:/ & fzf). Give me some time.
The problem is %CD% evaluates to C:\ if on root. In a subdirectory, there's no extra backslash.

Does it still work with those external tools?

External tools should work as before but ! is allowed in filenames so it can affect preview. It's not the common case though and it can be escaped with ^ so it's fine.

[janlazo]

%CD% can be replaced with %__CD__% to guarantee a backslash. It's works for Windows 7+. Windows Vista reached EOL last year so I think it's okay to use it directly.

Reference:
https://ss64.com/nt/syntax-variables.html
https://stackoverflow.com/questions/20156490/why-cant-i-access-a-variable-named-cd-on-windows-7/20169219#20169219

[janlazo]

@junegunn I can test it again if you want but it's good to go.

[junegunn]

Thanks, I replaced the alpha binaries on the page with the updated code (fzf --version should print 781d19e), can you test again?

[janlazo]

It broke on C:\Program Files (x86) because of the brackets. Need to escape them with double quotes.

[janlazo]

Now that it's double-quoted, filepaths with "%! can break the for-loop unlike the current default command, dir /s/b. " in filenames is not allowed but user can bypass that rule with cygwin. %! in filenames may result in environment expansion but they're uncommon.

It can break for vim undofiles (:h persistent-undofile) because it uses % (why?) to substitute path separators in Windows but that's not fzf's concern.

I'll add a new entry to restore the old behavior after this is merged.

@junegunn If you don't mind, can you compile me another alpha version?

[junegunn]

New binaries are up.

[janlazo]

It worked. I think it's safe to merge now, at least for other users to try out.

[junegunn]

Works for me too, thanks!

[janlazo]

@kelleyma49 Can you test the new default command on Windows?

[kelleyma49]

Hi - I've been testing under Windows and I've found a bug.

If you run the latest fzf from C:, a $GetCurrent is prepended to the path. See the attached screenshot:

[janlazo]

I use ConEmu + powershell as well but I haven't encountered that issue. The new default commands relies on __CD__ and _curfile environment variables so either $GetCurrent is a directory or it is expanded from an environment variable.

Does it happen in cmd.exe (launched directly) or powershell.exe (with -NoProfile)?

[janlazo]

$GetCurrent is a directory on Windows 10. for /r picks up hidden folders so it can works on $Recycle.Bin (Windows 7+).

https://www.ghacks.net/2017/05/16/can-i-delete-getcurrent-sysreset-windows-ws-and-hyper-v-tmp/

[kelleyma49]

Good catch. I learn something new about Windows every day!

I tested your changes against fzf version 0.17.3. I ran both versions of fzf in my c:\Program Files\ directory, 3 times each.

0.17.3 completes the directory listing in an average of 5 seconds; the version on head completes with an average of 19 seconds. I see similar the similar 4x slowdown in other directories.

Is anyone else seeing this slowdown?

[janlazo]

@kelleyma49 Can you benchmark both commands without fzf and warm/cold caches? I used Measure-Command on both commands and ran them on C:\Program Files\ but their speed is about the same (differ by a few milliseconds if they output the same files).

Note that dir /s/b does not list all files/directories so you have to customize it such that it outputs the same files as for /r %P in (*) do @echo %P.

[kelleyma49]

@janlazo - I did some more testing by building a version with your change and another version with the previous change. You are correct: I don't see a measurable difference in speed between the two versions.

I still see a slow down between the 0.17.3 distributed binary and the exes I build from source. Must be something in my configuration...