hstr icon indicating copy to clipboard operation
hstr copied to clipboard

hstr shouldn't use .bash_history as data source and not advertise to use "history -n" in PROMPT_COMMAND

Open calestyo opened this issue 4 years ago • 20 comments

Hey.

Not sure whether the following would really work in the end, but perhaps it's a better way of handling things.

AFAIU there are actually two histories...

  • the in-memory one of each shell process, which is what's actually used by bash itself, e.g. by the built-in-command history, bash's Ctrl-r or UP-key.
  • the history file (typically .bash_history), more or less "shared" by all open bash shell processes and either overwritten or appended to when a shell process quits (or when calling history with some parameters).

Also, AFAIU, there are basically two ways to use hstr:

  1. without PROMPT_COMMAND="history -a; history -n"
  2. or with

Each of them has problems associated:

(1): When not using PROMPT_COMMAND="history -a; history -n", then newly entered commands in this shell won't get appended to the history file until the shell quits. This also means, that any invocation of hstr won't see those commands, which is obviously bad.

(2): When using PROMPT_COMMAND="history -a; history -n", then newly entered commands will get appended to the history file immediately. While this solves the problem of not seeing newly entered commands of this shell it makes things pretty messy when using multiple shells:

Example, with two shells (the comment after each command is the order in which commands were entered in the two shells): Shell I:

$ aaa #1
$ bbb #2
$ ccc #4

Shell II:

$ AAA #3
$ BBB #5

Looking at the actual .bash_history things look as expected:

aaa #1
bbb #2
AAA #3
ccc #4
BBB #5

But looking at the in-memory-histories of the two shells (which is what will also be used for e.g. the UP-key) things are pretty messed up: Shell I:

 5032  aaa #1
 5033  bbb #2
 5034  ccc #4
 5035  ccc #4

Shell II:

 5032  AAA #3
 5033  bbb #2
 5034  AAA #3
 5035  BBB #5
 5036  BBB #5

Some entries are doubled, some are even missing. Strangely they're still missing when entering further commands (so it's not just one further missing reading of the history file), guess that's because of how bash appends/merges the history to/from the history file.

The whole thing can get even quite disastrous when people don't expect this behaviour, especially when e.g. multiple people use the same user account (role accounts like root), consider in chronological order e.g.: User A: $ date User A: # presses UP key, gets date again, presses ENTER User B: $ cd tmp User B: $ rm -rf * User A: # presses UP key, gets date again, presses ENTER

It's not so unlikely, that A does this so fast (not expecting rm -rf might show up, that he actually executesrm -rf * wherever he is.

A partial solution for this might be to not advertise people to use: PROMPT_COMMAND="history -a; history -n" but rather just PROMPT_COMMAND="history -a" that way changes from other shells wouldn't be read in again (only for newly started shells).

This has however still the behaviour that commands are immediately appended to the history file, which has the following properties: Since hstr uses the history file as data source, commands from other shells will show up immediately in this shell This can be both, an advantage:

  • one will get commands from other shells in the history

and disadvantage:

  • one will get commands from other shells in the history (in the sense, it clutters mixes up the linear history (e.g. when using the raw-history view). Also it can again lead to unexpected results, when doing quick incremental searches. User A: might expect "Ctrl-r f ENTER" to execute his most recently entered "firefox --profile foo & exit", while he'll actually get User B's "rm -rf" which was more recent in the history file.

I think the disadvantage overweights by far,... it's effectively the same problem as above just with hstr and not the UP key or Bash's Ctrl-r (after all, a user might have kept Bash's Ctrl-r for use and bound hstr to another key).

I think the following could possibly solve this: Don't use the history file at all as a data source, except perhaps when deleting commands from it. Instead have someting like like "history | hstr --" bound to the key. hsrt would then need to read the history from stdin (removing a portion of something like "^\(\*\|\)[[:digit:]]* *" from it's beginning - the history entry numbers might be prefixed with *).

The same would need to be done for directly invoking hstr, e.g. something like alias hstr='history | hstr'

Has the advantage of not getting messed up with history file manipulations of other shells, while still seeing new commands in this shell. No need to set any PROMPT_COMMAND at all, no need to sync the file after each command (which can easily cause defragmentation especially on CoW filesystems).

Deleting an entry would also need to be done via the buil-tin history -d offset and no longer affect the histories of other shells unless these are re-loaded. But I think it would have the advantage of not having to write to the history file, which bash probably doesn't expect anyone elese to ever do.

Cheers, Chris.

calestyo avatar Apr 21 '20 00:04 calestyo

Oh and in addition: I'm not really sure whether aliases/keybindings like history | hstr (i.e. ones that contain a pipe) are really safe in bash.

I cannot think of a counter example right now but maybe when the alias is then used in combination with other shell syntax like compound commands. In any case one must of course prevent, that the history commands are all executed, which happened to me when I tried something like: #do-not-execute----alias cat='cat < $(history) | tail' (cat serving as a hstr replacement)

I forgot the "" around $(history) an the first history line was take as file for the redirect and the remainder of history was executed... while many of that will fail (since it starts with a number)... lines containing a & will cause that to be executed... lucky me, the first one was a & exit.

So perhaps it would be better to use constructs like: hstr <(history) that way, the worst, I can think of, that can happen is that something gets a fd with the history as content.

calestyo avatar Apr 21 '20 00:04 calestyo

Thank you for the comprehnesive analysis Christoph!

You are right, sending history command output to hstr is better solution. I actually planned to implement pipe support in the next release https://github.com/dvorka/hstr/milestone/12, however, I originally I didn't want to change how hstr works.

I will redesign hstr bash/zsh integration as you suggested in the next release. Overall I think that history-based design is much simpler and more robust (append & reload feels like hack, finding the right bash/zsh history file on different distros/installation is not straightforward and there are other problems as well).

I really appreciate your interest in hstr, time you invested into current design drawbacks analysis and making it Debian package. Thank you again!

dvorka avatar Apr 21 '20 20:04 dvorka

Thanks for your kind words :-)

