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

Guard against ancient versions of bash (3.2) where Atuin does not work. #1794

Merged
merged 1 commit into from Mar 1, 2024

Conversation

fragmede
Copy link
Contributor

@fragmede fragmede commented Feb 29, 2024

Hi.

Checks

  • [✔️ ] I am happy for maintainers to push small adjustments to this PR, to speed up the review cycle
  • [✔️ ] I have checked that there are no existing pull requests for the same thing
    • I searched issues for "bash 3.2" and got nothing relevant.

I'm on OS X, version Sonoma 14.2.1 (23C71), which ships an ancient version of bash (3.2).

Atuin doesn't work under bash 3.2. it runs when I hit "up", but then when I hit enter, the command doesn't get put into the line to be run if I hit enter a second time.

Of course, the first thing to do with that is to upgrade to a recent version of bash using homebrew.

Unfortunately, that doesn't always take fully, resulting in bash 3.2 getting run... sometimes.

This adds a guard to install.sh, erroring out, saying that Atuin doesn't work in bash 3.2.

@fragmede fragmede changed the title Guard against ancient versions of bash (3.) where Atuin does not work. Guard against ancient versions of bash (3.2) where Atuin does not work. Feb 29, 2024
@akinomyoga
Copy link
Contributor

Bash 3.2 is supported by Atuin. As far as I test it, Atuin works with both Bash 3.2/bash-preexec and Bash 3.2/ble.sh.

the command doesn't get put into the line to be run if I hit enter a second time.

I guess you set enter_accept = false because otherwise there is no second enter. If that is the case, you missed "PLEASE NOTE" section on README:

(Quoted from PLEASE NOTE in Bash section in README)

To use Atuin in bash < 4 with bash-preexec, the option enter_accept needs to be turned on (which is so by default).

install.sh Outdated Show resolved Hide resolved
@fragmede
Copy link
Contributor Author

fragmede commented Feb 29, 2024

If that is the case, you missed "PLEASE NOTE" section on README:

I did miss that, but it doesn't seem to be the default.

If I run /bin/bash --noprofileit doesn't work (mac osx sonoma 14.2.1, nor does it work if I run set enter_accept=true in bash before setting up atuin.

I'm also not finding much online imageabout this option.

@akinomyoga
Copy link
Contributor

enter_accept is Atuin's option. It's not Bash's option. It's turned on in Atuin's default configuration automatically created at ~/.config/atuin/config.toml. But, if the file creation is blocked by some permissions, etc, the config may not be turned on.

nor does it work if I run set enter_accept=true in bash before setting up atuin.

Also, set x=y is not the way to set a config. set arg sets $1 to be arg. You are just assigning the string "enter_accept=true" to $1, which doesn't have any effect.

@fragmede
Copy link
Contributor Author

(oops yeah I meant export not set)

Clearing out ~/.config/atuin/config.toml gets it to work for me under bash 3.2. I installed atuin a while ago, before the config file set that option to true by default.

Updated the text slightly.

@akinomyoga
Copy link
Contributor

(oops yeah I meant export not set)

Ah, I see. Then, that makes more sense.

Clearing out ~/.config/atuin/config.toml gets it to work for me under bash 3.2. I installed atuin a while ago, before the config file set that option to true by default.

I see. The option was recently added.

Updated the text slightly.

Have you reverted it again? After force-pushing, nothing seems to be changed from the very initial version.

@ellie
Copy link
Member

ellie commented Feb 29, 2024

Is this PR still required? Atuin does work under bash 3.2. Perhaps not as well as with later versions, but it works nonetheless. Looks like this was over some config issues?

@akinomyoga
Copy link
Contributor

akinomyoga commented Feb 29, 2024

This PR is not required. I even think it shouldn't be merged in the current state.

However, I thought it could be useful if it is updated to notify the users of the limitation instead of canceling the installation. I suggested that, and the author @fragmede seemed to accept the suggestion, but then the author reverted the applied change to the original state. I'm not sure what is the author's intent. We can wait for the author's reply.

install.sh Outdated Show resolved Hide resolved
@ellie
Copy link
Member

ellie commented Mar 1, 2024

Thank you both!

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.

Seeing as this is your first time contributing, if you would like a holographic contributors-only Atuin sticker, then please fill out this form!

We do also have a Discord if you'd like to ask any questions, or just fancy hanging out!

@ellie ellie merged commit 515f78f into atuinsh:main Mar 1, 2024
15 checks passed
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.

None yet

3 participants