[Search files] fix preview for paths containing spaces #137

PatrickF1 · 2021-03-14T19:19:56Z

@kidonng and I separately found two different bugs around paths containing spaces. Boy this search files code is getting complicated!

Bug 1

This bug happens when the current token is a directory and fzf.fish uses it as the base directory to search for files in.
e.g.

create dir named "folder with spaces"
add a file in that folder with some text
type in "folder with spaces/" and with the cursor on that token, execute search files
notice the preview function doesn't work on the file in the folder

Bug 2

This bug was found by @kidonng. When using a custom preview directory command ($fzf_preview_dir_cmd) is set, this line from __fzf_preview_file.fish

eval $fzf_preview_dir_cmd "$file_path"

results in this code being executed:

$fzf_preview_dir_cmd $file_path

Notice there are no quotes around file_path, which means if file_path is more than one word, it gets broken up into multiple args.
The solution is to escape the quotes around file_path to make sure eval understands it is to be passed as a single arg to $fzf_preview_dir_cmd.

kidonng · 2021-03-15T01:20:37Z

So the whole point of these changes is just to pass expanded and escaped token to __fzf_preview_file. ~~Not sure if it works as there's eval.~~ However it doesn't solve the issue.

But if you don't unescape the token first, the expand may fail. For example it doesn't match '~/foo'. Well we will be using eval in the other PR so it should not be an issue, but if so why another PR.

Again I'm not seeing better improvements, please read my comment in #133 (comment) and decide whether way you want.

PatrickF1 · 2021-03-16T16:01:23Z

But if you don't unescape the token first, the expand may fail. For example it doesn't match '~/foo'

