Repository: cantino/mcfly (language: rust)

Issue:
Globally set HISTFILE variable with Fish results in fish_history corruption when running a bash subshell
As can be seen in this issue: https://github.com/fish-shell/fish-shell/issues/8349, 'corruption' of the `fish_history` file occurs when using mcfly with Fish.
Due to HISTFILE being set globally with `set -gx`: https://github.com/cantino/mcfly/blob/2daa339f0ee52c5e7b17321c3e64d1fe715d8d6d/mcfly.fish#L12
running `bash` inside a Fish shell using this plugin causes the `bash` shell to inherit the HISTFILE variable which results in it being set to `~/.local/share/fish/fish_history` instead of `~/.bash_history`. Subsequently, bash starts writing in the `fish_history` file which uses a different format and in turn breaks the history until manually fixed.


Discussion / hints:
Thanks @btglr. I don't use Fish. What's the best way to fix this?
Is there a reason you need HISTFILE to be set globally? I added `set -e HISTFILE` (which erases the variable) to my Fish config as recommended in the issue I linked and everything still seems to work fine in regards to your plugin. Fish itself doesn't need that variable to be set.
I suspect that's cruft left over from the bash implementation that's not needed for fish.
As I think about it, this issue is likely to occur whenever you call one shell (without mcfly init code) inside a shell with mcfly init code. Not sure what the best solution is.

Fix patch:
diff --git a/mcfly.fish b/mcfly.fish
index cdbbfe2a..8b69c73c 100644
--- a/mcfly.fish
+++ b/mcfly.fish
@@ -8,11 +8,11 @@ set -g __MCFLY_LOADED "loaded"
 
 # Note: we only use the history file for the session when this file was sourced.
 # Would have to reset this before calling mcfly if you want commands from another session later.
-if not set -q HISTFILE
-  set -gx HISTFILE (set -q XDG_DATA_HOME; and echo $XDG_DATA_HOME; or echo $HOME/.local/share)/fish/(set -q fish_history; and echo $fish_history; or echo fish)_history
+if not set -q MCFLY_HISTFILE
+  set -gx MCFLY_HISTFILE (set -q XDG_DATA_HOME; and echo $XDG_DATA_HOME; or echo $HOME/.local/share)/fish/(set -q fish_history; and echo $fish_history; or echo fish)_history
 end
-if not test -r "$HISTFILE"
-  echo "McFly: $HISTFILE does not exist or is not readable. Please fix this or set HISTFILE to something else before using McFly." >&2
+if not test -r "$MCFLY_HISTFILE"
+  echo "McFly: $MCFLY_HISTFILE does not exist or is not readable. Please fix this or set MCFLY_HISTFILE to something else before using McFly." >&2
   exit 1
 end
 
diff --git a/src/history_cleaner.rs b/src/history_cleaner.rs
index a978399d..9066eae0 100644
--- a/src/history_cleaner.rs
+++ b/src/history_cleaner.rs
@@ -15,10 +15,17 @@ pub fn clean(settings: &Settings, history: &History, command: &str) {
             // directory.
             clean_temporary_files(&settings.mcfly_history, settings.history_format, command);
 
-            // Clean up HISTFILE.
-            let histfile = PathBuf::from(env::var("HISTFILE").unwrap_or_else(|err| {
-                panic!("McFly error: Please ensure that HISTFILE is set ({})", err)
-            }));
+            // Clean up HISTFILE/MCFLY_HISTFILE.
+            let histfile = PathBuf::from(
+                env::var("HISTFILE")
+                    .or_else(|_| env::var("MCFLY_HISTFILE"))
+                    .unwrap_or_else(|err| {
+                        panic!(
+                            "McFly error: Please ensure that HISTFILE/MCFLY_HISTFILE is set ({})",
+                            err
+                        )
+                    }),
+            );
             shell_history::delete_lines(&histfile, settings.history_format, command)
         }
         // Fish integration does not use a MCFLY_HISTORY file because we can get the last command
