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

fix(bash): fix small issues of enter_accept for the plain Bash #1467

Merged
merged 5 commits into from
Jan 1, 2024

Conversation

akinomyoga
Copy link
Contributor

This PR resolves small issues of enter_accept for Bash without ble.sh. There are five different fixes, and again the details are in the commit messages. The original author @arcuru might have comments.

I plan to submit another PR for refactoring and a small performance improvement (which contains again 5 commits). If I should separate the PR into smaller chunks, please let me know. In such a case, I can re-submit those commits in separate PRs.

With the current implementation, the user command "--" would not be
executed even if it were the intended one.  This is because it would
be confused as an option by the "eval" builtin.  We can specify "--"
to tell "eval" that the later arguments should be literally treated as
the command.
The exit status of preexec_ret_value is used to suppress the execution
of the corresponding command in the extdebug mode.  The current
implementation somehow tries to set $? before the call of stty, which
does not have any effect.  Instead, we can manually turn off the
execution of the user command when the condition matches.
Copy link

vercel bot commented Dec 29, 2023

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 Dec 29, 2023 0:20am

@akinomyoga
Copy link
Contributor Author

akinomyoga commented Dec 29, 2023

Not directly related to the fixes in this PR, but we might separate the related code into a new function __atuin_accept_line for the refactoring because most lines of __atuin_history are somehow the accept-line emulation despite the function name. What do you think?

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 for all the fixes! And I appreciate how nicely broken down the commits here are 🙏

but we might separate the related code into a new function __atuin_accept_line for the refactoring because most lines of __atuin_history are somehow the accept-line emulation despite the function name. What do you think?

I think that would make sense. I don't feel strongly enough that it should block this PR, but it would be nice

@ellie ellie merged commit 1350d3f into atuinsh:main Jan 1, 2024
9 checks passed
@akinomyoga akinomyoga deleted the fix-enter_accept-bash branch January 1, 2024 14:29
@akinomyoga
Copy link
Contributor Author

I think that would make sense. I don't feel strongly enough that it should block this PR, but it would be nice

Thank you for your reply! Then, should I submit a PR for __atuin_accept_line?

As I mentioned in the first comment, I'll submit a PR for additional refactoring, but I suggest separating the __atuin_accept_line PR from the refactoring PR because the history tracking of git blame would be discontinued if we mix the code movement and changes in a single commit.

@ellie
Copy link
Member

ellie commented Jan 1, 2024

Yes please! Sounds good to me 😊

@akinomyoga
Copy link
Contributor Author

Thanks!

@akinomyoga
Copy link
Contributor Author

I created PR #1476 for __atuin_accept_line.

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