From 3051e66e0dd720edd3ba08100819320439cdb407 Mon Sep 17 00:00:00 2001
From: Fred Hebert <mononcqc@ferd.ca>
Date: Mon, 31 Jul 2023 14:55:37 +0000
Subject: [PATCH] Rework argument parsing in do/as providers

The current mechanism had a few oddities, as reported in
https://github.com/erlang/rebar3/issues/2811:

- `rebar3 do a,b` runs `a` then `b`
- `rebar3 as profile task --a=b,c` runs the task with the flag `a` set
  to `b,c`
- `rebar3 as profile task -a b,c` runs the task with the flag `a` set to
  `b`, and then tries to run `c` as a task
- `rebar3 as profile task -a b, c` runs the task with the flag `a` set to
  `b`, and then tries to run `c` as a task

So the issue is that to pass a comma to an argument, we can only do so
when it exists as a long form argument (`--flag`) and that it does not
contain spaces.

This patch attempts to correct things by making the white-space
following the comma significant. If it's missing, it gets grouped as a
single entity. If it's there, the task is considered to be split.

This becomes significant for commands that internally parse the `,` to
allow list of args (such as `rebar3 release --relnames one,two`) which
were only invokable as `relnames=one,two`, and not the spaced version
nor the short version (`m one,two`), which should now be supported.

This should *not* break any backwards compatibility, but there is the
possibility that users who did invoke many commands with both arguments
and sequences without spaces (`rebar3 do release --relname a,tar`) now
get broken commands.

I don't quite know if we'd be okay taking that risk, so this is up for
comments I guess.
---
 apps/rebar/src/rebar_utils.erl        | 34 ++++++++++++++++++++++++---
 apps/rebar/test/rebar_utils_SUITE.erl | 16 +++++++++++++
 2 files changed, 47 insertions(+), 3 deletions(-)

diff --git a/apps/rebar/src/rebar_utils.erl b/apps/rebar/src/rebar_utils.erl
index ab553de33..95efac967 100644
--- a/apps/rebar/src/rebar_utils.erl
+++ b/apps/rebar/src/rebar_utils.erl
@@ -891,9 +891,20 @@ arg_or_flag(["," ++ Task|Rest], Acc) -> new_task([Task|Rest], Acc);
 %% a flag
 arg_or_flag(["-" ++ _ = Flag|Rest], [{Task, Args}|Acc]) ->
     case maybe_ends_in_comma(Flag) of
-        false   -> arg_or_flag(Rest, [{Task, [Flag|Args]}|Acc]);
-        NewFlag -> new_task(Rest, [{Task,
-                                    lists:reverse([NewFlag|Args])}|Acc])
+        false   ->
+            case rest_arg_self_contains_comma(Rest) of
+                false ->
+                    arg_or_flag(Rest, [{Task, [Flag|Args]}|Acc]);
+                {ExtraArg,NewRest} ->
+                    case maybe_ends_in_comma(ExtraArg) of
+                        false ->
+                            arg_or_flag(NewRest, [{Task, [ExtraArg,Flag|Args]}|Acc]);
+                        TrimmedArg ->
+                            new_task(NewRest, [{Task, [Flag,TrimmedArg|Args]}|Acc])
+                    end
+            end;
+        NewFlag ->
+            new_task(Rest, [{Task, lists:reverse([NewFlag|Args])}|Acc])
     end;
 %% an argument or a sequence of arguments
 arg_or_flag([ArgList|Rest], [{Task, Args}|Acc]) ->
@@ -914,6 +925,23 @@ maybe_ends_in_comma(H) ->
         _           -> false
     end.
 
+%% looks whether a sequence of arguments comes with a comma inside of it.
+%% For example `-a a,b' would match here, but `-a x' or `-a x,' wouldn't.
+%% This is used by the caller to know whether to break up a task or assume
+%% the comma is part of the argument itself.
+rest_arg_self_contains_comma([]) -> false;
+rest_arg_self_contains_comma([ArgList|Rest]) ->
+    case re:split(ArgList, ",", [{return, list}, {parts, 2}, unicode]) of
+        [_Arg, ""] ->
+            false;
+        [_Arg, _More] ->
+            {ArgList, Rest};
+        [_Arg] ->
+            false
+    end.
+
+
+
 get_http_vars(Scheme) ->
     OS = case os:getenv(atom_to_list(Scheme)) of
         Str when is_list(Str) -> Str;
diff --git a/apps/rebar/test/rebar_utils_SUITE.erl b/apps/rebar/test/rebar_utils_SUITE.erl
index 233fcff3e..52cbc6897 100644
--- a/apps/rebar/test/rebar_utils_SUITE.erl
+++ b/apps/rebar/test/rebar_utils_SUITE.erl
@@ -24,6 +24,7 @@
          task_with_flag_with_commas/1,
          task_with_multiple_flags/1,
          special_task_do/1,
+         special_task_as_with_commas/1,
          valid_otp_version/1,
          valid_old_format_otp_version/1,
          valid_otp_version_equal/1,
@@ -70,6 +71,7 @@ groups() ->
                           task_with_flag_with_commas,
                           task_with_multiple_flags,
                           special_task_do,
+                          special_task_as_with_commas,
                           valid_otp_version,
                           valid_old_format_otp_version,
                           valid_otp_version_equal,
@@ -145,6 +147,20 @@ special_task_do(_Config) ->
                                                                         "do",
                                                                         "bar,",
                                                                         "baz"]).
+special_task_as_with_commas(_Config) ->
+    [{"as", ["profile"]}, {"bar", ["--x=y,z"]}, {"baz", ["--arg=a,b"]}] =
+      rebar_utils:args_to_tasks(["as", "profile,",
+                                 "bar", "--x=y,z,",
+                                 "baz", "--arg=a,b"]),
+    [{"as", ["profile"]}, {"bar", ["-x", "y,z"]}, {"baz", ["--a", "a,b"]}] =
+      rebar_utils:args_to_tasks(["as", "profile,",
+                                 "bar", "-x", "y,z,",
+                                 "baz", "--a", "a,b"]),
+    [{"as", ["profile"]}, {"bar", ["-x", "y"]}, {"z", []}, {"baz", ["--a", "a"]}, {"b",[]}] =
+      rebar_utils:args_to_tasks(["as", "profile,",
+                                 "bar", "-x", "y,", "z,",
+                                 "baz", "--a", "a,", "b"]),
+    ok.
 
 valid_otp_version(_Config) ->
     meck:new(rebar_utils, [passthrough]),