simplify `rabbit_misc:get_options/4'

Incidentally, it is also much more efficient (not that it matters).

I will probably rename it to something like `parse_arguments'.

`CommandOpts' and `GlobalOpts' could be one, but I think that `GlobalOpts'
is really useful since adding the global options to each single command
is a pain. We could, obviously, put them together in a tuple, but then
it's not that different from two separate arguments.

4 files changed, 84 insertions, 98 deletions

diff --git a/src/rabbit_control.erl b/src/rabbit_control.erl
index 554a31f9..3352027c 100644
--- a/src/rabbit_control.erl
+++ b/src/rabbit_control.erl
@@ -36,18 +36,18 @@
          reset,
          force_reset,
          rotate_logs,
-         
+
          cluster,
          force_cluster,
          cluster_status,
-         
+
          add_user,
          delete_user,
          change_password,
          clear_password,
          set_user_tags,
          list_users,
-         
+
          add_vhost,
          delete_vhost,
          list_vhosts,
@@ -55,11 +55,11 @@
          {clear_permissions, [?VHOST_OPT]},
          {list_permissions, [?VHOST_OPT]},
          {list_user_permissions, [?VHOST_OPT]},
-         
+
          set_parameter,
          clear_parameter,
          list_parameters,
-         
+
          {list_queues, [?VHOST_OPT]},
          {list_exchanges, [?VHOST_OPT]},
          {list_bindings, [?VHOST_OPT]},
@@ -70,7 +70,7 @@
          environment,
          report,
          eval,
-         
+
          close_connection,
          {trace_on, [?VHOST_OPT]},
          {trace_off, [?VHOST_OPT]},
@@ -116,9 +116,9 @@ start() ->
                                       {?VHOST_OPT, {option, "/"}}],
                                      init:get_plain_arguments())
         of
-            {ok, Res}      -> Res;
-            {invalid, Err} -> rabbit_misc:handle_invalid_arguments(Err),
-                              usage()
+            {ok, Res}  -> Res;
+            no_command -> print_error("could not recognise command", []),
+                          usage()
         end,
     Opts1 = [case K of
                  ?NODE_OPT -> {?NODE_OPT, rabbit_nodes:make(V)};
@@ -137,7 +137,7 @@ start() ->
                 print_error("invalid command '~s'",
                             [string:join([atom_to_list(Command) | Args], " ")])
         end,
-    
+
     %% The reason we don't use a try/catch here is that rpc:call turns
     %% thrown errors into normal return values
     case catch action(Command, Node, Args, Opts, Inform) of
diff --git a/src/rabbit_misc.erl b/src/rabbit_misc.erl
index 0b39e539..d3a567b0 100644
--- a/src/rabbit_misc.erl
+++ b/src/rabbit_misc.erl
@@ -49,7 +49,7 @@
 -export([version_compare/2, version_compare/3]).
 -export([dict_cons/3, orddict_cons/3, gb_trees_cons/3]).
 -export([gb_trees_fold/3, gb_trees_foreach/2]).
--export([get_options/4, handle_invalid_arguments/1]).
+-export([get_options/4]).
 -export([all_module_attributes/1, build_acyclic_graph/3]).
 -export([now_ms/0]).
 -export([const_ok/0, const/1]).
@@ -67,13 +67,12 @@
 
 -ifdef(use_specs).
 
--export_type([resource_name/0, thunk/1, invalid_arguments/0]).
+-export_type([resource_name/0, thunk/1]).
 
 -type(ok_or_error() :: rabbit_types:ok_or_error(any())).
 -type(thunk(T) :: fun(() -> T)).
 -type(resource_name() :: binary()).
 -type(optdef() :: flag | {option, string()}).
--type(invalid_arguments() :: 'command_not_found').
 -type(channel_or_connection_exit()
       :: rabbit_types:channel_exit() | rabbit_types:connection_exit()).
 -type(digraph_label() :: term()).
@@ -189,8 +188,7 @@
          [{string(), optdef()}],
          [string()])
         -> {'ok', {atom(), [{string(), string()}], [string()]}} |
-           {'invalid', invalid_arguments()}).
--spec(handle_invalid_arguments/1 :: (invalid_arguments()) -> 'ok').
+           'no_command').
 -spec(all_module_attributes/1 :: (atom()) -> [{atom(), [term()]}]).
 -spec(build_acyclic_graph/3 ::
         (graph_vertex_fun(), graph_edge_fun(), [{atom(), [term()]}])
@@ -751,92 +749,77 @@ gb_trees_foreach(Fun, Tree) ->
 %%    * A list [{string(), optdef()}] to determine what is a flag and what is
 %%      an option
 %%    * The list of arguments given by the user
-%% 
+%%
 %% Returns either {ok, {atom(), [{string(), string()}], [string()]} which are
 %% respectively the command, the key-value pairs of the options and the leftover
-%% arguments; or {invalid, Reason} if something went wrong.
-get_options(CommandsOpts0, GlobalOpts, Defs, As0) ->
+%% arguments; or no_command if no command could be parsed.
+get_options(CommandsOpts, GlobalOpts, Defs, As0) ->
     DefsDict = dict:from_list(Defs),
-    CommandsOpts = lists:map(fun ({C, Os}) -> {atom_to_list(C), Os};
-                                 (C)       -> {atom_to_list(C), []}
-                             end, CommandsOpts0),
-    %% For each command, drop all the options/flag that are available in that
-    %% command and see what remains
-    case lists:foldl(fun ({C, Os}, not_found) ->
-                             case drop_opts(DefsDict, GlobalOpts ++ Os, As0) of
-                                 [C | _] -> {found, {C, Os}};
-                                 _       -> not_found
-                             end;
-                         (_, {found, {C, Os}}) ->
-                             {found, {C, Os}}
-                     end, not_found, CommandsOpts)
-    of
-        not_found ->
-            {invalid, command_not_found};
-        {found, {C, Os}} ->
-            {KVs, Arguments} = get_options(sets:from_list(GlobalOpts ++ Os),
-                                           Defs, As0 -- [C]),
-            {ok, {list_to_atom(C), KVs, Arguments}}
-    end.
+    lists:foldl(fun ({C, Os}, no_command) ->
+                        process_opts(C, DefsDict, GlobalOpts ++ Os, As0);
+                    (_, {ok, Res}) ->
+                        {ok, Res}
+                end, no_command, CommandsOpts).
 
-drop_opts(Defs0, Opts0, As) ->
+process_opts(C, Defs0, Opts0, As) ->
     Opts = sets:from_list(Opts0),
     Defs = dict:filter(fun (K, _) -> sets:is_element(K, Opts) end, Defs0),
+    %% Opts0 must be a subset of the keys of Defs0 - we want to make sure that
+    %% all the options are defined.
     case sets:size(Opts) =:= dict:size(Defs) of
         true  -> ok;
         false -> throw({error, undefined_option})
     end,
-    drop_opts(Defs, As).
-
-drop_opts(_, [])          -> [];
-drop_opts(Opts, [A | As]) ->
-    case dict:find(A, Opts) of
-        {ok, flag}        -> drop_opts(Opts, As);
-        {ok, {option, _}} -> case As of
-                                 []        -> [];
-                                 [_ | As1] -> drop_opts(Opts, As1)
-                             end;
-        error             -> [A | As]
+    KVs1 = dict:map(fun (_, flag)        -> false;
+                        (_, {option, V}) -> V
+                    end, Defs),
+    case process_opts1(atom_to_list(C), Defs, KVs1, As) of
+        no_command        -> no_command;
+        {ok, {KVs2, As1}} -> {ok, {list_to_atom(C), dict:to_list(KVs2), As1}}
+    end.
+
+%% Consume flags/options until you find the correct command. If there are no
+%% arguments or the first argument is not the command we're expecting, fail.
+process_opts1(_, _, _, []) ->
+    no_command;
+process_opts1(C, Defs, KVs1, [A | As]) ->
+    case case dict:find(A, Defs) of
+             {ok, flag} ->
+                 {opt, {dict:store(A, true, KVs1), As}};
+             {ok, {option, _}} ->
+                 case As of
+                     []        -> bad_argument;
+                     [V | As1] -> {opt, {dict:store(A, V, KVs1), As1}}
+                 end;
+             error when A =:= C ->
+                 {ok, {KVs1, As}};
+             error ->
+                 no_command
+         end
+    of
+        {opt, {KVs2, As2}} -> process_opts1(C, Defs, KVs2, As2);
+        {ok, {KVs2, As2}}  -> {ok, process_opts1(Defs, KVs2, As2)};
+        no_command         -> no_command
     end.
 
-get_options(Opts, Defs, As) ->
-    %% Check that each flag is OK with this command, and get its value
-    lists:foldl(fun (Def, {Accum, As1}) ->
-                        K = case Def of
-                                {K0, flag}        -> K0;
-                                {K0, {option, _}} -> K0
-                            end,
-                        case sets:is_element(K, Opts) of
-                            false -> {Accum, As1};
-                            true  -> {As2, V} = get_def(Def, As1),
-                                     {[{K, V} | Accum], As2}
-                        end
-                end, {[], As}, Defs).
-    
-get_def({Flag, flag}, As)             -> get_flag(Flag, As);
-get_def({Opt, {option, Default}}, As) -> get_option(Opt, Default, As).
-
-get_option(K, _Default, [K, V | As]) ->
-    {As, V};
-get_option(K, Default, [Nk | As]) ->
-    {As1, V} = get_option(K, Default, As),
-    {[Nk | As1], V};
-get_option(_, Default, As) ->
-    {As, Default}.
-
-get_flag(K, [K | As]) ->
-    {As, true};
-get_flag(K, [Nk | As]) ->
-    {As1, V} = get_flag(K, As),
-    {[Nk | As1], V};
-get_flag(_, []) ->
-    {[], false}.
-
-handle_invalid_arguments(command_not_found) ->
-    rabbit_misc:format_stderr("could not recognise command~n", []);
-handle_invalid_arguments({invalid_function, Command, Opt}) ->
-    rabbit_misc:format_stderr("invalid option '~s' for command '~s'~n",
-                              [Opt, Command]).
+%% Finish consuming the flags/options.
+process_opts1(_, KVs, []) ->
+    {KVs, []};
+process_opts1(Defs, KVs1, [A | As]) ->
+    {KVs2, AsL, AsR1} =
+        case dict:find(A, Defs) of
+            {ok, flag} ->
+                {dict:store(A, true, KVs1), [], As};
+            {ok, {option, _}} ->
+                case As of
+                    []        -> {KVs1, [A], []};
+                    [V | As1] -> {dict:store(A, V, KVs1), [], As1}
+                end;
+            error ->
+                {KVs1, [A], As}
+        end,
+    {KVs3, AsR2} = process_opts1(Defs, KVs2, AsR1),
+    {KVs3, AsL ++ AsR2}.
 
 now_ms() ->
     timer:now_diff(now(), {0,0,0}) div 1000.
diff --git a/src/rabbit_plugins.erl b/src/rabbit_plugins.erl
index 53639dc1..c05cee72 100644
--- a/src/rabbit_plugins.erl
+++ b/src/rabbit_plugins.erl
@@ -61,9 +61,9 @@ start() ->
                                       {?ENABLED_ALL_OPT, flag}],
                                      init:get_plain_arguments())
         of
