options: get rid of ambiguous option parsing

Options parsing used to be ambiguous, as in the splitting into option
and values pairs was ambiguous. Example:

    -option -something

It wasn't clear whether -option actually takes an argument or not. The
string "-something" could either be a separate option, or an argument
to "-option". The code had to call the option specific parser function
to resolve this.

This made everything complicated and didn't even have a real use. There
was only one case where this was actually used: string lists
(m_option_type_string_list) and options based on it. That is because
this option type actually turns a single option into a proxy for several
real arguments, e.g. "vf*" can handle "-vf-add" and "-vf-clr". Options
suffixed with "-clr" are the only options of this group which take no
arguments.

This is ambiguous only with the "old syntax" (as shown above). The "new"
option syntax always puts option name and value into same argument.
(E.g. "--option=--something" or "--option" "--something".)

Simplify the code by making it statically known whether an option takes
a parameter or not with the flag M_OPT_TYPE_OLD_SYNTAX_NO_PARAM. If it's
set, the option parser assumes the option takes no argument.

The only real ambiguity left, string list options that end on "-clr",
are special cased in the parser.

Remove some duplication of the logic in the command line parser by
moving all argument splitting logic into split_opt(). (It's arguable
whether that can be considered code duplication, but now the code is a
bit simpler anyway. This might be subjective.)

Remove the "ambiguous" parameter from all option parsing related code.

Make m_config unaware of the pre-parsing concept.

Make most CONF_NOCFG options also CONF_GLOBAL (except those explicitly
usable as per-file options.)

1 files changed, 203 insertions, 184 deletions

diff --git a/parser-mpcmd.c b/parser-mpcmd.c
index d84ec16432..5a91c093f9 100644
--- a/parser-mpcmd.c
+++ b/parser-mpcmd.c
@@ -39,58 +39,120 @@
 #define dvd_range(a)  (a > 0 && a < 256)
 
 
