vim-patch:9.0.1359: too many "else if" statements in handling options

Problem:    Too many "else if" statements in handling options.
Solution:   Add more functions for handling option changes. (Yegappan
            Lakshmanan, closes vim/vim#12060)

https://github.com/vim/vim/commit/5da901bb68717b2baff6e971c1517219b6ee3a67

5 files changed, 174 insertions, 52 deletions

diff --git a/src/nvim/mapping.c b/src/nvim/mapping.c
index a2f2ed4d86..c1d02e89b8 100644
--- a/src/nvim/mapping.c
+++ b/src/nvim/mapping.c
@@ -2386,7 +2386,7 @@ void langmap_init(void)
 
 /// Called when langmap option is set; the language map can be
 /// changed at any time!
-const char *did_set_langmap(optset_T *args FUNC_ATTR_UNUSED)
+const char *did_set_langmap(optset_T *args)
 {
   char *p;
   char *p2;
@@ -2434,11 +2434,10 @@ const char *did_set_langmap(optset_T *args FUNC_ATTR_UNUSED)
         }
       }
       if (to == NUL) {
-        // TODO(vim): Need to use errbuf argument for this error message
-        // and return it.
-        semsg(_("E357: 'langmap': Matching character missing for %s"),
-              transchar(from));
-        return NULL;
+        snprintf(args->os_errbuf, args->os_errbuflen,
+                 _("E357: 'langmap': Matching character missing for %s"),
+                 transchar(from));
+        return args->os_errbuf;
       }
 
       if (from >= 256) {
@@ -2456,10 +2455,10 @@ const char *did_set_langmap(optset_T *args FUNC_ATTR_UNUSED)
           p = p2;
           if (p[0] != NUL) {
             if (p[0] != ',') {
-              // TODO(vim): Need to use errbuf argument for this error
-              // message and return it.
-              semsg(_("E358: 'langmap': Extra characters after semicolon: %s"), p);
-              return NULL;
+              snprintf(args->os_errbuf, args->os_errbuflen,
+                       _("E358: 'langmap': Extra characters after semicolon: %s"),
+                       p);
+              return args->os_errbuf;
             }
             p++;
           }
diff --git a/src/nvim/option_defs.h b/src/nvim/option_defs.h
index 1a562a80d8..c808154186 100644
--- a/src/nvim/option_defs.h
+++ b/src/nvim/option_defs.h
@@ -1002,6 +1002,17 @@ typedef struct {
   // Currently only used for boolean options.
   int os_doskip;
 
+  // Option value was checked to be safe, no need to set P_INSECURE
+  // Used for the 'keymap', 'filetype' and 'syntax' options.
+  int os_value_checked;
+  // Option value changed.  Used for the 'filetype' and 'syntax' options.
+  int os_value_changed;
+
+  // Used by the 'isident', 'iskeyword', 'isprint' and 'isfname' options.
+  // Set to true if the character table is modified when processing the
+  // option and need to be restored because of a failure.
+  int os_restore_chartab;
+
   // If the value specified for an option is not valid and the error message
   // is parameterized, then the "os_errbuf" buffer is used to store the error
   // message (when it is not NULL).
diff --git a/src/nvim/options.lua b/src/nvim/options.lua
index 4f50fb80e4..7ab3e7e27e 100644
--- a/src/nvim/options.lua
+++ b/src/nvim/options.lua
@@ -821,7 +821,8 @@ return {
       alloced=true,
       expand=true,
       varname='p_ft',
-      defaults={if_true=""}
+      defaults={if_true=""},
+      cb='did_set_filetype_or_syntax'
     },
     {
       full_name='fillchars', abbreviation='fcs',
@@ -1287,7 +1288,8 @@ return {
         -- ( and ) are used in text separating fnames */
         if_true="@,48-57,/,\\,.,-,_,+,,,#,$,%,{,},[,],:,@-@,!,~,=",
         if_false="@,48-57,/,.,-,_,+,,,#,$,%,~,="
-      }
+      },
+      cb='did_set_isopt'
     },
     {
       full_name='isident', abbreviation='isi',
@@ -1299,7 +1301,8 @@ return {
         condition='MSWIN',
         if_true="@,48-57,_,128-167,224-235",
         if_false="@,48-57,_,192-255"
-      }
+      },
+      cb='did_set_isopt'
     },
     {
       full_name='iskeyword', abbreviation='isk',
@@ -1308,7 +1311,8 @@ return {
       deny_duplicates=true,
       alloced=true,
       varname='p_isk',
-      defaults={if_true="@,48-57,_,192-255"}
+      defaults={if_true="@,48-57,_,192-255"},
+      cb='did_set_isopt'
     },
     {
       full_name='isprint', abbreviation='isp',
@@ -1317,7 +1321,8 @@ return {
       deny_duplicates=true,
       redraw={'all_windows'},
       varname='p_isp',
-      defaults={if_true="@,161-255"}
+      defaults={if_true="@,161-255"},
+      cb='did_set_isopt'
     },
     {
       full_name='joinspaces', abbreviation='js',
@@ -1344,7 +1349,8 @@ return {
       alloced=true,
       redraw={'statuslines', 'current_buffer'},
       varname='p_keymap', pv_name='p_kmap',
-      defaults={if_true=""}
+      defaults={if_true=""},
+      cb='did_set_keymap'
     },
     {
       full_name='keymodel', abbreviation='km',
@@ -1959,7 +1965,8 @@ return {
       type='string', scope={'window'},
       alloced=true,
       redraw={'current_window'},
-      defaults={if_true="search"}
+      defaults={if_true="search"},
+      cb='did_set_rightleftcmd'
     },
     {
       full_name='ruler', abbreviation='ru',
@@ -2486,7 +2493,8 @@ return {
       normal_fname_chars=true,
       alloced=true,
       varname='p_syn',
-      defaults={if_true=""}
+      defaults={if_true=""},
+      cb='did_set_filetype_or_syntax'
     },
     {
       full_name='tagfunc', abbreviation='tfu',
diff --git a/src/nvim/optionstr.c b/src/nvim/optionstr.c
index 8b77af591c..694738d8f1 100644
--- a/src/nvim/optionstr.c
+++ b/src/nvim/optionstr.c
@@ -727,15 +727,17 @@ const char *did_set_breakindentopt(optset_T *args)
 
 /// The 'isident' or the 'iskeyword' or the 'isprint' or the 'isfname' option is
 /// changed.
-static void did_set_isopt(buf_T *buf, bool *did_chartab, const char **errmsg)
+const char *did_set_isopt(optset_T *args)
 {
+  buf_T *buf = (buf_T *)args->os_buf;
   // 'isident', 'iskeyword', 'isprint or 'isfname' option: refill g_chartab[]
   // If the new option is invalid, use old value.
   // 'lisp' option: refill g_chartab[] for '-' char
   if (buf_init_chartab(buf, true) == FAIL) {
-    *did_chartab = true;           // need to restore it below
-    *errmsg = e_invarg;            // error in value
+    args->os_restore_chartab = true;  // need to restore it below
+    return e_invarg;                  // error in value
   }
+  return NULL;
 }
 
 /// The 'helpfile' option is changed.
@@ -978,12 +980,14 @@ static void did_set_encoding(buf_T *buf, char **varp, char **gvarp, int opt_flag
   }
 }
 
-static void did_set_keymap(buf_T *buf, char **varp, int opt_flags, int *value_checked,
-                           const char **errmsg)
+/// The 'keymap' option has changed.
+const char *did_set_keymap(optset_T *args)
 {
-  if (!valid_filetype(*varp)) {
-    *errmsg = e_invarg;
-    return;
+  buf_T *buf = (buf_T *)args->os_buf;
+  int opt_flags = args->os_flags;
+
+  if (!valid_filetype(args->os_varp)) {
+    return e_invarg;
   }
 
   int secure_save = secure;
@@ -993,13 +997,13 @@ static void did_set_keymap(buf_T *buf, char **varp, int opt_flags, int *value_ch
   secure = 0;
 
   // load or unload key mapping tables
-  *errmsg = keymap_init();
+  const char *errmsg = keymap_init();
 
   secure = secure_save;
 
   // Since we check the value, there is no need to set P_INSECURE,
   // even when the value comes from a modeline.
-  *value_checked = true;
+  args->os_value_checked = true;
 
   if (errmsg == NULL) {
     if (*buf->b_p_keymap != NUL) {
@@ -1023,6 +1027,8 @@ static void did_set_keymap(buf_T *buf, char **varp, int opt_flags, int *value_ch
     }
     status_redraw_buf(buf);
   }
+
+  return errmsg;
 }
 
 /// The 'fileformat' option is changed.
@@ -1739,20 +1745,31 @@ const char *did_set_lispoptions(optset_T *args)
   return NULL;
 }
 
+/// The 'rightleftcmd' option is changed.
+const char *did_set_rightleftcmd(optset_T *args)
+{
+  // Currently only "search" is a supported value.
+  if (*args->os_varp != NUL && strcmp(args->os_varp, "search") != 0) {
+    return e_invarg;
+  }
+
+  return NULL;
+}
+
 /// The 'filetype' or the 'syntax' option is changed.
-static void did_set_filetype_or_syntax(char **varp, char *oldval, int *value_checked,
-                                       bool *value_changed, const char **errmsg)
+const char *did_set_filetype_or_syntax(optset_T *args)
 {
-  if (!valid_filetype(*varp)) {
-    *errmsg = e_invarg;
-    return;
+  if (!valid_filetype(args->os_varp)) {
+    return e_invarg;
   }
 
-  *value_changed = strcmp(oldval, *varp) != 0;
+  args->os_value_changed = strcmp(args->os_oldval.string, args->os_varp) != 0;
 
   // Since we check the value, there is no need to set P_INSECURE,
   // even when the value comes from a modeline.
-  *value_checked = true;
+  args->os_value_checked = true;
+
+  return NULL;
 }
 
 const char *did_set_winhl(optset_T *args)
@@ -1955,7 +1972,7 @@ static const char *did_set_string_option_for(buf_T *buf, win_T *win, int opt_idx
                                              size_t errbuflen, int opt_flags, int *value_checked)
 {
   const char *errmsg = NULL;
-  bool did_chartab = false;
+  int restore_chartab = false;
   vimoption_T *opt = get_option(opt_idx);
   bool free_oldval = (opt->flags & P_ALLOCED);
   opt_did_set_cb_T did_set_cb = get_option_did_set_cb(opt_idx);
@@ -1970,6 +1987,9 @@ static const char *did_set_string_option_for(buf_T *buf, win_T *win, int opt_idx
     .os_flags = opt_flags,
     .os_oldval.string = oldval,
     .os_newval.string = value,
+    .os_value_checked = false,
+    .os_value_changed = false,
+    .os_restore_chartab = false,
     .os_errbuf = errbuf,
     .os_errbuflen = errbuflen,
     .os_win = curwin,
@@ -1991,17 +2011,21 @@ static const char *did_set_string_option_for(buf_T *buf, win_T *win, int opt_idx
     if (errmsg == NULL && is_expr_option(curwin, varp, gvarp)) {
       *varp = args.os_varp;
     }
-  } else if (varp == &p_isi                             // 'isident'
-             || varp == &buf->b_p_isk                   // 'iskeyword'
-             || varp == &p_isp                          // 'isprint'
-             || varp == &p_isf) {                       // 'isfname'
-    did_set_isopt(buf, &did_chartab, &errmsg);
+    // The 'filetype' and 'syntax' option callback functions may change
+    // the os_value_changed field.
+    value_changed = args.os_value_changed;
+    // The 'keymap', 'filetype' and 'syntax' option callback functions
+    // may change the os_value_checked field.
+    *value_checked = args.os_value_checked;
+    // The 'isident', 'iskeyword', 'isprint' and 'isfname' options may
+    // change the character table.  On failure, this needs to be restored.
+    restore_chartab = args.os_restore_chartab;
+  } else if (varp == &p_shada) {                        // 'shada'
+    errmsg = did_set_shada(&opt, &opt_idx, &free_oldval, errbuf, errbuflen);
   } else if (varp == &p_enc                             // 'encoding'
              || gvarp == &p_fenc                        // 'fileencoding'
              || gvarp == &p_menc) {                     // 'makeencoding'
     did_set_encoding(buf, varp, gvarp, opt_flags, &errmsg);
-  } else if (varp == &buf->b_p_keymap) {                // 'keymap'
-    did_set_keymap(buf, varp, opt_flags, value_checked, &errmsg);
   } else if (varp == &p_lcs                             // global 'listchars'
              || varp == &p_fcs) {                       // global 'fillchars'
     did_set_global_listfillchars(win, varp, opt_flags, &errmsg);
@@ -2009,11 +2033,6 @@ static const char *did_set_string_option_for(buf_T *buf, win_T *win, int opt_idx
     errmsg = set_chars_option(win, varp, true);
   } else if (varp == &win->w_p_fcs) {                   // local 'fillchars'
     errmsg = set_chars_option(win, varp, true);
-  } else if (varp == &p_shada) {                        // 'shada'
-    errmsg = did_set_shada(&opt, &opt_idx, &free_oldval, errbuf, errbuflen);
-  } else if (gvarp == &p_ft                             // 'filetype'
-             || gvarp == &p_syn) {                      // 'syntax'
-    did_set_filetype_or_syntax(varp, oldval, value_checked, &value_changed, &errmsg);
   }
 
   // If an error is detected, restore the previous value.
@@ -2021,7 +2040,7 @@ static const char *did_set_string_option_for(buf_T *buf, win_T *win, int opt_idx
     free_string_option(*varp);
     *varp = oldval;
     // When resetting some values, need to act on it.
-    if (did_chartab) {
+    if (restore_chartab) {
       (void)buf_init_chartab(buf, true);
     }
   } else {
diff --git a/test/old/testdir/test_options.vim b/test/old/testdir/test_options.vim
index 8a3a022748..e4cdf539e2 100644
--- a/test/old/testdir/test_options.vim
+++ b/test/old/testdir/test_options.vim
@@ -45,6 +45,10 @@ func Test_isfname()
   set isfname=
   call assert_equal("~X", expand("~X"))
   set isfname&
+  " Test for setting 'isfname' to an unsupported character
+  let save_isfname = &isfname
+  call assert_fails('exe $"set isfname+={"\u1234"}"', 'E474:')
+  call assert_equal(save_isfname, &isfname)
 endfunc
 
 " Test for getting the value of 'pastetoggle'
@@ -1388,6 +1392,7 @@ func Test_string_option_revert_on_failure()
         \ ['fileencoding', 'utf-8', 'a123,'],
         \ ['fileformat', 'mac', 'a123'],
         \ ['fileformats', 'mac', 'a123'],
+        \ ['filetype', 'abc', 'a^b'],
         \ ['fillchars', 'diff:~', 'a123'],
         \ ['foldclose', 'all', 'a123'],
         \ ['foldmarker', '[[[,]]]', '[[['],
@@ -1396,7 +1401,49 @@ func Test_string_option_revert_on_failure()
         \ ['formatoptions', 'an', '*'],
         \ ['guicursor', 'n-v-c:block-Cursor/lCursor', 'n-v-c'],
         \ ['helplang', 'en', 'a'],
-        "\ ['highlight', '!:CursorColumn', '8:']
+        "\ ['highlight', '!:CursorColumn', '8:'],
+        \ ['keymodel', 'stopsel', 'a123'],
+        "\ ['keyprotocol', 'kitty:kitty', 'kitty:'],
+        \ ['lispoptions', 'expr:1', 'a123'],
+        \ ['listchars', 'tab:->', 'tab:'],
+        \ ['matchpairs', '<:>', '<:'],
+        \ ['mkspellmem', '100000,1000,100', '100000'],
+        \ ['mouse', 'nvi', 'z'],
+        \ ['mousemodel', 'extend', 'a123'],
+        \ ['nrformats', 'alpha', 'a123'],
+        \ ['omnifunc', 'MyOmniFunc', '1a-'],
+        \ ['operatorfunc', 'MyOpFunc', '1a-'],
+        "\ ['previewpopup', 'width:20', 'a123'],
+        "\ ['printoptions', 'paper:A4', 'a123:'],
+        \ ['quickfixtextfunc', 'MyQfFunc', '1a-'],
+        \ ['rulerformat', '%l', '%['],
+        \ ['scrollopt', 'hor,jump', 'a123'],
+        \ ['selection', 'exclusive', 'a123'],
+        \ ['selectmode', 'cmd', 'a123'],
+        \ ['sessionoptions', 'options', 'a123'],
+        \ ['shortmess', 'w', '2'],
+        \ ['showbreak', '>>', "\x01"],
+        \ ['showcmdloc', 'statusline', 'a123'],
+        \ ['signcolumn', 'no', 'a123'],
+        \ ['spellcapcheck', '[.?!]\+', '%\{'],
+        \ ['spellfile', 'MySpell.en.add', "\x01"],
+        \ ['spelllang', 'en', "#"],
+        \ ['spelloptions', 'camel', 'a123'],
+        \ ['spellsuggest', 'double', 'a123'],
+        \ ['splitkeep', 'topline', 'a123'],
+        \ ['statusline', '%f', '%['],
+        "\ ['swapsync', 'sync', 'a123'],
+        \ ['switchbuf', 'usetab', 'a123'],
+        \ ['syntax', 'abc', 'a^b'],
+        \ ['tabline', '%f', '%['],
+        \ ['tagcase', 'ignore', 'a123'],
+        \ ['tagfunc', 'MyTagFunc', '1a-'],
+        \ ['thesaurusfunc', 'MyThesaurusFunc', '1a-'],
+        \ ['viewoptions', 'options', 'a123'],
+        \ ['virtualedit', 'onemore', 'a123'],
+        \ ['whichwrap', '<,>', '{,}'],
+        \ ['wildmode', 'list', 'a123'],
+        \ ['wildoptions', 'pum', 'a123']
         \ ]
   if has('gui')
     call add(optlist, ['browsedir', 'buffer', 'a123'])
@@ -1411,11 +1458,49 @@ func Test_string_option_revert_on_failure()
     call add(optlist, ['cscopequickfix', 't-', 'z-'])
   endif
   if !has('win32') && !has('nvim')
-    call add(optlist, ['imactivatefunc', 'MyCmplFunc', '1a-'])
+    call add(optlist, ['imactivatefunc', 'MyActFunc', '1a-'])
+    call add(optlist, ['imstatusfunc', 'MyStatusFunc', '1a-'])
+  endif
+  if has('keymap')
+    call add(optlist, ['keymap', 'greek', '[]'])
+  endif
+  if has('mouseshape')
+    call add(optlist, ['mouseshape', 'm:no', 'a123:'])
+  endif
+  if has('win32') && has('gui')
+    call add(optlist, ['renderoptions', 'type:directx', 'type:directx,a123'])
+  endif
+  if has('rightleft')
+    call add(optlist, ['rightleftcmd', 'search', 'a123'])
+  endif
+  if has('terminal')
+    call add(optlist, ['termwinkey', '<C-L>', '<C'])
+    call add(optlist, ['termwinsize', '24x80', '100'])
+  endif
+  if has('win32') && has('terminal')
+    call add(optlist, ['termwintype', 'winpty', 'a123'])
+  endif
+  if has('+toolbar')
+    call add(optlist, ['toolbar', 'text', 'a123'])
+    call add(optlist, ['toolbariconsize', 'medium', 'a123'])
+  endif
+  if has('+mouse')
+    call add(optlist, ['ttymouse', 'xterm', 'a123'])
+  endif
+  if has('+vartabs')
+    call add(optlist, ['varsofttabstop', '12', 'a123'])
+    call add(optlist, ['vartabstop', '4,20', '4,'])
+  endif
+  if has('gui')
+    call add(optlist, ['winaltkeys', 'no', 'a123'])
   endif
   for opt in optlist
     exe $"let save_opt = &{opt[0]}"
-    exe $"let &{opt[0]} = '{opt[1]}'"
+    try
+      exe $"let &{opt[0]} = '{opt[1]}'"
+    catch
+      call assert_report($"Caught {v:exception} with {opt->string()}")
+    endtry
     call assert_fails($"let &{opt[0]} = '{opt[2]}'", '', opt[0])
     call assert_equal(opt[1], eval($"&{opt[0]}"), opt[0])
     exe $"let &{opt[0]} = save_opt"