fix(fish): handle dash in fzf key bindings

[LangLangBart]

description

harkabeeparolus: If you use standard vanilla fish shell, without any third-party plugins, by default the builtin cd command is wrapped by a function that provides Directory History. This function maintains the dirprev and dirnext lists, which is what enables fish shells's Directory History (cdh and dirh commands, as well as the Alt-Left and Alt-Right hotkeys) to work as intended. When fzf bypasses this function with builtin cd, the previous directory is never saved to the Directory History, and this information is lost to the user.

Source: #3830 (comment)

Unfortuantely, we can't use the safe cd -- … syntax as there is a small bug in zoxide.

The underlying problem between zoxide and fzf is caused by zoxide actually being incompatible with the default cd command in fish. Regular cd can handle the safer syntax cd -- newdirectory #2659, but zoxide can not ajeetdsouza/zoxide#332. The zoxide people tried to work around this by modifying fzf #2799, which caused fzf Alt-C to break fish shell's default Directory History and cd -.

Source: #3826 (comment)

Therefore, a check for a leading dash has been added, and the prepended builtin command has been removed.

Thanks to @harkabeeparolus for the investigation.

[junegunn]

Thanks, but I think zoxide should fix the bug. It's not ideal that we have to maintain extra code for a minor bug in another project.

[LangLangBart]

I see, I saw your comment earlier¹, but I thought the code change was so small it might go through.

Do you still want to remove the builtin or just leave it as is ?

Footnotes

1. fix: use builtins for cd and history in fish by LangLangBart · Pull Request #3830 · junegunn/fzf · GitHub ↩

[junegunn]

Do you still want to remove the builtin or just leave it as is ?

We should remove it. It would be ideal if we could directly call "the wrapper function" of fish, but it doesn't seem possible because it's just named cd.

https://github.com/fish-shell/fish-shell/blob/master/share/functions/cd.fish

[junegunn]

I thought the code change was so small it might go through

The patch looks okay. But I'm saying that we first need to check if there's a good reason why zoxide cannot handle --.

[LangLangBart]

The patch looks okay. But I'm saying that we first need to check if there's a good reason why zoxide cannot handle --.

I think I've got it. Recently, a user fixed the issue for bash¹, but it hasn't yet been included in a new release.

Below, is a comparision between the latest release vs building from source, the latter succeeds when using cd -- ….

# install zoxide
brew install zoxide
# Start a minimal bash session
command env -i HOME=$HOME TERM=$TERM USER=$USER PATH=/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin /usr/local/bin/bash --norc --noprofile

# init zoxide with the 'cd' command
eval "$(zoxide init --cmd cd bash)"
# notice it fails
cd -- /tmp
# zoxide: no match found

git clone https://github.com/ajeetdsouza/zoxide
cd zoxide
cargo build --release
# start a bash session with the path for zoxide binary build at the beginning
command env -i HOME=$HOME TERM=$TERM USER=$USER PATH=/Users/paria/Developer/zoxide/target/release:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin /usr/local/bin/bash --norc --noprofile

# init zoxide with the 'cd' command
eval "$(zoxide init --cmd cd bash)"
# notice it succeeds
cd -- /tmp
pwd
# /tmp

If one were to alter the fish part similarly to how it was done for bash, then it should work to use the safe cd -- … syntax in fish.

--- a/templates/fish.txt
+++ b/templates/fish.txt
@@ -83,4 +83,6 @@ function __zoxide_z
     else if test $argc -eq 1 -a -d $argv[1]
         __zoxide_cd $argv[1]
+    else if test $argc -eq 2 -a "$argv[1]" = '--'
+        __zoxide_cd $argv[2]
     else if set -l result (string replace --regex -- $__zoxide_z_prefix_regex '' $argv[-1]); and test -n $result
         __zoxide_cd $result

Footnotes

1. support autocd option by solodov · Pull Request #695 · ajeetdsouza/zoxide · GitHub ↩

[LangLangBart]

Since the issue is with zoxide, we can simply revert to cd -- … for the fish part as it was before commit dbe8dc3. If anyone complains, ask them to submit a PR for zoxide.

Do you agree?

[junegunn]

Yes, I agree.

[junegunn]

Looking at the fish code of zoxide, I'm pretty certain it's going to be trivial to fix it for anyone using fish. Not sure why no one has tried yet.

[junegunn]

Thank you!

[LangLangBart]

Not sure why no one has tried yet.

🧐

---

Sorry @Undefined01, the builtin for the cd command was reverted. See the PR description for more details, and please ask zoxide to fix the cd -- … syntax in their fish setup.

[Undefined01]

That's OK. I don't expect the story has happened before and we just repeat it again. Thanks for all your efforts.

[LangLangBart]

Thanks for fixing it in the zoxidie repo, @Undefined01.