-static bool split_opt(struct bstr *opt, struct bstr *param, bool *old_syntax)
+struct parse_state {
+    struct m_config *config;
+    int argc;
+    char **argv;
+
+    bool no_more_opts;
+    bool error;
+
+    const struct m_option *mp_opt;      // NULL <=> it's a file arg
+    struct bstr arg;
+    struct bstr param;
+};
+
+// Returns 0 if a valid option/file is available, <0 on error, 1 on end of args.
+static int split_opt_silent(struct parse_state *p)
 {
-    if (!bstr_startswith0(*opt, "-") || opt->len == 1)
-        return false;
-    if (bstr_startswith0(*opt, "--")) {
-        *old_syntax = false;
-        *opt = bstr_cut(*opt, 2);
-        *param = bstr0(NULL);
-        int idx = bstrchr(*opt, '=');
+    assert(!p->error);
+
+    if (p->argc < 1)
+        return 1;
+
+    p->mp_opt = NULL;
+    p->arg = bstr0(p->argv[0]);
+    p->param = bstr0(NULL);
+
+    p->argc--;
+    p->argv++;
+
+    if (p->no_more_opts || !bstr_startswith0(p->arg, "-") || p->arg.len == 1)
+        return 0;
+
+    bool old_syntax = !bstr_startswith0(p->arg, "--");
+    if (old_syntax) {
+        p->arg = bstr_cut(p->arg, 1);
+    } else {
+        p->arg = bstr_cut(p->arg, 2);
+        int idx = bstrchr(p->arg, '=');
         if (idx > 0) {
-            *param = bstr_cut(*opt, idx + 1);
-            *opt = bstr_splice(*opt, 0, idx);
+            p->param = bstr_cut(p->arg, idx + 1);
+            p->arg = bstr_splice(p->arg, 0, idx);
         }
-    } else {
-        *old_syntax = true;
-        *opt = bstr_cut(*opt, 1);
     }
-    return true;
+
+    p->mp_opt = m_config_get_option(p->config, p->arg);
+    if (!p->mp_opt) {
+        // Automagic "no-" arguments: "--no-bla" turns into "--bla=no".
+        if (!bstr_startswith0(p->arg, "no-"))
+            return -1;
+
+        struct bstr s = bstr_cut(p->arg, 3);
+        p->mp_opt = m_config_get_option(p->config, s);
+        if (!p->mp_opt || p->mp_opt->type != &m_option_type_flag)
+            return -1;
+        // Avoid allowing "--no-no-bla".
+        if (bstr_startswith(bstr0(p->mp_opt->name), bstr0("no-")))
+            return -1;
+        // Flag options never have parameters.
+        old_syntax = false;
+        if (p->param.len)
+            return -2;
+        p->arg = s;
+        p->param = bstr0("no");
+    }
+
+    if (bstr_endswith0(p->arg, "-clr"))
+        old_syntax = false;
+
+    if (old_syntax && !(p->mp_opt->type->flags & M_OPT_TYPE_OLD_SYNTAX_NO_PARAM))
+    {
+        if (p->argc < 1)
+            return -3;
+        p->param = bstr0(p->argv[0]);
+        p->argc--;
+        p->argv++;
+    }
+
+    return 0;
 }
 
-static int map_to_option(struct m_config *config, bool old_syntax,
-                         const struct m_option **mp_opt,
-                         struct bstr *optname, struct bstr *param)
+// Returns true if more args, false if all parsed or an error occurred.
+static bool split_opt(struct parse_state *p)
 {
-    if (!mp_opt)
-        mp_opt = &(const struct m_option *){0};
-    *mp_opt = m_config_get_option(config, *optname);
-    if (*mp_opt)
-        return 0;
-    if (!bstr_startswith0(*optname, "no-"))
-        return -1;
-    struct bstr s = bstr_cut(*optname, 3);
-    *mp_opt = m_config_get_option(config, s);
-    if (!*mp_opt || (*mp_opt)->type != &m_option_type_flag)
-        return -1;
-    if (param->len)
-        return -2;
-    if (old_syntax)
-        return -3;
-    *optname = s;
-    *param = bstr0("no");
-    return 0;
+    int r = split_opt_silent(p);
+    if (r >= 0)
+        return r == 0;
+    p->error = true;
+    if (r == -2)
+        mp_tmsg(MSGT_CFGPARSER, MSGL_ERR,
+                "A no-* option can't take parameters: --%.*s=%.*s\n",
+                BSTR_P(p->arg), BSTR_P(p->param));
+    else if (r == -3)
+        mp_tmsg(MSGT_CFGPARSER, MSGL_ERR,
+                "Option %.*s needs a parameter.\n", BSTR_P(p->arg));
+    else
+        mp_tmsg(MSGT_CFGPARSER, MSGL_ERR,
+                "Unknown option on the command line: %.*s\n",
+                BSTR_P(p->arg));
+    return false;
+}
+
+static bool parse_flag(bstr name, bstr f)
+{
+    struct m_option opt = {NULL, NULL, CONF_TYPE_FLAG, 0, 0, 1, NULL};
+    int val = 0;
+    m_option_parse(&opt, name, f, &val);
+    return !!val;
 }
 
 bool m_config_parse_mp_command_line(m_config_t *config, struct playlist *files,
                                     int argc, char **argv)
 {
     int mode = 0;
-    bool no_more_opts = false;
     bool opt_exit = false;   // exit immediately after parsing (help options)
     struct playlist_entry *local_start = NULL;
-    struct bstr orig_opt;
     bool shuffle = false;
 
     int local_params_count = 0;
@@ -98,8 +160,6 @@ bool m_config_parse_mp_command_line(m_config_t *config, struct playlist *files,
 
     assert(config != NULL);
     assert(!config->file_local_mode);
-    assert(argv != NULL);
-    assert(argc >= 1);
 
     config->mode = M_COMMAND_LINE;
     mode = GLOBAL;
@@ -108,132 +168,110 @@ bool m_config_parse_mp_command_line(m_config_t *config, struct playlist *files,
         return true;
 #endif
 
-    for (int i = 1; i < argc; i++) {
-        //next:
-        struct bstr opt = bstr0(argv[i]);
-        orig_opt = opt;
-        /* check for -- (no more options id.) except --help! */
-        if (!bstrcmp0(opt, "--")) {
-            no_more_opts = true;
-            continue;
-        }
-        if (!bstrcmp0(opt, "--{")) {
-            if (mode != GLOBAL) {
-                mp_msg(MSGT_CFGPARSER, MSGL_ERR, "'--{' can not be nested\n");
+    struct parse_state p = {config, argc, argv};
+    while (split_opt(&p)) {
+        if (p.mp_opt) {
+            int r;
+            if (mode == GLOBAL && !(p.mp_opt->flags & M_OPT_PRE_PARSE)) {
+                r = m_config_set_option(config, p.arg, p.param);
+            } else {
+                r = m_config_check_option(config, p.arg, p.param);
+            }
+            if (r <= M_OPT_EXIT) {
+                opt_exit = true;
+                r = M_OPT_EXIT - r;
+            } else if (r < 0) {
+                char *msg = m_option_strerror(r);
+                if (!msg)
+                    goto print_err;
+                mp_tmsg(MSGT_CFGPARSER, MSGL_FATAL,
+                        "Error parsing commandline option %.*s: %s\n",
+                        BSTR_P(p.arg), msg);
                 goto err_out;
             }
-            mode = LOCAL;
-            // Needed for option checking.
-            m_config_enter_file_local(config);
-            assert(!local_start);
-            local_start = files->last;
-            continue;
-        }
 
-        if (!bstrcmp0(opt, "--}")) {
-            if (mode != LOCAL) {
-                mp_msg(MSGT_CFGPARSER, MSGL_ERR, "too many closing '--}'\n");
-                goto err_out;
+            // Handle some special arguments outside option parser.
+
+            if (!bstrcmp0(p.arg, "{")) {
+                if (mode != GLOBAL) {
+                    mp_msg(MSGT_CFGPARSER, MSGL_ERR,
+                           "'--{' can not be nested.\n");
+                    goto err_out;
+                }
+                mode = LOCAL;
+                // Needed for option checking.
+                m_config_enter_file_local(config);
+                assert(!local_start);
+                local_start = files->last;
+                continue;
             }
-            if (local_params_count) {
-                // The files added between '{' and '}' are the entries from the
-                // entry _after_ local_start, until the end of the list. If
-                // local_start is NULL, the list was empty on '{', and we want
-                // all files in the list.
-                struct playlist_entry *cur
-                    = local_start ? local_start->next : files->first;
-                if (!cur)
-                    mp_msg(MSGT_CFGPARSER, MSGL_WARN, "ignored options\n");
-                while (cur) {
-                    playlist_entry_add_params(cur, local_params,
-                                              local_params_count);
-                    cur = cur->next;
+
+            if (!bstrcmp0(p.arg, "}")) {
+                if (mode != LOCAL) {
+                    mp_msg(MSGT_CFGPARSER, MSGL_ERR,
+                           "Too many closing '--}'.\n");
+                    goto err_out;
+                }
+                if (local_params_count) {
+                    // The files added between '{' and '}' are the entries from
+                    // the entry _after_ local_start, until the end of the list.
+                    // If local_start is NULL, the list was empty on '{', and we
+                    // want all files in the list.
+                    struct playlist_entry *cur
+                        = local_start ? local_start->next : files->first;
+                    if (!cur)
+                        mp_msg(MSGT_CFGPARSER, MSGL_WARN, "Ignored options!\n");
+                    while (cur) {
+                        playlist_entry_add_params(cur, local_params,
+                                                local_params_count);
+                        cur = cur->next;
+                    }
                 }
+                local_params_count = 0;
+                mode = GLOBAL;
+                m_config_leave_file_local(config);
+                local_start = NULL;
+                shuffle = false;
+                continue;
             }
-            local_params_count = 0;
-            mode = GLOBAL;
-            m_config_leave_file_local(config);
-            local_start = NULL;
-            continue;
-        }
 
-        struct bstr param = bstr0(i+1 < argc ? argv[i+1] : NULL);
-        bool old_syntax;
-        if (!no_more_opts && split_opt(&opt, &param, &old_syntax)) {
-            const struct m_option *mp_opt;
-            int ok = map_to_option(config, old_syntax, &mp_opt, &opt, &param);
-            if (ok < 0) {
-                if (ok == -3)
-                    mp_tmsg(MSGT_CFGPARSER, MSGL_ERR,
-                            "Option --%.*s can't be used with single-dash "
-                            "syntax\n", BSTR_P(opt));
-                else if (ok == -2)
-                    mp_tmsg(MSGT_CFGPARSER, MSGL_ERR,
-                            "A --no-* option can't take parameters: "
-                            "--%.*s=%.*s\n", BSTR_P(opt), BSTR_P(param));
-                else
-                    mp_tmsg(MSGT_CFGPARSER, MSGL_ERR,
-                            "Unknown option on the command line: --%.*s\n",
-                            BSTR_P(opt));
-                goto print_err;
+            if (bstrcmp0(p.arg, "shuffle") == 0) {
+                shuffle = parse_flag(p.arg, p.param);
+                continue;
             }
-            // Handle some special arguments outside option parser.
-            // --loop when it applies to a group of files (per-file is option)
-            if (bstrcasecmp0(opt, "shuffle") == 0) {
-                shuffle = true;
-            } else if (bstrcasecmp0(opt, "no-shuffle") == 0) {
-                shuffle = false;
-            } else if (bstrcasecmp0(opt, "playlist") == 0) {
-                if (param.len <= 0)
-                    goto print_err;
+
+            if (bstrcmp0(p.arg, "playlist") == 0) {
                 // append the playlist to the local args
-                char *param0 = bstrdup0(NULL, param);
+                char *param0 = bstrdup0(NULL, p.param);
                 struct playlist *pl = playlist_parse_file(param0);
                 talloc_free(param0);
                 if (!pl)
                     goto print_err;
                 playlist_transfer_entries(files, pl);
                 talloc_free(pl);
-                i += old_syntax;
-            } else {
-                // "normal" options
-                int r;
-                if (mode == GLOBAL) {
-                    r = m_config_set_option(config, opt, param, old_syntax);
-                } else {
-                    r = m_config_check_option(config, opt, param, old_syntax);
-                    if (r >= 0) {
-                        if (r == 0)
-                            param = bstr0(NULL);  // for old_syntax case
-                        struct playlist_param p = {opt, param};
-                        MP_TARRAY_APPEND(NULL, local_params,
-                                         local_params_count, p);
-                    }
-                }
-                if (r <= M_OPT_EXIT) {
-                    opt_exit = true;
-                    r = M_OPT_EXIT - r;
-                } else if (r < 0) {
-                    char *msg = m_option_strerror(r);
-                    if (!msg)
-                        goto print_err;
-                    mp_tmsg(MSGT_CFGPARSER, MSGL_FATAL,
-                            "Error parsing commandline option \"%.*s\": %s\n",
-                            BSTR_P(orig_opt), msg);
-                    goto err_out;
-                }
-                if (old_syntax)
-                    i += r;
+                continue;
+            }
+
+            if (bstrcmp0(p.arg, "v") == 0) {
+                verbose++;
+                continue;
             }
-        } else {  /* filename */
-            int is_dvdnav = strstr(argv[i], "dvdnav://") != NULL;
-            mp_msg(MSGT_CFGPARSER, MSGL_DBG2, "Adding file %s\n", argv[i]);
+
+            if (mode == LOCAL) {
+                MP_TARRAY_APPEND(NULL, local_params, local_params_count,
+                                 (struct playlist_param) {p.arg, p.param});
+            }
+        } else {
+            // filename
+            bstr file = p.arg;
+            char *file0 = bstrdup0(NULL, p.arg);
+            int is_dvdnav = bstr_startswith0(file, "dvdnav://");
             // expand DVD filename entries like dvd://1-3 into component titles
-            if (strstr(argv[i], "dvd://") != NULL || is_dvdnav) {
+            if (bstr_startswith0(file, "dvd://") || is_dvdnav) {
                 int offset = is_dvdnav ? 9 : 6;
-                char *splitpos = strstr(argv[i] + offset, "-");
+                char *splitpos = strstr(file0 + offset, "-");
                 if (splitpos != NULL) {
-                    int start_title = strtol(argv[i] + offset, NULL, 10);
+                    int start_title = strtol(file0 + offset, NULL, 10);
                     int end_title;
                     //entries like dvd://-2 imply start at title 1
                     if (start_title < 0) {
@@ -252,20 +290,21 @@ bool m_config_parse_mp_command_line(m_config_t *config, struct playlist *files,
                         }
                     } else
                         mp_tmsg(MSGT_CFGPARSER, MSGL_ERR,
-                                "Invalid play entry %s\n", argv[i]);
+                                "Invalid play entry %s\n", file0);
 
                 } else // dvd:// or dvd://x entry
-                    playlist_add_file(files, argv[i]);
+                    playlist_add_file(files, file0);
             } else
-                playlist_add_file(files, argv[i]);
+                playlist_add_file(files, file0);
+            talloc_free(file0);
 
             // Lock stdin if it will be used as input
-            if (strcasecmp(argv[i], "-") == 0)
-                m_config_set_option0(config, "consolecontrols", "no", false);
+            if (bstrcmp0(file, "-") == 0)
+                m_config_set_option0(config, "consolecontrols", "no");
         }
     }
 
-    if (opt_exit)
+    if (p.error)
         goto err_out;
 
     if (mode != GLOBAL) {
@@ -274,6 +313,9 @@ bool m_config_parse_mp_command_line(m_config_t *config, struct playlist *files,
         goto err_out;
     }
 
+    if (opt_exit)
+        goto err_out;
+
     if (shuffle)
         playlist_shuffle(files);
 
@@ -283,8 +325,7 @@ bool m_config_parse_mp_command_line(m_config_t *config, struct playlist *files,
 
 print_err:
     mp_tmsg(MSGT_CFGPARSER, MSGL_FATAL,
-            "Error parsing option on the command line: %.*s\n",
-            BSTR_P(orig_opt));
+            "Error parsing option on the command line: %.*s\n", BSTR_P(p.arg));
 err_out:
     talloc_free(local_params);
     if (config->file_local_mode)
@@ -299,43 +340,21 @@ extern int mp_msg_levels[];
  * command line parsing), and --really-quiet suppresses messages printed
  * during normal options parsing.
  */
-int m_config_preparse_command_line(m_config_t *config, int argc, char **argv,
-                                   int *verbose)
+void m_config_preparse_command_line(m_config_t *config, int argc, char **argv)
 {
-    int ret = 0;
-
     // Hack to shut up parser error messages
     int msg_lvl_backup = mp_msg_levels[MSGT_CFGPARSER];
     mp_msg_levels[MSGT_CFGPARSER] = -11;
 
-    config->mode = M_COMMAND_LINE_PRE_PARSE;
-
-    for (int i = 1 ; i < argc ; i++) {
-        struct bstr opt = bstr0(argv[i]);
-        // No more options after --
-        if (!bstrcmp0(opt, "--"))
-            break;
-        struct bstr param = bstr0(i+1 < argc ? argv[i+1] : NULL);
-        bool old_syntax;
-        if (!split_opt(&opt, &param, &old_syntax))
-            continue;   // Ignore non-option arguments
-        // Ignore invalid options
-        if (map_to_option(config, old_syntax, NULL, &opt, &param) < 0)
-            continue;
-        // "-v" is handled here
-        if (!bstrcmp0(opt, "v")) {
-            (*verbose)++;
-            continue;
+    struct parse_state p = {config, argc, argv};
+    while (split_opt_silent(&p) == 0) {
+        if (p.mp_opt) {
+            // Ignore non-pre-parse options. They will be set later.
+            // Option parsing errors will be handled later as well.
+            if (p.mp_opt->flags & M_OPT_PRE_PARSE)
+                m_config_set_option(config, p.arg, p.param);
         }
-        // Set, non-pre-parse options will be ignored
-        int r = m_config_set_option(config, opt, param, old_syntax);
-        if (r < 0)
-            ret = r;
-        else if (old_syntax)
-            i += r;
     }
 
     mp_msg_levels[MSGT_CFGPARSER] = msg_lvl_backup;
-
-    return ret;
 }