[Bug-readline] vi-mode: fix relationship of `.' to f/F/t/T

[Richard Todd]

Hi all,

I believe this is a vi-mode POSIX issue, this time:

Let's take the following line:

    'ball' 'animal' 'angle'

With the cursor at the front, in vim's compatibility mode, I can issue the
following commands (with spaces added only for clarity here):

    fb dt' fa . ; . 

... and I will be left with three sets of quotes:

    '' '' ''

This sequence illustrates two features not present in readline's vi-mode:

  1) Since the `fa' command will overwrite _rl_vi_last_search_char, the 
following `.'
     command doesn't remember it should delete up to a single-quote.

     The POSIX section for "[count] ."  states: "Repeated commands with 
associated
     motion commands shall repeat the motion command as well".  The dot should
     repeat dt' regardless of any intervening t/T/f/F commands.

  2) Since the `.' command overwrites _rl_cs_dir to FTO, the next `;' command
     has forgotten that it is repeating a FFIND command (fa).

     The POSIX section for "[count] ."  states: "If the motion component of the
     repeated command is f, F, t, or T, the repeated command shall not set the
     remembered search character for the ; and , commands."

     I note that, strictly speaking, it only talks about not setting the search
     character, and doesn't specify the interaction between the t/T/f/F command
     with subsequent `;'.  But, vim-compatibility-mode keeps `.' and `;' fully
     independent, and I think that is the least surprising interpretation from
     a user's perspective.  nvi disagrees with vim here, though, so maybe it is
     an extension after all.

(I'm using http://pubs.opengroup.org/onlinepubs/9699919799/utilities/vi.html as
my reference)

I've altered my local copy so that rl_vi_char_search() tracks when it is
running as the motion part of a d/c/y command, and stores the search character
to a new variable (_rl_vi_last_search_motion_char), so that it can be used as
part of redo commands.  That takes care of issue (1).

Second, I've simplified the handling of _rl_cs_dir (no more need for the _orig_
variant), and made certain not to overwrite it in the case of a redo command.
This takes care of issue (2).

I tested this with multibyte on and off. I'm honestly not sure how to test the
"callback mode" code path, though I did attempt to make the correct changes for
that as well.

Patch against plain git devel is below, if you are interested.  I set 
RL_STATE_VIMOTION even when we aren't using callback-mode, so that the search
code can always tell if it is servicing c/d/y commands. It seems innocuous, but
it also felt slightly wrong since vi-mode seems to only use those states to
keep track of where it is in the face of select()-style callbacks. But my main
goal was to demonstrate the type of changes I think would help without
disturbing the existing code much.


diff --git a/vi_mode.c b/vi_mode.c
index d6fa38e..3b50855 100644
--- a/vi_mode.c
+++ b/vi_mode.c
@@ -108,11 +108,15 @@ static int vi_insert_buffer_size;
 static int _rl_vi_last_repeat = 1;
 static int _rl_vi_last_arg_sign = 1;
 static int _rl_vi_last_motion;
+static int _rl_cs_dir;
 #if defined (HANDLE_MULTIBYTE)
 static char _rl_vi_last_search_mbchar[MB_LEN_MAX];
 static int _rl_vi_last_search_mblen;
+static char _rl_vi_last_search_motion_mbchar[MB_LEN_MAX];
+static int _rl_vi_last_search_motion_mblen;
 #else
 static int _rl_vi_last_search_char;
+static int _rl_vi_last_search_motion_char;
 #endif
 static char _rl_vi_last_replacement[MB_LEN_MAX+1];     /* reserve for trailing 
NULL */
 
