Skip to content
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

[fish] exit as well when called from non-interactive shell #3467

Merged
merged 1 commit into from
Oct 12, 2023

Conversation

calestyo
Copy link
Contributor

@calestyo calestyo commented Oct 8, 2023

Let's see whether the test suite would pass if the old --is-interactive is used.

@calestyo
Copy link
Contributor Author

calestyo commented Oct 8, 2023

Nope. ^^ (Thus reverted the commit to the non-deprecated form).

Well, I don't know should we keep this until someone finds out what's wrong with the test suite on github?
I don't mind as I don't use fish (but i checked it with the commit, and it seemed to work).

@calestyo calestyo force-pushed the improve-interactiveness-checks2 branch from f1998eb to ff1837c Compare October 8, 2023 23:09
@junegunn
Copy link
Owner

junegunn commented Oct 11, 2023

# GitHub actions runs this version of ubuntu
docker run -it ubuntu:22.04

# Install packages
apt-get update
apt install -y git curl fish
fish --version
  # fish, version 3.3.1

git clone --depth 1 https://github.com/junegunn/fzf.git ~/.fzf
~/.fzf/install --all

cd ~/.fzf
patch -p1 < <(curl https://patch-diff.githubusercontent.com/raw/junegunn/fzf/pull/3467.diff)

fish
~/.config/fish/functions/fzf_key_bindings.fish (line 14): 'return' outside of function definition
status is-interactive; or return 0
                          ^
from sourcing file ~/.config/fish/functions/fzf_key_bindings.fish
        called on line 1 of file ~/.config/fish/functions/fish_user_key_bindings.fish
in command substitution
        called on line 2 of file ~/.config/fish/functions/fish_user_key_bindings.fish
in function 'fish_user_key_bindings'
in function '__fish_reload_key_bindings'
        called on line 189 of file /usr/share/fish/functions/__fish_config_interactive.fish
in function '__fish_config_interactive'
        called on line 138 of file /usr/share/fish/config.fish
in function '__fish_on_interactive'
in event handler: handler for generic event 'fish_prompt'
source: Error while reading file '/root/.config/fish/functions/fzf_key_bindings.fish'
fish: Unknown command: fzf_key_bindings
~/.config/fish/functions/fish_user_key_bindings.fish (line 2):
  fzf_key_bindings
  ^
in function 'fish_user_key_bindings'
in function '__fish_reload_key_bindings'
        called on line 189 of file /usr/share/fish/functions/__fish_config_interactive.fish
in function '__fish_config_interactive'
        called on line 138 of file /usr/share/fish/config.fish
in function '__fish_on_interactive'
in event handler: handler for generic event 'fish_prompt'

But this still works, which means the file is not loaded in non-interactive environment

fish -c whoami
  # root

Related: NixOS/nix#7152 (comment)

So the question is: Are we going to drop support for fish 3.3 or below? I don't see a compelling reason to do so.

@calestyo calestyo force-pushed the improve-interactiveness-checks2 branch from ff1837c to 5cb5bbd Compare October 11, 2023 18:26
@calestyo
Copy link
Contributor Author

Thanks. That helped a lot.

It seems it's only since 3359e5d2e9bcbf19d1652636c8e448a6889302ae in fish 3.4.0, that return may be used outside a function (including in a . script).

We could now simply use … or exit, but while that would work fine for interactive shells, it would exit non-interactive ones (of course, the script should never be sourced there, but expectation is that it just gracefully doesn’t load the our script, not exit the sourcing one).

So I think the best way for now is to an if block. I've also open a 2nd PR #3475 already, which corrects that an which we can leave open for say 4 years, till all old versions of fish are out of use.

So the question is: Are we going to drop support for fish 3.3 or below? I don't see a compelling reason to do so.

Well, I guess you know my opinion from the ancient-bash-versions topic... but in this case I'd actually agree.

fish-shell/fish-shell@3359e5d is from 2021... that's not so old.

But I think we should re-visit this in some years... I don't see a reason to keep workarounds that just make the code more complex or error prone for completely outdated stuff - if someone really wants to keep using such things, it's IMO upon him to maintain a patchset for compatibility.

Anyway, for now, let's keep it compatible and revisit this via #3475 it in 2027 and see whether <3.4.0 is still somewhere in use. ;-)

Test runs through now. Please review&merge this one, thanks :-)

@calestyo calestyo marked this pull request as ready for review October 11, 2023 18:47
Just like with the other shells, exit fish to, if called from a non-interactive
shell.

We cannot use `return`, as older versions of fish (namely < 3.4.0) did not
support to use `return` in `.`-scripts (this was only added with fish commit
3359e5d2e9bcbf19d1652636c8e448a6889302ae).

Unlike in POSIX, fish’s `exit` is however documented to no cause the calling
shell to exit when executed in a sourced script (see:
https://github.com/fish-shell/fish-shell/blob/0f70b2c0d310d97b5956b5360ad6cbc548baf72d/doc_src/cmds/exit.rst?plain=1#L20
)

Co-authored-by: Junegunn Choi <[email protected]>
Signed-off-by: Christoph Anton Mitterer <[email protected]>
@calestyo calestyo force-pushed the improve-interactiveness-checks2 branch from 5cb5bbd to 591e209 Compare October 12, 2023 13:15
@junegunn junegunn merged commit 7e89458 into junegunn:master Oct 12, 2023
5 checks passed
@calestyo calestyo deleted the improve-interactiveness-checks2 branch October 12, 2023 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants