[history] Move default location to ~/.local/share/oils

- Bug fix: don't overwrite HISTFILE if exported
- Consistently check (IOError, OSError) in history -a -r
- Respect HISTFILE in main() as well
- Fix history -d spec test

Refactor into ShellFiles object, which we can use for the startup file
too.

User-facing announcement: #1623

core/comp_ui.py

@@ -525,14 +525,15 @@ def ExecutePrintCandidates(display, sub, matches, max_len):
   display.PrintCandidates(sub, matches, max_len)
 
 
-def InitReadline(readline, history_filename, root_comp, display, debug_f):
-  # type: (Optional[Readline], str, completion.RootCompleter, _IDisplay, _DebugFile) -> None
+def InitReadline(readline, hist_file, root_comp, display, debug_f):
+  # type: (Optional[Readline], Optional[str], completion.RootCompleter, _IDisplay, _DebugFile) -> None
   assert readline
 
-  try:
-    readline.read_history_file(history_filename)
-  except (IOError, OSError):
-    pass
+  if hist_file is not None:
+    try:
+      readline.read_history_file(hist_file)
+    except (IOError, OSError):
+      pass
 
   readline.parse_and_bind('tab: complete')
 

core/shell.py

@@ -8,7 +8,7 @@
 
 from _devbuild.gen import arg_types
 from _devbuild.gen.option_asdl import option_i, builtin_i
-from _devbuild.gen.runtime_asdl import cmd_value
+from _devbuild.gen.runtime_asdl import cmd_value, value, value_e
 from _devbuild.gen.syntax_asdl import (
     loc, source, source_t, IntParamBox, CompoundWord
 )
@@ -69,7 +69,7 @@
 
 import posix_ as posix
 
-from typing import List, Dict, Optional, TYPE_CHECKING
+from typing import List, Dict, Optional, TYPE_CHECKING, cast
 
 if TYPE_CHECKING:
   from _devbuild.gen.runtime_asdl import cmd_value, Proc
@@ -296,6 +296,45 @@ def InitAssignmentBuiltins(mem, procs, errfmt):
   return assign_b
 
 