The problem with using the history file is generally that the shell probably doesn't expect anyone to do this. So there might be all kinds of weird bugs, e.g. when the shell truncates the file and so on. I'd say even for deleting entries, hstr should directly edit the history file.

Before you invest a lot of time in redesigning, perhaps it makes sense to reach out for bash/zsh upstream, whether they'd be interested in you directly integrating hstr into the respective shell. Of course I have no idea how much more of an effort that would be... but could have some advantages in terms of features.

Oh and btw: It wasn't me who packaged hstr for Debian, the maintainer is Daniel Echeverri.

calestyo avatar Apr 22 '20 02:04 calestyo

Great to know this is on the roadmap. Before finding this issue, I was thinking of a similar idea, sending just the in-memory history in through a pipe and appending that to the data from the history file. Not using the file directly at all seems cleaner though.

I want this because the idea of writing to my SSD every time I hit enter makes me twitchy. A silly concern, most likely, but hey.

chewi avatar Nov 01 '20 21:11 chewi

just wondered, whether this is still considered? :-) @dvorka

calestyo avatar Feb 17 '21 02:02 calestyo

Just to add to the conversation, there are more edge cases to keep in mind if you're going to implement this. The output of history can also contain an arbitrarily-formatted timestamp as defined by the HISTTIMEFORMAT env var.

I suggest taking a look at what fzf is doing to address this in their default CTRL+R binding: https://github.com/junegunn/fzf/blob/a4bc08f5a35da6e374a2ce0da4c931d7fd32fdf1/shell/key-bindings.bash#L47

For example, they use the fc builtin instead of the history builtin, which allows for omitting the line numbers and ignoring HISTTIMEFORMAT.

iamjackg avatar May 28 '21 16:05 iamjackg

Isn't that also already the case when the history is read from the file?

calestyo avatar Jun 03 '21 01:06 calestyo

Temporary workaround: history -w ~/.bash_history.d/$$-temp && HISTFILE=~/.bash_history.d/$$-temp hstr -- && rm ~/.bash_history.d/$$-temp

Gooberpatrol66 avatar Jan 10 '22 05:01 Gooberpatrol66

But then one effectively looses persistent history?!

calestyo avatar Jan 10 '22 17:01 calestyo

I have a somewhat elaborate setup where history is being read from somewhere else in bashrc and processed. I assumed that was the point of the thread, doing advanced things with the history and still having hstr work.

Gooberpatrol66 avatar Jan 10 '22 18:01 Gooberpatrol66

@calestyo @Gooberpatrol66 Hey hello! Yes, I think that this feature makes sense (it might be either configurable option OR replace existing implementation), however, I'm unfortunately busy and don't have enough time to implement it. Apologies!

dvorka avatar Jan 11 '22 18:01 dvorka

We received a related report at https://github.com/ohmybash/oh-my-bash/discussions/422. The strange behavior that we observed there turned out to be caused by the settings instructed by hstr.

I'm not sure what is the original intent of hstr, but at least history -a; history -n won't work in a sane way unfortunately. It will duplicate entries of the current session randomly. It randomly picks up history entries added by other Bash sessions in a non-predictable way.

Possible solutions are:

  • If hstr wants to just reference the history entries from the history file, history -a is sufficient.
  • If hstr wants to also offer the shared-history feature, history -a; history -c; history -r should be used.

The harm of history -a; history -n and the trick history -a; history -c; history -r are described in the following links in detail:

https://github.com/dvorka/hstr/issues/400#issuecomment-617397408

I will redesign hstr bash/zsh integration as you suggested in the next release.