Is that the only problem you see with this PR? Cause I don't mind not supporting that. When would the user by typing in '~/foo` and expecting the ~ to be expanded. Most users would know that tildes don't expand inside a string so why expect fzf.fish to expand it?

PatrickF1 · 2021-03-18T05:14:44Z

@kidonng yeah I still can't figure out why a user would have their current token be a string containing ~. Autocomplete wouldn't quote it and users should know better than to quote ~. I'm going to merge this unless I hear from you tomorrow.

kidonng · 2021-03-18T05:23:30Z

Please ignore that bad example.

To be clear, I'm OK with these changes, but it doesn't fix the issue.

To quote my self:

eval takes arguments right? But if you just quote the strings directly, the eval sees them as regular arguments without quotes.
This is not related to eval, not even Fish, it's just how passing arguments works. Do you see quotes in the output of echo 'hello'?
With escaped quotes, you tell the shell to treat the quotes as part of the string, so the eval works as expected.

PatrickF1 · 2021-03-20T16:42:54Z

Thanks for explaining it the quotes and the all the interpolation and escaping logic going on in fzf_seatch_current_dir. It makes sense to me now. I agree that this change you're making now is the easiest way to fix the bug in its current state. However, that's not to say we can't do some refactoring to make the code more readable as it is. Which is why I'd still like to merge this.

kidonng · 2021-03-20T16:47:28Z

Then please include the escapes in this PR or else it won't match the title (and description).

PatrickF1 · 2021-03-21T17:39:45Z

Then please include the escapes in this PR or else it won't match the title (and description).

Done. Thanks. I just realized we were miscommunicating for a long long time because there were actually 2 different bugs and I was talking about one (__fzf_preview_file receiving unescaped token) and you were talking about the other (eval receiving file_path as multiple args).

kidonng · 2021-03-21T23:00:18Z

__fzf_preview_file receiving unescaped token

As far as I can see there's no issue with it (and whether you change it or not doesn't matter).

PatrickF1 · 2021-03-21T23:31:24Z

As far as I can see there's no issue with it (and whether you change it or not doesn't matter).

Did you try to repro it using the 4 steps I outlined above in my first post?

kidonng · 2021-03-21T23:59:28Z

I see, I fixed this in my PR previously (by using $argv directly, which is now reverted), so I thought they were the same thing.

PatrickF1 · 2021-03-22T00:59:35Z

I see, I fixed this in my PR previously (by using $argv directly, which is now reverted), so I thought they were the same thing.

Oh yes, that was the other thing that made our communication difficult! I thought your original solution was very complicated and that's why I said "the multiple levels of interpolation and evaluation of strings and commands and arguments in this file is mind boggling". Man it's quite hard to communicate on GitHub. Anyway, appreciate your patience and our cooperation on this.

kidonng · 2021-03-22T01:04:57Z

Ah, wait, I did realize they are two different bugs: #133 (comment).

And I kinda got confused by the whole discussion

PatrickF1 · 2021-03-22T01:10:03Z

Oh you're right, I think I must've had a hard time understanding it, glossed over it, and assumed it was just one bug.

kidonng · 2021-03-22T01:20:38Z

Well, though laborious, these discussions are fruitful:

eval is preserved since we are solving it properly
Using escaped token is more explicit than using $argv
Refactoring make code more readable

All things should be clear know, we should focus on gettings the bugs fixed and feature delivered.

PatrickF1 · 2021-03-22T16:11:52Z

functions/__fzf_preview_file.fish

@@ -4,7 +4,8 @@ function __fzf_preview_file --argument-names file_path --description "Print a pr
        bat --style=numbers --color=always "$file_path"
    else if test -d "$file_path" # directory
        if set --query fzf_preview_dir_cmd
-            eval $fzf_preview_dir_cmd "$file_path"
+            # need to escape quotes to make sure eval receives file_path as a single arg
+            eval $fzf_preview_dir_cmd \"$file_path\"


This is the fix for bug 2.

PatrickF1 · 2021-03-22T16:28:45Z

functions/__fzf_preview_file.fish

@@ -28,6 +29,6 @@ function __fzf_preview_file --argument-names file_path --description "Print a pr
    else if test -p "$file_path"
        __fzf_report_file_type "$file_path" "named pipe"
    else
-        echo "File doesn't exist or is empty." >&2
+        echo "$file_path doesn't exist." >&2


Outputting file_path because it helps with debugging.
Removing the "is empty" part because empty files count as regular files and so will result in an empty preview.

PatrickF1 · 2021-03-22T17:49:32Z

functions/__fzf_search_current_dir.fish

        # use the directory name as fzf's prompt to indicate the search is limited to that directory
-        set --append fzf_arguments --prompt=$token --preview="__fzf_preview_file $token{}"
-        set file_paths_selected $expanded_token(fd $fd_opts 2>/dev/null | fzf $fzf_arguments)
+        set --append fzf_arguments --prompt="$unescaped_exp_token" --preview="__fzf_preview_file $expanded_token{}"


Here's bugfix 2: we need to pass the expanded but not yet unescaped token to __fzf_preview_file in order for the preview to work for paths containing spaces.

Not a fan of using expanded token as prompt since it looks bloated... But given we will be adding variable expanding soon and it's better explicit than implicit, I will pass.

Yeah I actually had your PR in mind when I changed it.

PatrickF1 · 2021-03-22T17:49:58Z

tests/preview_file/custom_dir_preview.fish

+set fzf_preview_dir_cmd ls
+
+set actual (__fzf_preview_file "$multi_word_dir")
+@test "correctly uses custom command to preview directories" "$actual" = "file 1.txt file 2.txt"


This test protects against a regression of bug 2

PatrickF1 · 2021-03-22T17:55:38Z

tests/search_current_dir/base_dir_with_spaces.fish

+
+set actual (__fzf_search_current_dir)
+set expected "'tests/_resources/multi word dir/file 1.txt' 'tests/_resources/multi word dir/file 2.txt'"
+@test "uses current token as base directory if it ends in / and is a directory" "$actual" = $expected


This test doesn't protect against bug 1 (it's impossible to test preview functionality in scripts AFAIK) but should prevent other regressions.

Just realized we never really fixed bug 1.

EDIT: actually I can't reproduce this anymore, using 3c3ba4f on fish 3.2.2. Now I tend to believe bug 1 is actually fixed in this PR 🤷‍♀️. I have posted my new summary below.

How would you modify the test to catch this new case? I don't understand why it wasn't caught.

I'm unable to repro this

How would you modify the test to catch this new case? I don't understand why it wasn't caught.

Not to my knowledge, sorry. May take a look when I get the time.

I'm unable to repro this

I can confirm it's not related to my shell profile. Can you reproduce with the same condition as the screenshot (i.e. mkdir 'with spaces', type ./with\ spaces/ and trigger the function)?

Crazy I made the PR about fixing this bug but I didn't actually fix it 🤦‍♂️

Well I was quite confused I didn't notice it either back then.

A good lesson on always testing changes manually if possible 😄

Nvm I see that the test doesn't test the preview function, which I don't know how to do.

Maybe replace the preview function file with a test function, which tests if the command output is expected.

Maybe replace the preview function file with a test function, which tests if the command output is expected.

I can't get fzf to execute the search function in scripted mode. It'd have to not exit automatically, but if it doesn't exit automatically, how do I make it exit so that the test can continue to run.

Start a background shell, sleep for a few seconds and kill the fzf processes?

Thanks for the idea. I might use it if depending on what happens to this issue junegunn/fzf#2442.

PatrickF1 · 2021-03-22T20:46:08Z

.github/workflows/continuous_integration.yml

@@ -28,8 +28,15 @@ jobs:
          git clone --depth 1 https://github.com/junegunn/fzf.git ~/.fzf
          ~/.fzf/install

+      - name: Install fd


Had to add installation of fd because one of the new tests had to make full of it

Actually you can use Homebrew on both macOS and Linux in GitHub Actions, so these can be simplified into brew install fzf fd

kidonng · 2021-03-23T02:52:04Z

Good work 👍

kidonng · 2021-04-10T16:41:50Z

functions/__fzf_search_current_dir.fish

        # use the directory name as fzf's prompt to indicate the search is limited to that directory
-        set --append fzf_arguments --prompt=$token --preview="__fzf_preview_file $token{}"
-        set file_paths_selected $expanded_token(fd $fd_opts 2>/dev/null | fzf $fzf_arguments)
+        set --append fzf_arguments --prompt="$unescaped_exp_token" --preview="__fzf_preview_file $expanded_token{}"


The filename should have been quoted here:

Suggested change

set --append fzf_arguments --prompt="$unescaped_exp_token" --preview="__fzf_preview_file $expanded_token{}"

set --append fzf_arguments --prompt="$unescaped_exp_token" --preview="__fzf_preview_file '$expanded_token{}'"

Apparently, I didn't actually fix this bug when I tried to fix it in #137.

@kidonng

This PR fixes two different bugs around the search files feature and paths containing spaces. # Bug 1 This bug happens when the current token is a directory and fzf.fish uses it as the base directory to search for files in. e.g. 1. create dir named "folder with spaces" 2. add a file in that folder with some text 3. type in "folder with spaces/" and with the cursor on that token, execute search files 4. notice the preview function doesn't work on the file in the folder # Bug 2 This bug was found by @kidonng. When using a custom preview directory command (`$fzf_preview_dir_cmd`) is set, this line from `__fzf_preview_file.fish` ``` eval $fzf_preview_dir_cmd "$file_path" ``` results in this code being executed: ``` $fzf_preview_dir_cmd $file_path ``` Notice there are no quotes around `file_path`, which means if `file_path` is more than one word, it gets broken up into multiple args. The solution is to escape the quotes around `file_path` to make sure eval understands it is to be passed as a single arg to `$fzf_preview_dir_cmd`.

Apparently, I didn't actually fix this bug when I tried to fix it in PatrickF1#137.

kidonng · 2021-05-12T07:38:52Z

Post mortem:

Bug 1 is introduced in Limit file search to directory under cursor if it has a trailing slash #75, caused by __fzf_preview_file receiving unescaped arguments. It is fixed in [Search files] fix preview for paths containing spaces #137 (this PR) by leaving the arguments as is.
Bug 2 is introduced in Make preview directory command configurable #120, caused by programs receiving arguments mangled by eval. There is two attempts to fix it: [Search files] fix preview for paths containing spaces #137 (this PR) and [Search files] fix preview for files with spaces (again) #152, but it is not completely fixed.
Final attempt: [Search directory] fix preview when base dir contains spaces (patch 3) #161

start fix

82dcf6b

PatrickF1 mentioned this pull request Mar 14, 2021

[Search directory] expand variables in current token #133

Merged

PatrickF1 added 2 commits March 14, 2021 17:25

fix fzf prompt as well

80d3dce

write test later

65cf0d5

make sure eval receives file_path as single arg

44e6b1c

PatrickF1 changed the title ~~Fix broken file preview for complicated file paths~~ [Search files] fix preview of multi-word paths Mar 21, 2021

PatrickF1 added 6 commits March 21, 2021 11:14

fix using dir as base path

b5974e5

add a test for using multi word dir as base dir

5ce54ab

start work on next test

c154340

fix base dir

5e76ab4

instead of creating temp files, use permanent test resources

cb038af

remove stash mistake

a6d3b10

PatrickF1 added 2 commits March 21, 2021 16:03

fix test

a3f5070

transition to using test resources

a5d9451

only run .fish files

8798753

PatrickF1 changed the title ~~[Search files] fix preview of multi-word paths~~ [Search files] fix preview for paths containing spaces Mar 22, 2021

PatrickF1 added 2 commits March 22, 2021 08:46

almost done with tests

6db4fe2

tdd: write failing test first

460ae59

PatrickF1 commented Mar 22, 2021

View reviewed changes

PatrickF1 added 2 commits March 22, 2021 09:26

finish test

207a340

fix

2b98808

PatrickF1 commented Mar 22, 2021

View reviewed changes

try installing fd on CI

0c2c387

PatrickF1 force-pushed the fix-preview-for-spaced-files branch from 7079640 to 0c2c387 Compare March 22, 2021 17:04

PatrickF1 added 2 commits March 22, 2021 10:16

fix

86e1c0b

improve readability

46c7ee0

PatrickF1 commented Mar 22, 2021

View reviewed changes

PatrickF1 merged commit 3c3ba4f into main Mar 22, 2021

PatrickF1 deleted the fix-preview-for-spaced-files branch March 22, 2021 17:59

PatrickF1 commented Mar 22, 2021

View reviewed changes

kidonng reviewed Apr 10, 2021

View reviewed changes

PatrickF1 mentioned this pull request Apr 15, 2021

[Search files] fix preview for files with spaces (again) #152

Merged

PatrickF1 added a commit that referenced this pull request Apr 18, 2021

[Search files] fix preview for files with spaces (again) (#152)

4ba889a

Apparently, I didn't actually fix this bug when I tried to fix it in #137.

hrshtst pushed a commit to hrshtst/fzf.fish that referenced this pull request Apr 26, 2021

[Search files] fix preview for files with spaces (again) (PatrickF1#152)

73002ee

Apparently, I didn't actually fix this bug when I tried to fix it in PatrickF1#137.

PatrickF1 mentioned this pull request May 12, 2021

[Search directory] fix preview when base dir contains spaces (patch 3) #161

Merged

kidonng mentioned this pull request May 20, 2021

[Search directory] fix specifying base directories using quotes #163

Merged

	set --append fzf_arguments --prompt="$unescaped_exp_token" --preview="__fzf_preview_file $expanded_token{}"
	set --append fzf_arguments --prompt="$unescaped_exp_token" --preview="__fzf_preview_file '$expanded_token{}'"

[Search files] fix preview for paths containing spaces #137

[Search files] fix preview for paths containing spaces #137

Conversation

PatrickF1 commented Mar 14, 2021 • edited

Bug 1

Bug 2

kidonng commented Mar 15, 2021 • edited

PatrickF1 commented Mar 16, 2021

PatrickF1 commented Mar 18, 2021

kidonng commented Mar 18, 2021

PatrickF1 commented Mar 20, 2021

kidonng commented Mar 20, 2021 • edited

PatrickF1 commented Mar 21, 2021

kidonng commented Mar 21, 2021 • edited

PatrickF1 commented Mar 21, 2021

kidonng commented Mar 21, 2021

PatrickF1 commented Mar 22, 2021

kidonng commented Mar 22, 2021

PatrickF1 commented Mar 22, 2021

kidonng commented Mar 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kidonng Mar 23, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kidonng Apr 10, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kidonng Apr 15, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kidonng Apr 15, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kidonng commented Mar 23, 2021

kidonng Apr 10, 2021 • edited

Choose a reason for hiding this comment

kidonng commented May 12, 2021 • edited

PatrickF1 commented Mar 14, 2021 •

edited

kidonng commented Mar 15, 2021 •

edited

kidonng commented Mar 20, 2021 •

edited

kidonng commented Mar 21, 2021 •

edited

kidonng Mar 23, 2021 •

edited

kidonng Apr 10, 2021 •

edited

kidonng Apr 15, 2021 •

edited

kidonng Apr 15, 2021 •

edited

kidonng Apr 10, 2021 •

edited

kidonng commented May 12, 2021 •

edited