[completion] Separate base options and ephemeral/dynamic options.

This was necessary because the base options can CHANGE when the "124
protocol" is invoked.

This manifested as a lack of trailing slash the FIRST time we completed
'.' using bash-completion's _minimal function.  This bug is now fixed.

Addresses issue #208.

bin/oil.py

@@ -143,7 +143,7 @@ def _InitDefaultCompletions(ex, complete_builtin, comp_lookup):
     A1 = completion.TestAction(['foo.py', 'foo', 'bar.py'])
     A2 = completion.TestAction(['m%d' % i for i in xrange(5)], delay=0.1)
     C1 = completion.UserSpec([A1, A2], [], [], lambda candidate: True)
-    comp_lookup.RegisterName('slowc', completion.Options([]), C1)
+    comp_lookup.RegisterName('slowc', {}, C1)
 
 
 def _MaybeWriteHistoryFile(history_filename):

core/completion.py

@@ -115,33 +115,13 @@ def Matches(self, comp):
     return []
 
 
-class Options(object):
-  def __init__(self, opt_changes):
-    self.initial = dict(opt_changes)  # option name -> bool
-    self.ephemeral = {}  # during one completion
-
-  def Reset(self):
-    self.ephemeral.clear()
-
-  def Get(self, opt_name):
-    try:
-      return self.ephemeral[opt_name]
-    except KeyError:
-      return self.initial.get(opt_name, False)  # all default to False
-
-  def Set(self, opt_name, b):
-    self.ephemeral[opt_name] = b
-
-  def __repr__(self):
-    return '(Options %s)' % (self.initial or '')
-
 # NOTE: How to create temporary options?  With copy.deepcopy()?
 # We might want that as a test for OVM.  Copying is similar to garbage
 # collection in that you walk a graph.
 
 
 # These values should never be mutated.
-_DEFAULT_OPTS = Options([])
+_DEFAULT_OPTS = {}
 _DO_NOTHING = (_DEFAULT_OPTS, NullCompleter())
 
 
@@ -152,7 +132,7 @@ def __init__(self):
     # For the IN-PROGRESS completion.
     self.currently_completing = False
     # should be SET to a COPY of the registration options by the completer.
-    self.current_opts = None
+    self.dynamic_opts = None
 
 
 class Lookup(object):
@@ -176,20 +156,20 @@ def __str__(self):
   def PrintSpecs(self):
     """For 'complete' without args."""
     for name in sorted(self.lookup):
-      comp_opts, user_spec = self.lookup[name]
-      print('%-15s %s  %s' % (name, comp_opts, user_spec))
+      base_opts, user_spec = self.lookup[name]
+      print('%-15s %s  %s' % (name, base_opts, user_spec))
     print('---')
     for pat, spec in self.patterns:
       print('%s = %s' % (pat, spec))
 
-  def RegisterName(self, name, comp_opts, user_spec):
+  def RegisterName(self, name, base_opts, user_spec):
     """Register a completion action with a name.
     Used by the 'complete' builtin.
     """
-    self.lookup[name] = (comp_opts, user_spec)
+    self.lookup[name] = (base_opts, user_spec)
 
-  def RegisterGlob(self, glob_pat, comp_opts, user_spec):
-    self.patterns.append((glob_pat, comp_opts, user_spec))
+  def RegisterGlob(self, glob_pat, base_opts, user_spec):
+    self.patterns.append((glob_pat, base_opts, user_spec))
 
   def GetFirstSpec(self):
     return self.lookup['__first']
@@ -208,10 +188,10 @@ def GetSpecForName(self, argv0):
     if user_spec:
       return user_spec
 
-    for glob_pat, comp_opts, user_spec in self.patterns:
+    for glob_pat, base_opts, user_spec in self.patterns:
       #log('Matching %r %r', key, glob_pat)
       if libc.fnmatch(glob_pat, key):
-        return user_spec
+        return base_opts, user_spec
 
     # Nothing matched
     return self.lookup['__fallback']
@@ -816,7 +796,7 @@ def LastColForWord(w):
             yield name
           return
 
