Fix search file feature unnecessarily prepending ./ #119
Conversation
| else | ||
| set --append fzf_arguments --query=$token --preview='__fzf_preview_file {}' | ||
| set --append fzf_arguments --query=$expanded_token --preview='__fzf_preview_file {}' |
There was a problem hiding this comment.
I don't see the bug. We used token intentionally because in all of these instances, ~ does expand to $HOME
There was a problem hiding this comment.
Without those if I do a search with ~/ as token and select desktop I end up with './~/desktop'.
The result doesn't work because it's escaped.
There was a problem hiding this comment.
Did some more tests, the expanded_token is only needed there
| set current_token (commandline --current-token) | ||
| if test "$commandline_tokens" = "$current_token" | ||
| set file_paths_selected ./$file_paths_selected | ||
| end |
There was a problem hiding this comment.
Can you explain the bug here, please?
There was a problem hiding this comment.
If you do a query with /etc/ as token and select the systemd directory you end up with .//etc/systemd
|
Oh boy I'm starting to regret introducing the ./ feature. Thanks Quentin! |
ovv
force-pushed
the
ovv/search_current_dir_fix
branch
from
0fa4a54 to
ddee6ff
Compare
Doing a query on a token starting with '~' lead to an escaped result. Using $extended_token makes the result be the full path which doesn't need escaping and can be used as is.
A path starting from the root directory doesn't need './' to cd additionally this conflict with the starting / of the path
ovv
force-pushed
the
ovv/search_current_dir_fix
branch
from
ddee6ff to
e8ae9e6
Compare
|
Hey Quentin, let's back up. The bug you're trying to fix is basically when the current token is used as the base path and it starts with |
PatrickF1
changed the title
Sure no problem, you have a lot more experience with this code than me. Take this PR as an extended bug report :)
Exactly, I just tested your changes. They do fix the Any result with a and then fish will not expand |
You're right. I caught that and was looking for a way to avoid having to always expand ~ because it makes the resulting command line kinda ugly. But it looks like I don't have a choice without terribly complicating the code. Darn |
PatrickF1
changed the title
PatrickF1
changed the title
|
@ovv updated. Can you help me test one more time, please? |
|
everything looks good |
PatrickF1
force-pushed
the
ovv/search_current_dir_fix
branch
from
217abcb to
c6c0928
Compare
PatrickF1
changed the title
| @@ -23,6 +23,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." >&2 | |||
| echo "File doesn't exist or is empty." >&2 | |||
There was a problem hiding this comment.
Turns out this case is hit also when the file is empty.
If the file path already starts with . or /, which can happen if such a path was used as the base directory, then don't prepend ./. Without such logic, the path becomes invalid. For example: - if the current token is /etc/ and you select systemd, you end up with './/etc/systemd' - if the current token is ~/ and you select Desktop, you end up with './~/desktop' Additionally, since the output paths are quoted, we need to use the expanded token when outputting paths because ~ doesn't get expanded inside quotes.
If the file path already starts with . or /, which can happen if such a path was used as the base directory, then don't prepend ./.
Without such logic, the path becomes invalid. For example:
/etc/and you selectsystemd, you end up with'.//etc/systemd'~/and you selectDesktop, you end up with'./~/desktop'Additionally, since the output paths are quoted, we need to use the expanded token when outputting paths because ~ doesn't get expanded inside quotes.