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

Feature: automatic history management #1940

Merged
merged 4 commits into from
Feb 13, 2022
Merged

Conversation

gaelicWizard
Copy link
Contributor

@gaelicWizard gaelicWizard commented Sep 10, 2021

Description

Two new functions _bash_it_history_auto_save() and _bash_it_history_auto_load(), which append new history to disk and load new history from disk, respectively.

These functions operate based on four modes configured in $HISTCONTROL:

  1. autoshare: append to $HISTFILE when _bash_it_history_auto_save() called, and read from $HISTFILE when _bash_it_history_auto_load() called.
  2. autosave: append to $HISTFILE when _bash_it_history_auto_save() called, but do not do anything when _bash_it_history_auto_load() called.
  3. autoload: do not do anything when _bash_it_history_auto_save() called, but do read from $HISTFILE when _bash_it_history_auto_load() called.
  4. noauto (default): don't do anything.

Motivation and Context

This PR is based on @davidpfarrell's suggestions in and fixes #1595.

_bash_it_history_auto_save() should be added to preexec hook and _bash_it_history_auto_load() should be added to precmd hook, so history is saved immediately and loaded on the next prompt.

Ok, I've refactored slightly. There's four commits. The first three just do a little cleanup on the existing plugins, and the last one adds the "automatic history management" feature.

A few minor notes:

  • Don't use export as it exports the variable to the environment of external binaries, which both adds clutter and alsö can occasionally (ksh?) cause very unexpected weirds including potential loss of the history file. We don't want a newly executed bash or ksh to inherit $HISTFILE but then otherwise set $HISTFILESIZE to anything shorter. readonly only survives the current shell, so make sure not to export.
  • Do use readonly in plugin/history-eternal so that all three variables ($HISTSIZE, $HISTFILESIZE, and $HISTFILE) are all set simultaneously and aren't mis-matched by some clumsy script or plugin somewhere else. This combination ensures that the user's eternal history file is only touched with proper settings configured (or by hand).
  • The two new history management functions both save history (history -a) on every run (unless configured not to). This is due to the fact that the preexec hook sometimes doesn't run due to weirds relating to emulating using the DEBUG hook, so just I save history twice. The cases where the preexec hook doesn't run are nearly instantaneous, so no functionality is lost. This doesn't have a performance penalty as Bash doesn't actually save twice; it doesn't do anything when asked to append to an up-to-date history file. This combination maximizes the synchronicity of multiple shells writing to the file. Both running shells should see and update the file immediately, before and after user input.

I don't think that history management should be handled by the theme. We can set some defaults in plugins/history and let the user customize further. Maybe add to the template?

How Has This Been Tested?

Tested locally, and all tests pass.

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist:

  • My code follows the code style of this project.
  • If my change requires a change to the documentation, I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • If I have added a new file, I also added it to clean_files.txt and formatted it using lint_clean_files.sh.
  • I have added tests to cover my changes, and all the new and existing tests pass.

Copy link
Member

@NoahGorny NoahGorny left a comment

Choose a reason for hiding this comment

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

Seems like a good idea, I'll let other folks chime in 😄

@NoahGorny
Copy link
Member

@davidpfarrell wanna take a look before I merge?

@cornfeedhobo
Copy link
Member

cornfeedhobo commented Sep 29, 2021

@gaelicWizard so far I like this, but do want to do a deeper review after preexec has been dealt with.

As for right now, I'd also like to add "infinite history", but not sure if that belongs in a dedicated plugin.

export HISTFILE=$HOME/.bash_history_unlimited
export HISTSIZE=
export HISTFILESIZE=

By setting a dedicated file, if the user's environment ever gets reset, their history isn't overwritten and truncated when bash defaults back to .bash_history.

@gaelicWizard
Copy link
Contributor Author

(No need for export)

@cornfeedhobo
Copy link
Member

I'd like to revisit this after we have merged a version of cornfeedhobo#1

@gaelicWizard
Copy link
Contributor Author

Man page states Non-numeric values and numeric values less than zero inhibit truncation for $HISTFILESIZE but Numeric values less than zero result in every command being saved on the history list (there is no limit) for $HISTSIZE, so I'm thinking:

