Skip to content

Conversation

@akinomyoga
Copy link
Contributor

(First commit) Check interactive shell.

In a typical configuration of .bashrc, in an interactive session (where $- contains the letter i), it returns at the beginning of ~/.bashrc. However, a different configuration is possible. In such a case, eval "$(atuin init bash)" can be called from non-interactive shells, and causes error messages related to the keybindings, i.e., the uses of the bind command in non-interactive shells causes warning messages. We can have a check also at the beginning of atuin.bash.

(Second commit) Check the Bash version

The requirement on the Bash version does not seem to be clarified anywhere, so I suggest explicitly specifying it to be bash >= 3.1. This is the same as that of bash-preexec.

  • As far as I tested it in Bash 3.1 and 3.2 with bash-preexec, it doesn't work perfectly, but it still works in not a too bad way with enter_accept. In Bash 3.1 & 3.2, there is no way to obtain the content of the current command line because READLINE_LINE and READLINE_POINT are only introduced in bash >= 4. This forces Atuin to always start with the full history for the search (without filtering by the current command-line content), but it still works with enter_accept. Also, there is no way to clear up the current command-line content after enter_accept. This behavior might feel a bit strange, but it's still acceptable.
  • Without enter_accept, there is no robust way to insert the suggestion in the command line. I here suggest just noting that in README. Alternatively, we could still insert the suggestion by doing a hack similar to what is done by fzf-keybindings.bash L110. Although this can be broken by various conditions, if would like to introduce, this one I can consider the possibility of implementing it.
  • Note: this does not happen with ble.sh. ble.sh implements READLINE_LINE and READLINE_POINT also for Bash 3.

(Third commit) Work around missing READLINE_LINE in Bash 3 w/ bash-preexec

However, in bash < 4, there is still possibly a strange behavior caused by the remaining READLINE_LINE from the previous call of atuin. To prevent it, I suggest localizing READLINE_LINE and READLINE_POINT in bash < 4 without ble.sh. I also left some descriptions in the commit message.

(Fourth commit) Avoid double initialization

See the commit message. As another option, we may add our hooks into preexec_functions and precmd_functions only when there are no existing ones. This enables the reloading of Atuin's settings in the same shell session after updating Atuin. If you'd like it this way, I can adjust the PR.

@vercel
Copy link

vercel bot commented Jan 9, 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 10, 2024 1:48pm

@akinomyoga akinomyoga changed the title fix(bash): work around bash <= 3 and introduce initialization guards fix(bash): work around bash < 3 and introduce initialization guards Jan 9, 2024
@akinomyoga akinomyoga changed the title fix(bash): work around bash < 3 and introduce initialization guards fix(bash): work around bash < 4 and introduce initialization guards Jan 9, 2024
ellie
ellie previously approved these changes Jan 10, 2024
Copy link
Member

@ellie ellie left a comment

Choose a reason for hiding this comment

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

Thank you!

Although this can be broken by various conditions, if would like to introduce, this one I can consider the possibility of implementing it.

How broken would it be, and by what conditions?

@ellie
Copy link
Member

ellie commented Jan 10, 2024

ah, some conflicts that need resolving before we can merge

In bash < 4, the variables READLINE_LINE and READLINE_POINT are not
supported for the shell commands called by `bind -x`.  Even if it is
not supported, atuin works in not a bad way.  However, this sometimes
causes a strange behavior by the remaining values of READLINE_LINE set
in the previous calls of __atuin_history.  In bash < 4, we can
consistently use an empty string instead of $READLINE_LINE, and the
changes to READLINE_LINE and READLINE_POINT should be localized within
the function.
In bash, it is customary to reload the settings by sourcing `.bashrc`
again after modifying it.  In such a case, `eval "$(atuin init bash)"`
is executed again.  This registers duplicate hooks to
`preexec_functions` and `precmd_functions`.  To prevent this in this
patch, we introduce an include guard, so that the initialization is
not performed more than once.
@akinomyoga
Copy link
Contributor Author

Thank you for reviewing. For the conflict, I've rebased those commits on top of the current main branch.

Although this can be broken by various conditions, if would like to introduce, this one I can consider the possibility of implementing it.

How broken would it be, and by what conditions?

What is done in fzf/keybindings.bash is essentially a keyboard macro, so if any of the keys involved in the macro is customized by the user, it will not work. It assumes that all the keybindings to the keys in the macro are the default ones. Also, it uses the kill ring (i.e., something like an internal clipboard of Bash for copying and pasting) to remember the original content and the cursor position of the command line, so the text content that the user copied previously would be lost (or at least, becomes harder to access).

@ellie
Copy link
Member

ellie commented Jan 10, 2024

What is done in fzf/keybindings.bash is essentially a keyboard macro, so if any of the keys involved in the macro is customized by the user, it will not work

Hm I see. Best to be avoided then imo - thank you!

@ellie ellie merged commit a7bed61 into atuinsh:main Jan 10, 2024
@akinomyoga akinomyoga deleted the bash-version branch January 10, 2024 14:17
@akinomyoga
Copy link
Contributor Author

Thanks!

akinomyoga added a commit to akinomyoga/atuin that referenced this pull request Apr 5, 2024
We have introduced initialization guards in atuinsh#1533 [1], where `return
0` was used to cancel the initialization.  However, this cancels the
processing of the caller (which is typically `~/.bashrc`) instead of
just canceling Atuin's initialization.  In this patch, we avoid using
`return 0`.  Instead, we enclose the main part of the initialization
in a big if-statement.

[1] atuinsh#1533
ellie pushed a commit that referenced this pull request Apr 8, 2024
We have introduced initialization guards in #1533 [1], where `return
0` was used to cancel the initialization.  However, this cancels the
processing of the caller (which is typically `~/.bashrc`) instead of
just canceling Atuin's initialization.  In this patch, we avoid using
`return 0`.  Instead, we enclose the main part of the initialization
in a big if-statement.

[1] #1533
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