+class ShellFiles(object):
+  def __init__(self, lang, home_dir, mem, flag):
+    # type: (str, str, state.Mem, arg_types.main) -> None
+    self.lang = lang
+    self.home_dir = home_dir
+    self.mem = mem
+    self.flag = flag
+
+  def _DefaultHistoryFile(self):
+    # type: () -> str
+    return os_path.join(
+        self.home_dir, '.local/share/oils/%s_history' % self.lang)
+
+  def InitAfterLoadingEnv(self):
+    # type: () -> None
+
+    if self.mem.GetValue('HISTFILE').tag() == value_e.Undef:
+      # Note: if the directory doesn't exist, GNU readline ignores
+      state.SetGlobalString(self.mem, 'HISTFILE', self._DefaultHistoryFile())
+
+  def HistoryFile(self):
+    # type: () -> Optional[str]
+    # TODO: In non-strict mode we should try to cast the HISTFILE value to a
+    # string following bash's rules
+
+    UP_val = self.mem.GetValue('HISTFILE')
+    if UP_val.tag() == value_e.Str:
+      val = cast(value.Str, UP_val)
+      return val.s
+    else:
+      # Note: if HISTFILE is an array, bash will return ${HISTFILE[0]}
+      return None
+      #return self._DefaultHistoryFile()
+
+      # TODO: can we recover line information here?
+      #       might be useful to show where HISTFILE was set
+      #raise error.Strict("$HISTFILE should only ever be a string", loc.Missing)
+
+
 def Main(lang, arg_r, environ, login_shell, loader, readline):
   # type: (str, args.Reader, Dict[str, str], bool, pyutil._ResourceLoader, Optional[Readline]) -> int
   """The full shell lifecycle.  Used by bin/osh and bin/oil.
@@ -503,9 +542,8 @@ def Main(lang, arg_r, environ, login_shell, loader, readline):
   home_dir = pyos.GetMyHomeDir()
   assert home_dir is not None
 
-  # init the HISTFILE variable to our default history file
-  history_filename = os_path.join(home_dir, '.config/oil/history_%s' % lang)
-  state.SetGlobalString(mem, 'HISTFILE', history_filename)
+  sh_files = ShellFiles(lang, home_dir, mem, flag)
+  sh_files.InitAfterLoadingEnv()
 
   #
   # Initialize builtins that don't depend on evaluators
@@ -529,7 +567,8 @@ def Main(lang, arg_r, environ, login_shell, loader, readline):
 
   # Interactive, depend on readline
   builtins[builtin_i.bind] = builtin_lib.Bind(readline, errfmt)
-  builtins[builtin_i.history] = builtin_lib.History(readline, mem, errfmt, mylib.Stdout())
+  builtins[builtin_i.history] = builtin_lib.History(
+      readline, sh_files, errfmt, mylib.Stdout())
 
   #
   # Initialize Evaluators
@@ -762,8 +801,8 @@ def Main(lang, arg_r, environ, login_shell, loader, readline):
       else:
         display = comp_ui.MinimalDisplay(comp_ui_state, prompt_state, debug_f)
 
-      comp_ui.InitReadline(readline, history_filename, root_comp, display,
-                           debug_f)
+      comp_ui.InitReadline(readline, sh_files.HistoryFile(), root_comp,
+                           display, debug_f)
 
       _InitDefaultCompletions(cmd_ev, complete_builtin, comp_lookup)
 
@@ -800,10 +839,12 @@ def Main(lang, arg_r, environ, login_shell, loader, readline):
       status = mut_status.i
 
     if readline:
-      try:
-        readline.write_history_file(history_filename)
-      except (IOError, OSError):
-        pass
+      hist_file = sh_files.HistoryFile()
+      if hist_file is not None:
+        try:
+          readline.write_history_file(hist_file)
+        except (IOError, OSError):
+          pass
 
     return status
 

osh/builtin_lib.py

@@ -8,6 +8,7 @@
 from _devbuild.gen.runtime_asdl import value, value_e
 from _devbuild.gen.syntax_asdl import loc
 from core import error
+from core import pyutil
 from core import state
 from core import vm
 from core.error import e_usage
@@ -20,6 +21,7 @@
   from _devbuild.gen.runtime_asdl import cmd_value
   from frontend.py_readline import Readline
   from core.ui import ErrorFormatter
+  from core import shell
 
 
 class Bind(vm._Builtin):
@@ -39,30 +41,13 @@ def Run(self, cmd_val):
 class History(vm._Builtin):
   """Show interactive command history."""
 
-  def __init__(self, readline, mem, errfmt, f):
-    # type: (Optional[Readline], state.Mem, ErrorFormatter, mylib.Writer) -> None
+  def __init__(self, readline, sh_files, errfmt, f):
+    # type: (Optional[Readline], shell.ShellFiles, ErrorFormatter, mylib.Writer) -> None
     self.readline = readline
-    self.mem = mem
+    self.sh_files = sh_files
     self.errfmt = errfmt
     self.f = f  # this hook is for unit testing only
 
-  def GetHistoryFilename(self):
-    # type: () -> str
-    # TODO: In non-strict mode we should try to cast the HISTFILE value to a
-    # string following bash's rules
-
-    UP_val = self.mem.GetValue('HISTFILE')
-    if UP_val.tag() == value_e.Str:
-      val = cast(value.Str, UP_val)
-      return val.s
-    else:
-      # TODO: support bash-like behaviour here where we try to convert $HISTFILE
-      # to a string in anyway possible
-
-      # TODO: can we recover line information here?
-      #       might be useful to show where HISTFILE was set
-      raise error.Strict("$HISTFILE should only ever be a string", loc.Missing)
-
   def Run(self, cmd_val):
     # type: (cmd_value.Argv) -> int
     # NOTE: This builtin doesn't do anything in non-interactive mode in bash?
@@ -81,16 +66,33 @@ def Run(self, cmd_val):
       return 0
 
     if arg.a:
-      readline.write_history_file(self.GetHistoryFilename())
+      hist_file = self.sh_files.HistoryFile()
+      if hist_file is None:
+        return 1
+
+      try:
+        readline.write_history_file(hist_file)
+      except (IOError, OSError) as e:
+        self.errfmt.Print_(
+            'Error writing HISTFILE %r: %s' % (hist_file, pyutil.strerror(e)),
+            loc.Missing)
+        return 1
+
       return 0
 
     if arg.r:
-      history_filename = self.GetHistoryFilename()
-      if not path_stat.exists(history_filename):
-        self.errfmt.Print_("HISTFILE %r doesn't exist" % history_filename, loc.Missing)
+      hist_file = self.sh_files.HistoryFile()
+      if hist_file is None:
+        return 1
+
+      try:
+        readline.read_history_file(hist_file)
+      except (IOError, OSError) as e:
+        self.errfmt.Print_(
+            'Error reading HISTFILE %r: %s' % (hist_file, pyutil.strerror(e)),
+            loc.Missing)
         return 1
 
-      readline.read_history_file(history_filename)
       return 0
 
     # Delete history entry by id number

spec/builtin-history.test.sh

@@ -67,53 +67,91 @@ exists
 
 #### HISTFILE must point to a file
 
+rm -f _tmp/does-not-exist
+
 echo '
 HISTFILE=_tmp/does-not-exist
 history -r
-echo $?
+echo status=$?
 ' | $SH -i
 
 # match osh's behaviour of echoing ^D for EOF
 case $SH in bash) echo '^D' ;; esac
 
 ## STDOUT:
-1
+status=1
 ^D
 ## END
 
-#### HISTFILE must be a string
-
-# TODO: we should support bash's behaviour here
+#### HISTFILE set to array
 
 echo '
 HISTFILE=(a b c)
 history -a
-echo $?
+echo status=$?
 ' | $SH -i
 
+case $SH in bash) echo '^D' ;; esac
+
+# note that bash actually writes the file 'a', since that's ${HISTFILE[0]} 
+
 ## STDOUT:
-0
+status=1
+^D
 ## END
-## OK osh STDOUT:
-1
+
+## OK bash STDOUT:
+status=0
+^D
+## END
+
+#### HISTFILE unset
+
+echo '
+unset HISTFILE
+history -a
+echo status=$?
+' | $SH -i
+
+case $SH in bash) echo '^D' ;; esac
+
+## STDOUT:
+status=1
 ^D
 ## END
 
+
 #### history -d to delete history item
 
-# TODO: Respect HISTFILE and fix this test
+export HISTFILE=myhist
+
+$SH --norc -i <<EOF
+
+echo 42
+echo 43
+echo 44
+
+history -a
 
 history -d 1
 echo status=$?
 
-# problem: default for integers is -1
+# Invalid integers
 history -d -1
 echo status=$?
 history -d -2
 echo status=$?
 
+case $SH in bash) echo '^D' ;; esac
+
+EOF
+
 ## STDOUT:
+42
+43
+44
 status=0
-status=1
-status=1
+status=0
+status=0
+^D
 ## END

spec/interactive.test.sh

@@ -309,3 +309,34 @@ $SH --rcfile myrc -i -c 'show-shell-state main'
 # comparisons.
 # The --details flag is useful
 
+
+#### HISTFILE is written in interactive shell
+
+rm -f myhist
+export HISTFILE=myhist
+echo 'echo hist1; echo hist2' | $SH --norc -i
+
+if test -n "$BASH_VERSION"; then
+  echo '^D'  # match OSH for now
+fi
+
+cat myhist
+# cat ~/.config/oil/history_osh
+
+## STDOUT:
+hist1
+hist2
+^D
+echo hist1; echo hist2
+## END
+
+
+#### HISTFILE default value
+
+# it ends with _history
+$SH --norc -i -c 'echo HISTFILE=$HISTFILE' | egrep -q '_history$'
+echo status=$?
+
+## STDOUT:
+status=0
+## END

test/spec_osh.py

@@ -35,7 +35,6 @@ def Define(sp):
   sp.OshFile(
       'builtin-history',
       compare_shells = 'bash',
-      failures_allowed = 1,
       tags = ['interactive'])
 
   sp.OshFile(