vim-patch:8.1.0669: the ex_sign() function is too long

Problem:    The ex_sign() function is too long.
Solution:   Refactor the function.  Add a bit more testing. (Yegappan
            Lakshmanan, closes vim/vim#3745)
https://github.com/vim/vim/commit/a355652ea5b0c1633e8126ad9af2d970e05f4e1a

2 files changed, 341 insertions, 217 deletions

diff --git a/src/nvim/ex_cmds.c b/src/nvim/ex_cmds.c
index 5d9dc7508f..212aeb4c27 100644
--- a/src/nvim/ex_cmds.c
+++ b/src/nvim/ex_cmds.c
@@ -5830,6 +5830,9 @@ int sign_place(
     int
 sign_unplace(int sign_id, char_u *sign_group, buf_T *buf, linenr_T atlnum)
 {
+    if (buf->b_signlist == NULL) {	// No signs in the buffer
+	return OK;
+    }
   if (sign_id == 0)
   {
     // Delete all the signs in the specified buffer
@@ -5866,298 +5869,389 @@ static void sign_unplace_at_cursor(char_u *groupname)
 }
 
 /*
- * ":sign" command
+ * sign define command
+ *   ":sign define {name} ..."
  */
-void ex_sign(exarg_T *eap)
+static void sign_define_cmd(char_u *sign_name, char_u *cmdline)
 {
-  char_u *arg = eap->arg;
-  char_u *p;
-  int idx;
-  sign_T *sp;
+  char_u	*arg;
+  char_u	*p = cmdline;
+  char_u	*icon = NULL;
+  char_u	*text = NULL;
+  char_u	*linehl = NULL;
+  char_u	*texthl = NULL;
+  char_u	*numhl = NULL;
+  int failed = FALSE;
+
+  // set values for a defined sign.
+  for (;;) {
+    arg = skipwhite(p);
+    if (*arg == NUL) {
+      break;
+    }
+    p = skiptowhite_esc(arg);
+    if (STRNCMP(arg, "icon=", 5) == 0) {
+      arg += 5;
+      icon = vim_strnsave(arg, (int)(p - arg));
+    } else if (STRNCMP(arg, "text=", 5) == 0) {
+      arg += 5;
+      text = vim_strnsave(arg, (int)(p - arg));
+    } else if (STRNCMP(arg, "linehl=", 7) == 0) {
+      arg += 7;
+      linehl = vim_strnsave(arg, (int)(p - arg));
+    } else if (STRNCMP(arg, "texthl=", 7) == 0) {
+      arg += 7;
+      texthl = vim_strnsave(arg, (int)(p - arg));
+    } else if (STRNCMP(arg, "numhl=", 6) == 0) {
+      arg += 6;
+      numhl = vim_strnsave(arg, (int)(p - arg));
+    } else {
+      EMSG2(_(e_invarg2), arg);
+      failed = TRUE;
+      break;
+    }
+  }
 
-  // Parse the subcommand.
-  p = skiptowhite(arg);
-  idx = sign_cmd_idx(arg, p);
-  if (idx == SIGNCMD_LAST) {
-    EMSG2(_("E160: Unknown sign command: %s"), arg);
-    return;
+  if (!failed) {
+    sign_define_by_name(sign_name, icon, linehl, text, texthl, numhl);
   }
-  arg = skipwhite(p);
 
-  if (idx <= SIGNCMD_LIST) {
-    // Define, undefine or list signs.
-    if (idx == SIGNCMD_LIST && *arg == NUL) {
-      // ":sign list": list all defined signs
-      for (sp = first_sign; sp != NULL && !got_int; sp = sp->sn_next) {
-        sign_list_defined(sp);
-      }
-    } else if (*arg == NUL) {
-      EMSG(_("E156: Missing sign name"));
+  xfree(icon);
+  xfree(text);
+  xfree(linehl);
+  xfree(texthl);
+  xfree(numhl);
+}
+
+/*
+ * :sign place command
+ */
+static void sign_place_cmd(
+    buf_T		*buf,
+    linenr_T	lnum,
+    char_u		*sign_name,
+    int		id,
+    char_u		*group,
+    int		prio
+    )
+{
+  if (id <= 0)
+  {
+    // List signs placed in a file/buffer
+    //   :sign place file={fname}
+    //   :sign place group={group} file={fname}
+    //   :sign place group=* file={fname}
+    //   :sign place buffer={nr}
+    //   :sign place group={group} buffer={nr}
+    //   :sign place group=* buffer={nr}
+    //   :sign place
+    //   :sign place group={group}
+    //   :sign place group=*
+    if (lnum >= 0 || sign_name != NULL ||
+        (group != NULL && *group == '\0')) {
+      EMSG(_(e_invarg));
     } else {
-      char_u	*name;
-      char_u	*icon = NULL;
-      char_u	*text = NULL;
-      char_u	*linehl = NULL;
-      char_u	*texthl = NULL;
-      char_u	*numhl = NULL;
+      sign_list_placed(buf, group);
+    }
+  } else {
+    // Place a new sign
+    if (sign_name == NULL || buf == NULL ||
+        (group != NULL && *group == '\0')) {
+      EMSG(_(e_invarg));
+      return;
+    }
 
-      // Isolate the sign name.  If it's a number skip leading zeroes,
-      // so that "099" and "99" are the same sign.  But keep "0".
-      p = skiptowhite(arg);
-      if (*p != NUL) {
-        *p++ = NUL;
-      }
-      while (arg[0] == '0' && arg[1] != NUL) {
-        arg++;
-      }
-      name = vim_strsave(arg);
+    sign_place(&id, group, sign_name, buf, lnum, prio);
+  }
+}
 
-      if (idx == SIGNCMD_DEFINE) {
-        int failed = FALSE;
-        // ":sign define {name} ...": define a sign
+/*
+ * :sign unplace command
+ */
+static void sign_unplace_cmd(
+	buf_T		*buf,
+	linenr_T	lnum,
+	char_u		*sign_name,
+	int		id,
+	char_u		*group
+)
+{
+    if (lnum >= 0 || sign_name != NULL || (group != NULL && *group == '\0')) {
+      EMSG(_(e_invarg));
+      return;
+    }
 
-        // set values for a defined sign.
-        for (;;) {
-          arg = skipwhite(p);
-          if (*arg == NUL) {
-            break;
+    if (id == -2)
+    {
+      if (buf != NULL) {
+        // :sign unplace * file={fname}
+        // :sign unplace * group={group} file={fname}
+        // :sign unplace * group=* file={fname}
+        // :sign unplace * buffer={nr}
+        // :sign unplace * group={group} buffer={nr}
+        // :sign unplace * group=* buffer={nr}
+        sign_unplace(0, group, buf, 0);
+      } else {
+        // :sign unplace *
+        // :sign unplace * group={group}
+        // :sign unplace * group=*
+        FOR_ALL_BUFFERS(buf) {
+          if (buf->b_signlist != NULL) {
+            buf_delete_signs(buf, group);
           }
-          p = skiptowhite_esc(arg);
-          if (STRNCMP(arg, "icon=", 5) == 0) {
-            arg += 5;
-            icon = vim_strnsave(arg, (int)(p - arg));
-          } else if (STRNCMP(arg, "text=", 5) == 0) {
-            arg += 5;
-            text = vim_strnsave(arg, (int)(p - arg));
-          } else if (STRNCMP(arg, "linehl=", 7) == 0) {
-            arg += 7;
-            linehl = vim_strnsave(arg, (int)(p - arg));
-          } else if (STRNCMP(arg, "texthl=", 7) == 0) {
-            arg += 7;
-            texthl = vim_strnsave(arg, (int)(p - arg));
-          } else if (STRNCMP(arg, "numhl=", 6) == 0) {
-            arg += 6;
-            numhl = vim_strnsave(arg, (int)(p - arg));
-          } else {
-            EMSG2(_(e_invarg2), arg);
-            failed = TRUE;
-            break;
+        }
+      }
+    } else {
+      if (buf != NULL) {
+        // :sign unplace {id} file={fname}
+        // :sign unplace {id} group={group} file={fname}
+        // :sign unplace {id} group=* file={fname}
+        // :sign unplace {id} buffer={nr}
+        // :sign unplace {id} group={group} buffer={nr}
+        // :sign unplace {id} group=* buffer={nr}
+        sign_unplace(id, group, buf, 0);
+      } else {
+        if (id == -1)
+        {
+          // :sign unplace group={group}
+          // :sign unplace group=*
+          sign_unplace_at_cursor(group);
+        } else {
+          // :sign unplace {id}
+          // :sign unplace {id} group={group}
+          // :sign unplace {id} group=*
+          FOR_ALL_BUFFERS(buf) {
+            sign_unplace(id, group, buf, 0);
           }
         }
+      }
+    }
+}
 
-        if (!failed)
-          sign_define_by_name(name, icon, linehl, text, texthl, numhl);
+/*
+ * Jump to a placed sign
+ *   :sign jump {id} file={fname}
+ *   :sign jump {id} buffer={nr}
+ *   :sign jump {id} group={group} file={fname}
+ *   :sign jump {id} group={group} buffer={nr}
+ */
+static void sign_jump_cmd(
+	buf_T		*buf,
+	linenr_T	lnum,
+	char_u		*sign_name,
+	int		id,
+	char_u		*group
+  )
+{
+  if (buf == NULL && sign_name == NULL && group == NULL && id == -1) {
+    EMSG(_(e_argreq));
+    return;
+  }
 
-        xfree(icon);
-        xfree(text);
-        xfree(linehl);
-        xfree(texthl);
-      } else if (idx == SIGNCMD_LIST) {
-        // ":sign list {name}"
-        sign_list_by_name(name);
-      } else {
-        // ":sign undefine {name}"
-        sign_undefine_by_name(name);
-      }
+  if (buf == NULL || (group != NULL && *group == '\0') ||
+      lnum >= 0 || sign_name != NULL) {
+    // File or buffer is not specified or an empty group is used
+    // or a line number or a sign name is specified.
+    EMSG(_(e_invarg));
+    return;
+  }
 
-      xfree(name);
-      return;
-    }
-  } else {
-    int id = -1;
-    linenr_T lnum = -1;
-    char_u *sign_name = NULL;
-    char_u *group = NULL;
-    int prio = SIGN_DEF_PRIO;
-    char_u *arg1;
-    int	bufarg = FALSE;
+  if ((lnum = buf_findsign(buf, id, group)) <= 0) {
+    EMSGN(_("E157: Invalid sign ID: %ld"), id);
+    return;
+  }
 
-    if (*arg == NUL) {
-      if (idx == SIGNCMD_PLACE) {
-        // ":sign place": list placed signs in all buffers
-        sign_list_placed(NULL, NULL);
-      } else if (idx == SIGNCMD_UNPLACE) {
-        // ":sign unplace": remove placed sign at cursor
-        sign_unplace_at_cursor(NULL);
-      } else {
-        EMSG(_(e_argreq));
-      }
+  // goto a sign ...
+  if (buf_jump_open_win(buf) != NULL)
+  {			// ... in a current window
+    curwin->w_cursor.lnum = lnum;
+    check_cursor_lnum();
+    beginline(BL_WHITE);
+  } else {			// ... not currently in a window
+    if (buf->b_fname == NULL) {
+      EMSG(_("E934: Cannot jump to a buffer that does not have a name"));
       return;
     }
+    size_t cmdlen = STRLEN(buf->b_fname) + 24;
+    char *cmd = xmallocz(cmdlen);
+    snprintf(cmd, cmdlen, "e +%" PRId64 " %s",
+        (int64_t)lnum, buf->b_fname);
+    do_cmdline_cmd(cmd);
+    xfree(cmd);
+  }
 
-    if (idx == SIGNCMD_UNPLACE && arg[0] == '*' && arg[1] == NUL) {
-      // ":sign unplace *": remove all placed signs
-      buf_delete_all_signs(NULL);
-      return;
-    }
+  foldOpenCursor();
+}
+
+/*
+ * Parse the command line arguments for the ":sign place", ":sign unplace" and
+ * ":sign jump" commands.
+ * The supported arguments are: line={lnum} name={name} group={group}
+ * priority={prio} and file={fname} or buffer={nr}.
+ */
+static int parse_sign_cmd_args(
+	int	    cmd,
+	char_u	    *arg,
+	char_u	    **sign_name,
+	int	    *signid,
+	char_u	    **group,
+	int	    *prio,
+	buf_T	    **buf,
+	linenr_T    *lnum
+)
+{
+    char_u	*arg1;
+    char_u	*name;
+    char_u	*filename = NULL;
 
     // first arg could be placed sign id
     arg1 = arg;
     if (ascii_isdigit(*arg)) {
-      id = getdigits_int(&arg);
+      *signid = getdigits_int(&arg);
       if (!ascii_iswhite(*arg) && *arg != NUL) {
-        id = -1;
+        *signid = -1;
         arg = arg1;
       } else {
         arg = skipwhite(arg);
-        if (idx == SIGNCMD_UNPLACE && *arg == NUL) {
-          // ":sign unplace {id}": remove placed sign by number
-          FOR_ALL_BUFFERS(buf) {
-			sign_unplace(id, NULL, buf, 0);
-          }
-          return;
-        }
       }
     }
 
-    // Check for line={lnum} name={name} and file={fname} or buffer={nr}.
-    // Leave "arg" pointing to {fname}.
-
-    buf_T *buf = NULL;
     while (*arg != NUL) {
       if (STRNCMP(arg, "line=", 5) == 0) {
         arg += 5;
-        lnum = atoi((char *)arg);
+        *lnum = atoi((char *)arg);
         arg = skiptowhite(arg);
-      } else if (STRNCMP(arg, "*", 1) == 0 && idx == SIGNCMD_UNPLACE) {
-        if (id != -1) {
+      } else if (STRNCMP(arg, "*", 1) == 0 && cmd == SIGNCMD_UNPLACE) {
+        if (*signid != -1) {
           EMSG(_(e_invarg));
-          return;
+          return FAIL;
         }
-        id = -2;
+        *signid = -2;
         arg = skiptowhite(arg + 1);
       } else if (STRNCMP(arg, "name=", 5) == 0) {
         arg += 5;
-        sign_name = arg;
+        name = arg;
         arg = skiptowhite(arg);
         if (*arg != NUL) {
           *arg++ = NUL;
         }
-        while (sign_name[0] == '0' && sign_name[1] != NUL) {
-          sign_name++;
+        while (name[0] == '0' && name[1] != NUL) {
+          ++name;
         }
+        *sign_name = name;
  	    } else if (STRNCMP(arg, "group=", 6) == 0) {
         arg += 6;
-        group = arg;
+        *group = arg;
         arg = skiptowhite(arg);
         if (*arg != NUL) {
           *arg++ = NUL;
         }
       } else if (STRNCMP(arg, "priority=", 9) == 0) {
         arg += 9;
-        prio = atoi((char *)arg);
+        *prio = atoi((char *)arg);
         arg = skiptowhite(arg);
       } else if (STRNCMP(arg, "file=", 5) == 0) {
         arg += 5;
-        buf = buflist_findname_exp(arg);
-        bufarg = TRUE;
+        filename = arg;
+        *buf = buflist_findname_exp(arg);
         break;
       } else if (STRNCMP(arg, "buffer=", 7) == 0) {
         arg += 7;
-        buf = buflist_findnr(getdigits_int(&arg));
+        filename = arg;
+        *buf = buflist_findnr(getdigits_int(&arg));
         if (*skipwhite(arg) != NUL) {
           EMSG(_(e_trailing));
         }
-        bufarg = TRUE;
         break;
       } else {
         EMSG(_(e_invarg));
-        return;
+        return FAIL;
       }
       arg = skipwhite(arg);
     }
 
-    if ((!bufarg && group == NULL) || (group != NULL && *group == '\0')) {
-      // File or buffer is not specified or an empty group is used
-      EMSG(_(e_invarg));
-      return;
+    if (filename != NULL && *buf == NULL) {
+      EMSG2(_("E158: Invalid buffer name: %s"), filename);
+      return FAIL;
     }
 
-    if (bufarg && buf == NULL) {
-      EMSG2(_("E158: Invalid buffer name: %s"), arg);
-    } else if (id <= 0 && idx == SIGNCMD_PLACE) {
-      if ((group == NULL) && (lnum >= 0 || sign_name != NULL)) {
-        EMSG(_(e_invarg));
-      } else {
-        // ":sign place file={fname}": list placed signs in one file
-        // ":sign place group={group} file={fname}"
-        // ":sign place group=* file={fname}"
-        sign_list_placed(buf, group);
+    return OK;
+}
+
+/*
+ * ":sign" command
+ */
+void ex_sign(exarg_T *eap)
+{
+  char_u *arg = eap->arg;
+  char_u *p;
+  int idx;
+  sign_T *sp;
+
+  // Parse the subcommand.
+  p = skiptowhite(arg);
+  idx = sign_cmd_idx(arg, p);
+  if (idx == SIGNCMD_LAST) {
+    EMSG2(_("E160: Unknown sign command: %s"), arg);
+    return;
+  }
+  arg = skipwhite(p);
+
+  if (idx <= SIGNCMD_LIST) {
+    // Define, undefine or list signs.
+    if (idx == SIGNCMD_LIST && *arg == NUL) {
+      // ":sign list": list all defined signs
+      for (sp = first_sign; sp != NULL && !got_int; sp = sp->sn_next) {
+        sign_list_defined(sp);
       }
-    } else if (idx == SIGNCMD_JUMP) {
-      // ":sign jump {id} file={fname}"
-      // ":sign jump {id} group={group} file={fname}"
-      if (lnum >= 0 || sign_name != NULL || buf == NULL){
-        EMSG(_(e_invarg));
-      } else if ((lnum = buf_findsign(buf, id, group)) > 0) {
-        // goto a sign ...
-        if (buf_jump_open_win(buf) != NULL) {
-          // ... in a current window
-          curwin->w_cursor.lnum = lnum;
-          check_cursor_lnum();
-          beginline(BL_WHITE);
-        } else {
-          // ... not currently in a window
-          if (buf->b_fname == NULL) {
-            EMSG(_("E934: Cannot jump to a buffer that does not have a name"));
-            return;
-          }
-          size_t cmdlen = STRLEN(buf->b_fname) + 24;
-          char *cmd = xmallocz(cmdlen);
-          snprintf(cmd, cmdlen, "e +%" PRId64 " %s",
-                   (int64_t)lnum, buf->b_fname);
-          do_cmdline_cmd(cmd);
-          xfree(cmd);
-        }
+    } else if (*arg == NUL) {
+      EMSG(_("E156: Missing sign name"));
+    } else {
+      char_u	*name;
 
-        foldOpenCursor();
-      } else {
-        EMSGN(_("E157: Invalid sign ID: %" PRId64), id);
+      // Isolate the sign name.  If it's a number skip leading zeroes,
+      // so that "099" and "99" are the same sign.  But keep "0".
+      p = skiptowhite(arg);
+      if (*p != NUL) {
+        *p++ = NUL;
       }
-    } else if (idx == SIGNCMD_UNPLACE) {
-      if (lnum >= 0 || sign_name != NULL) {
-        EMSG(_(e_invarg));
-      } else if (id == -2) {
-        if (buf != NULL) {
-          // ":sign unplace * file={fname}"
-          sign_unplace(0, group, buf, 0);
-        } else {
-          // ":sign unplace * group=*": remove all placed signs
-          FOR_ALL_BUFFERS(buf) {
-            if (buf->b_signlist != NULL) {
-              buf_delete_signs(buf, group);
-            }
-          }
-        }
+      while (arg[0] == '0' && arg[1] != NUL) {
+        arg++;
+      }
+      name = vim_strsave(arg);
+
+      if (idx == SIGNCMD_DEFINE) {
+        sign_define_cmd(name, p);
+      } else if (idx == SIGNCMD_LIST) {
+        // ":sign list {name}"
+        sign_list_by_name(name);
       } else {
-        if (buf != NULL) {
-          // ":sign unplace {id} file={fname}"
-          // ":sign unplace {id} group={group} file={fname}"
-          // ":sign unplace {id} group=* file={fname}"
-          sign_unplace(id, group, buf, 0);
-        } else {
-          if (id == -1) {
-            // ":sign unplace group={group}":
-            // ":sign unplace group=*":
-            // remove sign in the current line in specified group
-            sign_unplace_at_cursor(group);
-          } else {
-            // ":sign unplace {id} group={group}":
-            // ":sign unplace {id} group=*":
-            //     remove all placed signs in this group.
-            FOR_ALL_BUFFERS(buf) {
-              if (buf->b_signlist != NULL) {
-                sign_unplace(id, group, buf, 0);
-              }
-            }
-          }
-        }
+        // ":sign undefine {name}"
+        sign_undefine_by_name(name);
       }
-    } else if (sign_name != NULL && buf != NULL) {
-      // idx == SIGNCMD_PLACE
-      sign_place(&id, group, sign_name, buf, lnum, prio);
-    } else {
-      EMSG(_(e_invarg));
+
+      xfree(name);
+      return;
+    }
+  } else {
+    int id = -1;
+    linenr_T lnum = -1;
+    char_u *sign_name = NULL;
+    char_u *group = NULL;
+    int prio = SIGN_DEF_PRIO;
+    buf_T *buf = NULL;
+
+    // Parse command line arguments
+    if (parse_sign_cmd_args(idx, arg, &sign_name, &id, &group, &prio,
+          &buf, &lnum) == FAIL)
+      return;
+
+    if (idx == SIGNCMD_PLACE) {
+      sign_place_cmd(buf, lnum, sign_name, id, group, prio);
+    } else if (idx == SIGNCMD_UNPLACE) {
+      sign_unplace_cmd(buf, lnum, sign_name, id, group);
+    } else if (idx == SIGNCMD_JUMP) {
+      sign_jump_cmd(buf, lnum, sign_name, id, group);
     }
   }
 }
diff --git a/src/nvim/testdir/test_signs.vim b/src/nvim/testdir/test_signs.vim
index dc76812d88..e3cbd465ce 100644
--- a/src/nvim/testdir/test_signs.vim
+++ b/src/nvim/testdir/test_signs.vim
@@ -104,6 +104,16 @@ func Test_sign()
   exe 'sign jump 43 file=' . fn
   call assert_equal('B', getline('.'))
 
+  " Check for jumping to a sign in a hidden buffer
+  enew! | only!
+  edit foo
+  call setline(1, ['A', 'B', 'C', 'D'])
+  let fn = expand('%:p')
+  exe 'sign place 21 line=3 name=Sign3 file=' . fn
+  hide edit bar
+  exe 'sign jump 21 file=' . fn
+  call assert_equal('C', getline('.'))
+
   " can't define a sign with a non-printable character as text
   call assert_fails("sign define Sign4 text=\e linehl=Comment", 'E239:')
   call assert_fails("sign define Sign4 text=a\e linehl=Comment", 'E239:')
@@ -131,6 +141,18 @@ func Test_sign()
   sign define Sign4 text=\\ linehl=Comment
   sign undefine Sign4
 
+  " define a sign with a leading 0 in the name
+  sign unplace *
+  sign define 004 text=#> linehl=Comment
+  let a = execute('sign list 4')
+  call assert_equal("\nsign 4 text=#> linehl=Comment", a)
+  exe 'sign place 20 line=3 name=004 buffer=' . bufnr('')
+  let a = execute('sign place')
+  call assert_equal("\n--- Signs ---\nSigns for foo:\n    line=3  id=20  name=4 priority=10\n", a)
+  exe 'sign unplace 20 buffer=' . bufnr('')
+  sign undefine 004
+  call assert_fails('sign list 4', 'E155:')
+
   " Error cases
   call assert_fails("sign place abc line=3 name=Sign1 buffer=" .
 			  \ bufnr('%'), 'E474:')
@@ -241,6 +263,14 @@ func Test_sign_invalid_commands()
   call assert_fails('sign undefine', 'E156:')
   call assert_fails('sign list xxx', 'E155:')
   call assert_fails('sign place 1 buffer=999', 'E158:')
+  call assert_fails('sign place 1 name=Sign1 buffer=999', 'E158:')
+  call assert_fails('sign place buffer=999', 'E158:')
+  call assert_fails('sign jump buffer=999', 'E158:')
+  call assert_fails('sign jump 1 file=', 'E158:')
+  call assert_fails('sign jump 1 group=', 'E474:')
+  call assert_fails('sign jump 1 name=', 'E474:')
+  call assert_fails('sign jump 1 name=Sign1', 'E474:')
+  call assert_fails('sign jump 1 line=100', '474:')
   call assert_fails('sign define Sign2 text=', 'E239:')
   " Non-numeric identifier for :sign place
   call assert_fails("sign place abc line=3 name=Sign1 buffer=" . bufnr('%'), 'E474:')