-            {ok, Res}      -> Res;
-            {invalid, Err} -> rabbit_misc:handle_invalid_arguments(Err),
-                              usage()
+            {ok, Res}  -> Res;
+            no_command -> print_error("could not recognise command", []),
+                          usage()
         end,
 
     PrintInvalidCommandError =
@@ -71,7 +71,7 @@ start() ->
                 print_error("invalid command '~s'",
                             [string:join([atom_to_list(Command) | Args], " ")])
         end,
-                    
+
     case catch action(Command, Args, Opts, PluginsFile, PluginsDir) of
         ok ->
             rabbit_misc:quit(0);
@@ -407,4 +407,3 @@ report_change() ->
         _ ->
              ok
     end.
-
diff --git a/src/rabbit_tests.erl b/src/rabbit_tests.erl
index 23176267..24d55e53 100644
--- a/src/rabbit_tests.erl
+++ b/src/rabbit_tests.erl
@@ -814,8 +814,8 @@ test_option_parser() ->
                 rabbit_misc:get_options(Commands1, GlobalOpts1, Defs1, Args)
         end,
 
-    check_get_options({invalid, command_not_found}, GetOptions, []),
-    check_get_options({invalid, command_not_found}, GetOptions, ["foo", "bar"]),
+    check_get_options(no_command, GetOptions, []),
+    check_get_options(no_command, GetOptions, ["foo", "bar"]),
     check_get_options({ok, {command1, [{"-f1", false}, {"-o1", "foo"}], []}},
                       GetOptions, ["command1"]),
     check_get_options({ok, {command1, [{"-f1", false}, {"-o1", "blah"}], []}},
@@ -830,8 +830,12 @@ test_option_parser() ->
     check_get_options(
       {ok, {command1, [{"-f1", true}, {"-o1", "blah"}], ["quux", "baz"]}},
       GetOptions, ["command1", "quux", "-f1", "-o1", "blah", "baz"]),
+    %% For duplicate flags, the last one counts
+    check_get_options(
+      {ok, {command1, [{"-f1", false}, {"-o1", "second"}], []}},
+      GetOptions, ["-o1", "first", "command1", "-o1", "second"]),
     %% If the flag "eats" the command, the command won't be recognised
-    check_get_options({invalid, command_not_found}, GetOptions,
+    check_get_options(no_command, GetOptions,
                       ["-o1", "command1", "quux"]),
     %% If the flag doesn't have an argument, it won't be recognised
     check_get_options(
@@ -1661,7 +1665,7 @@ expand_options(As, Bs) ->
 
 check_get_options(ExpRes, Fun, As) ->
     SortRes =
-        fun ({invalid, Reason})  -> {invalid, Reason};
+        fun (no_command)         -> no_command;
             ({ok, {C, KVs, As}}) -> {ok, {C, lists:sort(KVs), lists:sort(As)}}
         end,