[Search directory] expand variables in current token #133

kidonng · 2021-03-09T09:29:29Z

Evolved from #109. Now not only does tilde (~) in the current token get expanded, variables do as well. This is useful for quickly accessing known directory variables, e.g. $XDG_CONFIG_HOME, $TMPDIR, $JAVA_HOME, $fisher_path, $__fish_user_data_dir.

kidonng · 2021-03-09T12:18:03Z

In addition to the intended changes of this PR (which is only 2 LOC), I fixed two other issues:

__fzf_preview_file may receive a path containing spaces, so we should directly use $argv as filename.
Even if we fixed 1, eval will wreck the path, no matter if you escape it or not.

Funny how this feature also makes use of eval 😂

But changing eval to command `has its own issues:

Test breaks (fixed)
Users can't specify a function anymore. Though I doubt if there are really needs for this.

PatrickF1 · 2021-03-09T17:13:06Z

Wow this is kind of a crazy idea. I'm not sure how I feel about it right now tbh. You know I've been hesitant to invest more in search files. On one hand, this makes search files more complicated and hard to understand. On the other hand, I can see this being useful for when users paste in well known directory variables, e.g. $XDG_CONFIG_HOME, $JAVA_HOME, $fisher_path, $__fish_user_data_dir. Though even that is rare...and the (risk * cost) of this breaking and having to maintain this is high.
Hmm I might need some convincing to merge this.

kidonng · 2021-03-10T02:17:38Z

On one hand, this makes search files more complicated and hard to understand.

It doesn't make things more complicated, we are already expanding tilde, so why not do it for everything. And how is it hard to understand? It do what the user wants to do, nothing more.

On the other hand, I can see this being useful for when users paste in well known directory variables, e.g. $XDG_CONFIG_HOME, $JAVA_HOME, $fisher_path, $__fish_user_data_dir.

My motivation for this is I wanted to search macOS's temporary directory (not /tmp but $TMPDIR which is a lonnnnnnng path), and then realized it doesn't recognize variables.

Though even that is rare...and the (risk * cost) of this breaking and having to maintain this is high.

This includes expanding tilde which is not rare at all. Plus, I wonder how will this break things (the fixed bugs is not related to this change). It's literally just echoing strings.

PatrickF1 · 2021-03-10T21:27:33Z

@kidonng You make good points. I would also like to be able to use this feature and it doesn't add much complexity. Let's work on a bit more but for sure, I will eventually merge. Thanks for your patience!

tests/preview_file/custom_dir_preview.fish

PatrickF1 · 2021-03-11T03:35:20Z

functions/__fzf_preview_file.fish

@@ -1,10 +1,11 @@
 # helper function for __fzf_search_current_dir
-function __fzf_preview_file --argument-names file_path --description "Print a preview for the given file based on its file type."
+function __fzf_preview_file --description "Print a preview for the given file based on its file type."
+    set file_path $argv


I can't repro the bug that this fixes. I created a dir named "dir with space" and put some files, also containing spaces in the name, and was able to preview them just fine using the code on main. Can you teach me how to repro the bug?

Ah ok I see why this bug happens. It was introduced by this PR, because we have to pass the unescaped expanded token to __fzf_preview_file. This is a good example of additional features that introduces bugs and more complicated code...

Sorry for the delay!

Can you teach me how to repro the bug?

Actually I just realized there's a implicit prerequisite: you have to set fzf_preview_dir_cmd or the code won't run eval $fzf_preview_dir_cmd "$file_path" but just regular command ls -A -F "$file_path".

That said, here is a video demo of the issue:

Screen.Recording.2021-03-13.at.17.15.06.mov

It was introduced by this PR

No it's not. We do need to pass expanded_token now, but that's not how the bug works.

PatrickF1 · 2021-03-12T19:16:13Z

@kidonng Mind I'm ready to merge in the main change but not the bug fix. Mind if I or you revert the unnecessary changes and merge them in later?

kidonng · 2021-03-13T09:31:51Z

Mind I'm ready to merge in the main change but not the bug fix. Mind if I or you revert the unnecessary changes and merge them in later?

That's ok if you want to, but I'm not in favor of the idea because:

This feature is affected by the bug
The new PR still needs to refer this one
The fix is only 3 LOC (unless you have to include the test changes)

PatrickF1 · 2021-03-13T18:53:27Z

That's ok if you want to, but I'm not in favor of the idea because:

This feature is affected by the bug

The new PR still needs to refer this one

The fix is only 3 LOC (unless you have to include the test changes)

True true we'll keep it in this PR. Let me reset it for you.
I don't know what you meant by

Actually I just realized there's a implicit prerequisite: you have to set fzf_preview_dir_cmd or the code won't run eval $fzf_preview_dir_cmd "$file_path" but just regular command ls -A -F "$file_path".

Can you fix that by taking over this PR again?

kidonng · 2021-03-14T07:02:39Z

Can you fix that by taking over this PR again?

Perhaps I'm not expressing myself clearly, I was saying that probably you didn't set fzf_preview_dir_cmd so can't reproduce.

Maybe eval can be kept, we just need to quote the arguments properly.

PatrickF1 · 2021-03-14T18:36:26Z

functions/__fzf_search_current_dir.fish


    # If the current token is a directory and has a trailing slash,
    # then use it as fd's base directory.
    if string match --quiet -- "*/" $token && test -d $expanded_token
        set --append fd_opts --base-directory=$expanded_token
        # 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 --append fzf_arguments --prompt=$token --preview="__fzf_preview_file $expanded_token{}"


Interesting solution adding escaped quotes around the token. What if you just added single quotes here

--preview="__fzf_preview_file '$expanded_token{}'"

?

oh nvm that wouldn't work because single quotes prevent string interpolation...hmmm okay

TBH it seems to work but I don't like the fix. It's too hacky, hard to read, hard to understand. Let me see if I can fix it myself. I started a new branch because it's a complicated fix and I realized I'm going to have to add tons more test and documentation.

Gonna work on it here #137

It's too hacky, hard to read, hard to understand.

The fix is only adding two backslashes in __fzf_preview_file, how in the world is it complicated? And the problematic eval isn't even introduced in this PR.

As for the feature itself only two other lines are changed.

I think we will never get there if you want to work on a new fix, since I don't see any simper/better solution. If you really find this expanding feature troublesome, let's just skip it.

The fix is only adding two backslashes in __fzf_preview_file, how in the world is it complicated?

It is complicated: I've never seen any shell code force quotes except when needing to put quotes inside a string for printing, not running a command. Maybe I'm obtuse, but the multiple levels of interpolation and evaluation of strings and commands and arguments in this file is mind boggling. It's hard to grasp, much harder to document and test.

As for the feature itself only two other lines are changed.

True, that's why I'm willing to merge it. I'll merge it as soon as I figure out a better fix that's hopefully simpler in #137.

If you really find this expanding feature troublesome, let's just skip it.

To be perfectly transparent so there's no surprises, I get disturbed just thinking about how complicated __fzf_search_current_dir.fish is. I really, really want to cut back on the number of features in it. And the most complicated one of them of all is the expansion of ~ and $vars. I am going to try to keep it because we've already put in the work and I know you and some other people really want it and use it. But personally, it's eating up what little time I have to devote to this project so ultimately it hurts everyone. If I can't find a simple fix for this, I might just remove the feature. Sorry.

I've never seen any shell code force quotes except when needing to put quotes inside a string for printing, not running a command.

Fine, let me explain it clearly.
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.

The example above is equivalent to

count with spaces count "with spaces"

Maybe I'm obtuse, but the multiple levels of interpolation and evaluation of strings and commands and arguments in this file is mind boggling.

I think you are referring to fzf_seatch_current_dir.

"Multiple levels of interpolation", well you can treat it that way. I don't understand how is this abnormal, should you do everything in one line?

Evaluation, that's how things get interesting but again it's pretty simple:

we unescape the current token first so other programs understand it

we expand it so tilde and variables works

finally we escape the output file path so it is directly usable in the shell

as soon as I figure out a better fix that's hopefully simpler in #137.

This is the simplest solution.

I get disturbed just thinking about how complicated __fzf_search_current_dir.fish is. I really, really want to cut back on the number of features in it. And the most complicated one of them of all is the expansion of ~ and $vars.

Sorry for causing you tremendous maintenance burden, but as I have repeatedly said in this PR, this change doesn't introduce more complicated code, we are just improving existing code.

So let me sum up the changes again (three lines in total):

We are already expanding tilde with string replace, we are just expanding it to variables, and possibly other things via eval. (You may argue eval is complicated by itself, well I have explained it above so no.)

We are passing $token to fzf_preview_file, but if we want clarity, we should already be using $expanded_token when I submitedt the PR to expand tilde (my fault).
So why is it still working? Well we are passing tilde directly to fish, so fish expands it by itself, but it only see exported variables so we have to use $expanded_token in this PR.

The bug fix, is just properly passing arguments to eval. This is explained above.

I hope I made things clear enough thus no more back and forth needed.

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

@kidonng Finally merged #137. Can you fix the merge conflicts and we may finally merge this today? Thanks! Sorry for the long wait.

kidonng · 2021-03-23T03:41:38Z

Should be covered by existing test for tilde expanding but we can also add tests for variable expanding if you want.

PatrickF1 · 2021-03-23T03:44:16Z

Yes please, could you add a test for expanding variables? An idea I have is to execute mktmp and then check for it in TMPDIR. Up to you.

PatrickF1 · 2021-03-23T15:58:01Z

functions/__fzf_search_current_dir.fish

-    # expand ~ in the directory name since fd can't expand it
-    set expanded_token (string replace --regex -- "^~/" $HOME/ $token)
+    # expand the token (which may include tilde, variables, etc.) into full path
+    set expanded_token (eval echo -- \"$token\")


What's an example where removing the quotes would break this?

I'm just quoting it like in __fzf_preview_file.

I just can't think of when token when ever be separated by an unescaped space. Whatever, I suppose it's safer this way.

users should know better than to quote ~

You are totally right... I was too careful to do it right.

Oh shoot, this is actually a bug. Quoting it broke this for tilde. Just tested.

Fixed in 83cb78f

kidonng · 2021-03-24T01:09:10Z

Nice work there 👍 sorry for always letting you finishing the tests.

PatrickF1 · 2021-03-24T01:12:05Z

Nice work there 👍 sorry for always letting you finishing the tests.

No problem, it's teamwork! You've also brought a lot of good ideas and expanded my perspective on what a good plugin is.

I also realized that the test meant to catch this exact issue was giving false positives because the tilde would automatically expand when echoed, so I fixed it by escaping the tilde. Relates to #133 (comment).

Evolved from PatrickF1#109. Now not only does tilde (~) in the current token get expanded, variables do as well. This is useful for quickly accessing known directory variables, e.g. $XDG_CONFIG_HOME, $TMPDIR, $JAVA_HOME, $fisher_path, $__fish_user_data_dir.

I also realized that the test meant to catch this exact issue was giving false positives because the tilde would automatically expand when echoed, so I fixed it by escaping the tilde. Relates to PatrickF1#133 (comment).

kidonng added 2 commits March 9, 2021 17:24

[Search files] expand entire token

262a39b

That thing

05dffd1

kidonng marked this pull request as draft March 9, 2021 10:17

kidonng added 5 commits March 9, 2021 18:25

Use expaned token in the preview

2e29c3e

Fix escaping

b6b60bd

eval is evil

e24052f

Try to fix test

ecbe926

🤷‍♀️

231ec29

kidonng marked this pull request as ready for review March 9, 2021 11:45

kidonng and others added 3 commits March 9, 2021 19:51

fix typo

b87a24b

Save a escape

5fa321b

Update __fzf_search_current_dir.fish

eeedadb

PatrickF1 reviewed Mar 10, 2021

View reviewed changes

tests/preview_file/custom_dir_preview.fish Outdated Show resolved Hide resolved

PatrickF1 reviewed Mar 11, 2021

View reviewed changes

Merge branch 'main' into expand-token

ed6f2c7

PatrickF1 force-pushed the expand-token branch from bdce934 to ed6f2c7 Compare March 13, 2021 18:54

kidonng added 2 commits March 14, 2021 15:07

Escape properly

b4b4186

Quote the new eval as well

f81af52

PatrickF1 reviewed Mar 14, 2021

View reviewed changes

PatrickF1 mentioned this pull request Mar 14, 2021

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

Merged

PatrickF1 force-pushed the main branch from c8a630b to ed5b4fe Compare March 22, 2021 22:24

PatrickF1 force-pushed the main branch from ed5b4fe to 8fbdc23 Compare March 22, 2021 22:26

kidonng and others added 5 commits March 23, 2021 11:11

Merge branch 'main' into expand-token

1617780

Merge

43d131f

Fix space

931c052

Update __fzf_search_current_dir.fish

78c43a7

Update __fzf_search_current_dir.fish

5f30c49

PatrickF1 changed the title ~~[Search files] expand entire token~~ [Search directory] expand entire token Mar 23, 2021

PatrickF1 added 3 commits March 22, 2021 23:47

start test

0b8975b

Merge branch 'main' into expand-token

6957773

work on new test

62b26e1

PatrickF1 reviewed Mar 23, 2021

View reviewed changes

PatrickF1 added 2 commits March 23, 2021 09:08

finish test

62d97cd

add timeout to tests

d8d3f1e

PatrickF1 changed the title ~~[Search directory] expand entire token~~ [Search directory] expand variables in current token Mar 23, 2021

PatrickF1 merged commit 2c0fb53 into PatrickF1:main Mar 23, 2021

kidonng mentioned this pull request Mar 25, 2021

[Search directory] fix tilde expanding #145

Closed

kidonng mentioned this pull request Jun 22, 2021

[Search directory] fix quick directory change for hidden directories #178

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Search directory] expand variables in current token #133

[Search directory] expand variables in current token #133

kidonng commented Mar 9, 2021 •

edited by PatrickF1

Loading

kidonng commented Mar 9, 2021 •

edited

Loading

PatrickF1 commented Mar 9, 2021

kidonng commented Mar 10, 2021 •

edited

Loading

PatrickF1 commented Mar 10, 2021

PatrickF1 Mar 11, 2021

PatrickF1 Mar 13, 2021

kidonng Mar 13, 2021

PatrickF1 commented Mar 12, 2021

kidonng commented Mar 13, 2021 •

edited

Loading

PatrickF1 commented Mar 13, 2021

kidonng commented Mar 14, 2021

PatrickF1 Mar 14, 2021

PatrickF1 Mar 14, 2021

PatrickF1 Mar 14, 2021

PatrickF1 Mar 14, 2021

kidonng Mar 14, 2021 •

edited

Loading

PatrickF1 Mar 15, 2021 •

edited

Loading

kidonng Mar 15, 2021 •

edited

Loading

PatrickF1 commented Mar 22, 2021

kidonng commented Mar 23, 2021

PatrickF1 commented Mar 23, 2021 •

edited

Loading

PatrickF1 Mar 23, 2021

kidonng Mar 24, 2021

PatrickF1 Mar 24, 2021

kidonng Mar 25, 2021 •

edited

Loading

PatrickF1 Mar 25, 2021

PatrickF1 Mar 25, 2021

kidonng commented Mar 24, 2021

PatrickF1 commented Mar 24, 2021

[Search directory] expand variables in current token #133

[Search directory] expand variables in current token #133

Conversation

kidonng commented Mar 9, 2021 • edited by PatrickF1 Loading

kidonng commented Mar 9, 2021 • edited Loading

PatrickF1 commented Mar 9, 2021

kidonng commented Mar 10, 2021 • edited Loading

PatrickF1 commented Mar 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PatrickF1 commented Mar 12, 2021

kidonng commented Mar 13, 2021 • edited Loading

PatrickF1 commented Mar 13, 2021

kidonng commented Mar 14, 2021

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 Mar 14, 2021 • edited Loading

Choose a reason for hiding this comment

PatrickF1 Mar 15, 2021 • edited Loading

Choose a reason for hiding this comment

kidonng Mar 15, 2021 • edited Loading

Choose a reason for hiding this comment

PatrickF1 commented Mar 22, 2021

kidonng commented Mar 23, 2021

PatrickF1 commented Mar 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kidonng Mar 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kidonng commented Mar 24, 2021

PatrickF1 commented Mar 24, 2021

kidonng commented Mar 9, 2021 •

edited by PatrickF1

Loading

kidonng commented Mar 9, 2021 •

edited

Loading

kidonng commented Mar 10, 2021 •

edited

Loading

kidonng commented Mar 13, 2021 •

edited

Loading

kidonng Mar 14, 2021 •

edited

Loading

PatrickF1 Mar 15, 2021 •

edited

Loading

kidonng Mar 15, 2021 •

edited

Loading

PatrickF1 commented Mar 23, 2021 •

edited

Loading

kidonng Mar 25, 2021 •

edited

Loading