-    comp_opts = None
+    base_opts = None
     user_spec = None   # Set below
 
     if trail.words:
@@ -861,9 +841,9 @@ def LastColForWord(w):
           raise AssertionError
         elif n == 1:
           # First
-          comp_opts, user_spec = self.comp_lookup.GetFirstSpec()
+          base_opts, user_spec = self.comp_lookup.GetFirstSpec()
         else:
-          comp_opts, user_spec = self.comp_lookup.GetSpecForName(
+          base_opts, user_spec = self.comp_lookup.GetSpecForName(
               partial_argv[0])
 
         # Update the API for user-defined functions.
@@ -879,14 +859,14 @@ def LastColForWord(w):
 
     # Reset it back to what was registered.  User-defined functions can mutate
     # it.
-    comp_opts.Reset()
-    self.comp_state.current_opts = comp_opts
+    dynamic_opts = {}
+    self.comp_state.dynamic_opts = dynamic_opts
     self.comp_state.currently_completing = True
     try:
       done = False
       while not done:
         try:
-          for entry in self._PostProcess(comp_opts, user_spec, comp):
+          for entry in self._PostProcess(base_opts, dynamic_opts, user_spec, comp):
             yield entry
         except _RetryCompletion as e:
           debug_f.log('Got 124, trying again ...')
@@ -899,16 +879,16 @@ def LastColForWord(w):
             raise AssertionError
           elif n == 1:
             # First
-            comp_opts, user_spec = self.comp_lookup.GetFirstSpec()
+            base_opts, user_spec = self.comp_lookup.GetFirstSpec()
           else:
-            comp_opts, user_spec = self.comp_lookup.GetSpecForName(
+            base_opts, user_spec = self.comp_lookup.GetSpecForName(
                 partial_argv[0])
         else:
           done = True  # exhausted candidates without getting a retry
     finally:
       self.comp_state.currently_completing = False
 
-  def _PostProcess(self, comp_opts, user_spec, comp):
+  def _PostProcess(self, base_opts, dynamic_opts, user_spec, comp):
     """
     Add trailing spaces / slashes to completion candidates, and time them.
 
@@ -937,14 +917,28 @@ def _PostProcess(self, comp_opts, user_spec, comp):
       #m = util.BackslashEscape(m, SHELL_META_CHARS)
       self.debug_f.log('after shell escaping: %s', m)
 
+      # SUBTLE: dynamic_opts is part of comp_state, which ShellFuncAction can
+      # mutate!  So we don't want to pull this out of the loop.
+      opt_filenames = False
+      if 'filenames' in dynamic_opts:
+        opt_filenames = dynamic_opts['filenames']
+      if 'filenames' in base_opts:
+        opt_filenames = base_opts['filenames']
+
       # compopt -o filenames is for user-defined actions.  Or any
       # FileSystemAction needs it.
-      if is_fs_action or comp_opts.Get('filenames'):
+      if is_fs_action or opt_filenames:
         if os_path.isdir(m):  # TODO: test coverage
           yield m + '/'
           continue
 
-      if comp_opts.Get('nospace'):
+      opt_nospace = False
+      if 'nospace' in dynamic_opts:
+        opt_nospace = dynamic_opts['nospace']
+      if 'nospace' in base_opts:
+        opt_nospace = base_opts['nospace']
+
+      if opt_nospace:
         yield m
       else:
         yield m + ' '

core/completion_test.py

@@ -32,7 +32,7 @@
 A1 = completion.TestAction(['foo.py', 'foo', 'bar.py'])
 U1 = completion.UserSpec([A1], [], [], lambda candidate: True)
 
-COMP_OPTS = completion.Options([])
+BASE_OPTS = {}
 
 mem = state.Mem('', [], {}, None)
 
@@ -103,12 +103,12 @@ def _MakeComp(self, words, index, to_complete):
 
   def testLookup(self):
     c = completion.Lookup()
-    c.RegisterName('grep', COMP_OPTS, U1)
+    c.RegisterName('grep', BASE_OPTS, U1)
     print(c.GetSpecForName('grep'))
     print(c.GetSpecForName('/usr/bin/grep'))
 
-    c.RegisterGlob('*.py', COMP_OPTS, U1)
-    comp = c.GetSpecForName('/usr/bin/foo.py')
+    c.RegisterGlob('*.py', BASE_OPTS, U1)
+    base_opts, comp = c.GetSpecForName('/usr/bin/foo.py')
     print('py', comp)
     # NOTE: This is an implementation detail
     self.assertEqual(1, len(comp.actions))
@@ -349,8 +349,8 @@ def testCompletesRedirectArguments(self):
   def testCompletesWords(self):
     comp_lookup = completion.Lookup()
 
-    comp_lookup.RegisterName('grep', COMP_OPTS, U1)
-    comp_lookup.RegisterName('__first', COMP_OPTS, U2)
+    comp_lookup.RegisterName('grep', BASE_OPTS, U1)
+    comp_lookup.RegisterName('__first', BASE_OPTS, U2)
     r = _MakeRootCompleter(comp_lookup=comp_lookup)
 
     comp = MockApi('grep f')

osh/builtin_comp.py

@@ -102,7 +102,7 @@ def __init__(self, ex, parse_ctx, word_ev, splitter):
     self.word_ev = word_ev
     self.splitter = splitter
 
-  def Build(self, argv, arg, comp_opts):
+  def Build(self, argv, arg, base_opts):
     """Given flags to complete/compgen, built a UserSpec."""
     ex = self.ex
 
@@ -202,14 +202,14 @@ def Build(self, argv, arg, comp_opts):
       actions.append(a)
 
     extra_actions = []
-    if comp_opts.Get('plusdirs'):
+    if base_opts.get('plusdirs'):
       extra_actions.append(completion.FileSystemAction(dirs_only=True))
 
     # These only happen if there were zero shown.
     else_actions = []
-    if comp_opts.Get('default'):
+    if base_opts.get('default'):
       else_actions.append(completion.FileSystemAction())
-    if comp_opts.Get('dirnames'):
+    if base_opts.get('dirnames'):
       else_actions.append(completion.FileSystemAction(dirs_only=True))
 
     if not actions and not else_actions:
@@ -266,18 +266,18 @@ def __call__(self, argv):
       self.comp_lookup.PrintSpecs()
       return 0
 
-    comp_opts = completion.Options(arg.opt_changes)
+    base_opts = dict(arg.opt_changes)
     try:
-      user_spec = self.spec_builder.Build(argv, arg, comp_opts)
+      user_spec = self.spec_builder.Build(argv, arg, base_opts)
     except util.ParseError as e:
       # error printed above
       return 2
     for command in commands:
-      self.comp_lookup.RegisterName(command, comp_opts, user_spec)
+      self.comp_lookup.RegisterName(command, base_opts, user_spec)
 
     patterns = []
     for pat in patterns:
-      self.comp_lookup.RegisterGlob(pat, comp_opts, user_spec)
+      self.comp_lookup.RegisterGlob(pat, base_opts, user_spec)
 
     return 0
 
@@ -311,9 +311,9 @@ def __call__(self, argv):
 
     matched = False
 
-    comp_opts = completion.Options(arg.opt_changes)
+    base_opts = dict(arg.opt_changes)
     try:
-      user_spec = self.spec_builder.Build(argv, arg, comp_opts)
+      user_spec = self.spec_builder.Build(argv, arg, base_opts)
     except util.ParseError as e:
       # error printed above
       return 2
@@ -361,11 +361,9 @@ def __call__(self, argv):
       util.error('compopt: not currently executing a completion function')
       return 1
 
-    for name, b in arg.opt_changes:
-      #log('setting %s = %s', name, b)
-      self.comp_state.current_opts.Set(name, b)
+    self.comp_state.dynamic_opts.update(arg.opt_changes)
     #log('compopt: %s', arg)
-    #log('compopt %s', comp_opts)
+    #log('compopt %s', base_opts)
     return 0