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 no-exec commands not making it into the history #1319

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

norpol
Copy link

@norpol norpol commented Oct 21, 2023

Fixes #467
Uses the same as zsh-histdb.
This will fix not adding to the history:

  • when you start a command with # so #echo asdf will be stored in the history, even if you are using zsh setopt interactivecomments
  • when you have a typo in your CLI, it should also now end up in the history, before atuin would only successfully store anything that could be executed

https://github.com/larkery/zsh-histdb/blob/30797f0c50c31c8d8de32386970c5d480e5ab35d/sqlite-history.zsh#L175C14-L175C27

Link to official zsh docs on this

https://zsh.sourceforge.io/Doc/Release/Functions.html#:~:text=is%20being%20executed.-,zshaddhistory,-Executed%20when%20a

@vercel
Copy link

vercel bot commented Oct 21, 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 Nov 3, 2023 1:38pm

@ellie
Copy link
Member

ellie commented Oct 23, 2023

Totally happy to merge this - however I would like to wait until after v17, which I'm releasing this week (now that I am properly home). It's a small change but has a large potential for unknown issues, so I'd like to run it for a while before release.

@norpol
Copy link
Author

norpol commented Oct 27, 2023

For the time being one can add this snippet to their .zshrc as a workaround:

if echo "${preexec_functions}" | grep -qs atuin; then
	add-zsh-hook -d preexec _atuin_preexec
	add-zsh-hook zshaddhistory _atuin_preexec
else
	echo "remove atuin zshaddhistory hook workaround"
fi

@norpol
Copy link
Author

norpol commented Nov 3, 2023

So after running it for two weeks now, the only issue I encounter is that occasionally empty history items make it into the history.

@norpol
Copy link
Author

norpol commented Nov 3, 2023

I've added the check

[[ -n "${${1//[[:space:]]/}//\\/}" ]] || return which would ensure no calls to atuin history start when you'd accidentally type in spaces and empty new-lines.

@@ -12,6 +12,9 @@ autoload -U add-zsh-hook
export ATUIN_SESSION=$(atuin uuid)

_atuin_preexec() {
# skip history for empty calls
# remove space, \t, \n, and literal '\'
[[ -n "${${1//[[:space:]]/}//\\/}" ]] || return
Copy link
Member

Choose a reason for hiding this comment

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

I appreciate why this is here, but would it be possible to implement it rust-side instead? we could then add some tests as well and ensure it doesn't happen for any shell

Copy link
Author

Choose a reason for hiding this comment

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

Would this be the right place to introduce such a check?

let command = command.join(" ");

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.

Add option to include commands that start with # in history
2 participants