diff --git a/src/main.rs b/src/main.rs
index 3c9c7978..affcb5c0 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -23,9 +23,16 @@ fn handle_addition(settings: &Settings) {
         );
 
         if settings.append_to_histfile {
-            let histfile = PathBuf::from(env::var("HISTFILE").unwrap_or_else(|err| {
-                panic!("McFly error: Please ensure that HISTFILE is set ({})", err)
-            }));
+            let histfile = PathBuf::from(
+                env::var("HISTFILE")
+                    .or_else(|_| env::var("MCFLY_HISTFILE"))
+                    .unwrap_or_else(|err| {
+                        panic!(
+                            "McFly error: Please ensure that $HISTFILE/$MCFLY_HISTFILE is set ({})",
+                            err
+                        )
+                    }),
+            );
             let command = shell_history::HistoryCommand::new(
                 &settings.command,
                 settings.when_run.unwrap_or(
@@ -63,7 +70,7 @@ fn handle_search(settings: &Settings) {
             out.push('\n');
 
             // Finally, any requests for deletion of commands from shell history, for cases where
-            // shells need to handle this natively instead of through us editing HISTFILE.
+            // shells need to handle this natively instead of through us editing HISTFILE/MCFLY_HISTFILE.
             for delete_request in result.delete_requests {
                 out.push_str("delete ");
                 out.push_str(&delete_request);
diff --git a/src/settings.rs b/src/settings.rs
index 51b38011..84e6d504 100644
--- a/src/settings.rs
+++ b/src/settings.rs
@@ -155,7 +155,7 @@ impl Settings {
                     .takes_value(true))
                 .arg(Arg::with_name("append_to_histfile")
                     .long("append-to-histfile")
-                    .help("Also append new history to $HISTFILE (e.q., .bash_history)"))
+                    .help("Also append new history to $HISTFILE/$MCFLY_HISTFILE (e.q., .bash_history)"))
                 .arg(Arg::with_name("zsh_extended_history")
                     .long("zsh-extended-history")
                     .help("If appending, use zsh's EXTENDED_HISTORY format"))
diff --git a/src/shell_history.rs b/src/shell_history.rs
index 0df2ae60..9e316afc 100644
--- a/src/shell_history.rs
+++ b/src/shell_history.rs
@@ -55,15 +55,19 @@ fn has_leading_timestamp(line: &str) -> bool {
 }
 
 pub fn history_file_path() -> PathBuf {
-    let path = PathBuf::from(env::var("HISTFILE").unwrap_or_else(|err| {
-        panic!(
-            "McFly error: Please ensure HISTFILE is set for your shell ({})",
+    let path = PathBuf::from(
+        env::var("HISTFILE")
+            .or_else(|_| env::var("MCFLY_HISTFILE"))
+            .unwrap_or_else(|err| {
+                panic!(
+            "McFly error: Please ensure HISTFILE or MCFLY_HISTFILE is set for your shell ({})",
             err
         )
-    }));
+            }),
+    );
     fs::canonicalize(&path).unwrap_or_else(|err| {
         panic!(
-            "McFly error: The contents of $HISTFILE appear invalid ({})",
+            "McFly error: The contents of $HISTFILE/$MCFLY_HISTFILE appears invalid ({})",
             err
         )
     })
@@ -241,7 +245,12 @@ pub fn append_history_entry(command: &HistoryCommand, path: &Path, debug: bool)
         .write(true)
         .append(true)
         .open(path)
-        .unwrap_or_else(|err| panic!("McFly error: please make sure HISTFILE exists ({})", err));
+        .unwrap_or_else(|err| {
+            panic!(
+                "McFly error: please make sure HISTFILE/MCFLY_HISTFILE exists ({})",
+                err
+            )
+        });
 
     if debug {
         println!("McFly: Appended to file '{:?}': {}", &path, command);


Test patch: