Skip to content

Fix fish integration on older fishes#1563

Merged
ellie merged 2 commits intoatuinsh:mainfrom
mattgodbolt:mg/fix_1562
Jan 15, 2024
Merged

Fix fish integration on older fishes#1563
ellie merged 2 commits intoatuinsh:mainfrom
mattgodbolt:mg/fix_1562

Conversation

@mattgodbolt
Copy link
Contributor

  • On fish 3.3 the bash-style $(...) doesn't work, we should use (...)
  • Also quoting "(moo)" on older fishes gives a literal moo
  • The result of a (subcommand) is a single token, so no need to quote it, anyway

Tested by making the change, executing cargo run -- init fish > atuin.fish and then copying and source that script on a system with fish 3.3, as well as 3.6 and observing both still work.

Fixes #1562.

- On fish 3.3 the bash-style `$(...)` doesn't work, we should use `(...)`
- Also quoting `"(moo)"` on older fishes gives a literal `moo`
- The result of a `(subcommand)` is a single token, so no need to quote it, anyway

Tested by making the change, executing `cargo run -- init fish --disable-up-arrow` and then
executing that shell script on a system with fish 3.3, as well as 3.6 and observing both still work.

Fixes atuinsh#1562.
@vercel
Copy link

vercel bot commented Jan 14, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
atuin-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 14, 2024 9:35pm

@mattgodbolt
Copy link
Contributor Author

I couldn't think of a decent way of testing this, else I'd've tried to add a test to lock in the good behaviour.

@ellie
Copy link
Member

ellie commented Jan 14, 2024

Ahhh I see

So this was introduced intentionally in this PR, to fix multiline commands being squished onto one line: #1418

If there's no other way to workaround, we may have to switch on Fish version and drop multiline support for 3.3

@mattgodbolt
Copy link
Contributor Author

Oh darn it :) I'll see what I can do on my side. Upgrading fish is not the easiest; in the server I'm using we're on the 22.04LTS version.

@mattgodbolt
Copy link
Contributor Author

This does seem challening: https://fishshell.com/docs/current/language.html#expand-command-substitution
I'll see if I can make this work in all cases, else I'll update this to check the fish version, if that's indeed feasible too.

@mattgodbolt mattgodbolt marked this pull request as draft January 14, 2024 21:19
@mattgodbolt
Copy link
Contributor Author

Now rephrased in a way that works in both fish 3.3.x and 3.6.x. The release notes for fish say that the new syntax "$(command)" is identical to (command | string collect) and so that's what I now do.

Tested again locally on both 3.3 and 3.6.

Please take another looks

@mattgodbolt mattgodbolt marked this pull request as ready for review January 14, 2024 21:36
@ellie
Copy link
Member

ellie commented Jan 14, 2024

Lovely! Thank you so much

Copy link
Contributor

@arcuru arcuru left a comment

Choose a reason for hiding this comment

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

A similar report came in #1489, I hadn't gotten around to making this PR yet. Thanks :)

@ellie ellie merged commit 69ec991 into atuinsh:main Jan 15, 2024
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.

fish shell init failing on v3.3.1

3 participants