HISTFILESIZE=unlimited
HISTSIZE=-1
HISTFILE="${XDG_STATE_HOME:-$HOME/.local/state}/bash_it/history"
  • Use "unlimited" so that meaning is clear if viewed without context.
  • Set the file size to unlimited first, to avoid unintentionally truncating the history file early.
  • I don't understand why the two variables behave differently; maybe it's just documentation phrasing weird...
  • Use $XDG_STATE_HOME

Question: what about setting these read-only?

  • Downside would be needing to reload to disable.
  • Upside would be never truncating the "unlimited history" file.
  • Why: I've seen so so so many things just assume and overwrite variables (which is why I love the : "${VARBL:=value}" construct).

@cornfeedhobo
Copy link
Member

  1. all those bullet points should be comments in the code, because they're great.
  2. I support your idea of setting these as read only. the user can disable the plugin if they are having trouble.

As usual, I really appreciate your attention to detail. great job.

@cornfeedhobo
Copy link
Member

I've read through the linked issue, and I'm kinda stuck on the question of why we are letting themes mess with history at all. I looked through the repo's various prompt_setter functions and I don't see critical reasons for them to be doing that at all. Can someone help me understand?

@gaelicWizard
Copy link
Contributor Author

I've read through the linked issue, and I'm kinda stuck on the question of why we are letting themes mess with history at all. I looked through the repo's various prompt_setter functions and I don't see critical reasons for them to be doing that at all. Can someone help me understand?

I agree completely; my goal here is to fix/reduce/remove themes from managing history.

@gaelicWizard
Copy link
Contributor Author

I've rebased this on the recent cleanup of the history plugins, but I'm going to test it out for a minute to make sure ń̥o҉̙̻̖̲̮͓̩̀t̵̬̳͔ḩ̡̪͇͈i̼̦̮̙͝n̸̸̲̲͍̟g͏̭͖̠̩̭ went funny in the mean time. 😃

@gaelicWizard
Copy link
Contributor Author

gaelicWizard commented Dec 27, 2021

Ok, I've refactored slightly. There's four commits. The first three just do a little cleanup on the existing plugins, and the last one adds the "automatic history management" feature.

A few minor notes:

  • Don't use export as it exports the variable to the environment of external binaries, which both adds clutter and alsö can occasionally (ksh?) cause very unexpected weirds including potential loss of the history file. We don't want a newly executed bash or ksh to inherit $HISTFILE but then otherwise set $HISTFILESIZE to anything shorter. readonly only survives the current shell, so make sure not to export.
  • Do use readonly in plugin/history-eternal so that all three variables ($HISTSIZE, $HISTFILESIZE, and $HISTFILE) are all set simultaneously and aren't mis-matched by some clumsy script or plugin somewhere else. This combination ensures that the user's eternal history file is only touched with proper settings configured (or by hand).
  • The two new history management functions both save history (history -a) on every run (unless configured not to). This is due to the fact that the preexec hook sometimes doesn't run due to weirds relating to emulating using the DEBUG hook, so just I save history twice. The cases where the preexec hook doesn't run are nearly instantaneous, so no functionality is lost. This doesn't have a performance penalty as Bash doesn't actually save twice; it doesn't do anything when asked to append to an up-to-date history file. This combination maximizes the synchronicity of multiple shells writing to the file. Both running shells should see and update the file immediately, before and after user input.

@gaelicWizard gaelicWizard marked this pull request as draft December 29, 2021 07:52
@gaelicWizard
Copy link
Contributor Author

At long last, I'm happy to report that this bad boy is ready to go!

gaelicWizard and others added 4 commits February 8, 2022 16:59
...so the plugin is friendly to variables already marked read-only.
...and hide errors relating to setting already-readonly variables.

`plugin/history-eternal` does not need to force loading after `plugin/history` because both plugins will play nicely with read-only variables, and since we're overwritting and marking read-only then the intended result survives no matter which loads first.

plugin/history-eternal: require Bash v4.3+

Unlimited history is only possible in _Bash_ version 4.3 and up
There's no need for these plugins to load after `plugin/history`. None of the history plugins depend upon each other loading before, after, or at all.
Two new functions `_bash-it-history-auto-save()` and `_bash-it-history-auto-load()`, which append new history to disk and load new history from disk, respectively.

See Bash-it#1595 for discussion.
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.

Make possible to customize themes "history" behavior consistently
3 participants