After three years, I still find history -a; history -n in the codebase. I'm not sure if hstr has completed the redesign or not, but if it would have been already completed and the history manipulation would not be needed anymore, could we just remove these hooks to PROMPT_COMMAND?

Overall I think that history-based design is much simpler and more robust

As described above, it is not robust and causes a problem, unfortunately.

akinomyoga avatar Apr 03 '23 23:04 akinomyoga

Hey @akinomyoga.

Every time I've "met" you so far, you proved have extremely deep knowledge of bash and the stuff around it ... so most likely what you say is what should be done, but I don't quite understand it ^^

Possible solutions are: If hstr wants to just reference the history entries from the history file, history -a is sufficient.

This is the "partial solution" I've looked at in the original report.

  • if really used alone (i.e. no history -r or history -n) it should not cause any messing with bash’s history (as nothing is re-read, unless you start a fresh new bash - where this is expected)
  • however, it as hstr reads in its data directly from the file it may affect that, with the possible advantages and disadvantages I've described above

If hstr wants to also offer the shared-history feature, history -a; history -c; history -r should be used.

  • same problems here, since the history file is always written, other instances of hstr (in other instances of bash) will be affected by that (which may have disadvantages)... and since history -r is done, couldn't that cause just the problematic behaviour I've described above? Another shell may overwrite the history file after the history -a from the current shell, the current would -read that of the other and then the history contents would (as described in the examples of my original report) contain unexpected entries.

I still think the main problem here is that hstr uses the history file as data source. It should use the "in-memory" history of the respective shell - and not use any history -X at all and never read/write the history file either.

calestyo avatar Apr 04 '23 00:04 calestyo

Also, I don't think anything has been done in the meantime to get that fixed.

Overall I think that history-based design is much simpler and more robust

As described above, it is not robust and causes a problem, unfortunately.

What do you think would be the problem if using the output from history, i.e. the "in-memory" history?

calestyo avatar Apr 04 '23 00:04 calestyo

Overall I think that history-based design is much simpler and more robust

As described above, it is not robust and causes a problem, unfortunately.

What do you think would be the problem if using the output from history, i.e. the "in-memory" history?

Ah, OK. So "the history-based design" means the in-memory version but not the history -a; history -n approach? I meant the latter version of history -a; history -n. I agree that the in-memory version like history | hstr ... would be more robust.

akinomyoga avatar Apr 04 '23 02:04 akinomyoga

This seems like a more appropriate issue to point out a really nice feature that I have heard many would find very useful - timestamps next to the command history indicating the most recent time the command was executed. This was also mentioned in #128, at least I think that's what they are asking for.

Reading this issue makes you realize the built in bash history is pretty limited considering the features that could be added to hstr if they were supported by it, such as:

  • storing first execution date, # times executed, etc., current working directory of execution
  • storing the result of execution if non-zero and coloring or hiding failed commands
  • any features that require information not stored in the history file

rpm5099 avatar Aug 24 '23 16:08 rpm5099

This doesn't seem to be related to the issue originally discussed here, or is it?

I guess it might make more sense to have this in a separate issue.

calestyo avatar Aug 24 '23 20:08 calestyo

This doesn't seem to be related to the issue originally discussed here, or is it?

I guess it might make more sense to have this in a separate issue.

Because timestamps are not automatically stored in the bash history if the method of accessing and updating the history is re-tooled to in-memory methods then support for timestamps would likely need to be factored into that. I have not fully researched the functionality of bash history timestamps, so you could be correct @calestyo that this belongs in a separate issue.

rpm5099 avatar Aug 24 '23 21:08 rpm5099

btw: Until this is fixed in hstr, fzf and it's "Ctrl-R history mode" can be used as a somewhat similar alternative.

calestyo avatar Sep 24 '23 00:09 calestyo

To not have hstr append history items from each terminal immediately to disk (PROMPT_COMMAND="history -a; history -n), but read the history from the in-memory history of terminal it's running in, you can use something like:


function _hstr() {
    fc -ln -2147483648 |
      sed 's/^[ \t]\+ //' \
      >~/.hstr.tmp

    HISTFILE=~/.hstr.tmp hstr "${@}"
    rm - ~/.hstr.tmp
}

#export PROMPT_COMMAND="history -a; history -n; ${PROMPT_COMMAND}"
if [[ $- =~ .*i.* ]]; then bind '"\C-r": "\C-a _hstr -- \C-j"'; fi
# if this is interactive shell, then bind 'kill last command' to Ctrl-x k
if [[ $- =~ .*i.* ]]; then bind '"\C-xk": "\C-a _hstr -k \C-j"'; fi`

Notice the usage of _hstr in the bind command and the commenting of the PROMPT_COMAND.

fzf is good, but inferior to hstr for searching history conveniently.

Amain avatar Mar 16 '24 21:03 Amain