Skip to content

Fix nvim-remote commands for fish shell #4506

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 27, 2025

Conversation

SavingFrame
Copy link
Contributor

Fish shell does not support "&&" and "||" operators like POSIX-compatible shells. Instead, it uses a different syntax structure based on begin/end and if/else.

This caused existing lazygit nvim-remote integration templates to break when fish was the user's default shell.

This commit adds explicit fish shell detection using the FISH_VERSION environment variable, and provides fish-compatible templates that correctly handle launching Neovim or sending remote commands via $NVIM.

Fixes behavior where edits would not open in a new Neovim tab or line navigation would fail when $NVIM was set.

Ensures smoother editing experience for users running fish shell (supported since Nov 2012 with FISH_VERSION).

  • PR Description

  • Please check if the PR fulfills these requirements

  • Cheatsheets are up-to-date (run go generate ./...)
  • Code has been formatted (see here)
  • Tests have been added/updated (see here for the integration test guide)
  • Text is internationalised (see here)
  • If a new UserConfig entry was added, make sure it can be hot-reloaded (see here)
  • Docs have been updated if necessary
  • You've read through your own file changes for silly mistakes etc

@stefanhaller stefanhaller added the bug Something isn't working label Apr 23, 2025
@stefanhaller
Copy link
Collaborator

Thanks for fixing this! It also came up here, and my suggestion for fixing it was to not use the users shell when calling editor commands, but always using bash as we did before. But your fix may be the better one, as it allows users to use aliases or custom paths for their editors, which came up as a request once or twice.

I pushed two fixups: one is to use same logic to decide which shell to use as the code that eventually calls the command (dc0e1a6); the other is to use more specific variable names (9a77469).

I'll let @jesseduffield decide if he's happy with the approach used.

Copy link
Owner

@jesseduffield jesseduffield left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with the more specific variable names

Fish shell does not support "&&" and "||" operators like
POSIX-compatible shells. Instead, it uses a different syntax structure
based on begin/end and if/else.

This caused existing lazygit nvim-remote integration templates to break
when fish was the user's default shell.

This commit adds explicit fish shell detection using the FISH_VERSION
environment variable, and provides fish-compatible templates that
correctly handle launching Neovim or sending remote commands via $NVIM.

Fixes behavior where edits would not open in a new Neovim tab or line
navigation would fail when $NVIM was set.

Ensures smoother editing experience for users running fish shell
(supported since Nov 2012 with FISH_VERSION).
@stefanhaller stefanhaller force-pushed the fix-fish-remote-nvim branch from 9a77469 to 4d0eaea Compare April 27, 2025 18:12
@stefanhaller stefanhaller enabled auto-merge April 27, 2025 18:13
@stefanhaller stefanhaller merged commit bb64e3c into jesseduffield:master Apr 27, 2025
14 checks passed
@DerRockWolf
Copy link

This fix IMO isn't 100% ideal.

In my setup, I have custom nvim-remote commands (to not open the file in a new tab).

I can now update my lazygit config and fix the custom commands, but then my lazygit config is not shell agnostic anymore.
I would appreciate if the used shell (for the edit things) could be overridden somehow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants