incorporate most of Simon's patch changes

It's everything but the removal of global options and the possibility of
providing just the command atom instead of {command, opts}, + 2 bug fixes
in the patch code (returning `bad_argument' when there is an option with
no argument and reversing the arguments).

4 files changed, 74 insertions, 129 deletions

diff --git a/src/rabbit_control.erl b/src/rabbit_control.erl
index 83786cfa..2a84f7b2 100644
--- a/src/rabbit_control.erl
+++ b/src/rabbit_control.erl
@@ -26,7 +26,11 @@
 -define(NODE_OPT, "-n").
 -define(VHOST_OPT, "-p").
 
--define(GLOBAL_OPTS, [?QUIET_OPT, ?NODE_OPT]).
+-define(QUIET_DEF, {?QUIET_OPT, flag}).
+-define(NODE_DEF(Node), {?NODE_OPT, {option, Node}}).
+-define(VHOST_DEF, {?VHOST_OPT, {option, "/"}}).
+
+-define(GLOBAL_DEFS(Node), [?QUIET_DEF, ?NODE_DEF(Node)]).
 
 -define(COMMANDS,
         [stop,
@@ -51,29 +55,29 @@
          add_vhost,
          delete_vhost,
          list_vhosts,
-         {set_permissions, [?VHOST_OPT]},
-         {clear_permissions, [?VHOST_OPT]},
-         {list_permissions, [?VHOST_OPT]},
-         {list_user_permissions, [?VHOST_OPT]},
+         {set_permissions, [?VHOST_DEF]},
+         {clear_permissions, [?VHOST_DEF]},
+         {list_permissions, [?VHOST_DEF]},
+         {list_user_permissions, [?VHOST_DEF]},
 
          set_parameter,
          clear_parameter,
          list_parameters,
 
-         {list_queues, [?VHOST_OPT]},
-         {list_exchanges, [?VHOST_OPT]},
-         {list_bindings, [?VHOST_OPT]},
-         {list_connections, [?VHOST_OPT]},
+         {list_queues, [?VHOST_DEF]},
+         {list_exchanges, [?VHOST_DEF]},
+         {list_bindings, [?VHOST_DEF]},
+         {list_connections, [?VHOST_DEF]},
          list_channels,
-         {list_consumers, [?VHOST_OPT]},
+         {list_consumers, [?VHOST_DEF]},
          status,
          environment,
          report,
          eval,
 
          close_connection,
-         {trace_on, [?VHOST_OPT]},
-         {trace_off, [?VHOST_OPT]},
+         {trace_on, [?VHOST_DEF]},
+         {trace_off, [?VHOST_DEF]},
          set_vm_memory_high_watermark
         ]).
 
@@ -109,11 +113,7 @@
 start() ->
     {ok, [[NodeStr|_]|_]} = init:get_argument(nodename),
     {Command, Opts, Args} =
-        case rabbit_misc:parse_arguments(?COMMANDS,
-                                         ?GLOBAL_OPTS,
-                                         [{?QUIET_OPT, flag},
-                                          {?NODE_OPT, {option, NodeStr}},
-                                          {?VHOST_OPT, {option, "/"}}],
+        case rabbit_misc:parse_arguments(?COMMANDS, ?GLOBAL_DEFS(NodeStr),
                                          init:get_plain_arguments())
         of
             {ok, Res}  -> Res;
diff --git a/src/rabbit_misc.erl b/src/rabbit_misc.erl
index 5c2928fb..661a4cb4 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([parse_arguments/4]).
+-export([parse_arguments/3]).
 -export([all_module_attributes/1, build_acyclic_graph/3]).
 -export([now_ms/0]).
 -export([const_ok/0, const/1]).
@@ -182,9 +182,8 @@
 -spec(gb_trees_fold/3 :: (fun ((any(), any(), A) -> A), A, gb_tree()) -> A).
 -spec(gb_trees_foreach/2 ::
         (fun ((any(), any()) -> any()), gb_tree()) -> 'ok').
--spec(parse_arguments/4 ::
-        ([{atom(), string()} | atom()],
-         [string()],
+-spec(parse_arguments/3 ::
+        ([{atom(), [{string(), optdef()}]} | atom()],
          [{string(), optdef()}],
          [string()])
         -> {'ok', {atom(), [{string(), string()}], [string()]}} |
@@ -742,88 +741,61 @@ gb_trees_foreach(Fun, Tree) ->
     gb_trees_fold(fun (Key, Val, Acc) -> Fun(Key, Val), Acc end, ok, Tree).
 
 %% Takes:
-%%    * A list of [{atom(), [string()]} | atom()], where the atom()s are
-%%      the accepted commands and the optional [string()] is the list of
+%%    * A list of [{atom(), [{string(), optdef()]} | atom()], where the atom()s
+%%      are the accepted commands and the optional [string()] is the list of
 %%      accepted options for that command
-%%    * A list [string()] of options valid for all commands
-%%    * A list [{string(), optdef()}] to determine what is a flag and what is
-%%      an option
+%%    * A list [{string(), optdef()}] of options valid for all commands
 %%    * 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 no_command if no command could be parsed.
-parse_arguments(CommandsOpts0, GlobalOpts, Defs, As0) ->
-    CommandsOpts = lists:map(fun ({C, Fs}) -> {atom_to_list(C), Fs};
-                                 (C)       -> {atom_to_list(C), []}
-                             end, CommandsOpts0),
-    DefsDict = dict:from_list(Defs),
-    lists:foldl(fun ({C, Os}, no_command) ->
-                        process_opts(C, DefsDict, GlobalOpts ++ Os, As0);
-                    (_, {ok, Res}) ->
-                        {ok, Res}
-                end, no_command, CommandsOpts).
-
-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,
-    KVs1 = dict:map(fun (_, flag)        -> false;
+parse_arguments(Commands, GlobalDefs, As) ->
+    lists:foldl(maybe_process_opts(GlobalDefs, As), no_command, Commands).
+
+maybe_process_opts(GDefs, As) ->
+    fun({C, Os}, no_command) ->
+            process_opts(atom_to_list(C), dict:from_list(GDefs ++ Os), As);
+       (C, no_command) ->
+            (maybe_process_opts(GDefs, As))({C, []}, no_command);
+       (_, {ok, Res}) ->
+            {ok, Res}
+    end.
+
+process_opts(C, Defs, As0) ->
+    KVs0 = dict:map(fun (_, flag)        -> false;
                         (_, {option, V}) -> V
                     end, Defs),
-    case process_opts1(C, Defs, KVs1, As) of
-        no_command        -> no_command;
-        {ok, {KVs2, As1}} -> {ok, {list_to_atom(C), dict:to_list(KVs2), As1}}
-    end.
+    process_opts(Defs, C, As0, not_found, KVs0, []).
 
 %% 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(_, _, _, []) ->
+process_opts(_Defs, C, [], found, KVs, Outs) ->
+    {ok, {list_to_atom(C), dict:to_list(KVs), lists:reverse(Outs)}};
+process_opts(_Defs, _C, [], not_found, _, _) ->
     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
+process_opts(Defs, C, [A | As], Found, KVs, Outs) ->
+    OptType = case dict:find(A, Defs) of
+                  error             -> none;
+                  {ok, flag}        -> flag;
+                  {ok, {option, _}} -> option
+              end,
+    case {OptType, C, Found} of
+        {flag, _, _}     -> process_opts(
+                              Defs, C, As, Found, dict:store(A, true, KVs),
+                              Outs);
+        {option, _, _}   -> case As of
+                                []        -> process_opts(Defs, C, [], Found,
+                                                          KVs, [A | Outs]);
+                                [V | As1] -> process_opts(
+                                               Defs, C, As1, Found,
+                                               dict:store(A, V, KVs), Outs)
+                            end;
+        {none, A, _}     -> process_opts(Defs, C, As, found, KVs, Outs);
+        {none, _, found} -> process_opts(Defs, C, As, found, KVs, [A | Outs]);
+        {none, _, _}     -> no_command
     end.
 
-%% 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 13c68429..30c7bb37 100644
--- a/src/rabbit_plugins.erl
+++ b/src/rabbit_plugins.erl
@@ -25,10 +25,15 @@
 -define(ENABLED_OPT, "-E").
 -define(ENABLED_ALL_OPT, "-e").
 
--define(GLOBAL_OPTS, []).
+-define(VERBOSE_DEF, {?VERBOSE_OPT, flag}).
+-define(MINIMAL_DEF, {?MINIMAL_OPT, flag}).
+-define(ENABLED_DEF, {?ENABLED_OPT, flag}).
+-define(ENABLED_ALL_DEF, {?ENABLED_ALL_OPT, flag}).
+
+-define(GLOBAL_DEFS, []).
 
 -define(COMMANDS,
-        [{list, [?VERBOSE_OPT, ?MINIMAL_OPT, ?ENABLED_OPT, ?ENABLED_ALL_OPT]},
+        [{list, [?VERBOSE_DEF, ?MINIMAL_DEF, ?ENABLED_DEF, ?ENABLED_ALL_DEF]},
          enable,
          disable]).
 
@@ -53,12 +58,7 @@ start() ->
         init:get_argument(enabled_plugins_file),
     {ok, [[PluginsDir|_]|_]} = init:get_argument(plugins_dist_dir),
     {Command, Opts, Args} =
-        case rabbit_misc:parse_arguments(?COMMANDS,
-                                         ?GLOBAL_OPTS,
-                                         [{?VERBOSE_OPT, flag},
-                                          {?MINIMAL_OPT, flag},
-                                          {?ENABLED_OPT, flag},
-                                          {?ENABLED_ALL_OPT, flag}],
+        case rabbit_misc:parse_arguments(?COMMANDS, ?GLOBAL_DEFS,
                                          init:get_plain_arguments())
         of
             {ok, Res}  -> Res;
diff --git a/src/rabbit_tests.erl b/src/rabbit_tests.erl
index 7383d6bf..7c287b0a 100644
--- a/src/rabbit_tests.erl
+++ b/src/rabbit_tests.erl
@@ -802,16 +802,12 @@ test_log_management_during_startup() ->
     passed.
 
 test_arguments_parser() ->
-    Defs1 = [{"-o1", {option, "foo"}},
-             {"-o2", {option, "bar"}},
-             {"-f1", flag},
-             {"-f2", flag}],
-    GlobalOpts1 = ["-f1", "-o1"],
-    Commands1 = [command1, {command2, ["-f2", "-o2"]}],
+    GlobalOpts1 = [{"-f1", flag}, {"-o1", {option, "foo"}}],
+    Commands1 = [command1, {command2, [{"-f2", flag}, {"-o2", {option, "bar"}}]}],
 
     GetOptions =
         fun (Args) ->
-                rabbit_misc:parse_arguments(Commands1, GlobalOpts1, Defs1, Args)
+                rabbit_misc:parse_arguments(Commands1, GlobalOpts1, Args)
         end,
 
     check_parse_arguments(no_command, GetOptions, []),
@@ -861,29 +857,6 @@ test_arguments_parser() ->
                        {"-o1", "baz"}, {"-o2", "bar"}], ["quux", "foo"]}},
       GetOptions, ["-f2", "command2", "quux", "-o1", "baz", "foo"]),
 
-    %% If we pass non-defined flags, we get an error
-    Defs2 = [{"-o1", {option, "foo"}}, {"-f1", flag}],
-    Commands2 = [command1, {command2, ["-f1", "-bogus"]}],
-
-    CheckError = fun (Fun) ->
-                         case catch Fun() of
-                             ok ->
-                                 exit({got_success_but_expected_failure,
-                                       parse_arguments_undefined_option});
-                             {error, undefined_option} ->
-                                 ok
-                         end
-                 end,
-
-    CheckError(fun () ->
-                       rabbit_misc:parse_arguments(
-                         Commands2, ["-quux"], Defs2, ["command1"])
-               end),
-    CheckError(fun () ->
-                       rabbit_misc:parse_arguments(
-                         Commands2, ["-o1"], Defs2, ["command2"])
-               end),
-
     passed.
 
 test_cluster_management() ->
@@ -1668,8 +1641,8 @@ expand_options(As, Bs) ->
 
 check_parse_arguments(ExpRes, Fun, As) ->
     SortRes =
-        fun (no_command)         -> no_command;
-            ({ok, {C, KVs, As}}) -> {ok, {C, lists:sort(KVs), lists:sort(As)}}
+        fun (no_command)          -> no_command;
+            ({ok, {C, KVs, As1}}) -> {ok, {C, lists:sort(KVs), As1}}
         end,
 
     true = SortRes(ExpRes) =:= SortRes(Fun(As)).