cantino / mcfly

Fly through your shell history. Great Scott!
MIT License
6.77k stars 176 forks source link

BUG: McFly exporting `HISTFILE` can cause conflicts between shells #249

Open dithpri opened 2 years ago

dithpri commented 2 years ago

I pretty much exclusively use zsh. However, I sometimes need to run bash (or run it from nix-shell). I've had no problems with this until I installed McFly.

Since McFly exports HISTFILE when loaded in an interactive zsh shell: https://github.com/cantino/mcfly/blob/75808a60c78f65e90767ee53c9468086fcdf0cc5/mcfly.zsh#L13=

If bash is run from such a shell, because HISTFILE is exported, bash's HISTFILE is the same as zsh's. As it happens, the default history file size for bash is 500. So, every time I run bash, my zsh history file gets truncated:

$ wc -l "$HISTFILE"
7475 /home/dith/.local/share/zsh/history
$ bash -i =(echo true)
$ wc -l "$HISTFILE"
501 /home/dith/.local/share/zsh/history

Moreover, bash saves its history to the zsh HISTFILE...

The workaround is to set a different HISTFILE in .bashrc, but this is most definitely a bug introduced by McFly exporting the HISTFILE variable. McFly should honour the variable not being exported and define its own variables for internal use instead of exporting ones that don't belong to it.

Suggestion: export MCFLY_HISTFILE="${HISTFILE:-$HOME/.zsh_history}" (as it seems to be done for fish)

Same goes for the bash script https://github.com/cantino/mcfly/blob/75808a60c78f65e90767ee53c9468086fcdf0cc5/mcfly.bash#L13=

cantino commented 2 years ago

If you change mcfly.bash to something like the following, does everything start working for you?

# Ensure MCFLY_HISTFILE exists.
default_histfile="${HISTFILE:-$HOME/.bash_history}"
export MCFLY_HISTFILE="${MCFLY_HISTFILE:-$default_histfile}"
if [[ ! -r "${MCFLY_HISTFILE}" ]]; then
  echo "McFly: ${MCFLY_HISTFILE} does not exist or is not readable. Please fix this or set MCFLY_HISTFILE to something else before using McFly."
  return 1
fi
dithpri commented 2 years ago

No, that doesn't work, because HISTFILE is exported in mcfly.zsh (the issue I'm having is running bash from a zsh shell).

But changing mcfly.zsh to a similar snippet:

# Ensure MCFLY_HISTFILE exists.
default_histfile="${HISTFILE:-$HOME/.zsh_history}"
export MCFLY_HISTFILE="${MCFLY_HISTFILE:-$default_histfile}"
if [[ ! -r "${MCFLY_HISTFILE}" ]]; then
  echo "McFly: ${MCFLY_HISTFILE} does not exist or is not readable. Please fix this or set MCFLY_HISTFILE to something else before using McFly."
  return 1
fi

does fix this for me.

I think both your suggestion and this one will have to be applied so that it works both ways (so that zsh and bash don't share the same HISTFILE unless set so by the user in both shells' rcs.

This however does not update MCFLY_HISTFILE (but that worked the same way before) when a sub-shell is run, but that's a separate, related, bug.

cantino commented 2 years ago

That makes sense. We should just make this change everywhere. Would you be up for making it, or should I?

dithpri commented 2 years ago

I'll submit a pull request sometime this week.

AndrewKvalheim commented 1 year ago

I tried McFly for about five minutes today and this led to the silent deletion of years of my shell history.

cantino commented 1 year ago

I'm sorry to hear that Andrew, we'll prioritize getting this fixed.