@@ -1375,6 +1379,7 @@ int
 rl_vi_delete_to (int count, int key)
 {
   int c, r;
+  RL_SETSTATE (RL_STATE_VIMOTION);
 
   _rl_vimvcxt = _rl_mvcxt_alloc (VIM_DELETE, key);
   _rl_vimvcxt->start = rl_point;
@@ -1401,7 +1406,6 @@ rl_vi_delete_to (int count, int key)
 #if defined (READLINE_CALLBACKS)
   else if (RL_ISSTATE (RL_STATE_CALLBACK))
     {
-      RL_SETSTATE (RL_STATE_VIMOTION);
       return (0);
     }
 #endif
@@ -1464,6 +1468,8 @@ rl_vi_change_to (int count, int key)
 {
   int c, r;
 
+  RL_SETSTATE (RL_STATE_VIMOTION);
+
   _rl_vimvcxt = _rl_mvcxt_alloc (VIM_CHANGE, key);
   _rl_vimvcxt->start = rl_point;
 
@@ -1489,7 +1495,6 @@ rl_vi_change_to (int count, int key)
 #if defined (READLINE_CALLBACKS)
   else if (RL_ISSTATE (RL_STATE_CALLBACK))
     {
-      RL_SETSTATE (RL_STATE_VIMOTION);
       return (0);
     }
 #endif
@@ -1531,6 +1536,8 @@ rl_vi_yank_to (int count, int key)
 {
   int c, r;
 
+  RL_SETSTATE (RL_STATE_VIMOTION);
+
   _rl_vimvcxt = _rl_mvcxt_alloc (VIM_YANK, key);
   _rl_vimvcxt->start = rl_point;
 
@@ -1556,7 +1563,6 @@ rl_vi_yank_to (int count, int key)
 #if defined (READLINE_CALLBACKS)
   else if (RL_ISSTATE (RL_STATE_CALLBACK))
     {
-      RL_SETSTATE (RL_STATE_VIMOTION);
       return (0);
     }
 #endif
@@ -1731,29 +1737,46 @@ rl_vi_first_print (int count, int key)
   return (rl_vi_back_to_indent (1, key));
 }
 
-static int _rl_cs_dir, _rl_cs_orig_dir;
-
 #if defined (READLINE_CALLBACKS)
 static int
 _rl_vi_callback_char_search (_rl_callback_generic_arg *data)
 {
   int c;
+  _rl_cs_dir = data->i1;
+
 #if defined (HANDLE_MULTIBYTE)
-  c = _rl_vi_last_search_mblen = _rl_read_mbchar (_rl_vi_last_search_mbchar, 
MB_LEN_MAX);
+  c = _rl_read_mbchar (_rl_vi_last_search_mbchar, MB_LEN_MAX);
+  if (c <= 0)
+    {
+      RL_UNSETSTATE (RL_STATE_CHARSEARCH);
+      return -1;
+    }
+  _rl_vi_last_search_mblen = c;
+  /* When reading a c, d, or y command, remember the char
+     separately for redo purposes, since another [tTfF] search
+     could happen before the user does the '.' redo. */
+  if (RL_ISSTATE(RL_STATE_VIMOTION))
+    {
+      memcpy(_rl_vi_last_search_motion_mbchar,
+       _rl_vi_last_search_mbchar,
+       MB_LEN_MAX);
+      _rl_vi_last_search_motion_mblen = c;
+    }
 #else
   RL_SETSTATE(RL_STATE_MOREINPUT);
   c = rl_read_key ();
   RL_UNSETSTATE(RL_STATE_MOREINPUT);
-#endif
-
-  if (c <= 0)
+  if (c < 0)
     {
       RL_UNSETSTATE (RL_STATE_CHARSEARCH);
       return -1;
     }
-
-#if !defined (HANDLE_MULTIBYTE)
   _rl_vi_last_search_char = c;
+  /* When reading a c, d, or y command, remember the char
+     separately for redo purposes, since another [tTfF] search
+     could happen before the user does the '.' redo. */
+  if (RL_ISSTATE(RL_STATE_VIMOTION))
+    _rl_vi_last_search_motion_char = c;
 #endif
 
   _rl_callback_func = 0;
@@ -1764,7 +1787,7 @@ _rl_vi_callback_char_search (_rl_callback_generic_arg 
*data)
   return (_rl_char_search_internal (data->count, _rl_cs_dir, 
_rl_vi_last_search_mbchar, _rl_vi_last_search_mblen));
 #else
   return (_rl_char_search_internal (data->count, _rl_cs_dir, 
_rl_vi_last_search_char));
-#endif  
+#endif
 }
 #endif
 
@@ -1772,56 +1795,73 @@ int
 rl_vi_char_search (int count, int key)
 {
   int c;
+  int cs_dir;
+#if defined (HANDLE_MULTIBYTE)
+  char *target;
+  int *tlen;
+#else
+  int *target;
+#endif
+
+  /* Initially point the target to the last search. In the case
+     of _rl_vi_redoing it will be changed later to the last
+     motion command, EXCEPT when `key' is ; or , */
 #if defined (HANDLE_MULTIBYTE)
-  static char *target;
-  static int tlen;
+  target = _rl_vi_last_search_mbchar;
+  tlen = &_rl_vi_last_search_mblen;
 #else
-  static char target;
+  target = &_rl_vi_last_search_char;
 #endif
 
   if (key == ';' || key == ',')
     {
-      if (_rl_cs_orig_dir == 0)
+      if (_rl_cs_dir == 0)
        return 1;
 #if defined (HANDLE_MULTIBYTE)
-      if (_rl_vi_last_search_mblen == 0)
+      if (*tlen == 0)
        return 1;
 #else
-      if (_rl_vi_last_search_char == 0)
+      if (*target == 0)
        return 1;
 #endif
-      _rl_cs_dir = (key == ';') ? _rl_cs_orig_dir : -_rl_cs_orig_dir;
+      cs_dir = (key == ';') ? _rl_cs_dir : -_rl_cs_dir;
     }
   else
     {
       switch (key)
        {
        case 't':
-         _rl_cs_orig_dir = _rl_cs_dir = FTO;
+         cs_dir = FTO;
          break;
 
        case 'T':
-         _rl_cs_orig_dir = _rl_cs_dir = BTO;
+         cs_dir = BTO;
          break;
 
        case 'f':
-         _rl_cs_orig_dir = _rl_cs_dir = FFIND;
+         cs_dir = FFIND;
          break;
 
        case 'F':
-         _rl_cs_orig_dir = _rl_cs_dir = BFIND;
+         cs_dir = BFIND;
          break;
        }
 
       if (_rl_vi_redoing)
        {
-         /* set target and tlen below */
+         /* When redoing with '.' use the last motion char */
+#if defined (HANDLE_MULTIBYTE)
+         target = _rl_vi_last_search_motion_mbchar;
+         tlen = &_rl_vi_last_search_motion_mblen;
+#else
+         target = &_rl_vi_last_search_motion_char;
+#endif
        }
 #if defined (READLINE_CALLBACKS)
       else if (RL_ISSTATE (RL_STATE_CALLBACK))
        {
          _rl_callback_data = _rl_callback_data_alloc (count);
-         _rl_callback_data->i1 = _rl_cs_dir;
+         _rl_callback_data->i1 = cs_dir;
          _rl_callback_data->i2 = key;
          _rl_callback_func = _rl_vi_callback_char_search;
          RL_SETSTATE (RL_STATE_CHARSEARCH);
@@ -1830,11 +1870,22 @@ rl_vi_char_search (int count, int key)
 #endif
       else
        {
+         _rl_cs_dir = cs_dir;
 #if defined (HANDLE_MULTIBYTE)
          c = _rl_read_mbchar (_rl_vi_last_search_mbchar, MB_LEN_MAX);
          if (c <= 0)
            return -1;
          _rl_vi_last_search_mblen = c;
+         /* When reading a c, d, or y command, remember the char
+            separately for redo purposes, since another [tTfF] search
+            could happen before the user does the '.' redo. */
+         if (RL_ISSTATE(RL_STATE_VIMOTION))
+           {
+             memcpy(_rl_vi_last_search_motion_mbchar,
+               _rl_vi_last_search_mbchar,
+               MB_LEN_MAX);
+             _rl_vi_last_search_motion_mblen = c;
+           }
 #else
          RL_SETSTATE(RL_STATE_MOREINPUT);
          c = rl_read_key ();
@@ -1842,21 +1893,19 @@ rl_vi_char_search (int count, int key)
          if (c < 0)
            return -1;
          _rl_vi_last_search_char = c;
+         /* When reading a c, d, or y command, remember the char
+            separately for redo purposes, since another [tTfF] search
+            could happen before the user does the '.' redo. */
+         if (RL_ISSTATE(RL_STATE_VIMOTION))
+           _rl_vi_last_search_motion_char = c;
 #endif
        }
     }
 
 #if defined (HANDLE_MULTIBYTE)
-  target = _rl_vi_last_search_mbchar;
-  tlen = _rl_vi_last_search_mblen;
-#else
-  target = _rl_vi_last_search_char;
-#endif
-
-#if defined (HANDLE_MULTIBYTE)
-  return (_rl_char_search_internal (count, _rl_cs_dir, target, tlen));
+  return (_rl_char_search_internal (count, cs_dir, target, *tlen));
 #else
-  return (_rl_char_search_internal (count, _rl_cs_dir, target));
+  return (_rl_char_search_internal (count, cs_dir, *target));
 #endif
 }